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

Jump to bottom

paho.mqtt.proxy_set() - support #127

Merged
merged 8 commits into from
Sep 1, 2022
Merged

paho.mqtt.proxy_set() - support #127

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
16083f1
Added paho-mqtt tls_set functionality to asyncio-mqtt
Sohaib90 Aug 30, 2022
ed9df64
changed default parameters values for tls_set
Sohaib90 Aug 31, 2022
89aadb4
Added * in TLSParameters __init__
Sohaib90 Aug 31, 2022
a6f3c54
renamed tls_set_params to tls_params
Sohaib90 Aug 31, 2022
3d05f0c
Added proxy_set functionality
Sohaib90 Sep 1, 2022
e92b06f
Merge branch 'master' into proxy_set
Sohaib90 Sep 1, 2022
e3fa156
modified required params for proxy
Sohaib90 Sep 1, 2022
293282f
Merge branch 'proxy_set' of github.com:Sohaib90/asyncio-mqtt into pro…
Sohaib90 Sep 1, 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
24 changes: 23 additions & 1 deletion asyncio_mqtt/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,25 @@ def __init__(
self.ciphers = ciphers
self.keyfile_password = keyfile_password


# Proxy parameters class
class ProxySettings:
def __init__(
self,
*,
proxy_type: int = None,
Copy link
Member

Choose a reason for hiding this comment

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

You forgot the Optional part here. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the description here: https://github.com/eclipse/paho.mqtt.python/blob/225ab3757f6818ba85eb80564948d1c787190cba/src/paho/mqtt/client.py#L875

It says

(Required)
proxy_type: One of {socks.HTTP, socks.SOCKS4, or socks.SOCKS5}
proxy_addr: IP address or DNS name of proxy server
(Optional)
proxy_rdns: boolean indicating whether proxy lookup should be performed
    remotely (True, default) or locally (False)
proxy_username: username for SOCKS5 proxy, or userid for SOCKS4 proxy
proxy_password: password for SOCKS5 proxy

In line with that shouldnt it be like this, or should I change it to Optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also do you think it is a good idea to add some documentation so that the users know that the functionality exists (e.g a small example) in the README.md in another PR?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point. 👍 If those arguments are required then let's not use default valures (= None) for them.
Looking at the paho implementation, we could have a signature like this:

def __init__(
    self,
    *, 
    proxy_type: int,
    proxy_addr: str,
    proxy_rdns: Optional[bool] = True,
    proxy_username: Optional[str] = None,
    proxy_password: Optional[str] = None
): ...

This way, the user must supply the proxy_type and proxy_addr arguments.

Also do you think it is a good idea to add some documentation so that the users know that the functionality exists (e.g a small example) in the README.md in another PR?

Yes, that's a good idea. 👍
Specifically, we should let people know that this whole proxy is an "extra" feature (even in paho) that requires the PySocks dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have fixed it and committed it as well.
Alright. I will work on the documentation and create a PR for that

proxy_addr: str = None,
proxy_rdns: Optional[bool] = True,
proxy_username: Optional[str] = None,
proxy_password: Optional[str] = None
):
self.proxy_args = {
"proxy_type" : proxy_type,
"proxy_addr" : proxy_addr,
"proxy_rdns" : proxy_rdns,
"proxy_username" : proxy_username,
"proxy_password" : proxy_password
}


# See the overloads of `socket.setsockopt` for details.
SocketOption = Union[
Expand Down Expand Up @@ -140,6 +158,7 @@ def __init__(
client_id: Optional[str] = None,
tls_context: Optional[ssl.SSLContext] = None,
tls_params: Optional[TLSParameters] = None,
proxy: Optional[ProxySettings] = None,
protocol: Optional[ProtocolVersion] = None,
will: Optional[Will] = None,
clean_session: Optional[bool] = None,
Expand Down Expand Up @@ -220,6 +239,9 @@ def __init__(
tls_version= tls_params.tls_version,
ciphers= tls_params.ciphers,
keyfile_password= tls_params.keyfile_password)

if proxy is not None:
self._client.proxy_set(**proxy.proxy_args)

if websocket_path is not None or websocket_headers is not None:
self._client.ws_set_options(
Expand Down