Deep Review: 20260502-225351-pr-348

// extractKubeFlags scans args for the kubectl flags Resolve cares about
// (--kubeconfig and --context) and returns their values. Both the spaced
// form (--kubeconfig path) and the equals form (--kubeconfig=path) match.
// A flag missing from args yields an empty string, which clientcmd treats
// as "use the default".
func extractKubeFlags(args []string) (kubeconfig, context string) {
	for i, arg := range args {
		switch {
		case arg == "--kubeconfig" && i+1 < len(args):
			kubeconfig = args[i+1]
		case strings.HasPrefix(arg, "--kubeconfig="):
			kubeconfig = strings.TrimPrefix(arg, "--kubeconfig=")
		case arg == "--context" && i+1 < len(args):
			context = args[i+1]
		case strings.HasPrefix(arg, "--context="):
			context = strings.TrimPrefix(arg, "--context=")
		}
	}
	return kubeconfig, context
}

extractKubeFlags scans every argument. cmd/rdd/kubectl.go:60 passes the same raw args to Resolve and to the kubectl child, but kubectl treats arguments after -- as positional. rdd kubectl exec mypod -- mycmd --kubeconfig=/other makes the resolver probe /other, while the real kubectl invocation still targets the original kubeconfig. The current consequence is benign — a stray probe goes to a context the user never asked for, returns ok=false, and the resolver falls through — but the parsed values lie about the user's intent.

	}
	return command
}

func kubectlAction(cmd *cobra.Command, args []string) error {
	path, err := kuberlr.Resolve(cmd.Context(), args)
	if err != nil {
		// A download or sha-mismatch failure surfaces here. We refuse
		// to silently fall back to the embedded kubectl — running a
		// version-mismatched binary against the user's cluster would
		// hide the failure behind weird kubectl errors. Apiserver-probe

Fix: stop scanning at the first --, and add unit tests covering both the spaced and = forms after the delimiter.

 for i, arg := range args {
+    if arg == "--" {
+        break
+    }
     switch {

	}
	info, err := client.ServerVersion()
	if err != nil {
		return semver.Version{}, false, nil //nolint:nilerr // unreachable cluster: kubectl can still run client-only commands
	}
	v, err := semver.ParseTolerant(info.GitVersion)
	if err != nil {
		return semver.Version{}, false, fmt.Errorf("parsing server version %q: %w", info.GitVersion, err)
	}
	return v, true, nil
}

// loadClientConfig builds a rest.Config that honors KUBECONFIG plus any
// --kubeconfig and --context flags in args, the way kubectl itself

The other three failure modes — no kubeconfig, malformed config, unreachable cluster — return ok=false, err=nil and let the embedded kubectl handle client-only commands. An unparseable info.GitVersion is logically the same situation: with no comparable version, the resolver cannot make an informed decision, so the documented fall-through policy applies. Today this branch errors out and blocks every kubectl invocation against that cluster, including kubectl config view and kubectl version --client that never needed a server version.

Fix:

     v, err := semver.ParseTolerant(info.GitVersion)
     if err != nil {
-        return semver.Version{}, false, fmt.Errorf("parsing server version %q: %w", info.GitVersion, err)
+        return semver.Version{}, false, nil //nolint:nilerr // unparseable server version: fall through to embedded
     }

	return ensureCached(ctx, server)
}

// compatible reports whether a kubectl of client.Minor can drive a server
// of server.Minor under the standard Kubernetes skew of ±1 minor.
func compatible(client, server semver.Version) bool {
	diff := int64(client.Minor) - int64(server.Minor)
	return diff >= -1 && diff <= 1
}

// embeddedVersion reads the kubectl version baked into the rdd binary
// from k8s.io/component-base/version, populated by -X linker flags in
// the Makefile.
func embeddedVersion() (semver.Version, error) {

A future server at v2.30.0 against an embedded kubectl v1.30.0 computes diff = 0 and returns true, silently routing through a major-mismatched embedded kubectl. Kubernetes 2.x has no realistic horizon, so the impact today is theoretical, but the fix is one line and the unit table at resolver_test.go:26-33 only covers Major == 1.

		name   string
		client semver.Version
		server semver.Version
		want   bool
	}{
		{"same minor", v(1, 30), v(1, 30), true},
		{"client one ahead", v(1, 31), v(1, 30), true},
		{"client one behind", v(1, 29), v(1, 30), true},
		{"client two ahead", v(1, 32), v(1, 30), false},
		{"client two behind", v(1, 28), v(1, 30), false},
		{"server at zero, client at zero", v(1, 0), v(1, 0), true},
		{"server at zero, client at one", v(1, 1), v(1, 0), true},
		{"patch differences ignored", semver.Version{Major: 1, Minor: 30, Patch: 99}, semver.Version{Major: 1, Minor: 30, Patch: 0}, true},
	}
	for _, tc := range cases {
		t.Run(tc.name, func(t *testing.T) {
			assert.Equal(t, compatible(tc.client, tc.server), tc.want)
		})

Fix:

 func compatible(client, server semver.Version) bool {
+    if client.Major != server.Major {
+        return false
+    }
     diff := int64(client.Minor) - int64(server.Minor)
     return diff >= -1 && diff <= 1
 }

Add a test case: {"different major", v(1, 30), semver.Version{Major: 2, Minor: 30}, false}.

# Service Commands

Service commands are for working with the control plane. The [Service API](api_service.md) document has additional information about the control plane internals, like startup and shutdown logic.

## Directories

The PR adds the kubectl_cache service path (cmd/rdd/service_paths.go:32) and introduces operator-facing RDD_CACHE_DIR (pkg/kuberlr/cache.go:22) and RDD_KUBECTL_MIRROR (pkg/kuberlr/downloader.go:30). docs/design/cmd_service.md lists no kubectl_cache key, and docs/design/environment.md describes neither variable. Verified against the worktree: neither file contains the new identifiers.

		"tls_dir":       instance.TLSDir(),
		"config":        instance.Config(),
		"pid_file":      instance.PIDFile(),
		"args_file":     instance.ArgsFile(),
		"docker_socket": instance.DockerSocket(),
		"kubectl_cache": kuberlr.CacheDir(),
	}
}

const (
	outputTable = "table"

)

// envCacheDir lets tests (and operators) override the rdd-wide cache root.
// Anticipates a shared rdd cache that future consumers (k3s images, lima
// templates) can join — kuberlr appends its own subdirectory.
const envCacheDir = "RDD_CACHE_DIR"

// CacheDir returns the directory holding downloaded kubectl binaries. All
// rdd instances share this cache; kubectl does not vary per instance.
// Setting RDD_CACHE_DIR overrides the rdd-wide root; kuberlr always
// appends kubectl/<os>-<arch>/ inside that root.

const defaultKubeMirror = "https://dl.k8s.io"

// envKubeMirror lets tests (and operators) point the resolver at an
// alternate mirror. Useful for offline mirrors and for the BATS cache
// lifecycle test, which serves a fake binary from a local HTTP server.
const envKubeMirror = "RDD_KUBECTL_MIRROR"

// mirrorURL returns the mirror base URL the resolver downloads from.
//
// TODO(offline): pair this with a "may we download?" toggle so air-gapped
// users can pre-populate CacheDir() and forbid network fetches.

Fix: add kubectl_cache to the paths table in cmd_service.md, and document RDD_CACHE_DIR (rdd-wide cache root with kubectl appending its own subdirectory) and RDD_KUBECTL_MIRROR (alternate release mirror, default https://dl.k8s.io) in environment.md. The path-vars table in environment.md should also gain RDD_KUBECTL_CACHE so rdd svc paths --output=shell is fully described.

	"os"
	"path/filepath"
	"runtime"
)

// envCacheDir lets tests (and operators) override the rdd-wide cache root.
// Anticipates a shared rdd cache that future consumers (k3s images, lima
// templates) can join — kuberlr appends its own subdirectory.
const envCacheDir = "RDD_CACHE_DIR"

// CacheDir returns the directory holding downloaded kubectl binaries. All
// rdd instances share this cache; kubectl does not vary per instance.
// Setting RDD_CACHE_DIR overrides the rdd-wide root; kuberlr always

The variable name promises an rdd-wide root, but kubectl is the sole consumer. If a future cache user adopts a different env var, the "rdd-wide" promise breaks and operators have to learn a second variable. Either rename to RDD_KUBECTL_CACHE_DIR so the name matches the actual scope (and reserve RDD_CACHE_DIR for the day a real shared cache lands), or scope the comment: "kubectl cache root; future consumers may layer their own subdirectories under the same value."

        KUBECONFIG="${KUBECONFIG_PATH}" \
        rdd kubectl get pods

    assert_output --partial 'fake-kubectl: get pods'
    assert_file_executable "${cached}"
    grep -q '^GET /version' "${LOG_FILE}"
    grep -q "^GET /release/v1.99.0/bin/${GOOS}/${GOARCH}/kubectl${EXE}.sha512$" "${LOG_FILE}"
    grep -q "^GET /release/v1.99.0/bin/${GOOS}/${GOARCH}/kubectl${EXE}$" "${LOG_FILE}"
}

