Fix issue #23774 Make transport_test use exec #23775

smikulcik · 2017-04-19T23:11:50Z

SUMMARY

Since ssh may re-route traffic to any number of places before it
reaches a host by the ssh config files and proxy hosts, we cannot use
sockets to test our transport.

It would make sense to use the same mechanism that we use to
perform actions on a host as we would to verify connectivity.

This change performs a dummy exec_command on the host to verify
that its transport works instead of using a single socket.

In particular, this fixes #23774

By using the ssh configuration built into exec_command, we are able to
handle proxy jumps specified in ssh config files. This allows us to handle
bastion hosts when using wait_for_connection

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

wait_for_connection
ansible.plugins.connection.accelerate
ansible.plugins.connection.ssh
ansible.plugins.connection.paramiko_ssh
ansible.plugins.connection.winrm

ANSIBLE VERSION

ansible 2.4.0
  config file = 
  configured module search path = [u'/Users/smikulc/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/smikulc/virtualenvs/ansible/lib/python2.7/site-packages/ansible-2.4.0-py2.7.egg/ansible
  executable location = /Users/smikulc/virtualenvs/ansible/bin/ansible
  python version = 2.7.10 (default, Jul 30 2016, 19:40:32) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.34)]

ADDITIONAL INFORMATION

For testing purposes, here is a demo that illustrates the bug fixed in this change https://github.com/smikulcik/ansible-waitforconnection-bug-demo

dagwieers

We wanted to avoid doing a full roundtrip using the protocol as it is more expensive than a single TCP test. So maybe we should modify the default delay for these tests.

We could also get rid of the transport_test altogether, and only do the real remote module run.

dagwieers · 2017-04-20T12:44:14Z

lib/ansible/plugins/connection/accelerate.py

-        display.vvv("attempting transport test to %s:%s" % (host, port))
-        sock = socket.create_connection((host, port), connect_timeout)
-        sock.close()
+        display.vvv("attempting transport test to %s" % self.host)


Fails because self_host is not defined for accelerate transport.

dagwieers · 2017-04-20T12:44:25Z

lib/ansible/plugins/connection/accelerate.py

-        sock.close()
+        display.vvv("attempting transport test to %s" % self.host)
+        self.reset()
+        self.exec_command("exit")


I am not sure if exit is an accepted command.

dagwieers · 2017-04-20T12:44:56Z

lib/ansible/plugins/connection/paramiko_ssh.py

-        display.vvv("attempting transport test to %s:%s" % (host, port))
-        sock = socket.create_connection((host, port), connect_timeout)
-        sock.close()
+        display.vvv("attempting transport test to %s" % self.host)


Here also self.host is not defined.

dagwieers · 2017-04-20T12:45:26Z

lib/ansible/plugins/connection/winrm.py

-        display.vvv("attempting transport test to %s:%s" % (host, port))
-        sock = socket.create_connection((host, port), connect_timeout)
-        sock.close()
+        display.vvv("attempting transport test to %s" % self.host)


Again self_host is not defined for winrm.

dagwieers · 2017-04-20T12:46:21Z

lib/ansible/plugins/connection/winrm.py

-        sock.close()
+        display.vvv("attempting transport test to %s" % self.host)
+        self.reset()
+        self.exec_command("exit")


I would probably go for "Exit" here, but that's more cosmetic than functional.

Some ansible hosts are not directly reachable via socket connections. It is best to use the transport mechanism itself than to assume that the host is reachable via socket. If we can exec a command, then the transport is working.

smikulcik · 2017-04-20T16:52:32Z

@dagwieers Thanks for the feedback. What would you recommend for configuring connect_timeout? Currently, the exec_command uses the same timeout from the self._play_context.timeout, this code is ignoring anything set for the connect_timeout.

dagwieers · 2017-04-20T23:11:37Z

@smikulcik Good question, I don't know what we do best in this case. Maybe we should discuss this in the core meeting.

dagwieers · 2017-04-20T23:14:17Z

The integration test found a weird issue related to our code:

timed out waiting for connection port up: list index out of range

