Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent subscription creation if opgroup incorrect #244

Merged
merged 1 commit into from
May 9, 2024
Merged

Prevent subscription creation if opgroup incorrect #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 36 additions & 26 deletions controllers/operatorpolicy_controller.go
View file
Open in desktop
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
Open in desktop
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