From 83f2d01ff88fc448d914a5cedffe6c7b1a7fa886 Mon Sep 17 00:00:00 2001 From: Angie Wang Date: Thu, 19 Sep 2024 19:48:12 -0400 Subject: [PATCH] Handle special overrides for extraLabels and extraAnnotations In normal cases, ClusterInstance input data from the ClusterRequest overrides the defaults from the configmap. However, for the fields extraLabels and extraAnnotations, the default values from the configmap should take precedence if the same labels/annotations exist in both. Add a function to handle these special overrides. If extraLabels or extraAnnotations are present in both the ClusterRequest and the configmap, the values from ClusterRequest will be overriden with the defaults. The utility merge function is invoked after to perform the normal merging behavior, where ClusterRequest overrides defaults if both exist. --- .../controllers/clusterrequest_controller.go | 12 +- internal/controllers/utils/utils.go | 64 ++++ internal/controllers/utils/utils_test.go | 291 ++++++++++++++++++ 3 files changed, 366 insertions(+), 1 deletion(-) diff --git a/internal/controllers/clusterrequest_controller.go b/internal/controllers/clusterrequest_controller.go index f383ee12..032e6bef 100644 --- a/internal/controllers/clusterrequest_controller.go +++ b/internal/controllers/clusterrequest_controller.go @@ -426,6 +426,16 @@ func (t *clusterRequestReconcilerTask) getMergedClusterInputData( return nil, fmt.Errorf("failed to get template defaults from ConfigMap %s: %w", templateDefaultsCm, err) } + if dataType == utils.ClusterInstanceDataType { + // Special handling for overrides of ClusterInstance's extraLabels and extraAnnotations. + // The clusterTemplateInputMap will be overridden with the values from defaut configmap + // if same labels/annotations exist in both. + if err := utils.OverrideClusterInstanceLabelsOrAnnotations( + clusterTemplateInputMap, clusterTemplateDefaultsMap); err != nil { + return nil, utils.NewInputError(err.Error()) + } + } + // Get the merged cluster data mergedClusterDataMap, err := mergeClusterTemplateInputWithDefaults(clusterTemplateInputMap, clusterTemplateDefaultsMap) if err != nil { @@ -433,7 +443,7 @@ func (t *clusterRequestReconcilerTask) getMergedClusterInputData( } t.logger.Info( - fmt.Sprintf("Merged the %s default data with the clusterTemplateInput data for ClusterRequest", dataType), + fmt.Sprintf("Merged the %s default data with the clusterTemplateInput data for ClusterRequest: %+v", dataType, mergedClusterDataMap), slog.String("name", t.object.Name), ) return mergedClusterDataMap, nil diff --git a/internal/controllers/utils/utils.go b/internal/controllers/utils/utils.go index b91ab0db..14ed6454 100644 --- a/internal/controllers/utils/utils.go +++ b/internal/controllers/utils/utils.go @@ -721,6 +721,70 @@ func DeepMergeSlices[K comparable, V any](dst, src []V, checkType bool) ([]V, er return result, nil } +// OverrideClusterInstanceLabelsOrAnnotations handles the overrides of ClusterInstance's extraLabels +// or extraAnnotations. It overrides the values in the ClusterRequest with those from the default +// configmap when the same labels/annotations exist in both. Labels/annotations that are not common +// between the default configmap and ClusterRequest are ignored. +func OverrideClusterInstanceLabelsOrAnnotations(dstClusterRequestInput, srcConfigmap map[string]any) error { + fields := []string{"extraLabels", "extraAnnotations"} + + for _, field := range fields { + srcValue, existsSrc := srcConfigmap[field] + dstValue, existsDst := dstClusterRequestInput[field] + // Check only when both configmap and ClusterRequestInput contain the field + if existsSrc && existsDst { + dstMap, okDst := dstValue.(map[string]any) + srcMap, okSrc := srcValue.(map[string]any) + if !okDst || !okSrc { + return fmt.Errorf("type mismatch for field %s: (from ClusterRequest: %T, from default Configmap: %T)", + field, dstValue, srcValue) + } + + // Iterate over the resource types (e.g., ManagedCluster, AgentClusterInstall) + // Check labels/annotations only if the resource exists in both + for resourceType, srcFields := range srcMap { + if dstFields, exists := dstMap[resourceType]; exists { + dstFieldsMap, okDstFields := dstFields.(map[string]any) + srcFieldsMap, okSrcFields := srcFields.(map[string]any) + if !okDstFields || !okSrcFields { + return fmt.Errorf("type mismatch for field %s: (from ClusterRequest: %T, from default Configmap: %T)", + field, dstValue, srcValue) + } + + // Override ClusterRequestInput's values with defaults if the label/annotation key exists in both + for srcFieldKey, srcLabelValue := range srcFieldsMap { + if _, exists := dstFieldsMap[srcFieldKey]; exists { + dstFieldsMap[srcFieldKey] = srcLabelValue + } + } + } + } + } + } + + // Process label/annotation overrides for nodes + dstNodes, dstExists := dstClusterRequestInput["nodes"] + srcNodes, srcExists := srcConfigmap["nodes"] + if dstExists && srcExists { + // Determine the minimum length to merge + minLen := len(dstNodes.([]any)) + if len(srcNodes.([]any)) < minLen { + minLen = len(srcNodes.([]any)) + } + + for i := 0; i < minLen; i++ { + if err := OverrideClusterInstanceLabelsOrAnnotations( + dstNodes.([]any)[i].(map[string]any), + srcNodes.([]any)[i].(map[string]any), + ); err != nil { + return fmt.Errorf("type mismatch for nodes: %w", err) + } + } + } + + return nil +} + func CopyK8sSecret(ctx context.Context, c client.Client, secretName, sourceNamespace, targetNamespace string) error { // Get the secret from the source namespace secret := &corev1.Secret{} diff --git a/internal/controllers/utils/utils_test.go b/internal/controllers/utils/utils_test.go index 80529ec7..5922cfcb 100644 --- a/internal/controllers/utils/utils_test.go +++ b/internal/controllers/utils/utils_test.go @@ -1554,3 +1554,294 @@ var _ = Describe("GetLabelsForPolicies", func() { }) }) + +var _ = Describe("OverrideClusterInstanceLabelsOrAnnotations", func() { + var ( + dstClusterRequestInput map[string]any + srcConfigmap map[string]any + ) + + BeforeEach(func() { + dstClusterRequestInput = make(map[string]any) + srcConfigmap = make(map[string]any) + }) + + It("should override only existing keys", func() { + dstClusterRequestInput = map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "value1", + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation1": "value1", + }, + }, + "clusterName": "cluster-1", + } + + srcConfigmap = map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "new_value1", // Existing key in dst + "label2": "value2", // New key, should be ignored + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation2": "value2", // New key, should be ignored + }, + }, + } + + expected := map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "new_value1", // Overridden + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation1": "value1", + }, + }, + "clusterName": "cluster-1", + } + + err := OverrideClusterInstanceLabelsOrAnnotations(dstClusterRequestInput, srcConfigmap) + Expect(err).ToNot(HaveOccurred()) + Expect(dstClusterRequestInput).To(Equal(expected)) + }) + + It("should not add new resource types to dstClusterRequestInput", func() { + dstClusterRequestInput = map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "value1", + }, + }, + "clusterName": "cluster-1", + } + + srcConfigmap = map[string]any{ + "extraLabels": map[string]any{ + "AgentClusterInstall": map[string]any{ + "label1": "value1", // New resource type, should be ignored + }, + }, + } + + expected := map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "value1", // Should remain unchanged + }, + }, + "clusterName": "cluster-1", + } + + err := OverrideClusterInstanceLabelsOrAnnotations(dstClusterRequestInput, srcConfigmap) + Expect(err).ToNot(HaveOccurred()) + Expect(dstClusterRequestInput).To(Equal(expected)) + }) + + It("should not add extraLabels/extraAnnotations field if not found in ClusterRequestInput", func() { + dstClusterRequestInput = map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "value1", + }, + }, + "clusterName": "cluster-1", + } + + srcConfigmap = map[string]any{ + "extraAnnotations": map[string]any{ // Field does not exist in dstClusterRequestInput + "ManagedCluster": map[string]any{ + "annotation1": "value1", + }, + }, + } + + expected := map[string]any{ + "extraLabels": map[string]any{ // Should remain unchanged + "ManagedCluster": map[string]any{ + "label1": "value1", + }, + }, + "clusterName": "cluster-1", + } + + err := OverrideClusterInstanceLabelsOrAnnotations(dstClusterRequestInput, srcConfigmap) + Expect(err).ToNot(HaveOccurred()) + Expect(dstClusterRequestInput).To(Equal(expected)) + }) + + It("should merge nodes and handle nested labels/annotations", func() { + dstClusterRequestInput = map[string]any{ + "clusterName": "cluster-1", + "nodes": []any{ + map[string]any{ + "hostName": "node1", + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "value1", + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation1": "value1", + }, + }, + }, + map[string]any{ + "hostName": "node2", + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label2": "value2", + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation2": "value2", + }, + }, + }, + }, + } + + srcConfigmap = map[string]any{ + "nodes": []any{ + map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "new_value1", // Existing label, should be overridden + "label2": "value2", // New label, should be ignored + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation2": "value2", // New annotation, should be ignored + }, + }, + }, + map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "value1", // New label, should be ignored + "label2": "new_value2", // Existing label, should be overridden + }, + }, + }, + }, + } + + expected := map[string]any{ + "clusterName": "cluster-1", + "nodes": []any{ + map[string]any{ + "hostName": "node1", + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "new_value1", // Overridden + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation1": "value1", // no change + }, + }, + }, + map[string]any{ + "hostName": "node2", + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label2": "new_value2", // Overridden + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation2": "value2", + }, + }, + }, + }, + } + + err := OverrideClusterInstanceLabelsOrAnnotations(dstClusterRequestInput, srcConfigmap) + Expect(err).ToNot(HaveOccurred()) + Expect(dstClusterRequestInput).To(Equal(expected)) + }) + + It("should not add the new node to dstClusterRequestInput", func() { + dstClusterRequestInput = map[string]any{ + "clusterName": "cluster-1", + "nodes": []any{ + map[string]any{ + "hostName": "node1", + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "value1", + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation1": "value1", + }, + }, + }, + }, + } + + srcConfigmap = map[string]any{ + "nodes": []any{ + map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "new_value1", // Existing label, should be overridden + "label2": "value2", // New label, should be ignored + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation2": "value2", // New annotation, should be ignored + }, + }, + }, + // New node, should be ignored + map[string]any{ + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "value1", + "label2": "value2", + }, + }, + }, + }, + } + + expected := map[string]any{ + "clusterName": "cluster-1", + "nodes": []any{ + map[string]any{ + "hostName": "node1", + "extraLabels": map[string]any{ + "ManagedCluster": map[string]any{ + "label1": "new_value1", // Overridden + }, + }, + "extraAnnotations": map[string]any{ + "ManagedCluster": map[string]any{ + "annotation1": "value1", // no change + }, + }, + }, + }, + } + + err := OverrideClusterInstanceLabelsOrAnnotations(dstClusterRequestInput, srcConfigmap) + Expect(err).ToNot(HaveOccurred()) + Expect(dstClusterRequestInput).To(Equal(expected)) + }) +})