@test 'rdd kubectl skips the download when the cache already has a match' {
    cache_subdir=${CACHE_DIR}/kubectl/${GOOS}-${GOARCH}
    mkdir -p "${cache_subdir}"

bats/helpers/utils.bash:91 defines refute_file_contains; the suite-wide assert_file_contains is used by bats/tests/20-service/4-start-parameters.bats and bats/tests/20-service/5-port-fallback.bats. The bats-assert idioms surface both the expected pattern and the actual file contents on failure, while bare grep -q only sets the exit status.

refute_file_exists() {
    assert_file_not_exists "$@"
}

refute_file_contains() {
    assert_file_not_contains "$@"
}

########################################################################

Fix sketch:

-    grep -q '^GET /version' "${LOG_FILE}"
-    grep -q "^GET /release/v1.99.0/bin/${GOOS}/${GOARCH}/kubectl${EXE}.sha512$" "${LOG_FILE}"
-    grep -q "^GET /release/v1.99.0/bin/${GOOS}/${GOARCH}/kubectl${EXE}$" "${LOG_FILE}"
+    assert_file_contains "${LOG_FILE}" '^GET /version'
+    assert_file_contains "${LOG_FILE}" "^GET /release/v1.99.0/bin/${GOOS}/${GOARCH}/kubectl${EXE}.sha512$"
+    assert_file_contains "${LOG_FILE}" "^GET /release/v1.99.0/bin/${GOOS}/${GOARCH}/kubectl${EXE}$"

    MIRROR_ROOT=${BATS_FILE_TMPDIR}/mirror
    MIRROR_BIN_DIR=${MIRROR_ROOT}/release/v1.99.0/bin/${GOOS}/${GOARCH}
    mkdir -p "${MIRROR_BIN_DIR}"
    # Build inside the helper module so go.mod resolution picks up
    # bats/fake-kube/go.mod, not the parent rancher-desktop-daemon one.
    (
        cd "${BATS_TEST_DIRNAME}/fake-kube"
        GOOS=${GOOS} GOARCH=${GOARCH} \
            go build -ldflags='-s -w' \
            -o "${MIRROR_BIN_DIR}/kubectl${EXE}" \
            ./kubectl
        go build -ldflags='-s -w' \
            -o "${BATS_FILE_TMPDIR}/fake-kube-server" \
            ./server
    )
    if is_macos; then
        shasum -a 512 "${MIRROR_BIN_DIR}/kubectl${EXE}" | awk '{print $1}' >"${MIRROR_BIN_DIR}/kubectl${EXE}.sha512"
    else
        sha512sum "${MIRROR_BIN_DIR}/kubectl${EXE}" | awk '{print $1}' >"${MIRROR_BIN_DIR}/kubectl${EXE}.sha512"
    fi

Both invocations target the host today because GOOS=$(go env GOOS) returns the host value, but the asymmetry hints at a forgotten cross-compile concern. Drop the explicit env on the kubectl build (host targets host) or pass it on both for consistency.

	gitVersionFile := flag.String("git-version-file", "", "optional file overriding gitVersion at request time")
	versionStatusFile := flag.String("version-status-file", "", "optional file overriding /version HTTP status at request time")
	portFile := flag.String("port-file", "", "file to receive the assigned port")
	logFile := flag.String("log-file", "", "file to receive one request line per request")
	flag.Parse()
	if *root == "" || *portFile == "" || *logFile == "" {
		log.Fatal("--root, --port-file, and --log-file are required")
	}

	// O_APPEND so per-test truncation (`: > log`) by BATS is honored:
	// without it, the server's fd position drifts past the new EOF and
	// writes land in a sparse hole that grep treats as binary noise.
	logFD, err := os.OpenFile(*logFile, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o644)
	if err != nil {
		log.Fatalf("opening log file: %v", err)
	}
	defer logFD.Close()
	var logMu sync.Mutex

	mux := http.NewServeMux()
	mux.HandleFunc("/version", func(w http.ResponseWriter, r *http.Request) {
		recordRequest(logFD, &logMu, r)
		status := readIntFile(*versionStatusFile, http.StatusOK)
		gv := readStringFile(*gitVersionFile, *gitVersion)
		w.Header().Set("Content-Type", "application/json")
		w.WriteHeader(status)
		if status == http.StatusOK {
			_, _ = fmt.Fprintf(w,
				`{"major":%q,"minor":%q,"gitVersion":%q,"gitCommit":"fake","gitTreeState":"clean","buildDate":"2026-01-01T00:00:00Z","goVersion":"go1.21","compiler":"gc","platform":"%s/%s"}`,
				*major, *minor, gv, runtime.GOOS, runtime.GOARCH,
			)
		}
	})
	mux.Handle("/release/", logging(logFD, &logMu, http.StripPrefix("/release/", http.FileServer(http.Dir(filepath.Join(*root, "release"))))))

	listener, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		log.Fatalf("listen: %v", err)
	}
	port := listener.Addr().(*net.TCPAddr).Port
	if err := os.WriteFile(*portFile, []byte(strconv.Itoa(port)), 0o644); err != nil {
		log.Fatalf("writing port file: %v", err)
	}

	server := &http.Server{Handler: mux}
	if err := server.Serve(listener); err != nil && err != http.ErrServerClosed {
		log.Fatalf("serve: %v", err)
	}
}

