[WIP]Fix: Paramiko ssh config support #3032
Conversation
Current coverage is 77.09% (diff: 68.52%)@@ master #3032 diff @@
==========================================
Files 430 430
Lines 21997 22049 +52
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 16929 16998 +69
+ Misses 5068 5051 -17
Partials 0 0
|
|
Thanks - changes look good to me now. Please also add changelog entry with appropriate attribution and make corresponding docs change. |
| cfg.BoolOpt('use_ssh_config', | ||
| default=False, | ||
| help='Use the .ssh/config file. Useful to override ports etc.') | ||
| cfg.StrOpt('ssh_config_path', |
There was a problem hiding this comment.
Wait - so do we want two config options or not - use_ssh_config and ssh_config_path?
If we only have one (ssh_config_path), we should default it to None or similar for backward compatibility. Another option is to have two options, default use_ssh_config to False (same as before) and then we can leave default value for ssh_config_path as is.
There was a problem hiding this comment.
Oops, my bad, I may have deleted the use_ssh_config while sorting out the merge conflicts. Fix on its way.
humblearner
changed the title
| @@ -579,13 +579,42 @@ def _connect(self, host, socket=None): | |||
| if socket: | |||
| conninfo['sock'] = socket | |||
|
|
|||
| if cfg.CONF.ssh_runner.use_ssh_config: | |||
| conninfo_host_ssh_config = self._get_conninfo_from_ssh_config_for_host(host) | |||
There was a problem hiding this comment.
Overall the PR looks good to me, but to make debugging easier, please add a log statement which says something along the lines of "Using SSH config file from ".
And perhaps also log which options (with values) are overridden from the config file.
| @@ -579,13 +579,42 @@ def _connect(self, host, socket=None): | |||
| if socket: | |||
| conninfo['sock'] = socket | |||
|
|
|||
| if cfg.CONF.ssh_runner.use_ssh_config: | |||
| conninfo_host_ssh_config = self._get_conninfo_from_ssh_config_for_host(host) | |||
| conninfo.update(conninfo_host_ssh_config) | |||
There was a problem hiding this comment.
Actually, now that I think of it - is the current behavior the desired one?
It looks like currently values from the config have precedence over other values (provided as action parameters)? It seems like values which are provided as parameters should have precedence over the ones provided in the config.
In any case, it's something we need to document and have good tests for so there will be no surprises.
There was a problem hiding this comment.
That is correct @Kami. If we have use_ssh_config = True, values from config have precedence over action's parameters. There is one more caveat: core.remote fails with hosts=localhost.
| @@ -80,8 +79,7 @@ def test_fail_set_proxycommand(self, mock_ProxyCommand): | |||
| cfg.CONF.set_override(name='use_ssh_config', | |||
| override=True, group='ssh_runner') | |||
|
|
|||
| conn_params = {'hostname': 'dummy.host.org', | |||
| 'username': 'ubuntu'} | |||
There was a problem hiding this comment.
Why was username removed?
| if 'user' in user_config: | ||
| ssh_conn_info['username'] = user_config['user'] | ||
| else: | ||
| raise Exception('User directive missing for host: %s' % ssh_conn_info['hostname']) |
There was a problem hiding this comment.
This is not right. There can be an entry without a user field. It's a valid SSH entry.
| @@ -603,15 +606,22 @@ def _get_conninfo_from_ssh_config_for_host(host): | |||
| user_config_file, e.errno, e.strerror)) | |||
|
|
|||
| user_config = ssh_config.lookup(host) | |||
| for k in ('hostname', 'username', 'port'): | |||
There was a problem hiding this comment.
s/'username'/'user' would have been just fine.
| if 'user' in user_config: | ||
| ssh_conn_info['username'] = user_config['user'] | ||
| else: | ||
| raise Exception('User directive missing for host: %s' % ssh_conn_info['hostname']) | ||
| if 'proxycommand' in user_config: | ||
| ssh_conn_info['sock'] = paramiko.ProxyCommand(user_config['proxycommand']) | ||
|
|
||
| if 'identityfile' in user_config: |
There was a problem hiding this comment.
Can you check if we handle case mixup in ssh config file?
…-support Conflicts: CHANGELOG.rst
| logger = logging.getLogger("ParamikoSSHClient") | ||
|
|
||
| # No Action params. | ||
| if conninfo['username'] == cfg.CONF.system_user.user\ |
There was a problem hiding this comment.
This method seems unnecessary complex to me.
I was envisioning something along the lines of having a method which retrieves parameters from ssh config file and then basically doing something like this:
conn_info = {}
# 1. Retrieve parameters from ssh config
conn_info = self._get_parameters_from_ssh_config()
# 2. Add in arguments which are passed to the constructor. Those are action and runner parameters which have precedence over ones in the config
conn_info['username'] = username
...
client.connect(**conn_info)This seems sufficient to me and we don't need any complex if statements, etc. If I'm missing / misunderstanding something, please let me know.
No description provided.