feat: improve the override failure handling and message

pkg/controllers/workgenerator/controller.go

@@ -188,7 +188,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req controllerruntime.Reques
 				Status:             metav1.ConditionFalse,
 				Type:               string(fleetv1beta1.ResourceBindingOverridden),
 				Reason:             condition.OverriddenFailedReason,
-				Message:            fmt.Sprintf("Failed to apply the override rules on the resources: %s", errorMessage),
+				Message:            errorMessage,
 				ObservedGeneration: resourceBinding.Generation,
 			})
 		} else {
@@ -624,13 +624,13 @@ func (r *Reconciler) fetchAllResourceSnapshots(ctx context.Context, resourceBind
 func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePrefix string, resourceBinding *fleetv1beta1.ClusterResourceBinding,
 	resourceSnapshot *fleetv1beta1.ClusterResourceSnapshot, envelopeObj *unstructured.Unstructured, resourceOverrideSnapshotHash, clusterResourceOverrideSnapshotHash string) (*fleetv1beta1.Work, error) {
 	// we group all the resources in one configMap to one work
-	manifest, err := extractResFromConfigMap(envelopeObj)
+	manifests, err := extractResFromConfigMap(envelopeObj)
 	if err != nil {
 		klog.ErrorS(err, "configMap has invalid content", "snapshot", klog.KObj(resourceSnapshot),
 			"resourceBinding", klog.KObj(resourceBinding), "configMapWrapper", klog.KObj(envelopeObj))
 		return nil, controller.NewUserError(err)
 	}
-	klog.V(2).InfoS("Successfully extract the enveloped resources from the configMap", "numOfResources", len(manifest),
+	klog.V(2).InfoS("Successfully extract the enveloped resources from the configMap", "numOfResources", len(manifests),
 		"snapshot", klog.KObj(resourceSnapshot), "resourceBinding", klog.KObj(resourceBinding), "configMapWrapper", klog.KObj(envelopeObj))
 
 	// Try to see if we already have a work represent the same enveloped object for this CRP in the same cluster
@@ -643,7 +643,7 @@ func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePre
 		fleetv1beta1.EnvelopeNamespaceLabel: envelopeObj.GetNamespace(),
 	}
 	workList := &fleetv1beta1.WorkList{}
-	if err := r.Client.List(ctx, workList, envelopWorkLabelMatcher); err != nil {
+	if err = r.Client.List(ctx, workList, envelopWorkLabelMatcher); err != nil {
 		return nil, controller.NewAPIServerError(true, err)
 	}
 	// we need to create a new work object
@@ -680,7 +680,7 @@ func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePre
 			},
 			Spec: fleetv1beta1.WorkSpec{
 				Workload: fleetv1beta1.WorkloadTemplate{
-					Manifests: manifest,
+					Manifests: manifests,
 				},
 				ApplyStrategy: resourceBinding.Spec.ApplyStrategy,
 			},
@@ -699,7 +699,7 @@ func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePre
 	work.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = resourceBinding.Spec.ResourceSnapshotName
 	work.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = resourceOverrideSnapshotHash
 	work.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = clusterResourceOverrideSnapshotHash
-	work.Spec.Workload.Manifests = manifest
+	work.Spec.Workload.Manifests = manifests
 	work.Spec.ApplyStrategy = resourceBinding.Spec.ApplyStrategy
 	return &work, nil
 }

pkg/controllers/workgenerator/override.go

@@ -137,8 +137,9 @@ func (r *Reconciler) applyOverrides(resource *placementv1beta1.ResourceContent,
 			continue // should not happen
 		}
 		if err := applyOverrideRules(resource, cluster, snapshot.Spec.OverrideSpec.Policy.OverrideRules); err != nil {
-			klog.ErrorS(err, "Failed to apply the override rules", "clusterResourceOverrideSnapshot", klog.KObj(snapshot))
-			return false, err
+			overrideErr := fmt.Errorf("failed to apply the cluster override rule `%s` on resource `%s/%s/%s`: err = %s", snapshot.Name, uResource.GroupVersionKind(), uResource.GetNamespace(), uResource.GetName(), err.Error())
+			klog.ErrorS(err, "Failed to apply the override rules", "resource", klog.KObj(&uResource), "clusterResourceOverrideSnapshot", klog.KObj(snapshot))
+			return false, overrideErr
 		}
 	}
 	klog.V(2).InfoS("Applied clusterResourceOverrideSnapshots", "resource", klog.KObj(&uResource), "numberOfOverrides", len(croMap[key]))
@@ -160,8 +161,9 @@ func (r *Reconciler) applyOverrides(resource *placementv1beta1.ResourceContent,
 				continue // should not happen
 			}
 			if err := applyOverrideRules(resource, cluster, snapshot.Spec.OverrideSpec.Policy.OverrideRules); err != nil {
-				klog.ErrorS(err, "Failed to apply the override rules", "resourceOverrideSnapshot", klog.KObj(snapshot))
-				return false, err
+				overrideErr := fmt.Errorf("failed to apply the resource override rule %s/%s on resource `%s/%s/%s`: err = %s", snapshot.Namespace, snapshot.Name, uResource.GroupVersionKind(), uResource.GetNamespace(), uResource.GetName(), err.Error())
+				klog.ErrorS(err, "Failed to apply the override rules", "resource", klog.KObj(&uResource), "resourceOverrideSnapshot", klog.KObj(snapshot))
+				return false, overrideErr
 			}
 		}
 		klog.V(2).InfoS("Applied resourceOverrideSnapshots", "resource", klog.KObj(&uResource), "numberOfOverrides", len(roMap[key]))
@@ -174,7 +176,7 @@ func applyOverrideRules(resource *placementv1beta1.ResourceContent, cluster *clu
 		matched, err := overrider.IsClusterMatched(cluster, rule)
 		if err != nil {
 			klog.ErrorS(controller.NewUnexpectedBehaviorError(err), "Found an invalid override rule")
-			return controller.NewUserError(err) // should not happen though and should be rejected by the webhook
+			return err // should not happen though and should be rejected by the webhook
 		}
 		if !matched {
 			continue
@@ -265,7 +267,7 @@ func replaceClusterLabelKeyVariables(input string, cluster *clusterv1beta1.Membe
 			return "", fmt.Errorf("label key %s not found on cluster %s", keyName, cluster.Name)
 		}
 		// replace this instance of the variable with the actual label value
-		fullVariable := result[startIdx : endIdx+1]
+		fullVariable := fmt.Sprintf("%s%s}", placementv1alpha1.OverrideClusterLabelKeyVariablePrefix, keyName)
 		result = strings.Replace(result, fullVariable, labelValue, 1)
 	}
 	return result, nil

pkg/controllers/workgenerator/override_test.go

@@ -1123,7 +1123,7 @@ func TestApplyOverrides_clusterScopedResource(t *testing.T) {
 			}
 
 			var clusterRole rbacv1.ClusterRole
-			if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &clusterRole); err != nil {
+			if err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &clusterRole); err != nil {
 				t.Fatalf("Failed to convert the result to clusterole: %v, want nil", err)
 			}
 

test/e2e/actuals_test.go

@@ -976,7 +976,7 @@ func workNamespaceRemovedFromClusterActual(cluster *framework.Cluster) func() er
 	ns := appNamespace()
 	return func() error {
 		if err := client.Get(ctx, types.NamespacedName{Name: ns.Name}, &corev1.Namespace{}); !errors.IsNotFound(err) {
-			return fmt.Errorf("work namespace %s still exists or an unexpected error occurred: %w", ns.Name, err)
+			return fmt.Errorf("work namespace %s still exists on cluster %s or an unexpected error occurred: %w", ns.Name, cluster.ClusterName, err)
 		}
 		return nil
 	}

test/e2e/placement_ro_test.go

