Skip to content

Commit

Permalink
Restrict reported overlaps to enforced policies
Browse files Browse the repository at this point in the history 
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 <jkulikau@redhat.com>
  • Loading branch information
@JustinKuli @openshift-merge-bot
JustinKuli authored and openshift-merge-bot[bot] committed Jun 18, 2024
1 parent 3351576 commit 9a2ada5
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 37 deletions.
10 changes: 7 additions & 3 deletions controllers/operatorpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
Expand All @@ -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) {
Expand Down
92 changes: 58 additions & 34 deletions test/e2e/case38_install_operator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
)
})

Expand Down Expand Up @@ -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`,
)
})

Expand All @@ -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,
Expand All @@ -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",
Expand Down Expand Up @@ -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 (
Expand Down

0 comments on commit 9a2ada5

Please sign in to comment.