-
Notifications
You must be signed in to change notification settings - Fork 192
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.
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
Add the SshFabricTransport
plugin
#6154
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
########################################################################### | ||
# Copyright (c), The AiiDA team. All rights reserved. # | ||
# This file is part of the AiiDA code. # | ||
# # | ||
# The code is hosted on GitHub at https://github.com/aiidateam/aiida-core # | ||
# For further information on the license, see the LICENSE.txt file # | ||
# For further information please visit http://www.aiida.net # | ||
########################################################################### | ||
"""Plugin for transport over SSH (and SFTP for file transfer).""" | ||
|
||
import pathlib | ||
|
||
import paramiko | ||
|
||
from .ssh import SshTransport | ||
|
||
__all__ = ('SshAutoTransport',) | ||
|
||
|
||
class SshAutoTransport(SshTransport): | ||
"""Support connection, command execution and data transfer to remote computers via SSH+SFTP.""" | ||
|
||
_valid_connect_params = [] | ||
_valid_auth_options = [] | ||
|
||
FILEPATH_CONFIG: pathlib.Path = pathlib.Path('~').expanduser() / '.ssh' / 'config' | ||
|
||
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs, key_policy='AutoAddPolicy') | ||
|
||
config_ssh = paramiko.SSHConfig() | ||
|
||
try: | ||
with self.FILEPATH_CONFIG.open() as handle: | ||
config_ssh.parse(handle) | ||
except FileNotFoundError as exception: | ||
raise RuntimeError( | ||
f'Could not determine connection configuration as the `{self.FILEPATH_CONFIG}` does not exist.' | ||
) from exception | ||
except PermissionError as exception: | ||
raise RuntimeError( | ||
f'Could not determine connection configuration as the `{self.FILEPATH_CONFIG}` is not readable.' | ||
) from exception | ||
|
||
if self._machine not in config_ssh.get_hostnames(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any case where the connection would still work when the host is not in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking: it might. And since it will raise in that case anyway, why not give it a shot? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One consideration might be that the user might focus more on the (somewhat unclear) error and miss the very clear warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. One more reason why I decided to use a warning is that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also not critical for me, and I see your point. In fact, now that I think of it, I might already know of a user that had a setup I wasn't familiar with, which might work fine even though the remote isn't specified in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mbercx did you already tried the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yes! I tried 3 different clusters, with and without proxy jump. Also messed with the configuration to make it fail and then fixed it. All seems to work as expected. So I think once we remove |
||
self.logger.warning( | ||
f'The host `{self._machine}` is not defined in SSH config, connection will most likely fail to open.' | ||
) | ||
|
||
config_host = config_ssh.lookup(self._machine) | ||
|
||
self._connect_args = { | ||
'port': config_host.get('port', 22), | ||
'username': config_host.get('user'), | ||
'key_filename': config_host.get('identityfile', []), | ||
'timeout': config_host.get('connecttimeout', 60), | ||
'proxy_command': config_host.get('proxycommand', None), | ||
'proxy_jump': config_host.get('proxyjump', None), | ||
} | ||
|
||
self._machine = config_host['hostname'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in one of my comments above, I'm wondering if we should name the plugin
SshConfigTransport
(entry pointssh_config
), since it takes the information from the~/.ssh/config
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point of it corresponding to the
~/.ssh/config
file, but I don't think it is as obvious to a user. If a user knows nothing about the way these plugins work, but they have the option betweencore.ssh
andcore.ssh_auto
, I think it is safe to say that they will pick the latter if they want the easy experience. In contrast, if the options arecore.ssh
andcore.ssh_config
I think they would guess thatcore.ssh_config
would require more configuration. That is why I would vote forcore.ssh_auto
and just have the docstring explain that it automatically parses the config file. This also makes it less tied to exactly what it is parsing automatically if in the future we want to add more sources of information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, fair enough! Happy to keep auto then. :)