From 0e74faf8d0a5b0a95d94199a6b52cf9ba35fb6ae Mon Sep 17 00:00:00 2001 From: Jeffrey Luo Date: Mon, 25 Mar 2024 16:15:22 -0400 Subject: [PATCH] Fix constraints not satisfiable message causing compliance flood ConstraintsNotSatisfiable message types in the subscription status are formatted "constraints not satisfiable: clause1, clause2, clause3 ...". However, the order of the clauses is nondeterministic, which causes compliance event flapping as each update leads to a different ordering of messages, which in turn triggers another update. This was fixed by sorting the message prior to evaluation. Ref: https://issues.redhat.com/browse/ACM-10204 Signed-off-by: Jeffrey Luo --- controllers/operatorpolicy_controller.go | 57 ++++++++++++++---- controllers/operatorpolicy_controller_test.go | 59 +++++++++++++++++++ controllers/operatorpolicy_status.go | 16 +++++ 3 files changed, 122 insertions(+), 10 deletions(-) diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 1e01b80c..65fa23f0 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -11,6 +11,8 @@ import ( "fmt" "reflect" "regexp" + "slices" + "strings" operatorv1 "github.com/operator-framework/api/pkg/operators/v1" operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -653,19 +655,17 @@ func (r *OperatorPolicyReconciler) handleSubscription( } if includesSubscription { - cond := metav1.Condition{ - Type: subConditionType, - Status: metav1.ConditionFalse, - Reason: subResFailed.Reason, - Message: subResFailed.Message, + // a "constraints not satisfiable" message has nondeterministic clauses, and thus + // need to be sorted in order to check that they are not duplicates of the current message. + if constraintMessageMatch(policy, &subResFailed) { + return mergedSub, nil, false, nil } - if subResFailed.LastTransitionTime != nil { - cond.LastTransitionTime = *subResFailed.LastTransitionTime - } - - return mergedSub, nil, updateStatus(policy, cond, nonCompObj(foundSub, subResFailed.Reason)), nil + return mergedSub, nil, updateStatus( + policy, subResFailedCond(subResFailed), nonCompObj(foundSub, subResFailed.Reason)), nil } + + return mergedSub, nil, false, nil } return mergedSub, nil, updateStatus(policy, matchesCond("Subscription"), matchedObj(foundSub)), nil @@ -727,6 +727,43 @@ func messageIncludesSubscription(subscription *operatorv1alpha1.Subscription, me return regexp.MatchString(regex, message) } +// constraintMessageMatch checks if the ConstraintsNotSatisfiable message is actually different +// from the old one by sorting the clauses of the message +func constraintMessageMatch(policy *policyv1beta1.OperatorPolicy, cond *operatorv1alpha1.SubscriptionCondition) bool { + const cnfPrefix = "constraints not satisfiable: " + + var policyMessage, subMessage string + + for _, cond := range policy.Status.Conditions { + if strings.Contains(cond.Message, cnfPrefix) { + policyMessage = cond.Message + } + } + + if policyMessage == "" { + return false + } + + if strings.Contains(cond.Message, cnfPrefix) { + subMessage = cond.Message + } else { + return false + } + + policyMessage = strings.TrimPrefix(policyMessage, cnfPrefix) + subMessage = strings.TrimPrefix(subMessage, cnfPrefix) + + // The ConstraintsNotSatisfiable message is always formatted as follows: + // constraints not satisfiable: clause1, clause2, clause3 ... + policyMessageSlice := strings.Split(policyMessage, ", ") + slices.Sort(policyMessageSlice) + + subMessageSlice := strings.Split(subMessage, ", ") + slices.Sort(subMessageSlice) + + return reflect.DeepEqual(policyMessageSlice, subMessageSlice) +} + func (r *OperatorPolicyReconciler) handleInstallPlan( ctx context.Context, policy *policyv1beta1.OperatorPolicy, sub *operatorv1alpha1.Subscription, ) (bool, error) { diff --git a/controllers/operatorpolicy_controller_test.go b/controllers/operatorpolicy_controller_test.go index c183d838..55ef19bb 100644 --- a/controllers/operatorpolicy_controller_test.go +++ b/controllers/operatorpolicy_controller_test.go @@ -199,3 +199,62 @@ func TestMessageIncludesSubscription(t *testing.T) { ) } } + +func TestMessageContentOrderMatching(t *testing.T) { + t.Parallel() + + testPolicy := &policyv1beta1.OperatorPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "default", + }, + Spec: policyv1beta1.OperatorPolicySpec{ + Severity: "low", + RemediationAction: "enforce", + ComplianceType: "musthave", + Subscription: runtime.RawExtension{ + Raw: []byte(`{ + "source": "my-catalog", + "sourceNamespace": "my-ns", + "name": "my-operator", + "channel": "stable", + "startingCSV": "my-operator-v1", + "installPlanApproval": "Automatic" + }`), + }, + }, + Status: policyv1beta1.OperatorPolicyStatus{ + ComplianceState: "NonCompliant", + Conditions: []metav1.Condition{ + { + Type: "SubscriptionCompliant", + Status: "False", + ObservedGeneration: 0, + LastTransitionTime: metav1.Now(), + Reason: "ConstraintsNotSatisfiable", + Message: "constraints not satisfiable: " + + "no operators found in package gatekeeper-operator-product " + + "in the catalog referenced by subscription gatekeeper-operator-product, " + + "clusterserviceversion gatekeeper-operator-product.v3.11.1 exists " + + "and is not referenced by a subscription, " + + "subscription gatekeeper-operator-product4 exists", + }, + }, + }, + } + + testCond := &operatorv1alpha1.SubscriptionCondition{ + Type: "ResolutionFailed", + Status: "True", + Reason: "ConstraintsNotSatisfiable", + Message: "constraints not satisfiable: " + + "subscription gatekeeper-operator-product4 exists, " + + "no operators found in package gatekeeper-operator-product " + + "in the catalog referenced by subscription gatekeeper-operator-product, " + + "clusterserviceversion gatekeeper-operator-product.v3.11.1 exists " + + "and is not referenced by a subscription", + } + + ret := constraintMessageMatch(testPolicy, testCond) + assert.Equal(t, true, ret) +} diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index 6d1f70f8..20ed84f3 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -473,6 +473,22 @@ func validationCond(validationErrors []error) metav1.Condition { } } +// subResFailedCond takes a failed SubscriptionCondition and converts it to a generic Condition +func subResFailedCond(subFailedCond operatorv1alpha1.SubscriptionCondition) metav1.Condition { + cond := metav1.Condition{ + Type: subConditionType, + Status: metav1.ConditionFalse, + Reason: subFailedCond.Reason, + Message: subFailedCond.Message, + } + + if subFailedCond.LastTransitionTime != nil { + cond.LastTransitionTime = *subFailedCond.LastTransitionTime + } + + return cond +} + // opGroupPreexistingCond is a Compliant condition with Reason 'PreexistingOperatorGroupFound', // and Message 'the policy does not specify an OperatorGroup but one already exists in the // namespace - assuming that OperatorGroup is correct'