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

[WIP] Make paramiko connection plugin not hang on wrong password #57526

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

@ahlinc
Copy link

@ahlinc ahlinc commented Jun 7, 2019

Fixes #12608

SUMMARY

It uses a mechanism available in Paramiko channel buffers to notify
about new data available for reading. The notification implemented
with threading.Event shared across stdout and stderr buffers. In
the fix the code just waits for the event, which clears on the
paramiko side on any attempt to read data from the buffers.
Part of the code for the fix was taken from Ansible ssh connection
module, maybe this will help to move ssh and paramiko connection
modules to a shared codebase for the sake of feature parity.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

Paramiko connection plugin

ADDITIONAL INFORMATION

N/A

…assword

It uses a mechanism available in Paramiko channel buffers to notify
about new data available for reading. The notification implemented
with threading.Event shared across stdout and stderr buffers. In
the fix the code just waits for the event, which clears on the
paramiko side on any attempt to read data from the buffers.
Part of the code for the fix was taken from Ansible ssh connection
module, may be this will help to move ssh and paramiko connection
modules to a shared code base with support identical features.
@ahlinc ahlinc force-pushed the bugfix/paramiko_sudo_badpass branch from bf735e1 to 5e5c69f Jun 7, 2019
@ahlinc ahlinc changed the title [WIP] Fix for #12608 paramiko connection pluging hung with wrong password [WIP] Fixing for #12608 paramiko connection plugin hung with wrong password Jun 7, 2019
@webknjaz webknjaz self-requested a review Jun 7, 2019

chan.exec_command(cmd)
Copy link
Member

@webknjaz webknjaz Jun 7, 2019

Can this raise socket.timeout?


chan.exec_command(cmd)
if self.become and self.become.expect_prompt():
while not (self._flags['become_success'] or chan.exit_status_ready()):
Copy link
Member

@webknjaz webknjaz Jun 7, 2019

I'd put (self._flags['become_success'] or chan.exit_status_ready()) into a @property for readability so that it'd look like

Suggested change
while not (self._flags['become_success'] or chan.exit_status_ready()):
while not self._cmd_finished:

display.debug("stdout chunk is: %s" % chunk)
b_tmp_stdout += chunk

b_output, b_unprocessed = self._examine_output('stdout', b_tmp_stdout, sudoable)
Copy link
Member

@webknjaz webknjaz Jun 7, 2019

Do you really need this extra var?
Maybe just save stuff there right away?

Suggested change
b_output, b_unprocessed = self._examine_output('stdout', b_tmp_stdout, sudoable)
b_output, b_tmp_stdout = self._examine_output('stdout', b_tmp_stdout, sudoable)


stdout = b''.join(chan.makefile('rb', bufsize))
stderr = b''.join(chan.makefile_stderr('rb', bufsize))

return (chan.recv_exit_status(), no_prompt_out + stdout, no_prompt_out + stderr)
return (chan.recv_exit_status(), b_stdout + stdout, b_stderr + stderr)
Copy link
Member

@webknjaz webknjaz Jun 7, 2019

It probably makes sense to clean-up flags before exiting this method.

b_stderr += b_output
b_tmp_stderr = b_unprocessed

if self._flags['become_prompt']:
Copy link
Member

@webknjaz webknjaz Jun 7, 2019

I don't really like storing flags on the instance level. I think it'd be cleaner if this var was scoped to this method. Or maybe even factor out state store into a separate class (w/ Enum values maybe).

@webknjaz webknjaz changed the title [WIP] Fixing for #12608 paramiko connection plugin hung with wrong password [WIP] Make paramiko connection plugin not hang on wrong password Jun 7, 2019
@ansibot ansibot added pre_azp and removed stale_ci labels Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants