Deep Review: 20260519-235059-pr-348

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.

// --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, round 9 S1 unaddressed)

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

(suggestion, enhancement, round 9 S2 unaddressed)

	}
	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 today, but the reader has to mentally verify the deferred call cannot delete the new dst. A boolean-guarded defer expresses intent more clearly.

(suggestion, enhancement, round 9 S3 unaddressed)

Only pkg/kuberlr/exec_windows.go:39 builds the struct with Err == nil today. The contract lives only in a comment at main.go:170-171; a future caller that omits Err silently loses its log output. 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

	)
	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)
	}
}

(suggestion, enhancement, round 9 S4 unaddressed)

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

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 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, round 9 S5 unaddressed)

	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody)
	if err != nil {
		return err
	}
	req.Header.Set("User-Agent", userAgent)
	resp, err := httpClient.Do(req)
	if err != nil {
		return err
	}
	defer resp.Body.Close()
	if resp.StatusCode != http.StatusOK {
		return fmt.Errorf("GET %s: %s", url, resp.Status)
	}
	_, err = io.Copy(w, io.LimitReader(resp.Body, maxBytes))
	return err
}

When a corporate proxy intercepts the connection and returns a custom 403 page, the operator sees GET https://dl.k8s.io/...: 403 Forbidden without an indication that traffic was intercepted. Including the Via and X-Cache response headers on non-2xx, or echoing the response body's first ~256 bytes, points the operator at the right team. Proxies typically embed enough text in the rejection to identify themselves.

(suggestion, enhancement)

	}
	return path, nil
}

// cachedPath returns the absolute path of the cached kubectl for v.
func cachedPath(v semver.Version) string {
	name := fmt.Sprintf("kubectl-v%d.%d.%d", v.Major, v.Minor, v.Patch)
	if runtime.GOOS == "windows" {
		name += ".exe"
	}
	return filepath.Join(CacheDir(), name)
}

// download fetches kubectl v from the upstream mirror, verifies its sha512
// against the mirror's published checksum, and renames the verified file to
// dst. A failure leaves no partial file behind for the next call to mistake
// for a valid binary.

The same runtime.GOOS == "windows" + .exe append appears in both cachedPath (line 88) and download (line 100). A refactor changing one and not the other lands a mismatched cache key vs. mirror URL; TestCachedPath (resolver_test.go:65) only checks the suffix, so the bug would slip through. Factor a kubectlBinaryName(v) helper that both call.

