Issue #336, ssh_config_file and ssh_known_hosts_file True behaviour with system transport #337
Conversation
| @@ -613,16 +618,19 @@ def _resolve_ssh_config(self, ssh_config_file: str) -> str: | |||
| N/A | |||
|
|
|||
| """ | |||
| self.logger.debug("attempting to resolve 'ssh_config_file' file") | |||
| self.logger.debug(f"attempting to resolve 'ssh_config_file' file {ssh_config_file}") | |||
There was a problem hiding this comment.
nice, that seems like an obviously better thing to log 😁
scrapli/driver/base/base_driver.py
Outdated
| resolved_ssh_config_file = str(Path("/etc/ssh/ssh_config")) | ||
| if ssh_config_file == "" and transport == "system": | ||
| resolved_ssh_config_file = BaseTransport.SSH_SYSTEM_CONFIG_MAGIC_STRING | ||
| else: |
There was a problem hiding this comment.
haven't looked too closely yet, but can we maybe "flatten" this out a bit if possible? guard clause or something -- like if this thing return, then no extra indented else block (hopefully that makes sense)
There was a problem hiding this comment.
Good point, flat is indeed better, should be better now!
scrapli/driver/base/base_driver.py
Outdated
| resolved_ssh_known_hosts = str(Path("/etc/ssh/ssh_known_hosts")) | ||
| if ssh_known_hosts == "" and transport == "system": | ||
| resolved_ssh_known_hosts = BaseTransport.SSH_SYSTEM_CONFIG_MAGIC_STRING | ||
| else: |
There was a problem hiding this comment.
same "flatten-y" comment here (if possible/easy, if not we can ignore -- and sorry just looking quickly so maybe this is a bad comment!)
There was a problem hiding this comment.
Changed it the same way as above :-)
| @@ -23,6 +23,8 @@ class BasePluginTransportArgs: | |||
|
|
|||
|
|
|||
| class BaseTransport(ABC): | |||
| SSH_SYSTEM_CONFIG_MAGIC_STRING: str = "__SCRAPLI_RULES__" | |||
There was a problem hiding this comment.
lol, while I love and appreciate this placeholder string... lets do two things:
- I think this is really only ever going to be relevant for system transport right? if "yes", maybe we should put the it in the system transport package?
- hah scrapli does rule, but maybe we call it something more descriptive for the variable and the actual string -- like SYSTEM_TRANSPORT_SSH_CONFIG_TRUE: str = "SYSTEM_TRANSPORT_SSH_CONFIG_TRUE" lol so much less fun but probably more understandable :P
There was a problem hiding this comment.
Yeah probably not the best place to drop a compliment, but hey 😆 I changed it to what you suggested!
|
|
||
| if self.plugin_transport_args.ssh_config_file: | ||
| self.open_cmd.extend(["-F", self.plugin_transport_args.ssh_config_file]) | ||
| if ( |
There was a problem hiding this comment.
two things here:
-
if possible and we can flatten things a bit, lets do that :P (sorry for nit picks but flat is good :D)
-
should we have a second magic string for this since users could set known hosts -> True but not ssh config file? (I think? its been a while, maybe im wrong about that too hah)
There was a problem hiding this comment.
Flattened out, and good point that we should add a separate variable for the known_hosts_file ( I was being lazy 😁 ) added it!
| ) | ||
| def test_setup_ssh_file_args_resolved(fs_, base_driver, test_data): | ||
| def test_setup_ssh_file_args_resolved(fs_, test_data, base_driver): |
There was a problem hiding this comment.
🔥 nice, thank you for adding test cases!
|
boom, LGTM, thanks a bunch @bennnnnnnn !! |
Description
See issue #336, when passing True to the ssh_config_file and using the system transport, you now effectively get the same behaviour as the regular ssh client (no -F args are passed). That means that both the user and the system ssh config are respected. I also implemented the same change for the known_hosts_file.
Type of change
How Has This Been Tested?
Added necessary tests in test_base_base_driver.py, did a real-world test against a device
Checklist:
make lintbeforecommitting!)
docstrings include a summary, args, returns and raises fields (even if N/A)
note if there are any considerations for the vrnetlab setup