Deep Review: 20260421-132014-pr-333

		return err
	}
	return s.Remove(name)
}

// getDockerContextHost returns the full Docker host URL (e.g. "unix:///path/to/docker.sock"
// or "tcp://192.168.1.1:2376") for the named context's docker endpoint.
// Returns an empty string if the context does not exist or has no docker endpoint.
func getDockerContextHost(name string) (string, error) {
	s, err := newContextStore()
	if err != nil {
		return "", err
	}
	md, err := s.GetMetadata(name)
	if err != nil {
		if errdefs.IsNotFound(err) {
			return "", nil
		}
		return "", err
	}
	ep, err := docker.EndpointFromContext(md)
	if err != nil {
		// Missing or mistyped docker endpoint — treat as empty, not an error.
		// The configured EndpointTypeGetter normally prevents the mistyped case.
		return "", nil
	}
	return ep.Host, nil
}

// getCurrentDockerContext reads the currentContext field from ~/.docker/config.json.
// Returns an empty string if the file does not exist or no context is set.
// config.Load validates every "auths" entry; a malformed credential fails
// this call (and set/clearCurrentDockerContext) until the user repairs

After the probe-rewrite, engine_reconciler.go calls probeNamedDockerContext directly. A repo-wide grep shows getDockerContextHost now appears only in docker_context.go (the definition) and four spots in docker_context_test.go — the function survives solely to support its own unit tests.

Fix: delete getDockerContextHost; rewrite the three existing tests that use it (Test_createReplaceDockerContext, Test_deleteDockerContext, Test_getDockerContextHost_missing) to fetch the endpoint via newContextStore + docker.EndpointFromContext, or collapse them into a single probeNamedDockerContext unit test that exercises the same paths end-to-end.

// probeNamedDockerContext pings the Docker endpoint for the
// named context within dockerContextProbeTimeout.
// For contexts with non-tcp/unix endpoints (e.g. SSH), or with TLS config we can't load,
// we assume healthy to avoid overriding the user's choice.
func probeNamedDockerContext(ctx context.Context, name string) bool {
	s, err := newContextStore()
	if err != nil {
		return true
	}
	md, err := s.GetMetadata(name)
	if err != nil {
		return !errdefs.IsNotFound(err)
	}
	epMeta, err := docker.EndpointFromContext(md)
	if err != nil {
		return true
	}
	scheme, _, _ := strings.Cut(epMeta.Host, "://")
	switch scheme {
	case "unix", "tcp":
	default:
		return true
	}
	ep, err := docker.WithTLSData(s, name, epMeta)
	if err != nil {
		return true
	}
	opts, err := ep.ClientOpts()
	if err != nil {
		return true
	}
	probeCtx, cancel := context.WithTimeout(ctx, dockerContextProbeTimeout)
	defer cancel()
	return pingDocker(probeCtx, opts...)
}

Four error paths plus the non-NotFound branch all return true without logging. The "assume healthy" posture is the right default — the function must not stomp on a context it cannot decode — but an operator debugging "why does RDD refuse to become the default context?" gets no signal. A single V(1) log per bailout preserves the hot-path silence while making diagnosis tractable.

Fix: take a logr.Logger (or pull it from the context via logf.FromContext) and log the underlying err and name at V(1) before each return true. The caller in manageDockerContext already carries a log := logf.FromContext(r.watcherCtx).WithName("docker-context"); thread it through or have the probe accept a logger.

func probeNamedDockerContext(ctx context.Context, name string) bool {
	s, err := newContextStore()
	if err != nil {
		return true
	}
	md, err := s.GetMetadata(name)
	if err != nil {
		return !errdefs.IsNotFound(err)
	}
	epMeta, err := docker.EndpointFromContext(md)
	if err != nil {
		return true
	}
	scheme, _, _ := strings.Cut(epMeta.Host, "://")
	switch scheme {
	case "unix", "tcp":
	default:
		return true
	}
	ep, err := docker.WithTLSData(s, name, epMeta)
	if err != nil {
		return true
	}

The comment on the function says the optimistic return covers "non-tcp/unix endpoints (e.g. SSH), or with TLS config we can't load." EndpointFromContext upstream (github.com/docker/cli/.../load.go:153-160) returns an error when the context has no Docker endpoint at all or when the endpoint metadata fails to decode. That broader case — a malformed or missing endpoint — is not SSH-or-TLS; it is a context that docker itself cannot use. Treating it as healthy prevents RDD from helping a user whose Docker config is actually broken.

The counter-argument is conservatism: a partially corrupted config may be mid-edit, and stomping on it is worse than leaving it alone. Either position is defensible; the point is that the current code is more permissive than its docstring claims.

Fix: pick a stance and align the code with the comment. Either narrow the return true to the specific non-tcp/unix + TLS-load error paths, or broaden the comment to "any error decoding the context is treated as healthy to avoid clobbering a partially edited config."

    # Isolate all Docker config reads and writes from the developer's real
    # ~/.docker directory. Set DOCKER_CONFIG before starting the service so
    # the controller process inherits it (service.Start uses exec.Command
    # without an explicit Env, so it inherits the caller's environment).
    export DOCKER_CONFIG="${BATS_SUITE_TMPDIR}/docker-config"
    mkdir -p "${DOCKER_CONFIG}"

    # Deliberately skip setup_rdd_control_plane here: `rdd set` internally
    # calls ensureServiceRunning, which is exactly the CLI path we want to
    # exercise — the engine controller is part of the default controller

Every other test file in the repo that needs a tmpdir uses BATS_FILE_TMPDIR (see bats/tests/34-containers-controllers/containers-mock.bats:20, bats/tests/31-rdd-controllers/crd-upgrade.bats:16). BATS_SUITE_TMPDIR is shared across every file in the suite, so any future test file that also touches DOCKER_CONFIG would collide.

    # external mock-controller on the shared Container/Image/Volume
    # mirrors in the rancher-desktop namespace.
    setup_rdd_control_plane "containers,-engine"
    echo "${RDD_LOG_DIR}/mock-controller.log" >&3
    "mock-controller${EXE}" &>"${RDD_LOG_DIR}/mock-controller.log" &
    echo "$!" >"${BATS_FILE_TMPDIR}/controller_pid"
}

local_teardown_file() {
    if [[ -f "${BATS_FILE_TMPDIR}/controller_pid" ]]; then
        read -r controller_pid <"${BATS_FILE_TMPDIR}/controller_pid"

    rdd svc delete
    rdd svc start
}

local_teardown_file() {
    if [[ -f "${BATS_FILE_TMPDIR}/controller_pid" ]]; then
        kill "$(cat "${BATS_FILE_TMPDIR}/controller_pid")" 2>/dev/null || true
    fi
    rdd svc delete
}

Fix:

-    export DOCKER_CONFIG="${BATS_SUITE_TMPDIR}/docker-config"
+    export DOCKER_CONFIG="${BATS_FILE_TMPDIR}/docker-config"

// manageDockerContext creates the instance Docker context and, in a goroutine,
// probes the user's current context; if it is absent or unhealthy, switches
// the default to the new context. At most one probe runs at a time.
func (r *EngineReconciler) manageDockerContext(socketPath string) {
	// TODO(windows): Docker context management uses unix:// sockets; Windows
	// requires npipe:// in the context meta file, an npipe-aware probe client,
	// and platform-specific path handling in getCurrentDockerContext and
	// createReplaceDockerContext. Track in a follow-up issue before shipping
	// Windows support.
	if runtime.GOOS == "windows" {
		return
	}
	contextName := instance.Name()
	log := logf.FromContext(r.watcherCtx).WithName("docker-context")

getCurrentDockerContext delegates to docker/cli's config.Load, which already handles USERPROFILE on Windows via os.UserHomeDir(). The Windows-specific work concentrates in createReplaceDockerContext (scheme must be npipe://, socket path formatting differs) and in the probe client. Dropping getCurrentDockerContext from the list keeps the TODO honest.

Fix:

-    // TODO(windows): Docker context management uses unix:// sockets; Windows
-    // requires npipe:// in the context meta file, an npipe-aware probe client,
-    // and platform-specific path handling in getCurrentDockerContext and
-    // createReplaceDockerContext. Track in a follow-up issue before shipping
-    // Windows support.
+    // TODO(windows): Docker context management uses unix:// sockets; Windows
+    // requires npipe:// in the context meta file written by
+    // createReplaceDockerContext, and an npipe-aware probe client. Track in
+    // a follow-up issue before shipping Windows support.