search

Polyconseil / aioamqp

AMQP implementation using asyncio
Other
280 stars 88 forks source link

support amqps:// to enable SSL connection #213

Closed jonnyyu closed 2 years ago

jonnyyu commented 4 years ago

from_url() treat amqps as a valid scheme, but it doesn't set ssl argument. This becomes inconvenient when you want to enable SSL via URL parameter.

This PR will check for amqps:// scheme and set ssl argument with default SSL context when ssl argument is not specified explicitly.

Added UT to verify ssl works in both situation.

dzen commented 4 years ago

I'm not fand of setting ssl=True since it should be an ssl context not a boolean. The ssl kwarg is given to create_connection method which eventually takes a boolean but it will create a ssl_context under the hood which I think we should create as a default.

I don't think the from_url() is a good API since it does not allow to fine tune the connection

jonnyyu commented 4 years ago

@dzen I changed it to ssl.create_default_context(), please review again. thx!

NotoriousRebel commented 3 years ago

Any updates on this? @jonnyyu

jonnyyu commented 3 years ago

Hi @NotoriousRebel, I'm still waiting @dzen review my changes. In the meantime, I'm using the following workaround:

ssl = urlparse(uri).scheme == 'amqps'
await aioamqp.from_url(uri, ssl=ssl)
RemiCardona commented 2 years ago

Many thanks this PR, I've taken the libery to rebase it and slightly adapt it to recently merged changes.