Needs investigation.

dagwieers · 2017-12-04T12:05:39Z

Beware that the deadline for getting new features/modules accepted in Ansible v2.5 is nearing, it is set to either 2018-01-15 or 2018-01-31. If you are blocked, or you need feedback, please discuss on IRC channel #ansible-windows or add a comment to [the Windows Working Group meeting agenda]

ansibot · 2018-01-05T12:37:40Z

cc @jborean93 @nitzmahone
click here for bot help

dagwieers · 2018-01-05T12:41:22Z

I still think this would be a useful addition, the transport test was disabled so it would still work over proxied connections, but I prefer keeping this around.

ShachafGoldstein · 2019-06-05T20:58:33Z

@dagwieers @jborean93 @nitzmahone - This one seems dead for quite a while, still relevant?

jborean93 · 2019-07-22T06:56:53Z

Considering the age and merge conflicts I am going to close this PR. If this is still a problem please feel free to open a new PR that has been rebased.

dagwieers suggested changes Apr 20, 2017

View reviewed changes

Fix issue ansible#23774 Make transport_test use exec

12d133c

Some ansible hosts are not directly reachable via socket connections. It is best to use the transport mechanism itself than to assume that the host is reachable via socket. If we can exec a command, then the transport is working.

smikulcik force-pushed the smikulcik-exec_transport_test branch from 7fde1c9 to 12d133c Compare April 20, 2017 16:43

s-hertel removed the needs_triage Needs a first human triage before being processed. label Apr 21, 2017

gundalow added the ci_verified Changes made in this PR are causing tests to fail. label Apr 22, 2017

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 23, 2017

ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Sep 1, 2017

ansibot added plugins/connection/paramiko_ssh plugins/connection/ssh plugins/connection/winrm labels Sep 9, 2017

ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017

ansibot removed the new_contributor This PR is the first contribution by a new community member. label Nov 3, 2017

ansibot added the new_contributor This PR is the first contribution by a new community member. label Jan 29, 2018

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 2, 2018

ansibot added the new_plugin This PR includes a new plugin. label May 21, 2018

ShachafGoldstein mentioned this pull request Jun 5, 2019

Windows Working Group agenda - 2019 ansible/community#420

Closed

jborean93 closed this Jul 22, 2019

ansible locked and limited conversation to collaborators Aug 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #23774 Make transport_test use exec #23775

Fix issue #23774 Make transport_test use exec #23775

smikulcik commented Apr 19, 2017 •

edited

dagwieers left a comment

dagwieers Apr 20, 2017

dagwieers Apr 20, 2017

dagwieers Apr 20, 2017

dagwieers Apr 20, 2017

dagwieers Apr 20, 2017

smikulcik commented Apr 20, 2017

dagwieers commented Apr 20, 2017

dagwieers commented Apr 20, 2017

dagwieers commented Dec 4, 2017

ansibot commented Jan 5, 2018

dagwieers commented Jan 5, 2018

ShachafGoldstein commented Jun 5, 2019

jborean93 commented Jul 22, 2019

Fix issue #23774 Make transport_test use exec #23775

Fix issue #23774 Make transport_test use exec #23775

Conversation

smikulcik commented Apr 19, 2017 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

dagwieers left a comment

Choose a reason for hiding this comment

dagwieers Apr 20, 2017

Choose a reason for hiding this comment

dagwieers Apr 20, 2017

Choose a reason for hiding this comment

dagwieers Apr 20, 2017

Choose a reason for hiding this comment

dagwieers Apr 20, 2017

Choose a reason for hiding this comment

dagwieers Apr 20, 2017

Choose a reason for hiding this comment

smikulcik commented Apr 20, 2017

dagwieers commented Apr 20, 2017

dagwieers commented Apr 20, 2017

dagwieers commented Dec 4, 2017

ansibot commented Jan 5, 2018

dagwieers commented Jan 5, 2018

ShachafGoldstein commented Jun 5, 2019

jborean93 commented Jul 22, 2019

smikulcik commented Apr 19, 2017 •

edited