@@ -161,6 +161,9 @@ var _ = Context("creating resourceOverride with multiple jsonPatchOverrides to o
 				Namespace: roNamespace,
 			},
 			Spec: placementv1alpha1.ResourceOverrideSpec{
+				Placement: &placementv1alpha1.PlacementRef{
+					Name: crpName, // assigned CRP name
+				},
 				ResourceSelectors: configMapSelector(),
 				Policy: &placementv1alpha1.OverridePolicy{
 					OverrideRules: []placementv1alpha1.OverrideRule{
@@ -599,6 +602,9 @@ var _ = Context("creating resourceOverride with a templated rules with cluster n
 				Namespace: roNamespace,
 			},
 			Spec: placementv1alpha1.ResourceOverrideSpec{
+				Placement: &placementv1alpha1.PlacementRef{
+					Name: crpName, // assigned CRP name
+				},
 				ResourceSelectors: configMapSelector(),
 				Policy: &placementv1alpha1.OverridePolicy{
 					OverrideRules: []placementv1alpha1.OverrideRule{
@@ -693,6 +699,9 @@ var _ = Context("creating resourceOverride with delete configMap", Ordered, func
 				Namespace: roNamespace,
 			},
 			Spec: placementv1alpha1.ResourceOverrideSpec{
+				Placement: &placementv1alpha1.PlacementRef{
+					Name: crpName, // assigned CRP name
+				},
 				ResourceSelectors: configMapSelector(),
 				Policy: &placementv1alpha1.OverridePolicy{
 					OverrideRules: []placementv1alpha1.OverrideRule{
@@ -789,3 +798,138 @@ var _ = Context("creating resourceOverride with delete configMap", Ordered, func
 		}, consistentlyDuration, consistentlyInterval).Should(BeTrue(), "Failed to delete work resources on member cluster %s", memberCluster.ClusterName)
 	})
 })
+
+var _ = FContext("creating resourceOverride with a templated rules  with cluster label key replacement", Ordered, func() {
+	crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess())
+	roName := fmt.Sprintf(roNameTemplate, GinkgoParallelProcess())
+	roNamespace := fmt.Sprintf(workNamespaceNameTemplate, GinkgoParallelProcess())
+
+	BeforeAll(func() {
+		By("creating work resources")
+		createWorkResources()
+
+		// Create the ro before crp so that the observed resource index is predictable.
+		ro := &placementv1alpha1.ResourceOverride{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      roName,
+				Namespace: roNamespace,
+			},
+			Spec: placementv1alpha1.ResourceOverrideSpec{
+				Placement: &placementv1alpha1.PlacementRef{
+					Name: crpName, // assigned CRP name
+				},
+				ResourceSelectors: configMapSelector(),
+				Policy: &placementv1alpha1.OverridePolicy{
+					OverrideRules: []placementv1alpha1.OverrideRule{
+						{
+							ClusterSelector: &placementv1beta1.ClusterSelector{
+								ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
+									{
+										LabelSelector: &metav1.LabelSelector{
+											MatchExpressions: []metav1.LabelSelectorRequirement{
+												{
+													Key:      regionLabelName,
+													Operator: metav1.LabelSelectorOpExists,
+												},
+												{
+													Key:      envLabelName,
+													Operator: metav1.LabelSelectorOpExists,
+												},
+											},
+										},
+									},
+								},
+							},
+							JSONPatchOverrides: []placementv1alpha1.JSONPatchOverride{
+								{
+									Operator: placementv1alpha1.JSONPatchOverrideOpAdd,
+									Path:     "/data/region",
+									Value:    apiextensionsv1.JSON{Raw: []byte(fmt.Sprintf(`"%s%s}"`, placementv1alpha1.OverrideClusterLabelKeyVariablePrefix, envLabelName))},
+								},
+								{
+									Operator: placementv1alpha1.JSONPatchOverrideOpReplace,
+									Path:     "/data/data",
+									Value:    apiextensionsv1.JSON{Raw: []byte(fmt.Sprintf(`"newdata-%s%s}"`, placementv1alpha1.OverrideClusterLabelKeyVariablePrefix, regionLabelName))},
+								},
+							},
+						},
+					},
+				},
+			},
+		}
+		By(fmt.Sprintf("creating resourceOverride %s", roName))
+		Expect(hubClient.Create(ctx, ro)).To(Succeed(), "Failed to create resourceOverride %s", roName)
+
+		// Create the CRP.
+		createCRP(crpName)
+	})
+
+	AfterAll(func() {
+		By(fmt.Sprintf("deleting placement %s and related resources", crpName))
+		ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)
+
+		By(fmt.Sprintf("deleting resourceOverride %s", roName))
+		cleanupResourceOverride(roName, roNamespace)
+	})
+
+	It("should update CRP status as expected", func() {
+		wantRONames := []placementv1beta1.NamespacedName{
+			{Namespace: roNamespace, Name: fmt.Sprintf(placementv1alpha1.OverrideSnapshotNameFmt, roName, 0)},
+		}
+		crpStatusUpdatedActual := crpStatusWithOverrideUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, "0", nil, wantRONames)
+		Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
+	})
+
+	It("should replace the cluster label key in the configMap", func() {
+		cmName := fmt.Sprintf(appConfigMapNameTemplate, GinkgoParallelProcess())
+		cmNamespace := fmt.Sprintf(workNamespaceNameTemplate, GinkgoParallelProcess())
+		for _, cluster := range allMemberClusters {
+
+			wantConfigMap := &corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      cmName,
+					Namespace: cmNamespace,
+				},
+				Data: map[string]string{
+					"data":   fmt.Sprintf("newdata-%s", labelsByClusterName[cluster.ClusterName][envLabelName]),
+					"region": labelsByClusterName[cluster.ClusterName][regionLabelName],
+				},
+			}
+			configMapActual := configMapPlacedOnClusterActual(cluster, wantConfigMap)
+			Eventually(configMapActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update configmap %s data as expected", cmName)
+		}
+	})
+
+	It("should handle non-existent cluster label key gracefully", func() {
+		By("Update the ResourceOverride to use a non-existent label key")
+		Eventually(func() error {
+			ro := &placementv1alpha1.ResourceOverride{}
+			if err := hubClient.Get(ctx, types.NamespacedName{Name: roName, Namespace: roNamespace}, ro); err != nil {
+				return err
+			}
+			ro.Spec.Policy.OverrideRules[0].JSONPatchOverrides[0].Value = apiextensionsv1.JSON{Raw: []byte(fmt.Sprintf(`"%snon-existent-label}"`, placementv1alpha1.OverrideClusterLabelKeyVariablePrefix))}
+			return hubClient.Update(ctx, ro)
+		}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update resourceOverride %s with non-existent label key", roName)
+
+		// Verify CRP status reflects the error
+		crpStatusUpdatedActual := crpStatusWithOverrideUpdatedFailedActual(workResourceIdentifiers(), allMemberClusterNames, "0", nil, nil)
+		Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "CRP %s status should reflect the error due to non-existent label key", crpName)
+
+		// Verify the configMap remains unchanged
+		cmName := fmt.Sprintf(appConfigMapNameTemplate, GinkgoParallelProcess())
+		cmNamespace := fmt.Sprintf(workNamespaceNameTemplate, GinkgoParallelProcess())
+		for _, cluster := range allMemberClusters {
+			wantConfigMap := &corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      cmName,
+					Namespace: cmNamespace,
+				},
+				Data: map[string]string{
+					"region": labelsByClusterName[cluster.ClusterName][regionLabelName],
+				},
+			}
+			configMapActual := configMapPlacedOnClusterActual(cluster, wantConfigMap)
+			Consistently(configMapActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "ConfigMap %s should remain unchanged", cmName)
+		}
+	})
+})