Expose received broker connection close method, in case of any error, to client during handshake #72

Chinmay1412 · 2022-04-06T15:40:39Z

At the moment, if client is not able to connect to broker because of reasons like invalid credentials, port, user or anything else, broker sends the connection Close method to the amqpprox server. And amqpprox immediately shutdowns and closes the sockets for clients. Because of that, client never receives the connection Close method, which is not good for the client. Because the Close method specifies the reason behind the closing the connection abruptly and it would be hard for client to debug any existing problem. So this PR fixes that issue. And it forwards the received connection close method to client. This way client will be able to know the reason of closure during handshake. And then it continues to shutdown and close sockets. Once we have the successful connection, amqpprox will forwards complete traffic to client as usual.

For example, if any client tries to connect to any vhost using wrong credentials, amqpprox will get the connection Close method from the server. And amqpprox will forward this method to client, before shutting down the socket.

Sample received Close method on client side, which describes the correct reason of the disconnection:

Received message: MESSAGE=Connection Close = [replyCode: 403, replyText: "ACCESS_REFUSED - Login was refused using authentication mechanism PLAIN. For details see the broker logfile.", classId: 0, methodId: 0] CHANNEL=0 LEN=119

libamqpprox/amqpprox_session.cpp

willhoy · 2022-04-07T15:26:50Z

libamqpprox/amqpprox_session.cpp

