Skip to content
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 support for Windows hosts in the SSH connection plugin #47732

Merged
merged 16 commits into from
Mar 8, 2019

Conversation

jborean93
Copy link
Contributor

@jborean93 jborean93 commented Oct 29, 2018

SUMMARY

2nd attempt at implementing Windows support with the SSH connection plugin. Basic tests indicate exec, fetch, and copy all work fine over SSH.

Known issues:

  • The piped transfer method does not work, it uses the dd command which Windows does not support
  • Fetch does not work with network files, this seems to be a bug with powershell.py shell plugin that strips out the leading \\ chars in the network path
  • Probably a lot of other things

Very basic timing checks, ssh is still roughly .2 seconds slower than the winrm plugin on a win_ping adhoc command but exponentially faster with file copy operations as expected.

Fixes #25344

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ssh

ANSIBLE VERSION
devel

@jborean93 jborean93 changed the title Add support for Windows hosts in the SSH connection plugin WIP Add support for Windows hosts in the SSH connection plugin Oct 29, 2018
@ansibot
Copy link
Contributor

ansibot commented Oct 29, 2018

Hi @jborean93, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Oct 29, 2018

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community labels Oct 29, 2018
@jborean93 jborean93 mentioned this pull request Oct 29, 2018
@ansibot

This comment has been minimized.

@ansibot
Copy link
Contributor

ansibot commented Oct 30, 2018

@ansibot ansibot added the test This PR relates to tests. label Oct 30, 2018
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Nov 1, 2018
@mattclay
Copy link
Member

mattclay commented Nov 6, 2018

@jborean93 We'll want additional logic in the ansible-test change classification code to handle this.

We already have this logic:

integration_name = 'connection_%s' % name
if integration_name not in self.integration_targets_by_name:
integration_name = None

We probably need to add something like this:

            windows_integration_name = 'connection_windows_%s' % name

            if windows_integration_name not in self.integration_targets_by_name:
                windows_integration_name = None

Then add it to the return values here:

return {
'integration': integration_name,
'units': units_path,
}

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 14, 2018
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Dec 4, 2018
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 14, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 26, 2018
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 23, 2019
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 1, 2019
@ansibot
Copy link
Contributor

ansibot commented Mar 1, 2019

@ansibot ansibot added docs This issue/PR relates to or includes documentation. new_plugin This PR includes a new plugin. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 1, 2019
@matsmcp
Copy link

matsmcp commented Mar 4, 2019

I new to Ansible but not to Windows or scripting.
Requiring Powershell as defaultshell has a good chance to break existing scripts that uses built in commands from CMD.exe.

It shoudn't be that much job to rewrite thoose scripts though and as I understand it WINRM could still be used if a rewrite isn't doable.

For me the main benefit with SSH transport is the possibility to use jump/bastion-hosts to reach the systems I want to manage. Can I live with having powershell as defaultshell to get that capability - H-ll Yes !
Therefore I would like to wote for shipping with the documented limitation that the module currently requires Powershell as shell

@jborean93
Copy link
Contributor Author

@matsmcp the latest commit adds support for the default shell choice of cmd. So this can now work with both cmd and PowerShell as the DefaultShell. You just need to set ansible_shell_type: cmd or ansible_shell_type: powershell based ok what the Windows host is currently configured with. This should be reflected in the docs that have been added.

Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one suggestion about setting the shell type in inventory, otherwise looking good. Code itself is ready to merge IMO.

Some wordsmithing and spelling/grammar tweaks needed needed in the docs, but I'll let @acozine do her thing with that, as she may want reorg or changes anyway.

docs/docsite/rst/user_guide/windows_faq.rst Show resolved Hide resolved
@matsmcp
Copy link

matsmcp commented Mar 5, 2019

@jborean93

That sounds great.

@nitzmahone nitzmahone assigned acozine and unassigned acozine Mar 5, 2019
@nitzmahone nitzmahone requested a review from acozine March 5, 2019 17:27
Copy link
Contributor

@acozine acozine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple of commits with most of my docs change ideas. I forgot the changelog fragment, though, so adding that here.

changelogs/fragments/windows-ssh.yaml Outdated Show resolved Hide resolved
Co-Authored-By: jborean93 <jborean93@gmail.com>
@nitzmahone nitzmahone merged commit 8ef2e6d into ansible:devel Mar 8, 2019
@nitzmahone
Copy link
Member

unrelated SSL failure on Ubuntu

@jborean93 jborean93 deleted the win-ssh branch March 8, 2019 00:39
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Powershell over SSH
7 participants