fix(elway): evaluate when:/creates:/changed_when: with the step's own sudo
Conditions ran unprivileged no matter what the step declared, and that fails in the dangerous direction. A root-requiring `when:` -- `pvesh get ...` exits 255 for a non-root user -- returns non-zero, elway reports the step `skipped`, and a playbook that never performed its change reports overall OK. "Skipped" is indistinguishable from working idempotency, so the run looks correct. Found the hard way on esh-pve: three consecutive runs of an exclusion playbook reported success while changing nothing. Only the verify phase caught it, by continuing to report the thing the steps claimed to have handled -- which is exactly why verify runs unconditionally. `creates:` had the same fault from the other side: a path under /root is unreadable to the login user, so `test -e` said absent and the step re-ran every time. It now correctly reports the file as already present. Sudo-less steps are unaffected: their conditions still evaluate as the login user, which is what they mean. Only a step that declares sudo: true gets privileged condition evaluation, so no existing playbook changes meaning unless it was already silently broken.
This commit is contained in:
+27
-7
@@ -570,14 +570,34 @@ def run_upload_step(ctx: SSHContext, step: Step, prefix: str) -> int:
|
||||
# ─── Skip + change evaluators (tier 1 + tier 2) ────────────────────────────
|
||||
|
||||
|
||||
def _quiet_rc(ctx: SSHContext, remote_cmd: str) -> int:
|
||||
"""Run a shell expression on the remote under bash -c, discarding output."""
|
||||
def _quiet_rc(ctx: SSHContext, remote_cmd: str, sudo: bool = False) -> int:
|
||||
"""Run a shell expression on the remote under bash -c, discarding output.
|
||||
|
||||
⚠ `sudo` MUST track the step's own sudo setting. Conditions used to run
|
||||
unprivileged no matter what the step said, and that fails in the dangerous
|
||||
direction: a root-requiring `when:` (say `pvesh get ...`, which exits 255
|
||||
for a non-root user) returns non-zero, elway reports the step `skipped`,
|
||||
and a playbook that never performed its change reports overall OK. Measured
|
||||
2026-09-19 on esh-pve — three runs of an exclusion playbook reported
|
||||
success while changing nothing, and only the verify phase caught it.
|
||||
|
||||
Sudo-less steps are unaffected: their conditions still evaluate as the
|
||||
login user, which is what they mean.
|
||||
"""
|
||||
# Wrapping in bash -c gives `!`, `[[`, pipes, etc. consistent semantics
|
||||
# across hosts whose default login shell might be dash (Debian default) or
|
||||
# something else weird.
|
||||
wrapped = "bash -c " + shlex.quote(remote_cmd)
|
||||
if sudo:
|
||||
ensure_sudo(ctx)
|
||||
wrapped = "sudo -S -p '' bash -c " + shlex.quote(remote_cmd)
|
||||
stdin_data = (ctx.sudo_password or "") + "\n"
|
||||
else:
|
||||
wrapped = "bash -c " + shlex.quote(remote_cmd)
|
||||
stdin_data = None
|
||||
return subprocess.run(
|
||||
ctx.ssh_cmd(wrapped),
|
||||
input=stdin_data,
|
||||
text=True,
|
||||
stdout=subprocess.DEVNULL,
|
||||
stderr=subprocess.DEVNULL,
|
||||
).returncode
|
||||
@@ -586,15 +606,15 @@ def _quiet_rc(ctx: SSHContext, remote_cmd: str) -> int:
|
||||
def evaluate_skip(ctx: SSHContext, step: Step) -> Optional[str]:
|
||||
"""If any pre-condition says skip, return a human-readable reason. Else None."""
|
||||
if step.when is not None:
|
||||
rc = _quiet_rc(ctx, step.when)
|
||||
rc = _quiet_rc(ctx, step.when, step.sudo)
|
||||
if rc != 0:
|
||||
return f"when: expr exited {rc} (needed 0 to run)"
|
||||
if step.creates is not None:
|
||||
rc = _quiet_rc(ctx, f"test -e {shlex.quote(step.creates)}")
|
||||
rc = _quiet_rc(ctx, f"test -e {shlex.quote(step.creates)}", step.sudo)
|
||||
if rc == 0:
|
||||
return f"creates: {step.creates} already exists"
|
||||
if step.removes is not None:
|
||||
rc = _quiet_rc(ctx, f"test -e {shlex.quote(step.removes)}")
|
||||
rc = _quiet_rc(ctx, f"test -e {shlex.quote(step.removes)}", step.sudo)
|
||||
if rc != 0:
|
||||
return f"removes: {step.removes} is already absent"
|
||||
return None
|
||||
@@ -604,7 +624,7 @@ def evaluate_changed(ctx: SSHContext, step: Step) -> bool:
|
||||
"""Post-step: should this count as `changed`? Default True (Ansible shell default)."""
|
||||
if step.changed_when is None:
|
||||
return True
|
||||
rc = _quiet_rc(ctx, step.changed_when)
|
||||
rc = _quiet_rc(ctx, step.changed_when, step.sudo)
|
||||
# exit 0 = the change-detector expression "fired" = step counts as changed.
|
||||
return rc == 0
|
||||
|
||||
|
||||
Reference in New Issue
Block a user