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

Return a subscription from handleSubscription #198

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Return a subscription from handleSubscription #198

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
49 changes: 29 additions & 20 deletions controllers/operatorpolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ func (r *OperatorPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
return reconcile.Result{}, err
}

if err := r.handleSubscription(ctx, policy); err != nil {
_, err = r.handleSubscription(ctx, policy)
if err != nil {
OpLog.Error(err, "Error handling Subscription")

return reconcile.Result{}, err
Expand Down Expand Up @@ -358,98 +359,106 @@ func buildOperatorGroup(
return operatorGroup, nil
}

func (r *OperatorPolicyReconciler) handleSubscription(ctx context.Context, policy *policyv1beta1.OperatorPolicy) error {
func (r *OperatorPolicyReconciler) handleSubscription(
ctx context.Context, policy *policyv1beta1.OperatorPolicy,
) (*operatorv1alpha1.Subscription, error) {
watcher := opPolIdentifier(policy.Namespace, policy.Name)

desiredSub, err := buildSubscription(policy)
if err != nil {
return fmt.Errorf("error building subscription: %w", err)
return nil, fmt.Errorf("error building subscription: %w", err)
}

foundSub, err := r.DynamicWatcher.Get(watcher, subscriptionGVK, desiredSub.Namespace, desiredSub.Name)
if err != nil {
return fmt.Errorf("error getting the Subscription: %w", err)
return nil, fmt.Errorf("error getting the Subscription: %w", err)
}

if foundSub == nil {
// Missing Subscription: report NonCompliance
err := r.updateStatus(ctx, policy, missingWantedCond("Subscription"), missingWantedObj(desiredSub))
if err != nil {
return fmt.Errorf("error updating status for a missing Subscription: %w", err)
return nil, fmt.Errorf("error updating status for a missing Subscription: %w", err)
}

if policy.Spec.RemediationAction.IsEnforce() {
err := r.Create(ctx, desiredSub)
if err != nil {
return fmt.Errorf("error creating the Subscription: %w", err)
return nil, fmt.Errorf("error creating the Subscription: %w", err)
}

desiredSub.SetGroupVersionKind(subscriptionGVK) // Create stripped this information

// Now it should match, so report Compliance
err = r.updateStatus(ctx, policy, createdCond("Subscription"), createdObj(desiredSub))
if err != nil {
return fmt.Errorf("error updating the status for a created Subscription: %w", err)
return nil, fmt.Errorf("error updating the status for a created Subscription: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning a subscription inside of the logic that handles the enforce remediationAction type would be useful for future handler functions. It would make sense for this function to return a subscription only if it was created successfully on the cluster.

}

return nil
return desiredSub, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning something like nil, nil would be better here if we can return the subscription in the enforce case. Then in my PR, I could use the return value to determine if the desired subscription exists on the cluster (nil vs a subscription object) and skip a call to r.DynamicWatcher.Get to retrieve the subscription.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible... but I think that if inform mode returned nil, nil here, it would artificially limit some of the information we can get. For example, it is generally possible to check if the CatalogSource specified in the policy exists on the cluster, even if the Subscription doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be overlooking something but is the point of this to reduce calls to buildSubscription?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly. This would reduce calls to buildSubscription and dynamicWatcher.Get for the Subscription.

This can also return the "merged" subscription, regardless of inform/enforce, which hypothetically could be useful for some other healthcheck... I don't see a need for that yet, but it feels like it's easy enough to "do it right" now, so we don't have to worry about it later. For example if some other check used the channel in the subscription, this function would always return the desired setting based on the policy, which is what I think the user would be expecting.

Copy link
Contributor

@zyjjay zyjjay Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I think that would be useful in general. For my specific case I'm only calling dynamicWatcher.Get on a catalogsource when the subscription doesn't exist yet, so I might need to end up calling dynamicWatcher.Get on the subscription anyways in handleCatalogSource because the return values of handleSubscription doesn't seem to indicate whether or not a subscription exists on the cluster. I will look into ways to work around this on my end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should mention that I'm specifically thinking about the inform case.

}

// Subscription found; check if specs match
desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredSub)
if err != nil {
return fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
return nil, fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
}

merged := foundSub.DeepCopy() // Copy it so that the value in the cache is not changed

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

mergedSub := new(operatorv1alpha1.Subscription)
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(merged.Object, mergedSub); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked and I don't believe this will produce an error if there are unknown fields (e.g. version drift of different OLM versions). So this looks to be safe if you are just reading from it.

return nil, fmt.Errorf("error converting the retrieved Subscription to the go type: %w", err)
}

if !updateNeeded {
// FUTURE: Check more details about the *status* of the Subscription
// For now, just mark it as compliant
err := r.updateStatus(ctx, policy, matchesCond("Subscription"), matchedObj(foundSub))
if err != nil {
return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
return nil, fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
}

return nil
return mergedSub, nil
}

// Specs don't match.
if policy.Spec.RemediationAction.IsEnforce() && skipUpdate {
err = r.updateStatus(ctx, policy, mismatchCondUnfixable("Subscription"), mismatchedObj(foundSub))
if err != nil {
return fmt.Errorf("error updating status for a mismatched Subscription that can't be enforced: %w", err)
return nil, fmt.Errorf(
"error updating status for a mismatched Subscription that can't be enforced: %w", err)
}

return nil
return mergedSub, nil
}

err = r.updateStatus(ctx, policy, mismatchCond("Subscription"), mismatchedObj(foundSub))
if err != nil {
return fmt.Errorf("error updating status for a mismatched Subscription: %w", err)
return nil, fmt.Errorf("error updating status for a mismatched Subscription: %w", err)
}

if policy.Spec.RemediationAction.IsEnforce() {
err := r.Update(ctx, merged)
if err != nil {
return fmt.Errorf("error updating the Subscription: %w", err)
return nil, fmt.Errorf("error updating the Subscription: %w", err)
}

desiredSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information
merged.SetGroupVersionKind(subscriptionGVK) // Update stripped this information

err = r.updateStatus(ctx, policy, updatedCond("Subscription"), updatedObj(desiredSub))
err = r.updateStatus(ctx, policy, updatedCond("Subscription"), updatedObj(merged))
if err != nil {
return fmt.Errorf("error updating status after updating the Subscription: %w", err)
return nil, fmt.Errorf("error updating status after updating the Subscription: %w", err)
}
}

return nil
return mergedSub, nil
}

// buildSubscription bootstraps the subscription spec defined in the operator policy
Expand Down
Loading