Deep Review: 20260424-130819-pr-333

    # host has a running Docker daemon at the default socket (e.g.
    # ubuntu-latest runners ship Docker pre-installed). Using a named context
    # also avoids the stale-env issue: the service process's DOCKER_HOST is
    # frozen at first start, so FromEnv-based probing of "" / "default" is
    # host-dependent and unreliable from inside a long-lived daemon.
    local probe_target="test-unreachable"
    run_e -0 docker_context_dir "${probe_target}"
    local probe_dir="${output}"
    mkdir -p "${probe_dir}"
    cat >"${probe_dir}/meta.json" <<EOF
{"Name":"${probe_target}","Endpoints":{"docker":{"Host":"unix:///nonexistent/does-not-exist.sock","SkipTLSVerify":false}}}

The round-1 test fix added two new local declarations at lines 790 and 792 (the diff against the round-1 base confirms both are new). The repo's BATS style rule flags local inside an @test block as redundant — each @test already runs in its own subshell. The pre-existing local context_name on line 780 is out of scope for this PR, but the two new ones are.

    run -0 jq -r '.Endpoints.docker.Host' "${meta_file}"
    assert_output "unix://${socket_path}"
}

@test "moby engine sets currentContext when no healthy context exists" {
    local context_name="rancher-desktop-${RDD_INSTANCE}"

    # Seed a named context pointing at an unreachable socket and make it
    # current. This forces the reconciler into the probeNamedDockerContext
    # branch with a guaranteed-failing endpoint, independent of whether the
    # host has a running Docker daemon at the default socket (e.g.

Fix:

-    local probe_target="test-unreachable"
+    probe_target="test-unreachable"
     run_e -0 docker_context_dir "${probe_target}"
-    local probe_dir="${output}"
+    probe_dir="${output}"

(suggestion, regression)

// When we cannot determine whether the context is healthy — because the store
// is unreadable, the endpoint metadata is missing or malformed, the scheme is
// not tcp/unix (e.g. SSH), or the TLS config cannot be loaded — we return true
// (assume healthy). This conservatism prevents RDD from clobbering a context
// that is in the middle of being edited or migrated.
func probeNamedDockerContext(ctx context.Context, name string) bool {
	log := logf.FromContext(ctx).WithName("docker-context").WithValues("context", name)

	s, err := newContextStore()
	if err != nil {
		log.V(1).Error(err, "Cannot open context store; assuming context healthy")
		return true
	}
	md, err := s.GetMetadata(name)
	if err != nil {
		if errdefs.IsNotFound(err) {
			return false
		}
		log.V(1).Error(err, "Cannot read context metadata; assuming context healthy")
		return true
	}
	epMeta, err := docker.EndpointFromContext(md)
	if err != nil {
		log.V(1).Error(err, "Cannot decode docker endpoint; assuming context healthy")
		return true
	}
	scheme, _, _ := strings.Cut(epMeta.Host, "://")
	switch scheme {
	case "unix", "tcp":
	default:
		log.V(1).Info("Non-tcp/unix endpoint scheme; assuming context healthy", "scheme", scheme)
		return true
	}
	ep, err := docker.WithTLSData(s, name, epMeta)
	if err != nil {
		log.V(1).Error(err, "Cannot load TLS data; assuming context healthy")
		return true
	}
	opts, err := ep.ClientOpts()
	if err != nil {
		log.V(1).Error(err, "Cannot build client options; assuming context healthy")
		return true
	}
	probeCtx, cancel := context.WithTimeout(ctx, dockerContextProbeTimeout)
	defer cancel()
	return pingDocker(probeCtx, opts...)
}

The new function has five distinct return paths (open-store error, NotFound → false, decode error, non-tcp/unix scheme → true, TLS/ClientOpts error, unreachable ping). Only the unreachable-unix-socket path has integration coverage via the BATS test; the unit-test file (docker_context_test.go) only renames getDockerContextHost calls to testGetContextHost and does not exercise the new function. The branches most likely to regress on a future rewrite — errdefs.IsNotFound returning false, and the ssh:// short-circuit returning true — have no unit test anchoring the contract the docstring describes.

Fix: add three table-driven cases to docker_context_test.go: (a) seed a context with Host: "ssh://user@host" and assert probeNamedDockerContext returns true; (b) call with a name the store does not contain and assert false; (c) seed a unix:// context pointing at a nonexistent socket and assert false. All three can run against the existing newDockerTestDir fixture. The TCP+TLS / mTLS / SkipTLSVerify paths — the motivation for the PR — deserve coverage too, and a seeded fake context store with TLS data plus an unreachable endpoint would pin the TLS construction without needing a live TLS daemon.

(suggestion, gap)

	if e != nil {
		e.stop()
	}
	r.removeDockerContext()
}

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

The function-level comment says RDD switches to its context when the user's current is "absent or unhealthy". After the refactor, "absent" (empty or "default" current) no longer unconditionally counts as "unhealthy" — probeDockerContext(ctx) pings the implicit default endpoint via FromEnv and returns true when a foreign daemon responds. docs/design/bootstrap.md carries the same stale contract:

• Line 58: *"It creates a rancher-desktop-2 docker context. If there is no current context, or if the context isn't accepting connections, then it makes the new context the default."*
• Line 86: *"rdd start has created the rancher-desktop-2 docker context, but it will only activate it if the current context doesn't exist, or is non-functional."*