// logging wraps h so every request lands in the request log before h sees
// it. The release file server does not get to bypass logging just because
// it sits behind a StripPrefix.

log.Fatal* calls os.Exit(1), which skips deferred functions. The defer logFD.Close() at line 62 reads as if it cleans up on every exit path; in practice it runs only on a clean Serve return, which never happens during a test run. The cleanup is harmless inside a fixture, but the deferred call misleads a future reader. Either drop the defer (the OS reclaims the fd anyway) or replace log.Fatal* with log.Print* followed by an explicit os.Exit(1) after a single deferred cleanup block.

		RunE:               ctlAction,
	}
	return command
}

func ctlAction(cmd *cobra.Command, args []string) error {
	if err := ensureServiceRunning(cmd.Context()); err != nil {
		return err
	}
	if len(args) > 0 && args[0] == "wait-condition" {
		return ctlWaitConditionAction(cmd, args[1:])
	}
	if err := os.Setenv("KUBECONFIG", instance.Config()); err != nil {
		return fmt.Errorf("failed to set KUBECONFIG: %w", err)
	}
	return kubectlAction(cmd, args)
}

func newKubectlCommand() *cobra.Command {
	command := &cobra.Command{
		Use:                "kubectl",
		Short:              "Run the kubectl command",

rdd ctl shares kubectlAction, so it now performs a serverVersion probe against the embedded apiserver on every invocation. The probe always reports the matching version, compatible returns true, and Resolve returns "" — functional behavior is unchanged. The PR description's "rdd ctl stays untouched" is technically inaccurate.

Either update the description to "rdd ctl behavior is unchanged; the resolver always falls through because the embedded apiserver and kubectl share a version", or short-circuit Resolve for the embedded path. A check against instance.Config() would skip the (small) extra probe; it is purely cosmetic since the probe goes to a localhost socket.

ensureCached only adds; no eviction, no GC, no per-version TTL. Each downloaded kubectl is roughly 50 MB. A user who switches across several remote clusters over months accumulates indefinitely. Acceptable for v1, but a TODO(eviction) near the existing TODO(offline) comment lets a future consumer reuse the framing instead of reinventing it.

//	Linux:   ~/.cache/rancher-desktop/kubectl/<os>-<arch>/  ($XDG_CACHE_HOME)
//	Windows: %LocalAppData%\rancher-desktop\kubectl\<os>-<arch>\
func CacheDir() string {
	root := os.Getenv(envCacheDir)
	if root == "" {
		cache, err := os.UserCacheDir()
		if err != nil {
			panic(fmt.Errorf("could not determine user cache directory: %w", err))
		}
		root = filepath.Join(cache, "rancher-desktop")
	}
	return filepath.Join(root, "kubectl", runtime.GOOS+"-"+runtime.GOARCH)
}

CacheDir() is invoked from instancePaths(), which rdd svc paths calls. os.UserCacheDir() errors only when neither $HOME nor $XDG_CACHE_HOME (Linux) nor %LocalAppData% (Windows) resolves — extremely rare, but a panic in rdd svc paths is a poor error UX for a path-listing command. Either fall back to os.TempDir() with a logged warning, or change CacheDir to return (string, error) so the path command surfaces a normal error.