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 7, 2024
1 parent 7984b1f commit 7f3fd49
Show file tree
Hide file tree
Showing 5 changed files with 634 additions and 15 deletions.
268 changes: 264 additions & 4 deletions controllers/operatorpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
operatorv1 "github.com/operator-framework/api/pkg/operators/v1"
operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
depclient "github.com/stolostron/kubernetes-dependency-watches/client"
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 All @@ -28,6 +29,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"

policyv1 "open-cluster-management.io/config-policy-controller/api/v1"
policyv1beta1 "open-cluster-management.io/config-policy-controller/api/v1beta1"
)

Expand All @@ -38,6 +40,7 @@ const (
var (
subscriptionGVK = schema.GroupVersionKind{Group: "operators.coreos.com", Version: "v1alpha1", Kind: "Subscription"}
operatorGroupGVK = schema.GroupVersionKind{Group: "operators.coreos.com", Version: "v1", Kind: "OperatorGroup"}
installPlanGVK = schema.GroupVersionKind{Group: "operators.coreos.com", Version: "v1alpha1", Kind: "InstallPlan"}
)

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

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

return reconcile.Result{}, err
}

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

return reconcile.Result{}, err
}

// FUTURE: more resource checks

return reconcile.Result{}, nil
Expand Down Expand Up @@ -418,11 +427,31 @@ 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 an OperatorGroup that matches: %w", err)
return nil, fmt.Errorf("error updating the status for a Subscription that matches: %w", err)
}

return mergedSub, nil
Expand Down Expand Up @@ -492,6 +521,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 @@ -550,3 +585,228 @@ 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 {
return errors.New("unable to handle InstallPlan: the subscription is 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

// 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 = ""
}

relObj := policyv1.RelatedObject{
Object: policyv1.ObjectResourceFromObj(&ownedInstallPlans[i]),
Properties: &policyv1.ObjectProperties{
UID: string(installPlan.GetUID()),
},
}

// A `reason` should be added to most InstallPlans based on the phase.
// Note that a `compliant` value will only be added in certain cases.
if phase != "" {
relObj.Reason = "The InstallPlan is " + phase
}

// handle some special phases
switch phase {
case string(operatorv1alpha1.InstallPlanPhaseRequiresApproval):
ipsRequiringApproval = append(ipsRequiringApproval, installPlan)

// FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`.
// For now, assume it is set to 'NonCompliant'
relObj.Compliant = string(policyv1.NonCompliant)
case string(operatorv1alpha1.InstallPlanPhaseInstalling):
anyInstalling = true

// if it's still installing, then it shouldn't be considered compliant yet.
relObj.Compliant = string(policyv1.NonCompliant)
}

relatedInstallPlans[i] = relObj
}

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), 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) == 0 {
// FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`.
// For now this condition assumes it is set to 'NonCompliant'
cond := installPlanUpgradeCond(allUpgradeVersions)
cond.Message += " but not allowed by the specified versions in the policy"

err := r.updateStatus(ctx, policy, cond, relatedInstallPlans...)
if err != nil {
return fmt.Errorf("error updating status when no approvable InstallPlans were found: %w", err)
}

return nil
}

if len(approvableInstallPlans) > 1 {
// FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`.
// For now this condition assumes it is set to 'NonCompliant'
cond := installPlanUpgradeCond(allUpgradeVersions)
cond.Message += " but multiple of those match the versions specified in the policy"

err := r.updateStatus(ctx, policy, cond, relatedInstallPlans...)
if err != nil {
return fmt.Errorf("error updating status when too many approvable InstallPlans were found: %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 7f3fd49

Please sign in to comment.