[cleanup][proxy] Use correct address for HAProxyMessage destination #16045

michaeljmarshall · 2022-06-14T05:19:55Z

Motivation

When reading through the Proxy code, I noticed that the remoteAddress was used where it probably makes sense to use the localAddress.

After researching it a bit with the AWS docs https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html and the protocol spec http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt, I think this implementation is correct.

Modifications

Replace inboundChannel.localAddress with inboundChannel.localAddress.
Fix type checking

doc-not-needed

github-actions · 2022-06-14T05:20:36Z

@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

github-actions · 2022-06-14T05:24:18Z

@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

github-actions · 2022-06-14T05:24:27Z

@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

github-actions · 2022-06-14T05:24:37Z

@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

lhotari · 2022-06-14T12:51:13Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java

@@ -233,7 +233,7 @@ private void writeHAProxyMessage() {
                InetSocketAddress clientAddress = (InetSocketAddress) inboundChannel.remoteAddress();
                String sourceAddress = clientAddress.getAddress().getHostAddress();
                int sourcePort = clientAddress.getPort();
-                InetSocketAddress proxyAddress = (InetSocketAddress) inboundChannel.remoteAddress();
+                InetSocketAddress proxyAddress = (InetSocketAddress) outboundChannel.localAddress();


I think that it should be using outboundChannel.remoteAddress() instead of .localAddress().

Suggested change

InetSocketAddress proxyAddress = (InetSocketAddress) outboundChannel.localAddress();

InetSocketAddress proxyAddress = (InetSocketAddress) outboundChannel.remoteAddress();

Should we also change the instanceof check in the conditional just above this code? I don't know anything about this code other than the fact that the names imply that the inboundChannel.remoteAddress should not be called twice and assigned to different variables.

I think it should be the address that the client requested. Actually, the destinationAddress is useless for now since we introduced HAProxyMessage only for getting the real client address even if users use other L4 proxies or the Pulsar proxy.

@codelipenghui - do you have any insight on this issue? It looks like you contributed some of the original code here.

Thanks for your response @codelipenghui. I finally got back to this and did some research with @lhotari. Based on these docs, https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html, it looks like we want the proxy address. I'm updating the PR now. I think it could be useful to expose the address in the broker logs to make it clear which proxy is doing the routing.

github-actions · 2022-07-17T02:14:10Z

The pr had no activity for 30 days, mark with Stale label.

lhotari

LGTM

codecov-commenter · 2022-10-12T08:38:31Z

Codecov Report

Merging #16045 (34db600) into master (6c65ca0) will increase coverage by 11.89%.
The diff coverage is 65.00%.

@@              Coverage Diff              @@
##             master   #16045       +/-   ##
=============================================
+ Coverage     34.91%   46.81%   +11.89%     
- Complexity     5707    17881    +12174     
=============================================
  Files           607     1573      +966     
  Lines         53396   128213    +74817     
  Branches       5712    14100     +8388     
=============================================
+ Hits          18644    60018    +41374     
- Misses        32119    62050    +29931     
- Partials       2633     6145     +3512

Flag	Coverage Δ
unittests	`46.81% <65.00%> (+11.89%)`	⬆️

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

Impacted Files	Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java	`52.07% <ø> (ø)`
.../service/SystemTopicBasedTopicPoliciesService.java	`62.65% <0.00%> (+11.06%)`	⬆️
...va/org/apache/pulsar/client/impl/ConsumerBase.java	`21.95% <0.00%> (ø)`
...he/pulsar/functions/worker/rest/api/SinksImpl.java	`36.82% <50.00%> (ø)`
...apache/pulsar/proxy/server/DirectProxyHandler.java	`63.63% <50.00%> (ø)`
...broker/delayed/InMemoryDelayedDeliveryTracker.java	`65.00% <75.00%> (+65.00%)`	⬆️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	`71.15% <80.00%> (ø)`
...pulsar/broker/admin/impl/PersistentTopicsBase.java	`52.60% <100.00%> (+41.20%)`	⬆️
...rg/apache/pulsar/broker/service/BrokerService.java	`59.24% <100.00%> (+11.24%)`	⬆️
...g/apache/pulsar/compaction/CompactedTopicImpl.java	`81.42% <100.00%> (+70.71%)`	⬆️
... and 1148 more

michaeljmarshall · 2022-10-12T17:20:33Z

I am going to classify this as "cleanup" instead of "fix" because it's not really a bug. We could look at using the value in the broker logs if we wanted to make it clear which proxy the traffic is ingressing through. For now, I propose we just fix the code in the proxy.

michaeljmarshall added type/bug The PR fixed a bug or issue reported a bug area/proxy doc-not-needed Your PR changes do not impact docs labels Jun 14, 2022

michaeljmarshall added this to the 2.11.0 milestone Jun 14, 2022

michaeljmarshall requested review from lhotari, codelipenghui and hezhangjian June 14, 2022 05:19

michaeljmarshall self-assigned this Jun 14, 2022

github-actions bot removed the doc-not-needed Your PR changes do not impact docs label Jun 14, 2022

github-actions bot added the doc-label-missing label Jun 14, 2022

michaeljmarshall added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Jun 14, 2022

github-actions bot removed the doc-not-needed Your PR changes do not impact docs label Jun 14, 2022

github-actions bot added the doc-label-missing label Jun 14, 2022

lhotari mentioned this pull request Jun 14, 2022

The new Docbot spams PRs with multiple comments #16046

Closed

sijie mentioned this pull request Jun 14, 2022

ISSUE-16046: The new Docbot spams PRs with multiple comments streamnative/pulsar-archived#4372

Open

lhotari requested changes Jun 14, 2022

View reviewed changes

This was referenced Jun 14, 2022

Implement a new documentation bot apache/pulsar-test-infra#43

Merged

Remove Doc Bot Logic To Update PR Descriptions apache/pulsar-test-infra#47

Closed

github-actions bot added the Stale label Jul 17, 2022

codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022

michaeljmarshall force-pushed the fix-ha-proxy-message branch from eb546e1 to 1316cf4 Compare October 12, 2022 06:04

michaeljmarshall changed the title ~~[fix][proxy] Use correct channel for HAProxyMessage dest address~~ [fix][proxy] Use correct address for HAProxyMessage destination Oct 12, 2022

michaeljmarshall requested a review from lhotari October 12, 2022 06:07

lhotari approved these changes Oct 12, 2022

View reviewed changes

michaeljmarshall added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use and removed type/bug The PR fixed a bug or issue reported a bug labels Oct 12, 2022

michaeljmarshall changed the title ~~[fix][proxy] Use correct address for HAProxyMessage destination~~ [cleanup][proxy] Use correct address for HAProxyMessage destination Oct 12, 2022

michaeljmarshall added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing Stale labels Oct 12, 2022

michaeljmarshall added 2 commits October 13, 2022 21:24

[fix][proxy] Use correct channel for HAProxyMessage dest address

e80353a

Fix implementation after researching HAProxy

34db600

michaeljmarshall force-pushed the fix-ha-proxy-message branch from 1316cf4 to 34db600 Compare October 14, 2022 02:24

michaeljmarshall merged commit 7dc9a5f into apache:master Oct 14, 2022

michaeljmarshall deleted the fix-ha-proxy-message branch October 14, 2022 15:05

michaeljmarshall mentioned this pull request May 10, 2023

[improve][broker] Add haProxyProtocolEnabled more description #19967

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cleanup][proxy] Use correct address for HAProxyMessage destination #16045

