You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/17 08:01:20 UTC
[GitHub] [pulsar] lhotari opened a new pull request #11681: [Broker] Support disabling non-TLS service ports
lhotari opened a new pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681
Fixes #11548
### Motivation
Disabling non-TLS service ports isn't possible at the moment. Issue #11548 has been reported and contains instructions to reproduce the issue.
### Modifications
- Make changes to supports disabling non-TLS service ports in `broker.conf`/`standalone.conf` by providing empty values:
```
brokerServicePort=
webServicePort=
```
- there were a few locations where the Optional port value was not checked for existence.
- embedded Function Worker Service didn't work with TLS only broker. Fix issues in configuring Function Worker Service.
- add support for MockedPulsarServiceBaseTest tests to disable non-TLS ports.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#discussion_r690183362
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -1122,7 +1122,7 @@ private String getBrokerAddress() {
return String.format("%s:%s", pulsar.getAdvertisedAddress(),
pulsar.getConfiguration().getWebServicePort().isPresent()
? pulsar.getConfiguration().getWebServicePort().get()
- : pulsar.getConfiguration().getWebServicePortTls());
+ : pulsar.getConfiguration().getWebServicePortTls().get());
Review comment:
The same pattern is used elsewhere. There is a check in Broker startup that if webServicePort is Optional.empty, then webServicePortTls must be set. It should be fine to have the code as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900821479
> Thanks for your contribution. Do we need to update docs here https://pulsar.apache.org/docs/en/next/reference-configuration/#broker?
@Anonymitaet Yes, it would be useful to add docs that it's possible to disable the non-TLS ports `brokerServicePort` and `webServicePort` by providing an empty value. In this case it's necessary to specify `brokerClientTlsEnabled=true` and either set `tlsAllowInsecureConnection=true` or configure `brokerClientTlsEnabledWithKeyStore=true` and the related settings `brokerClientTlsTrustStore` and `brokerClientTlsTrustStorePassword`. I'll add this to the docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900151810
without making any changes, disabling non-TLS service ports works for Pulsar Stanadlone when Function Worker is disabled and when `loadManagerClassName=org.apache.pulsar.broker.loadbalance.impl.ModularLoadManagerImpl` setting is used.
I verified by appending these lines to conf/standalone.conf
```
brokerServicePort=
brokerServicePortTls=6651
webServicePort=
webServicePortTls=8443
brokerClientTlsEnabled=true
tlsEnabled=true
tlsAllowInsecureConnection=true
tlsCertificateFilePath=pulsar-broker/src/test/resources/authentication/tls-http/broker.cert.pem
tlsKeyFilePath=pulsar-broker/src/test/resources/authentication/tls-http/broker.key-pk8.pem
tlsTrustCertsFilePath=pulsar-broker/src/test/resources/authentication/tls-http/ca.cert.pem
loadManagerClassName=org.apache.pulsar.broker.loadbalance.impl.ModularLoadManagerImpl
```
and starting with `./bin/pulsar standalone --no-functions-worker --no-stream-storage`
@sijie This is the reason why this fix contains some changes to Functions worker configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900740090
Thanks for your contribution. Do we need to update docs here https://pulsar.apache.org/docs/en/next/reference-configuration/#broker?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900824889
@Anonymitaet I added docs to `security-tls-keystore.md` file. Please review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900740090
Thanks for your contribution. Do we need to update docs here https://pulsar.apache.org/docs/en/next/reference-configuration/#broker?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari edited a comment on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900130782
> I am not sure how does your PR solve #11548. From what I can see, you are basically changing the function worker service and tests. There is nothing related to brokers. Can you clarify how does your PR fix #11548?
@sijie This is the first issue to hit with "pulsar standalone":
```
12:11:04.314 [main] ERROR org.apache.pulsar.broker.PulsarService - Failed to start Pulsar service: No value present
java.util.NoSuchElementException: No value present
at java.util.Optional.get(Optional.java:148) ~[?:?]
at org.apache.pulsar.broker.loadbalance.NoopLoadManager.start(NoopLoadManager.java:55) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.startLoadManagementService(PulsarService.java:1047) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:751) [pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandalone.start(PulsarStandalone.java:296) [pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandaloneStarter.main(PulsarStandaloneStarter.java:131) [pulsar-broker.jar:2.9.0-SNAPSHOT]
12:11:04.314 [main] ERROR org.apache.pulsar.PulsarStandaloneStarter - Failed to start pulsar service.
org.apache.pulsar.broker.PulsarServerException: java.util.NoSuchElementException: No value present
at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:796) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandalone.start(PulsarStandalone.java:296) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandaloneStarter.main(PulsarStandaloneStarter.java:131) [pulsar-broker.jar:2.9.0-SNAPSHOT]
Caused by: java.util.NoSuchElementException: No value present
at java.util.Optional.get(Optional.java:148) ~[?:?]
at org.apache.pulsar.broker.loadbalance.NoopLoadManager.start(NoopLoadManager.java:55) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.startLoadManagementService(PulsarService.java:1047) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:751) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
... 2 more
```
Perhaps the PR description is currently confusing. In PR #11548, the problem is that when you set non-tls ports to an empty value, exceptions will happen. I tested with Pulsar development version by building Pulsar locally and appending these settings to `standalone.conf`:
```
brokerServicePort=
brokerServicePortTls=6651
webServicePort=
webServicePortTls=8443
brokerClientTlsEnabled=true
tlsEnabled=true
tlsAllowInsecureConnection=true
tlsCertificateFilePath=pulsar-broker/src/test/resources/authentication/tls-http/broker.cert.pem
tlsKeyFilePath=pulsar-broker/src/test/resources/authentication/tls-http/broker.key-pk8.pem
tlsTrustCertsFilePath=pulsar-broker/src/test/resources/authentication/tls-http/ca.cert.pem
```
and then starting Pulsar with `./bin/pulsar standalone`. I simply kept iterating until the problems were fixed.
I also added unit tests to verify that an empty port value gets converted to Optional.empty. The unit test for TLS without non-TLS ports enabled is also added to verify that in unit tests. That's the reason to add these tests. I added these tests before starting to work on testing it manually with Pulsar standalone.
The reason why the change to Function Worker service is required is that "useTls" setting is deprecated in configuration:
https://github.com/apache/pulsar/blob/047fb6a3ca0cf9c137a5597339e20247d4c61cdf/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java#L357-L360
The same applies to "tlsEnabled" in broker.conf. [It has been marked as deprecated](https://github.com/apache/pulsar/blob/ddeae659be1f80f428fdb8c82c7a9a0c931c81fe/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L1020-L1021).
tlsEnabled was deprecated by PR #2988 many years ago.
At first I was testing without setting tlsEnabled=true and that's why I hit the issue and needed to make the changes.
@sijie I didn't record all of the exceptions that happen without the changes and what this PR fixes. Is that necessary for accepting this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-902167808
PR for cherry-picking to branch-2.7 is #11724
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#discussion_r691044967
##########
File path: site2/docs/security-tls-keystore.md
##########
@@ -131,6 +131,13 @@ brokerClientTlsTrustStorePassword=clientpw
NOTE: it is important to restrict access to the store files via filesystem permissions.
+When TLS is configured on the broker, the non-TLS ports `brokerServicePort` and `webServicePort` can be disabled by providing an empty value in configuration.
+```
+brokerServicePort=
+webServicePort=
+```
+In this case it's mandatory to specify `brokerClientTlsEnabled=true`, `brokerClientTlsEnabledWithKeyStore=true` and the related configuration properties `brokerClientTlsTrustStore` and `brokerClientTlsTrustStorePassword`.
Review comment:
```suggestion
In this case, you need to set the following configurations.
```conf
brokerClientTlsEnabled=true // Set this to true
brokerClientTlsEnabledWithKeyStore=true // Set this to true
brokerClientTlsTrustStore= // Set this to your desired value
brokerClientTlsTrustStorePassword= // Set this to your desired value
```
```
##########
File path: site2/docs/security-tls-keystore.md
##########
@@ -131,6 +131,13 @@ brokerClientTlsTrustStorePassword=clientpw
NOTE: it is important to restrict access to the store files via filesystem permissions.
+When TLS is configured on the broker, the non-TLS ports `brokerServicePort` and `webServicePort` can be disabled by providing an empty value in configuration.
Review comment:
```suggestion
If you have configured TLS on the broker, to disable non-TLS ports, you can set the values of the following configurations to empty as below.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900821479
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#discussion_r690968459
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -1122,7 +1122,7 @@ private String getBrokerAddress() {
return String.format("%s:%s", pulsar.getAdvertisedAddress(),
pulsar.getConfiguration().getWebServicePort().isPresent()
? pulsar.getConfiguration().getWebServicePort().get()
- : pulsar.getConfiguration().getWebServicePortTls());
+ : pulsar.getConfiguration().getWebServicePortTls().get());
Review comment:
makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-901175813
> Thanks for your contribution. I've made some comments, PTAL.
@Anonymitaet thanks for the suggestions. I committed them. Please PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#discussion_r690162266
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -1122,7 +1122,7 @@ private String getBrokerAddress() {
return String.format("%s:%s", pulsar.getAdvertisedAddress(),
pulsar.getConfiguration().getWebServicePort().isPresent()
? pulsar.getConfiguration().getWebServicePort().get()
- : pulsar.getConfiguration().getWebServicePortTls());
+ : pulsar.getConfiguration().getWebServicePortTls().get());
Review comment:
If the Optional has no value `get` will throw a RuntimeException.
What about using orElse(null).
This case won't probably happen but it is better to not risk
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/service/ProxyServer.java
##########
@@ -120,7 +122,9 @@ public void addRestResources(String basePath, String javaPackages, String attrib
}
public void start() throws PulsarServerException {
- log.info("Starting web socket proxy at port {}", conf.getWebServicePort().get());
+ log.info("Starting web socket proxy at port {}", String.join(",", Arrays.stream(server.getConnectors())
+ .map(ServerConnector.class::cast).map(ServerConnector::getPort).map(Object::toString)
+ .collect(Collectors.toList())));
Review comment:
Nit: there is a Collectors.joining utility method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900142910
@sijie This was the 2nd exception that this PR fixed in Pulsar Standalone (with TLS ports only config mentioned in my previous comment):
```
12:29:46.424 [main] ERROR org.apache.pulsar.functions.worker.PulsarWorkerService - Error Starting up in worker
java.lang.NullPointerException: null
at org.apache.pulsar.client.admin.internal.PulsarAdminImpl.<init>(PulsarAdminImpl.java:189) ~[pulsar-client-admin-original.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.client.admin.internal.PulsarAdminBuilderImpl.build(PulsarAdminBuilderImpl.java:47) ~[pulsar-client-admin-original.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.functions.worker.WorkerUtils.getPulsarAdminClient(WorkerUtils.java:221) ~[pulsar-functions-worker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.functions.worker.WorkerUtils.getPulsarAdminClient(WorkerUtils.java:196) ~[pulsar-functions-worker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.functions.worker.PulsarWorkerService$1.newPulsarAdmin(PulsarWorkerService.java:144) ~[pulsar-functions-worker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.functions.worker.PulsarWorkerService.start(PulsarWorkerService.java:433) [pulsar-functions-worker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.startWorkerService(PulsarService.java:1575) [pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:764) [pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandalone.start(PulsarStandalone.java:296) [pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandaloneStarter.main(PulsarStandaloneStarter.java:131) [pulsar-broker.jar:2.9.0-SNAPSHOT]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900130782
> I am not sure how does your PR solve #11548. From what I can see, you are basically changing the function worker service and tests. There is nothing related to brokers. Can you clarify how does your PR fix #11548?
@sijie This is the first issue to hit with "pulsar standalone":
```
12:11:04.314 [main] ERROR org.apache.pulsar.broker.PulsarService - Failed to start Pulsar service: No value present
java.util.NoSuchElementException: No value present
at java.util.Optional.get(Optional.java:148) ~[?:?]
at org.apache.pulsar.broker.loadbalance.NoopLoadManager.start(NoopLoadManager.java:55) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.startLoadManagementService(PulsarService.java:1047) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:751) [pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandalone.start(PulsarStandalone.java:296) [pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandaloneStarter.main(PulsarStandaloneStarter.java:131) [pulsar-broker.jar:2.9.0-SNAPSHOT]
12:11:04.314 [main] ERROR org.apache.pulsar.PulsarStandaloneStarter - Failed to start pulsar service.
org.apache.pulsar.broker.PulsarServerException: java.util.NoSuchElementException: No value present
at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:796) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandalone.start(PulsarStandalone.java:296) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.PulsarStandaloneStarter.main(PulsarStandaloneStarter.java:131) [pulsar-broker.jar:2.9.0-SNAPSHOT]
Caused by: java.util.NoSuchElementException: No value present
at java.util.Optional.get(Optional.java:148) ~[?:?]
at org.apache.pulsar.broker.loadbalance.NoopLoadManager.start(NoopLoadManager.java:55) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.startLoadManagementService(PulsarService.java:1047) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:751) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
... 2 more
```
Perhaps the PR description is currently confusing. In PR #11548, the problem is that when you set non-tls ports to an empty value, exceptions will happen. I tested with Pulsar development version by building Pulsar locally and appending these settings to `standalone.conf`:
```
brokerServicePort=
brokerServicePortTls=6651
webServicePort=
webServicePortTls=8443
brokerClientTlsEnabled=true
tlsEnabled=true
tlsAllowInsecureConnection=true
tlsCertificateFilePath=pulsar-broker/src/test/resources/authentication/tls-http/broker.cert.pem
tlsKeyFilePath=pulsar-broker/src/test/resources/authentication/tls-http/broker.key-pk8.pem
tlsTrustCertsFilePath=pulsar-broker/src/test/resources/authentication/tls-http/ca.cert.pem
``
and then starting Pulsar with `./bin/pulsar standalone`. I simply kept iterating until the problems were fixed.
I also added unit tests to verify that an empty port value gets converted to Optional.empty. The unit test for TLS without non-TLS ports enabled is also added to verify that in unit tests. That's the reason to add these tests. I added these tests before starting to work on testing it manually with Pulsar standalone.
The reason why the change to Function Worker service is required is that "useTls" setting is deprecated in configuration:
https://github.com/apache/pulsar/blob/047fb6a3ca0cf9c137a5597339e20247d4c61cdf/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java#L357-L360
The same applies to "tlsEnabled" in broker.conf. [It has been marked as deprecated](https://github.com/apache/pulsar/blob/ddeae659be1f80f428fdb8c82c7a9a0c931c81fe/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L1020-L1021).
tlsEnabled was deprecated by PR #2988 many years ago.
At first I was testing without setting tlsEnabled=true and that's why I hit the issue and needed to make the changes.
@sijie I didn't record all of the exceptions that happen without the changes and what this PR fixes. Is that necessary for accepting this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#discussion_r691044967
##########
File path: site2/docs/security-tls-keystore.md
##########
@@ -131,6 +131,13 @@ brokerClientTlsTrustStorePassword=clientpw
NOTE: it is important to restrict access to the store files via filesystem permissions.
+When TLS is configured on the broker, the non-TLS ports `brokerServicePort` and `webServicePort` can be disabled by providing an empty value in configuration.
+```
+brokerServicePort=
+webServicePort=
+```
+In this case it's mandatory to specify `brokerClientTlsEnabled=true`, `brokerClientTlsEnabledWithKeyStore=true` and the related configuration properties `brokerClientTlsTrustStore` and `brokerClientTlsTrustStorePassword`.
Review comment:
```suggestion
In this case, you need to set the following configurations.
```conf
brokerClientTlsEnabled=true // Set this to true
brokerClientTlsEnabledWithKeyStore=true // Set this to true
brokerClientTlsTrustStore= // Set this to your desired value
brokerClientTlsTrustStorePassword= // Set this to your desired value
```
```
##########
File path: site2/docs/security-tls-keystore.md
##########
@@ -131,6 +131,13 @@ brokerClientTlsTrustStorePassword=clientpw
NOTE: it is important to restrict access to the store files via filesystem permissions.
+When TLS is configured on the broker, the non-TLS ports `brokerServicePort` and `webServicePort` can be disabled by providing an empty value in configuration.
Review comment:
```suggestion
If you have configured TLS on the broker, to disable non-TLS ports, you can set the values of the following configurations to empty as below.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11681: [Broker] Support disabling non-TLS service ports
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#discussion_r690968459
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java
##########
@@ -1122,7 +1122,7 @@ private String getBrokerAddress() {
return String.format("%s:%s", pulsar.getAdvertisedAddress(),
pulsar.getConfiguration().getWebServicePort().isPresent()
? pulsar.getConfiguration().getWebServicePort().get()
- : pulsar.getConfiguration().getWebServicePortTls());
+ : pulsar.getConfiguration().getWebServicePortTls().get());
Review comment:
makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org