From b3dca7eb8a4580d9f156316c81ec5f0844df407f Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 17 Aug 2024 16:42:11 -0400 Subject: [PATCH] Isolate mergo use, add test coverage and error checking Kubernetes-commit: 3b3886a4509a83aff0afe7ad42da9b7819a17aed --- tools/clientcmd/client_config.go | 48 +++++-- tools/clientcmd/client_config_test.go | 190 ++++++++++++++++++++++---- tools/clientcmd/loader.go | 17 ++- tools/clientcmd/merge.go | 29 ++++ 4 files changed, 242 insertions(+), 42 deletions(-) create mode 100644 tools/clientcmd/merge.go diff --git a/tools/clientcmd/client_config.go b/tools/clientcmd/client_config.go index a4f74840..a47e701c 100644 --- a/tools/clientcmd/client_config.go +++ b/tools/clientcmd/client_config.go @@ -29,8 +29,6 @@ import ( clientauth "k8s.io/client-go/tools/auth" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "k8s.io/klog/v2" - - "github.com/imdario/mergo" ) const ( @@ -241,10 +239,14 @@ func (config *DirectClientConfig) ClientConfig() (*restclient.Config, error) { if err != nil { return nil, err } - mergo.Merge(clientConfig, userAuthPartialConfig, mergo.WithOverride) + if err := merge(clientConfig, userAuthPartialConfig); err != nil { + return nil, err + } serverAuthPartialConfig := getServerIdentificationPartialConfig(configClusterInfo) - mergo.Merge(clientConfig, serverAuthPartialConfig, mergo.WithOverride) + if err := merge(clientConfig, serverAuthPartialConfig); err != nil { + return nil, err + } } return clientConfig, nil @@ -326,8 +328,12 @@ func (config *DirectClientConfig) getUserIdentificationPartialConfig(configAuthI promptedConfig := makeUserIdentificationConfig(*promptedAuthInfo) previouslyMergedConfig := mergedConfig mergedConfig = &restclient.Config{} - mergo.Merge(mergedConfig, promptedConfig, mergo.WithOverride) - mergo.Merge(mergedConfig, previouslyMergedConfig, mergo.WithOverride) + if err := merge(mergedConfig, promptedConfig); err != nil { + return nil, err + } + if err := merge(mergedConfig, previouslyMergedConfig); err != nil { + return nil, err + } config.promptedCredentials.username = mergedConfig.Username config.promptedCredentials.password = mergedConfig.Password } @@ -335,7 +341,7 @@ func (config *DirectClientConfig) getUserIdentificationPartialConfig(configAuthI return mergedConfig, nil } -// makeUserIdentificationFieldsConfig returns a client.Config capable of being merged using mergo for only user identification information +// makeUserIdentificationFieldsConfig returns a client.Config capable of being merged for only user identification information func makeUserIdentificationConfig(info clientauth.Info) *restclient.Config { config := &restclient.Config{} config.Username = info.User @@ -495,12 +501,16 @@ func (config *DirectClientConfig) getContext() (clientcmdapi.Context, error) { mergedContext := clientcmdapi.NewContext() if configContext, exists := contexts[contextName]; exists { - mergo.Merge(mergedContext, configContext, mergo.WithOverride) + if err := merge(mergedContext, configContext); err != nil { + return clientcmdapi.Context{}, err + } } else if required { return clientcmdapi.Context{}, fmt.Errorf("context %q does not exist", contextName) } if config.overrides != nil { - mergo.Merge(mergedContext, config.overrides.Context, mergo.WithOverride) + if err := merge(mergedContext, config.overrides.Context); err != nil { + return clientcmdapi.Context{}, err + } } return *mergedContext, nil @@ -513,12 +523,16 @@ func (config *DirectClientConfig) getAuthInfo() (clientcmdapi.AuthInfo, error) { mergedAuthInfo := clientcmdapi.NewAuthInfo() if configAuthInfo, exists := authInfos[authInfoName]; exists { - mergo.Merge(mergedAuthInfo, configAuthInfo, mergo.WithOverride) + if err := merge(mergedAuthInfo, configAuthInfo); err != nil { + return clientcmdapi.AuthInfo{}, err + } } else if required { return clientcmdapi.AuthInfo{}, fmt.Errorf("auth info %q does not exist", authInfoName) } if config.overrides != nil { - mergo.Merge(mergedAuthInfo, config.overrides.AuthInfo, mergo.WithOverride) + if err := merge(mergedAuthInfo, config.overrides.AuthInfo); err != nil { + return clientcmdapi.AuthInfo{}, err + } } return *mergedAuthInfo, nil @@ -531,15 +545,21 @@ func (config *DirectClientConfig) getCluster() (clientcmdapi.Cluster, error) { mergedClusterInfo := clientcmdapi.NewCluster() if config.overrides != nil { - mergo.Merge(mergedClusterInfo, config.overrides.ClusterDefaults, mergo.WithOverride) + if err := merge(mergedClusterInfo, config.overrides.ClusterDefaults); err != nil { + return clientcmdapi.Cluster{}, err + } } if configClusterInfo, exists := clusterInfos[clusterInfoName]; exists { - mergo.Merge(mergedClusterInfo, configClusterInfo, mergo.WithOverride) + if err := merge(mergedClusterInfo, configClusterInfo); err != nil { + return clientcmdapi.Cluster{}, err + } } else if required { return clientcmdapi.Cluster{}, fmt.Errorf("cluster %q does not exist", clusterInfoName) } if config.overrides != nil { - mergo.Merge(mergedClusterInfo, config.overrides.ClusterInfo, mergo.WithOverride) + if err := merge(mergedClusterInfo, config.overrides.ClusterInfo); err != nil { + return clientcmdapi.Cluster{}, err + } } // * 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 17d1b7b2..4872eadf 100644 --- a/tools/clientcmd/client_config_test.go +++ b/tools/clientcmd/client_config_test.go @@ -22,16 +22,17 @@ import ( "strings" "testing" - utiltesting "k8s.io/client-go/util/testing" - - "github.com/imdario/mergo" + "github.com/google/go-cmp/cmp" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/dump" restclient "k8s.io/client-go/rest" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + utiltesting "k8s.io/client-go/util/testing" + "k8s.io/utils/ptr" ) -func TestMergoSemantics(t *testing.T) { +func TestMergeSemantics(t *testing.T) { type U struct { A string B int64 @@ -41,7 +42,11 @@ func TestMergoSemantics(t *testing.T) { X string Y int64 U U + + StringPtr *string + StructPtr *U } + var testDataStruct = []struct { dst T src T @@ -62,22 +67,31 @@ func TestMergoSemantics(t *testing.T) { src: T{S: []string{"test1", "test2", "test3"}}, expected: T{S: []string{"test1", "test2", "test3"}}, }, + { + dst: T{}, + src: T{StringPtr: ptr.To("a"), StructPtr: ptr.To(U{A: "a", B: 1})}, + expected: T{StringPtr: ptr.To("a"), StructPtr: ptr.To(U{A: "a", B: 1})}, + }, + { + dst: T{StringPtr: ptr.To("a"), StructPtr: ptr.To(U{A: "a", B: 1})}, + src: T{}, + expected: T{StringPtr: ptr.To("a"), StructPtr: ptr.To(U{A: "a", B: 1})}, + }, + { + dst: T{StringPtr: ptr.To("a"), StructPtr: ptr.To(U{A: "a", B: 1})}, + src: T{StringPtr: ptr.To("b"), StructPtr: ptr.To(U{A: "b", B: 0})}, // zero value B overrides non-zero value B in pointer member + expected: T{StringPtr: ptr.To("b"), StructPtr: ptr.To(U{A: "b", B: 0})}, + }, } - for _, data := range testDataStruct { - err := mergo.Merge(&data.dst, &data.src, mergo.WithOverride) + for i, data := range testDataStruct { + t.Logf("case %d: %+v, %+v", i, data.dst, data.src) + err := merge(&data.dst, &data.src) if err != nil { t.Errorf("error while merging: %s", err) } if !reflect.DeepEqual(data.dst, data.expected) { - // The mergo library has previously changed in a an incompatible way. - // example: - // - // https://github.com/imdario/mergo/commit/d304790b2ed594794496464fadd89d2bb266600a - // // This test verifies that the semantics of the merge are what we expect. - // If they are not, the mergo library may have been updated and broken - // unexpectedly. - t.Errorf("mergo.MergeWithOverwrite did not provide expected output: %+v doesn't match %+v", data.dst, data.expected) + t.Errorf("merge did not provide expected output:\ngot: %s\nwant: %s\ndiff: %s", dump.OneLine(data.dst), dump.OneLine(data.expected), cmp.Diff(data.dst, data.expected)) } } @@ -93,22 +107,152 @@ func TestMergoSemantics(t *testing.T) { }, } for _, data := range testDataMap { - err := mergo.Merge(&data.dst, &data.src, mergo.WithOverride) + err := merge(&data.dst, &data.src) if err != nil { t.Errorf("error while merging: %s", err) } if !reflect.DeepEqual(data.dst, data.expected) { - // The mergo library has previously changed in a an incompatible way. - // example: - // - // https://github.com/imdario/mergo/commit/d304790b2ed594794496464fadd89d2bb266600a - // // This test verifies that the semantics of the merge are what we expect. - // If they are not, the mergo library may have been updated and broken - // unexpectedly. - t.Errorf("mergo.MergeWithOverwrite did not provide expected output: %+v doesn't match %+v", data.dst, data.expected) + t.Errorf("merge did not provide expected output: %+v doesn't match %+v", data.dst, data.expected) } } + + type Struct struct { + String string + StringP *string + Int int + IntP *int + Bool bool + BoolP *bool + Slice []string + SliceP *[]string + Map map[string]string + MapP *map[string]string + } + type T2 struct { + String string + StringP *string + Int int + IntP *int + Bool bool + BoolP *bool + Slice []string + SliceP *[]string + Map map[string]string + MapP *map[string]string + Struct Struct + StructP *Struct + } + + var testcases = []struct { + name string + dst T2 + src T2 + expected T2 + }{ + { + name: "zero noop", + dst: T2{}, + src: T2{}, + expected: T2{}, + }, + { + name: "override zero dst", + dst: T2{}, + src: T2{ + String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"}), + Struct: Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}, + StructP: ptr.To(Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}), + }, + expected: T2{ + String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"}), + Struct: Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}, + StructP: ptr.To(Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}), + }, + }, + { + name: "noop zero src", + dst: T2{ + String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"}), + Struct: Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}, + StructP: ptr.To(Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}), + }, + src: T2{}, + expected: T2{ + String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"}), + Struct: Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}, + StructP: ptr.To(Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}), + }, + }, + { + name: "overwrite non-zero values, merge maps", + dst: T2{ + String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), + Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"}), + Struct: Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}, + StructP: ptr.To(Struct{String: "a", StringP: ptr.To("a"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"a"}, SliceP: ptr.To([]string{"a"}), Map: map[string]string{"a": "1"}, MapP: ptr.To(map[string]string{"a": "1"})}), + }, + src: T2{ + String: "b", StringP: ptr.To("b"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"b"}, SliceP: ptr.To([]string{"b"}), + Map: map[string]string{"b": "1"}, MapP: ptr.To(map[string]string{"b": "1"}), + Struct: Struct{String: "b", StringP: ptr.To("b"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"b"}, SliceP: ptr.To([]string{"b"}), Map: map[string]string{"b": "1"}, MapP: ptr.To(map[string]string{"b": "1"})}, + StructP: ptr.To(Struct{String: "b", StringP: ptr.To("b"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"b"}, SliceP: ptr.To([]string{"b"}), Map: map[string]string{"b": "1"}, MapP: ptr.To(map[string]string{"b": "1"})}), + }, + expected: T2{ + // overwritten + String: "b", StringP: ptr.To("b"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"b"}, SliceP: ptr.To([]string{"b"}), MapP: ptr.To(map[string]string{"b": "1"}), + // merged + Map: map[string]string{"a": "1", "b": "1"}, + Struct: Struct{ + // overwritten + String: "b", StringP: ptr.To("b"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"b"}, SliceP: ptr.To([]string{"b"}), MapP: ptr.To(map[string]string{"b": "1"}), + // merged + Map: map[string]string{"a": "1", "b": "1"}, + }, + // overwritten + StructP: ptr.To(Struct{String: "b", StringP: ptr.To("b"), Int: 1, IntP: ptr.To(1), Bool: true, BoolP: ptr.To(true), Slice: []string{"b"}, SliceP: ptr.To([]string{"b"}), Map: map[string]string{"b": "1"}, MapP: ptr.To(map[string]string{"b": "1"})}), + }, + }, + { + name: "overwrite non-zero values, merge maps", + dst: T2{ + Struct: Struct{ + String: "a", Int: 1, Map: map[string]string{"a": "1", "shared": "1"}, MapP: ptr.To(map[string]string{"a": "1", "shared": "1"}), + }, + StructP: ptr.To(Struct{ + String: "a", Int: 1, Map: map[string]string{"a": "1", "shared": "1"}, MapP: ptr.To(map[string]string{"a": "1", "shared": "1"}), + }), + }, + src: T2{ + Struct: Struct{ + String: "b", Int: 0, Map: map[string]string{"b": "1"}, + }, + StructP: ptr.To(Struct{ + String: "b", Int: 0, Map: map[string]string{"b": "1"}, + }), + }, + expected: T2{ + Struct: Struct{ + String: "b", Int: 1 /* preserved */, Map: map[string]string{"a": "1", "b": "1", "shared": "1"} /* merged */, MapP: ptr.To(map[string]string{"a": "1", "shared": "1"}), /* preserved */ + }, + StructP: ptr.To(Struct{ + String: "b", Int: 0 /* overwritten */, Map: map[string]string{"b": "1"} /* unmerged */, MapP: nil, /* overwritten*/ + }), + }, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + err := merge(&tc.dst, &tc.src) + if err != nil { + t.Errorf("error while merging: %s", err) + } + if !reflect.DeepEqual(tc.dst, tc.expected) { + // This test verifies that the semantics of the merge are what we expect. + t.Errorf("merge did not provide expected output:\ngot: %s\nwant: %s\ndiff: %s", dump.OneLine(tc.dst), dump.OneLine(tc.expected), cmp.Diff(tc.dst, tc.expected)) + } + }) + } } func createValidTestConfig() *clientcmdapi.Config { diff --git a/tools/clientcmd/loader.go b/tools/clientcmd/loader.go index b75737f1..c900e5fd 100644 --- a/tools/clientcmd/loader.go +++ b/tools/clientcmd/loader.go @@ -24,7 +24,6 @@ import ( goruntime "runtime" "strings" - "github.com/imdario/mergo" "k8s.io/klog/v2" "k8s.io/apimachinery/pkg/runtime" @@ -248,7 +247,9 @@ func (rules *ClientConfigLoadingRules) Load() (*clientcmdapi.Config, error) { mapConfig := clientcmdapi.NewConfig() for _, kubeconfig := range kubeconfigs { - mergo.Merge(mapConfig, kubeconfig, mergo.WithOverride) + if err := merge(mapConfig, kubeconfig); err != nil { + return nil, err + } } // merge all of the struct values in the reverse order so that priority is given correctly @@ -256,14 +257,20 @@ func (rules *ClientConfigLoadingRules) Load() (*clientcmdapi.Config, error) { nonMapConfig := clientcmdapi.NewConfig() for i := len(kubeconfigs) - 1; i >= 0; i-- { kubeconfig := kubeconfigs[i] - mergo.Merge(nonMapConfig, kubeconfig, mergo.WithOverride) + if err := merge(nonMapConfig, kubeconfig); err != nil { + return nil, err + } } // 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.Merge(config, mapConfig, mergo.WithOverride) - mergo.Merge(config, nonMapConfig, mergo.WithOverride) + if err := merge(config, mapConfig); err != nil { + return nil, err + } + if err := merge(config, nonMapConfig); err != nil { + return nil, err + } if rules.ResolvePaths() { if err := ResolveLocalPaths(config); err != nil { diff --git a/tools/clientcmd/merge.go b/tools/clientcmd/merge.go new file mode 100644 index 00000000..9fdf6e3a --- /dev/null +++ b/tools/clientcmd/merge.go @@ -0,0 +1,29 @@ +/* +Copyright 2024 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 ( + "github.com/imdario/mergo" +) + +// recursively merges src into dst: +// - non-pointer struct fields are recursively merged +// - maps are shallow merged with src keys taking priority over dst +// - non-zero src fields encountered during recursion that are not maps or structs overwrite and recursion stops +func merge(dst, src any) error { + return mergo.Merge(dst, src, mergo.WithOverride) +}