fix: support multi-file KUBECONFIG with three-way merge#970
Conversation
The three-way merge code path used a shallow copy of EnvSettings which caused ConfigFlags.KubeConfig to point to the wrong struct. When KUBECONFIG contained multiple colon-separated paths, the entire string was passed as ExplicitPath to the k8s client loader, causing os.Stat() to fail with 'no such file or directory'. Fix by creating a fresh EnvSettings and clearing KubeConfig when it contains multiple paths, allowing the default loading rules to handle multi-file KUBECONFIG via the Precedence field. Signed-off-by: yxxhero <aiopsclub@163.com>
|
@copilot please fix ci issue. |
Agent-Logs-Url: https://github.com/databus23/helm-diff/sessions/6dac559a-e362-4546-a4d7-669c57b560ef Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes the three-way-merge code path in helm diff upgrade so it works with multi-file KUBECONFIG values (path-list) and correctly honors --kube-context when initializing the Kubernetes client.
Changes:
- Replace the shallow-copy
EnvSettingssetup with a dedicatedprepareEnvSettings()helper to avoid miswiredConfigFlagspointers and to clearKubeConfigwhen it contains multiple paths. - Add unit tests covering single vs multi-file
KUBECONFIGhandling and kube-context override behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/upgrade.go | Introduces prepareEnvSettings() and uses it for three-way-merge/take-ownership action initialization to support multi-file KUBECONFIG and correct kube-context wiring. |
| cmd/upgrade_test.go | Adds tests for prepareEnvSettings() behavior across single/multi-file kubeconfig and kube-context override scenarios. |
| func TestPrepareEnvSettings_MultiFileKubeconfig(t *testing.T) { | ||
| original := os.Getenv("KUBECONFIG") | ||
| defer os.Setenv("KUBECONFIG", original) | ||
|
|
There was a problem hiding this comment.
The test restores KUBECONFIG via original := os.Getenv(...) + defer os.Setenv(...), which can leave KUBECONFIG set to an empty string even if it was originally unset. Prefer t.Setenv (per subtest) or use os.LookupEnv and os.Unsetenv to restore the pre-test state accurately (and avoid ignoring os.Setenv errors).
|
|
||
| if env.KubeContext != "my-override-context" { | ||
| t.Errorf("env.KubeContext = %q, want %q", env.KubeContext, "my-override-context") | ||
| } |
There was a problem hiding this comment.
Subtest name suggests it verifies ConfigFlags/RESTClientGetter wiring, but it only asserts env.KubeContext. To catch the original regression, also assert that the context used by the REST client loader reflects the override (e.g., env.ConfigFlags.Context points to the right value and/or the raw kubeconfig loader resolves the expected current context).
| } | |
| } | |
| if env.ConfigFlags == nil { | |
| t.Fatal("env.ConfigFlags is nil") | |
| } | |
| if env.ConfigFlags.Context == nil { | |
| t.Fatal("env.ConfigFlags.Context is nil") | |
| } | |
| if got := *env.ConfigFlags.Context; got != "my-override-context" { | |
| t.Errorf("env.ConfigFlags.Context = %q, want %q", got, "my-override-context") | |
| } | |
| getter := env.RESTClientGetter() | |
| rawLoader := getter.ToRawKubeConfigLoader() | |
| rawCfg, err := rawLoader.RawConfig() | |
| if err != nil { | |
| t.Fatalf("RawConfig() returned error: %v", err) | |
| } | |
| if rawCfg.CurrentContext != "my-override-context" { | |
| t.Errorf("raw kubeconfig CurrentContext = %q, want %q", rawCfg.CurrentContext, "my-override-context") | |
| } |
|
|
||
| if env.KubeConfig != "/tmp/single-config" { | ||
| t.Errorf("env.KubeConfig = %q, want %q", env.KubeConfig, "/tmp/single-config") | ||
| } |
There was a problem hiding this comment.
This subtest says it preserves ExplicitPath, but it only checks env.KubeConfig. Consider also asserting that env.RESTClientGetter().ToRawKubeConfigLoader().ConfigAccess().GetExplicitFile() equals the single kubeconfig path so the test actually validates ExplicitPath behavior.
| } | |
| } | |
| getter := env.RESTClientGetter() | |
| rawConfig := getter.ToRawKubeConfigLoader() | |
| loadingRules := rawConfig.ConfigAccess() | |
| if loadingRules == nil { | |
| t.Fatal("ConfigAccess() = nil, want non-nil for single-file KUBECONFIG") | |
| } | |
| if explicitPath := loadingRules.GetExplicitFile(); explicitPath != "/tmp/single-config" { | |
| t.Errorf("ExplicitPath = %q, want %q", explicitPath, "/tmp/single-config") | |
| } |
Summary
Fixes #969
Multi-file
KUBECONFIG(e.g.,KUBECONFIG=/path/a:/path/b:/path/c) failed withHELM_DIFF_THREE_WAY_MERGE=truedue to the entire colon-separated string being passed asExplicitPathto the k8s client config loader, which calledos.Stat()on the unsplit path.prepareEnvSettings()helper that creates a freshEnvSettingsand clearsKubeConfigwhen it contains multiple paths, allowing the default loading rules to handle multi-file KUBECONFIG via thePrecedencefield--kube-contextflag being silently ignored in the three-way merge path (the previous shallow copy*localEnv = *envSettingsleftConfigFlags.Contextpointing to the wrong struct)