brechtold commented 5 years ago

Added proxy_dict so it would be possible to tunnel an SSH connection through the proxy. _establish_connection has been slightly changed. Usage: pd = {'host': 'proxy_host', 'username': 'proxy_user', 'password': 'proxy_pass', 'known_hosts': None } device = {'username': 'remote_user', 'password': 'remote_pass', 'device_type': 'device_type', 'host': 'remote_host', 'timeout': 60, 'proxy_dict': pd, }

selfuryon commented 5 years ago

Hello! Sorry for a long time answer, I'm on vacation... I didn't know that asyncssh has a tunnel param! It's excellent! Thank you for PR, I will test it several times and after merge it!

brechtold commented 5 years ago

Hello! No problem! Thanks for acceppting. It's my first merge request so please be patient with me. The better solution is even to use a (lookalike) asyncssh object in the netdev.create(param, tunnel=asyncssh_object) wrapper, and rewrite the create function to use the tunnel param in asyncssh.connect(self._connect_params_dict, tunnel=self._tunnel)

class BaseDevice(object): ... def __init__(...tunnel=None): ... self._tunnel = tunnel

` async def _establish_connection(self): ...

initiate SSH connection

    fut = asyncssh.connect(**self._connect_params_dict, tunnel=self._tunnel)`

This way, no additional if/else is needed and the call for the tunnel looks like async with asyncssh.connect('', username='', password='', agent_forwarding=True, known_hosts=None) as conn: async with netdev.create(**param, tunnel=conn) as ios: ... The most important thing is to set agent_forwarding=True for the proxy connection. Without it, the whole solution is useless.

selfuryon commented 5 years ago

Yes, I have looked into asyncssh API and I think the same. So we need to add two things:

Add tunnel params with logic inside
Add all missing params from asyncssh factory function (I found out that I missed many of them since I created my Base class)

Do you want to implement it and make PR?

selfuryon commented 5 years ago

Thank you! There is another problem in docstrings. I use docstring and Sphinx for documentation (http://netdev.readthedocs.io/), so there is a reStructuredText syntax. You deleted some empty strings from it like this (at 25 line): default And It brokes a valid syntax. A broken view: broken

A good view: normal

Can you restore the empty strings?

brechtold commented 5 years ago

I'm sorry for breaking the docstrings. Just to clarify: the only issue here is one newline after the description of BaseClass?

selfuryon commented 5 years ago

No, it occurs in every method... You can check it by running sphinx for building HTML, you need to install Sphinx and sphinx-rtd-theme for it. Or another solution - I can restore it after merging in develop branch

brechtold commented 5 years ago

Ok, i will fix this up this week. One thing that came to my mind: why not move the definition of _connect_params_dict to the init? This way the BadeDevice would not be polluted by a bunch of values which are not needed. With te solution right now 24 out of 26 (self._host and self._timeout are reused)

selfuryon commented 5 years ago

Yeah, right now we have so many parameters, so it makes sense!

selfuryon commented 5 years ago

One note about PR - I use this flow:

Master branch is only for releases
Develop branch is the main branch for development
Every merge from develop to master is release

So the main development occurs in develop branch (sometimes when there is a big or complicated feature - I use a separated branch for this and merge it to develop). Can you make your PR to develop branch? I think that I need a contributing file with a description of it. I will create it soon :)

brechtold commented 5 years ago

Yeah, I will close this PR as soon as I am done with all the changes and do another to the devel branch. Most likely I will be done by Monday, as this week is already pretty packed.

selfuryon commented 5 years ago

That's no problem! I will be awaiting your PR!

selfuryon commented 5 years ago

I'm looking into a new python 3.7 documentation and found out that right now it has asyncio.get_running_loop(), so we don't need a loop argument in __init__() function. There are many places (like this) which have a mention about plans to make it deprecated. I suggest to remove it maybe by this PR or next releases

brechtold commented 5 years ago

I'm looking into a new python 3.7 documentation and found out that right now it has asyncio.get_running_loop(), so we don't need a loop argument in __init__() function. There are many places (like this) which have a mention about plans to make it deprecated. I suggest to remove it maybe by this PR or next releases

I think this change would need careful testing as it's only supported in 3.7. I don't have 3.7 in use anywhere right now, and it will take time to push everything up to 3.7. However, the final solution is up to you, as this is your repo and I just had one idea :) The deprecation mention for the loop argument is for Py3.10, this will take a while.

selfuryon commented 5 years ago

Yeah, maybe you are right. I just planning to create custom executor which will work with many processes and loops in there to gain more massive deployment where it needed. And with using get_running_loop() it becomes more easy to implement. I think I implement it only for python starting from 3.7 when 3.7 became the main version in several distros. Make it in another branch

brechtold commented 5 years ago

I am finally done with everything. I even tried to redo the docs, but to be honest I was not quite sure what I was doing, so I will leave this up to you. sorry. :)

selfuryon commented 5 years ago

@brechtold No problem, I'll fix docs :)

selfuryon / netdev

Update base.py #16

initiate SSH connection