From af0e2a11afd4bcd63aae184a9253279a87cae5dc Mon Sep 17 00:00:00 2001 From: Ethan Pini Date: Mon, 14 Apr 2025 15:19:04 -0700 Subject: [PATCH] kubectl: Fix current-context being written to wrong file This is what happens when writing back a OIDC refresh token: - plugin/pkg/client/auth/oidc/oidc.go:282 Calls `Persist` to save the new refresh token. - tools/clientcmd/config.go:372 Calls `ModifyConfig` to save the config. - tools/clientcmd/config.go:167 Calls `configAccess.GetLoadingPrecedence()` to get the files listed from the `KUBECONFIG` environment variable. - tools/clientcmd/loader.go:334 If the `ConfigAccess` was a `ClientConfigLoadingRules`, it directly returns the `Precedence` slice from its `rules` field. THE PROBLEM: The slice can be modified by the caller, unintentionally changing the value of the `ClientConfigLoadingRules`' `Precedence` field. - tools/clientcmd/config.go:170 Then proceeds to in-place sort the slice returned by the `ConfigAccess`. This is the same slice (by identity) as the `ClientConfigLoadingRules`' `Precedence` field, destroying its intended order. - tools/clientcmd/config.go:179 Calls `configAccess.GetStartingConfig` to read the original config so it can be compared with the new config. - tools/clientcmd/loader.go:339 Calls `NewNonInteractiveDeferredLoadingClientConfig` with itself as a parameter. CONSEQUENCE: At this point, its the `Precedence` has been unintentionally sorted. When it loads the config again, it gives precedence to whichever file comes first in ascending alphabetical order. - tools/clientcmd/config.go:192 If the file returned by `GetStartingConfig` has a different `current-context` than the new config, it calls `writeCurrentContext` to update the first kubeconfig file in the `KUBECONFIG` environment variable. - tools/clientcmd/config.go:403 Calls `configAccess.GetDefaultFilename` to find the destination kubeconfig file. - tools/clientcmd/loader.go:358 Iterates through the kubeconfig files returned by `GetLoadingPreferences` to find the first file that exists. CONSEQUENCE: With the slice being sorted earlier, the files returned by this call of `GetLoadingPreferences` will be sorted alphabetically, rather than by their intended order. Kubernetes-commit: ffa084f81129ea685b176a282921c4d54906c539 --- tools/clientcmd/config_test.go | 119 +++++++++++++++++++++++++++++++++ tools/clientcmd/loader.go | 5 +- 2 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 tools/clientcmd/config_test.go diff --git a/tools/clientcmd/config_test.go b/tools/clientcmd/config_test.go new file mode 100644 index 00000000..74e9ab4d --- /dev/null +++ b/tools/clientcmd/config_test.go @@ -0,0 +1,119 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package clientcmd + +import ( + "fmt" + "os" + "path/filepath" + "testing" +) + +func TestModifyConfigWritesToFirstKubeconfigFile(t *testing.T) { + const ( + contextNameA = "context-a" + contextNameB = "context-b" + newContextName = "new-context" + ) + + tempdir := t.TempDir() + configFile1, _ := os.Create(filepath.Join(tempdir, "kubeconfig-a")) + configFile2, _ := os.Create(filepath.Join(tempdir, "kubeconfig-b")) + + // The first kubeconfig has everything. + err := os.WriteFile(configFile1.Name(), []byte(` +kind: Config +apiVersion: v1 +clusters: +- cluster: + api-version: v1 + server: https://kubernetes.default.svc:443 + certificate-authority: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt + name: kubeconfig-cluster +contexts: +- context: + cluster: kubeconfig-cluster + namespace: default + user: kubeconfig-user + name: `+contextNameA+` +current-context: `+contextNameA+` +users: +- name: kubeconfig-user + user: + tokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token +`), os.FileMode(0755)) + + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + // The second kubeconfig declares a new context and activates it. + err = os.WriteFile(configFile2.Name(), []byte(` +kind: Config +apiVersion: v1 +contexts: +- context: + cluster: kubeconfig-cluster + namespace: a-different-namespace + user: kubeconfig-user + name: `+contextNameB+` +current-context: `+contextNameB+` +`), os.FileMode(0755)) + + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + // Set KUBECONFIG to the files, in descending alphabetical order. + // This will be used to check that they don't get sorted. + envVarValue := fmt.Sprintf("%s%c%s", configFile2.Name(), filepath.ListSeparator, configFile1.Name()) + t.Setenv(RecommendedConfigPathEnvVar, envVarValue) + + // Load the kubeconfigs, change the active context, and call ModifyConfig. + loadingRules := NewDefaultClientConfigLoadingRules() + config, err := loadingRules.Load() + + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + newConfig := config.DeepCopy() + newConfig.CurrentContext = newContextName + err = ModifyConfig(loadingRules, *newConfig, false) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + // Load the files again and check that only configFile2 was changed. + config1, err := LoadFromFile(configFile1.Name()) // file sorts first, but was specified last + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if config1.CurrentContext != contextNameA { + t.Errorf("Config should not be modified, but was. Expected %q, got %q", contextNameA, config1.CurrentContext) + } + + config2, err := LoadFromFile(configFile2.Name()) // file sorts last, but was specified first + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if config2.CurrentContext != newContextName { + t.Errorf("Config should be modified, but was not. Expected %q, got %q", newContextName, config2.CurrentContext) + } +} diff --git a/tools/clientcmd/loader.go b/tools/clientcmd/loader.go index c900e5fd..b127e2e0 100644 --- a/tools/clientcmd/loader.go +++ b/tools/clientcmd/loader.go @@ -331,7 +331,10 @@ func (rules *ClientConfigLoadingRules) GetLoadingPrecedence() []string { return []string{rules.ExplicitPath} } - return rules.Precedence + // Create a copy in case something tries to sort the returned slice. + precedence := make([]string, len(rules.Precedence)) + copy(precedence, rules.Precedence) + return precedence } // GetStartingConfig implements ConfigAccess