Deep Review: 20260417-123813-pr-320

		current, err := getCurrentDockerContext()
		if err != nil {
			log.Error(err, "Failed to read current Docker context")
		}

		// 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.
		var healthy bool
		if current != "" && current != "default" {
			currentHost, err := getDockerContextHost(current)
			if err != nil {
				log.Error(err, "Failed to resolve current Docker context host", "context", current)
			} else if currentHost != "" {
				healthy = probeDockerContext(probeCtx, currentHost)
			}
		}
		// 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 comment claims "" and "default" both mean "no working context", but default is a real Docker context: on Linux with Docker Engine installed it points to /var/run/docker.sock, which is typically running. By skipping the probe entirely for these values, the code leaves healthy=false and then unconditionally writes currentContext = "rancher-desktop-<suffix>", silently overriding the user's working local Docker daemon on the most common Linux configuration. Same gap on a freshly-installed Docker Desktop alternative where currentContext was never set.

This was raised as round 1 S2 and is unaddressed; Gemini correctly re-flags it (at Critical, downgraded here to Important because recovery is one docker context use default away).

Fix: probe the default endpoint instead of skipping it. The Docker client's FromEnv resolver picks up DOCKER_HOST and falls back to the platform default Unix socket:

 var healthy bool
 if current != "" && current != "default" {
     currentHost, err := getDockerContextHost(current)
     if err != nil {
         log.Error(err, "Failed to resolve current Docker context host", "context", current)
     } else if currentHost != "" {
         healthy = probeDockerContext(probeCtx, currentHost)
     }
+} else {
+    // "" or "default" → probe the implicit default endpoint
+    // (DOCKER_HOST or the platform default socket).
+    healthy = probeDockerContext(probeCtx, "")
 }

And teach probeDockerContext to use dockerclient.FromEnv when the host is empty:

 func probeDockerContext(ctx context.Context, host string) bool {
     probeCtx, cancel := context.WithTimeout(ctx, dockerContextProbeTimeout)
     defer cancel()
-    cli, err := dockerclient.New(dockerclient.WithHost(host))
+    var opts []dockerclient.Opt
+    if host != "" {
+        opts = append(opts, dockerclient.WithHost(host))
+    } else {
+        opts = append(opts, dockerclient.FromEnv)
+    }
+    cli, err := dockerclient.New(opts...)
     ...
 }

}

// 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) {
	path, err := contextMetaPath(name)
	if err != nil {
		return "", err
	}
	data, err := os.ReadFile(path)
	if errors.Is(err, os.ErrNotExist) {
		return "", nil
	}
	if err != nil {
		return "", err
	}
	var meta dockerContextMeta
	if err := json.Unmarshal(data, &meta); err != nil {
		return "", err
	}
	return meta.Endpoints["docker"].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.
func getCurrentDockerContext() (string, error) {

getDockerContextHost returns only Endpoints["docker"].Host, so any context that depends on extra transport state — TLS certificate paths in ~/.docker/contexts/tls/<hash>/, SSH options, mTLS, identity files — is probed as if it were a bare host URL. The client built with only WithHost cannot complete the TLS handshake or SSH dial, the Ping fails, and the goroutine treats the context as unhealthy and overrides it. This violates the PR's documented "leave healthy contexts alone" contract for any user whose currentContext is a TCP+TLS or SSH endpoint (a common pattern for CI runners and remote-engine setups).

Round 1's I2 fix moved the probe from unix://-prefix-stripping to full host pass-through, which closed the tcp:// URL-construction bug, but the deeper "context resolution requires more than the host URL" issue remained.

Fix: reconstruct the full Docker context client from the context store, not from the host URL alone. The smallest change is to read the TLS material from ~/.docker/contexts/tls/<hash>/docker/{ca,cert,key}.pem when present and pass it via WithTLSClientConfig. The cleaner change is to use github.com/docker/cli/cli/context/store (see Design Observations) so the probe inherits whatever the user configured.

			}
			r.contextMu.Unlock()
			cancel()
		}()

		current, err := getCurrentDockerContext()
		if err != nil {
			log.Error(err, "Failed to read current Docker context")
		}

		// 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.
		var healthy bool
		if current != "" && current != "default" {
			currentHost, err := getDockerContextHost(current)
			if err != nil {
				log.Error(err, "Failed to resolve current Docker context host", "context", current)
			} else if currentHost != "" {
				healthy = probeDockerContext(probeCtx, currentHost)
			}
		}
		// 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)
			}
		}
	}()
}

// removeDockerContext cancels any in-flight probe, waits for it to finish,
// then resets the current context if it points at our instance and deletes

When getCurrentDockerContext fails (transient EBUSY/EIO, antivirus lock on a CIFS-mounted $HOME, fs snapshot, etc.), the code logs and proceeds with current="". healthy stays false, and setCurrentDockerContext is called. That call routes through updateDockerConfig, which reads the file again. If the second read succeeds, the user's other keys are preserved but their chosen currentContext is silently replaced with ours. The user picked a specific context deliberately; the next time they run docker ps it talks to our VM instead.

