Merge pull request #5671 from chaunceyjiang/fieldoverrider_validate

feat: validate fieldOverrider operation
karmada-io · Oct 12, 2024 · b52775f · b52775f
2 parents d295576 + f45aedb
commit b52775f
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 0 deletions.
diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/go-openapi/jsonpointer"
 	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/util/validation"
@@ -340,6 +341,7 @@ func validateJSONPatchSubPaths(patches []policyv1alpha1.JSONPatchOperation, fiel
 		if _, err := jsonpointer.New(patch.SubPath); err != nil {
 			allErrs = append(allErrs, field.Invalid(patchPath.Child("subPath"), patch.SubPath, err.Error()))
 		}
+		allErrs = append(allErrs, validateOverrideOperator(patch.Operator, patch.Value, patchPath.Child("value"))...)
 	}
 	return allErrs
 }
@@ -351,6 +353,22 @@ func validateYAMLPatchSubPaths(patches []policyv1alpha1.YAMLPatchOperation, fiel
 		if _, err := jsonpointer.New(patch.SubPath); err != nil {
 			allErrs = append(allErrs, field.Invalid(patchPath.Child("subPath"), patch.SubPath, err.Error()))
 		}
+		allErrs = append(allErrs, validateOverrideOperator(patch.Operator, patch.Value, patchPath.Child("value"))...)
+	}
+	return allErrs
+}
+
+func validateOverrideOperator(operator policyv1alpha1.OverriderOperator, value apiextensionsv1.JSON, fldPath *field.Path) field.ErrorList {
+	var allErrs field.ErrorList
+	switch operator {
+	case policyv1alpha1.OverriderOpAdd, policyv1alpha1.OverriderOpReplace:
+		if value.Size() == 0 {
+			allErrs = append(allErrs, field.Invalid(fldPath, value, "value is required for add or replace operation"))
+		}
+	case policyv1alpha1.OverriderOpRemove:
+		if value.Size() != 0 {
+			allErrs = append(allErrs, field.Invalid(fldPath, value, "value is not allowed for remove operation"))
+		}
 	}
 	return allErrs
 }
diff --git a/pkg/webhook/overridepolicy/validating_test.go b/pkg/webhook/overridepolicy/validating_test.go
@@ -296,6 +296,100 @@ func TestValidatingAdmission_Handle(t *testing.T) {
 				Message: "spec.overrideRules[0].overriders.fieldOverrider[0].yaml[0].subPath: Invalid value: \"invalidSubPath\": JSON pointer must be empty or start with a \"/",
 			},
 		},
+		{
+			name: "Handle_InvalidJSONValue_DeniesAdmission_OverriderOpReplace",
+			decoder: &fakeValidationDecoder{
+				obj: &policyv1alpha1.OverridePolicy{
+					Spec: policyv1alpha1.OverrideSpec{
+						OverrideRules: []policyv1alpha1.RuleWithCluster{
+							{
+								Overriders: policyv1alpha1.Overriders{
+									FieldOverrider: []policyv1alpha1.FieldOverrider{
+										{
+											FieldPath: "/data/config",
+											JSON: []policyv1alpha1.JSONPatchOperation{
+												{
+													SubPath:  "/db-config",
+													Operator: policyv1alpha1.OverriderOpReplace,
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			req: admission.Request{},
+			want: TestResponse{
+				Type:    Denied,
+				Message: "spec.overrideRules[0].overriders.fieldOverrider[0].json[0].value: Invalid value: v1.JSON{Raw:[]uint8(nil)}: value is required for add or replace operation",
+			},
+		},
+		{
+			name: "Handle_InvalidJSONValue_DeniesAdmission_OverriderOpAdd",
+			decoder: &fakeValidationDecoder{
+				obj: &policyv1alpha1.OverridePolicy{
+					Spec: policyv1alpha1.OverrideSpec{
+						OverrideRules: []policyv1alpha1.RuleWithCluster{
+							{
+								Overriders: policyv1alpha1.Overriders{
+									FieldOverrider: []policyv1alpha1.FieldOverrider{
+										{
+											FieldPath: "/data/config",
+											JSON: []policyv1alpha1.JSONPatchOperation{
+												{
+													SubPath:  "/db-config",
+													Operator: policyv1alpha1.OverriderOpAdd,
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			req: admission.Request{},
+			want: TestResponse{
+				Type:    Denied,
+				Message: "spec.overrideRules[0].overriders.fieldOverrider[0].json[0].value: Invalid value: v1.JSON{Raw:[]uint8(nil)}: value is required for add or replace operation",
+			},
+		},
+		{
+			name: "Handle_InvalidJSONValue_DeniesAdmission_OverriderOpRemove",
+			decoder: &fakeValidationDecoder{
+				obj: &policyv1alpha1.OverridePolicy{
+					Spec: policyv1alpha1.OverrideSpec{
+						OverrideRules: []policyv1alpha1.RuleWithCluster{
+							{
+								Overriders: policyv1alpha1.Overriders{
+									FieldOverrider: []policyv1alpha1.FieldOverrider{
+										{
+											FieldPath: "/data/config",
+											JSON: []policyv1alpha1.JSONPatchOperation{
+												{
+													SubPath:  "/db-config",
+													Operator: policyv1alpha1.OverriderOpRemove,
+													Value:    apiextensionsv1.JSON{Raw: []byte(`{"db": "new"}`)},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			req: admission.Request{},
+			want: TestResponse{
+				Type:    Denied,
+				Message: "spec.overrideRules[0].overriders.fieldOverrider[0].json[0].value: Invalid value: v1.JSON{Raw:[]uint8{0x7b, 0x22, 0x64, 0x62, 0x22, 0x3a, 0x20, 0x22, 0x6e, 0x65, 0x77, 0x22, 0x7d}}: value is not allowed for remove operation",
+			},
+		},
 	}
 
 	for _, tt := range tests {