You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/06/14 05:19:57 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
michaeljmarshall opened a new pull request, #16045:
URL: https://github.com/apache/pulsar/pull/16045
### Motivation
When reading through the Proxy code, I noticed that the `inboundChannel` was used where it probably makes sense to use the `outboundChannel`. I am not sure if I've proposed the correct solution, so I'll need someone more familiar to verify this PR's correctness.
Note that a few lines above the changed code, you can see that there is this assertion: `outboundChannel.localAddress() instanceof InetSocketAddress`.
### Modifications
* Replace `inboundChannel.remoteAddress` with `outboundChannel.localAddress`.
### Verifying this change
I probably need to add a test, but I haven't focused on that yet. I'd like to confirm that this is indeed the right solution first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#issuecomment-1154727530
@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#issuecomment-1154727645
@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#discussion_r896777535
##########
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();
Review Comment:
I think that it should be using `outboundChannel.remoteAddress()` instead of `.localAddress()`.
```suggestion
InetSocketAddress proxyAddress = (InetSocketAddress) outboundChannel.remoteAddress();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#issuecomment-1154727429
@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall merged pull request #16045: [cleanup][proxy] Use correct address for HAProxyMessage destination
Posted by GitBox <gi...@apache.org>.
michaeljmarshall merged PR #16045:
URL: https://github.com/apache/pulsar/pull/16045
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#discussion_r993031119
##########
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();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #16045: [fix][proxy] Use correct address for HAProxyMessage destination
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#issuecomment-1275799110
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/16045?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@72e0445`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/pulsar/pull/16045?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #16045 +/- ##
=========================================
Coverage ? 26.39%
Complexity ? 3412
=========================================
Files ? 393
Lines ? 43418
Branches ? 4462
=========================================
Hits ? 11460
Misses ? 30132
Partials ? 1826
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `26.39% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #16045: [fix][proxy] Use correct address for HAProxyMessage destination
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#issuecomment-1276499938
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#discussion_r898697868
##########
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();
Review Comment:
@codelipenghui - do you have any insight on this issue? It looks like you contributed some of the original code here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#issuecomment-1154724788
@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#issuecomment-1186375483
The pr had no activity for 30 days, mark with Stale label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#discussion_r897989610
##########
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();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16045: [fix][proxy] Use correct channel for HAProxyMessage dest address
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16045:
URL: https://github.com/apache/pulsar/pull/16045#discussion_r897012262
##########
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();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org