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
This commit is contained in:
Ethan Pini 2025-04-14 15:19:04 -07:00 committed by Kubernetes Publisher
parent 0341f077c9
commit af0e2a11af
2 changed files with 123 additions and 1 deletions

View File

@ -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)
}
}

View File

@ -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