ARTEMIS-3915 Support PROXY protocol

[jbertram]

No description provided.

[jbertram]

FYI - test-suite is good on this.

[gemmellr]

I havent had a chance to look properly at this after all the changes yet...but skimming the tests, I only see spotted positive tests where it was expected to work and did. We should have negative tests for this, verifying connecting doesnt work when proxyProtocolEnabled=true and a connection connects without the header and vice versa fails when proxyProtocolEnabled=false (/defaulted) a connection with the proxy-protocol header comes in.

[jbertram]

I added negative tests as well in org.apache.activemq.artemis.tests.e2e.proxy_protocol.HAProxyTest:

• testNonProxiedConnectionToProxyAcceptor
• testProxiedConnectionToNonProxyAcceptor

Both fail successfully.

[jbertram]

FWIW, the test-suite is green on this.

[tabish121]

I've realized there is a bug here now that this is acting as a shared static handler that likely means you would want to revert to using new instances with this being a ByteToMessageDecoder subclass. The problem would arise on both proxy enabled and not enabled cases but likely is more of an issue in the non-proxy enabled case where the needed bytes don't arrive in the first packet leaded to dropping the first bytes sent on the connection. I created a test case to add to HAProxyTest that shows this:

diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/proxyprotocol/HAProxyTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/proxyprotocol/HAProxyTest.java
index f877dea06b..f15878897e 100644
--- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/proxyprotocol/HAProxyTest.java
+++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/proxyprotocol/HAProxyTest.java
@@ -51,6 +51,8 @@ import org.apache.activemq.artemis.tests.integration.jms.multiprotocol.Multiprot
 import org.apache.activemq.artemis.utils.RandomUtil;
 import org.apache.activemq.artemis.utils.Wait;
 import org.apache.activemq.transport.netty.NettyHAProxyServer;
+import org.apache.qpid.protonj2.test.driver.ProtonTestClient;
+import org.apache.qpid.protonj2.test.driver.codec.security.SaslCode;
 import org.eclipse.paho.mqttv5.client.IMqttToken;
 import org.eclipse.paho.mqttv5.client.MqttCallback;
 import org.eclipse.paho.mqttv5.client.MqttClient;
@@ -61,6 +63,7 @@ import org.eclipse.paho.mqttv5.common.MqttException;
 import org.eclipse.paho.mqttv5.common.MqttMessage;
 import org.eclipse.paho.mqttv5.common.packet.MqttProperties;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -90,7 +93,7 @@ public class HAProxyTest extends MultiprotocolJMSClientTestSupport {
    protected ActiveMQServer createServer() throws Exception {
       server = createServer(false, createDefaultNettyConfig()
          .clearAcceptorConfigurations()
-         .addAcceptorConfiguration("standard", "tcp://127.0.0.1:" + BROKER_STANDARD_PORT + "?protocols=CORE")
+         .addAcceptorConfiguration("standard", "tcp://127.0.0.1:" + BROKER_STANDARD_PORT + "?protocols=CORE,AMQP")
          .addAcceptorConfiguration("proxyEnabled", "tcp://127.0.0.1:" + BROKER_PROXY_PORT + "?proxyProtocolEnabled=true")
          .addAcceptorConfiguration("proxyAndSslEnabled", "tcp://127.0.0.1:" + BROKER_PROXY_SSL_PORT + "?proxyProtocolEnabled=true;sslEnabled=true;protocols=CORE,AMQP,MQTT,OPENWIRE;supportAdvisory=false;suppressInternalManagementObjects=true;keyStorePath=server-keystore.jks;keyStorePassword=securepass"));
 
@@ -331,6 +334,37 @@ public class HAProxyTest extends MultiprotocolJMSClientTestSupport {
       producer.disconnect();
    }
 
+   @Test
+   @Timeout(30)
+   public void testBrokerHandlesOutOfOrderDeliveryIdInTransfer() throws Exception {
+      try (ProtonTestClient receivingPeer = new ProtonTestClient()) {
+         receivingPeer.connect("localhost", BROKER_STANDARD_PORT);
+         receivingPeer.waitForScriptToComplete(5, TimeUnit.SECONDS);
+
+         receivingPeer.expectSASLHeader();
+         receivingPeer.expectSaslMechanisms().withSaslServerMechanism("ANONYMOUS");
+         receivingPeer.remoteSaslInit().withMechanism("ANONYMOUS").queue();
+         receivingPeer.expectSaslOutcome().withCode(SaslCode.OK);
+         receivingPeer.remoteAMQPHeader().queue();
+         receivingPeer.expectAMQPHeader();
+
+         receivingPeer.remoteBytes().withBytes(new byte[] {'A', 'M'}).now();
+         receivingPeer.remoteBytes().withBytes(new byte[] {'Q', 'P', 3, 1, 0, 0}).later(10);
+
+         receivingPeer.waitForScriptToComplete(5, TimeUnit.SECONDS);
+
+         // Broker response after SASL anonymous connect
+         receivingPeer.expectOpen();
+         receivingPeer.expectBegin();
+
+         // Create basic connection with session
+         receivingPeer.remoteOpen().withContainerId("test-sender").now();
+         receivingPeer.remoteBegin().withNextOutgoingId(100).now();
+
+         receivingPeer.waitForScriptToComplete();
+      }
+   }
+
    private int startV1Proxy() {
       return startProxy(HAProxyProtocolVersion.V1, BROKER_PROXY_PORT);
    }

The AMQP Header arrives in fragments which leads the initial bit to get dropped leading to the broker never completing the open phase of the AMQP connection.

This causes me to ask why the enforcer is needed in the negative case and if there's some other simple changes in the protocol handler that could negate that need as it would be nice to not add more overhead to connections that never expect to have a proxy header as I'd assume is how most folks operate. I'd guess in the majority of cases the connection establishment would fail if the proxy message arrives at the front of the connection request.

[jbertram]

Everything will still work without the enforcer. I mainly added it to fail fast based on this feedback. The tests just take longer to run without fa fail-fast mechanism.

[jbertram]

Check out the latest push. I added your test, and I removed the enforcer object for the normal (i.e. proxyProtocolEnabled=false) case although I still check in the ProtocolDecoder so we can fail fast.

[tabish121]

This test name is not right for what its doing, I should have renamed it when I commented about it, maybe

testBrokerHandlesSplitAMQPHeaderBytesDuringConnectWithNoProxyHeader

I started from an existing test and forgot to change the name

[jbertram]

I didn't even look at the name. 😆

Fixed now with your suggestion.