Repository: rancher/fleet
Pull request #1955 (2023-11-22 15:50:14)

 [v0.9] Backport: Replace json-based implementation of DeepCopy on GenericMap
Backporting https://github.com/rancher/fleet/pull/1915
Part of https://github.com/rancher/fleet/issues/1842

weyfonk: More changes may need to be backported for E2E tests to pass, as single-cluster test data has been updated to use more lightweight charts, in order to reduce test flakiness.
See [this PR](https://github.com/rancher/fleet/pull/1957) backporting such changes, and [this PR](https://github.com/rancher/fleet/pull/1876/files#diff-806ba4894fb85441356e46e5ff3f34af0849c0afc7549549962c454c0cbee36f) for more context.
aruiz14: > More changes may need to be backported for E2E tests to pass, as single-cluster test data has been updated to use more lightweight charts, in order to reduce test flakiness. See [this PR](https://github.com/rancher/fleet/pull/1957) backporting such changes, and [this PR](https://github.com/rancher/fleet/pull/1876/files#diff-806ba4894fb85441356e46e5ff3f34af0849c0afc7549549962c454c0cbee36f) for more context.

Thanks @weyfonk, I've added #1959 (and later #1961) to address that

Patch:
diff --git a/internal/cmd/controller/target/target.go b/internal/cmd/controller/target/target.go
index 0e3e9dfc..d362588c 100644
--- a/internal/cmd/controller/target/target.go
+++ b/internal/cmd/controller/target/target.go
@@ -8,6 +8,7 @@ package target
 import (
 	"bytes"
 	"fmt"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -45,6 +46,7 @@ var (
 const (
 	maxTemplateRecursionDepth = 50
 	clusterLabelPrefix        = "global.fleet.clusterLabels."
+	byBundleIndexerName       = "fleet.byBundle"
 )
 
 type Manager struct {
@@ -65,6 +67,16 @@ func New(
 	namespaceCache corecontrollers.NamespaceCache,
 	contentStore manifest.Store,
 	bundleDeployments fleetcontrollers.BundleDeploymentCache) *Manager {
+	bundleDeployments.AddIndexer(byBundleIndexerName, func(bd *fleet.BundleDeployment) ([]string, error) {
+		if bdLabels := bd.GetLabels(); bdLabels != nil {
+			bundleNamespace := bdLabels[fleet.BundleNamespaceLabel]
+			bundleName := bdLabels[fleet.BundleLabel]
+			if bundleNamespace != "" && bundleName != "" {
+				return []string{bundleNamespace + "/" + bundleName}, nil
+			}
+		}
+		return nil, nil
+	})
 
 	return &Manager{
 		clusterGroups:               clusterGroups,
@@ -193,15 +205,13 @@ func (m *Manager) BundlesForCluster(cluster *fleet.Cluster) (bundlesToRefresh, b
 }
 
 func (m *Manager) GetBundleDeploymentsForBundleInCluster(bundle *fleet.Bundle, cluster *fleet.Cluster) (result []*fleet.BundleDeployment, err error) {
-	bundleDeployments, err := m.bundleDeploymentCache.List("", labels.SelectorFromSet(labels.Set{
-		fleet.BundleLabel:          bundle.Name,
-		fleet.BundleNamespaceLabel: bundle.Namespace,
-		fleet.ClusterLabel:         cluster.Name,
-	}))
-
+	bundleDeployments, err := m.bundleDeploymentCache.GetByIndex(byBundleIndexerName, bundleIndexKey(bundle))
 	if err != nil {
 		return nil, err
 	}
+	bundleDeployments = slices.DeleteFunc(bundleDeployments, func(bd *fleet.BundleDeployment) bool {
+		return bd.Labels[fleet.ClusterLabel] != cluster.Name
+	})
 
 	return bundleDeployments, nil
 }
@@ -378,10 +388,7 @@ func toDict(values map[string]string) map[string]interface{} {
 
 // foldInDeployments adds the existing bundledeployments to the targets.
 func (m *Manager) foldInDeployments(bundle *fleet.Bundle, targets []*Target) error {
-	bundleDeployments, err := m.bundleDeploymentCache.List("", labels.SelectorFromSet(labels.Set{
-		fleet.BundleLabel:          bundle.Name,
-		fleet.BundleNamespaceLabel: bundle.Namespace,
-	}))
+	bundleDeployments, err := m.bundleDeploymentCache.GetByIndex(byBundleIndexerName, bundleIndexKey(bundle))
 	if err != nil {
 		return err
 	}
@@ -398,6 +405,10 @@ func (m *Manager) foldInDeployments(bundle *fleet.Bundle, targets []*Target) err
 	return nil
 }
 
+func bundleIndexKey(bundle *fleet.Bundle) string {
+	return bundle.Namespace + "/" + bundle.Name
+}
+
 type Target struct {
 	Deployment    *fleet.BundleDeployment
 	ClusterGroups []*fleet.ClusterGroup
diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/generic_map.go b/pkg/apis/fleet.cattle.io/v1alpha1/generic_map.go
new file mode 100644
index 00000000..b6676c8a
--- /dev/null
+++ b/pkg/apis/fleet.cattle.io/v1alpha1/generic_map.go
@@ -0,0 +1,63 @@
+package v1alpha1
+
+import (
+	"encoding/json"
+)
+
+type GenericMap struct {
+	Data map[string]interface{} `json:"-"`
+}
+
+func (in GenericMap) MarshalJSON() ([]byte, error) {
+	return json.Marshal(in.Data)
+}
+
+func (in *GenericMap) UnmarshalJSON(data []byte) error {
+	in.Data = map[string]interface{}{}
+	return json.Unmarshal(data, &in.Data)
+}
+
+func (in *GenericMap) DeepCopyInto(out *GenericMap) {
+	out.Data = make(map[string]interface{}, len(in.Data))
+	deepCopyMap(in.Data, out.Data)
+}
+
+// deepCopyMap will recursively copy each entry from src into dest, ensuring that nested maps and slices are copies as well
+//
+// NOTE: The implementation relies on GenericMap's data being loaded from a JSON or YAML via Unmarshal, which produces map[string]interface{} and []interface{} types.
+// The recursive copy won't work with other variants as pointers or specific value types.
+func deepCopyMap(src, dest map[string]interface{}) {
+	for key := range src {
+		switch value := src[key].(type) {
+		case map[string]interface{}:
+			destValue := make(map[string]interface{}, len(value))
+			deepCopyMap(value, destValue)
+			dest[key] = destValue
+		case []interface{}:
+			destValue := make([]interface{}, len(value))
+			deepCopySlice(value, destValue)
+			dest[key] = destValue
+		default:
+			dest[key] = value
+		}
+	}
+}
+
+// deepCopyMap will recursively copy each entry from src into dest, ensuring that nested maps and slices are copies as well
+// It assumes that both slices have the same capacity.
+func deepCopySlice(src, dest []interface{}) {
+	for i := range src {
+		switch value := src[i].(type) {
+		case map[string]interface{}:
+			destValue := make(map[string]interface{}, len(value))
+			deepCopyMap(value, destValue)
+			dest[i] = destValue
+		case []interface{}:
+			destValue := make([]interface{}, len(value))
+			deepCopySlice(value, destValue)
+			dest[i] = destValue
+		default:
+			dest[i] = value
+		}
+	}
+}
diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/values.go b/pkg/apis/fleet.cattle.io/v1alpha1/values.go
deleted file mode 100644
index 9917948b..00000000
--- a/pkg/apis/fleet.cattle.io/v1alpha1/values.go
+++ /dev/null
@@ -1,27 +0,0 @@
-package v1alpha1
-
-import (
-	"encoding/json"
-
-	"github.com/rancher/wrangler/pkg/data/convert"
-)
-
-type GenericMap struct {
-	Data map[string]interface{} `json:"-"`
-}
-
-func (in GenericMap) MarshalJSON() ([]byte, error) {
-	return json.Marshal(in.Data)
-}
-
-func (in *GenericMap) UnmarshalJSON(data []byte) error {
-	in.Data = map[string]interface{}{}
-	return json.Unmarshal(data, &in.Data)
-}
-
-func (in *GenericMap) DeepCopyInto(out *GenericMap) {
-	out.Data = map[string]interface{}{}
-	if err := convert.ToObj(in.Data, &out.Data); err != nil {
-		panic(err)
-	}
-}


Test patch:
diff --git a/internal/cmd/controller/target/target_test.go b/internal/cmd/controller/target/target_test.go
index d0779f70..8620a161 100644
--- a/internal/cmd/controller/target/target_test.go
+++ b/internal/cmd/controller/target/target_test.go
@@ -3,6 +3,7 @@ package target
 //go:generate mockgen --build_flags=--mod=mod -destination=../mocks/bundle_deployment_cache_mock.go -package=mocks github.com/rancher/fleet/pkg/generated/controllers/fleet.cattle.io/v1alpha1 BundleDeploymentCache
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/golang/mock/gomock"
@@ -10,7 +11,6 @@ import (
 	"github.com/rancher/wrangler/pkg/yaml"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 
 	"github.com/rancher/fleet/internal/cmd/controller/mocks"
 	"github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
@@ -473,6 +473,39 @@ func TestDisablePreProcessFlagMissing(t *testing.T) {
 }
 
 func TestGetBundleDeploymentForBundleInCluster(t *testing.T) {
+	bundleDeployment := func(namespace, clusterName string) *v1alpha1.BundleDeployment {
+		return &v1alpha1.BundleDeployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: fmt.Sprintf("cluster-%s-%s-1df72965a9b5", namespace, clusterName),
+				Labels: map[string]string{
+					"fleet.cattle.io/cluster": clusterName,
+				},
+			},
+		}
+	}
+	bundle := func(name, namespace string) *v1alpha1.Bundle {
+		return &v1alpha1.Bundle{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      name,
+				Namespace: namespace,
+			},
+		}
+	}
+	cluster := func(name string) *v1alpha1.Cluster {
+		return &v1alpha1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+		}
+	}
+	bundleName := "my-bundle"
+	bundleNamespace := "fleet-default"
+	clusterName := "my-cluster"
+	bundleDeployments := []*v1alpha1.BundleDeployment{
+		bundleDeployment(bundleNamespace, clusterName),
+		bundleDeployment(bundleNamespace, "another-cluster"),
+	}
+
 	testCases := []struct {
 		name                       string
 		bundleName                 string
@@ -484,34 +517,27 @@ func TestGetBundleDeploymentForBundleInCluster(t *testing.T) {
 	}{
 		{
 			name:            "returns listed bundle deployments",
-			bundleName:      "my-bundle-my-cluster",
-			bundleNamespace: "fleet-default",
-			clusterName:     "my-cluster",
+			bundleName:      bundleName,
+			bundleNamespace: bundleNamespace,
+			clusterName:     clusterName,
 			expectedBundleDeployments: []*v1alpha1.BundleDeployment{
-				{
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: "cluster-fleet-default-my-cluster-1df72965a9b5",
-						Labels: map[string]string{
-							"fleet.cattle.io/cluster": "my-cluster",
-						},
-					},
-				},
+				bundleDeployment(bundleNamespace, clusterName),
 			},
 		},
 		{
 			name:                       "returns error from bundle deployment cache listing",
-			bundleName:                 "my-bundle-my-cluster",
-			bundleNamespace:            "fleet-default",
+			bundleName:                 bundleName,
+			bundleNamespace:            bundleNamespace,
 			listBundleDeploymentsError: errors.New("something happened"),
 			expectedBundleDeployments:  nil,
 			wantError:                  true,
 		},
 		{
 			name:                      "returns no bundle deployments when none are listed",
-			bundleName:                "my-bundle-my-cluster",
-			bundleNamespace:           "fleet-default",
-			clusterName:               "my-cluster",
-			expectedBundleDeployments: nil,
+			bundleName:                bundleName,
+			bundleNamespace:           bundleNamespace,
+			clusterName:               "yet-another-cluster",
+			expectedBundleDeployments: []*v1alpha1.BundleDeployment{},
 		},
 	}
 
@@ -521,27 +547,12 @@ func TestGetBundleDeploymentForBundleInCluster(t *testing.T) {
 
 			mockBundleDeploymentCache := mocks.NewMockBundleDeploymentCache(ctrl)
 
-			bundle := v1alpha1.Bundle{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      tc.bundleName,
-					Namespace: tc.bundleNamespace,
-				},
-			}
-
-			cluster := v1alpha1.Cluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: tc.clusterName,
-				},
-			}
-
-			mockBundleDeploymentCache.EXPECT().List("", labels.SelectorFromSet(labels.Set{
-				v1alpha1.BundleLabel:          bundle.Name,
-				v1alpha1.BundleNamespaceLabel: bundle.Namespace,
-				v1alpha1.ClusterLabel:         cluster.Name,
-			})).Return(tc.expectedBundleDeployments, tc.listBundleDeploymentsError)
+			mockBundleDeploymentCache.EXPECT().AddIndexer(byBundleIndexerName, gomock.Any())
+			mockBundleDeploymentCache.EXPECT().GetByIndex(byBundleIndexerName, fmt.Sprintf("%s/%s", tc.bundleNamespace, tc.bundleName)).
+				Return(bundleDeployments, tc.listBundleDeploymentsError)
 
 			manager := New(nil, nil, nil, nil, nil, nil, mockBundleDeploymentCache)
-			result, err := manager.GetBundleDeploymentsForBundleInCluster(&bundle, &cluster)
+			result, err := manager.GetBundleDeploymentsForBundleInCluster(bundle(tc.bundleName, tc.bundleNamespace), cluster(tc.clusterName))
 
 			if tc.wantError {
 				assert.Error(t, err)
diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/generic_map_test.go b/pkg/apis/fleet.cattle.io/v1alpha1/generic_map_test.go
new file mode 100644
index 00000000..a8d56ff1
--- /dev/null
+++ b/pkg/apis/fleet.cattle.io/v1alpha1/generic_map_test.go
@@ -0,0 +1,92 @@
+package v1alpha1
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/api/equality"
+)
+
+func Test_deepCopyMap(t *testing.T) {
+	type fixture func(map[string]any)
+	tests := []struct {
+		name string
+		src  map[string]interface{}
+		// perform changes after creation and ensure the original does not change
+		fixtures []fixture
+	}{
+		{name: "copies top-level keys", src: map[string]interface{}{
+			"str":     "value",
+			"list":    []any{1, "dos", true},
+			"boolean": false,
+		}},
+		{name: "copies nested values", src: map[string]interface{}{
+			"first": map[string]any{
+				"second": map[string]any{
+					"third": map[string]any{
+						"str":     "value",
+						"boolean": false,
+					},
+					"list":    []any{1, "dos", true},
+					"boolean": false,
+				},
+			},
+		}},
+		{name: "changing top-level values preserves the original map", src: map[string]interface{}{
+			"str":      "value",
+			"number":   3,
+			"boolean":  false,
+			"todelete": "foo",
+		}, fixtures: []fixture{
+			func(m map[string]interface{}) {
+				m["str"] = "anotherValue"
+			},
+			func(m map[string]interface{}) {
+				m["number"] = "anotherValue"
+			},
+			func(m map[string]interface{}) {
+				m["boolean"] = true
+			},
+			func(m map[string]interface{}) {
+				m["boolean"] = "notanymore"
+			},
+			func(m map[string]interface{}) {
+				delete(m, "todelete")
+			},
+		}},
+		{name: "modifying maps inside a slice preserves the original map", src: map[string]interface{}{
+			"list": []any{map[string]any{
+				"original": "value",
+			}},
+		}, fixtures: []fixture{
+			func(m map[string]interface{}) {
+				list := m["list"].([]any)
+				mapInList := list[0].(map[string]any)
+				mapInList["original"] = "newvalue"
+			},
+			func(m map[string]interface{}) {
+				list := m["list"].([]any)
+				mapInList := list[0].(map[string]any)
+				mapInList["modified"] = "another"
+			},
+		}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			res := map[string]interface{}{}
+			deepCopyMap(tt.src, res)
+
+			if got, want := res, tt.src; !equality.Semantic.DeepEqual(got, want) {
+				t.Errorf("the produced copy is not identical, got: %s, want: %s", got, want)
+			}
+
+			for _, modify := range tt.fixtures {
+				modify(res)
+				if equality.Semantic.DeepEqual(tt.src, res) {
+					t.Errorf("original was modified after modifying the copy")
+				}
+				// Apply same modifications to the original after comparing, to only test the delta
+				modify(tt.src)
+			}
+		})
+	}
+}