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

Fix SSH Timeout PTY allocation #676

Merged
merged 1 commit into from
Apr 14, 2021
Merged

Fix SSH Timeout PTY allocation #676

merged 1 commit into from
Apr 14, 2021

Conversation

clintoncwolfe
Copy link
Contributor

Description

Do not automatically allocate a PTY when a timeout is given for SSH connections - it changes the semantics of commands such as ls.

Related Issue

inspec/inspec#5459

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New content (non-breaking change)
  • Breaking change (a content change which would break existing functionality or processes)

Checklist:

  • I have read the CONTRIBUTING document.

…onnections - it changes the semantics of commands such as ls

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
@tas50
Copy link
Contributor

tas50 commented Apr 13, 2021

We've had a lot of TTY changes made recently for knife bootstrap, one of which broke many bootstrap scenarios. Before we merge this we need additional testing to make sure it doesn't impact the boostrap scenarios.

@clintoncwolfe
Copy link
Contributor Author

After discussion with @tas50, I explained that this change only affects situations using the timeout feature of the SSH transports, which is relatively new and has been broken since being introduced. The change is to revert back to previous behavior, only allocating a PTY when explicitly requested. He felt comfortable with the merge if I am confident in low impact of the change, which I am.

@clintoncwolfe clintoncwolfe added the Type: Bug Feature not working as expected label Apr 14, 2021
@clintoncwolfe clintoncwolfe merged commit 20c1d5d into master Apr 14, 2021
@james-stocks
Copy link

FYI @clintoncwolfe the reason for the PTY being allocated when using a timeout was to ensure that the command being executed would terminate on the target host.

e.g. if you run command("sleep 5000", timeout: 5) , when train times out after 5 seconds you do not want to see the sleep command still running on the host. Without this PTY, if I remember correctly, you will see the sleep process still waiting the remining 4995 seconds.
If a command runs indefinitely, then processes will stack up as InSpec does repeated compliance scans and repeatedly runs the command then gives up on it.

Seems like forcing a PTY isn't the right way to do that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: SSH Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants