-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 wsl connection plugin #9795
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
69e5503
to
5ea8dfa
Compare
5ea8dfa
to
02c2e8a
Compare
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.
hi @rgl thanks for your contribution!!
I have a couple of comments below. :-)
plugins/connection/wsl.py
Outdated
description: | ||
- Run commands or put/fetch files to an existing WSL distribution using wsl.exe CLI via SSH. | ||
- Uses the Python SSH implementation (Paramiko) to connect to the WSL host. | ||
version_added: "10.3.0" |
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.
version_added: "10.3.0" | |
version_added: "10.4.0" |
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.
done. please check it now.
raise AnsibleError( | ||
f'error occurred while writing SSH host keys!\n{to_text(e)}') |
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.
The collection has a convention of 160 chars long lines, so there is no need to break the line here, and this might/will eventually be concatenated in the future.
raise AnsibleError( | |
f'error occurred while writing SSH host keys!\n{to_text(e)}') | |
raise AnsibleError(f'error occurred while writing SSH host keys!\n{to_text(e)}') |
# paramiko 2.2 introduced auth_timeout parameter | ||
if LooseVersion(paramiko.__version__) >= LooseVersion('2.2.0'): | ||
ssh_connect_kwargs['auth_timeout'] = self.get_option('timeout') |
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.
This is not mentioned in the docs for the user. It should have some note about it, like the "banner timeout" below has.
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.
done. please verify.
display.warning('Paramiko ProxyCommand support unavailable. ' | ||
'Please upgrade to Paramiko 1.9.0 or newer. ' | ||
'Not using configured ProxyCommand') |
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.
This should be mentioned in the docs for proxy_command
.
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.
done. please verify.
plugins/connection/wsl.py
Outdated
# ------------------------ | ||
# Inventory: inventory.yml | ||
# ------------------------ | ||
# all: | ||
# children: | ||
# wsl: | ||
# hosts: | ||
# ubuntu: | ||
# ansible_host: 10.0.0.10 | ||
# wsl_distribution: ubuntu | ||
# debian: | ||
# ansible_host: 10.0.0.10 | ||
# wsl_distribution: debian | ||
# vars: | ||
# ansible_connection: community.general.wsl | ||
# ansible_user: vagrant | ||
# wsl_user: wsl |
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.
No need to comment it out, EXAMPLES is kinda free form. The only thing we need is that you add the marker for another YAML document within the same file, ---
on the top of each section/example, as in before line 248.
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.
ah, much better! please check it now.
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.
oh, it seems that comment can only have a single document due to #9795 (comment)
should this change be reverted? or the CI be changed?
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.
done. please verify.
except IOError: | ||
pass # file was not found, but not required to function |
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 have not investigated that in paramiko, but I believe that there might be reasons for an IOError
other than the file not found. The usual exception for that case, in Python, is FileNotFoundError
but you should verify if paramiko makes the distinction.
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.
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 also only copied it from: paramiko_ssh
I guessed this would cover some edge case, so I left it in.
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.
FWIW, that code change was introduced in 2015 by @bcoca in ansible/ansible@b9710b4 anda its still present in ansible 2.18.3.
stderr = b''.join(chan.makefile_stderr('rb', bufsize)) | ||
returncode = chan.recv_exit_status() | ||
|
||
# NB the full english error message is: |
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 wonder if that is not subject to localization settings. If the remote user, or the remote system, sets the language to be something else, like fr_FR
or pt_BR
, would the error message still be in English? I know this is something we usually need to consider in Linux, and given that WSL runs an Ubuntu (or other distro) on top of itself, it might be something to consider as well.
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.
it probably is subject to localization settings, thou, not sure how to handle it, please advise.
with FileLock().lock_file(lockfile, dirname, self.get_option('lock_file_timeout')): | ||
# just in case any were added recently | ||
|
||
self.ssh.load_system_host_keys() | ||
self.ssh._host_keys.update(self.ssh._system_host_keys) | ||
|
||
# gather information about the current key file, so | ||
# we can ensure the new file has the correct mode/owner | ||
|
||
key_dir = os.path.dirname(self.keyfile) | ||
if os.path.exists(self.keyfile): | ||
key_stat = os.stat(self.keyfile) | ||
mode = key_stat.st_mode & 0o777 | ||
uid = key_stat.st_uid | ||
gid = key_stat.st_gid | ||
else: | ||
mode = 0o644 | ||
uid = os.getuid() | ||
gid = os.getgid() | ||
|
||
# Save the new keys to a temporary file and move it into place | ||
# rather than rewriting the file. We set delete=False because | ||
# the file will be moved into place rather than cleaned up. | ||
|
||
with tempfile.NamedTemporaryFile(dir=key_dir, delete=False) as tmp_keyfile: | ||
tmp_keyfile_name = tmp_keyfile.name | ||
os.chmod(tmp_keyfile_name, mode) | ||
os.chown(tmp_keyfile_name, uid, gid) | ||
self._save_ssh_host_keys(tmp_keyfile_name) | ||
|
||
os.rename(tmp_keyfile_name, self.keyfile) |
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.
My Concurrency-fu is a little bit rusty, but I think you might benefit from a finer granularity in the lock here.
I might be wrong, but it doesn't look like load_system_host_keys()
touches the path referred by keyfile
. In that case, you might perhaps lock the file just for the rename
operation, instead of the entire process.
And now that I wrote that, I just remembered that there is an utility function in Ansible called... atomic_move
, but it is not an utility function, it is a method from the AnsibleModule
class :( anyways, it might be interesting taking a look and see if you can get some ideas out of it. :-)
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.
It seems, this loads the entire known hosts file, adds the inventory hosts, then saves it to disk. Something that needs to be done in a single transaction, that is, its there to prevent multiple concurrent executions of this section of the code, which seems fine. Tho, I have no idea weather the same procedure is followed by other apps (e.g. by the ssh
command), so it might not protect against those modifying the known hosts file at the same time.
Indeed, at a first glance, the last part, seems to implement something similar to AnsibleModule.atomic_move
.
This was introduced by @mietzen in #8424.
@mietzen, maybe you can tell us more about this?
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.
It seems, this loads the entire known hosts file, adds the inventory hosts, then saves it to disk. Something that needs to be done in a single transaction, that is, its there to prevent multiple concurrent executions of this section of the code, which seems fine.
Yes this is what it should do.
Tho, I have no idea weather the same procedure is followed by other apps (e.g. by the ssh command), so it might not protect against those modifying the known hosts file at the same time.
Essentially I just refactored the close method of paramiko_ssh.
Indeed, at a first glance, the last part, seems to implement something similar to
AnsibleModule.atomic_move
.
I didn't knew about AnsibleModule.atomic_move
, at the time I wrote that code.
plugins/connection/wsl.py
Outdated
if password_prompt: | ||
if self.become: | ||
become_pass = self.become.get_option('become_pass') | ||
chan.sendall(to_bytes(become_pass, errors='surrogate_or_strict') + b'\n') |
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.
Minor style nit picking. It might be leaner as:
chan.sendall(to_bytes(become_pass, errors='surrogate_or_strict') + b'\n') | |
chan.sendall(to_bytes(become_pass + '\n', errors='surrogate_or_strict')) |
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.
done. please verify.
02c2e8a
to
d3bb34c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@russoz thank you for the review! I've added the suggested changes for the code that is wsl specific, the other ones, I think, should first be applied to the existing what do you think? |
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.
Thanks for your contribution! I'm wondering a bit whether this plugin would be more appropriate to be added to community.windows, mostly because community.windows does have test infrastructure with Windows remotes (community.general does not and does not plan to add any). (I don't know whether their Windows remotes do have WSL installed, though.)
CC @jborean93 @briantist since they know community.windows a lot better.
d3bb34c
to
923da85
Compare
The test
|
This would not be accepted there for a few reasons:
Maybe it would be better to look into proxy command support to proxy the initial Windows SSH connection to the WSL instance and take advantage of the builtin ssh connection plugin. Personally I'm not sure it should even be in this collection and is better off suited towards keeping it in your own but I'll leave that for the maintainers here to decide. |
I am far from being knowledgeable in Windows so forgive me if I'm saying something silly here. But thinking of this again, I cannot help thinking: it looks like the plugin connects to a sshd daemon that is NOT running in the WSL, hence it needs to connect, starts the WSL and then run whatever it is that needs running in there. Wouldn't it be easier to start the ssh service within the WSL (which is an Ubuntu or some other distro) directly? Then it would be a plain old Linux to Linux ssh connection. A quick googling found some posts indicating that is possible but not entirely straightforward, so YMMV. |
It would be even simpler to install Linux right away instead of using WSL in Windows. But in most cases there are some external requirements not under your control that force you to use a specific setup, and I guess there will be some folks who need SSH to run under Windows itself, outside WSL, and who'd like Ansible to connect to WSL inside their Windows. They will have to jump through this extra hoop. From that POV I'm ok with merging this here, since this seems like something that is useful for parts of the community. |
I also tried the install open ssh daemon inside the WSl distribution, but that has more moving parts, and has a major caveat for me, the WSl dist shutdowns after a while (although you can configure it, which is something more to look for), and if it is shutdown, you cannot access the sshd to manage it. The only thing that is always up, is the windows host itself, hence, this PR. If the CI host has all the windows features for WSl requirements (no idea if that works in a github hosted runner; maybe not, as it requires nested virtualization), creating a Ubuntu dist is quite straightforward (thou, it requires downloading a large rootfs tarball, when not cached somewhere). |
Regarding the failing sanity checks: you need to add
to |
923da85
to
76acdd0
Compare
Hi @rgl thanks for the updates made so far. Please note that there are still a number of comments in the code and, there should be some testing associated with new plugins. Writing integration tests for connection plugins is certainly a harder thing to do, so I would suggest you take a look at the existing tests for other connection plugins and try and mimic/adapt their logic to your plugin. |
plugins/connection/wsl.py
Outdated
vars: | ||
- name: become | ||
- name: ansible_become | ||
_wsl_command_use_bash_quoting: |
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 don't like the idea of using a plugin option for this. This is part of the plugin's public interface and will be visible in the documentation.
How about using a special environment variable instead, that you set in runme.sh
? For example, _ANSIBLE_TEST_WSL_CONNECTION_PLUGIN_Waeri5tepheeSha2fae8
. The last 20 characters are a randomly generated password (on my machine), that should avoid any kind of collision with intentionally chosen environment variables. (The name should already do that, it's just to be on the safe side.)
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.
ah, indeed, much better, its now using an environment variable, please check it out :)
0ad7826
to
4eebc5d
Compare
I think this is ready to be reviewed again. I'm not sure about the etiquette here, who should close the open review threads? Whoever open them, or whoever fixed them? I prefer the former, because the reviewer has a chance to verify, and finally close the thread. Please note that I didn't do anything about the review threads that were inherited/copied from the proxmox_pct_remote connection plugin code, as I think they should be fixed in the original plugin, and after that, I would merge the changes here. But, please advise otherwise. |
4eebc5d
to
e0e3f5e
Compare
There's no perfect solution here. Generally I prefer to let reviewers resolve them (for the same reasons as you do), but unfortunately only folks with commit access (and the PR's author) can resolve, so most reviewers are not able to do this. |
1b8890e
to
f12f2ac
Compare
f12f2ac
to
71a2482
Compare
Ditto what @felixfontein wrote.
I think we need to evaluate those reviews based on their own merit - if we agree to adjust, then we go back to |
This comment was marked as outdated.
This comment was marked as outdated.
please review this again. it seems the TODO about paramiko that I've added to the code, really had impact on the failed CI tests above, that means I'm not sure how to handle the paramiko imports and I'm not really sure how to force paramiko to be installed in the tests, please advise. :-) |
@rgl, @felixfontein and @russoz, just my two cents. Now that there are two "remote connection" plugins that share a common codebase, it might be a good idea to migrate all common code into a base class, e.g., I wanted to do this for a |
The test
The test
The test
The test
The test
The test
The test
The test
The test
The test
The test
The test
|
from paramiko.client import SSHClient | ||
from paramiko.pkey import PKey |
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.
These paramiko imports will not work if paramiko is not present. You must only do them if paramiko
is truish:
from paramiko.client import SSHClient | |
from paramiko.pkey import PKey | |
if paramiko: | |
from paramiko.client import SSHClient | |
from paramiko.pkey import PKey |
Also since you only need this for typing, you should only import them if type-checking is done:
from paramiko.client import SSHClient | |
from paramiko.pkey import PKey | |
if t.TYPE_CHECKING and paramiko: | |
from paramiko.client import SSHClient | |
from paramiko.pkey import PKey |
SUMMARY
This adds the
community.general.wsl
connection plugin.This allows Ansible to remotely manage a WSL Distribution by using SSH to connect to a Windows machine, then use
wsl.exe
to execute commands inside a WSL Distribution.This is derived from the existing
community.general.proxmox_pct_remote
connection plugin.ISSUE TYPE
COMPONENT NAME
community.general.wsl
ADDITIONAL INFORMATION
Here's an simple example how to use this. The full example is at the branch at https://github.com/rgl/terraform-libvirt-ansible-windows-example/tree/wsl.
Example inventory:
Example playbook: