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.

Sign up for GitHub

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

Jump to bottom

Allow timeout option for WinRM commands #27

Merged
merged 2 commits into from
Sep 30, 2020
Merged

Allow timeout option for WinRM commands #27

merged 2 commits into from
Sep 30, 2020

Conversation

james-stocks
Copy link

@james-stocks james-stocks commented Sep 23, 2020 •
edited
Loading

Allows end users (e.g. InSpec test coders) to specify a timeout for a potentially long running command.
If the timeout is reached, the command is expected to be terminated on the host and an exception is raised (a subsequent change to InSpec will handle this exception).
This complements recent changes to the base connection and ssh connection in train: inspec/train#625

Tested with a variety of commands with and without the timeout option against Server 2019 and 2012R2 instances.

Signed-off-by: James Stocks [email protected]

Related Issue

inspec/inspec#3772

Types of changes

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

Checklist:

  • [ X] I have read the CONTRIBUTING document.

@james-stocks james-stocks marked this pull request as draft September 23, 2020 15:21
@james-stocks james-stocks requested a review from Schwad September 23, 2020 15:21
Schwad
Schwad reviewed Sep 24, 2020
View reviewed changes
Copy link

@Schwad Schwad left a comment

Choose a reason for hiding this comment

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

The rub here is definitely the testing- I don't have a slick WinRM setup to hand locally but if this passes your local manual tests LGTM 👍

Leaving as a comment since this is still a draft but LMK when you'd like an approved review

lib/train-winrm/connection.rb Show resolved Hide resolved
@james-stocks james-stocks force-pushed the js/command_timeouts branch 4 times, most recently from a6323fb to f2ff783 Compare September 28, 2020 13:17
@james-stocks james-stocks marked this pull request as ready for review September 28, 2020 13:20
clintoncwolfe
clintoncwolfe approved these changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Follows the same pattern as the SSH code with some finer error handling - looks good. I'm not able to test this myself so this is just a read-over. Looks like there are some unrelated linting issues but changes look solid.

@mwrock mwrock mentioned this pull request Sep 28, 2020
Open
james-stocks
james-stocks commented Sep 30, 2020
View reviewed changes
lib/train-winrm/connection.rb
# If this command hits timeout, calling close with the command currently running causes
# a RuntimeError error in WinRM's cleanup code. This specific error can be ignored.
# The command will be terminated and further commands can be sent on the connection.
raise e unless timeout && e.to_s == "opts[:shell_id] is required"
Copy link
Author

@james-stocks james-stocks Sep 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

WinRb/WinRM#316 addresses this but is not yet released in a gem. Even once it is available, there's a chance there'll just be another exception we need to catch and dismiss.

@james-stocks
Copy link
Author

I've done more testing to confirm that exceptions don't happen when running commands without a timeout.
With the timeout, there may be rare race exceptions that get thrown and will surface to the downstream library instead of the train CommandReachedTimeout exception. I would argue this is acceptable since this timeout is like an emergency brake - you're in fault territory if you reach this timeout.

This can potentially be replaced in future by using an elevated shell, which features command timeouts. Tracking that via inspec/inspec#1418

James Stocks added 2 commits September 30, 2020 12:34
Allow timeout option for WinRM commands
afb2f0f 
Allows end users (e.g. InSpec test coders) to specify a timeout for a potentially long running command.
If the timeout is reached, the command is expected to be terminated on the host and an exception is raised (a subsequent change to InSpec will handle this exception).
This complements recent changes to the base connection and ssh connection in train: inspec/train#625

Signed-off-by: James Stocks <[email protected]>
Address minitest deprecation warnings
8a057ec 
Signed-off-by: James Stocks <[email protected]>
Schwad
Schwad approved these changes Sep 30, 2020
View reviewed changes
Copy link

@Schwad Schwad left a comment

Choose a reason for hiding this comment

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

💯 Just looked over the files/changes again (including the new changes) and looks sound to me. ✅

@james-stocks james-stocks merged commit 0938a72 into master Sep 30, 2020
@james-stocks james-stocks deleted the js/command_timeouts branch September 30, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catriona1 catriona1 catriona1 approved these changes

@Schwad Schwad Schwad approved these changes

@clintoncwolfe clintoncwolfe clintoncwolfe approved these changes

@mwrock mwrock Awaiting requested review from mwrock

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@james-stocks @clintoncwolfe @Schwad @catriona1