Tunneling Proxy for Non-WebSocket MQTT5 Connection Using Mqtt5ClientBuilder

[elryno]

I have not been able to get a tunneling proxy connection to work using Mqtt5ClientBuilder. Looking through the Build() function shows that the HttpClientConnectionProxyOptions field m_proxyOptions is only used when m_websocketConfig.has_value() returns true. Is this intentional, or is this a bug?

It seems that this would fix the issue....

diff --git a/source/iot/Mqtt5Client.cpp b/source/iot/Mqtt5Client.cpp
index 46aaa8e..091ae0c 100644
--- a/source/iot/Mqtt5Client.cpp
+++ b/source/iot/Mqtt5Client.cpp
@@ -596,10 +596,11 @@ namespace Aws
                 {
                     m_options->WithHttpProxyOptions(m_websocketConfig->ProxyOptions.value());
                 }
-                else if (m_proxyOptions.has_value())
-                {
-                    m_options->WithHttpProxyOptions(m_proxyOptions.value());
-                }
+            }
+
+            if (m_proxyOptions.has_value())
+            {
+                m_options->WithHttpProxyOptions(m_proxyOptions.value());
             }
 
             return Crt::Mqtt5::Mqtt5Client::NewMqtt5Client(*m_options, m_allocator);

Can you confirm? If so, should I submit a PR?

[bretambrose]

You are correct. But from a full backwards compatibility standpoint, the new check needs to also confirm that m_options->m_proxyOptions does not have a value set yet.

[elryno]

There does not appear to be a simple way to check if m_options->m_proxyOptions is already set, as the current code does not check this. The only code path where this gets set is when m_websocketConfig.has_value() is true, so set it if m_proxyOptions.has_value() is true and m_websocketConfig.has_value() is false

Does this work?

diff --git a/source/iot/Mqtt5Client.cpp b/source/iot/Mqtt5Client.cpp
index 46aaa8e..94af027 100644
--- a/source/iot/Mqtt5Client.cpp
+++ b/source/iot/Mqtt5Client.cpp
@@ -601,6 +601,10 @@ namespace Aws
                     m_options->WithHttpProxyOptions(m_proxyOptions.value());
                 }
             }
+            else if (m_proxyOptions.has_value())
+            {
+                m_options->WithHttpProxyOptions(m_proxyOptions.value());
+            }
 
             return Crt::Mqtt5::Mqtt5Client::NewMqtt5Client(*m_options, m_allocator);
         }

[bretambrose]

I would just add a local boolean indicator that's true if it's been set earlier in the function

[elryno]

OK, I updated the PR with our suggestion.

[elryno]

I submitted a PR to fix this bug. #644

[jmklix]

merged in this PR: #647