[Broker] Support disabling non-TLS service ports #11681

lhotari · 2021-08-17T08:01:04Z

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.

sijie

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?

eolivelli

Overall looks good to me.
I left one comment PTAL

...ar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java

pulsar-websocket/src/main/java/org/apache/pulsar/websocket/service/ProxyServer.java

lhotari · 2021-08-17T09:13:43Z

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:

pulsar/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java

Lines 357 to 360 in 047fb6a

    
           // TLS for Functions -> Broker 
        
           // @deprecated use "pulsar+ssl://" in serviceUrl to enable 
        
           @Deprecated 
        
           private boolean useTls = false;

The same applies to "tlsEnabled" in broker.conf. It has been marked as deprecated.
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?

lhotari · 2021-08-17T09:31:31Z

@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]

lhotari · 2021-08-17T09:45:55Z

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.

Anonymitaet · 2021-08-18T01:18:22Z

Thanks for your contribution. Do we need to update docs here https://pulsar.apache.org/docs/en/next/reference-configuration/#broker?

michaeljmarshall

LGTM

It's worth adding that the helm chart already creates the pulsar broker service in such a way that either the non-TLS port or the TLS port (exclusively) is open for the pulsar protocol: https://github.com/apache/pulsar-helm-chart/blob/c3e4ea272b15d7e806d2585059686e8270b98b67/charts/pulsar/templates/broker-service.yaml#L32-L45

  ports:
  # prometheus needs to access /metrics endpoint
  - name: http
    port: {{ .Values.broker.ports.http }}
  {{- if or (not .Values.tls.enabled) (not .Values.tls.broker.enabled) }}
  - name: pulsar
    port: {{ .Values.broker.ports.pulsar }}
  {{- end }}
  {{- if and .Values.tls.enabled .Values.tls.broker.enabled }}
  - name: https
    port: {{ .Values.broker.ports.https }}
  - name: pulsarssl
    port: {{ .Values.broker.ports.pulsarssl }}
  {{- end }}

lhotari · 2021-08-18T05:24:31Z

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.

lhotari · 2021-08-18T05:34:57Z

@Anonymitaet I added docs to security-tls-keystore.md file. Please review

Anonymitaet

Thanks for your contribution. I've made some comments, PTAL.

site2/docs/security-tls-keystore.md

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>

lhotari · 2021-08-18T14:44:40Z

Thanks for your contribution. I've made some comments, PTAL.

@Anonymitaet thanks for the suggestions. I committed them. Please PTAL

* Support disabling non-tls service ports * Add docs for disabling non-TLS ports * Update site2/docs/security-tls-keystore.md Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com> (cherry picked from commit 50b6e79)

lhotari · 2021-08-19T19:05:47Z

PR for cherry-picking to branch-2.7 is #11724

* Support disabling non-tls service ports * Add docs for disabling non-TLS ports * Update site2/docs/security-tls-keystore.md Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com> (cherry picked from commit 50b6e79)

* Support disabling non-tls service ports * Add docs for disabling non-TLS ports * Update site2/docs/security-tls-keystore.md Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>

…, and apache#11681

lhotari · 2023-06-09T08:56:58Z

#20535 is also required for supporting disabling of non-TLS service ports.

lhotari added type/bug The PR fixed a bug or issue reported a bug area/security labels Aug 17, 2021

lhotari added this to the 2.9.0 milestone Aug 17, 2021

lhotari requested review from merlimat, sijie, rdhabalia and eolivelli August 17, 2021 08:01

lhotari self-assigned this Aug 17, 2021

sijie requested review from codelipenghui, hangc0276 and jiazhai August 17, 2021 08:29

sijie reviewed Aug 17, 2021

View reviewed changes

eolivelli approved these changes Aug 17, 2021

View reviewed changes

...ar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java Show resolved Hide resolved

pulsar-websocket/src/main/java/org/apache/pulsar/websocket/service/ProxyServer.java Outdated Show resolved Hide resolved

Support disabling non-tls service ports

16832a1

lhotari force-pushed the lh-support-disabling-nontls-service-ports branch from 2ca5173 to 16832a1 Compare August 17, 2021 08:56

lhotari mentioned this pull request Aug 17, 2021

Pulsar server non-tls ports are still listening when TLS in-transit encryption is enabled #11548

Closed

merlimat approved these changes Aug 18, 2021

View reviewed changes

michaeljmarshall approved these changes Aug 18, 2021

View reviewed changes

Add docs for disabling non-TLS ports

feae9fd

lhotari requested a review from sijie August 18, 2021 07:17

Anonymitaet reviewed Aug 18, 2021

View reviewed changes

site2/docs/security-tls-keystore.md Outdated Show resolved Hide resolved

site2/docs/security-tls-keystore.md Outdated Show resolved Hide resolved

Anonymitaet added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Aug 18, 2021

lhotari and others added 2 commits August 18, 2021 12:51

Update site2/docs/security-tls-keystore.md

4a86971

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>

Update site2/docs/security-tls-keystore.md

5be0d48

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>

eolivelli requested a review from Anonymitaet August 18, 2021 14:00

Anonymitaet approved these changes Aug 19, 2021

View reviewed changes

lhotari merged commit 50b6e79 into apache:master Aug 19, 2021

lhotari mentioned this pull request Aug 19, 2021

[cherry-pick] [Broker] Support disabling non-TLS service ports (#11681) #11724

Merged

lhotari added cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.4 release/2.8.2 labels Aug 23, 2021

codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Sep 9, 2021

Anonymitaet mentioned this pull request Dec 29, 2021

[Doc] add docs for client, security, geo #13556

Merged

michaeljmarshall mentioned this pull request Apr 26, 2022

[feature][Function Worker] Support disabling non-TLS service port #15328

Merged

1 task

momo-jun added a commit to momo-jun/pulsar that referenced this pull request Aug 4, 2022

Merge 2.8.2-specific docs from apache#12175, apache#11988, apache#11828…

8869ffe

…, and apache#11681

nicoloboschi mentioned this pull request Jan 19, 2023

[fix][fn] Fix TLS configuration for fn worker to broker if auth disabled #19290

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Broker] Support disabling non-TLS service ports #11681

[Broker] Support disabling non-TLS service ports #11681

lhotari commented Aug 17, 2021

sijie left a comment

eolivelli left a comment

lhotari commented Aug 17, 2021 •

edited

Loading

lhotari commented Aug 17, 2021

lhotari commented Aug 17, 2021

Anonymitaet commented Aug 18, 2021

michaeljmarshall left a comment •

edited

Loading

lhotari commented Aug 18, 2021

lhotari commented Aug 18, 2021

Anonymitaet left a comment

lhotari commented Aug 18, 2021

lhotari commented Aug 19, 2021

lhotari commented Jun 9, 2023

[Broker] Support disabling non-TLS service ports #11681

[Broker] Support disabling non-TLS service ports #11681

Conversation

lhotari commented Aug 17, 2021

Motivation

Modifications

sijie left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

lhotari commented Aug 17, 2021 • edited Loading

lhotari commented Aug 17, 2021

lhotari commented Aug 17, 2021

Anonymitaet commented Aug 18, 2021

michaeljmarshall left a comment • edited Loading

Choose a reason for hiding this comment

lhotari commented Aug 18, 2021

lhotari commented Aug 18, 2021

Anonymitaet left a comment

Choose a reason for hiding this comment

lhotari commented Aug 18, 2021

lhotari commented Aug 19, 2021

lhotari commented Jun 9, 2023

lhotari commented Aug 17, 2021 •

edited

Loading

michaeljmarshall left a comment •

edited

Loading