Deep Review: 20260503-142128-pr-348

load '../../helpers/load'

ALL_KEYS="args_file config dir kubectl_cache lima_home log_dir pid_file short_dir tls_dir"

@test 'rdd svc paths prints all keys in table format' {
    run -0 rdd svc paths
    for key in ${ALL_KEYS}; do
        assert_line --regexp "^${key} "

instancePaths() returns ten keys (cmd/rdd/service_paths.go:21-33): the nine in ALL_KEYS plus docker_socket. The BATS suite iterates ALL_KEYS to assert each key appears in rdd svc paths output, so docker_socket is silently exempt — a regression that drops docker_socket from the output ships green.

	"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/instance"
	"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/kuberlr"
)

func instancePaths() map[string]string {
	return map[string]string{
		"dir":           instance.Dir(),
		"log_dir":       instance.LogDir(),
		"short_dir":     instance.ShortDir(),
		"lima_home":     instance.LimaHome(),
		"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"
	outputJSON  = "json"

The same omission carries through both docs touched by this PR: environment.md's table lists nine RDD_* rows (no RDD_DOCKER_SOCKET), and the cmd_service.md example block (line 148–156) shows nine table rows (no docker_socket). The PR added the new kubectl_cache row to all three but did not add the docker_socket row that the implementation has emitted since the docker-socket-address PR landed.

Fix: include docker_socket everywhere these lists claim to be complete.

-ALL_KEYS="args_file config dir kubectl_cache lima_home log_dir pid_file short_dir tls_dir"
+ALL_KEYS="args_file config dir docker_socket kubectl_cache lima_home log_dir pid_file short_dir tls_dir"

Add an RDD_DOCKER_SOCKET row to the environment.md table and a docker_socket row to the cmd_service.md example. The durable fix derives the BATS key list from rdd svc paths itself rather than restating it.

    # Count /version requests to prove the resolver probed: the resolver
    # sends one, then the embedded kubectl's `version` subcommand sends
    # one of its own. A regression that short-circuits the resolver
    # would drop the count to 1 — assert_file_contains alone cannot
    # tell that case from this one.
    version_requests=$(awk '/^GET \/version$/ {n++} END {print n+0}' "${LOG_FILE}")
    [[ ${version_requests} -ge 2 ]] ||
        fail "expected >= 2 GET /version (resolver + embedded kubectl), got ${version_requests}: $(cat "${LOG_FILE}")"
}

The project's BATS style (deep-review-context.md and bats-style.md) explicitly forbids command substitution for command output: "Use run -0 instead of command substitution. run -0 fails with a useful error (command, exit code, output) if the command returns non-zero. A bare substitution gives an empty variable and a cryptic failure later." The new assertion uses both the forbidden capture form and a second $(cat …) inside the fail argument.

Fix:

-    version_requests=$(awk '/^GET \/version$/ {n++} END {print n+0}' "${LOG_FILE}")
-    [[ ${version_requests} -ge 2 ]] ||
-        fail "expected >= 2 GET /version (resolver + embedded kubectl), got ${version_requests}: $(cat "${LOG_FILE}")"
+    run -0 awk '/^GET \/version$/ {n++} END {print n+0}' "${LOG_FILE}"
+    version_requests=${output}
+    if [[ ${version_requests} -lt 2 ]]; then
+        run -0 cat "${LOG_FILE}"
+        fail "expected >= 2 GET /version (resolver + embedded kubectl), got ${version_requests}: ${output}"
+    fi

Round 3 fixed line 154 (the rdd svc paths example) per S1; the rest of the document still describes the file as config.json:

Examples:

```console
$ rdd svc paths
args_file      /path/to/rancher-desktop-default/rdd.args
config         /path/to/rancher-desktop-default/config.yaml
dir            /path/to/rancher-desktop-default
kubectl_cache  /path/to/Caches/rancher-desktop/kubectl/darwin-arm64
lima_home      /path/to/.rd2/lima
log_dir        /path/to/rancher-desktop-default/log
pid_file       /path/to/rancher-desktop-default/rdd.pid

• Line 15 — file table: config.json | service config settings
• Line 41 — rdd service create: "Stores the configuration settings in the config.json file."
• Line 78 — rdd service start: "starts the apiserver and the controller-manager using the configuration found in config.json."
• Line 80 — same paragraph: "No controller should access config.json directly."
• Line 87 — rdd service config: "updates config.json with latest settings."

pkg/instance/instance.go:91 returns config.yaml. Either rename all five at once, or leave the file alone — a partial rename causes new readers to chase the wrong filename.

	return filepath.Join(Dir(), "args.json")
})

// Config returns the path to the kubeconfig file.
var Config = sync.OnceValue(func() string {
	return filepath.Join(Dir(), "config.yaml")
})

// PIDFile returns the path to the service PID file.
var PIDFile = sync.OnceValue(func() string {
	return filepath.Join(Dir(), "rdd.pid")

Fix: apply the same rename at lines 15, 41, 78, 80, 87.

// digest, two spaces, and the filename — fields() takes the digest and
// drops the trailing filename so both formats verify. fetchSha512
// rejects non-128-hex tokens so a misconfigured mirror surfaces as a
// "malformed checksum" error rather than the misleading "checksum
// mismatch" the comparison at line 110 would otherwise produce.
func fetchSha512(ctx context.Context, url string) (string, error) {
	var sb strings.Builder
	if err := streamGet(ctx, url, &sb, maxSha512Bytes); err != nil {
		return "", err
	}
	fields := strings.Fields(sb.String())
	if len(fields) == 0 {
		return "", fmt.Errorf("empty checksum response from %s", url)
	}
	digest := fields[0]
	if len(digest) != sha512.Size*2 {
		return "", fmt.Errorf("malformed checksum from %s: %d chars, want %d", url, len(digest), sha512.Size*2)
	}
	for _, c := range digest {
		if !(c >= '0' && c <= '9' || c >= 'a' && c <= 'f' || c >= 'A' && c <= 'F') {
			return "", fmt.Errorf("malformed checksum from %s: non-hex character %q", url, c)
		}
	}
	return digest, nil
}

// streamGet performs a GET on url and copies the response body into w,
// capped at maxBytes. The cap turns a malicious or misconfigured mirror
// into a bounded failure: the underlying io.LimitReader stops reading

fetchSha512 admits A-F in the hex validator and returns the raw digest. download computes got := hex.EncodeToString(h.Sum(nil)) (lowercase by Go's spec) and compares with if got != want. A mirror populated by PowerShell Get-FileHash or any tool emitting uppercase hex passes validation, then surfaces a misleading "checksum mismatch" — exactly the misdirection the digest-format check was added to prevent.

Two coherent options: (a) reject uppercase in the validator (mirror operators normalise), or (b) accept and normalise in fetchSha512. (b) is the user-friendlier choice and a one-line edit.

-    digest := fields[0]
+    digest := strings.ToLower(fields[0])

The 128-hex length and character checks then run against the normalised form.

// and its value when it walks args looking for the subcommand. The
// list covers what kubectl exposes via genericclioptions.ConfigFlags
// and the klog flags. An unknown flag falls through as no-value;
// kubectl rejects unknown globals anyway, so the realistic miss set
// is empty.
var kubectlGlobalFlagsTakingValue = map[string]bool{
	"--kubeconfig":            true,
	"--context":               true,
	"--cluster":               true,
	"--user":                  true,
	"--namespace":             true,
	"-n":                      true,
	"--server":                true,
	"-s":                      true,
	"--token":                 true,
	"--certificate-authority": true,
	"--client-certificate":    true,
	"--client-key":            true,
	"--tls-server-name":       true,
	"--cache-dir":             true,
	"--request-timeout":       true,
	"--password":              true,
	"--username":              true,
	"--as":                    true,
	"--as-group":              true,
	"--as-uid":                true,
	"--profile":               true,
	"--profile-output":        true,
	"--log-backtrace-at":      true,
	"--log-dir":               true,
	"--log-file":              true,
	"--log-file-max-size":     true,
	"--log-flush-frequency":   true,
	"--stderrthreshold":       true,
	"-v":                      true,
	"--v":                     true,
	"--vmodule":               true,
}

// clientOnlySubcommands lists kubectl subcommands that never contact
// a cluster. config and completion manipulate local state only;
// options prints kubectl's global flags; help prints help text.
var clientOnlySubcommands = map[string]bool{

genericclioptions.NewConfigFlags(true).AddFlags binds --as-user-extra (a StringArrayVar, spaced form takes the next arg). The docstring at lines 160–162 names it explicitly:

// parseKubeConfigFlags binds genericclioptions.NewConfigFlags(true) to
// a pflag FlagSet, parses args, and returns the populated flags. The
// bound surface is whatever NewConfigFlags(true).AddFlags exposes —
// the kubectl connection-override flags (--kubeconfig, --context,
// --server/-s, --cluster, --user, --token, --certificate-authority,
// --client-certificate, --client-key, --tls-server-name,
// --insecure-skip-tls-verify, --request-timeout, --as, --as-group,
// --as-uid, --as-user-extra, --cache-dir, --disable-compression,
// --namespace/-n). Unknown flags (kubectl subcommands and their own
// flags) and positional arguments pass through silently.
//
// The deprecated --username and --password flags stay unbound. Binding
// them via WithDeprecatedPasswordFlag would make ToRawKubeConfigLoader

// --insecure-skip-tls-verify, --request-timeout, --as, --as-group,
// --as-uid, --as-user-extra, --cache-dir, --disable-compression,

But the value-taking map omits it. isClientOnly walks args manually; --as-user-extra k=v help lands at arg = "--as-user-extra", the map lookup misses, the walker advances by one (no value consumed), the next arg k=v becomes the subcommand candidate, and clientOnlySubcommands["k=v"] returns false — so the resolver probes the cluster for an invocation that should have short-circuited.

Net effect: a help / completion / config invocation that uses --as-user-extra in spaced form pays the 2 s probe cost. No correctness break (the kubectl child still receives the args correctly), but it regresses the Round 2 isClientOnly work for one flag.

Fix: add the entry.

     "--as-group":              true,
     "--as-uid":                true,
+    "--as-user-extra":         true,
     "--profile":               true,

The longer-term option is to drive isClientOnly from the same pflag set parseKubeConfigFlags already builds; see Design Observations.

//
// TODO(offline): when no cached kubectl matches and the network is down,
// Resolve currently fails with the download error. Add an opt-out for
// download attempts (config flag or RDD_KUBECTL_OFFLINE) and fall back to
// the cached binary closest to the server's minor.
func Resolve(ctx context.Context, args []string) (string, error) {
	if os.Getenv(envSkipResolver) != "" {
		return "", nil
	}
	if isClientOnly(args) {
		return "", nil
	}
	embedded, err := embeddedVersion()
	if err != nil {
		// `go run ./cmd/rdd kubectl ...` and IDE debug builds skip the
		// Makefile's -ldflags -X, so componentbasever returns the
		// in-tree default `v0.0.0-master+$Format:%H$`, which fails
		// semver parsing. Fall through to the embedded kubectl rather
		// than break every dev invocation.
		logrus.WithError(err).Debug("kubectl resolver: embedded version not parseable; using embedded kubectl")
		return "", nil
	}
	server, ok, err := serverVersion(args)
	if err != nil {
		return "", err
	}
	if !ok {
		return "", nil
	}
	if compatible(embedded, server) {
		return "", nil
	}
	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. A
// different Major rules out compatibility outright.
func compatible(client, server semver.Version) bool {

Resolve forwards ctx only to ensureCached. The discovery client at line 133 uses cfg.Timeout to cap each request at 2 s, but does not honour ctx.Done(). Cancellation does not propagate while the probe is in flight; a user pressing Ctrl+C waits up to 2 s per kubectl invocation.

	client, err := discovery.NewDiscoveryClientForConfig(cfg)
	if err != nil {
		logrus.WithError(err).Debug("kubectl resolver: cannot build discovery client; using embedded kubectl")
		return semver.Version{}, false, nil //nolint:nilerr // legitimate fall-through to embedded kubectl
	}
	info, err := client.ServerVersion()
	if err != nil {
		logrus.WithError(err).Debug("kubectl resolver: cluster probe failed; using embedded kubectl")
		return semver.Version{}, false, nil //nolint:nilerr // legitimate fall-through to embedded kubectl
	}
	v, err := semver.ParseTolerant(info.GitVersion)

Bounded UX issue, not a correctness issue (the embedded kubectl runs after the probe times out either way). Low priority because the ensureCached path already honours ctx and the 2 s bound puts a ceiling on the wait.

Fix: thread ctx through and bind it to the request. The discovery client exposes the underlying REST client:

-func serverVersion(args []string) (_ semver.Version, ok bool, _ error) {
+func serverVersion(ctx context.Context, args []string) (_ semver.Version, ok bool, _ error) {
     cfg, err := loadClientConfig(args)
     ...
-    info, err := client.ServerVersion()
+    body, err := client.RESTClient().Get().AbsPath("/version").DoRaw(ctx)
+    var info apimachineryversion.Info
+    if err == nil { err = json.Unmarshal(body, &info) }

const (
	maxSha512Bytes  = 4 << 10   // 4 KiB
	maxKubectlBytes = 256 << 20 // 256 MiB
)

// fetchSha512 downloads the sha512 hex digest at url. dl.k8s.io serves
// the bare digest, but a mirror populated with `sha512sum` writes the
// digest, two spaces, and the filename — fields() takes the digest and
// drops the trailing filename so both formats verify. fetchSha512
// rejects non-128-hex tokens so a misconfigured mirror surfaces as a
// "malformed checksum" error rather than the misleading "checksum
// mismatch" the comparison at line 110 would otherwise produce.
func fetchSha512(ctx context.Context, url string) (string, error) {
	var sb strings.Builder
	if err := streamGet(ctx, url, &sb, maxSha512Bytes); err != nil {
		return "", err
	}

Hard-coded line numbers in comments rot on the next edit (and have already drifted by one). Replace the locator with a name.

-// "malformed checksum" error rather than the misleading "checksum
-// mismatch" the comparison at line 110 would otherwise produce.
+// "malformed checksum" error rather than the misleading "checksum
+// mismatch" that download's digest comparison would otherwise produce.

		{"--client-certificate", []string{"--client-certificate=/c.crt", "get"}, want{certFile: "/c.crt"}},
		{"--client-key", []string{"--client-key=/c.key", "get"}, want{keyFile: "/c.key"}},
		{"--tls-server-name", []string{"--tls-server-name=api.example", "get"}, want{tlsName: "api.example"}},
		{"--insecure-skip-tls-verify bare", []string{"--insecure-skip-tls-verify", "get"}, want{insecure: true}},
		{"--insecure-skip-tls-verify=true", []string{"--insecure-skip-tls-verify=true", "get"}, want{insecure: true}},
		{"--insecure-skip-tls-verify=false", []string{"--insecure-skip-tls-verify=false", "get"}, want{insecure: false}},
		{"--namespace spaced", []string{"--namespace", "kube-system", "get"}, want{namespace: "kube-system"}},
		{"-n alias", []string{"-n", "kube-system", "get"}, want{namespace: "kube-system"}},
		{"all auth/tls together", []string{"--server=https://x", "--token=tk", "--certificate-authority=/ca", "--insecure-skip-tls-verify", "get"}, want{server: "https://x", token: "tk", caFile: "/ca", insecure: true}},
		{"flag at end without value is ignored", []string{"get", "--server"}, want{}},
		{"later flag wins", []string{"--context=a", "--context=b"}, want{context: "b"}},

NewConfigFlags(true) initialises Insecure to *bool → false. The test asserts the post-parse value equals false — the default. The row passes even when the flag binding is absent.

Two fixes: drop the row (the bare and =true rows already cover the binding), or force a non-default starting state and assert the parser flipped it back.

// option 2: force a true → false transition
cf := parseKubeConfigFlagsFor("--insecure-skip-tls-verify=false", "get")
*cf.Insecure = true                  // would be overwritten by re-parse; not viable for table tests

The table-test shape favours dropping the row.

	}
	want, err := fetchSha512(ctx, base+".sha512")
	if err != nil {
		return fmt.Errorf("fetching checksum: %w", err)
	}
	tmp, err := os.CreateTemp(filepath.Dir(dst), ".kubectl-*")
	if err != nil {
		return err
	}
	tmpName := tmp.Name()
	defer os.Remove(tmpName)
	h := sha512.New()
	if err := streamGet(ctx, base, io.MultiWriter(tmp, h), maxKubectlBytes); err != nil {
		tmp.Close()
		return fmt.Errorf("downloading %s: %w", base, err)
	}

Normal failure paths and orderly cancellation hit the defer; SIGKILL during a slow download or a sudden power loss leave a ~50 MB temp file behind. Each subsequent failure adds another. The eviction TODO in cache.go:33-36 does not currently target temp leftovers either.

//
//	macOS:   ~/Library/Caches/rancher-desktop/kubectl/<os>-<arch>/
//	Linux:   ~/.cache/rancher-desktop/kubectl/<os>-<arch>/  ($XDG_CACHE_HOME)
//	Windows: %LocalAppData%\rancher-desktop\kubectl\<os>-<arch>\
//
// TODO(eviction): the cache only grows; a user switching across many
// remote clusters accumulates ~50 MB per minor version indefinitely.
// Pair with a per-version TTL or LRU sweep before the directory becomes
// a noticeable footprint.
func CacheDir() string {
	root := os.Getenv(envCacheDir)
	if root == "" {
		cache, err := os.UserCacheDir()
		if err != nil {

Fix: sweep *.kubectl-* from CacheDir() once at the start of ensureCached, or fold a sweep into the eventual eviction pass. Either fix is small enough to land alongside the eviction work; flag here so it is not forgotten.

// 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"

// downloadTimeout caps each mirror request. Five minutes turns a hung
// mirror into a bounded failure and still covers a ~50 MB kubectl
// binary on slow links (~170 kB/s).
const downloadTimeout = 5 * time.Minute

// httpClient enforces downloadTimeout on every kuberlr request.
// http.DefaultClient sets no Timeout, and the inherited cobra context
// carries no deadline. If the request context later carries a shorter
// deadline, that deadline wins.
var httpClient = &http.Client{Timeout: downloadTimeout}

// 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.

The comment estimates "covers a ~50 MB kubectl binary on slow links (~170 kB/s)" — exactly five minutes at that rate, leaving zero headroom for TLS handshake or initial latency, and failing outright below that throughput. Round 3 added the deadline-composition note (a backstop). Round 4 raises the magnitude question: 5 minutes assumes a fast-enough link, and the user on a slow link sees a hard "context deadline exceeded" with no obvious diagnosis.

Two coherent options: (a) bump downloadTimeout to 10 minutes (covers ~85 kB/s links), or (b) move from Client.Timeout to a transport with ResponseHeaderTimeout so only stalls (not slow throughput) trip the cap. (b) is the durable fix; (a) is a one-character edit.