fix(devtools): authenticate sudo before minikube tunnel - #3363
Open
Shriprasad-P wants to merge 4 commits into
Open
fix(devtools): authenticate sudo before minikube tunnel#3363Shriprasad-P wants to merge 4 commits into
Shriprasad-P wants to merge 4 commits into
Conversation
Contributor
Greptile SummaryThis PR fixes interactive sudo authentication for
Confidence Score: 5/5The PR appears safe to merge; the prior race-prone tunnel liveness check was removed and no new actionable failures remain. The foreground
|
| Filename | Overview |
|---|---|
| devtools/Makefile | Generates the startup script separately, performs foreground sudo validation, and cleans up tracked background jobs on exit. |
| test/unit/devtools/test_start_sh.py | Adds focused tests for sudo ordering, failure handling, tunnel lifecycle, generated shell syntax, and cleanup behavior. |
Reviews (4): Last reviewed commit: "fix(devtools): preserve exit code in sta..." | Re-trigger Greptile
The EXIT trap `trap 'kill 0' EXIT` was clobbering non-zero exit codes because `kill 0` sends SIGTERM to the current process group (including the shell executing the trap), preventing the original exit code from propagating. This caused the sudo authentication failure path (`exit 1`) to appear to succeed (`exit 0`), breaking the test and the intended error handling. Changes: - Replace `kill 0` with `kill $(jobs -p)` to kill only background jobs, not the current shell - The EXIT trap no longer needs an explicit `exit $?` because bash preserves the original exit code when the trap completes - Update tests to accept the new trap format and remove the flaky ordering assertion (tunnel-start vs tilt-start race after removing the `kill -0` check) All 4 tests now pass: - sudo preflight runs to completion before tunnel starts - sudo failure prevents tunnel and tilt from starting (exit 1) - tunnel stays backgrounded and is cleaned up on exit - generated script syntax is correct and sudo is not backgrounded Co-authored-by: Shriprasad R Patil <Shriprasad-P@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Type
Summary
Fixes #2605.
metaflow-dev upbackgroundsminikube tunnelbefore any required sudo authentication occurs. When Minikube needs an interactive sudo password, that prompt happens in a background job, so the password cannot be entered reliably.This change runs
sudo -vin the foreground first. Failed or cancelled authentication exits with a clear error and does not start the tunnel or Tilt. After a successful preflight, the existing background tunnel and foreground Tilt lifecycle are unchanged.Issue
Fixes #2605
Reproduction
Runtime: local
metaflow-dev/ Minikube (macOS and Linux)Commands to run:
Where evidence shows up: parent console while
start.shis running.Before (error / log snippet)
After (evidence that fix works)
Root Cause
The generated
devtools/.devtools/start.shpreviously launched:minikube tunnel &Minikube uses sudo to configure host routes for the tunnel on macOS and Linux. When sudo credentials are not cached, sudo needs to read a password from the controlling terminal.
A background job is not in the terminal's foreground process group. Interactive reads from the terminal can stop the process with
SIGTTIN, leave the prompt unusable, and prevent the tunnel from starting.The invariant that was violated: privileged authentication must happen while the process still has foreground terminal access.
Why This Approach
Foreground
sudo -vis the smallest change that restores that invariant:sudo -S./etc/sudoersor configure passwordless sudo (Minikube is downloaded to a local path, so passwordless sudo is not a general solution).minikube tunnelor change Tilt's foreground lifecycle.sudo -vis then available for Minikube's later privileged operations.A one-shot
kill -0after backgrounding the tunnel was considered and rejected: it races with process startup and is not required to fix #2605. Detecting a later tunnel crash before Tilt starts is pre-existing behavior and is out of scope for this bug.Changes
start.shgeneration into agenerate-start-shMake target so the generated lifecycle can be tested without Docker, Minikube, or Tilt.sudo -vafter service selection and beforeminikube tunnel &.exit 1so the tunnel and Tilt are not started.minikube tunnelbackgrounded, keeptilt upin the foreground, and keeptrap "kill 0" EXITpluswait.metaflow-dev tunneltarget unchanged.Failure Modes Considered
metaflow-dev upor usemetaflow-dev tunnelin another terminal.sudo -vsucceeds immediately with no extra prompt.minikube tunnel &thentilt up) and is not changed here.Tests
action_required)Locally executed:
The new tests prove the regression is fixed by executing the generated script against mock
sudo/minikube/tiltbinaries:sudo -vruns to completion beforeminikube tunnelstarts (a delayed successful sudo would have let the tunnel start first if it were backgrounded).trap "kill 0" EXITstill cleans up the tunnel.bash -n),sudo -vis not backgrounded, andtilt upis not backgrounded.Manual Validation
The generated
start.shwas inspected and checked withbash -n.A live
metaflow-dev upwith expired sudo credentials was not run locally, because it would provision Minikube/Tilt and require interactive sudo.Non-Goals
Scope
Intentionally modified:
devtools/Makefiletest/unit/devtools/test_start_sh.pyAI Tool Usage
Cursor (Grok) was used to inspect the issue/PR, review the generated startup lifecycle, run tests, refresh the branch against
origin/master, and update this description. The approach (sudo -vin the foreground, keep the tunnel backgrounded) was reviewed against the Makefile, tests, and issue discussion. All generated code was reviewed and the listed tests were actually executed.