Merge pull request #1124 from lonelyCZ/override-validating

Implementing karmada-webhook of validating OverridePolicy

artifacts/deploy/webhook-configuration.yaml

@@ -111,6 +111,20 @@ webhooks:
     sideEffects: None
     admissionReviewVersions: ["v1"]
     timeoutSeconds: 3
+  - name: overridepolicy.karmada.io
+    rules:
+      - operations: ["CREATE", "UPDATE"]
+        apiGroups: ["policy.karmada.io"]
+        apiVersions: ["*"]
+        resources: ["overridepolicies"]
+        scope: "Namespaced"
+    clientConfig:
+      url: https://karmada-webhook.karmada-system.svc:443/validate-overridepolicy
+      caBundle: {{caBundle}}
+    failurePolicy: Fail
+    sideEffects: None
+    admissionReviewVersions: ["v1"]
+    timeoutSeconds: 3
   - name: config.karmada.io
     rules:
       - operations: ["CREATE", "UPDATE"]

charts/templates/_karmada_webhook_configuration.tpl

@@ -115,6 +115,20 @@ webhooks:
     sideEffects: None
     admissionReviewVersions: ["v1"]
     timeoutSeconds: 3
+  - name: overridepolicy.karmada.io
+    rules:
+      - operations: ["CREATE", "UPDATE"]
+        apiGroups: ["policy.karmada.io"]
+        apiVersions: ["*"]
+        resources: ["overridepolicies"]
+        scope: "Namespaced"
+    clientConfig:
+      url: https://{{ $name }}-webhook.{{ $namespace }}.svc:443/validate-overridepolicy
+      {{- include "karmada.webhook.caBundle" . | nindent 6 }}
+    failurePolicy: Fail
+    sideEffects: None
+    admissionReviewVersions: ["v1"]
+    timeoutSeconds: 3
   - name: config.karmada.io
     rules:
       - operations: ["CREATE", "UPDATE"]

cmd/webhook/app/webhook.go

@@ -89,6 +89,7 @@ func Run(ctx context.Context, opts *options.Options) error {
 	hookServer.Register("/mutate-clusterpropagationpolicy", &webhook.Admission{Handler: &clusterpropagationpolicy.MutatingAdmission{}})
 	hookServer.Register("/validate-clusterpropagationpolicy", &webhook.Admission{Handler: &clusterpropagationpolicy.ValidatingAdmission{}})
 	hookServer.Register("/mutate-overridepolicy", &webhook.Admission{Handler: &overridepolicy.MutatingAdmission{}})
+	hookServer.Register("/validate-overridepolicy", &webhook.Admission{Handler: &overridepolicy.ValidatingAdmission{}})
 	hookServer.Register("/mutate-work", &webhook.Admission{Handler: &work.MutatingAdmission{}})
 	hookServer.Register("/convert", &conversion.Webhook{})
 	hookServer.Register("/validate-resourceinterpreterwebhookconfiguration", &webhook.Admission{Handler: &configuration.ValidatingAdmission{}})

pkg/util/validation/validation.go

@@ -74,3 +74,37 @@ func ValidatePolicyFieldSelector(fieldSelector *policyv1alpha1.FieldSelector) er
 
 	return nil
 }
+
+// ValidateOverrideSpec tests if the overrideRules and (overriders or targetCluster) co-exist
+func ValidateOverrideSpec(overrideSpec *policyv1alpha1.OverrideSpec) error {
+	if overrideSpec == nil {
+		return nil
+	}
+
+	if overrideSpec.OverrideRules == nil {
+		return nil
+	}
+
+	if overrideSpec.TargetCluster != nil || !EmptyOverrides(overrideSpec.Overriders) {
+		return fmt.Errorf("overrideRules and (overriders or targetCluster) can't co-exist")
+	}
+
+	return nil
+}
+
+// EmptyOverrides check if the overriders of override policy is empty
+func EmptyOverrides(overriders policyv1alpha1.Overriders) bool {
+	if len(overriders.Plaintext) != 0 {
+		return false
+	}
+	if len(overriders.ImageOverrider) != 0 {
+		return false
+	}
+	if len(overriders.CommandOverrider) != 0 {
+		return false
+	}
+	if len(overriders.ArgsOverrider) != 0 {
+		return false
+	}
+	return true
+}

pkg/util/validation/validation_test.go

