Deep Review: 20260420-095326-pr-328

			docker.DockerEndpoint: docker.EndpointMeta{Host: "unix://" + socketPath},
		},
	})
}

func deleteDockerContext(name string) error {
	s, err := newContextStore()
	if err != nil {
		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

store.ContextStore.Remove calls metadataStore.remove and tlsStore.remove; both wrap os.RemoveAll, which returns nil when the path is absent. The non-nil wrappers (fmt.Errorf("failed to remove metadata: %w", err) and failed to remove TLS data) carry no notFound interface, so errdefs.IsNotFound cannot match.

I verified this against cli@v29.2.0+incompatible/cli/context/store/{store.go:140-150,metadatastore.go:99-104,tlsstore.go:49-54}. The "second delete is a no-op" assertion in docker_context_test.go:68 passes because Remove returns nil unconditionally for a missing context, not because the guard fires.

	override := t.TempDir()
	t.Setenv("DOCKER_CONFIG", override)

	dir, err := dockerConfigDir()
	assert.NilError(t, err)
	assert.Equal(t, dir, override)

	// Context files land under DOCKER_CONFIG, not HOME/.docker.
	assert.NilError(t, createReplaceDockerContext("rancher-desktop-2", "/tmp/docker.sock"))
	_, err = os.Stat(filepath.Join(override, "contexts"))
	assert.NilError(t, err, "contexts/ must exist under DOCKER_CONFIG")

Fix: drop the guard, or attach a one-line comment explaining the intent is defensive against a future upstream change.

-    if err := s.Remove(name); err != nil && !errdefs.IsNotFound(err) {
-        return err
-    }
-    return nil
+    return s.Remove(name)

configfile.LoadFromReader decodes every auths[*].auth value with decodeAuth, which fails on invalid base64 or on a decoded payload missing : (cli@v29.2.0+incompatible/cli/config/configfile/file.go:95-112,265-285). The hand-rolled code only parsed currentContext and ignored the rest. A user with one corrupt auth entry will now hit an error inside getCurrentDockerContext / setCurrentDockerContext / clearCurrentDockerContext where the old code stayed quiet.

Impact is low and self-healing — one log line per reconcile until the user fixes their config — and the trade is reasonable for the smaller surface. Worth a sentence in the commit message and consider logging at info rather than error in manageDockerContext so a bad user-side config does not look like an rdd fault.

func createReplaceDockerContext(name, socketPath string) error {
	s, err := newContextStore()
	if err != nil {
		return err
	}
	return s.CreateOrUpdate(store.Metadata{
		Name:     name,
		Metadata: map[string]any{"Description": "Rancher Desktop " + name},
		Endpoints: map[string]any{
			docker.DockerEndpoint: docker.EndpointMeta{Host: "unix://" + socketPath},
		},
	})
}

func deleteDockerContext(name string) error {
	s, err := newContextStore()
	if err != nil {

The previous code passed Metadata: map[string]any{}. The new code injects a Description, which docker context ls surfaces as a column. Probably welcome, but the change is not mentioned in the commit message and no test reads it back, so a future refactor could drop it silently.

Fix: extend Test_createReplaceDockerContext to read the metadata back and assert the description round-trips, or note the intent in the commit message. The BATS test bats/tests/32-app-controllers/engine-docker.bats:576 (moby engine creates Docker context for the instance) is a natural place to assert the field on an integration run.

The pre-PR updateDockerConfig (commit d1c228f line 60/74) used MkdirAll(..., 0o700) and WriteFile(..., 0o600). The new path delegates to metadataStore.createOrUpdate, which uses MkdirAll(contextDir, 0o755) and atomicwriter.WriteFile(..., 0o644) (cli@v29.2.0+incompatible/cli/context/store/metadatastore.go:35,42). The contents are only socket paths — not sensitive — and the new modes match Docker CLI's defaults, so this is almost certainly intentional interop. Worth calling out so a future security scan does not flag it as a regression and so the rationale lives somewhere outside reviewer memory.

Fix: a sentence in the commit message noting the permission alignment with docker CLI is enough.

		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
// config.json.

docker.EndpointFromContext (cli@v29.2.0+incompatible/cli/context/docker/load.go:152-160) returns an error in two cases: the endpoint is absent ("cannot find docker endpoint in context") and the endpoint value has the wrong type ("endpoint %q is not of type EndpointMeta"). The comment only covers the first case. The configured EndpointTypeGetter should keep the type assertion succeeding, but a future reader who relies on the comment would be misled.

Fix: broaden the comment.

-        // No docker endpoint on this context — treat as empty, not an error.
+        // Missing or mistyped docker endpoint — treat as empty, not an error.
+        // The configured EndpointTypeGetter normally prevents the mistyped case.

// dockerContextProbeTimeout is the maximum time allowed to ping a Docker
// socket when checking the user's current context is healthy.
const dockerContextProbeTimeout = 3 * time.Second

// dockerConfigDir returns $DOCKER_CONFIG when it is set, otherwise ~/.docker.
// We compute this ourselves rather than calling config.Dir() because that
// caches its result via sync.Once, which defeats t.Setenv("HOME", …) in tests.
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
}

// newContextStore builds a store.Store rooted at ~/.docker/contexts and
// configured with Docker CLI's standard metadata + endpoint types, so the
// files it writes are interoperable with the docker CLI.
func newContextStore() (store.Store, error) {

Every helper in this file routes through dockerConfigDir. The function honours DOCKER_CONFIG first, but the comment still says "returns ~/.docker" with no mention of the env override.

Fix: state the precedence explicitly.

-// dockerConfigDir returns ~/.docker. We compute this ourselves rather than
-// calling config.Dir() because that caches its result via sync.Once, which
-// defeats t.Setenv("HOME", …) in tests.
+// dockerConfigDir returns $DOCKER_CONFIG when it is set, otherwise ~/.docker.
+// We compute this ourselves rather than calling config.Dir() because that
+// caches its result via sync.Once, which defeats t.Setenv("HOME", …) in tests.

The test's comment reads // Seed a config with an unrelated field., but auths is a typed top-level field that ConfigFile knows about. The PR description acknowledges that unknown top-level keys are dropped on save. Seeding with a truly unknown field (e.g. {"myCustomField": "value"}) would document that limitation directly; the current test only proves a known field round-trips. Comment vs. intent mismatch — no logic failure.

Fix: rename the seed to a known-irrelevant field ({"experimental":"enabled"}) and update the comment, or add a second sub-test that asserts an unknown field is dropped.