fix(devtools): authenticate sudo before minikube tunnel

devtools/Makefile

@@ -166,14 +166,13 @@ 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
 	@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"
@@ -182,12 +181,23 @@ 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 '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 +356,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

test/unit/devtools/test_start_sh.py

@@ -0,0 +1,284 @@
+"""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")
+        # 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)
+        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)
+    # 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)