Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Pyamqp] Remember Proxy Params #25564

Merged
merged 5 commits into from
Aug 8, 2022
Merged

[Pyamqp] Remember Proxy Params #25564

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e071472
fix to keep proxy params
kashifkhan Aug 4, 2022
841fa4c
async changes for proxy
kashifkhan Aug 5, 2022
5142cbe
unit tests
kashifkhan Aug 5, 2022
e3a2baa
changes
kashifkhan Aug 8, 2022
c7dca9c
more changes
kashifkhan Aug 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/sasl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,11 @@ class SASLWithWebSocket(WebSocketTransport, SASLTransportMixin):
def __init__(self, host, credential, port=WEBSOCKET_PORT, connect_timeout=None, ssl=None, **kwargs):
self.credential = credential
ssl = ssl or True
http_proxy = kwargs.pop('http_proxy', None)
Copy link
Member

@swathipil swathipil Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need to make the same change in async as well. Is there any way to test this? I'm guessing not since we don't have an http_proxy test in EH or a lot of pyamqp tests.
But maybe for now, we can just create a client and check client._handler._auth._transport._http_proxy is set to what we passed in, without actually sending? Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch !!

Can't really test this because of the reasons you outlined (though there are convos about adding some proxy based tests into the pipeline).

The other reason is that just creating a client simply keeps the proxy information inside client._config.http_proxy and that it does properly. Since we lazily create Connections, nothing happens till you do something like create_batch etc. Its then when proxy stuff gets passed around and its truly right as its connecting inside WebSocketTransport that it looks for whats sent in and tries to apply it.

I can add in a unit test in to pyamqp so that we can make sure the behavior is maintained over changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense. one other idea: would a negative test be possible? for ex, if you pass in an invalid http proxy arg, it would raise the correct error since you haven't actually set up a proxy. This would specifically test against the error that you're fixing, which was that it was sending anyway despite http proxy arg being invalid.

self._transport = WebSocketTransport(
host,
port=port,
connect_timeout=connect_timeout,
ssl=ssl,
http_proxy=http_proxy,
**kwargs
)
super().__init__(host, port, connect_timeout, ssl, **kwargs)
Expand Down