Reviewer @mook-as raised this at discussion_r3102364628 and @Nino-K agreed (discussion_r3102730266: "I guess it would make sense to return here, since an error here means the config file exists but could not be read or parsed") but deferred to a Windows follow-up. Fix is one return:

 current, err := getCurrentDockerContext()
 if err != nil {
-    log.Error(err, "Failed to read current Docker context")
+    log.Error(err, "Failed to read current Docker context; skipping promotion to avoid clobbering user config")
+    return
 }

The user can still address our context explicitly via DOCKER_CONTEXT=rancher-desktop-<N> docker ...; not promoting on a read failure is strictly safer than promoting from a stale "".

# Engine controller tests — verify that the engine controller mirrors Docker
# containers, images, and volumes into Container, Image, and Volume
# resources, and that deletions and spec.state changes are forwarded
# to Docker.

local_setup_file() {
    # The Docker socket access pattern used by these tests is not yet wired
    # up for Windows/WSL2.
    skip_on_windows
    # 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
    # set, so no explicit --controllers selection is needed.
    rdd svc delete
    rdd set running=true
    run -0 rdd svc paths docker_socket
    export DOCKER_HOST="unix://${output}"
    # Mirror resources live in App.spec.namespace. Override RDD_NAMESPACE
    # to whatever the App was created with so the test queries the same
    # namespace the engine controller uses, regardless of CRD defaults.
    RDD_NAMESPACE=$(rdd ctl get app app -o jsonpath='{.spec.namespace}')
    export RDD_NAMESPACE
}

# --- Startup ---

@test "ContainerNamespace moby exists" {

local_setup_file exports DOCKER_HOST but does not isolate HOME or DOCKER_CONFIG. The "save → mutate → restore" inline block in the currentContext test rewrites the developer's real ~/.docker/config.json in place, and the restore only runs when control reaches the bottom of the test — any failed assertion above leaves currentContext cleared on the developer's machine. The base BATS teardown_file only stops rdd. There is no safety net.

This was raised as round 1 S5 and is unaddressed.

Fix (in order of preference):

1. Override DOCKER_CONFIG (or HOME) in local_setup_file to a per-suite temp directory. Once I5 lands, exporting DOCKER_CONFIG is the cross-platform-clean version (Windows uses USERPROFILE, not HOME).
2. Move the save/restore into a BATS teardown so it runs on failure too. The set -e/set -o pipefail framing inside try means a failed assertion above the inline restore aborts the test body without touching the cleanup.

type dockerEndpointMeta struct {
	Host          string `json:"Host"`
	SkipTLSVerify bool   `json:"SkipTLSVerify"`
}

func dockerConfigDir() (string, error) {
	home, err := os.UserHomeDir()
	if err != nil {
		return "", err
	}
	return filepath.Join(home, ".docker"), nil
}

// contextMetaPath returns the path to the meta.json file for the named context.
// Docker creates the directory name using sha256(contextName).
func contextMetaPath(name string) (string, error) {
	dir, err := dockerConfigDir()

The Docker CLI honours $DOCKER_CONFIG to relocate the config directory; it's standard for dev containers, CI runners, Nix profiles, and any user who keeps $HOME clean. With this implementation, the context meta file lands under ~/.docker/contexts/meta/<hash>/ while the user's CLI reads from $DOCKER_CONFIG/contexts/meta/<hash>/, so the new context is invisible. The promote step writes to ~/.docker/config.json for the same reason, and the user's actual config dir is left untouched.

This was raised as round 1 S3 and is unaddressed.

Fix:

 func dockerConfigDir() (string, error) {
+    if dir := os.Getenv("DOCKER_CONFIG"); dir != "" {
+        return dir, nil
+    }
     home, err := os.UserHomeDir()
     if err != nil {
         return "", err
     }
     return filepath.Join(home, ".docker"), nil
 }

Side benefit: the unit tests that currently do t.Setenv("HOME", ...) become t.Setenv("DOCKER_CONFIG", ...), which is portable to the Windows test runner where os.UserHomeDir() consults USERPROFILE.

// 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) {
	// Docker context management uses unix:// sockets; Windows requires
	// npipe:// which is not yet implemented.
	if runtime.GOOS == "windows" {
		return
	}
	contextName := instance.Name()
	log := logf.FromContext(r.watcherCtx).WithName("docker-context")

	if err := createReplaceDockerContext(contextName, socketPath); err != nil {
		log.Error(err, "Failed to create Docker context", "context", contextName)

@mook-as approved the PR conditionally (discussion_r3101962597): "Just needs to have an issue filed to keep track of the Windows thing I think." Neither a TODO: marker that grep TODO would surface nor a referenced issue number appears in the code. Without one, the asymmetry is invisible to a future contributor scanning for follow-up work.

The comment also understates the gap: only the URL scheme is called out, but probeDockerContext would also need a different client construction (named-pipe transport), and getCurrentDockerContext / createReplaceDockerContext would need npipe-aware host serialization.

Fix: file a tracking issue and reference it, or add a grep-able marker in both functions:

 func (r *EngineReconciler) manageDockerContext(socketPath string) {
-    // Docker context management uses unix:// sockets; Windows requires
-    // npipe:// which is not yet implemented.
+    // TODO(windows): Docker context management uses unix:// sockets;
+    // Windows requires npipe:// in the meta file, an npipe-aware probe
+    // client, and platform-specific path handling. Tracked in #<N>.
     if runtime.GOOS == "windows" {
         return
     }

Apply the same TODO + issue reference to removeDockerContext.