// against the mirror's published checksum, and renames the verified file to
// dst. A failure leaves no partial file behind for the next call to mistake
// for a valid binary.
func download(ctx context.Context, v semver.Version, dst string) error {
	base := fmt.Sprintf("%s/release/v%d.%d.%d/bin/%s/%s/kubectl", mirrorURL(), v.Major, v.Minor, v.Patch, runtime.GOOS, runtime.GOARCH)
	if runtime.GOOS == "windows" {
		base += ".exe"
	}
	want, err := fetchSha512(ctx, base+".sha512")
	if err != nil {
		return fmt.Errorf("fetching checksum: %w", err)

(suggestion, gap)

Both helpers run genericclioptions.NewConfigFlags(true).AddFlags(fs) and fs.Parse(args) on the same args within a single Resolve call (isClientOnly → loadClientConfig → parseKubeConfigFlags). The duplication risks the binding surface drifting between the two — a future change to ConfigFlags would update one site and miss the other. Share a single parsed ConfigFlags through Resolve, or split out the bare-bool resolver-specific binding (--client, --warnings-as-errors, --match-server-version) into a separate small function that isClientOnly composes onto whatever parseKubeConfigFlags returns.

(suggestion, gap)

The doc comment names "Exec sets it on the kubectl child process" but does not spell out RDD_KUBECTL_RESOLVED. A reader debugging a re-exec loop greps for the literal env-var name, not the Go identifier. Adding (RDD_KUBECTL_RESOLVED) matches how envCacheDir and envKubeMirror comment their constants.

(suggestion, documentation)

version: 2
updates:
- package-ecosystem: github-actions
  directory: /
  schedule:
    interval: daily

The bats/tests/10-cli/fake-kube/go.mod declares zero dependencies (stdlib only plus the fake-kube self-import). Dependabot's gomod ecosystem still scans the directory on every interval. Drop the second entry until the module grows a real dependency, or add a comment explaining the deliberate scaffold.

(suggestion, enhancement)

		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 {

The current JSON covers gitCommit, gitTreeState, buildDate, goVersion, compiler, platform. A future Kubernetes bump that adds a required field to k8s.io/apimachinery/pkg/version.Info would break discovery silently without the test noticing. Construct the value via the upstream struct and encoding/json.Marshal so the fixture stays trivially in sync.

(suggestion, gap)

    SERVER_PID=$!
    # setup_file and teardown_file run in separate subshells, so the env
    # var alone would vanish; save_var persists it via BATS_RUN_TMPDIR.
    save_var SERVER_PID
    # Wait for the port file. Server picks an ephemeral port, so we read it back.
    local i
    for i in {1..50}; do
        [[ -s ${PORT_FILE} ]] && break
        sleep 0.1
    done
    [[ -s ${PORT_FILE} ]] || fail "fake-kube-server did not write a port file"
    PORT=$(<"${PORT_FILE}")
    export PORT

    KUBECONFIG_PATH=${BATS_FILE_TMPDIR}/kubeconfig
    export KUBECONFIG_PATH

A flaky CI host that misses the 5-second budget fails with a generic message and no clue why the server failed to start. log.Fatalf errors from the server (e.g., "listen: bind: address already in use") go to stderr only, not to LOG_FILE. Capture stderr (2>"${BATS_FILE_TMPDIR}/server.stderr") and cat it on failure.

(suggestion, enhancement)

	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody)
	if err != nil {
		return err
	}
	req.Header.Set("User-Agent", userAgent)
	resp, err := httpClient.Do(req)
	if err != nil {
		return err
	}
	defer resp.Body.Close()
	if resp.StatusCode != http.StatusOK {
		return fmt.Errorf("GET %s: %s", url, resp.Status)
	}
	_, err = io.Copy(w, io.LimitReader(resp.Body, maxBytes))
	return err
}

resp.Body.Close() on an unread body prevents the keep-alive connection from being returned to the pool. For one-shot CLI use this does not matter, but the same pattern in a long-lived daemon (the TODO mentions k3s images, Lima templates) would leak connections. io.Copy(io.Discard, resp.Body) before return is the idiomatic fix.

(suggestion, gap)

The comment at resolver.go:218-224 documents that --client=garbage version (leading-flag form) halts pflag mid-args, lands in the empty-positionals branch, and returns true (treats as client-only). The existing test case "version --client=garbage stays false" at resolver_test.go:178 only exercises the trailing-flag form. Add the leading-flag case so the documented behavior is locked in:

// values do not leak into klog's process-global state; pflag's
// UnknownFlags handling consumes the assumed value of any spaced
// unknown flag, so `--v 4 config view` correctly leaves [config,
// view] as positionals.
//
// Bool-parse errors short-circuit pflag's parser: a malformed value
// like `--client=garbage version` halts parse mid-args, leaves no
// positionals, and lands in the empty-positionals branch below
// (return true). This deviates from a strict conservative bias, but
// kubectl rejects the same malformed flag in the embedded path
// before contacting any cluster, so no silent mismatched execution
// follows.
//
// Conservative bias: when isClientOnly cannot identify the
// invocation, prefer to probe; misclassifying as client-only would
// skip the version skew check and silently use a mismatched embedded
// kubectl.

		{"version --client=true", []string{"version", "--client=true"}, true},
		{"version --client=false", []string{"version", "--client=false"}, false},
		{"version --client=1", []string{"version", "--client=1"}, true},
		{"version --client then --client=false (last wins)", []string{"version", "--client", "--client=false"}, false},
		{"version --client=false then --client (last wins)", []string{"version", "--client=false", "--client"}, true},
		{"version --client=garbage stays false", []string{"version", "--client=garbage"}, false},
		{"version without --client", []string{"version"}, false},
		{"completion bash", []string{"completion", "bash"}, true},
		{"config view", []string{"config", "view"}, true},
		{"config get-contexts", []string{"config", "get-contexts"}, true},
		{"kustomize bare", []string{"kustomize"}, true},

+		{"--client=garbage leading halts parse, no positionals", []string{"--client=garbage", "version"}, true},

Separately, --client binds at the global level via fs.BoolVar(&client, "client", false, "") at resolver.go:251. Today this is harmless because isClientOnly only consults client when subcommand == "version" (line 262), but the binding's signal-to-noise would improve if the bool lived only when relevant. Either form is fine; documenting the choice or scoping the bind keeps the surface honest.

	fs.BoolVar(&matchServerVersion, "match-server-version", false, "")

	_ = fs.Parse(args)

	if help {
		return true
	}
	positionals := fs.Args()
	if len(positionals) == 0 {
		return true
	}

(suggestion, gap)