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

[master] Improve salt-ssh error handling #64542

Merged
merged 17 commits into from
Dec 18, 2023

Conversation

lkubb
Copy link
Contributor

@lkubb lkubb commented Jun 25, 2023

Note: This will stay a draft until #64576 finds its way into master (or the regression is fixed otherwise) Since there has been no activity on the referenced PR for months, I went ahead and merged it into this branch to unblock this PR. It's in. :)

What does this PR do?

  • Ensures that state rendering is interrupted when an execution module run on the target exits with a non-zero exit code or returns non-decodable or invalid data. Thereby ensures that templates are not rendered with garbage inputs. Wrapper modules will receive an exception and thus will not mistake error returns for remote function output. See salt-ssh swallows exceptions in custom modules #52452 (comment) for details.
  • Excludes error returns from mine data, as it is done in usual Salt as well.
  • Makes state.* wrappers treat non-decodable json output (usually caused by Salt crashing completely and printing a stacktrace only) as an error condition.
  • Ensures salt-ssh job ret events contain all usual data (saw it while reading the code and included the minor fix here, hope that's OK)

What issues does this PR fix or reference?

Fixes: #52452
Fixes: #64531
Fixes: #42065
Fixes: #50727 (last-minute addition: after re-reading the report, I think it's relatively clear why it was not working before and that this should fix it, see test in tests/pytests/integration/ssh/state/test_retcode_state_run_remote_exception.py)

Previous Behavior

  • Executing any non-wrapped module inside a template or wrapper module would imply the possibility that errors are evaluated as truthy or randomly rendered salt-ssh error return dicts would mess up the YAML etc. structure when printing the output.
  • Mine function returns could contain the same garbage.
  • Fatal errors on the remote during state execution would be treated as OK.

New Behavior

  • Exceptions are raised when a remote run returns with a non-zero exit code, returns data that indicates failure or if JSON output was expected, but deserialization failed. This causes template rendering to abort and wrapper modules to be confronted with an exception.
  • Error returns are excluded from the mine.
  • Fatal errors on the remote during state execution return a non-zero exit code.

Merge requirements satisfied?

Commits signed with GPG?

Yes

@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Improve salt-ssh error handling [master] Improve salt-ssh error handling Jun 25, 2023
@lkubb lkubb changed the base branch from master to 3006.x June 25, 2023 19:07
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title [master] Improve salt-ssh error handling [3006.x] Improve salt-ssh error handling Jun 25, 2023
@lkubb lkubb force-pushed the salt-ssh-error-handling branch from 3da07b9 to d1146ce Compare June 25, 2023 19:12
@lkubb lkubb temporarily deployed to ci June 25, 2023 19:49 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 19:49 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 19:49 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 19:49 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 20:07 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 20:13 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 21:37 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 21:38 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 21:39 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 21:39 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 21:53 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 21:57 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 22:53 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 22:53 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 22:53 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 22:53 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 22:54 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 25, 2023 22:54 — with GitHub Actions Inactive
@lkubb lkubb force-pushed the salt-ssh-error-handling branch from 998a939 to 7eea362 Compare June 26, 2023 09:30
@lkubb lkubb temporarily deployed to ci June 26, 2023 09:46 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 26, 2023 09:46 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 26, 2023 09:46 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 26, 2023 09:46 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 26, 2023 09:59 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 26, 2023 10:05 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci June 26, 2023 10:50 — with GitHub Actions Inactive
@dwoz
Copy link
Contributor

dwoz commented Dec 18, 2023

@lkubb Awesome, lets hope all the tests pass now :D

@dwoz dwoz merged commit 6f6fd93 into saltstack:master Dec 18, 2023
531 checks passed
lkubb added a commit to lkubb/salt that referenced this pull request Dec 18, 2023
lkubb added a commit to lkubb/salt that referenced this pull request Dec 18, 2023
lkubb added a commit to lkubb/salt that referenced this pull request Dec 28, 2023
lkubb added a commit to lkubb/salt that referenced this pull request Dec 28, 2023
dwoz pushed a commit to lkubb/salt that referenced this pull request Dec 29, 2023
lkubb added a commit to lkubb/salt that referenced this pull request Dec 29, 2023
Before PR saltstack#64542, SSH error returns depended on whether the command was
executed non-wrapped or wrapped. The former would always return an error
dict including `stdout` and `stderr` keys, while the latter would return
a string. Synchronizing this behavior to work like usual introduced a
breaking change, which was reverted afterwards.

This cleans up unnecessary code after the revert of the mentioned
behavior.

1) SSHCommandExecutionErrors are only thrown when retcode > 0
2) `Probably got garbage` is the return of the module which should never
   be reached (when failing, it's in the returned string, it's never
   found in a dict key `ret["stderr"]`).
lkubb added a commit to lkubb/salt that referenced this pull request Dec 29, 2023
Before PR saltstack#64542, SSH error returns depended on whether the command was
executed non-wrapped or wrapped. The former would always return an error
dict including `stdout` and `stderr` keys, while the latter would return
a string. Synchronizing this behavior to work like usual introduced a
breaking change, which was reverted afterwards.

This cleans up unnecessary code after the revert of the mentioned
behavior.

1) SSHCommandExecutionErrors are only thrown when retcode > 0
2) `Probably got garbage` is the return of the module which should never
   be reached (when failing, it's in the returned string, it's never
   found in a dict key `ret["stderr"]`).
lkubb added a commit to lkubb/salt that referenced this pull request Dec 29, 2023
dwoz pushed a commit that referenced this pull request Dec 30, 2023
Before PR #64542, SSH error returns depended on whether the command was
executed non-wrapped or wrapped. The former would always return an error
dict including `stdout` and `stderr` keys, while the latter would return
a string. Synchronizing this behavior to work like usual introduced a
breaking change, which was reverted afterwards.

This cleans up unnecessary code after the revert of the mentioned
behavior.

1) SSHCommandExecutionErrors are only thrown when retcode > 0
2) `Probably got garbage` is the return of the module which should never
   be reached (when failing, it's in the returned string, it's never
   found in a dict key `ret["stderr"]`).
dwoz pushed a commit that referenced this pull request Jan 2, 2024
lkubb added a commit to lkubb/salt that referenced this pull request Mar 7, 2024
lkubb added a commit to lkubb/salt that referenced this pull request May 16, 2024
lkubb added a commit to lkubb/salt that referenced this pull request Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants