From b3dca7eb8a4580d9f156316c81ec5f0844df407f Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 17 Aug 2024 16:42:11 -0400 Subject: [PATCH 1/4] 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 a4f748408..a47e701c1 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 17d1b7b20..4872eadf3 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 b75737f1c..c900e5fd1 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 000000000..9fdf6e3a0 --- /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) +} From 56b7eaf344c3799fa0e02f0b70b56c4d837c923c Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 17 Aug 2024 20:43:53 -0400 Subject: [PATCH 2/4] Narrow merge interface to merging the same types Kubernetes-commit: 9fde1c6a85c1390c7be7705385eb99b3ebc8e06a --- tools/clientcmd/client_config.go | 8 ++++---- tools/clientcmd/merge.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/clientcmd/client_config.go b/tools/clientcmd/client_config.go index a47e701c1..cd0a8649b 100644 --- a/tools/clientcmd/client_config.go +++ b/tools/clientcmd/client_config.go @@ -508,7 +508,7 @@ func (config *DirectClientConfig) getContext() (clientcmdapi.Context, error) { return clientcmdapi.Context{}, fmt.Errorf("context %q does not exist", contextName) } if config.overrides != nil { - if err := merge(mergedContext, config.overrides.Context); err != nil { + if err := merge(mergedContext, &config.overrides.Context); err != nil { return clientcmdapi.Context{}, err } } @@ -530,7 +530,7 @@ func (config *DirectClientConfig) getAuthInfo() (clientcmdapi.AuthInfo, error) { return clientcmdapi.AuthInfo{}, fmt.Errorf("auth info %q does not exist", authInfoName) } if config.overrides != nil { - if err := merge(mergedAuthInfo, config.overrides.AuthInfo); err != nil { + if err := merge(mergedAuthInfo, &config.overrides.AuthInfo); err != nil { return clientcmdapi.AuthInfo{}, err } } @@ -545,7 +545,7 @@ func (config *DirectClientConfig) getCluster() (clientcmdapi.Cluster, error) { mergedClusterInfo := clientcmdapi.NewCluster() if config.overrides != nil { - if err := merge(mergedClusterInfo, config.overrides.ClusterDefaults); err != nil { + if err := merge(mergedClusterInfo, &config.overrides.ClusterDefaults); err != nil { return clientcmdapi.Cluster{}, err } } @@ -557,7 +557,7 @@ func (config *DirectClientConfig) getCluster() (clientcmdapi.Cluster, error) { return clientcmdapi.Cluster{}, fmt.Errorf("cluster %q does not exist", clusterInfoName) } if config.overrides != nil { - if err := merge(mergedClusterInfo, config.overrides.ClusterInfo); err != nil { + if err := merge(mergedClusterInfo, &config.overrides.ClusterInfo); err != nil { return clientcmdapi.Cluster{}, err } } diff --git a/tools/clientcmd/merge.go b/tools/clientcmd/merge.go index 9fdf6e3a0..61590faa1 100644 --- a/tools/clientcmd/merge.go +++ b/tools/clientcmd/merge.go @@ -24,6 +24,6 @@ import ( // - 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 { +func merge[T any](dst, src *T) error { return mergo.Merge(dst, src, mergo.WithOverride) } From a398951ea1b4305aacf8505fb6b2678a9358a561 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 17 Aug 2024 21:27:26 -0400 Subject: [PATCH 3/4] Implement limited merge function Kubernetes-commit: 4d8cbad58e21e701af735c5ffa1a9fd3393de096 --- tools/clientcmd/merge.go | 98 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 95 insertions(+), 3 deletions(-) diff --git a/tools/clientcmd/merge.go b/tools/clientcmd/merge.go index 61590faa1..3d74e6029 100644 --- a/tools/clientcmd/merge.go +++ b/tools/clientcmd/merge.go @@ -17,13 +17,105 @@ limitations under the License. package clientcmd import ( - "github.com/imdario/mergo" + "fmt" + "reflect" + "strings" ) // recursively merges src into dst: -// - non-pointer struct fields are recursively merged +// - non-pointer struct fields with any exported fields are recursively merged +// - non-pointer struct fields with only unexported fields prefer src if the field is non-zero // - 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[T any](dst, src *T) error { - return mergo.Merge(dst, src, mergo.WithOverride) + if dst == nil { + return fmt.Errorf("cannot merge into nil pointer") + } + if src == nil { + return nil + } + return mergeValues(nil, reflect.ValueOf(dst).Elem(), reflect.ValueOf(src).Elem()) +} + +func mergeValues(fieldNames []string, dst, src reflect.Value) error { + dstType := dst.Type() + // no-op if we can't read the src + if !src.IsValid() { + return nil + } + // sanity check types match + if srcType := src.Type(); dstType != srcType { + return fmt.Errorf("cannot merge mismatched types (%s, %s) at %s", dstType, srcType, strings.Join(fieldNames, ".")) + } + + switch dstType.Kind() { + case reflect.Struct: + if hasExportedField(dstType) { + // recursively merge + for i, n := 0, dstType.NumField(); i < n; i++ { + if err := mergeValues(append(fieldNames, dstType.Field(i).Name), dst.Field(i), src.Field(i)); err != nil { + return err + } + } + } else if dst.CanSet() { + // If all fields are unexported, overwrite with src. + // Using src.IsZero() would make more sense but that's not what mergo did. + dst.Set(src) + } + + case reflect.Map: + if dst.CanSet() && !src.IsZero() { + // initialize dst if needed + if dst.IsZero() { + dst.Set(reflect.MakeMap(dstType)) + } + // shallow-merge overwriting dst keys with src keys + for _, mapKey := range src.MapKeys() { + dst.SetMapIndex(mapKey, src.MapIndex(mapKey)) + } + } + + case reflect.Slice: + if dst.CanSet() && src.Len() > 0 { + // overwrite dst with non-empty src slice + dst.Set(src) + } + + case reflect.Pointer: + if dst.CanSet() && !src.IsZero() { + // overwrite dst with non-zero values for other types + if dstType.Elem().Kind() == reflect.Struct { + // use struct pointer as-is + dst.Set(src) + } else { + // shallow-copy non-struct pointer (interfaces, primitives, etc) + dst.Set(reflect.New(dstType.Elem())) + dst.Elem().Set(src.Elem()) + } + } + + default: + if dst.CanSet() && !src.IsZero() { + // overwrite dst with non-zero values for other types + dst.Set(src) + } + } + + return nil +} + +// hasExportedField returns true if the given type has any exported fields, +// or if it has any anonymous/embedded struct fields with exported fields +func hasExportedField(dstType reflect.Type) bool { + for i, n := 0, dstType.NumField(); i < n; i++ { + field := dstType.Field(i) + if field.Anonymous && field.Type.Kind() == reflect.Struct { + if hasExportedField(dstType.Field(i).Type) { + return true + } + } else if len(field.PkgPath) == 0 { + return true + } + } + return false } From 1b9b709183cc84902465a81f27fa188f45d5db41 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Sat, 17 Aug 2024 21:42:00 -0400 Subject: [PATCH 4/4] Update vendor Kubernetes-commit: 745ae75a15cad2f1c5da5518c00f2eb366ffb786 --- go.mod | 11 ++++++++--- go.sum | 18 ++++++++++++------ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 2b3298c1f..40819813e 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,6 @@ require ( github.com/google/uuid v1.6.0 github.com/gorilla/websocket v1.5.0 github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7 - github.com/imdario/mergo v0.3.6 github.com/peterbourgon/diskv v2.0.1+incompatible github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.9.0 @@ -27,8 +26,8 @@ require ( golang.org/x/time v0.3.0 google.golang.org/protobuf v1.34.2 gopkg.in/evanphx/json-patch.v4 v4.12.0 - k8s.io/api v0.0.0-20240920202009-71385f038c10 - k8s.io/apimachinery v0.0.0-20240926041705-dc03077c038e + k8s.io/api v0.0.0 + k8s.io/apimachinery v0.0.0 k8s.io/klog/v2 v2.130.1 k8s.io/kube-openapi v0.0.0-20240827152857-f7e401e7b4c2 k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 @@ -67,3 +66,9 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) + +replace ( + k8s.io/api => ../api + k8s.io/apimachinery => ../apimachinery + k8s.io/client-go => ../client-go +) diff --git a/go.sum b/go.sum index 11caa93f4..bbd1021b4 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,9 @@ +cloud.google.com/go/compute/metadata v0.3.0/go.mod h1:zFmK7XCadkQkj6TtorcaGlCW1hT1fIilQDwofLpJ20k= +github.com/NYTimes/gziphandler v1.1.1/go.mod h1:n/CVRwUEOgIxrgPvAQhUUr9oeUtvrhMomdKFjzJNB0c= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs= +github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY= +github.com/chzyer/readline v1.5.1/go.mod h1:Eh+b79XXUwfKfcPLepksvw2tcLE/Ct21YObkaSkeBlk= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -45,8 +49,7 @@ github.com/gorilla/websocket v1.5.0 h1:PPwGk2jz7EePpoHN/+ClbZu8SPxiqlu12wZP/3sWm github.com/gorilla/websocket v1.5.0/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7 h1:pdN6V1QBWetyv/0+wjACpqVH+eVULgEjkurDLq3goeM= github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA= -github.com/imdario/mergo v0.3.6 h1:xTNEAn+kxVO7dTZGu0CegyqKZmoWFI0rF8UxjlB2d28= -github.com/imdario/mergo v0.3.6/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA= +github.com/ianlancetaylor/demangle v0.0.0-20240312041847-bd984b5ce465/go.mod h1:gx7rwoVhcfuVKG5uya9Hs3Sxj7EIvldVofAWIUtGouw= github.com/josharian/intern v1.0.0 h1:vlS4z54oSdjm0bgjRigI+G1HpF+tI+9rE5LLzOg8HmY= github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= @@ -91,6 +94,7 @@ github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= @@ -101,13 +105,16 @@ github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= +github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= +golang.org/x/crypto v0.26.0/go.mod h1:GY7jblb9wI+FOo5y8/S2oY4zWP07AkOJ4+jxCqdqn54= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= +golang.org/x/mod v0.20.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= @@ -119,11 +126,13 @@ golang.org/x/oauth2 v0.21.0/go.mod h1:XYTD2NtWslqkgxebSiOHnXEap4TF09sJSc7H1sXbht golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.8.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.23.0 h1:YfKFowiIMvtgl1UERQoTPPToxltDeZfbj4H7dVUCwmM= golang.org/x/sys v0.23.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/telemetry v0.0.0-20240521205824-bda55230c457/go.mod h1:pRgIJT+bRLFKnoM1ldnzKoxTIn14Yxz928LQRYYgIN0= golang.org/x/term v0.23.0 h1:F6D4vR+EHoL9/sWAWgAR1H2DcHr4PareCbAaCo1RpuU= golang.org/x/term v0.23.0/go.mod h1:DgV24QBUrK6jhZXl+20l6UWznPlwAHm1Q1mGHtydmSk= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -157,10 +166,7 @@ gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -k8s.io/api v0.0.0-20240920202009-71385f038c10 h1:shjQe98Co9zBlDzQkxb5IJEWtReSl7qunr56C4Jgc70= -k8s.io/api v0.0.0-20240920202009-71385f038c10/go.mod h1:KCEt6+W/Yn1Vc48pYXeLf0mGK52kJhvt+rcaUVsIaKQ= -k8s.io/apimachinery v0.0.0-20240926041705-dc03077c038e h1:/N2qUkIcQ8VaZbmy28grdhVBbNXO6KLZRdz6F2siNeY= -k8s.io/apimachinery v0.0.0-20240926041705-dc03077c038e/go.mod h1:5rKPDwwN9qm//xASFCZ83nyYEanHxxhc7pZ8AC4lukY= +k8s.io/gengo/v2 v2.0.0-20240826214909-a7b603a56eb7/go.mod h1:EJykeLsmFC60UQbYJezXkEsG2FLrt0GPNkU5iK5GWxU= k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk= k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE= k8s.io/kube-openapi v0.0.0-20240827152857-f7e401e7b4c2 h1:GKE9U8BH16uynoxQii0auTjmmmuZ3O0LFMN6S0lPPhI=