Fix: update the docstring and both design-doc bullets to say empty / "default" current is now treated as "use Docker's implicit default endpoint, and only promote RDD when that endpoint does not respond."

(suggestion, documentation)

	_, err := getCurrentDockerContext()
	assert.Assert(t, err != nil, "malformed auths entry must surface as an error")
}

func Test_getDockerContextHost_missing(t *testing.T) {
	newDockerTestDir(t)
	host, err := testGetContextHost(t, "does-not-exist")
	assert.NilError(t, err)
	assert.Equal(t, host, "")
}

The commit deletes getDockerContextHost from production code and introduces a test-only testGetContextHost helper. The Test_getDockerContextHost_missing test calls the helper, not the removed production function, so a future reader grepping for getDockerContextHost lands on a test whose subject is gone. The other two call sites (Test_createReplaceDockerContext, Test_deleteDockerContext) already name their subject by the production function they exercise, so the pattern is inconsistent.

Fix: rename to match the helper, or fold the NotFound case into a Test_probeNamedDockerContext_missing that asserts probeNamedDockerContext returns false for an absent context (and delete the helper, which now has only one caller).

(suggestion, enhancement)

			// would overwrite a file we cannot safely round-trip.
			log.Error(err, "Failed to read current Docker context")
			return
		}

		var healthy bool
		if current != "" && current != "default" {
			healthy = probeNamedDockerContext(probeCtx, current)
		} else {
			healthy = probeDockerContext(probeCtx)
		}
		// Guard against writing currentContext after removeDockerContext has
		// already cancelled probeCtx and deleted the context directory.
		if !healthy && probeCtx.Err() == nil {
			if err := setCurrentDockerContext(contextName); err != nil {
				log.Error(err, "Failed to set current Docker context", "context", contextName)

The old code carried an explicit comment above this block: *"Probe the current context's host — not our own — to decide whether to promote ours. default and empty both mean no working context is set; treat them identically."* The refactor deleted the comment, and the new behavior (probe the implicit default via FromEnv so a foreign Docker daemon does not get clobbered) now has no call-site explanation. probeDockerContext's own docstring states what it does but not why the reconciler routes empty/default into it.

Fix: add a one-line comment above the branch — e.g. // Probe the implicit default endpoint (DOCKER_HOST or platform default socket) so a foreign Docker daemon is not overridden. — so a reader sees the intent without cross-referencing probeDockerContext.

(suggestion, documentation)

// 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, and createReplaceDockerContext (as well as newContextStore) delegates to the store's built-in path handling; both resolve %USERPROFILE%\.docker on Windows through os.UserHomeDir() with no extra work required on RDD's side. The Windows-specific work concentrates in one line of createReplaceDockerContext (the hard-coded "unix://" prefix) and in the probe client selection. Round 1 (S5) flagged the same inaccuracy and it was not picked up.

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): createReplaceDockerContext hard-codes "unix://"; Windows
+	// needs "npipe://" plus an npipe-aware probe client in
+	// probeDockerContext / probeNamedDockerContext. Track in a follow-up
+	// issue before shipping Windows support.

(suggestion, enhancement, round 1 S5 unaddressed)

func probeNamedDockerContext(ctx context.Context, name string) bool {
	log := logf.FromContext(ctx).WithName("docker-context").WithValues("context", name)

	s, err := newContextStore()
	if err != nil {
		log.V(1).Error(err, "Cannot open context store; assuming context healthy")
		return true
	}
	md, err := s.GetMetadata(name)
	if err != nil {
		if errdefs.IsNotFound(err) {

The five bailouts (store open error, metadata read error, endpoint decode error, TLS-data load error, ClientOpts error) all log at V(1). V(1) is off at the default verbosity, so an operator diagnosing "why didn't RDD promote its context?" sees no trace unless they raise verbosity first. Each bailout fires at most once per probe transition and covers a genuinely unusual condition (corrupt or partial Docker config), so the default level would not be noisy. The non-tcp/unix scheme branch at line 185 is the exception — SSH/npipe are common and the V(1).Info there is correct.

	}
	scheme, _, _ := strings.Cut(epMeta.Host, "://")
	switch scheme {
	case "unix", "tcp":
	default:
		log.V(1).Info("Non-tcp/unix endpoint scheme; assuming context healthy", "scheme", scheme)
		return true
	}
	ep, err := docker.WithTLSData(s, name, epMeta)
	if err != nil {
		log.V(1).Error(err, "Cannot load TLS data; assuming context healthy")

Fix: change the five log.V(1).Error calls to log.Error (default level). Leave the scheme-mismatch V(1).Info as-is.

(suggestion, enhancement)

    run -0 rdd ctl get ContainerNamespaces --namespace="${RDD_NAMESPACE}" --output=name
    refute_output
}

# --- Docker context management ---

# docker_context_dir returns the ~/.docker/contexts/meta/<hash> directory for
# the named context. Docker derives the sub-directory from sha256(name).
docker_context_dir() {
    local name="$1"
    local hash
    # sha256sum on Linux, shasum on macOS

The helper's body now returns ${DOCKER_CONFIG}/contexts/meta/${hash}, but the docstring still says ~/.docker/contexts/meta/<hash>. The two diverged with the DOCKER_CONFIG isolation change.

Fix: update the comment to say ${DOCKER_CONFIG}/contexts/meta/<hash>, or parameterize the description as "the contexts/meta/<hash> directory under DOCKER_CONFIG".

(suggestion, documentation)