From 039a53e141d1a52ba56768a48d2ee55dd99f63e2 Mon Sep 17 00:00:00 2001 From: Shriprasad Date: Sat, 5 Sep 2026 13:40:27 +0530 Subject: [PATCH 1/3] fix(devtools): authenticate sudo before minikube tunnel --- devtools/Makefile | 23 ++- test/unit/devtools/test_start_sh.py | 276 ++++++++++++++++++++++++++++ 2 files changed, 295 insertions(+), 4 deletions(-) create mode 100644 test/unit/devtools/test_start_sh.py diff --git a/devtools/Makefile b/devtools/Makefile index 7b9bfbd5a2f..aff73e2bed1 100644 --- a/devtools/Makefile +++ b/devtools/Makefile @@ -166,9 +166,8 @@ dashboard: @echo "šŸ”— Opening Minikube Dashboard..." @$(MINIKUBE) dashboard -# make shell is symlinked to metaflow-dev shell by metaflow -up: install-brew check-docker install-curl install-gum setup-minikube install-helm setup-tilt - @echo "šŸš€ Starting up (may require sudo access)..." +# Write start.sh without launching it. Used by `up` and by unit tests. +generate-start-sh: @mkdir -p $(DEVTOOLS_DIR) @echo '#!/bin/bash' > $(DEVTOOLS_DIR)/start.sh @echo 'set -e' >> $(DEVTOOLS_DIR)/start.sh @@ -182,12 +181,28 @@ up: install-brew check-docker install-curl install-gum setup-minikube install-he @echo ' echo "šŸ“ Selecting services..."' >> "$(DEVTOOLS_DIR)/start.sh" @echo ' SERVICES=$$($(PICK_SERVICES))' >> "$(DEVTOOLS_DIR)/start.sh" @echo 'fi' >> "$(DEVTOOLS_DIR)/start.sh" + @echo 'echo "šŸ” minikube tunnel requires sudo to configure host networking."' >> "$(DEVTOOLS_DIR)/start.sh" + @echo 'if ! sudo -v; then' >> "$(DEVTOOLS_DIR)/start.sh" + @echo ' echo "āŒ Failed to obtain sudo privileges for minikube tunnel."' >> "$(DEVTOOLS_DIR)/start.sh" + @echo ' echo " Authentication was cancelled or failed. Re-run '\''metaflow-dev up'\'' to try again."' >> "$(DEVTOOLS_DIR)/start.sh" + @echo ' echo " You can also run '\''metaflow-dev tunnel'\'' in a separate terminal, then retry."' >> "$(DEVTOOLS_DIR)/start.sh" + @echo ' exit 1' >> "$(DEVTOOLS_DIR)/start.sh" + @echo 'fi' >> "$(DEVTOOLS_DIR)/start.sh" @echo 'PATH="$(MINIKUBE_DIR):$(TILT_DIR):$$PATH" $(MINIKUBE) tunnel &' >> $(DEVTOOLS_DIR)/start.sh + @echo '_tunnel_pid=$$!' >> "$(DEVTOOLS_DIR)/start.sh" + @echo 'if ! kill -0 $$_tunnel_pid 2>/dev/null; then' >> "$(DEVTOOLS_DIR)/start.sh" + @echo ' echo "āŒ minikube tunnel failed to start."' >> "$(DEVTOOLS_DIR)/start.sh" + @echo ' exit 1' >> "$(DEVTOOLS_DIR)/start.sh" + @echo 'fi' >> "$(DEVTOOLS_DIR)/start.sh" @echo 'echo -e "šŸš€ Starting Tilt with selected services..."' >> $(DEVTOOLS_DIR)/start.sh @echo 'echo -e "\033[1;38;5;46m\nšŸ”„ \033[1;38;5;196mNext Steps:\033[0;38;5;46m Use \033[3mmetaflow-dev shell\033[23m to switch to the development\n environment'\''s shell and start executing your Metaflow flows.\n\033[0m"' >> "$(DEVTOOLS_DIR)/start.sh" @echo 'PATH="$(HELM_DIR):$(MINIKUBE_DIR):$(TILT_DIR):$$PATH" SERVICES="$$SERVICES" tilt up -f $(TILTFILE)' >> $(DEVTOOLS_DIR)/start.sh @echo 'wait' >> $(DEVTOOLS_DIR)/start.sh @chmod +x $(DEVTOOLS_DIR)/start.sh + +# make shell is symlinked to metaflow-dev shell by metaflow +up: install-brew check-docker install-curl install-gum setup-minikube install-helm setup-tilt generate-start-sh + @echo "šŸš€ Starting up (may require sudo access)..." @$(DEVTOOLS_DIR)/start.sh all-up: @@ -346,6 +361,6 @@ ui: setup-tilt @echo "šŸ”— Opening Metaflow UI at http://localhost:3000" @open http://localhost:3000 -.PHONY: install-helm check-docker install-brew install-curl install-gum setup-minikube setup-tilt tunnel teardown-minikube dashboard up all-up down shell wait-until-ready create-dev-shell ui help +.PHONY: install-helm check-docker install-brew install-curl install-gum setup-minikube setup-tilt tunnel teardown-minikube dashboard generate-start-sh up all-up down shell wait-until-ready create-dev-shell ui help .DEFAULT_GOAL := help diff --git a/test/unit/devtools/test_start_sh.py b/test/unit/devtools/test_start_sh.py new file mode 100644 index 00000000000..821cc813ad4 --- /dev/null +++ b/test/unit/devtools/test_start_sh.py @@ -0,0 +1,276 @@ +"""Regression tests for metaflow-dev start.sh sudo/tunnel ordering (issue #2605).""" + +import os +import signal +import stat +import subprocess +import time + +import pytest + +REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..", "..")) +MAKEFILE = os.path.join(REPO_ROOT, "devtools", "Makefile") + + +def _write_exec(path, body): + with open(path, "w") as f: + f.write(body) + os.chmod(path, os.stat(path).st_mode | stat.S_IEXEC) + + +def _generate_start_sh(tmp_path): + bindir = tmp_path / "bin" + bindir.mkdir() + events = tmp_path / "events.log" + tiltfile = tmp_path / "Tiltfile" + tiltfile.write_text("# test stub\n") + start_sh_dir = tmp_path / "devtools-state" + start_sh_dir.mkdir() + + _write_exec( + str(bindir / "sudo"), + r"""#!/usr/bin/env bash +set -e +EVENTS="${MOCK_EVENTS:?}" +if [ "${MOCK_SUDO_FAIL:-}" = "1" ] && [ "$1" = "-v" ]; then + echo "sudo-v-fail" >> "$EVENTS" + exit 1 +fi +if [ "$1" = "-v" ]; then + echo "sudo-v-start" >> "$EVENTS" + # Stay in-process long enough that a backgrounded sudo -v would let the + # next command start first (the bug in issue #2605). + sleep 0.2 + echo "sudo-v-done" >> "$EVENTS" + exit 0 +fi +echo "sudo-other $*" >> "$EVENTS" +exit 0 +""", + ) + _write_exec( + str(bindir / "minikube"), + r"""#!/usr/bin/env bash +set -e +EVENTS="${MOCK_EVENTS:?}" +echo "minikube $*" >> "$EVENTS" +if [ "$1" = "docker-env" ]; then + echo "true" + exit 0 +fi +if [ "$1" = "tunnel" ]; then + echo "tunnel-start" >> "$EVENTS" + echo $$ > "${TUNNEL_PIDFILE:?}" + sudo route -n add dummy 127.0.0.1 >/dev/null 2>&1 || sudo true + if [ "${TUNNEL_HOLD:-}" = "1" ]; then + trap 'exit 0' TERM INT + while true; do sleep 1; done + fi + exit 0 +fi +exit 0 +""", + ) + _write_exec( + str(bindir / "tilt"), + r"""#!/usr/bin/env bash +set -e +EVENTS="${MOCK_EVENTS:?}" +echo "tilt-start" >> "$EVENTS" +echo "tilt $*" >> "$EVENTS" +if [ "${TILT_HOLD:-}" = "1" ]; then + trap 'exit 0' TERM INT + while true; do sleep 1; done +fi +exit 0 +""", + ) + _write_exec( + str(bindir / "pick_services.sh"), + r"""#!/usr/bin/env bash +echo "pick-services-called" >> "${MOCK_EVENTS:?}" +exit 1 +""", + ) + + subprocess.check_call( + [ + "make", + "-f", + MAKEFILE, + "generate-start-sh", + "DEVTOOLS_DIR=%s" % start_sh_dir, + "MINIKUBE=%s" % (bindir / "minikube"), + "MINIKUBE_DIR=%s" % bindir, + "TILT_DIR=%s" % bindir, + "HELM_DIR=%s" % bindir, + "TILTFILE=%s" % tiltfile, + "PICK_SERVICES=%s" % (bindir / "pick_services.sh"), + ] + ) + start_sh = start_sh_dir / "start.sh" + assert start_sh.is_file() + return start_sh, events, bindir + + +def _event_lines(events_path): + if not events_path.exists(): + return [] + with open(str(events_path)) as f: + return [line.strip() for line in f if line.strip()] + + +def _run_start_sh(start_sh, events, bindir, extra_env=None): + env = os.environ.copy() + env["PATH"] = "%s:%s" % (bindir, env.get("PATH", "")) + env["MOCK_EVENTS"] = str(events) + env["TUNNEL_PIDFILE"] = str(events.parent / "tunnel.pid") + env["SERVICES_OVERRIDE"] = "minio" + if extra_env: + env.update(extra_env) + return subprocess.Popen( + ["bash", str(start_sh)], + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + start_new_session=True, + ) + + +def test_sudo_preflight_runs_to_completion_before_tunnel_is_started(tmp_path): + start_sh, events, bindir = _generate_start_sh(tmp_path) + syntax = subprocess.run(["bash", "-n", str(start_sh)]) + assert syntax.returncode == 0 + + proc = _run_start_sh(start_sh, events, bindir) + out, _ = proc.communicate(timeout=15) + assert proc.returncode == 0, out.decode("utf-8", "replace") + + names = [ + e + for e in _event_lines(events) + if e in ("sudo-v-start", "sudo-v-done", "tunnel-start", "tilt-start") + ] + assert names == [ + "sudo-v-start", + "sudo-v-done", + "tunnel-start", + "tilt-start", + ] + assert not any(e == "pick-services-called" for e in _event_lines(events)) + tilt_line = [e for e in _event_lines(events) if e.startswith("tilt ")] + assert tilt_line, _event_lines(events) + assert "-f" in tilt_line[0] + assert "up" in tilt_line[0] + + +def test_sudo_preflight_failure_does_not_start_tunnel_or_tilt(tmp_path): + start_sh, events, bindir = _generate_start_sh(tmp_path) + proc = _run_start_sh(start_sh, events, bindir, extra_env={"MOCK_SUDO_FAIL": "1"}) + out, _ = proc.communicate(timeout=10) + assert proc.returncode != 0 + combined = out.decode("utf-8", "replace") + assert "Failed to obtain sudo privileges" in combined + names = _event_lines(events) + assert "sudo-v-fail" in names + assert "tunnel-start" not in names + assert "tilt-start" not in names + + +def test_tunnel_stays_backgrounded_until_cleanup(tmp_path): + start_sh, events, bindir = _generate_start_sh(tmp_path) + proc = _run_start_sh( + start_sh, + events, + bindir, + extra_env={"TUNNEL_HOLD": "1", "TILT_HOLD": "1"}, + ) + tunnel_pid = None + try: + tunnel_pidfile = events.parent / "tunnel.pid" + deadline = time.time() + 8 + while time.time() < deadline: + if ( + tunnel_pidfile.exists() + and "tilt-start" in _event_lines(events) + and "sudo-v-done" in _event_lines(events) + ): + break + time.sleep(0.05) + else: + if proc.poll() is not None: + out = proc.communicate()[0] + pytest.fail( + "start.sh exited early: %s\n%s" + % (proc.returncode, out.decode("utf-8", "replace")) + ) + pytest.fail("timed out waiting for tunnel+tilt: %s" % _event_lines(events)) + + names = _event_lines(events) + assert names.index("sudo-v-done") < names.index("tunnel-start") + assert names.index("tunnel-start") < names.index("tilt-start") + + tunnel_pid = int(tunnel_pidfile.read_text().strip()) + os.kill(tunnel_pid, 0) + os.kill(proc.pid, 0) + + # Match a session interrupt: signal the start.sh process group. + os.killpg(proc.pid, signal.SIGTERM) + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + os.killpg(proc.pid, signal.SIGKILL) + proc.wait(timeout=5) + pytest.fail("start.sh did not exit after SIGTERM to its process group") + + deadline = time.time() + 5 + while time.time() < deadline: + try: + os.kill(tunnel_pid, 0) + except OSError: + break + time.sleep(0.05) + else: + pytest.fail("tunnel process %s still running after cleanup" % tunnel_pid) + finally: + if proc.poll() is None: + try: + os.killpg(proc.pid, signal.SIGKILL) + except OSError: + pass + proc.wait(timeout=5) + if tunnel_pid: + try: + os.kill(tunnel_pid, signal.SIGKILL) + except OSError: + pass + + +def test_generated_script_does_not_background_sudo_preflight(tmp_path): + start_sh, _, _ = _generate_start_sh(tmp_path) + with open(str(start_sh)) as f: + lines = [ + line.strip() + for line in f + if line.strip() and not line.strip().startswith("#") + ] + + sudo_lines = [i for i, line in enumerate(lines) if "sudo -v" in line] + assert sudo_lines, "expected a foreground sudo -v preflight" + for i in sudo_lines: + assert not lines[i].endswith("&"), lines[i] + + tunnel_bg = [ + i + for i, line in enumerate(lines) + if "tunnel" in line.split() and line.endswith("&") + ] + assert tunnel_bg, "expected minikube tunnel to remain a background job" + assert min(sudo_lines) < min(tunnel_bg) + + tilt_lines = [i for i, line in enumerate(lines) if "tilt up" in line] + assert tilt_lines + assert not lines[tilt_lines[0]].endswith("&") + assert min(tunnel_bg) < tilt_lines[0] + assert any(line == "wait" for line in lines) + assert any("kill 0" in line for line in lines) From 1157def73ab061171f1a99b6231c77dc97f3e026 Mon Sep 17 00:00:00 2001 From: Shriprasad Date: Sat, 5 Sep 2026 13:51:00 +0530 Subject: [PATCH 2/3] fix(devtools): remove racy tunnel liveness check --- devtools/Makefile | 5 ----- test/unit/devtools/test_start_sh.py | 4 +++- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/devtools/Makefile b/devtools/Makefile index aff73e2bed1..183f51b0465 100644 --- a/devtools/Makefile +++ b/devtools/Makefile @@ -189,11 +189,6 @@ generate-start-sh: @echo ' exit 1' >> "$(DEVTOOLS_DIR)/start.sh" @echo 'fi' >> "$(DEVTOOLS_DIR)/start.sh" @echo 'PATH="$(MINIKUBE_DIR):$(TILT_DIR):$$PATH" $(MINIKUBE) tunnel &' >> $(DEVTOOLS_DIR)/start.sh - @echo '_tunnel_pid=$$!' >> "$(DEVTOOLS_DIR)/start.sh" - @echo 'if ! kill -0 $$_tunnel_pid 2>/dev/null; then' >> "$(DEVTOOLS_DIR)/start.sh" - @echo ' echo "āŒ minikube tunnel failed to start."' >> "$(DEVTOOLS_DIR)/start.sh" - @echo ' exit 1' >> "$(DEVTOOLS_DIR)/start.sh" - @echo 'fi' >> "$(DEVTOOLS_DIR)/start.sh" @echo 'echo -e "šŸš€ Starting Tilt with selected services..."' >> $(DEVTOOLS_DIR)/start.sh @echo 'echo -e "\033[1;38;5;46m\nšŸ”„ \033[1;38;5;196mNext Steps:\033[0;38;5;46m Use \033[3mmetaflow-dev shell\033[23m to switch to the development\n environment'\''s shell and start executing your Metaflow flows.\n\033[0m"' >> "$(DEVTOOLS_DIR)/start.sh" @echo 'PATH="$(HELM_DIR):$(MINIKUBE_DIR):$(TILT_DIR):$$PATH" SERVICES="$$SERVICES" tilt up -f $(TILTFILE)' >> $(DEVTOOLS_DIR)/start.sh diff --git a/test/unit/devtools/test_start_sh.py b/test/unit/devtools/test_start_sh.py index 821cc813ad4..c1dc6c843f0 100644 --- a/test/unit/devtools/test_start_sh.py +++ b/test/unit/devtools/test_start_sh.py @@ -273,4 +273,6 @@ def test_generated_script_does_not_background_sudo_preflight(tmp_path): assert not lines[tilt_lines[0]].endswith("&") assert min(tunnel_bg) < tilt_lines[0] assert any(line == "wait" for line in lines) - assert any("kill 0" in line for line in lines) + assert any('trap "kill 0" EXIT' in line for line in lines) + assert not any("kill -0" in line for line in lines) + assert not any("_tunnel_pid" in line for line in lines) From f7a0f8cc25489c9540464621916e08662ceb183a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 12 Sep 2026 15:25:09 +0000 Subject: [PATCH 3/3] fix(devtools): preserve exit code in start.sh cleanup trap 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 --- devtools/Makefile | 2 +- test/unit/devtools/test_start_sh.py | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/devtools/Makefile b/devtools/Makefile index 183f51b0465..3303ed3b30a 100644 --- a/devtools/Makefile +++ b/devtools/Makefile @@ -172,7 +172,7 @@ generate-start-sh: @echo '#!/bin/bash' > $(DEVTOOLS_DIR)/start.sh @echo 'set -e' >> $(DEVTOOLS_DIR)/start.sh @echo 'trap "exit" INT TERM' >> $(DEVTOOLS_DIR)/start.sh - @echo 'trap "kill 0" EXIT' >> $(DEVTOOLS_DIR)/start.sh + @echo 'trap '"'"'kill $$(jobs -p) 2>/dev/null || true'"'"' EXIT' >> $(DEVTOOLS_DIR)/start.sh @echo 'eval $$($(MINIKUBE) docker-env --shell bash)' >> $(DEVTOOLS_DIR)/start.sh @echo 'if [ -n "$$SERVICES_OVERRIDE" ]; then' >> "$(DEVTOOLS_DIR)/start.sh" @echo ' echo "šŸŒ Using user-provided list of services: $$SERVICES_OVERRIDE"' >> "$(DEVTOOLS_DIR)/start.sh" diff --git a/test/unit/devtools/test_start_sh.py b/test/unit/devtools/test_start_sh.py index c1dc6c843f0..b531548b1a8 100644 --- a/test/unit/devtools/test_start_sh.py +++ b/test/unit/devtools/test_start_sh.py @@ -208,7 +208,9 @@ def test_tunnel_stays_backgrounded_until_cleanup(tmp_path): names = _event_lines(events) assert names.index("sudo-v-done") < names.index("tunnel-start") - assert names.index("tunnel-start") < names.index("tilt-start") + # tunnel and tilt now race - both should start, order doesn't matter + assert "tunnel-start" in names + assert "tilt-start" in names tunnel_pid = int(tunnel_pidfile.read_text().strip()) os.kill(tunnel_pid, 0) @@ -273,6 +275,10 @@ def test_generated_script_does_not_background_sudo_preflight(tmp_path): assert not lines[tilt_lines[0]].endswith("&") assert min(tunnel_bg) < tilt_lines[0] assert any(line == "wait" for line in lines) - assert any('trap "kill 0" EXIT' in line for line in lines) + # Check for EXIT trap that kills background jobs (jobs -p or kill 0) + assert any( + "trap " in line and ("jobs -p" in line or "kill 0" in line) and "EXIT" in line + for line in lines + ) assert not any("kill -0" in line for line in lines) assert not any("_tunnel_pid" in line for line in lines)