Repository navigation
Conversation
|
I'm currently fixing the merge-based errorrs, 1 sec |
Fizzadar
left a comment
There was a problem hiding this comment.
Makes sense, I'll try to check out some possible fixes for this
Any luck with this @Dexmachi? Still keen to get this in if you are!
Heads up that 3.x has moved on a fair bit since May - askpass cache keys now include the temp dir, and we match sudo-rs' "interactive authentication is required" as well as sudo's own phrasing, so this'll need a rebase before I can review it properly (as-is it'd revert both).
I still think the marker wants removing before the exit 1 rather than sticking around for the whole connection - PIDs get recycled and we only clean up on disconnect, so a later sudo can inherit a stale marker and get handed an empty password for no reason.
One other thing I missed first time round: _ensure_su_askpass_set_for_host shares ASKPASS_COMMAND, so a su password failure will print sudo: 1 incorrect password attempt - wrong tool name, and it'll trip the sudo branch in execute_command_with_sudo_retry.
|
@Fizzadar ty for the heads up!! I'm currently qorking on a bunch of other shtuff, and got a bit burnt out for a while, but this is definitely on my back burner! I'll make sure to address the review and try to get it to a less hacky method! |
There was a problem hiding this comment.
I checked the sudo password flows. The new retry logic introduces a few regressions that need fixing first.
Issues
src/pyinfra/connectors/util.py:293: an incorrect explicitly supplied_sudo_passwordnow enters the interactive retry loop instead of returning the command failure. Non-interactive runs can raiseEOFErrorfromgetpass; this reproduces on the PR head and is failingtest_e2e_ssh_sudo_password,end-to-end-test (ubuntu-22.04, ssh), andtests-done.src/pyinfra/connectors/util.py:468: a cached prompted password now overrides every later explicit_sudo_password, so per-operation or changed credentials are silently ignored. The PR head generates a command withold-passwordin this case:host.connector_data["prompted_sudo_password"] = "old-password" make_unix_command_for_host(..., _sudo=True, _sudo_password="new-password")
src/pyinfra/connectors/util.py:41: a successful askpass call leaves$0.${PPID}.calledbehind until disconnect. If sudo later reuses that PID, its first askpass call takes the incorrect-password branch and returns no password, so the earlier stale-marker feedback is only partly resolved.
Notes
- The
.claude/skillssymlink replacement and newdocs/_deploy_globals.rstare unrelated to sudo handling and should be removed from this PR.
Done at PyCon FR.
…n overriding new explicit passwords + removal of marker
|
@wowi42 ty for the review! These were issues I honestly did not think of when designing these fixes, and this PR actually fixes an actual issue I have on my own little project so I'm glad to fix them right up. btw, about the SKILLS and the rst, they came with a random merge, I am not really sure how they ended up here, but I did remove them, thank you for pointing this out |
Closes #1043
Closes #1688
How:
Added a {0}.{PPID}.called marker + env LC_ALL=C to sudo calls, allowing Pyinfra to keep track of sudo call attempts and to have a standardized retry catch.
the marker gets removed alongside the sudo password file, in the same call.
Why:
@Local sudo handling was fragile, requiring a specific locale and not dealing with auto sudo retries, resulting in faillocks and premature errors.
Test Changes:
Had to change test_ssh and test_util to assert "env LC_ALL=C " at the start of sudo and su calls.
Before/After:
Before(normal usage):
After(normal usage):
Before(ad-hoc):
After(ad-hoc):
3.xat this time)scripts/dev-test.sh)scripts/dev-lint.sh)