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

[BUG] salt-ssh error reporting/handling of remote errors/misbehavior is optimistic to a fault #64531

Closed
lkubb opened this issue Jun 22, 2023 · 0 comments · Fixed by #64542
Closed
Labels
Bug broken, incorrect, or confusing behavior Salt-SSH Sulfur v3006.2

Comments

@lkubb
Copy link
Contributor

lkubb commented Jun 22, 2023

Description
There are several situations in which salt-ssh deviates from regular salt-call regarding error reporting/handling. This is intended as a collective issue/overview since they are related.

  1. state.* wrappers treat an exception during the state run on the target as success.
  2. When calling execution modules on the target using the FunctionWrapper, remote errors/exceptions are generally swallowed. The function seemingly returns the error dict now.
    .a This results in undefined behavior in wrapper modules and rendering templates which call non-wrapped modules that raise an exception.
    .b This also results in mine functions that cause remote exceptions to happily pass along the error dict as the function's return without reporting a non-zero exitcode.
  3. When calling execution modules on the target using the FunctionWrapper, local deserialization errors are generally swallowed. The function seemingly returns the error dict now.
    .a see 2.a
    .b see 2.b
    .c This might be related to salt-ssh return exit code 0 if salt-call fails to run #50727 if still relevant.
  4. When calling execution modules on the target using the FunctionWrapper and the remote does not return a dict containing "return", an empty dict is returned instead.
    .a see 2.a
    .b see 2.b

Setup
irrelevant

Steps to Reproduce the behavior
See linked issues.

(1) Is a bit hard to reproduce since it requires Salt crashing on the remote, but can be easily inferred from the code:

stdout, stderr, _ = single.cmd_block()
# Clean up our tar
try:
os.remove(trans_tar)
except OSError:
pass
# Read in the JSON data and return the data structure
try:
return salt.utils.json.loads(stdout)
except Exception as e: # pylint: disable=broad-except
log.error("JSON Render failed for: %s\n%s", stdout, stderr)
log.error(str(e))
# If for some reason the json load fails, return the stdout
return stdout

(2.a) #52452 (comment)
(2.b)
ret = salt.utils.json.dumps({"local": {"return": result}})

(2/3/4)
try:
ret = salt.utils.json.loads(stdout)
if len(ret) < 2 and "local" in ret:
ret = ret["local"]
ret = ret.get("return", {})
except ValueError:
ret = {
"_error": "Failed to return clean data",
"stderr": stderr,
"stdout": stdout,
"retcode": retcode,
}
return ret

(2/3/4) might come into play for any wfunc call
opts_pkg = pre_wrapper["test.opts_pkg"]() # pylint: disable=E1102
if "_error" in opts_pkg:
# Refresh failed
retcode = opts_pkg["retcode"]
ret = salt.utils.json.dumps({"local": opts_pkg})
return ret, retcode

Expected behavior
Behave as similarly as possible to salt-call. Treat exceptions as exceptions, report them as such and especially do not pass off a truthy value as the return of a function that clearly failed.

Versions Report
Still present in current master.

Additional context
Related:
QubesOS/qubes-issues#8146 (1)
#50727 (maybe 3)
#52452 (2.a)

#64514 (already fixed yesterday)

@lkubb lkubb added Bug broken, incorrect, or confusing behavior needs-triage labels Jun 22, 2023
@lkubb lkubb changed the title [BUG] salt-ssh error reporting/handling of remote errors is optimistic to a fault [BUG] salt-ssh error reporting/handling of remote errors/misbehavior is optimistic to a fault Jun 22, 2023
lkubb added a commit to lkubb/salt that referenced this issue Jun 23, 2023
lkubb added a commit to lkubb/salt that referenced this issue Jun 25, 2023
This commit ensures that output indicating failure is not
passed off as the output of the command when it is called
via `__salt__`, e.g. during template rendering or in wrapper
modules. It does this by raising exceptions when issues
are detected and thus simulates the remote error locally.
See issue saltstack#64531 for a detailed description.
@garethgreenaway garethgreenaway added this to the Sulfur v3006.2 milestone Jun 26, 2023
lkubb added a commit to lkubb/salt that referenced this issue Jun 28, 2023
lkubb added a commit to lkubb/salt that referenced this issue Jun 28, 2023
This commit ensures that output indicating failure is not
passed off as the output of the command when it is called
via `__salt__`, e.g. during template rendering or in wrapper
modules. It does this by raising exceptions when issues
are detected and thus simulates the remote error locally.
See issue saltstack#64531 for a detailed description.
@anilsil anilsil modified the milestones: Sulfur v3006.3, Sulfur v3006.4 Sep 8, 2023
lkubb added a commit to lkubb/salt that referenced this issue Oct 19, 2023
lkubb added a commit to lkubb/salt that referenced this issue Oct 19, 2023
This commit ensures that output indicating failure is not
passed off as the output of the command when it is called
via `__salt__`, e.g. during template rendering or in wrapper
modules. It does this by raising exceptions when issues
are detected and thus simulates the remote error locally.
See issue saltstack#64531 for a detailed description.
lkubb added a commit to lkubb/salt that referenced this issue Dec 18, 2023
lkubb added a commit to lkubb/salt that referenced this issue Dec 18, 2023
This commit ensures that output indicating failure is not
passed off as the output of the command when it is called
via `__salt__`, e.g. during template rendering or in wrapper
modules. It does this by raising exceptions when issues
are detected and thus simulates the remote error locally.
See issue saltstack#64531 for a detailed description.
dwoz pushed a commit that referenced this issue Dec 18, 2023
dwoz pushed a commit that referenced this issue Dec 18, 2023
This commit ensures that output indicating failure is not
passed off as the output of the command when it is called
via `__salt__`, e.g. during template rendering or in wrapper
modules. It does this by raising exceptions when issues
are detected and thus simulates the remote error locally.
See issue #64531 for a detailed description.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Salt-SSH Sulfur v3006.2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants