[PIP 95][Issue 12040][broker] Decouple advertised listeners from bind addresses#12079
Merged
eolivelli merged 5 commits intoapache:masterfrom Oct 15, 2021
Merged
Conversation
# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/MessagingServiceShutdownHook.java # pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/NoopLoadManager.java
This was referenced Oct 13, 2021
eolivelli
approved these changes
Oct 15, 2021
Contributor
eolivelli
left a comment
There was a problem hiding this comment.
LGTM
@codelipenghui @merlimat PTAL
codelipenghui
approved these changes
Oct 15, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Master Issue: #12040
Related: #3501
Motivation
This PR decouples the advertised listener configuration from the broker bind addresses, and fixes a few places where the broker is assumed to have a plaintext cluster endpoint or conflates the TLS/plaintext endpoints.
The goal is to support a specific configuration, where the broker has an advertised listener corresponding to an Istio gateway, and the gateway terminates TLS (
pulsar+ssl://broker1.gateway:6651/). Because Istio uses mTLS transparently within the mesh, the broker has only a plaintext server endpoint (pulsar://broker1.cluster:6650/). Without this patch, the broker rejects the configuration because it erroneously assumes that the broker should have a TLS binding to support a TLS advertised address.This PR deprecates the
PulsarService::getSafeBrokerServiceUrlmethod, which returns a plaintext endpoint if available, and a TLS endpoint otherwise. The resultant endpoint is then advertised to Pulsar clients but the scheme information isn't always considered. Observe that client code disregards the scheme in some cases:pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/HttpLookupService.java
Lines 95 to 105 in d81b5f8
Note that this PR is focused on the Pulsar protocol endpoints, not the REST API endpoints.
Modifications
PulsarService::getSafeBrokerServiceUrland be explicit about which address is used.Verifying this change
This change is already covered by existing tests, such as:
MultipleListenerValidatorTestDoes this pull request potentially affect one of the following parts:
The information that is returned by the broker is slightly different in the case of a pure-TLS broker. In that case, lookup results (which consist of a pair of URLs) might not have a plaintext URL. In concept a client could break, though in practice this is unlikely since the broker didn't really work in such a configuration anyway.
Documentation
Need to update docs?
Internal changes.