Deep Review: 20260519-105717-pr-348

# winpath converts an MSYS-namespace path to its Windows-native
# equivalent (e.g. /tmp/x -> C:\msys64\tmp\x). Use it for env vars
# and command args that pass straight to a native Windows binary
# without traversing the rdd shell function's path-conversion path.
# Passes through unchanged on Linux and macOS.
winpath() {
    if is_msys; then
        cygpath -w "$1"
    else
        printf '%s' "$1"
    fi
}

# Convert 'export KEY="C:\win\path"' to POSIX paths (WSL or MSYS2).
win_to_posix_exports() {
    tr -d "\r" | while IFS= read -r line; do
        if [[ "${line}" =~ ^(export\ [A-Z_]+=)\"(.+)\"$ ]]; then

winpath only converts on MSYS. WSL is the other shell that calls a native Windows rdd.exe (commands.bash:4-6 sets EXE=.exe whenever using_windows_exe is true), and the existing repo convention for WSL path conversion is wslpath -w (see commands.bash:65-66 and paths.bash:26-27). On WSL the $(winpath ...) call sites pass Linux-form paths into rdd.exe, which cannot read them. The narrow fix:

EXE=""
PLATFORM=${OS}
if is_windows; then
    if using_windows_exe; then
        EXE=".exe"
        PLATFORM=win32
    else
        # WSL with Linux binary (not yet supported).
        PLATFORM=linux
    fi
fi

                fi
            done
        else
            # WSL: convert /mnt/... paths.
            for arg in "$@"; do
                if [[ "${arg}" != "${arg#/mnt/}" ]]; then
                    args+=("$(wslpath -w "${arg}")")
                else
                    args+=("${arg}")
                fi
            done
            # Adjust WSLENV to include everything starting with RDD_

# Convert 'export KEY="C:\win\path"' to POSIX paths (WSL or MSYS2).
win_to_posix_exports() {
    tr -d "\r" | while IFS= read -r line; do
        if [[ "${line}" =~ ^(export\ [A-Z_]+=)\"(.+)\"$ ]]; then
            if command -v wslpath >/dev/null 2>&1; then
                printf '%s"%s"\n' "${BASH_REMATCH[1]}" "$(wslpath -u "${BASH_REMATCH[2]}")"
            else
                printf '%s"%s"\n' "${BASH_REMATCH[1]}" "$(cygpath -u "${BASH_REMATCH[2]}")"
            fi
        fi
    done

 winpath() {
     if is_msys; then
         cygpath -w "$1"
+    elif is_windows && using_windows_exe; then
+        wslpath -w "$1"
     else
         printf '%s' "$1"
     fi
 }

A wider fix routes the call sites through the rdd() shell function (see I3); doing both is defensible because winpath is also used in local_setup_file for raw ${SERVER_BIN} args that never traverse the rdd() wrapper.

//
// 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.
func isClientOnly(args []string) bool {
	if len(args) == 0 {
		return true
	}
	fs := pflag.NewFlagSet("rdd-kubectl", pflag.ContinueOnError)
	fs.ParseErrorsAllowlist.UnknownFlags = true
	fs.SetOutput(io.Discard)
	fs.Usage = func() {}

	cf := genericclioptions.NewConfigFlags(true)
	cf.AddFlags(fs)

	var help, client, warningsAsErrors bool
	fs.BoolVarP(&help, "help", "h", false, "")
	fs.BoolVar(&client, "client", false, "")
	fs.BoolVar(&warningsAsErrors, "warnings-as-errors", false, "")

	_ = fs.Parse(args)

	if help {
		return true
	}
	positionals := fs.Args()
	if len(positionals) == 0 {
		return true
	}
	subcommand := positionals[0]
	if subcommand == "version" {
		// `kubectl version` (no --client) probes the server for its
		// version. Treat that as cluster-bound so the resolver picks
		// a version-matched binary to do the probe.
		return client
	}
	return clientOnlySubcommands[subcommand]
}

The pflag UnknownFlags handling at lines 217-224 documents — and the test at resolver_test.go:200 confirms — that an unknown spaced flag consumes its next token as the assumed value. kubectl --match-server-version config view therefore has pflag swallow config, leaves view as the sole positional, and view is not in clientOnlySubcommands, so the resolver probes the cluster for a command kubectl can answer locally. The fall-through is correct (no silent mismatched execution), but the user pays the 2-second probe penalty. --match-server-version is the obvious candidate to add; an audit of upstream staging/src/k8s.io/kubectl/pkg/cmd/cmd.go would surface any other bool globals kubectl declares above the subcommand layer.

		{"--server URL must not be read as subcommand", []string{"--server", "completion", "get", "pods"}, false},
		{"-n namespace before subcommand", []string{"-n", "kube-system", "get", "pods"}, false},
		{"-n with config", []string{"-n", "ns", "config", "view"}, true},
		{"--as-user-extra spaced before subcommand", []string{"--as-user-extra", "k=v", "config", "view"}, true},
		{"--as-user-extra equals before subcommand", []string{"--as-user-extra=k=v", "config", "view"}, true},
		{"unknown long flag swallows next non-flag arg as assumed value", []string{"--unknown", "config", "view"}, false},
		{"stops at --", []string{"exec", "pod", "--", "completion"}, false},
		{"-- before subcommand still finds the subcommand", []string{"--", "get", "pods"}, false},
		{"unknown --v spaced rides UnknownFlags path", []string{"--v", "4", "config", "view"}, true},
		{"unknown --v=N equals form rides UnknownFlags path", []string{"--v=4", "config", "view"}, true},
		{"unknown --vmodule spaced rides UnknownFlags path", []string{"--vmodule", "foo=2", "config", "view"}, true},

// (--v, --vmodule, --log_dir, …) intentionally stay unbound so their
// 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.

 var help, client, warningsAsErrors bool
 fs.BoolVarP(&help, "help", "h", false, "")
 fs.BoolVar(&client, "client", false, "")
 fs.BoolVar(&warningsAsErrors, "warnings-as-errors", false, "")
+var matchServerVersion bool
+fs.BoolVar(&matchServerVersion, "match-server-version", false, "")

A mirror of the existing --warnings-as-errors test cases locks the behavior in: {"--match-server-version before config", []string{"--match-server-version", "config", "view"}, true}.

const envSkipResolver = "RDD_KUBECTL_RESOLVED"

// skipResolver short-circuits Resolve for the rest of this process.
// Same-process toggle; envSkipResolver covers the cross-process recursion
// guard that Exec sets on the kubectl child.
var skipResolver bool

// SkipResolver short-circuits Resolve for the rest of this process.
// rdd ctl calls this before kubectlAction because it always targets
// the embedded apiserver, whose version matches the embedded kubectl
// 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

SkipResolver() is a one-way toggle on a package-global; nothing flips it back. TestResolveEmbeddedVersionUnparseable swaps embeddedVersion via t.Cleanup but never touches skipResolver, so a future test that calls SkipResolver() would leave the bit set for any later test that exercises Resolve, and Resolve short-circuits on line 69 before the path the test means to exercise. The test would still pass but for the wrong reason.

// 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 skipResolver || os.Getenv(envSkipResolver) != "" {
		return "", nil
	}
	if isClientOnly(args) {
		return "", nil
	}

In production the CLI process exits after one command, so the global is fine; the concern is purely test hygiene against future test additions.

 func TestResolveEmbeddedVersionUnparseable(t *testing.T) {
+    origSkip := skipResolver
+    t.Cleanup(func() { skipResolver = origSkip })
     orig := embeddedVersion
     t.Cleanup(func() { embeddedVersion = orig })

	)
	if err := cli.RunNoErrOutput(cmd); err != nil {
		// *cliexit.Error lets a subcommand opt into a specific exit code; everything else gets exit 1.
		// Empty 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 msg := err.Error(); msg != "" {
				logrus.Error(err)
			}
			os.Exit(exitErr.Code)
		}
		logrus.Fatal(err)
	}
}

The captured msg is unused; logrus.Error(err) calls err.Error() again. The empty-string check works today because cliexit.Error.Error() returns "" iff Err == nil. If a future caller wraps the cliexit.Error (e.g. fmt.Errorf("foo: %w", &cliexit.Error{Code: c})), the outer err.Error() is non-empty even when the inner sentinel marks "subcommand already wrote output," and the redundant log line returns. Checking exitErr.Err != nil states intent precisely and survives the wrap.

 if errors.As(err, &exitErr) {
-    if msg := err.Error(); msg != "" {
+    if exitErr.Err != nil {
         logrus.Error(err)
     }
     os.Exit(exitErr.Code)
 }

    assert_output --partial 'no_such_key'
    assert_output --partial 'valid keys'
}

@test 'rdd svc paths honors RDD_CACHE_DIR override' {
    cache_override=$(winpath "${BATS_TEST_TMPDIR}/cache-override")

    run -0 env RDD_CACHE_DIR="${cache_override}" rdd svc paths cache_dir
    assert_output "${cache_override}"

    run -0 env RDD_CACHE_DIR="${cache_override}" rdd svc paths kubectl_cache

The repo's BATS style rule prefers run -0 cmd ...; value=${output} over value=$(cmd ...) because command substitution discards the exit code and leaves the variable empty on failure. winpath delegates to cygpath -w on MSYS, which can fail in pathological cases. At the standalone assignment sites — 5-paths.bats:43 and the SERVER_BIN=... shape near 7-kubectl-cache.bats:39 — the run -0 form locks in the safety. Sites inside a run command line (most of 7-kubectl-cache.bats) cannot use run -0 recursively; those are unavoidable shell composition.

    assert_output --partial 'no_such_key'
    assert_output --partial 'valid keys'
}

@test 'rdd svc paths honors RDD_CACHE_DIR override' {
    cache_override=$(winpath "${BATS_TEST_TMPDIR}/cache-override")

    run -0 env RDD_CACHE_DIR="${cache_override}" rdd svc paths cache_dir
    assert_output "${cache_override}"

    run -0 env RDD_CACHE_DIR="${cache_override}" rdd svc paths kubectl_cache

            ./kubectl
        go build -ldflags='-s -w' \
            -o "$(winpath "${BATS_FILE_TMPDIR}/fake-kube-server${EXE}")" \
            ./server
    )
    SERVER_BIN=${BATS_FILE_TMPDIR}/fake-kube-server${EXE}

    PORT_FILE=${BATS_FILE_TMPDIR}/port
    LOG_FILE=${BATS_FILE_TMPDIR}/server.log
    GIT_VERSION_FILE=${BATS_FILE_TMPDIR}/git-version
    VERSION_STATUS_FILE=${BATS_FILE_TMPDIR}/version-status

The five 7-kubectl-cache.bats tests all pin the fake apiserver at v1.99.0, always out of skew with the embedded kubectl. The end-to-end coverage exercises the download lifecycle (miss, hit, mirror 404, sha mismatch, apiserver 500 fall-through) but never the within-skew branch where compatible() returns true and Resolve returns "" without probing the mirror. The within-skew rule is covered by TestCompatible at the unit level; an end-to-end test (e.g. set --git-version to v$(EMBEDDED_MINOR).0) would prove the whole resolver respects the rule end-to-end.

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

A cluster stepping through v1.30.5 → v1.30.6 → v1.30.7 accumulates three ~50 MB binaries even though any one would satisfy the skew rule. The eviction TODO in cache.go:33-39 addresses growth but not consolidation. A future enhancement could cache one binary per server minor (the highest patch the mirror knows about for that minor) and reuse it for any server in the same minor band. Within scope of the existing TODO; flag for sequencing.

//
//	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.
// SIGKILL or power loss during a download also leaks the .kubectl-*
// temp file that defer os.Remove normally cleans up. A per-version
// TTL or LRU sweep should clear both stale binaries and stale
// .kubectl-* leftovers before the directory becomes a noticeable
// footprint.
func CacheDir() string {
	return filepath.Join(CacheRoot(), "kubectl", runtime.GOOS+"-"+runtime.GOARCH)
}

// CacheRoot returns the rdd-wide cache root. RDD_CACHE_DIR overrides the