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

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

Merged
merged 2 commits into from
May 10, 2023

Conversation

crossoverJie
Copy link
Member

@crossoverJie crossoverJie commented Mar 30, 2023

Motivation

When using broker-proxy, I wanted to obtain the real IP address of the client, but I couldn't find much useful information in the documentation. Finally, after reading the source code, I discovered this configuration.
Therefore, the description of haProxyProtocolEnabled was added.

Modifications

Add more description.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

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

If the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

crossoverJie#6

@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Mar 30, 2023
@Anonymitaet
Copy link
Member

@crossoverJie thanks for your contribution! Please attach this in the PR description as required.
image

@Anonymitaet
Copy link
Member

@BewareMyPower Could you please review this PR from a technical perspective? Thank you!

@crossoverJie
Copy link
Member Author

@crossoverJie thanks for your contribution! Please attach this in the PR description as required. image

PTAL

@crossoverJie
Copy link
Member Author

This is based on this pull request #8686 , please take a look.

@codelipenghui

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

This comment is correct but seems redundant - it adds no extra info from my perspective.

@crossoverJie
Copy link
Member Author

This comment is correct but seems redundant - it adds no extra info from my perspective.

image

From the purpose of this PR, it is to provide a way to obtain the real IP address, but using the "real" keyword in proxy.conf cannot find this configuration, and the current description of haProxyProtocolEnabled is also unclear about its relation to "real IP". Unless one looks at the source code, it may be difficult to quickly locate this configuration, so I believe adding a description would be helpful.

@github-actions
Copy link

github-actions bot commented May 2, 2023

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

@github-actions github-actions bot added the Stale label May 2, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #19967 (6392a08) into master (00f17e8) will increase coverage by 35.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19967       +/-   ##
=============================================
+ Coverage     37.61%   72.94%   +35.33%     
- Complexity    12589    31971    +19382     
=============================================
  Files          1691     1868      +177     
  Lines        129028   138586     +9558     
  Branches      14066    15236     +1170     
=============================================
+ Hits          48530   101096    +52566     
+ Misses        74183    29450    -44733     
- Partials       6315     8040     +1725     
Flag Coverage Δ
inttests 24.14% <100.00%> (-0.04%) ⬇️
systests 24.85% <100.00%> (+0.08%) ⬆️
unittests 72.24% <100.00%> (+39.05%) ⬆️

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

Impacted Files Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java 99.37% <ø> (+1.34%) ⬆️
.../apache/bookkeeper/mledger/impl/MetaStoreImpl.java 85.21% <100.00%> (+43.30%) ⬆️

... and 1426 files with indirect coverage changes

@tisonkun
Copy link
Member

Merging...

Thanks for your contribution!

@tisonkun tisonkun merged commit 16b0898 into apache:master May 10, 2023
@@ -85,6 +85,7 @@ advertisedAddress=
# internalListenerName=

# Enable or disable the HAProxy protocol.
# If true, the real IP addresses of consumers and producers can be obtained when getting topic statistics data.
Copy link
Member

Choose a reason for hiding this comment

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

Real might be misleading here. I think the IP address is the remote IP address from the proxy's perspective? If you agree, I think we should clarify further to prevent confusion. This is relevant when traffic ingresses to the proxy through any kind of NAT. A good example is a kubernetes load balancer with externalTrafficPolicy: cluster.

Copy link
Member

Choose a reason for hiding this comment

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

I also found this setting very misleading the first time I encountered it. We're not really even using the HA Proxy protocol. It seems that we appropriated the protocol message to propagate the client IP address, which seems worth documenting since the HAProxy part is largely irrelevant here.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like I might be somewhat mistaken. This PR has some relevant details #16045. The edge case is that you can get the "wrong" address if the inbound connection to the pulsar proxy does not start with the HAProxy message. Here is the relevant code:

private void writeHAProxyMessage() {
if (proxyConnection.hasHAProxyMessage()) {
final ByteBuf msg = encodeProxyProtocolMessage(proxyConnection.getHAProxyMessage());
writeAndFlush(msg);
} else {
if (inboundChannel.remoteAddress() instanceof InetSocketAddress
&& inboundChannel.localAddress() instanceof InetSocketAddress) {
InetSocketAddress clientAddress = (InetSocketAddress) inboundChannel.remoteAddress();
String sourceAddress = clientAddress.getAddress().getHostAddress();
int sourcePort = clientAddress.getPort();
InetSocketAddress proxyAddress = (InetSocketAddress) inboundChannel.localAddress();
String destinationAddress = proxyAddress.getAddress().getHostAddress();
int destinationPort = proxyAddress.getPort();
HAProxyMessage msg = new HAProxyMessage(HAProxyProtocolVersion.V1, HAProxyCommand.PROXY,
HAProxyProxiedProtocol.TCP4, sourceAddress, destinationAddress, sourcePort,
destinationPort);
final ByteBuf encodedMsg = encodeProxyProtocolMessage(msg);
writeAndFlush(encodedMsg);
msg.release();
}
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants