Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][client] Broker address resolution wrong if connect through a multi-dns names proxy #19597

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

nicoloboschi
Copy link
Contributor

@nicoloboschi nicoloboschi commented Feb 22, 2023

Motivation

Regression of #19327 (It only affects dev branches since no release have been done containing that pull)
in the client, to understand if is connected through a proxy, it checks the logical address vs the physical one, if they're different it supposes it has to target that specific broker passing through the proxy.

In the above pull request, the comparison logic is changed and the logical address is compared to ONE of the resolved dns names. in case of multiple dns names of the proxy, it might happens that the first resolved dns name doesn't match with the logical proxy address and therefore it's considered to be a broker dns name.
This leads to the proxy to recursive send connections to itself.

These are some logs extracted:

11:02:35.141 [pulsar-client-io-2-1] INFO  org.apache.pulsar.client.impl.PulsarChannelInitializer - initializeClientCnx DEBUG pulsar-proxy:6651, pulsar-proxy.pcert.svc.cluster.local:6651                                                                                                                                                                              
11:02:35.147 [pulsar-client-io-2-1] INFO  org.apache.pulsar.client.impl.ConnectionPool - [[id: 0xce7e9e49, L:/10.42.0.247:41194 - R:pulsar-proxy.pcert.svc.cluster.local/10.43.217.3:6651]] Connected to server                                                                                                                                                        11:02:35.172 [pulsar-client-io-2-1] INFO  org.apache.pulsar.client.impl.ClientCnx - [id: 0xce7e9e49, L:/10.42.0.247:41194 - R:pulsar-proxy.pcert.svc.cluster.local/10.43.217.3:6651] Connected through proxy to target broker at pulsar-proxy:6651

Modifications

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @nicoloboschi! Thank you for fixing this!

@codecov-commenter
Copy link

Codecov Report

Merging #19597 (f1b4bcf) into master (456d112) will increase coverage by 29.79%.
The diff coverage is 87.50%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19597       +/-   ##
=============================================
+ Coverage     32.30%   62.10%   +29.79%     
- Complexity     7103    25775    +18672     
=============================================
  Files          1670     1844      +174     
  Lines        125944   135330     +9386     
  Branches      13727    14882     +1155     
=============================================
+ Hits          40690    84043    +43353     
+ Misses        79263    43551    -35712     
- Partials       5991     7736     +1745     
Flag Coverage Δ
inttests 24.55% <75.00%> (-0.11%) ⬇️
systests 25.28% <62.50%> (+0.03%) ⬆️
unittests 59.31% <87.50%> (+39.71%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../org/apache/pulsar/client/impl/ConnectionPool.java 69.69% <85.71%> (+6.24%) ⬆️
...e/pulsar/client/impl/PulsarChannelInitializer.java 84.31% <100.00%> (+37.71%) ⬆️
...g/apache/pulsar/broker/service/StreamingStats.java 89.18% <0.00%> (-5.41%) ⬇️
...ce/ConsistentHashingStickyKeyConsumerSelector.java 76.92% <0.00%> (-3.85%) ⬇️
.../apache/pulsar/client/impl/ClientCnxIdleState.java 68.88% <0.00%> (-2.23%) ⬇️
...lsar/functions/runtime/process/ProcessRuntime.java 47.44% <0.00%> (-2.05%) ⬇️
...tion/pendingack/impl/PendingAckHandleDisabled.java 29.41% <0.00%> (-1.84%) ⬇️
...in/java/org/apache/pulsar/common/api/AuthData.java 71.42% <0.00%> (ø)
...ava/org/apache/pulsar/client/api/schema/Field.java 80.00% <0.00%> (ø)
.../apache/pulsar/broker/namespace/LookupOptions.java 87.50% <0.00%> (ø)
... and 1208 more

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes since I think we need to get the hostname right for the setRemoteHostName method.

@nicoloboschi
Copy link
Contributor Author

Requesting changes since I think we need to get the hostname right for the setRemoteHostName method.

Good point! I've fixed it PTAL

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nicoloboschi nicoloboschi merged commit e286339 into apache:master Feb 23, 2023
nicoloboschi added a commit that referenced this pull request Feb 23, 2023
@nicoloboschi nicoloboschi added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Feb 23, 2023
nicoloboschi added a commit that referenced this pull request Feb 23, 2023
…lti-dns names proxy (#19597)

(cherry picked from commit e286339)
(cherry picked from commit 14b070b)
nicoloboschi added a commit that referenced this pull request Feb 23, 2023
…lti-dns names proxy (#19597)

(cherry picked from commit e286339)
(cherry picked from commit 14b070b)
nicoloboschi added a commit that referenced this pull request Feb 23, 2023
…lti-dns names proxy (#19597)

(cherry picked from commit e286339)
(cherry picked from commit 14b070b)
@nicoloboschi nicoloboschi added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Feb 23, 2023
@nicoloboschi nicoloboschi deleted the fix-proxy-resolution branch February 23, 2023 15:18
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
…lti-dns names proxy (apache#19597)

(cherry picked from commit e286339)
(cherry picked from commit 14b070b)
(cherry picked from commit 27f0449)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants