Support HAProxy proxy protocol for broker and proxy #8686

codelipenghui · 2020-11-24T17:45:13Z

Motivation

Currently, if enable the proxy in the pulsar cluster and client connect to the cluster through the proxy, when we get topic stats, the consumer address and producer address are not the real client address. This PR fix this problem by leverage HAProxy proxy protocol since this is a more general approach. more details about the proxy protocol see
https://www.haproxy.com/blog/haproxy/proxy-protocol/
https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html
http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

Modifications

Allow enable proxy protocol on proxy or broker.

Verifying this change

Tests added

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

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Does this pull request introduce a new feature? (yes)

sijie · 2020-11-24T18:02:38Z

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

@@ -169,6 +171,11 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E

    @Override
    public void channelRead(final ChannelHandlerContext ctx, Object msg) throws Exception {
+        System.out.println(haProxyMessage);


remove this?

eolivelli · 2020-11-24T19:13:13Z

@diegosalvi you have recently implemented this feature on EmailSuccess, can you please take a look?

merlimat · 2020-11-25T01:51:32Z

conf/broker.conf

@@ -55,6 +55,9 @@ advertisedAddress=
 # The Default value is absent, the broker uses the first listener as the internal listener.
 # internalListenerName=

+# Enable or disable the proxy protocol.
+proxyProtocolEnabled=false


This naming can be confusing since we already have a "proxy" for Pulsar protocol

How about change to haProxyProtocolEnabled?

haProxyProtocol looks much better than proxyProtocol.

Ok, sounds good.

codelipenghui · 2020-11-25T08:21:11Z

/pulsarbot run-failure-checks

diegosalvi · 2020-11-25T08:43:35Z

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

        });
    }