@@ -3,6 +3,8 @@ package validation
 import (
 	"strings"
 	"testing"
+
+	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
 )
 
 func TestValidateClusterName(t *testing.T) {
@@ -93,3 +95,111 @@ func TestValidateClusterProxyURL(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateOverrideSpec(t *testing.T) {
+	var tests = []struct {
+		name         string
+		overrideSpec policyv1alpha1.OverrideSpec
+		expectError  bool
+	}{
+		{
+			name: "overrideRules is set, overriders and targetCluster aren't set",
+			overrideSpec: policyv1alpha1.OverrideSpec{
+				OverrideRules: []policyv1alpha1.RuleWithCluster{
+					{
+						TargetCluster: &policyv1alpha1.ClusterAffinity{
+							ClusterNames: []string{"cluster-name"},
+						},
+					},
+				},
+			},
+			expectError: false,
+		},
+		{
+			name: "overriders and targetCluster are set, overrideRules isn't set",
+			overrideSpec: policyv1alpha1.OverrideSpec{
+				TargetCluster: &policyv1alpha1.ClusterAffinity{
+					ClusterNames: []string{"cluster-name"},
+				},
+				Overriders: policyv1alpha1.Overriders{
+					Plaintext: []policyv1alpha1.PlaintextOverrider{
+						{
+							Path: "spec/image",
+						},
+					},
+				},
+			},
+			expectError: false,
+		},
+		{
+			name: "overrideRules and targetCluster can't co-exist",
+			overrideSpec: policyv1alpha1.OverrideSpec{
+				OverrideRules: []policyv1alpha1.RuleWithCluster{
+					{
+						TargetCluster: &policyv1alpha1.ClusterAffinity{
+							ClusterNames: []string{"cluster-name"},
+						},
+					},
+				},
+				TargetCluster: &policyv1alpha1.ClusterAffinity{
+					ClusterNames: []string{"cluster-name"},
+				},
+			},
+			expectError: true,
+		},
+		{
+			name: "overrideRules and overriders can't co-exist",
+			overrideSpec: policyv1alpha1.OverrideSpec{
+				OverrideRules: []policyv1alpha1.RuleWithCluster{
+					{
+						TargetCluster: &policyv1alpha1.ClusterAffinity{
+							ClusterNames: []string{"cluster-name"},
+						},
+					},
+				},
+				Overriders: policyv1alpha1.Overriders{
+					Plaintext: []policyv1alpha1.PlaintextOverrider{
+						{
+							Path: "spec/image",
+						},
+					},
+				},
+			},
+			expectError: true,
+		},
+		{
+			name: "overrideRules, targetCluster and overriders can't co-exist",
+			overrideSpec: policyv1alpha1.OverrideSpec{
+				OverrideRules: []policyv1alpha1.RuleWithCluster{
+					{
+						TargetCluster: &policyv1alpha1.ClusterAffinity{
+							ClusterNames: []string{"cluster-name"},
+						},
+					},
+				},
+				TargetCluster: &policyv1alpha1.ClusterAffinity{
+					ClusterNames: []string{"cluster-name"},
+				},
+				Overriders: policyv1alpha1.Overriders{
+					Plaintext: []policyv1alpha1.PlaintextOverrider{
+						{
+							Path: "spec/image",
+						},
+					},
+				},
+			},
+			expectError: true,
+		},
+	}
+
+	for _, test := range tests {
+		tc := test
+		err := ValidateOverrideSpec(&tc.overrideSpec)
+		if err != nil && tc.expectError != true {
+			t.Fatalf("expect no error but got: %v", err)
+		}
+		if err == nil && tc.expectError == true {
+			t.Fatalf("expect an error but got none")
+		}
+	}
+}

pkg/webhook/overridepolicy/validating.go

@@ -0,0 +1,47 @@
+package overridepolicy
+
+import (
+	"context"
+	"net/http"
+
+	"k8s.io/klog/v2"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
+	"github.com/karmada-io/karmada/pkg/util/validation"
+)
+
+// ValidatingAdmission validates PropagationPolicy object when creating/updating/deleting.
+type ValidatingAdmission struct {
+	decoder *admission.Decoder
+}
+
+// Check if our ValidatingAdmission implements necessary interface
+var _ admission.Handler = &ValidatingAdmission{}
+var _ admission.DecoderInjector = &ValidatingAdmission{}
+
+// Handle implements admission.Handler interface.
+// It yields a response to an AdmissionRequest.
+func (v *ValidatingAdmission) Handle(ctx context.Context, req admission.Request) admission.Response {
+	policy := &policyv1alpha1.OverridePolicy{}
+
+	err := v.decoder.Decode(req, policy)
+	if err != nil {
+		return admission.Errored(http.StatusBadRequest, err)
+	}
+	klog.V(2).Infof("Validating OverridePolicy(%s/%s for request: %s", policy.Namespace, policy.Name, req.Operation)
+
+	if err := validation.ValidateOverrideSpec(&policy.Spec); err != nil {
+		klog.Error(err)
+		return admission.Denied(err.Error())
+	}
+
+	return admission.Allowed("")
+}
+
+// InjectDecoder implements admission.DecoderInjector interface.
+// A decoder will be automatically injected.
+func (v *ValidatingAdmission) InjectDecoder(d *admission.Decoder) error {
+	v.decoder = d
+	return nil
+}