fix design issues inside ssh.py

[mpenkov]

The functional design in ssh.py was broken.

All other modules share these design characteristics:

• module.open: accepts module-specific keyword parameters
• module.open_uri function: accepts a URI and transport_params dict, function signature common across all modules
• module.open_uri unpacks transport_params dict and passes it to module.open

The SSH submodule, on the other hand, violates these characteristics. ssh.open_uri passes transport_params to ssh.open as-is, without unpacking them. It looks like this snuck into the code in this commit 4e67683 and then further developed more recently in 269c3a2.

This PR brings ssh.py back in line with the common design characteristics shared by other submodules.

[mpenkov]

@mrk-its and @wbeardall Can you please review?

[wbeardall]

Looks good to me, except that the docstring in ssh.py (line 261-262) should change

"""
If ``username`` or ``password`` are specified in *both* the uri and
``transport_params``, ``transport_params`` will take precedence
"""

to

"""
If ``username`` or ``password`` are specified *both* as function arguments 
and in ``connect_kwargs``, ``connect_kwargs`` will take precedence.
"""

[ddelange]

can there be mutable values inside connect_kwargs? if its a simple dict[str, str] or so, a mere connect_kwargs = connect_kwargs.copy() would suffice

[wbeardall]

There can technically be mutable values in connect_kwargs; key_filename could potentially be a list or other mutable iterable (see SSHClient). Using the regular copy should be fine, as I don't think Paramiko will ever modify the provided iterable, but I think I just left it as a deepcopy as a failsafe.

[greg-offerfit]

I have code that sets connect_kwargs["pkey"] as an instance of a Paramiko PKey subclass, as well as connect_kwargs["sock"] to an instance of Paramiko ProxyCommand, so those are likely mutable. They definitely shouldn't change though.

[ddelange]

hypothetically: if you'd want to share a sock across multiple threads (assuming sock is thread-safe), wouldn't deepcopy be unwanted here?

here is such a scenario but with s3. if smart_open would be calling deepcopy on my boto client in every call to open, that would defeat the purpose

[greg-offerfit]

Might be a moot point, paramiko itself isn't thread-safe. The recommended solution is to open a new connection entirely in each thread (so in smart_open implementation terms, ignore the cached connection). So the smart_open implementation already isn't thread-safe.

I would assume, though can't find any documentation, that this implies ProxyCommand is also not thread-safe. Looking at the implementation of ProxyCommand, I'm not even sure what deepcopying it would do - it's basically a wrapper around a spawned subprocess, and I don't know enough about the Python (deep)copy implementation to know if deepcopying would spawn a new instance of the subprocess to give to the deepcopy, or if it'd preserve the reference to the original spawned process and just pass the same subprocess to the deepcopy.

[ddelange]

these fixes were incorporated in #849