From 277eea62aa528da77c44b2d249085fc7941d062b Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Mon, 15 Jun 2020 20:59:21 +1000 Subject: [PATCH 1/4] Cleanup currentMigrationRules 1. Use filepath for filename manipulations 2. Restructure method logic Kubernetes-commit: 11800147f51e85b9a4fb7eb2654cae3ded9d8cf0 --- tools/clientcmd/loader.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/tools/clientcmd/loader.go b/tools/clientcmd/loader.go index 901ed50c..74da8f09 100644 --- a/tools/clientcmd/loader.go +++ b/tools/clientcmd/loader.go @@ -21,7 +21,6 @@ import ( "io" "io/ioutil" "os" - "path" "path/filepath" "reflect" goruntime "runtime" @@ -48,24 +47,24 @@ const ( ) var ( - RecommendedConfigDir = path.Join(homedir.HomeDir(), RecommendedHomeDir) - RecommendedHomeFile = path.Join(RecommendedConfigDir, RecommendedFileName) - RecommendedSchemaFile = path.Join(RecommendedConfigDir, RecommendedSchemaName) + RecommendedConfigDir = filepath.Join(homedir.HomeDir(), RecommendedHomeDir) + RecommendedHomeFile = filepath.Join(RecommendedConfigDir, RecommendedFileName) + RecommendedSchemaFile = filepath.Join(RecommendedConfigDir, RecommendedSchemaName) ) // currentMigrationRules returns a map that holds the history of recommended home directories used in previous versions. // Any future changes to RecommendedHomeFile and related are expected to add a migration rule here, in order to make // sure existing config files are migrated to their new locations properly. func currentMigrationRules() map[string]string { - oldRecommendedHomeFile := path.Join(os.Getenv("HOME"), "/.kube/.kubeconfig") - oldRecommendedWindowsHomeFile := path.Join(os.Getenv("HOME"), RecommendedHomeDir, RecommendedFileName) - - migrationRules := map[string]string{} - migrationRules[RecommendedHomeFile] = oldRecommendedHomeFile + var oldRecommendedHomeFileName string if goruntime.GOOS == "windows" { - migrationRules[RecommendedHomeFile] = oldRecommendedWindowsHomeFile + oldRecommendedHomeFileName = RecommendedFileName + } else { + oldRecommendedHomeFileName = ".kubeconfig" + } + return map[string]string{ + RecommendedHomeFile: filepath.Join(os.Getenv("HOME"), RecommendedHomeDir, oldRecommendedHomeFileName), } - return migrationRules } type ClientConfigLoader interface { From 6d09f8e62e0ac9ca1d1a5cbed05938481bd7d188 Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Mon, 15 Jun 2020 21:11:27 +1000 Subject: [PATCH 2/4] Stop using mergo.MergeWithOverwrite Use the recommended replacement instead. Kubernetes-commit: 243a9b204e14dc9c92f08cd3252c31731b9532fd --- tools/clientcmd/client_config.go | 24 ++++++++++++------------ tools/clientcmd/client_config_test.go | 4 ++-- tools/clientcmd/loader.go | 8 ++++---- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/tools/clientcmd/client_config.go b/tools/clientcmd/client_config.go index 9e1cd64a..fc180676 100644 --- a/tools/clientcmd/client_config.go +++ b/tools/clientcmd/client_config.go @@ -198,13 +198,13 @@ func (config *DirectClientConfig) ClientConfig() (*restclient.Config, error) { if err != nil { return nil, err } - mergo.MergeWithOverwrite(clientConfig, userAuthPartialConfig) + mergo.Merge(clientConfig, userAuthPartialConfig, mergo.WithOverride) serverAuthPartialConfig, err := getServerIdentificationPartialConfig(configAuthInfo, configClusterInfo) if err != nil { return nil, err } - mergo.MergeWithOverwrite(clientConfig, serverAuthPartialConfig) + mergo.Merge(clientConfig, serverAuthPartialConfig, mergo.WithOverride) } return clientConfig, nil @@ -225,7 +225,7 @@ func getServerIdentificationPartialConfig(configAuthInfo clientcmdapi.AuthInfo, configClientConfig.CAData = configClusterInfo.CertificateAuthorityData configClientConfig.Insecure = configClusterInfo.InsecureSkipTLSVerify configClientConfig.ServerName = configClusterInfo.TLSServerName - mergo.MergeWithOverwrite(mergedConfig, configClientConfig) + mergo.Merge(mergedConfig, configClientConfig, mergo.WithOverride) return mergedConfig, nil } @@ -294,8 +294,8 @@ func (config *DirectClientConfig) getUserIdentificationPartialConfig(configAuthI promptedConfig := makeUserIdentificationConfig(*promptedAuthInfo) previouslyMergedConfig := mergedConfig mergedConfig = &restclient.Config{} - mergo.MergeWithOverwrite(mergedConfig, promptedConfig) - mergo.MergeWithOverwrite(mergedConfig, previouslyMergedConfig) + mergo.Merge(mergedConfig, promptedConfig, mergo.WithOverride) + mergo.Merge(mergedConfig, previouslyMergedConfig, mergo.WithOverride) config.promptedCredentials.username = mergedConfig.Username config.promptedCredentials.password = mergedConfig.Password } @@ -463,12 +463,12 @@ func (config *DirectClientConfig) getContext() (clientcmdapi.Context, error) { mergedContext := clientcmdapi.NewContext() if configContext, exists := contexts[contextName]; exists { - mergo.MergeWithOverwrite(mergedContext, configContext) + mergo.Merge(mergedContext, configContext, mergo.WithOverride) } else if required { return clientcmdapi.Context{}, fmt.Errorf("context %q does not exist", contextName) } if config.overrides != nil { - mergo.MergeWithOverwrite(mergedContext, config.overrides.Context) + mergo.Merge(mergedContext, config.overrides.Context, mergo.WithOverride) } return *mergedContext, nil @@ -481,12 +481,12 @@ func (config *DirectClientConfig) getAuthInfo() (clientcmdapi.AuthInfo, error) { mergedAuthInfo := clientcmdapi.NewAuthInfo() if configAuthInfo, exists := authInfos[authInfoName]; exists { - mergo.MergeWithOverwrite(mergedAuthInfo, configAuthInfo) + mergo.Merge(mergedAuthInfo, configAuthInfo, mergo.WithOverride) } else if required { return clientcmdapi.AuthInfo{}, fmt.Errorf("auth info %q does not exist", authInfoName) } if config.overrides != nil { - mergo.MergeWithOverwrite(mergedAuthInfo, config.overrides.AuthInfo) + mergo.Merge(mergedAuthInfo, config.overrides.AuthInfo, mergo.WithOverride) } return *mergedAuthInfo, nil @@ -499,15 +499,15 @@ func (config *DirectClientConfig) getCluster() (clientcmdapi.Cluster, error) { mergedClusterInfo := clientcmdapi.NewCluster() if config.overrides != nil { - mergo.MergeWithOverwrite(mergedClusterInfo, config.overrides.ClusterDefaults) + mergo.Merge(mergedClusterInfo, config.overrides.ClusterDefaults, mergo.WithOverride) } if configClusterInfo, exists := clusterInfos[clusterInfoName]; exists { - mergo.MergeWithOverwrite(mergedClusterInfo, configClusterInfo) + mergo.Merge(mergedClusterInfo, configClusterInfo, mergo.WithOverride) } else if required { return clientcmdapi.Cluster{}, fmt.Errorf("cluster %q does not exist", clusterInfoName) } if config.overrides != nil { - mergo.MergeWithOverwrite(mergedClusterInfo, config.overrides.ClusterInfo) + mergo.Merge(mergedClusterInfo, config.overrides.ClusterInfo, mergo.WithOverride) } // * An override of --insecure-skip-tls-verify=true and no accompanying CA/CA data should clear already-set CA/CA data diff --git a/tools/clientcmd/client_config_test.go b/tools/clientcmd/client_config_test.go index ea3c77b5..9d232f4f 100644 --- a/tools/clientcmd/client_config_test.go +++ b/tools/clientcmd/client_config_test.go @@ -63,7 +63,7 @@ func TestMergoSemantics(t *testing.T) { }, } for _, data := range testDataStruct { - err := mergo.MergeWithOverwrite(&data.dst, &data.src) + err := mergo.Merge(&data.dst, &data.src, mergo.WithOverride) if err != nil { t.Errorf("error while merging: %s", err) } @@ -92,7 +92,7 @@ func TestMergoSemantics(t *testing.T) { }, } for _, data := range testDataMap { - err := mergo.MergeWithOverwrite(&data.dst, &data.src) + err := mergo.Merge(&data.dst, &data.src, mergo.WithOverride) if err != nil { t.Errorf("error while merging: %s", err) } diff --git a/tools/clientcmd/loader.go b/tools/clientcmd/loader.go index 74da8f09..33ecd870 100644 --- a/tools/clientcmd/loader.go +++ b/tools/clientcmd/loader.go @@ -226,7 +226,7 @@ func (rules *ClientConfigLoadingRules) Load() (*clientcmdapi.Config, error) { mapConfig := clientcmdapi.NewConfig() for _, kubeconfig := range kubeconfigs { - mergo.MergeWithOverwrite(mapConfig, kubeconfig) + mergo.Merge(mapConfig, kubeconfig, mergo.WithOverride) } // merge all of the struct values in the reverse order so that priority is given correctly @@ -234,14 +234,14 @@ func (rules *ClientConfigLoadingRules) Load() (*clientcmdapi.Config, error) { nonMapConfig := clientcmdapi.NewConfig() for i := len(kubeconfigs) - 1; i >= 0; i-- { kubeconfig := kubeconfigs[i] - mergo.MergeWithOverwrite(nonMapConfig, kubeconfig) + mergo.Merge(nonMapConfig, kubeconfig, mergo.WithOverride) } // since values are overwritten, but maps values are not, we can merge the non-map config on top of the map config and // get the values we expect. config := clientcmdapi.NewConfig() - mergo.MergeWithOverwrite(config, mapConfig) - mergo.MergeWithOverwrite(config, nonMapConfig) + mergo.Merge(config, mapConfig, mergo.WithOverride) + mergo.Merge(config, nonMapConfig, mergo.WithOverride) if rules.ResolvePaths() { if err := ResolveLocalPaths(config); err != nil { From 6cc39819fdfbc221a43b482747661c25de6373b1 Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Mon, 15 Jun 2020 21:17:45 +1000 Subject: [PATCH 3/4] Make inClusterConfigProvider thread safe If configuration object is used concurrently it is not safe to mutate self. There is no need for mutation so avoid it just in case. Kubernetes-commit: 9e360eb05efafd0fcabd5a065b62cb8226da94c2 --- tools/clientcmd/client_config.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tools/clientcmd/client_config.go b/tools/clientcmd/client_config.go index fc180676..a50ce5e3 100644 --- a/tools/clientcmd/client_config.go +++ b/tools/clientcmd/client_config.go @@ -548,11 +548,12 @@ func (config *inClusterClientConfig) RawConfig() (clientcmdapi.Config, error) { } func (config *inClusterClientConfig) ClientConfig() (*restclient.Config, error) { - if config.inClusterConfigProvider == nil { - config.inClusterConfigProvider = restclient.InClusterConfig + inClusterConfigProvider := config.inClusterConfigProvider + if inClusterConfigProvider == nil { + inClusterConfigProvider = restclient.InClusterConfig } - icc, err := config.inClusterConfigProvider() + icc, err := inClusterConfigProvider() if err != nil { return nil, err } @@ -572,7 +573,7 @@ func (config *inClusterClientConfig) ClientConfig() (*restclient.Config, error) } } - return icc, err + return icc, nil } func (config *inClusterClientConfig) Namespace() (string, bool, error) { From 29b07456f5849e6fe7f602057b7fdfd402050541 Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Mon, 15 Jun 2020 21:47:08 +1000 Subject: [PATCH 4/4] Check errors of the Close call Error from out.Close() was not checked Kubernetes-commit: f9b928f1f13821b65ea4ef783f847993c51fb4dd --- tools/clientcmd/loader.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tools/clientcmd/loader.go b/tools/clientcmd/loader.go index 33ecd870..78bd9ed8 100644 --- a/tools/clientcmd/loader.go +++ b/tools/clientcmd/loader.go @@ -18,7 +18,6 @@ package clientcmd import ( "fmt" - "io" "io/ioutil" "os" "path/filepath" @@ -282,20 +281,15 @@ func (rules *ClientConfigLoadingRules) Migrate() error { return fmt.Errorf("cannot migrate %v to %v because it is a directory", source, destination) } - in, err := os.Open(source) + data, err := ioutil.ReadFile(source) if err != nil { return err } - defer in.Close() - out, err := os.Create(destination) + // destination is created with mode 0666 before umask + err = ioutil.WriteFile(destination, data, 0666) if err != nil { return err } - defer out.Close() - - if _, err = io.Copy(out, in); err != nil { - return err - } } return nil