Merge pull request #3037 from chaunceyjiang/composedlabels

fix: OverridePolicy of labels/annotations with composed-labels

pkg/util/overridemanager/labelannotationoverrider.go

@@ -47,6 +47,13 @@ func buildLabelAnnotationOverriderPatches(rawObj *unstructured.Unstructured, ove
 				continue
 			}
 		}
+		// If the key contains '/', we must replace(escape) it to '~1' according to the
+		// rule of jsonpath identifying a specific value.
+		// See https://jsonpatch.com/#json-pointer for more details.
+		// Note: here don't replace '~' because it is not a valid character for
+		// both annotations and labels, the key with '~' should be prevented at
+		// the validation phase.
+		key = strings.ReplaceAll(key, "/", "~1")
 		patches = append(patches, overrideOption{
 			Op:    string(overrider.Operator),
 			Path:  "/" + strings.Join(append(path, key), "/"),

pkg/util/overridemanager/labelannotationoverrider_test.go

@@ -147,6 +147,13 @@ func Test_applyAnnotationsOverriders(t *testing.T) {
 	}
 	deployment2 := helper.NewDeployment(metav1.NamespaceDefault, "test")
 	deployment2.Annotations = nil
+
+	deployment3 := helper.NewDeployment(metav1.NamespaceDefault, "test")
+	deployment3.Annotations = map[string]string{
+		"testannotation/projectId": "c-m-lfx9lk92:p-v86cf",
+		"foo":                      "foo",
+	}
+
 	tests := []struct {
 		name    string
 		args    args
@@ -306,6 +313,48 @@ func Test_applyAnnotationsOverriders(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "test add composed annotation",
+			args: args{
+				rawObj: func() *unstructured.Unstructured {
+					deploymentObj, _ := utilhelper.ToUnstructured(deployment)
+					return deploymentObj
+				}(),
+				commandOverriders: []policyv1alpha1.LabelAnnotationOverrider{
+					{
+						Operator: policyv1alpha1.OverriderOpAdd,
+						Value: map[string]string{
+							"testannotation/projectId": "c-m-lfx9lk92:p-v86cf",
+						},
+					},
+				},
+			},
+			want: map[string]string{
+				"foo":                      "foo",
+				"bar":                      "bar",
+				"testannotation/projectId": "c-m-lfx9lk92:p-v86cf",
+			},
+			wantErr: false,
+		},
+		{
+			name: "test remove composed annotation",
+			args: args{
+				rawObj: func() *unstructured.Unstructured {
+					deploymentObj, _ := utilhelper.ToUnstructured(deployment3)
+					return deploymentObj
+				}(),
+				commandOverriders: []policyv1alpha1.LabelAnnotationOverrider{
+					{
+						Operator: policyv1alpha1.OverriderOpRemove,
+						Value: map[string]string{
+							"testannotation/projectId": "c-m-lfx9lk92:p-v86cf",
+						},
+					},
+				},
+			},
+			want:    map[string]string{"foo": "foo"},
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/util/validation/validation.go

@@ -4,7 +4,10 @@ import (
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
+	apivalidation "k8s.io/apimachinery/pkg/api/validation"
+	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
 	"github.com/karmada-io/karmada/pkg/util"
@@ -68,7 +71,7 @@ func ValidateSpreadConstraint(spreadConstraints []policyv1alpha1.SpreadConstrain
 	return nil
 }
 
-// ValidateOverrideSpec tests if the overrideRules and (overriders or targetCluster) co-exist
+// ValidateOverrideSpec validates that the overrider specification is correctly defined.
 func ValidateOverrideSpec(overrideSpec *policyv1alpha1.OverrideSpec) error {
 	if overrideSpec == nil {
 		return nil
@@ -84,6 +87,25 @@ func ValidateOverrideSpec(overrideSpec *policyv1alpha1.OverrideSpec) error {
 		return fmt.Errorf("overrideRules and (overriders or targetCluster) can't co-exist")
 	}
 
+	for overrideRuleIndex, rule := range overrideSpec.OverrideRules {
+		rulePath := field.NewPath("spec").Child("overrideRules").Index(overrideRuleIndex)
+
+		// validates provided annotations.
+		for annotationIndex, annotation := range rule.Overriders.AnnotationsOverrider {
+			annotationPath := rulePath.Child("overriders").Child("annotationsOverrider").Index(annotationIndex)
+			if err := apivalidation.ValidateAnnotations(annotation.Value, annotationPath.Child("value")).ToAggregate(); err != nil {
+				return err
+			}
+		}
+
+		// validates provided labels.
+		for labelIndex, label := range rule.Overriders.LabelsOverrider {
+			labelPath := rulePath.Child("overriders").Child("labelsOverrider").Index(labelIndex)
+			if err := metav1validation.ValidateLabels(label.Value, labelPath.Child("value")).ToAggregate(); err != nil {
+				return err
+			}
+		}
+	}
 	return nil
 }
 

pkg/util/validation/validation_test.go

@@ -105,6 +105,48 @@ func TestValidateOverrideSpec(t *testing.T) {
 			},
 			expectError: true,
 		},
+		{
+			name: "invalid annotation should not be allowed",
+			overrideSpec: policyv1alpha1.OverrideSpec{
+				OverrideRules: []policyv1alpha1.RuleWithCluster{
+					{
+						TargetCluster: &policyv1alpha1.ClusterAffinity{
+							ClusterNames: []string{"cluster-name"},
+						},
+						Overriders: policyv1alpha1.Overriders{
+							AnnotationsOverrider: []policyv1alpha1.LabelAnnotationOverrider{
+								{
+									Operator: "add",
+									Value:    map[string]string{"testannotation~projectId": "c-m-lfx9lk92p-v86cf"},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectError: true,
+		},
+		{
+			name: "invalid label should not be allowed",
+			overrideSpec: policyv1alpha1.OverrideSpec{
+				OverrideRules: []policyv1alpha1.RuleWithCluster{
+					{
+						TargetCluster: &policyv1alpha1.ClusterAffinity{
+							ClusterNames: []string{"cluster-name"},
+						},
+						Overriders: policyv1alpha1.Overriders{
+							LabelsOverrider: []policyv1alpha1.LabelAnnotationOverrider{
+								{
+									Operator: "add",
+									Value:    map[string]string{"testannotation~projectId": "c-m-lfx9lk92p-v86cf"},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectError: true,
+		},
 	}
 
 	for _, test := range tests {