Update the webhook to validate the remediationAction field of policies

Currently, when the remediationAction field is unset in both the root policy and configuration policy in its policy template, the violation is generated as an event by the config-policy controller in the managed cluster. This PR addresses the issue by updating the validating webhook so that the violation can be caught on policy creation and updates on the hub. ref: https://issues.redhat.com/browse/ACM-7551 Signed-off-by: Jason Zhang <[email protected]> (cherry picked from commit 5adbb99)
stolostron · Oct 25, 2023 · 135c7fe · 135c7fe
1 parent 0ecd13b
commit 135c7fe
Show file tree

Hide file tree

Showing 6 changed files with 242 additions and 36 deletions.
diff --git a/api/v1/policy_webhook.go b/api/v1/policy_webhook.go
@@ -4,9 +4,11 @@
 package v1
 
 import (
+	"encoding/json"
 	"errors"
 	"unicode/utf8"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -19,6 +21,8 @@ var (
 	policylog = logf.Log.WithName("policy-validating-webhook")
 	errName   = errors.New("the combined length of the policy namespace and name " +
 		"cannot exceed 62 characters")
+	errRemediation = errors.New("RemediationAction field of the policy and " +
+		"policy template cannot both be unset")
 )
 
 func (r *Policy) SetupWebhookWithManager(mgr ctrl.Manager) error {
@@ -35,11 +39,28 @@ var _ webhook.Validator = &Policy{}
 func (r *Policy) ValidateCreate() (admission.Warnings, error) {
 	policylog.Info("Validate policy creation request", "name", r.Name)
 
-	return r.validateName()
+	err := r.validateName()
+	if err != nil {
+		return nil, err
+	}
+
+	err = r.validateRemediationAction()
+	if err != nil {
+		return nil, err
+	}
+
+	return nil, nil
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
 func (r *Policy) ValidateUpdate(_ runtime.Object) (admission.Warnings, error) {
+	policylog.Info("Validate policy update request", "name", r.Name)
+
+	err := r.validateRemediationAction()
+	if err != nil {
+		return nil, err
+	}
+
 	return nil, nil
 }
 
@@ -49,18 +70,43 @@ func (r *Policy) ValidateDelete() (admission.Warnings, error) {
 }
 
 // validate the policy name and namespace length
-func (r *Policy) validateName() (admission.Warnings, error) {
+func (r *Policy) validateName() error {
 	policylog.Info("Validating the policy name through a validating webhook")
 
 	// replicated policies don't need pass this validation
 	if _, ok := r.GetLabels()["policy.open-cluster-management.io/root-policy"]; ok {
-		return nil, nil
+		return nil
 	}
 
 	// 1 character for "."
 	if (utf8.RuneCountInString(r.Name) + utf8.RuneCountInString(r.Namespace)) > 62 {
-		return nil, errName
+		return errName
 	}
 
-	return nil, nil
+	return nil
+}
+
+// validate the remediationAction field of the root policy and its policy templates
+func (r *Policy) validateRemediationAction() error {
+	policylog.Info("Validating the Policy and ConfigurationPolicy remediationAction through a validating webhook")
+
+	if r.Spec.RemediationAction != "" {
+		return nil
+	}
+
+	plcTemplates := r.Spec.PolicyTemplates
+
+	for _, obj := range plcTemplates {
+		objUnstruct := &unstructured.Unstructured{}
+		_ = json.Unmarshal(obj.ObjectDefinition.Raw, objUnstruct)
+
+		if objUnstruct.GroupVersionKind().Kind == "ConfigurationPolicy" {
+			_, found, _ := unstructured.NestedString(objUnstruct.Object, "spec", "remediationAction")
+			if !found {
+				return errRemediation
+			}
+		}
+	}
+
+	return nil
 }
diff --git a/deploy/webhook.yaml b/deploy/webhook.yaml
@@ -56,6 +56,7 @@ webhooks:
     - v1
     operations:
     - CREATE
+    - UPDATE
     resources:
     - policies
   sideEffects: None
diff --git a/test/e2e/case17_policy_webhook_test.go b/test/e2e/case17_policy_webhook_test.go
@@ -15,45 +15,70 @@ import (
 )
 
 const (
-	case17PolicyLongYaml       string = "../resources/case17_policy_webhook/case17_policy_long.yaml"
-	case17PolicyReplicatedYaml string = "../resources/case17_policy_webhook/" +
-		"case17_policy_replicate.yaml"
-	longNamesapce              string = "long-long-long-long-long-long-long"
-	case17PolicyReplicatedName string = "case17-test-policy-replicated-longlong"
-	errMsg                     string = `admission webhook "policy.open-cluster-management.io.webhook" denied the ` +
-		`request: the combined length of the policy namespace and name ` +
-		`cannot exceed 62 characters`
+	case17Prefix                     string = "../resources/case17_policy_webhook/"
+	case17PolicyLongYaml             string = case17Prefix + "case17_policy_long.yaml"
+	case17PolicyReplicatedYaml       string = case17Prefix + "case17_policy_replicate.yaml"
+	case17PolicyRemediationYaml      string = case17Prefix + "case17_invalid_remediation_policy.yaml"
+	case17PolicyRootRemediationYaml  string = case17Prefix + "case17_valid_remediation_policy_root.yaml"
+	case17PolicyCfplcRemediationYaml string = case17Prefix + "case17_valid_remediation_policy_cfplc.yaml"
+	longNamespace                    string = "long-long-long-long-long-long-long"
+	case17PolicyReplicatedName       string = "case17-test-policy-replicated-longlong"
+	case17PolicyReplicatedPlr        string = "case17-test-policy-replicated-longlong-plr"
+	case17PolicyReplicatedPb         string = "case17-test-policy-replicated-longlong-pb"
+	case17PolicyRemediationName      string = "case17-test-policy-no-remediation"
+	case17PolicyRootRemediationName  string = "case17-test-policy-root-remediation"
+	case17PolicyCfplcRemediationName string = "case17-test-policy-cfplc-remediation"
+	errPrefix                        string = `admission webhook "policy.open-cluster-management.io.webhook" ` +
+		`denied the request: `
+	combinedLengthErr string = errPrefix + "the combined length of the policy namespace and name " +
+		"cannot exceed 62 characters"
+	remediationErr string = errPrefix + "RemediationAction field of the policy and policy template " +
+		"cannot both be unset"
 )
 
 var _ = Describe("Test policy webhook", Label("webhook"), Ordered, func() {
-	BeforeAll(func() {
-		_, err := utils.KubectlWithOutput("create",
-			"ns", longNamesapce,
-		)
-		Expect(err).ShouldNot(HaveOccurred())
-	})
-	AfterAll(func() {
-		// cleanup
-		_, err := utils.KubectlWithOutput("delete",
-			"ns", longNamesapce,
-			"--ignore-not-found",
-		)
-		Expect(err).ShouldNot(HaveOccurred())
-
-		_, err = utils.KubectlWithOutput("delete",
-			"policy", case17PolicyReplicatedName,
-			"-n", testNamespace,
-			"--ignore-not-found",
-		)
-		Expect(err).ShouldNot(HaveOccurred())
-	})
 	Describe("Test name + namespace over 63", func() {
+		BeforeAll(func() {
+			_, err := utils.KubectlWithOutput("create",
+				"ns", longNamespace,
+			)
+			Expect(err).ShouldNot(HaveOccurred())
+		})
+		AfterAll(func() {
+			// cleanup
+			_, err := utils.KubectlWithOutput("delete",
+				"ns", longNamespace,
+				"--ignore-not-found",
+			)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			_, err = utils.KubectlWithOutput("delete",
+				"policy", case17PolicyReplicatedName,
+				"-n", testNamespace,
+				"--ignore-not-found",
+			)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			_, err = utils.KubectlWithOutput("delete",
+				"placementrule", case17PolicyReplicatedPlr,
+				"-n", testNamespace,
+				"--ignore-not-found",
+			)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			_, err = utils.KubectlWithOutput("delete",
+				"placementbinding", case17PolicyReplicatedPb,
+				"-n", testNamespace,
+				"--ignore-not-found",
+			)
+			Expect(err).ShouldNot(HaveOccurred())
+		})
 		It("Should the error message is presented", func() {
 			output, err := utils.KubectlWithOutput("apply",
 				"-f", case17PolicyLongYaml,
-				"-n", longNamesapce)
+				"-n", longNamespace)
 			Expect(err).Should(HaveOccurred())
-			Expect(output).Should(ContainSubstring(errMsg))
+			Expect(output).Should(ContainSubstring(combinedLengthErr))
 		})
 		It("Should replicated policy should not be validated", func() {
 			_, err := utils.KubectlWithOutput("apply",
@@ -82,4 +107,79 @@ var _ = Describe("Test policy webhook", Label("webhook"), Ordered, func() {
 			Expect(plc).ToNot(BeNil())
 		})
 	})
+
+	Describe("The remediationAction field should not be unset in both root policy and policy templates", func() {
+		AfterAll(func() {
+			_, err := utils.KubectlWithOutput("delete",
+				"policy", case17PolicyRemediationName,
+				"-n", testNamespace,
+				"--ignore-not-found",
+			)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			_, err = utils.KubectlWithOutput("delete",
+				"policy", case17PolicyRootRemediationName,
+				"-n", testNamespace,
+				"--ignore-not-found",
+			)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			_, err = utils.KubectlWithOutput("delete",
+				"policy", case17PolicyCfplcRemediationName,
+				"-n", testNamespace,
+				"--ignore-not-found",
+			)
+			Expect(err).ShouldNot(HaveOccurred())
+		})
+
+		It("Should return a validation error when creating policies", func() {
+			By("Applying a policy where both the remediationAction of the root policy " +
+				"and the configuration policy in the policy templates are unset")
+			output, err := utils.KubectlWithOutput("apply",
+				"-f", case17PolicyRemediationYaml,
+				"-n", testNamespace)
+			Expect(err).Should(HaveOccurred())
+			Expect(output).Should(ContainSubstring(remediationErr))
+		})
+
+		It("Should not return a validation error when creating policies", func() {
+			By("Applying a policy where only the remediationAction of the " +
+				"root policy is set")
+			_, err := utils.KubectlWithOutput("apply",
+				"-f", case17PolicyRootRemediationYaml,
+				"-n", testNamespace)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			By("Applying a policy where only the remediationAction of the " +
+				"configuration policy in the policy templates is set")
+			_, err = utils.KubectlWithOutput("apply",
+				"-f", case17PolicyCfplcRemediationYaml,
+				"-n", testNamespace)
+			Expect(err).ShouldNot(HaveOccurred())
+		})
+
+		It("Should return a validation error when updating policies", func() {
+			By("Patching a policy so that the remediationAction field of both " +
+				"the root policy and configuration policy is unset")
+
+			output, err := utils.KubectlWithOutput("patch", "policy",
+				case17PolicyRootRemediationName, "-n", testNamespace,
+				"--type=json", "-p", "[{'op': 'remove', 'path': '/spec/remediationAction'}]",
+			)
+
+			Expect(err).Should(HaveOccurred())
+			Expect(output).Should(ContainSubstring(remediationErr))
+		})
+
+		It("Should not return a validation error when updating policies", func() {
+			By("Patching a policy so that only the remediationAction field of the root policy is unset")
+
+			_, err := utils.KubectlWithOutput("patch", "policy",
+				case17PolicyRootRemediationName, "-n", testNamespace, "--type=json", "-p",
+				"[{'op': 'add', 'path': '/spec/remediationAction', 'value': 'inform'}]",
+			)
+
+			Expect(err).ShouldNot(HaveOccurred())
+		})
+	})
 })
diff --git a/test/resources/case17_policy_webhook/case17_invalid_remediation_policy.yaml b/test/resources/case17_policy_webhook/case17_invalid_remediation_policy.yaml
@@ -0,0 +1,19 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: Policy
+metadata:
+  name: case17-test-policy-no-remediation
+spec:
+  disabled: false
+  policy-templates:
+    - objectDefinition:
+        apiVersion: policy.open-cluster-management.io/v1
+        kind: ConfigurationPolicy
+        metadata:
+          name: case17-cfplc1
+        spec:
+          severity: low
+          namespaceSelector:
+            exclude:
+              - kube-*
+            include:
+              - default
diff --git a/test/resources/case17_policy_webhook/case17_valid_remediation_policy_cfplc.yaml b/test/resources/case17_policy_webhook/case17_valid_remediation_policy_cfplc.yaml
@@ -0,0 +1,20 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: Policy
+metadata:
+  name: case17-test-policy-cfplc-remediation
+spec:
+  disabled: false
+  policy-templates:
+    - objectDefinition:
+        apiVersion: policy.open-cluster-management.io/v1
+        kind: ConfigurationPolicy
+        metadata:
+          name: case17-cfplc2
+        spec:
+          remediationAction: inform
+          severity: low
+          namespaceSelector:
+            exclude:
+              - kube-*
+            include:
+              - default
diff --git a/test/resources/case17_policy_webhook/case17_valid_remediation_policy_root.yaml b/test/resources/case17_policy_webhook/case17_valid_remediation_policy_root.yaml
@@ -0,0 +1,20 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: Policy
+metadata:
+  name: case17-test-policy-root-remediation
+spec:
+  remediationAction: inform
+  disabled: false
+  policy-templates:
+    - objectDefinition:
+        apiVersion: policy.open-cluster-management.io/v1
+        kind: ConfigurationPolicy
+        metadata:
+          name: case17-cfplc3
+        spec:
+          severity: low
+          namespaceSelector:
+            exclude:
+              - kube-*
+            include:
+              - default