Repository navigation
fix(kubeconfig): isolate each file to stop shared-name collisions - #524
Merged
Merged
Conversation
…isions Radar passed every discovered kubeconfig file to client-go's Precedence merger, which resolves cluster/user/context names on a first-wins-by-name basis and silently drops later duplicates. When two files shared a user name (e.g. both defined `user: oidc`), the second file's credentials were discarded at load time; switching to that "second" context then sent the first file's token to the second file's server — hence the auth / network errors in the linked issues. Replace the merge with a per-file context registry. Each kubeconfig is loaded via ExplicitPath as its own island, and SwitchContext routes to a single file by looking up the qualified context name. Shared user/cluster names across files are now structurally impossible to collide. When two files happen to share a context name, the registry qualifies the later one with its source filename (e.g. "my-ctx (staging)"); the common case — distinct context names, shared user/cluster names — sees no rename. Covers #519 directly, plus #411 (can't switch clusters with multiple kubeconfigs) and #514 (OIDC only works for the first cluster), which share the same root cause. Also: KUBECONFIG env var with multiple paths uses the same isolated loader; single-file mode is unchanged. The CAPI MergeAndSwitchContext path promotes to isolated-load mode on first use and returns the qualified context name so callers switch correctly even when the CAPI context name collides with an existing registered one. Tests: four new table-driven cases in context_registry_test.go cover the no-collision, #519 shared-user-and-cluster, context-name-collision, and three-way-collision scenarios.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- SwitchContext: snapshot kubeconfigPaths and perFileConfigs under clientMu alongside contextRegistry so aggregateExecPluginCommands reads a consistent set; previously a concurrent MergeAndSwitchContext mutation could trigger a data race (flagged by the code-reviewer and silent-failure-hunter). - MergeAndSwitchContext: rewrite to serialize the whole reuse-or-create decision under one clientMu.Lock, eliminating the TOCTOU window where two concurrent CAPI merges could both see "no existing path" and both leak a temp file. Key capiKubeconfigs by the caller-provided contextName (stable across reconnects from the same workload cluster) rather than the qualified name, so the reuse path actually hits. Stage registry mutations into local maps and only commit to globals after successful validation — no more half-state if registration fails. Refresh perFileConfigs on overwrite so GetAvailableContexts reflects YAML changes. - buildContextRegistry: add log.Printf + errorlog.Record when a file fails to parse. The doc comment promised a log line; now the code matches. - Rename contextEntry.OriginalName → InFileName (reads better at call sites: entry.InFileName == cfg.CurrentContext is unambiguous). - Rename mergedContextCount → totalContextCount. With isolated loading there is no merge, so the old name was actively misleading; drop the "diff churn" excuse comment. - setupIsolatedLoad: clarify the comment about why unlocked global writes are safe (called from doInit inside initOnce, so no concurrent readers). Tests: - TestSwitchContextRouting_SharedNames_RoutesToCorrectFile: regression guard for #519 at the routing level — two files with shared user/cluster names but distinct tokens/servers; verifies each context's ExplicitPath load surfaces *its own* file's credentials, not the first file's. - TestAggregateExecPluginCommands_EmptyCommandScopedByFile: covers the empty-Command aggregation path so diagnostics correctly identify which file holds a malformed exec block. Verified green with `go test ./...` and `go test -race ./internal/k8s/`.
CodeQL flagged server.go's "Connected to workload cluster" log line (alerts 230, 231) because qualifiedName is derived from user-uploaded kubeconfig YAML — a crafted context name with CR/LF could inject forged log lines when Radar's stderr is scraped by a log aggregator. Radar ships as both a local tool and an in-cluster component, so the scraped-logs scenario is realistic enough to care about. Switch the affected format specifiers to %q across both CAPI log sites (server.go:2526 and client.go:927, 1007 — same shape, not yet flagged but would be on the next scan).
| // crafted context name can't inject forged log lines when Radar's stderr | ||
| // is scraped by a log aggregator. CodeQL alert "Log entries created from | ||
| // user input". | ||
| log.Printf("[capi] Connected to workload cluster %s/%s (context: %q, kubeconfig: %q)", ns, name, qualifiedName, mergedPath) |
| // crafted context name can't inject forged log lines when Radar's stderr | ||
| // is scraped by a log aggregator. CodeQL alert "Log entries created from | ||
| // user input". | ||
| log.Printf("[capi] Connected to workload cluster %s/%s (context: %q, kubeconfig: %q)", ns, name, qualifiedName, mergedPath) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #519. Also fixes #411 and fixes #514 — the three are the same root-cause bug viewed from different angles.
The bug
Radar handed every discovered kubeconfig file to client-go's
ClientConfigLoadingRules.Precedencemerger. That merge resolvesclusters,users, andcontextson a first-wins-by-name basis and silently drops later duplicates. So when two files share a user name (e.g. both defineuser: oidcoruser: me) or a cluster name, the second file's definition is thrown away at load time. Switching to the "second" context then ends up sending the first file's token to the first file's server — which is why:user: oidccollapsed into one plugin invocation.user: meandcluster: gitlab_kaslose one entry completely — the reporter explicitly links it to oidc auth via exec doesn't work #514.The existing
"Kubeconfig loaded: files=34, contexts=18"log line was the only hint: 16 contexts silently vanished during merge.The fix
Replace the merge with a per-file context registry. Each kubeconfig is loaded via
ExplicitPathas its own island.GetAvailableContextsenumerates the registry across all files;SwitchContextlooks up the qualified context name, resolves to the source file + original-name inside it, and loads only that file. Shared user/cluster names across files can't collide because they're never asked to — each ExplicitPath load sees only one file's maps.When two files happen to share a context name (not just a user or cluster name), the registry qualifies the second one with its source filename, e.g.
my-ctx (staging). The common case — distinct context names with shared user/cluster names — sees no rename and no visible UI change. That matches the #519 repro: both files haveuser: mebut contextskas-107andkas-108, which now both work under their original names.KUBECONFIG env vars with multiple colon-separated paths go through the same isolated loader. Single-file mode is unchanged. The CAPI flow (
MergeAndSwitchContext) promotes to isolated-load mode on first use and returns the qualified context name so the caller switches correctly even if the incoming CAPI context name collides with an existing one.Verification
context_registry_test.gocover no-collision, the exact Kubeconfig directory option merges configs #519 shared-user-and-cluster case, context-name collision (gets qualified), and a three-way collision (numeric-suffix fallback).cluster: shared-cluster/user: shared-userbut different server URLs. Both contexts are visible in the dropdown under their original names; switching to each one correctly routes to that file's server URL. Under the old code, both would have hit the first file's server.go test ./...) green.Diagnostic logging is now honest: the
contexts=Nnumber is the sum across all files rather than the post-merge count, so "files discovered vs contexts reachable" is alwaysN == Munless a file fails to parse.