address comments

apis/placement/v1alpha1/override_types.go

@@ -194,7 +194,7 @@ type JSONPatchOverride struct {
 	// We have reserved a few variables in this field that will be replaced by the actual values.
 	// Those variables all start with `$` and are case sensitive.
 	// Here is the list of currently supported variables:
-	// `${MEMBER-CLUSTER-NAME}`:  this will be replaced by the actual cluster name.
+	// `${MEMBER-CLUSTER-NAME}`:  this will be replaced by the name of the memberCluster CR that represents this cluster.
 	// +optional
 	Value apiextensionsv1.JSON `json:"value,omitempty"`
 }

pkg/controllers/workgenerator/override.go

@@ -198,27 +198,22 @@ func applyJSONPatchOverride(resourceContent *placementv1beta1.ResourceContent, c
 	if len(overrides) == 0 { // do nothing
 		return nil
 	}
-	// go through the JSON patch overrides to replace the built-in variables
-	var processedOverrides []placementv1alpha1.JSONPatchOverride
-	for _, override := range overrides {
-		// Replace the built-in variable with the actual cluster name
-		processedOverride := placementv1alpha1.JSONPatchOverride{
-			Operator: override.Operator,
-			Path:     override.Path,
-		}
+	// go through the JSON patch overrides to replace the built-in variables before json Marshal
+	// as it may contain the built-in variables that cannot be marshaled directly
+	for i := range overrides {
 		// find and replace a few special built-in variables
-		processedJSONStr := []byte(strings.ReplaceAll(string(override.Value.Raw), placementv1alpha1.OverrideClusterNameVariable, cluster.Name))
-		processedOverride.Value.Raw = processedJSONStr
-		processedOverrides = append(processedOverrides, processedOverride)
+		// replace the built-in variable with the actual cluster name
+		processedJSONStr := []byte(strings.ReplaceAll(string(overrides[i].Value.Raw), placementv1alpha1.OverrideClusterNameVariable, cluster.Name))
+		overrides[i].Value.Raw = processedJSONStr
 	}
 
-	jsonPatchBytes, err := json.Marshal(processedOverrides)
+	jsonPatchBytes, err := json.Marshal(overrides)
 	if err != nil {
 		klog.ErrorS(err, "Failed to marshal JSON Patch overrides")
 		return err
 	}
 
-	patch, err := jsonpatch.DecodePatch([]byte(jsonPatchBytes))
+	patch, err := jsonpatch.DecodePatch(jsonPatchBytes)
 	if err != nil {
 		klog.ErrorS(err, "Failed to decode the passed JSON document as an RFC 6902 patch")
 		return err

pkg/controllers/workgenerator/override_test.go

@@ -2285,7 +2285,7 @@ func TestApplyJSONPatchOverride(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "typo in template variable should just be ignored",
+			name: "typo in template variable should just be rendered as is",
 			deployment: appsv1.Deployment{
 				TypeMeta: deploymentType,
 				ObjectMeta: metav1.ObjectMeta{
@@ -2302,6 +2302,11 @@ func TestApplyJSONPatchOverride(t *testing.T) {
 					Path:     "/metadata/labels/app",
 					Value:    apiextensionsv1.JSON{Raw: []byte(`"$CLUSTER_NAME"`)},
 				},
+				{
+					Operator: placementv1alpha1.JSONPatchOverrideOpAdd,
+					Path:     "/metadata/labels/${Member-Cluster-Name}",
+					Value:    apiextensionsv1.JSON{Raw: []byte(`"${CLUSTER-NAME}"`)},
+				},
 			},
 			cluster: &clusterv1beta1.MemberCluster{
 				ObjectMeta: metav1.ObjectMeta{
@@ -2314,7 +2319,8 @@ func TestApplyJSONPatchOverride(t *testing.T) {
 					Name:      "deployment-name",
 					Namespace: "deployment-namespace",
 					Labels: map[string]string{
-						"app": "$CLUSTER_NAME",
+						"app":                    "$CLUSTER_NAME",
+						"${Member-Cluster-Name}": "${CLUSTER-NAME}",
 					},
 				},
 			},
@@ -2340,7 +2346,7 @@ func TestApplyJSONPatchOverride(t *testing.T) {
 				{
 					Operator: placementv1alpha1.JSONPatchOverrideOpAdd,
 					Path:     "/metadata/annotations",
-					Value:    apiextensionsv1.JSON{Raw: []byte(fmt.Sprintf("{\"app\": \"%s\", \"test\": \"nginx\"}", placementv1alpha1.OverrideClusterNameVariable))},
+					Value:    apiextensionsv1.JSON{Raw: []byte(fmt.Sprintf("{\"app\": \"workload-%s\", \"test\": \"nginx\"}", placementv1alpha1.OverrideClusterNameVariable))},
 				},
 			},
 			cluster: &clusterv1beta1.MemberCluster{
@@ -2357,7 +2363,7 @@ func TestApplyJSONPatchOverride(t *testing.T) {
 						"app": "cluster-1",
 					},
 					Annotations: map[string]string{
-						"app":  "cluster-1",
+						"app":  "workload-cluster-1",
 						"test": "nginx",
 					},
 				},

test/e2e/placement_apply_strategy_test.go

@@ -60,7 +60,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 
 			// Create the CRP.
 			strategy := &placementv1beta1.ApplyStrategy{AllowCoOwnership: true}
-			createCRP(crpName, strategy)
+			createCRPWithApplyStrategy(crpName, strategy)
 		})
 
 		AfterAll(func() {
@@ -122,7 +122,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 
 			// Create the CRP.
 			strategy := &placementv1beta1.ApplyStrategy{AllowCoOwnership: false}
-			createCRP(crpName, strategy)
+			createCRPWithApplyStrategy(crpName, strategy)
 		})
 
 		AfterAll(func() {
@@ -175,7 +175,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 				Type:             placementv1beta1.ApplyStrategyTypeServerSideApply,
 				AllowCoOwnership: false,
 			}
-			createCRP(crpName, strategy)
+			createCRPWithApplyStrategy(crpName, strategy)
 		})
 
 		AfterAll(func() {
@@ -233,7 +233,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 				Type:             placementv1beta1.ApplyStrategyTypeServerSideApply,
 				AllowCoOwnership: false,
 			}
-			createCRP(crpName, strategy)
+			createCRPWithApplyStrategy(crpName, strategy)
 		})
 
 		AfterAll(func() {
@@ -368,7 +368,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 				ServerSideApplyConfig: &placementv1beta1.ServerSideApplyConfig{ForceConflicts: false},
 				AllowCoOwnership:      true,
 			}
-			createCRP(crpName, strategy)
+			createCRPWithApplyStrategy(crpName, strategy)
 		})
 
 		AfterAll(func() {
@@ -515,7 +515,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 				ServerSideApplyConfig: &placementv1beta1.ServerSideApplyConfig{ForceConflicts: true},
 				AllowCoOwnership:      true,
 			}
-			createCRP(crpName, strategy)
+			createCRPWithApplyStrategy(crpName, strategy)
 		})
 
 		AfterAll(func() {
@@ -584,7 +584,7 @@ var _ = Describe("validating two CRP selecting the same resources", Ordered, fun
 		BeforeAll(func() {
 			for i := 0; i < 2; i++ {
 				crpName := fmt.Sprintf(crpNameWithSubIndexTemplate, GinkgoParallelProcess(), i)
-				createCRP(crpName, nil)
+				createCRPWithApplyStrategy(crpName, nil)
 			}
 		})
 
@@ -620,7 +620,7 @@ var _ = Describe("validating two CRP selecting the same resources", Ordered, fun
 
 		It("add a new CRP and selecting the same resources", func() {
 			crpName := fmt.Sprintf(crpNameWithSubIndexTemplate, GinkgoParallelProcess(), 2)
-			createCRP(crpName, nil)
+			createCRPWithApplyStrategy(crpName, nil)
 		})
 
 		It("should update CRP status as expected", func() {
@@ -660,7 +660,7 @@ var _ = Describe("validating two CRP selecting the same resources", Ordered, fun
 		crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess())
 		conflictCRPName := fmt.Sprintf(crpNameWithSubIndexTemplate, GinkgoParallelProcess(), 0)
 		BeforeAll(func() {
-			createCRP(crpName, nil)
+			createCRPWithApplyStrategy(crpName, nil)
 		})
 
 		AfterAll(func() {
@@ -680,7 +680,7 @@ var _ = Describe("validating two CRP selecting the same resources", Ordered, fun
 		It("should place the selected resources on member clusters", checkIfPlacedWorkResourcesOnAllMemberClusters)
 
 		It("create another CRP with different apply strategy", func() {
-			createCRP(conflictCRPName, &placementv1beta1.ApplyStrategy{AllowCoOwnership: true})
+			createCRPWithApplyStrategy(conflictCRPName, &placementv1beta1.ApplyStrategy{AllowCoOwnership: true})
 		})
 
 		It("should update conflicted CRP status as expected", func() {
@@ -740,25 +740,6 @@ var _ = Describe("validating two CRP selecting the same resources", Ordered, fun
 	})
 })
 
-func createCRP(crpName string, applyStrategy *placementv1beta1.ApplyStrategy) {
-	crp := &placementv1beta1.ClusterResourcePlacement{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: crpName,
-			// Add a custom finalizer; this would allow us to better observe
-			// the behavior of the controllers.
-			Finalizers: []string{customDeletionBlockerFinalizer},
-		},
-		Spec: placementv1beta1.ClusterResourcePlacementSpec{
-			ResourceSelectors: workResourceSelector(),
-			Strategy: placementv1beta1.RolloutStrategy{
-				ApplyStrategy: applyStrategy,
-			},
-		},
-	}
-	By(fmt.Sprintf("creating placement %s", crpName))
-	Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP %s", crpName)
-}
-
 func buildApplyConflictFailedPlacements(generation int64, cluster []string) []placementv1beta1.ResourcePlacementStatus {
 	workNamespaceName := fmt.Sprintf(workNamespaceNameTemplate, GinkgoParallelProcess())
 	res := make([]placementv1beta1.ResourcePlacementStatus, 0, len(cluster))

test/e2e/placement_cro_test.go

@@ -372,7 +372,7 @@ var _ = Describe("creating clusterResourceOverride with different rules for each
 	BeforeAll(func() {
 		By("creating work resources")
 		createWorkResources()
-		CreateCRP(crpName)
+		createCRP(crpName)
 		cro := &placementv1alpha1.ClusterResourceOverride{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: croName,
@@ -440,24 +440,6 @@ var _ = Describe("creating clusterResourceOverride with different rules for each
 	})
 })
 
-// CreateCRP creates a ClusterResourcePlacement with the given name.
-func CreateCRP(crpName string) {
-	// Create the CRP.
-	crp := &placementv1beta1.ClusterResourcePlacement{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: crpName,
-			// Add a custom finalizer; this would allow us to better observe
-			// the behavior of the controllers.
-			Finalizers: []string{customDeletionBlockerFinalizer},
-		},
-		Spec: placementv1beta1.ClusterResourcePlacementSpec{
-			ResourceSelectors: workResourceSelector(),
-		},
-	}
-	By(fmt.Sprintf("creating placement %s", crpName))
-	Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP %s", crpName)
-}
-
 var _ = Describe("creating clusterResourceOverride with incorrect path", Ordered, func() {
 	crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess())
 	croName := fmt.Sprintf(croNameTemplate, GinkgoParallelProcess())

test/e2e/placement_ro_test.go

@@ -29,7 +29,7 @@ var _ = Describe("creating resourceOverride (selecting all clusters) to override
 		By("creating work resources")
 		createWorkResources()
 		// Create the CRP.
-		CreateCRP(crpName)
+		createCRP(crpName)
 		// Create the ro.
 		ro := &placementv1alpha1.ResourceOverride{
 			ObjectMeta: metav1.ObjectMeta{
@@ -181,7 +181,7 @@ var _ = Describe("creating resourceOverride with multiple jsonPatchOverrides to
 		Expect(hubClient.Create(ctx, ro)).To(Succeed(), "Failed to create resourceOverride %s", roName)
 
 		// Create the CRP.
-		CreateCRP(crpName)
+		createCRP(crpName)
 	})
 
 	AfterAll(func() {
@@ -240,7 +240,7 @@ var _ = Describe("creating resourceOverride with different rules for each cluste
 		By("creating work resources")
 		createWorkResources()
 		// Create the CRP.
-		CreateCRP(crpName)
+		createCRP(crpName)
 		// Create the ro.
 		ro := &placementv1alpha1.ResourceOverride{
 			ObjectMeta: metav1.ObjectMeta{
@@ -408,7 +408,7 @@ var _ = Describe("creating resourceOverride and clusterResourceOverride, resourc
 		Expect(hubClient.Create(ctx, ro)).To(Succeed(), "Failed to create resourceOverride %s", roName)
 
 		// Create the CRP.
-		CreateCRP(crpName)
+		createCRP(crpName)
 	})
 
 	AfterAll(func() {
@@ -457,7 +457,7 @@ var _ = Describe("creating resourceOverride with incorrect path", Ordered, func(
 		By("creating work resources")
 		createWorkResources()
 		// Create the CRP.
-		CreateCRP(crpName)
+		createCRP(crpName)
 		// Create the ro.
 		ro := &placementv1alpha1.ResourceOverride{
 			ObjectMeta: metav1.ObjectMeta{
@@ -520,7 +520,7 @@ var _ = Describe("creating resourceOverride and resource becomes invalid after o
 		By("creating work resources")
 		createWorkResources()
 		// Create the CRP.
-		CreateCRP(crpName)
+		createCRP(crpName)
 		// Create the ro.
 		ro := &placementv1alpha1.ResourceOverride{
 			ObjectMeta: metav1.ObjectMeta{
@@ -629,7 +629,7 @@ var _ = Describe("creating resourceOverride with a templated rules with cluster
 		Expect(hubClient.Create(ctx, ro)).To(Succeed(), "Failed to create resourceOverride %s", roName)
 
 		// Create the CRP.
-		CreateCRP(crpName)
+		createCRP(crpName)
 	})
 
 	AfterAll(func() {
@@ -725,7 +725,7 @@ var _ = Describe("creating resourceOverride with delete configMap", Ordered, fun
 		Expect(hubClient.Create(ctx, ro)).To(Succeed(), "Failed to create resourceOverride %s", roName)
 
 		// Create the CRP.
-		CreateCRP(crpName)
+		createCRP(crpName)
 	})
 
 	AfterAll(func() {

test/e2e/utils_test.go

@@ -1212,3 +1212,28 @@ func checkIfStatusErrorWithMessage(err error, errorMsg string) error {
 	}
 	return fmt.Errorf("error message %s not found in error %w", errorMsg, err)
 }
+
+// createCRPWithApplyStrategy creates a ClusterResourcePlacement with the given name and apply strategy.
+func createCRPWithApplyStrategy(crpName string, applyStrategy *placementv1beta1.ApplyStrategy) {
+	crp := &placementv1beta1.ClusterResourcePlacement{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: crpName,
+			// Add a custom finalizer; this would allow us to better observe
+			// the behavior of the controllers.
+			Finalizers: []string{customDeletionBlockerFinalizer},
+		},
+		Spec: placementv1beta1.ClusterResourcePlacementSpec{
+			ResourceSelectors: workResourceSelector(),
+		},
+	}
+	if applyStrategy != nil {
+		crp.Spec.Strategy.ApplyStrategy = applyStrategy
+	}
+	By(fmt.Sprintf("creating placement %s", crpName))
+	Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP %s", crpName)
+}
+
+// createCRP creates a ClusterResourcePlacement with the given name.
+func createCRP(crpName string) {
+	createCRPWithApplyStrategy(crpName, nil)
+}