[Issue 12040][broker] Fix advertised listener confusion #12353

EronWright · 2021-10-13T17:12:35Z

Master Issue: #12040

Motivation

A couple of PRs (#10312 and #10961) caused an unintended change to the broker address that is used in various scenarios. There are three important cases to consider:

the advertised broker service URL(s) (for pulsar:// and pulsar+ssl://)
the advertised web service URL (for http:// and https://)
the canonical name of the broker for identification purposes, e.g. bundle ownership stored in ZK

Note that the advertisedListeners configuration property is designed to support the broker service URL only, not the web service URL. Support for multiple web addresses will be addressed later.

The first PR unintentionally caused the web service URL to be derived from advertisedListeners. This caused #10951 and led to the second PR. The second PR introduced a parameter ignoreAdvertisedListener with some undesirable effects:

Causes the canonical broker name to be based on the internal advertisedListener though it is historically based on the web service URL. For example, the NamespaceService uses pulsar.getSafeWebServiceAddress() as the broker name.
Fails to use the advertisedListeners in computing PulsarService::brokerUrl, which is used for intra-cluster communication (eg. by the function worker).
Breaks the compactor tool in combination with advertisedListeners.

Modifications

This PR cleans up the code to use the correct identifier for each case.

For the broker service URL, a helper method is introduced (ServiceConfigurationUtils::getInternalListener) to obtain the endpoint to be used for intra-cluster connectivity. This is designed to work in all cases, even when advertisedListeners isn't configured. Also, the use of ServiceConfiguration::brokerServicePort was minimized for interoperability with #12079.

For the web service URL, a helper method is introduced (ServiceConfigurationUtils::getWebServiceAddress) to encapsulate the logic and to help with future enhancements. This PR re-enables the use of advertisedAddress to configure the web service URL, independent of the advertisedListeners.

For the broker canonical name, the codebase typically use PulsarService::getAdvertisedAddress. The value is simply a hostname and should never be used to connect to the broker; it is strictly for identification purposes. Historically the value is derived from the web service URL, not the broker service URL. For that reason, this PR doesn't use advertisedListeners when computing the canonical name.

To test the decoupling of advertised port from listen port, a test case was enhanced to use a simple port forwarder.

Specific changes:

deprecate ServiceConfigurationUtils::getAppliedAdvertisedAddress
introduce ServiceConfigurationUtils::getWebServiceAddress
introduce ServiceConfigurationUtils::getInternalListener
relax the limitation that advertisedAddress and advertisedListener cannot be used at the same time
clarify the purpose of PulsarService::advertisedAddress
relax validation of brokerServicePort (it is not needed in pure-TLS configurations)
stabilize the ordering of advertised listeners
create a utility class that implements port-forwarding for test purposes

Compatibility

This PR will impact installations of Pulsar 2.8.1 when advertisedListeners are configured. This is because #10961 was included in 2.8.1 and has various effects (as mentioned above). This PR will restore the original behavior. A possible mitigation strategy is to set the advertisedAddress configuration property, in addition to advertisedListeners. The advertisedAddress property is the basis for the broker canonical name; set it to the hostname of the internal advertised listener to avoid a change to the effective values when upgrading from 2.8.1 to 2.9.

Followups

Update protocol handlers to use ServiceConfigurationUtils::getInternalListener (see: Apply new getAppliedAdvertisedAddress signature streamnative/aop#242)

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

Unit tests were added or updated to:

org.apache.pulsar.broker.PulsarServiceTest
org.apache.pulsar.client.api.PulsarMultiListenersWithInternalListenerNameTest

Does this pull request potentially affect one of the following parts:

Anything that affects deployment: yes

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

doc-required

- deprecate ServiceConfigurationUtils::getAppliedAdvertisedAddress - clarify the purpose of PulsarService::advertisedAddress - introduce a helper method for webServiceAddress - stabilize the ordering of advertised listeners

- checkstyle

EronWright · 2021-10-13T22:09:09Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

@@ -216,6 +216,9 @@
    private PulsarClient client = null;
    private ZooKeeperClientFactory zkClientFactory = null;
    private final String bindAddress;
+    /**
+     * The host component of the broker's canonical name.
+     */
    private final String advertisedAddress;


I am inclined to deprecate the advertisedAddress in favor of a formal broker name property. In a follow-up PR, we could introduce a BrokerName type (as opposed to a String) to help with refactoring. For this PR, I left advertisedAddress as-is but clarified its purpose.

EronWright · 2021-10-13T22:13:39Z

...r-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java

@@ -936,7 +935,7 @@ private void updateLoadBalancingMetrics(final SystemResourceUsage systemResource
        List<Metrics> metrics = Lists.newArrayList();
        Map<String, String> dimensions = new HashMap<>();

-        dimensions.put("broker", ServiceConfigurationUtils.getAppliedAdvertisedAddress(conf, true));
+        dimensions.put("broker", pulsar.getAdvertisedAddress());


I believe the broker dimension represents the broker canonical name, not any particular network address. The effective value is unchanged.

-bugfix

- updated tests - remove obsolete validation check

…pplied-listener # Conflicts: # pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfigurationUtils.java

eolivelli

LGTM

Anonymitaet · 2021-10-14T09:28:36Z

Hi @EronWright since the due date of 2.9.0 went past, do you still want this PR to scope in 2.9.0?

…pplied-listener

EronWright · 2021-10-14T15:30:03Z

@Anonymitaet yes I believe it an important fix for 2.9.

EronWright · 2021-10-14T19:32:23Z

I am fixing the Broker Client test failure. The issue is that this PR fixes the broker address in PulsarService::brokerUrl to be the advertised listener address (which is correct since it is used to configure Pulsar clients). The failing test uses a static listener configuration while trying to use a dynamic bind address, leading to Connection refused.

Update: fixed by introducing a simple port forwarder into the test setup to forward traffic from the advertised port to the listen port. This makes the test stronger because we're testing the independence of those two ports.

codelipenghui · 2021-10-14T23:29:36Z

/pulsarbot run-failure-checks

- add a utility class for local port-forwarding - enhance the MockedPulsarServiceBaseTest to port-forward from advertised port to listen port - fix the PulsarMultiListenersWithInternalListenerNameTest

codelipenghui · 2021-10-15T10:08:18Z

@315157973 Please help review the PR since you have more context about this part.

tomscut · 2021-10-15T11:32:38Z

...roker-common/src/main/java/org/apache/pulsar/broker/validator/MultipleListenerValidator.java

@@ -38,8 +38,7 @@
 public final class MultipleListenerValidator {

    /**
-     * validate the configure of `advertisedListeners`, `internalListenerName`, `advertisedAddress`.
-     * 1. `advertisedListeners` and `advertisedAddress` must not appear together.
+     * Validate the configuration of `advertisedListeners`, `internalListenerName`.
     * 2. the listener name in `advertisedListeners` must not duplicate.


Do we need to change the java doc here, starting from 1.

@tomscut can you please send a PR to fix this ?
I am going to merge this patch now and prepare 2.9.0 release soon

@tomscut can you please send a PR to fix this ? I am going to merge this patch now and prepare 2.9.0 release soon

Yes, I sent a PR #12389, PTAL. Thank you.

### Motivation *Fix the java doc mentioned in [#12353](https://github.com/apache/pulsar/pull/12353/files#r729887373)* ### Modifications *Fix java doc for MultipleListenerValidator#validateAndAnalysisAdvertisedListener.*

### Motivation *Fix the java doc mentioned in [#12353](https://github.com/apache/pulsar/pull/12353/files#r729887373)* ### Modifications *Fix java doc for MultipleListenerValidator#validateAndAnalysisAdvertisedListener.* (cherry picked from commit 9d10b8b)

* [Issue 12040] - deprecate ServiceConfigurationUtils::getAppliedAdvertisedAddress - clarify the purpose of PulsarService::advertisedAddress - introduce a helper method for webServiceAddress - stabilize the ordering of advertised listeners - add a utility class for local port-forwarding - enhance the MockedPulsarServiceBaseTest to port-forward from advertised port to listen port - fix the PulsarMultiListenersWithInternalListenerNameTest Co-authored-by: penghui <[email protected]>

[Issue 12040]

0af51bc

- deprecate ServiceConfigurationUtils::getAppliedAdvertisedAddress - clarify the purpose of PulsarService::advertisedAddress - introduce a helper method for webServiceAddress - stabilize the ordering of advertised listeners

EronWright changed the title ~~[Issue 12040][broker] Fix advertised listeners~~ [Issue 12040][broker] Fix advertised listener confusion Oct 13, 2021

[Issue 12040]

c084cd5

- checkstyle

EronWright commented Oct 13, 2021

View reviewed changes

EronWright added 2 commits October 13, 2021 15:21

[Issue 12040]

cb4fd99

-bugfix

[Issue 12040]

c320c2e

- updated tests - remove obsolete validation check

codelipenghui added this to the 2.9.0 milestone Oct 14, 2021

codelipenghui assigned EronWright Oct 14, 2021

codelipenghui requested review from 315157973, eolivelli, merlimat, hangc0276 and codelipenghui October 14, 2021 01:19

Merge remote-tracking branch 'origin/master' into ewright/pip95-fix-a…

d561a47

…pplied-listener # Conflicts: # pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfigurationUtils.java

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

eolivelli approved these changes Oct 14, 2021

View reviewed changes

Merge remote-tracking branch 'apache/master' into ewright/pip95-fix-a…

b63ce16

…pplied-listener

[Issue 12040]

b5394a6

- add a utility class for local port-forwarding - enhance the MockedPulsarServiceBaseTest to port-forward from advertised port to listen port - fix the PulsarMultiListenersWithInternalListenerNameTest

gaoran10 added the area/broker label Oct 15, 2021

codelipenghui approved these changes Oct 15, 2021

View reviewed changes

tomscut reviewed Oct 15, 2021

View reviewed changes

eolivelli merged commit 6382be8 into apache:master Oct 15, 2021

codelipenghui deleted the ewright/pip95-fix-applied-listener branch October 15, 2021 15:23

EronWright mentioned this pull request Oct 16, 2021

Update for Pulsar 2.9 streamnative/aop#300

Closed

EronWright mentioned this pull request Oct 16, 2021

Update for Pulsar 2.9 streamnative/kop#815

Closed

cbornet mentioned this pull request Mar 16, 2022

Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy datastax/pulsar#37

Closed

Demogorgon314 mentioned this pull request Sep 30, 2022

[fix][doc] Rewrite the steps of docker-deploy #17853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 12040][broker] Fix advertised listener confusion #12353

[Issue 12040][broker] Fix advertised listener confusion #12353

EronWright commented Oct 13, 2021 •

edited

Loading

EronWright Oct 13, 2021 •

edited

Loading

EronWright Oct 13, 2021 •

edited

Loading

eolivelli left a comment

Anonymitaet commented Oct 14, 2021

EronWright commented Oct 14, 2021

EronWright commented Oct 14, 2021 •

edited

Loading

codelipenghui commented Oct 14, 2021

codelipenghui commented Oct 15, 2021

tomscut Oct 15, 2021

eolivelli Oct 15, 2021

eolivelli Oct 15, 2021

tomscut Oct 15, 2021

[Issue 12040][broker] Fix advertised listener confusion #12353

[Issue 12040][broker] Fix advertised listener confusion #12353

Conversation

EronWright commented Oct 13, 2021 • edited Loading

Motivation

Modifications

Compatibility

Followups

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

EronWright Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

EronWright Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

Anonymitaet commented Oct 14, 2021

EronWright commented Oct 14, 2021

EronWright commented Oct 14, 2021 • edited Loading

codelipenghui commented Oct 14, 2021

codelipenghui commented Oct 15, 2021

tomscut Oct 15, 2021

Choose a reason for hiding this comment

eolivelli Oct 15, 2021

Choose a reason for hiding this comment

eolivelli Oct 15, 2021

Choose a reason for hiding this comment

tomscut Oct 15, 2021

Choose a reason for hiding this comment

EronWright commented Oct 13, 2021 •

edited

Loading

EronWright Oct 13, 2021 •

edited

Loading

EronWright Oct 13, 2021 •

edited

Loading

EronWright commented Oct 14, 2021 •

edited

Loading