Deep Review: 20260519-131414-pr-348

	}
	assert.Assert(t, strings.HasSuffix(got, wantSuffix), "cachedPath = %q, want suffix %q", got, wantSuffix)
	assert.Assert(t, strings.HasPrefix(got, CacheDir()), "cachedPath = %q, want prefix %q", got, CacheDir())
}

func TestParseKubeConfigFlags(t *testing.T) {
	deref := func(p *string) string {
		if p == nil {
			return ""
		}
		return *p
	}
	derefBool := func(p *bool) bool {
		if p == nil {
			return false
		}
		return *p
	}

	type want struct {
		kubeconfig string
		context    string
		server     string
		cluster    string
		user       string
		token      string
		caFile     string
		certFile   string
		keyFile    string
		tlsName    string
		insecure   bool
		username   string
		password   string
		namespace  string
	}
	cases := []struct {
		name string
		args []string
		want want
	}{
		{"empty", nil, want{}},
		{"unrelated args", []string{"get", "pods"}, want{}},
		{"--kubeconfig spaced", []string{"--kubeconfig", "/k", "get", "pods"}, want{kubeconfig: "/k"}},
		{"--kubeconfig equals", []string{"--kubeconfig=/k", "get", "pods"}, want{kubeconfig: "/k"}},
		{"--context spaced", []string{"--context", "c", "get", "pods"}, want{context: "c"}},
		{"--context equals", []string{"--context=c", "get", "pods"}, want{context: "c"}},
		{"--server spaced", []string{"--server", "https://x:6443", "get"}, want{server: "https://x:6443"}},
		{"--server equals", []string{"--server=https://x:6443", "get"}, want{server: "https://x:6443"}},
		{"-s alias spaced", []string{"-s", "https://x:6443", "get"}, want{server: "https://x:6443"}},
		{"-s alias equals", []string{"-s=https://x:6443", "get"}, want{server: "https://x:6443"}},
		{"--cluster", []string{"--cluster=prod", "get"}, want{cluster: "prod"}},
		{"--user", []string{"--user=alice", "get"}, want{user: "alice"}},
		{"--token", []string{"--token=eyJabc", "get"}, want{token: "eyJabc"}},
		{"--certificate-authority", []string{"--certificate-authority=/ca.crt", "get"}, want{caFile: "/ca.crt"}},
		{"--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}},
		{"--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"}},
		{"stops at --", []string{"exec", "pod", "--", "tool", "--kubeconfig=/other", "--server=https://other"}, want{}},
		{"flags before -- still parse", []string{"--kubeconfig=/k", "--server=https://x", "exec", "pod", "--", "tool", "--context=other"}, want{kubeconfig: "/k", server: "https://x"}},
	}
	for _, tc := range cases {
		t.Run(tc.name, func(t *testing.T) {
			cf := parseKubeConfigFlags(tc.args)
			actual := want{
				kubeconfig: deref(cf.KubeConfig),
				context:    deref(cf.Context),
				server:     deref(cf.APIServer),
				cluster:    deref(cf.ClusterName),
				user:       deref(cf.AuthInfoName),
				token:      deref(cf.BearerToken),
				caFile:     deref(cf.CAFile),
				certFile:   deref(cf.CertFile),
				keyFile:    deref(cf.KeyFile),
				tlsName:    deref(cf.TLSServerName),
				insecure:   derefBool(cf.Insecure),
				username:   deref(cf.Username),
				password:   deref(cf.Password),
				namespace:  deref(cf.Namespace),
			}
			assert.Equal(t, actual, tc.want)
		})
	}
}

func TestIsClientOnly(t *testing.T) {
	cases := []struct {
		name string
		args []string

The want struct carries username and password fields and the comparison loop dereferences cf.Username / cf.Password, but no test case sets either flag in args. The docstring at resolver.go:168-175 justifies leaving them unbound to dodge ToRawKubeConfigLoader's interactive credential prompt; a future refactor that calls WithDeprecatedPasswordFlag would silently bind them and the test suite as written would not catch the regression. Add cases that pass the flags and assert the bound values remain empty:

// --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
// return the interactive variant that prompts on stdin when the
// kubeconfig context lacks credentials — the wrong behavior for a
// background version probe. The kubectl child still receives those
// flags and processes them per its own binding; the asymmetry only
// affects the resolver's probe, which falls through silently to the
// embedded kubectl on probe failure either way.
//
// parseKubeConfigFlags exists as a separate helper so unit tests can
// verify the binding without reaching for a real cluster.
func parseKubeConfigFlags(args []string) *genericclioptions.ConfigFlags {
	fs := pflag.NewFlagSet("rdd-kubectl", pflag.ContinueOnError)

+		{"--username unbound by design", []string{"--username=alice", "get"}, want{}},
+		{"--password unbound by design", []string{"--password=secret", "get"}, want{}},

(suggestion, gap)

	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

http.ErrServerClosed is a sentinel value; errors.Is(err, http.ErrServerClosed) is the standard idiom for sentinel comparison. The fixture never wraps the error today, so the equality compare works, but the code reads like production-shaped Go from a quick scan and the repo uses errors.Is for sentinels elsewhere.

-	if err := server.Serve(listener); err != nil && err != http.ErrServerClosed {
+	if err := server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) {

(suggestion, enhancement)

	}
	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)
	}
	if err := tmp.Close(); err != nil {
		return err
	}
	got := hex.EncodeToString(h.Sum(nil))
	if got != want {
		return fmt.Errorf("checksum mismatch for %s: want %s, got %s", base, want, got)
	}
	if err := os.Chmod(tmpName, 0o755); err != nil {
		return err
	}
	return os.Rename(tmpName, dst)
}