[cleanup][proxy] Use correct address for HAProxyMessage destination #16045

michaeljmarshall commented Jun 14, 2022 •

edited

Loading

github-actions bot commented Jun 14, 2022

github-actions bot commented Jun 14, 2022

github-actions bot commented Jun 14, 2022

github-actions bot commented Jun 14, 2022

lhotari Jun 14, 2022

michaeljmarshall Jun 14, 2022

codelipenghui Jun 15, 2022

michaeljmarshall Jun 16, 2022

michaeljmarshall Oct 12, 2022

github-actions bot commented Jul 17, 2022

lhotari left a comment

codecov-commenter commented Oct 12, 2022 •

edited

Loading

michaeljmarshall commented Oct 12, 2022

	InetSocketAddress proxyAddress = (InetSocketAddress) outboundChannel.localAddress();
	InetSocketAddress proxyAddress = (InetSocketAddress) outboundChannel.remoteAddress();

[cleanup][proxy] Use correct address for HAProxyMessage destination #16045

[cleanup][proxy] Use correct address for HAProxyMessage destination #16045

Conversation

michaeljmarshall commented Jun 14, 2022 • edited Loading

Motivation

Modifications

github-actions bot commented Jun 14, 2022

github-actions bot commented Jun 14, 2022

github-actions bot commented Jun 14, 2022

github-actions bot commented Jun 14, 2022

lhotari Jun 14, 2022

Choose a reason for hiding this comment

michaeljmarshall Jun 14, 2022

Choose a reason for hiding this comment

codelipenghui Jun 15, 2022

Choose a reason for hiding this comment

michaeljmarshall Jun 16, 2022

Choose a reason for hiding this comment

michaeljmarshall Oct 12, 2022

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2022

lhotari left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 12, 2022 • edited Loading

Codecov Report

michaeljmarshall commented Oct 12, 2022

michaeljmarshall commented Jun 14, 2022 •

edited

Loading

codecov-commenter commented Oct 12, 2022 •

edited

Loading