Isolate mergo use, add test coverage and error checking

Kubernetes-commit: 3b3886a4509a83aff0afe7ad42da9b7819a17aed

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)
+		if err := merge(mergedConfig, promptedConfig); err != nil {
-		mergo.Merge(mergedConfig, previouslyMergedConfig, mergo.WithOverride)
+			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

tools/clientcmd/client_config_test.go

@@ -22,16 +22,17 @@ import (
 	"strings"
 	"testing"
 
-	utiltesting "k8s.io/client-go/util/testing"
+	"github.com/google/go-cmp/cmp"
-
-	"github.com/imdario/mergo"
 
 	"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 {
+	for i, data := range testDataStruct {
-		err := mergo.Merge(&data.dst, &data.src, mergo.WithOverride)
+		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
+			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))
-			// unexpectedly.
-			t.Errorf("mergo.MergeWithOverwrite did not provide expected output: %+v doesn't match %+v", 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
+			t.Errorf("merge did not provide expected output: %+v doesn't match %+v", data.dst, data.expected)
-			// unexpectedly.
-			t.Errorf("mergo.MergeWithOverwrite 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 {

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)
+	if err := merge(config, mapConfig); err != nil {
-	mergo.Merge(config, nonMapConfig, mergo.WithOverride)
+		return nil, err
+	}
+	if err := merge(config, nonMapConfig); err != nil {
+		return nil, err
+	}
 
 	if rules.ResolvePaths() {
 		if err := ResolveLocalPaths(config); err != nil {

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