// Body-size caps for streamGet. maxSha512Bytes covers the longest
// `sha512sum`-format line (128 hex chars + two spaces + a generous
// filename); maxKubectlBytes covers the kubectl binary with headroom

After a successful Rename, tmpName no longer exists, so the deferred os.Remove returns ENOENT silently — harmless on both Unix and Windows. The reader still has to mentally verify the deferred call cannot delete the new dst (it cannot: Rename moves the inode, leaving tmpName empty). A small boolean-guarded defer expresses intent more clearly:

-	tmpName := tmp.Name()
-	defer os.Remove(tmpName)
+	tmpName := tmp.Name()
+	cleanup := true
+	defer func() {
+		if cleanup {
+			os.Remove(tmpName)
+		}
+	}()
...
-	return os.Rename(tmpName, dst)
+	if err := os.Rename(tmpName, dst); err != nil {
+		return err
+	}
+	cleanup = false
+	return nil

Purely an intent / readability change; no behavioral difference today.

(suggestion, enhancement)

	)
	if err := cli.RunNoErrOutput(cmd); err != nil {
		// *cliexit.Error lets a subcommand opt into a specific exit code; everything else gets exit 1.
		// A nil inner Err means the subcommand has already written its own output (e.g. kuberlr.Exec
		// propagating a kubectl child's exit code), so skip the redundant log line.
		var exitErr *cliexit.Error
		if errors.As(err, &exitErr) {
			if exitErr.Err != nil {
				logrus.Error(err)
			}
			os.Exit(exitErr.Code)
		}
		logrus.Fatal(err)
	}
}

The only producer of &cliexit.Error{Code: ...} with Err == nil today is pkg/kuberlr/exec_windows.go:39 (kubectl's child has already written its own stderr; rdd should not log on top of it). Round 8 S4's resolution captured the consumer-side change cleanly. The implicit contract still lives only in the comment at main.go:170-171; a future caller that builds &cliexit.Error{Code: 5} without an Err will silently lose its log output, which is the right behavior only if the caller has already written to stderr. A named constructor documents the convention at the producer site:

	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr
	err := cmd.Run()
	var exitErr *exec.ExitError
	if errors.As(err, &exitErr) {
		return &cliexit.Error{Code: exitErr.ExitCode()}
	}
	if err != nil {
		return fmt.Errorf("running %s: %w", path, err)
	}
	return nil

+// Silent wraps a kubectl-child exit code without an inner error. main.go
+// suppresses its log line in this case because the child has already
+// written its own stderr.
+func Silent(code int) *Error { return &Error{Code: code} }

(suggestion, enhancement)

// by construction — the probe would always fall through anyway.
func SkipResolver() {
	skipResolver = true
}

// serverProbeTimeout caps the discovery call. A reachable cluster answers
// in under 100 ms; an unreachable one would otherwise stall every kubectl
// invocation.
const serverProbeTimeout = 2 * time.Second

// Resolve returns the path to a kubectl binary compatible with the cluster
// the user's kubeconfig points at. An empty path means "use the embedded
// kubectl"; a non-empty path means "exec this binary instead". args holds
// the kubectl-style arguments the caller will pass through; Resolve binds

The comment's "under 100 ms" holds for LAN clusters and direct internet routes. A GKE / EKS / AKS control plane reached through a corporate split-tunnel VPN can routinely take 1-3 seconds to complete TLS handshake + IAM auth + return /version, especially on a cold connection. A spurious timeout falls through silently to the embedded kubectl (per serverVersion's contract at resolver.go:135-138), so an out-of-skew cluster reached over a slow VPN runs the embedded mismatched kubectl with no operator-visible reason. 5 seconds is the same order of magnitude and absorbs the realistic tail without weakening the unreachable-cluster guard.

	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
	}
	info, err := client.ServerVersion()
	if err != nil {
		logrus.WithError(err).Debug("kubectl resolver: cluster probe failed; using embedded kubectl")
		return semver.Version{}, false
	}
	v, err := semver.ParseTolerant(info.GitVersion)
	if err != nil {
		logrus.WithError(err).Warnf("kubectl resolver: cannot parse server version %q; using embedded kubectl", info.GitVersion)
		return semver.Version{}, false

(suggestion, enhancement)