+    private ByteBuf encodeProxyProtocolMessage(HAProxyMessage msg) {
+        // Max length of v1 version proxy protocol message is 108
+        ByteBuf out = Unpooled.buffer(108);


Actually maximum record size is 107 by spec:

worst case (optional fields set to 0xff) :```

"PROXY UNKNOWN ffff:f...f:ffff ffff:f...f:ffff 65535 65535\r\n"
=> 5 + 1 + 7 + 1 + 39 + 1 + 39 + 1 + 5 + 1 + 5 + 2 = 107 chars

[...]

The receiver must wait for the CRLF sequence before starting to decode the
addresses in order to ensure they are complete and properly parsed. If the CRLF
sequence is not found in the first 107 characters, the receiver should declare
the line invalid.

I'm using the netty-haproxy-codec directly. I think netty codec already handled this?

I just noticed the max length for v1 is 108 https://github.com/netty/netty/blob/4.1/codec-haproxy/src/main/java/io/netty/handler/codec/haproxy/HAProxyMessageDecoder.java#L37

diegosalvi · 2020-11-25T08:48:29Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarChannelInitializer.java

@@ -118,6 +119,9 @@ protected void initChannel(SocketChannel ch) throws Exception {
            ch.pipeline().addLast("ByteBufPairEncoder", ByteBufPair.ENCODER);
        }

+        if (pulsar.getConfiguration().isHaProxyProtocolEnabled()) {
+            ch.pipeline().addLast(OptionalProxyProtocolDecoder.NAME, new OptionalProxyProtocolDecoder());


I think we should have the possibilitiy to require proxy headers instead of handle them as optional.
Specification itself states:

The receiver MUST be configured to only receive the protocol described in this specification and MUST not try to guess whether the protocol header is present or not. This means that the protocol explicitly prevents port sharing between public and private access.

This is a flag to control if users want to enable the haproxy protocol support. We must provide a way to disable this feature.

Sure! What I was saying is that there isn't a way to REQUIRE proxy header if pulsar.getConfiguration().isHaProxyProtocolEnabled(). Specs states that is MUST expect a proxy header, it isn't optional. There could be another option to handle optional/mandatory

diegosalvi · 2020-11-25T09:06:13Z

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

+                            InetSocketAddress proxyAddress = (InetSocketAddress) inboundChannel.remoteAddress();
+                            String destinationAddress = proxyAddress.getAddress().getHostAddress();
+                            int destinationPort = proxyAddress.getPort();
+                            HAProxyMessage msg = new HAProxyMessage(HAProxyProtocolVersion.V1, HAProxyCommand.PROXY,


netty-haproxy can decode binary v2 version too (but doesn't implements encoding).
We could add another encoding method for v2 protocol and let the configurer choose which use.

For the encoding implementation I've done one for subethamail if you want take a peek. It has been done for testing purposes (so it permits some strange behaviour to check various edge cases and "wrong" headers and can surely be simplified): https://github.com/davidmoten/subethasmtp/blob/master/src/test/java/org/subethamail/smtp/internal/proxy/ProxyProtocolV2HandlerTest.java

It's not a required for this PR, we can support the proxy set the different protocol version in the following PRs.

Sounds good

sijie · 2020-11-30T03:27:33Z

/pulsarbot cherry-pick to branch-2.6

sijie · 2020-11-30T03:27:39Z

/pulsarbot cherry-pick to branch-2.7

### Motivation Currently, if enable the proxy in the pulsar cluster and client connect to the cluster through the proxy, when we get topic stats, the consumer address and producer address are not the real client address. This PR fix this problem by leverage HAProxy proxy protocol since this is a more general approach. more details about the proxy protocol see https://www.haproxy.com/blog/haproxy/proxy-protocol/ https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt ### Modifications Allow enable proxy protocol on proxy or broker. ### Verifying this change Tests added (cherry picked from commit 625627c)

codelipenghui · 2020-12-03T00:38:01Z

/pulsarbot cherry-pick to branch-2.6

Currently, if enable the proxy in the pulsar cluster and client connect to the cluster through the proxy, when we get topic stats, the consumer address and producer address are not the real client address. This PR fix this problem by leverage HAProxy proxy protocol since this is a more general approach. more details about the proxy protocol see https://www.haproxy.com/blog/haproxy/proxy-protocol/ https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt Allow enable proxy protocol on proxy or broker. Tests added (cherry picked from commit 625627c)

Currently, if enable the proxy in the pulsar cluster and client connect to the cluster through the proxy, when we get topic stats, the consumer address and producer address are not the real client address. This PR fix this problem by leverage HAProxy proxy protocol since this is a more general approach. more details about the proxy protocol see https://www.haproxy.com/blog/haproxy/proxy-protocol/ https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt Allow enable proxy protocol on proxy or broker. Tests added (cherry picked from commit 625627c) (cherry picked from commit 3813062)

Support HAProxy proxy protocol.

1a47b3c

codelipenghui requested review from sijie, rdhabalia, merlimat and jiazhai November 24, 2020 17:45

codelipenghui self-assigned this Nov 24, 2020

codelipenghui added this to the 2.8.0 milestone Nov 24, 2020

codelipenghui added the doc-required Your PR changes impact docs and you will update later. label Nov 24, 2020

sijie reviewed Nov 24, 2020

View reviewed changes

Fix comment

89532cd

merlimat reviewed Nov 25, 2020

View reviewed changes

codelipenghui added 2 commits November 25, 2020 10:01

Fix license check.

836beed

Change configuration param name.

0dfdfe0

diegosalvi reviewed Nov 25, 2020

View reviewed changes

sijie added cherry-pick/2.6.3 release/2.6.3 release/2.7.1 and removed cherry-pick/2.6.3 labels Nov 25, 2020

sijie approved these changes Nov 26, 2020

View reviewed changes

sijie merged commit 625627c into apache:master Nov 30, 2020

codelipenghui deleted the penghui/proxy_protocol branch December 2, 2020 09:27

codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Dec 2, 2020

codelipenghui added the cherry-picked/branch-2.6 label Dec 3, 2020

Anonymitaet mentioned this pull request Mar 1, 2021

[Doc] Add descriptions for HAProxy #9759

Merged

Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Mar 3, 2021

crossoverJie mentioned this pull request Mar 31, 2023

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support HAProxy proxy protocol for broker and proxy #8686

Support HAProxy proxy protocol for broker and proxy #8686

codelipenghui commented Nov 24, 2020

sijie Nov 24, 2020

eolivelli commented Nov 24, 2020

merlimat Nov 25, 2020

codelipenghui Nov 25, 2020 •

edited

Loading

sijie Nov 25, 2020

codelipenghui Nov 25, 2020

codelipenghui commented Nov 25, 2020

diegosalvi Nov 25, 2020

codelipenghui Nov 25, 2020

codelipenghui Nov 25, 2020

diegosalvi Nov 25, 2020

codelipenghui Nov 25, 2020

diegosalvi Nov 26, 2020

diegosalvi Nov 25, 2020

codelipenghui Nov 25, 2020

diegosalvi Nov 26, 2020

sijie commented Nov 30, 2020

sijie commented Nov 30, 2020

codelipenghui commented Dec 3, 2020

Support HAProxy proxy protocol for broker and proxy #8686

Support HAProxy proxy protocol for broker and proxy #8686

Conversation

codelipenghui commented Nov 24, 2020

Motivation

Modifications

Verifying this change

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

Documentation

Choose a reason for hiding this comment

eolivelli commented Nov 24, 2020

Choose a reason for hiding this comment

codelipenghui Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codelipenghui commented Nov 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sijie commented Nov 30, 2020

sijie commented Nov 30, 2020

codelipenghui commented Dec 3, 2020

codelipenghui Nov 25, 2020 •

edited

Loading