Skip to content

Commit

Permalink
Prevent subscription creation if opgroup incorrect
Browse files Browse the repository at this point in the history 
Previously, the subscription would be created by an enforced musthave
OperatorPolicy regardless of the status of the OperatorGroup. But this
could lead to a bad result where the operator is running in the wrong
mode because it is not using the correct operatorgroup. While the policy
was correctly non-compliant in this case, it should not create the
subscription.

Refs:
 - https://issues.redhat.com/browse/ACM-11507

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
  • Loading branch information
@JustinKuli @openshift-merge-bot
JustinKuli authored and openshift-merge-bot[bot] committed May 9, 2024
1 parent d734b7a commit 414cd18
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 31 deletions.
62 changes: 36 additions & 26 deletions controllers/operatorpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
desiredSubName = desiredSub.Name
}

earlyConds, changed, err := r.handleOpGroup(ctx, policy, desiredOG, desiredSubName)
ogCorrect, earlyConds, changed, err := r.handleOpGroup(ctx, policy, desiredOG, desiredSubName)
earlyComplianceEvents = append(earlyComplianceEvents, earlyConds...)
condChanged = condChanged || changed

Expand All @@ -243,7 +243,7 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
return earlyComplianceEvents, condChanged, err
}

subscription, earlyConds, changed, err := r.handleSubscription(ctx, policy, desiredSub)
subscription, earlyConds, changed, err := r.handleSubscription(ctx, policy, desiredSub, ogCorrect)
earlyComplianceEvents = append(earlyComplianceEvents, earlyConds...)
condChanged = condChanged || changed

Expand Down Expand Up @@ -620,40 +620,43 @@ func (r *OperatorPolicyReconciler) handleOpGroup(
policy *policyv1beta1.OperatorPolicy,
desiredOpGroup *operatorv1.OperatorGroup,
desiredSubName string,
) ([]metav1.Condition, bool, error) {
) (bool, []metav1.Condition, bool, error) {
watcher := opPolIdentifier(policy.Namespace, policy.Name)

if desiredOpGroup == nil || desiredOpGroup.Namespace == "" {
// Note: existing related objects will not be removed by this status update
return nil, updateStatus(policy, invalidCausingUnknownCond("OperatorGroup")), nil
return false, nil, updateStatus(policy, invalidCausingUnknownCond("OperatorGroup")), nil
}

foundOpGroups, err := r.DynamicWatcher.List(
watcher, operatorGroupGVK, desiredOpGroup.Namespace, labels.Everything())
if err != nil {
return nil, false, fmt.Errorf("error listing OperatorGroups: %w", err)
return false, nil, false, fmt.Errorf("error listing OperatorGroups: %w", err)
}

if policy.Spec.ComplianceType.IsMustHave() {
return r.musthaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups)
}

return r.mustnothaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups, desiredSubName)
earlyConds, changed, err := r.mustnothaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups, desiredSubName)

// In mustnothave mode, we can always act as if the OperatorGroup is not correct
return false, earlyConds, changed, err
}

func (r *OperatorPolicyReconciler) musthaveOpGroup(
ctx context.Context,
policy *policyv1beta1.OperatorPolicy,
desiredOpGroup *operatorv1.OperatorGroup,
foundOpGroups []unstructured.Unstructured,
) ([]metav1.Condition, bool, error) {
) (bool, []metav1.Condition, bool, error) {
switch len(foundOpGroups) {
case 0:
// Missing OperatorGroup: report NonCompliance
changed := updateStatus(policy, missingWantedCond("OperatorGroup"), missingWantedObj(desiredOpGroup))

if policy.Spec.RemediationAction.IsInform() {
return nil, changed, nil
return false, nil, changed, nil
}

earlyConds := []metav1.Condition{}
Expand All @@ -664,15 +667,15 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(

err := r.createWithNamespace(ctx, policy, desiredOpGroup)
if err != nil {
return nil, changed, fmt.Errorf("error creating the OperatorGroup: %w", err)
return false, nil, changed, fmt.Errorf("error creating the OperatorGroup: %w", err)
}

desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Create stripped this information

// Now the OperatorGroup should match, so report Compliance
updateStatus(policy, createdCond("OperatorGroup"), createdObj(desiredOpGroup))

return earlyConds, true, nil
return true, earlyConds, true, nil
case 1:
opGroup := foundOpGroups[0]

Expand All @@ -685,8 +688,8 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
// The policy doesn't specify what the OperatorGroup should look like, but what is already
// there is not the default one the policy would create.
// FUTURE: check if the one operator group is compatible with the desired subscription.
// For an initial implementation, assume if an OperatorGroup already exists, then it's a good one.
return nil, updateStatus(policy, opGroupPreexistingCond, matchedObj(&opGroup)), nil
// For now, assume if an OperatorGroup already exists, then it's a good one.
return true, nil, updateStatus(policy, opGroupPreexistingCond, matchedObj(&opGroup)), nil
}

// There is an OperatorGroup in the namespace that does not match the name of what is in the policy.
Expand All @@ -695,25 +698,25 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
missing := missingWantedObj(desiredOpGroup)
badExisting := mismatchedObj(&opGroup)

return nil, updateStatus(policy, mismatchCond("OperatorGroup"), missing, badExisting), nil
return false, nil, updateStatus(policy, mismatchCond("OperatorGroup"), missing, badExisting), nil
}

// check whether the specs match
desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredOpGroup)
if err != nil {
return nil, false, fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
return false, nil, false, fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
}

updateNeeded, skipUpdate, err := r.mergeObjects(
ctx, desiredUnstruct, &opGroup, string(policy.Spec.ComplianceType),
)
if err != nil {
return nil, false, fmt.Errorf("error checking if the OperatorGroup needs an update: %w", err)
return false, nil, false, fmt.Errorf("error checking if the OperatorGroup needs an update: %w", err)
}

if !updateNeeded {
// Everything relevant matches!
return nil, updateStatus(policy, matchesCond("OperatorGroup"), matchedObj(&opGroup)), nil
return true, nil, updateStatus(policy, matchesCond("OperatorGroup"), matchedObj(&opGroup)), nil
}

// Specs don't match.
Expand All @@ -722,19 +725,21 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
// The policy doesn't specify what the OperatorGroup should look like, but what is already
// there is not the default one the policy would create.
// FUTURE: check if the one operator group is compatible with the desired subscription.
// For an initial implementation, assume if an OperatorGroup already exists, then it's a good one.
return nil, updateStatus(policy, opGroupPreexistingCond, matchedObj(&opGroup)), nil
// For now, assume if an OperatorGroup already exists, then it's a good one.
return true, nil, updateStatus(policy, opGroupPreexistingCond, matchedObj(&opGroup)), nil
}

if policy.Spec.RemediationAction.IsEnforce() && skipUpdate {
return nil, updateStatus(policy, mismatchCondUnfixable("OperatorGroup"), mismatchedObj(&opGroup)), nil
changed := updateStatus(policy, mismatchCondUnfixable("OperatorGroup"), mismatchedObj(&opGroup))

return false, nil, changed, nil
}

// The names match, but the specs don't: report NonCompliance
changed := updateStatus(policy, mismatchCond("OperatorGroup"), mismatchedObj(&opGroup))

if policy.Spec.RemediationAction.IsInform() {
return nil, changed, nil
return false, nil, changed, nil
}

earlyConds := []metav1.Condition{}
Expand All @@ -747,18 +752,18 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(

err = r.Update(ctx, &opGroup)
if err != nil {
return nil, changed, fmt.Errorf("error updating the OperatorGroup: %w", err)
return false, nil, changed, fmt.Errorf("error updating the OperatorGroup: %w", err)
}

desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Update stripped this information

updateStatus(policy, updatedCond("OperatorGroup"), updatedObj(desiredOpGroup))

return earlyConds, true, nil
return true, earlyConds, true, nil
default:
// This situation will always lead to a "TooManyOperatorGroups" failure on the CSV.
// Consider improving this in the future: perhaps this could suggest one of the OperatorGroups to keep.
return nil, updateStatus(policy, opGroupTooManyCond, opGroupTooManyObjs(foundOpGroups)...), nil
return false, nil, updateStatus(policy, opGroupTooManyCond, opGroupTooManyObjs(foundOpGroups)...), nil
}
}

Expand Down Expand Up @@ -912,7 +917,10 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
}

func (r *OperatorPolicyReconciler) handleSubscription(
ctx context.Context, policy *policyv1beta1.OperatorPolicy, desiredSub *operatorv1alpha1.Subscription,
ctx context.Context,
policy *policyv1beta1.OperatorPolicy,
desiredSub *operatorv1alpha1.Subscription,
ogCorrect bool,
) (*operatorv1alpha1.Subscription, []metav1.Condition, bool, error) {
watcher := opPolIdentifier(policy.Namespace, policy.Name)

Expand All @@ -927,7 +935,7 @@ func (r *OperatorPolicyReconciler) handleSubscription(
}

if policy.Spec.ComplianceType.IsMustHave() {
return r.musthaveSubscription(ctx, policy, desiredSub, foundSub)
return r.musthaveSubscription(ctx, policy, desiredSub, foundSub, ogCorrect)
}

return r.mustnothaveSubscription(ctx, policy, desiredSub, foundSub)
Expand All @@ -938,12 +946,14 @@ func (r *OperatorPolicyReconciler) musthaveSubscription(
policy *policyv1beta1.OperatorPolicy,
desiredSub *operatorv1alpha1.Subscription,
foundSub *unstructured.Unstructured,
ogCorrect bool,
) (*operatorv1alpha1.Subscription, []metav1.Condition, bool, error) {
if foundSub == nil {
// Missing Subscription: report NonCompliance
changed := updateStatus(policy, missingWantedCond("Subscription"), missingWantedObj(desiredSub))

if policy.Spec.RemediationAction.IsInform() {
// If informing, or if the OperatorGroup is not correct, don't make any changes
if policy.Spec.RemediationAction.IsInform() || !ogCorrect {
return desiredSub, nil, changed, nil
}

Expand Down
75 changes: 70 additions & 5 deletions test/e2e/case38_install_operator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,9 +590,11 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
})
Describe("Testing Subscription behavior for musthave mode while enforcing", Ordered, func() {
const (
opPolYAML = "../resources/case38_operator_install/operator-policy-no-group.yaml"
opPolName = "oppol-no-group"
subName = "project-quay"
opPolYAML = "../resources/case38_operator_install/operator-policy-with-group.yaml"
opPolName = "oppol-with-group"
subName = "project-quay"
extraOpGroupYAML = "../resources/case38_operator_install/extra-operator-group.yaml"
extraOpGroupName = "extra-operator-group"
)

BeforeAll(func() {
Expand All @@ -601,6 +603,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
utils.Kubectl("delete", "ns", opPolTestNS)
})

utils.Kubectl("apply", "-f", extraOpGroupYAML, "-n", opPolTestNS)

createObjWithParent(parentPolicyYAML, parentPolicyName,
opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
})
Expand Down Expand Up @@ -630,9 +634,70 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
"the Subscription required by the policy was not found",
)
})
It("Should create the Subscription when enforced", func() {
It("Should not create the Subscription when another OperatorGroup already exists", func() {
utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
`[{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"}]`)
check(
opPolName,
true,
[]policyv1.RelatedObject{{
Object: policyv1.ObjectResource{
Kind: "OperatorGroup",
APIVersion: "operators.coreos.com/v1",
Metadata: policyv1.ObjectMetadata{
Name: extraOpGroupName,
Namespace: opPolTestNS,
},
},
Compliant: "NonCompliant",
Reason: "Resource found but does not match",
}, {
Object: policyv1.ObjectResource{
Kind: "OperatorGroup",
APIVersion: "operators.coreos.com/v1",
Metadata: policyv1.ObjectMetadata{
Name: "scoped-operator-group",
Namespace: opPolTestNS,
},
},
Compliant: "NonCompliant",
Reason: "Resource not found but should exist",
}},
metav1.Condition{
Type: "OperatorGroupCompliant",
Status: metav1.ConditionFalse,
Reason: "OperatorGroupMismatch",
Message: "the OperatorGroup found on the cluster does not match the policy",
},
"the OperatorGroup found on the cluster does not match the policy",
)
check(
opPolName,
true,
[]policyv1.RelatedObject{{
Object: policyv1.ObjectResource{
Kind: "Subscription",
APIVersion: "operators.coreos.com/v1alpha1",
Metadata: policyv1.ObjectMetadata{
Name: subName,
Namespace: opPolTestNS,
},
},
Compliant: "NonCompliant",
Reason: "Resource not found but should exist",
}},
metav1.Condition{
Type: "SubscriptionCompliant",
Status: metav1.ConditionFalse,
Reason: "SubscriptionMissing",
Message: "the Subscription required by the policy was not found",
},
"the Subscription required by the policy was not found",
)
})

It("Should create the Subscription after the additional OperatorGroup is removed", func() {
utils.Kubectl("delete", "operatorgroup", extraOpGroupName, "-n", opPolTestNS)
check(
opPolName,
false,
Expand Down Expand Up @@ -2895,7 +2960,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
}, consistentlyDuration, 3, ctx).ShouldNot(BeEmpty())
})
})
Describe("Testing defaulted values in an OperatorPolicy", func() {
Describe("Testing defaulted values of removalBehavior in an OperatorPolicy", func() {
const (
opPolYAML = "../resources/case38_operator_install/operator-policy-no-group.yaml"
opPolName = "oppol-no-group"
Expand Down

0 comments on commit 414cd18

Please sign in to comment.