From 721ef70d6b0cfaf5c87402df3a6bf3c86fc7514f Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Tue, 18 Jun 2024 13:18:24 -0400 Subject: [PATCH] Restrict reported overlaps to enforced policies Previously, informed OperatorPolicies could report that they were overlapping, but in those cases it is not necessary to prevent the policies from operating normally. Now, only overlapping *enforced* policies will be considered invalid. Refs: - https://issues.redhat.com/browse/ACM-12207 Signed-off-by: Justin Kulikauskas (cherry picked from commit 9a2ada5f70b50f951fdf049c6395dca78729f49f) --- controllers/operatorpolicy_controller.go | 10 ++- test/e2e/case38_install_operator_test.go | 92 +++++++++++++++--------- 2 files changed, 65 insertions(+), 37 deletions(-) diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 3a3a75eb..28ef5637 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -476,8 +476,8 @@ func (r *OperatorPolicyReconciler) checkSubOverlap( statusChanged = true } - if resolvedSubLabel == "" { - // No possible overlap if the subscription could not be determined + // No overlap if the subscription could not be determined or if the policy is in inform mode + if resolvedSubLabel == "" || policy.Spec.RemediationAction.IsInform() { if len(policy.Status.OverlappingPolicies) != 0 { policy.Status.OverlappingPolicies = []string{} statusChanged = true @@ -504,6 +504,10 @@ func (r *OperatorPolicyReconciler) checkSubOverlap( for _, otherPolicy := range opList.Items { if otherPolicy.Status.ResolvedSubscriptionLabel == resolvedSubLabel { + if otherPolicy.Spec.RemediationAction.IsInform() { + continue + } + if !(otherPolicy.Name == policy.Name && otherPolicy.Namespace == policy.Namespace) { overlappers = append(overlappers, otherPolicy.Name+"."+otherPolicy.Namespace) } @@ -522,7 +526,7 @@ func (r *OperatorPolicyReconciler) checkSubOverlap( slices.Sort(overlappers) - overlapError := fmt.Errorf("the specified operator is managed by multiple policies (%v)", + overlapError := fmt.Errorf("the specified operator is managed by multiple enforced policies (%v)", strings.Join(overlappers, ", ")) if !slices.Equal(overlappers, policy.Status.OverlappingPolicies) { diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 3cf97e0b..dafe86ee 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -3120,32 +3120,30 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu mustnothaveYAML, testNamespace, gvrPolicy, gvrOperatorPolicy) }) - It("Should display a validation error in both", func() { + It("Should not display a validation error when both are in inform mode", func() { check( mustnothaveName, - true, + false, []policyv1.RelatedObject{}, metav1.Condition{ - Type: "ValidPolicySpec", - Status: metav1.ConditionFalse, - Reason: "InvalidPolicySpec", - Message: `the specified operator is managed by multiple policies ` + - `(oppol-mustnothave.` + testNamespace + `, oppol-no-group.` + testNamespace + `)`, + Type: "ValidPolicySpec", + Status: metav1.ConditionTrue, + Reason: "PolicyValidated", + Message: `the policy spec is valid`, }, - `the specified operator is managed by multiple policies`, + `the policy spec is valid`, ) check( musthaveName, - true, + false, []policyv1.RelatedObject{}, metav1.Condition{ - Type: "ValidPolicySpec", - Status: metav1.ConditionFalse, - Reason: "InvalidPolicySpec", - Message: `the specified operator is managed by multiple policies ` + - `(oppol-mustnothave.` + testNamespace + `, oppol-no-group.` + testNamespace + `)`, + Type: "ValidPolicySpec", + Status: metav1.ConditionTrue, + Reason: "PolicyValidated", + Message: `the policy spec is valid`, }, - `the specified operator is managed by multiple policies`, + `the policy spec is valid`, ) }) @@ -3181,20 +3179,38 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu }, "10s", "1s").Should(Equal(totalReconciles)) }) - It("Should remove the validation error when the overlapping policy is removed", func() { - utils.Kubectl("delete", "operatorpolicy", musthaveName, "-n", testNamespace) + It("Should display a validation error when both are enforced", func() { + // enforce the policies + utils.Kubectl("patch", "operatorpolicy", mustnothaveName, "-n", testNamespace, "--type=json", "-p", + `[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`) + utils.Kubectl("patch", "operatorpolicy", musthaveName, "-n", testNamespace, "--type=json", "-p", + `[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`) check( mustnothaveName, - false, + true, []policyv1.RelatedObject{}, metav1.Condition{ - Type: "ValidPolicySpec", - Status: metav1.ConditionTrue, - Reason: "PolicyValidated", - Message: `the policy spec is valid`, + Type: "ValidPolicySpec", + Status: metav1.ConditionFalse, + Reason: "InvalidPolicySpec", + Message: `the specified operator is managed by multiple enforced policies ` + + `(oppol-mustnothave.` + testNamespace + `, oppol-no-group.` + testNamespace + `)`, }, - `the policy spec is valid`, + `the specified operator is managed by multiple enforced policies`, + ) + check( + musthaveName, + true, + []policyv1.RelatedObject{}, + metav1.Condition{ + Type: "ValidPolicySpec", + Status: metav1.ConditionFalse, + Reason: "InvalidPolicySpec", + Message: `the specified operator is managed by multiple enforced policies ` + + `(oppol-mustnothave.` + testNamespace + `, oppol-no-group.` + testNamespace + `)`, + }, + `the specified operator is managed by multiple enforced policies`, ) }) @@ -3203,15 +3219,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu // which would slow down the suite. As long as no other test files use OperatorPolicy, the // Ordered property on this file should ensure this is stable. It("Should not cause an infinite reconcile loop when enforced", func() { - createObjWithParent(parentPolicyYAML, parentPolicyName, - musthaveYAML, testNamespace, gvrPolicy, gvrOperatorPolicy) - - // enforce the policies - utils.Kubectl("patch", "operatorpolicy", mustnothaveName, "-n", testNamespace, "--type=json", "-p", - `[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`) - utils.Kubectl("patch", "operatorpolicy", musthaveName, "-n", testNamespace, "--type=json", "-p", - `[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`) - check( mustnothaveName, true, @@ -3220,10 +3227,10 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu Type: "ValidPolicySpec", Status: metav1.ConditionFalse, Reason: "InvalidPolicySpec", - Message: `the specified operator is managed by multiple policies ` + + Message: `the specified operator is managed by multiple enforced policies ` + `(oppol-mustnothave.` + testNamespace + `, oppol-no-group.` + testNamespace + `)`, }, - `the specified operator is managed by multiple policies`, + `the specified operator is managed by multiple enforced policies`, ) recMetrics := utils.GetMetrics("controller_runtime_reconcile_total", @@ -3252,6 +3259,23 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu return loopReconciles }, "10s", "1s").Should(Equal(totalReconciles)) }) + + It("Should remove the validation error when an overlapping policy is removed", func() { + utils.Kubectl("delete", "operatorpolicy", musthaveName, "-n", testNamespace) + + check( + mustnothaveName, + false, + []policyv1.RelatedObject{}, + metav1.Condition{ + Type: "ValidPolicySpec", + Status: metav1.ConditionTrue, + Reason: "PolicyValidated", + Message: `the policy spec is valid`, + }, + `the policy spec is valid`, + ) + }) }) Describe("Testing templates in an OperatorPolicy", Ordered, func() { const (