gemmellr commented on code in PR #5908:
URL: https://github.com/apache/activemq-artemis/pull/5908#discussion_r2329802995
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/TransportConstants.java:
##########
@@ -186,26 +188,28 @@ public class TransportConstants {
public static final int STOMP_DEFAULT_CONSUMER_WINDOW_SIZE = 10 * 1024; //
10K
- public static final String PROXY_ENABLED_PROP_NAME = "socksEnabled";
+ public static final String SOCKS_ENABLED_PROP_NAME = "socksEnabled";
- public static final String PROXY_HOST_PROP_NAME = "socksHost";
+ public static final String SOCKS_HOST_PROP_NAME = "socksHost";
- public static final String PROXY_PORT_PROP_NAME = "socksPort";
+ public static final String SOCKS_PORT_PROP_NAME = "socksPort";
public static final String PROXY_VERSION_PROP_NAME = "socksVersion";
- public static final String PROXY_USERNAME_PROP_NAME = "socksUsername";
+ public static final String SOCKS_USERNAME_PROP_NAME = "socksUsername";
- public static final String PROXY_PASSWORD_PROP_NAME = "socksPassword";
+ public static final String SOCKS_PASSWORD_PROP_NAME = "socksPassword";
- public static final String PROXY_REMOTE_DNS_PROP_NAME = "socksRemoteDNS";
+ public static final String SOCKS_REMOTE_DNS_PROP_NAME = "socksRemoteDNS";
public static final String AUTO_START = "autoStart";
public static final boolean DEFAULT_AUTO_START = true;
public static final boolean DEFAULT_SSL_ENABLED = false;
+ public static final boolean DEFAULT_PROXY_ENABLED = false;
Review Comment:
Similarly
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -184,7 +208,9 @@ public void channelRead(ChannelHandlerContext ctx, Object
msg) throws Exception
ctx.writeAndFlush(new DefaultFullHttpResponse(HTTP_1_1,
FORBIDDEN)).addListener(ChannelFutureListener.CLOSE);
}
} else {
- super.channelRead(ctx, msg);
+ // slice off the proxy-related bytes that have already been read
so other protocol handlers don't choke on them
+ super.channelRead(ctx, skipProxyBytes ? ((ByteBuf) msg).slice() :
msg);
+ skipProxyBytes = false;
Review Comment:
Any way this can be done once whilst servicing the proxy message, rather
than needing this to happen for every other read?
If not, could it use an if to make it clearer its a one-off, and to avoid
repeat setting the value false every time?
##########
docs/user-manual/versions.adoc:
##########
@@ -20,6 +20,10 @@
https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12315920&versio
=== Highlights
* Broker observability has improved with the new ability to export
xref:metrics.adoc#executor-services[metrics for executor services] (aka thread
pools).
+* The
https://github.com/haproxy/haproxy/blob/master/doc/proxy-protocol.txt[PROXY
Protocol] is now supported!
+This means that clients connecting through an applicable reverse proxy (e.g.
HAProxy, nginx, etc.) will have the original client IP address reflected in the
logs and in the web console rather than that of the proxy itself.
+Furthermore, this support is 100% transparent and requires no additional
configuration.
Review Comment:
Needs removed.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/view/ConsumerField.java:
##########
@@ -33,6 +33,8 @@ public enum ConsumerField {
CLIENT_ID("clientID"),
LOCAL_ADDRESS("localAddress"),
REMOTE_ADDRESS("remoteAddress"),
+ PROXY_ADDRESS("proxyAddress"),
+ PROXY_VERSION("proxyVersion"),
Review Comment:
Would it be sufficient just to have this on the connection, and people can
cross reference it there if truly needed?
Given how many connections wont have this populated at all (every currently
existing connection), and the ones that do will likely have the same value for
every connection/session/producer/consumer, it seems somewhat
redundant/unnecessary and potentially wasteful to include it for every
session/producer/consumer. Its could also be a bit unclear to some users given
that it wont ever be populated for any non-proxy-protocol proxying use cases,
again every proxying scenario connection that currently exists for Artemis
users.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/TransportConstants.java:
##########
@@ -36,6 +36,8 @@ public class TransportConstants {
public static final String SSL_ENABLED_PROP_NAME = "sslEnabled";
+ public static final String PROXY_ENABLED_PROP_NAME = "proxyEnabled";
Review Comment:
I'd go with the more specific 'proxy protocol', e.g _proxyProtocolEnabled_
(or even just _proxyProtocol).
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java:
##########
@@ -145,6 +146,8 @@ public class NettyAcceptor extends AbstractAcceptor {
private final boolean sslEnabled;
+ private final boolean proxyEnabled;
Review Comment:
Similar comments for this class on _proxyProtocolEnabled_
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -147,11 +156,26 @@ public void channelInactive(ChannelHandlerContext ctx)
throws Exception {
timeoutFuture.cancel(true);
timeoutFuture = null;
}
+ if (proxyAttributesSet) {
+ ctx.channel().attr(PROXY_PROTOCOL_SOURCE_ADDRESS).remove();
+ ctx.channel().attr(PROXY_PROTOCOL_SOURCE_PORT).remove();
+ ctx.channel().attr(PROXY_PROTOCOL_DESTINATION_ADDRESS).remove();
+ ctx.channel().attr(PROXY_PROTOCOL_DESTINATION_PORT).remove();
+ ctx.channel().attr(PROXY_PROTOCOL_VERSION).remove();
+ }
}
@Override
public void channelRead(ChannelHandlerContext ctx, Object msg) throws
Exception {
- if (msg instanceof FullHttpRequest httpRequest) {
+ if (msg instanceof HAProxyMessage haProxyMessage) {
Review Comment:
Related to prior comment later in this method...could another handler be
used for this one-off message, that removes itself on completion. That way it
wouldnt need to check every time through here for something thats not expected
or allowed after the protocol traffic starts?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