@@ -70,6 +71,20 @@ namespace amqpprox {
 using namespace boost::asio::ip;
 using namespace boost::system;

+namespace {
+void logException(const std::string_view &error,


Minor: I think passing string_view by value might be preferred. It makes essentially zero difference to perf either way & value types have fewer lifetime concerns. But this is just a style comment, happy either way.

willhoy · 2022-04-07T15:32:22Z

tests/amqpprox_session.t.cpp

+
+    // Client  <-------Close------- Proxy  <-------Close--------  Broker
+    testSetupProxyReceiveClose(8);
+


maybe I'm missing something here, but where are we testing that what comes back from the closemethod gets passed on to the client.

So basically this function simulates the method exchanges, when amqpprox receives the close method from server. So basically, first amqpprox starts handshake with client. And when amqpprox receives Open method, amqpprox initiate handshake with actual server and start sending same received frames to server.

When amqpprox sends the StartOk method, amqpprox receives Close method from the server (Inside testSetupProxyReceiveClose function d_clientState.pushItem call) (here one can assume that amqpprox got the Close method because of invalid credentials specified in StartOk method)

So now amqpprox will sends the Close method to client and then shutdown and close sockets for both clients and server. Specified all expectations inside testSetupProxyReceiveClose function d_serverState.expect and d_clientState.expect call.

Here I am testing with default Close method. But I can modify test to use custom Close method with dummy reason and error code to test more thoroughly.

I already ran this test without the core changes. And in that case, test failed, because we were not sending the Close method to client.

willhoy · 2022-04-08T11:16:42Z

tests/amqpprox_session.t.cpp

+    // Client  <--------Close-----  Proxy  <--------Close------  Broker
+    methods::Close receivedClose;
+    receivedClose.setReply(123, "Broker is closing the connection");
+    d_clientState.pushItem(idx, Data(encode(receivedClose)));


shouldn't this be server state?

Basically, here amqpprox works as a client, when it receives Close method from server and later on when it forwards the method to client, it works as a server. So we are using the convention d_clientState to interact with server and d_serverState to interact with client. It is bit confusing. But that is the way all the tests are written. And I think the core code also has the same convention.

willhoy · 2022-04-08T11:17:04Z

tests/amqpprox_session.t.cpp

+        EXPECT_THAT(items, Contains(VariantWith<Call>(Call("shutdown"))));
+        EXPECT_THAT(items, Contains(VariantWith<Call>(Call("close"))));
+    });
+    d_clientState.expect(idx, [this](const auto &items) {


and the expectation is here for the receivedClose?

As I mentioned earlier, this state represents interaction with server. So we will not have that expectation.

adamncasey

Some minor comments

libamqpprox/amqpprox_connector.cpp

adamncasey · 2022-04-08T17:18:13Z

libamqpprox/amqpprox_session.cpp

@@ -70,6 +71,20 @@ namespace amqpprox {
 using namespace boost::asio::ip;
 using namespace boost::system;

+namespace {
+void logException(const std::string_view &error,


Minor: I think passing string_view by value might be preferred. It makes essentially zero difference to perf either way & value types have fewer lifetime concerns. But this is just a style comment, happy either way.

adamncasey

Some comments on the tests - I think a small bit of tidy up would help out a lot here.

adamncasey · 2022-04-12T08:53:34Z

tests/amqpprox_session.t.cpp

+    TestSocketState::State clientBase;
+    clientBase.d_local  = makeEndpoint("1.2.3.4", 32000);
+    clientBase.d_remote = makeEndpoint("3.4.5.6", 5672);
+    clientBase.d_secure = false;


Can this code & above be shared between tests? I feel like this is obscuring what is important in the test

adamncasey · 2022-04-12T08:58:46Z

tests/amqpprox_session.t.cpp

+    // Client  <-------Close------- Proxy  <-------Close--------  Broker
+    testSetupProxyReceiveClose(8);


I think some tweaks would highlight better that this is the most important line in the test. I could also go for moving the contents of testSetupProxyReceiveClose into this method, since it will probably only be used here.

Suggested change

// Client <-------Close------- Proxy <-------Close-------- Broker

testSetupProxyReceiveClose(8);

// This is the important part of the test - ensure that if the broker sends a Close

// we pass this back to the client

// Client <-------Close------- Proxy <-------Close-------- Broker

testSetupProxyForwardsBrokerClose(8);

… to client during handshake

Chinmay1412 requested review from willhoy, adamncasey and vilmars April 7, 2022 15:30

willhoy reviewed Apr 7, 2022

View reviewed changes

willhoy reviewed Apr 8, 2022

View reviewed changes

willhoy approved these changes Apr 8, 2022

View reviewed changes

Chinmay1412 force-pushed the expose-close branch from 71f0992 to e4a5cf6 Compare April 8, 2022 17:08

adamncasey reviewed Apr 8, 2022

View reviewed changes

Chinmay1412 force-pushed the expose-close branch from e4a5cf6 to b190a3b Compare April 8, 2022 18:13

adamncasey reviewed Apr 12, 2022

View reviewed changes

Chinmay1412 force-pushed the expose-close branch from b74ba1b to 5679a65 Compare April 12, 2022 11:31

adamncasey approved these changes Apr 12, 2022

View reviewed changes

Chinmay1412 force-pushed the expose-close branch from 6ac2afe to 642257c Compare April 12, 2022 11:56

Expose received broker connection close method, in case of any error,…

fb6d005

… to client during handshake

Chinmay1412 force-pushed the expose-close branch from 642257c to fb6d005 Compare April 12, 2022 11:57

adamncasey approved these changes Apr 12, 2022

View reviewed changes

Chinmay1412 merged commit 3ead584 into main Apr 12, 2022

Chinmay1412 deleted the expose-close branch April 12, 2022 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose received broker connection close method, in case of any error, to client during handshake #72

Expose received broker connection close method, in case of any error, to client during handshake #72

Chinmay1412 commented Apr 6, 2022

willhoy Apr 7, 2022

adamncasey Apr 8, 2022

willhoy Apr 7, 2022

Chinmay1412 Apr 7, 2022

Chinmay1412 Apr 8, 2022

willhoy Apr 8, 2022

Chinmay1412 Apr 8, 2022

willhoy Apr 8, 2022

Chinmay1412 Apr 8, 2022

adamncasey left a comment

adamncasey Apr 8, 2022

adamncasey left a comment

adamncasey Apr 12, 2022

adamncasey Apr 12, 2022


		// Client <-------Close------- Proxy <-------Close-------- Broker
		testSetupProxyReceiveClose(8);

Expose received broker connection close method, in case of any error, to client during handshake #72

Expose received broker connection close method, in case of any error, to client during handshake #72

Conversation

Chinmay1412 commented Apr 6, 2022

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

adamncasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamncasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment