Skip to content

Commit

Permalink
Handle InstallPlan approval based on spec.versions
Browse files Browse the repository at this point in the history
When enforcing a policy that specifies which versions should be allowed,
the controller will now set the Subscription.InstallPlanApproval to
Manual, and will approve only the InstallPlans that match the versions
specified in the policy.

The controller now also reports the status of related InstallPlans.

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

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
  • Loading branch information
JustinKuli committed Feb 13, 2024
1 parent f6be8e7 commit 9ab77b0
Show file tree
Hide file tree
Showing 5 changed files with 730 additions and 26 deletions.
250 changes: 248 additions & 2 deletions controllers/operatorpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
depclient "github.com/stolostron/kubernetes-dependency-watches/client"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -64,6 +65,11 @@ var (
Version: "v1alpha1",
Kind: "CatalogSource",
}
installPlanGVK = schema.GroupVersionKind{
Group: "operators.coreos.com",
Version: "v1alpha1",
Kind: "InstallPlan",
}
)

// OperatorPolicyReconciler reconciles a OperatorPolicy object
Expand Down Expand Up @@ -158,6 +164,12 @@ func (r *OperatorPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
return reconcile.Result{}, err
}

if err := r.handleInstallPlan(ctx, policy, subscription); err != nil {
OpLog.Error(err, "Error handling InstallPlan")

return reconcile.Result{}, err
}

csv, err := r.handleCSV(ctx, policy, subscription)
if err != nil {
OpLog.Error(err, "Error handling CSVs")
Expand Down Expand Up @@ -529,8 +541,28 @@ func (r *OperatorPolicyReconciler) handleSubscription(
}

if !updateNeeded {
// FUTURE: Check more details about the *status* of the Subscription
// For now, just mark it as compliant
subResFailed := mergedSub.Status.GetCondition(operatorv1alpha1.SubscriptionResolutionFailed)

if subResFailed.Status == corev1.ConditionTrue {
cond := metav1.Condition{
Type: subConditionType,
Status: metav1.ConditionFalse,
Reason: subResFailed.Reason,
Message: subResFailed.Message,
}

if subResFailed.LastTransitionTime != nil {
cond.LastTransitionTime = *subResFailed.LastTransitionTime
}

err := r.updateStatus(ctx, policy, cond, nonCompObj(foundSub, subResFailed.Reason))
if err != nil {
return nil, fmt.Errorf("error updating the status for a Subscription in ResolutionFailed: %w", err)
}

return mergedSub, nil
}

err := r.updateStatus(ctx, policy, matchesCond("Subscription"), matchedObj(foundSub))
if err != nil {
return nil, fmt.Errorf("error updating the status for a Subscription that matches: %w", err)
Expand Down Expand Up @@ -603,6 +635,12 @@ func buildSubscription(
subscription.ObjectMeta.Namespace = ns
subscription.Spec = spec

// If the policy is in `enforce` mode and the allowed CSVs are restricted,
// the InstallPlanApproval will be set to Manual so that upgrades can be controlled.
if policy.Spec.RemediationAction.IsEnforce() && len(policy.Spec.Versions) > 0 {
subscription.Spec.InstallPlanApproval = operatorv1alpha1.ApprovalManual
}

return subscription, nil
}

Expand Down Expand Up @@ -790,3 +828,211 @@ func (r *OperatorPolicyReconciler) mergeObjects(

return updateNeeded, false, nil
}

func (r *OperatorPolicyReconciler) handleInstallPlan(
ctx context.Context, policy *policyv1beta1.OperatorPolicy, sub *operatorv1alpha1.Subscription,
) error {
if sub == nil {
err := r.updateStatus(ctx, policy, noInstallPlansCond, noInstallPlansObj(""))
if err != nil {
return fmt.Errorf("error updating status when the subscription is nil: %w", err)
}

return nil
}

watcher := opPolIdentifier(policy.Namespace, policy.Name)

foundInstallPlans, err := r.DynamicWatcher.List(
watcher, installPlanGVK, sub.Namespace, labels.Everything())
if err != nil {
return fmt.Errorf("error listing InstallPlans: %w", err)
}

ownedInstallPlans := make([]unstructured.Unstructured, 0, len(foundInstallPlans))

for _, installPlan := range foundInstallPlans {
for _, owner := range installPlan.GetOwnerReferences() {
match := owner.Name == sub.Name &&
owner.Kind == subscriptionGVK.Kind &&
owner.APIVersion == subscriptionGVK.GroupVersion().String()
if match {
ownedInstallPlans = append(ownedInstallPlans, installPlan)

break
}
}
}

// InstallPlans are generally kept in order to provide a history of actions on the cluster, but
// they can be deleted without impacting the installed operator. So, not finding any should not
// be considered a reason for NonCompliance.
if len(ownedInstallPlans) == 0 {
err := r.updateStatus(ctx, policy, noInstallPlansCond, noInstallPlansObj(sub.Namespace))
if err != nil {
return fmt.Errorf("error updating status when no relevant InstallPlans were found: %w", err)
}

return nil
}

OpLog := ctrl.LoggerFrom(ctx)
relatedInstallPlans := make([]policyv1.RelatedObject, len(ownedInstallPlans))
ipsRequiringApproval := make([]unstructured.Unstructured, 0)
anyInstalling := false
currentPlanFailed := false

// Construct the relevant relatedObjects, and collect any that might be considered for approval
for i, installPlan := range ownedInstallPlans {
phase, ok, err := unstructured.NestedString(installPlan.Object, "status", "phase")
if !ok && err == nil {
err = errors.New("the phase of the InstallPlan was not found")
}

if err != nil {
OpLog.Error(err, "Unable to determine the phase of the related InstallPlan",
"InstallPlan.Name", installPlan.GetName())

// The InstallPlan will be added as unknown
phase = ""
}

// consider some special phases
switch phase {
case string(operatorv1alpha1.InstallPlanPhaseRequiresApproval):
ipsRequiringApproval = append(ipsRequiringApproval, installPlan)
case string(operatorv1alpha1.InstallPlanPhaseInstalling):
anyInstalling = true
case string(operatorv1alpha1.InstallPlanFailed):
// Generally, a failed InstallPlan is not a reason for NonCompliance, because it could be from
// an old installation. But if the current InstallPlan is failed, we should alert the user.
if sub.Status.InstallPlanRef != nil && sub.Status.InstallPlanRef.Name == installPlan.GetName() {
currentPlanFailed = true
}
}

relatedInstallPlans[i] = existingInstallPlanObj(&ownedInstallPlans[i], phase)
}

if currentPlanFailed {
err := r.updateStatus(ctx, policy, installPlanFailed, relatedInstallPlans...)
if err != nil {
return fmt.Errorf("error updating status when the current InstallPlan has failed: %w", err)
}

return nil
}

if anyInstalling {
err := r.updateStatus(ctx, policy, installPlanInstallingCond, relatedInstallPlans...)
if err != nil {
return fmt.Errorf("error updating status when an installing InstallPlan was found: %w", err)
}

return nil
}

if len(ipsRequiringApproval) == 0 {
err := r.updateStatus(ctx, policy, installPlansFineCond, relatedInstallPlans...)
if err != nil {
return fmt.Errorf("error updating status when InstallPlans were fine: %w", err)
}

return nil
}

allUpgradeVersions := make([]string, len(ipsRequiringApproval))

for i, installPlan := range ipsRequiringApproval {
csvNames, ok, err := unstructured.NestedStringSlice(installPlan.Object,
"spec", "clusterServiceVersionNames")
if !ok && err != nil {
err = errors.New("the clusterServiceVersionNames field of the InstallPlan was not found")
}

if err != nil {
OpLog.Error(err, "Unable to determine the csv names of the related InstallPlan",
"InstallPlan.Name", installPlan.GetName())

csvNames = []string{"???"}
}

allUpgradeVersions[i] = fmt.Sprintf("%v", csvNames)
}

// Only report this status in `inform` mode, because otherwise it could easily oscillate between this and
// another condition below when being enforced.
if policy.Spec.RemediationAction.IsInform() {
// FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`.
// For now this condition assumes it is set to 'NonCompliant'
err := r.updateStatus(ctx, policy, installPlanUpgradeCond(allUpgradeVersions, nil), relatedInstallPlans...)
if err != nil {
return fmt.Errorf("error updating status when an InstallPlan requiring approval was found: %w", err)
}

return nil
}

approvedVersion := "" // this will only be accurate when there is only one approvable InstallPlan
approvableInstallPlans := make([]unstructured.Unstructured, 0)

for _, installPlan := range ipsRequiringApproval {
ipCSVs, ok, err := unstructured.NestedStringSlice(installPlan.Object,
"spec", "clusterServiceVersionNames")
if !ok && err != nil {
err = errors.New("the clusterServiceVersionNames field of the InstallPlan was not found")
}

if err != nil {
OpLog.Error(err, "Unable to determine the csv names of the related InstallPlan",
"InstallPlan.Name", installPlan.GetName())

continue
}

if len(ipCSVs) != 1 {
continue // Don't automate approving any InstallPlans for multiple CSVs
}

matchingCSV := len(policy.Spec.Versions) == 0 // true if `spec.versions` is not specified

for _, acceptableCSV := range policy.Spec.Versions {
if string(acceptableCSV) == ipCSVs[0] {
matchingCSV = true

break
}
}

if matchingCSV {
approvedVersion = ipCSVs[0]

approvableInstallPlans = append(approvableInstallPlans, installPlan)
}
}

if len(approvableInstallPlans) != 1 {
err := r.updateStatus(ctx, policy,
installPlanUpgradeCond(allUpgradeVersions, approvableInstallPlans), relatedInstallPlans...)
if err != nil {
return fmt.Errorf("error updating status when an InstallPlan can't be automatically approved: %w", err)
}

return nil
}

if err := unstructured.SetNestedField(approvableInstallPlans[0].Object, true, "spec", "approved"); err != nil {
return fmt.Errorf("error approving InstallPlan: %w", err)
}

if err := r.Update(ctx, &approvableInstallPlans[0]); err != nil {
return fmt.Errorf("error updating approved InstallPlan: %w", err)
}

err = r.updateStatus(ctx, policy, installPlanApprovedCond(approvedVersion), relatedInstallPlans...)
if err != nil {
return fmt.Errorf("error updating status after approving an InstallPlan: %w", err)
}

return nil
}
Loading

0 comments on commit 9ab77b0

Please sign in to comment.