From 2f0a380a724e5ede70959266d1f91e4dfc6def64 Mon Sep 17 00:00:00 2001 From: mprahl Date: Thu, 6 Jun 2024 14:21:44 -0400 Subject: [PATCH 1/2] Add a label and annotation for subscriptions managed by OperatorPolicy The operatorpolicy.policy.open-cluster-management.io/managed label is set to an empty value on the Subscription. The operatorpolicy.policy.open-cluster-management.io/managed annotation is set to .. The label doesn't set a value because the value could be too long. This will allow querying for subscriptions managed by operator policies. Signed-off-by: mprahl --- controllers/operatorpolicy_controller.go | 38 ++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 8e70a064..833a3598 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -48,6 +48,8 @@ const ( OperatorControllerName string = "operator-policy-controller" CatalogSourceReady string = "READY" olmGracePeriod = 30 * time.Second + ManagedByLabel string = "operatorpolicy.policy.open-cluster-management.io/managed" + ManagedByAnnotation string = ManagedByLabel ) var ( @@ -1148,6 +1150,36 @@ func (r *OperatorPolicyReconciler) musthaveSubscription( return nil, nil, false, fmt.Errorf("error converting the retrieved Subscription to the go type: %w", err) } + if policy.Spec.RemediationAction.IsEnforce() { + labels := mergedSub.GetLabels() + if _, ok := labels[ManagedByLabel]; !ok { + if labels == nil { + labels = map[string]string{} + } + + labels[ManagedByLabel] = "" + + mergedSub.SetLabels(labels) + + updateNeeded = true + } + + expectedValue := policy.Namespace + "." + policy.Name + annotations := mergedSub.GetAnnotations() + + if annotations[ManagedByAnnotation] != expectedValue { + if annotations == nil { + annotations = map[string]string{} + } + + annotations[ManagedByAnnotation] = expectedValue + + mergedSub.SetAnnotations(annotations) + + updateNeeded = true + } + } + if !updateNeeded { subResFailed := mergedSub.Status.GetCondition(operatorv1alpha1.SubscriptionResolutionFailed) @@ -1187,14 +1219,14 @@ func (r *OperatorPolicyReconciler) musthaveSubscription( opLog.Info("Updating Subscription to match the desired state", "subName", foundSub.GetName(), "subNamespace", foundSub.GetNamespace()) - err = r.TargetClient.Update(ctx, foundSub) + err = r.TargetClient.Update(ctx, mergedSub) if err != nil { return mergedSub, nil, changed, fmt.Errorf("error updating the Subscription: %w", err) } - foundSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information + mergedSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information - updateStatus(policy, updatedCond("Subscription"), updatedObj(foundSub)) + updateStatus(policy, updatedCond("Subscription"), updatedObj(mergedSub)) return mergedSub, earlyConds, true, nil } From 4f07c8298cd6654c62df666135e79a57581d2067 Mon Sep 17 00:00:00 2001 From: mprahl Date: Thu, 6 Jun 2024 15:40:03 -0400 Subject: [PATCH 2/2] Add support for approving InstallPlans with multiple CSVs When an InstallPlan contains multiple CSVs, it will be approved if all CSVs can be approved by the current operator policies. Rather than query the operator policies directly, the subscriptions in the namespace that are managed by an operator policy are queried and then their associated operator policies are queried to avoid having to resolve templates to see if they manage a subscription in the namespace. Relates: https://issues.redhat.com/browse/ACM-11981 Signed-off-by: mprahl --- controllers/operatorpolicy_controller.go | 401 +++++++++++++----- controllers/operatorpolicy_status.go | 24 +- test/e2e/case38_install_operator_test.go | 123 +++++- .../multiple-subscriptions.yaml | 31 ++ .../operator-policy-argocd.yaml | 24 ++ ...perator-policy-strimzi-kafka-operator.yaml | 24 ++ 6 files changed, 500 insertions(+), 127 deletions(-) create mode 100644 test/resources/case38_operator_install/multiple-subscriptions.yaml create mode 100644 test/resources/case38_operator_install/operator-policy-argocd.yaml create mode 100644 test/resources/case38_operator_install/operator-policy-strimzi-kafka-operator.yaml diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 833a3598..f649b419 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -12,6 +12,7 @@ import ( "reflect" "regexp" "slices" + "sort" "strconv" "strings" "time" @@ -28,8 +29,10 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/dynamic" ctrl "sigs.k8s.io/controller-runtime" @@ -1443,10 +1446,19 @@ func (r *OperatorPolicyReconciler) handleInstallPlan( return updateStatus(policy, invalidCausingUnknownCond("InstallPlan")), nil } + if sub.Status.CurrentCSV == "" && sub.Status.InstalledCSV == "" { + return updateStatus(policy, noInstallPlansCond, noInstallPlansObj(sub.Namespace)), nil + } + watcher := opPolIdentifier(policy.Namespace, policy.Name) - selector := subLabelSelector(sub) - installPlans, err := r.DynamicWatcher.List(watcher, installPlanGVK, sub.Namespace, selector) + // An InstallPlan will contain all CSVs that can be installed/updated in the namespace at the time the InstallPlan + // is created. The latest is denoted by the highest `spec.generation`, though `creationTimestamp` would likely + // also work. If the catalog is updated so that `status.currentCSV` on the Subscription does not point to an + // existing CSV, `status.currentCSV` will get included in the new InstallPlan. A new Subscription will also trigger + // a new InstallPlan. This code will always pick the latest InstallPlan so as to avoid installing older and + // potentially vulnerable operators. + installPlans, err := r.DynamicWatcher.List(watcher, installPlanGVK, sub.Namespace, labels.Everything()) if err != nil { return false, fmt.Errorf("error listing InstallPlans: %w", err) } @@ -1458,187 +1470,350 @@ func (r *OperatorPolicyReconciler) handleInstallPlan( return updateStatus(policy, noInstallPlansCond, noInstallPlansObj(sub.Namespace)), nil } + var latestInstallPlan *unstructured.Unstructured + var latestGeneration int64 = -1 + + // Find the latest InstallPlan that references either the installed CSV or the latest CSV available for update. + for i := range installPlans { + csvs, found, err := unstructured.NestedStringSlice( + installPlans[i].Object, "spec", "clusterServiceVersionNames", + ) + if !found || err != nil { + continue + } + + matchFound := false + containsCurrentCSV := false + + for _, csv := range csvs { + if csv == sub.Status.CurrentCSV { + containsCurrentCSV = true + matchFound = true + + break + } + + if csv == sub.Status.InstalledCSV { + matchFound = true + + break + } + } + + if !matchFound { + continue + } + + // If the InstallPlan contains the current CSV and the phase indicates it is installing or complete, always + // use that InstallPlan since race conditions in OLM can cause a newer InstallPlan to be created with this CSV. + if containsCurrentCSV { + phase, _, _ := unstructured.NestedString(installPlans[i].Object, "status", "phase") + ipInstalling := string(operatorv1alpha1.InstallPlanPhaseInstalling) + ipComplete := string(operatorv1alpha1.InstallPlanPhaseComplete) + + if phase == ipInstalling || phase == ipComplete { + latestInstallPlan = &installPlans[i] + + break + } + } + + generation, found, err := unstructured.NestedInt64(installPlans[i].Object, "spec", "generation") + if !found || err != nil { + continue + } + + if generation > latestGeneration { + latestGeneration = generation + latestInstallPlan = &installPlans[i] + } + } + + if latestInstallPlan == nil { + log.Info( + "No InstallPlan had the CSVs mentioned in the subscription status", + "subscription", sub.Name, "namespace", sub.Namespace, + ) + + return updateStatus(policy, noInstallPlansCond, noInstallPlansObj(sub.Namespace)), nil + } + if policy.Spec.ComplianceType.IsMustHave() { - changed, err := r.musthaveInstallPlan(ctx, policy, sub, installPlans) + changed, err := r.musthaveInstallPlan(ctx, policy, sub, latestInstallPlan) return changed, err } - return r.mustnothaveInstallPlan(policy, installPlans) + return r.mustnothaveInstallPlan(policy, latestInstallPlan) } func (r *OperatorPolicyReconciler) musthaveInstallPlan( ctx context.Context, policy *policyv1beta1.OperatorPolicy, sub *operatorv1alpha1.Subscription, - ownedInstallPlans []unstructured.Unstructured, + latestInstallPlanUnstruct *unstructured.Unstructured, ) (bool, error) { opLog := ctrl.LoggerFrom(ctx) - relatedInstallPlans := make([]policyv1.RelatedObject, 0, len(ownedInstallPlans)) - ipsRequiringApproval := make([]unstructured.Unstructured, 0) - anyInstalling := false - currentPlanFailed := false complianceConfig := policy.Spec.ComplianceConfig.UpgradesAvailable + latestInstallPlan := operatorv1alpha1.InstallPlan{} + + err := runtime.DefaultUnstructuredConverter.FromUnstructured(latestInstallPlanUnstruct.Object, &latestInstallPlan) + if err != nil { + opLog.Error(err, "Unable to determine the CSV names of the current InstallPlan", + "InstallPlan.Name", latestInstallPlanUnstruct.GetName()) + + return updateStatus(policy, installPlansNoApprovals), nil + } - // 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") + ipCSVs := latestInstallPlan.Spec.ClusterServiceVersionNames + phase := latestInstallPlan.Status.Phase + + // consider some special phases + switch phase { //nolint: exhaustive + case operatorv1alpha1.InstallPlanPhaseRequiresApproval: + // If OLM created an InstallPlan that requires approval but the installed CSV is already the latest, + // then the InstallPlan can be ignored. + installedCSV := sub.Status.InstalledCSV + currentCSV := sub.Status.CurrentCSV + + if installedCSV != "" && currentCSV != "" && installedCSV == currentCSV { + return updateStatus(policy, installPlansNoApprovals), nil } + case operatorv1alpha1.InstallPlanPhaseInstalling: + return updateStatus( + policy, + installPlanInstallingCond, + existingInstallPlanObj(&latestInstallPlan, string(phase), complianceConfig), + ), nil + case 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 len(ipCSVs) == 1 { + return updateStatus( + policy, installPlanFailed, existingInstallPlanObj(&latestInstallPlan, string(phase), complianceConfig), + ), nil + } + + // If there is more than one CSV in the InstallPlan, make sure this CSV failed before marking it as + // noncompliant. + for _, step := range latestInstallPlan.Status.Plan { + if step.Resolving != sub.Status.CurrentCSV { + continue + } - 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 + switch step.Status { //nolint: exhaustive + case operatorv1alpha1.StepStatusCreated: + case operatorv1alpha1.StepStatusPresent: + default: + return updateStatus( + policy, + installPlanFailed, + existingInstallPlanObj(&latestInstallPlan, string(phase), complianceConfig), + ), nil } } - relatedInstallPlans = append(relatedInstallPlans, - existingInstallPlanObj(&ownedInstallPlans[i], phase, complianceConfig)) + // If no step for this CSV failed in the InstallPlan, treat it the same as no install plans requiring approval + return updateStatus(policy, installPlansNoApprovals), nil + default: + return updateStatus( + policy, + installPlansNoApprovals, existingInstallPlanObj(&latestInstallPlan, string(phase), complianceConfig), + ), nil + } + + // Check if it's already approved. This can happen if this OperatorPolicy or another managing the same InstallPlan + // performed the approval but it hasn't started installing yet. + if latestInstallPlan.Spec.Approved { + return updateStatus( + policy, + installPlanApprovedCond(sub.Status.CurrentCSV), + existingInstallPlanObj(&latestInstallPlan, string(phase), complianceConfig), + ), nil } - opLog.V(2).Info("InstallPlans examined", "currentPlanFailed", currentPlanFailed, "anyInstalling", anyInstalling, - "ipsRequiringApprovalLen", len(ipsRequiringApproval)) + initialInstall := sub.Status.InstalledCSV == "" + autoUpgrade := policy.Spec.UpgradeApproval == "Automatic" - if currentPlanFailed { - return updateStatus(policy, installPlanFailed, relatedInstallPlans...), nil + // Only report this status when not approving an InstallPlan, because otherwise it could easily + // oscillate between this and another condition. + if policy.Spec.RemediationAction.IsInform() || (!initialInstall && !autoUpgrade) { + return updateStatus( + policy, + installPlanUpgradeCond(complianceConfig, ipCSVs, nil), + existingInstallPlanObj(&latestInstallPlan, string(phase), complianceConfig), + ), nil } - if anyInstalling { - return updateStatus(policy, installPlanInstallingCond, relatedInstallPlans...), nil + remainingCSVsToApprove, err := r.getRemainingCSVApprovals(ctx, policy, sub, ipCSVs) + if err != nil { + return false, fmt.Errorf("error finding the InstallPlan approvals: %w", err) } - if len(ipsRequiringApproval) == 0 { - return updateStatus(policy, installPlansNoApprovals, relatedInstallPlans...), nil + if len(remainingCSVsToApprove) != 0 { + return updateStatus( + policy, + installPlanUpgradeCond(complianceConfig, ipCSVs, remainingCSVsToApprove), + existingInstallPlanObj(&latestInstallPlan, string(phase), complianceConfig), + ), nil } - allUpgradeVersions := make([]string, 0, len(ipsRequiringApproval)) + opLog.Info("Approving InstallPlan", "InstallPlanName", latestInstallPlan.Name, + "InstallPlanNamespace", latestInstallPlan.Namespace) - for _, 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 := unstructured.SetNestedField(latestInstallPlanUnstruct.Object, true, "spec", "approved"); err != nil { + return false, fmt.Errorf("error approving InstallPlan: %w", err) + } - if err != nil { - opLog.Error(err, "Unable to determine the csv names of the related InstallPlan", - "InstallPlan.Name", installPlan.GetName()) + if err := r.TargetClient.Update(ctx, latestInstallPlanUnstruct); err != nil { + return false, fmt.Errorf("error updating approved InstallPlan: %w", err) + } - csvNames = []string{"unknown"} - } + return updateStatus( + policy, + installPlanApprovedCond(sub.Status.CurrentCSV), + existingInstallPlanObj(&latestInstallPlan, string(phase), complianceConfig), + ), + nil +} + +// getRemainingCSVApprovals will return all CSVs that can't be approved by an OperatorPolicy (currentPolicy and others). +func (r *OperatorPolicyReconciler) getRemainingCSVApprovals( + ctx context.Context, + currentPolicy *policyv1beta1.OperatorPolicy, + currentSub *operatorv1alpha1.Subscription, + csvNames []string, +) ([]string, error) { + requiredCSVs := sets.New(csvNames...) + approvedCSVs := sets.New[string]() + + // First try the current OperatorPolicy without checking others. + approvedCSV := r.getApprovedCSV(currentPolicy, currentSub, csvNames) + if approvedCSV != "" { + approvedCSVs.Insert(approvedCSV) - allUpgradeVersions = append(allUpgradeVersions, fmt.Sprintf("%v", csvNames)) + if requiredCSVs.Equal(approvedCSVs) { + return nil, nil + } } - initialInstall := sub.Status.InstalledCSV == "" - autoUpgrade := policy.Spec.UpgradeApproval == "Automatic" + watcher := opPolIdentifier(currentPolicy.Namespace, currentPolicy.Name) - // Only report this status when not approving an InstallPlan, because otherwise it could easily - // oscillate between this and another condition. - if policy.Spec.RemediationAction.IsInform() || (!initialInstall && !autoUpgrade) { - return updateStatus(policy, installPlanUpgradeCond(complianceConfig, allUpgradeVersions, nil), - relatedInstallPlans...), nil + labelRequirement, err := labels.NewRequirement(ManagedByLabel, selection.Exists, nil) + if err != nil { + return nil, err } - approvedVersion := "" // this will only be accurate when there is only one approvable InstallPlan - approvableInstallPlans := make([]unstructured.Unstructured, 0) + managedBySelector := labels.NewSelector().Add(*labelRequirement) - 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") + // List the subscriptions in the namespace managed by OperatorPolicy. This is done to avoid resolving all templates + // for every OperatorPolicy to see if it manages a subscription in the namespace. + subs, err := r.DynamicWatcher.List(watcher, subscriptionGVK, currentSub.Namespace, managedBySelector) + if err != nil { + return nil, err + } + + for _, sub := range subs { + // Skip the current subscription since it was already checked + if sub.GetUID() == currentSub.UID { + continue } - if err != nil { - opLog.Error(err, "Unable to determine the csv names of the related InstallPlan", - "InstallPlan.Name", installPlan.GetName()) + annotationValue := sub.GetAnnotations()[ManagedByAnnotation] + if annotationValue == "" { continue } - if len(ipCSVs) != 1 { - continue // Don't automate approving any InstallPlans for multiple CSVs + parts := strings.Split(annotationValue, ".") + if len(parts) != 2 { + continue } - matchingCSV := len(policy.Spec.Versions) == 0 // true if `spec.versions` is not specified - allowedVersions := make([]policyv1.NonEmptyString, 0, len(policy.Spec.Versions)+1) - allowedVersions = append(allowedVersions, policy.Spec.Versions...) - - if sub.Spec.StartingCSV != "" { - allowedVersions = append(allowedVersions, policyv1.NonEmptyString(sub.Spec.StartingCSV)) - } + policyNamespace := parts[0] + policyName := parts[1] - for _, acceptableCSV := range allowedVersions { - if string(acceptableCSV) == ipCSVs[0] { - matchingCSV = true + policy := policyv1beta1.OperatorPolicy{} - break + err := r.Get(ctx, types.NamespacedName{Namespace: policyNamespace, Name: policyName}, &policy) + if err != nil { + if k8serrors.IsNotFound(err) { + continue } + + return nil, err } - if matchingCSV { - approvedVersion = ipCSVs[0] + subTyped := operatorv1alpha1.Subscription{} + + err = runtime.DefaultUnstructuredConverter.FromUnstructured(sub.Object, &subTyped) + if err != nil { + continue + } - approvableInstallPlans = append(approvableInstallPlans, installPlan) + approvedCSV := r.getApprovedCSV(&policy, &subTyped, csvNames) + if approvedCSV != "" { + approvedCSVs.Insert(approvedCSV) } } - opLog.V(2).Info("Determined approvable InstallPlans", "count", len(approvableInstallPlans)) + unapprovedCSVs := requiredCSVs.Difference(approvedCSVs).UnsortedList() - if len(approvableInstallPlans) != 1 { - changed := updateStatus( - policy, - installPlanUpgradeCond(complianceConfig, allUpgradeVersions, approvableInstallPlans), - relatedInstallPlans..., - ) + sort.Strings(unapprovedCSVs) + + return unapprovedCSVs, nil +} - return changed, nil +// getApprovedCSV returns the CSV in the passed in csvNames that can be approved. If no approval can take place, an +// empty string is returned. +func (r *OperatorPolicyReconciler) getApprovedCSV( + policy *policyv1beta1.OperatorPolicy, sub *operatorv1alpha1.Subscription, csvNames []string, +) string { + // Only enforce policies can approve InstallPlans + if !policy.Spec.RemediationAction.IsEnforce() { + return "" } - opLog.Info("Approving InstallPlan", "InstallPlanName", approvableInstallPlans[0].GetName(), - "InstallPlanNamespace", approvableInstallPlans[0].GetNamespace()) + initialInstall := sub.Status.InstalledCSV == "" + autoUpgrade := policy.Spec.UpgradeApproval == "Automatic" - if err := unstructured.SetNestedField(approvableInstallPlans[0].Object, true, "spec", "approved"); err != nil { - return false, fmt.Errorf("error approving InstallPlan: %w", err) + // If the operator is already installed and isn't configured for automatic upgrades, then it can't approve + // InstallPlans. + if !autoUpgrade && !initialInstall { + return "" } - if err := r.TargetClient.Update(ctx, &approvableInstallPlans[0]); err != nil { - return false, fmt.Errorf("error updating approved InstallPlan: %w", err) + allowedCSVs := make([]policyv1.NonEmptyString, 0, len(policy.Spec.Versions)+1) + allowedCSVs = append(allowedCSVs, policy.Spec.Versions...) + + if sub.Spec.StartingCSV != "" { + allowedCSVs = append(allowedCSVs, policyv1.NonEmptyString(sub.Spec.StartingCSV)) + } + + // All versions for the operator are allowed if no version is specified + if len(policy.Spec.Versions) == 0 { + // currentCSV is the CSV related to the latest InstallPlan + if sub.Status.CurrentCSV != "" { + return sub.Status.CurrentCSV + } + } else { + for _, allowedCSV := range allowedCSVs { + for _, csvName := range csvNames { + if string(allowedCSV) == csvName { + return csvName + } + } + } } - return updateStatus(policy, installPlanApprovedCond(approvedVersion), relatedInstallPlans...), nil + return "" } func (r *OperatorPolicyReconciler) mustnothaveInstallPlan( policy *policyv1beta1.OperatorPolicy, - ownedInstallPlans []unstructured.Unstructured, + latestInstallPlan *unstructured.Unstructured, ) (bool, error) { - // Let OLM handle removing install plans - relatedInstallPlans := make([]policyv1.RelatedObject, 0, len(ownedInstallPlans)) - - for i := range ownedInstallPlans { - relatedInstallPlans = append(relatedInstallPlans, foundNotApplicableObj(&ownedInstallPlans[i])) - } - - changed := updateStatus(policy, notApplicableCond("InstallPlan"), relatedInstallPlans...) + changed := updateStatus(policy, notApplicableCond("InstallPlan"), foundNotApplicableObj(latestInstallPlan)) return changed, nil } diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index ee2b98dd..3e722ec0 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -688,27 +688,25 @@ var installPlansNoApprovals = metav1.Condition{ // policy status is updated. func installPlanUpgradeCond( complianceConfig policyv1beta1.ComplianceConfigAction, - versions []string, - approvableIPs []unstructured.Unstructured, + csvsInInstallPlan []string, + remainingCSVsToApprove []string, ) metav1.Condition { cond := metav1.Condition{ Type: installPlanConditionType, Reason: "InstallPlanRequiresApproval", } - if len(versions) == 1 { - cond.Message = fmt.Sprintf("an InstallPlan to update to %v is available for approval", versions[0]) - } else { - cond.Message = fmt.Sprintf("there are multiple InstallPlans available for approval (%v)", - strings.Join(versions, ", or ")) - } + sort.Strings(csvsInInstallPlan) + cond.Message = fmt.Sprintf( + "an InstallPlan to update to [%s] is available for approval", strings.Join(csvsInInstallPlan, ", "), + ) - if approvableIPs != nil && len(approvableIPs) == 0 { - cond.Message += " but not allowed by the specified versions in the policy" - } + if len(remainingCSVsToApprove) > 0 { + sort.Strings(remainingCSVsToApprove) - if len(approvableIPs) > 1 { - cond.Message += " but multiple of those match the versions specified in the policy" + cond.Message += fmt.Sprintf( + " but approval for [%v] is required", strings.Join(remainingCSVsToApprove, ", "), + ) } if complianceConfig == "Compliant" { diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 460cec4d..e7e3fe8f 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -7,6 +7,7 @@ import ( "reflect" "regexp" "slices" + "sort" "strconv" "strings" "time" @@ -1428,7 +1429,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu Status: metav1.ConditionFalse, Reason: "InstallPlanRequiresApproval", Message: "an InstallPlan to update to [strimzi-cluster-operator.v0.36.1] is available for " + - "approval but not allowed by the specified versions in the policy", + "approval but approval for [strimzi-cluster-operator.v0.36.1] is required", }, "the InstallPlan.*36.0.*was approved", ) @@ -3420,4 +3421,124 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu checkCompliance(opPolName, testNamespace, 2*olmWaitTimeout, policyv1.Compliant, 30, 3) }) }) + + Describe("Testing approving an InstallPlan with multiple CSVs", Ordered, func() { + const ( + opPolArgoCDYAML = "../resources/case38_operator_install/operator-policy-argocd.yaml" + opPolKafkaYAML = "../resources/case38_operator_install/operator-policy-strimzi-kafka-operator.yaml" + subscriptionsYAML = "../resources/case38_operator_install/multiple-subscriptions.yaml" + ) + + BeforeAll(func() { + preFunc() + }) + + It("OperatorPolicy can approve an InstallPlan with two CSVs", func(ctx SpecContext) { + By("Creating two Subscriptions to generate an InstallPlan with two CSVs") + KubectlTarget("-n", opPolTestNS, "create", "-f", subscriptionsYAML) + + Eventually(func(g Gomega) { + var installPlans *unstructured.UnstructuredList + + // Wait up to 10 seconds for the InstallPlan to appear + g.Eventually(func(g Gomega) { + var err error + installPlans, err = targetK8sDynamic.Resource(gvrInstallPlan).Namespace(opPolTestNS).List( + ctx, metav1.ListOptions{}, + ) + + g.Expect(err).ToNot(HaveOccurred()) + // OLM often creates duplicate InstallPlans so account for that. + g.Expect( + len(installPlans.Items) == 1 || len(installPlans.Items) == 2).To(BeTrue(), + "expected 1 or 2 InstallPlans", + ) + }, 10, 1).Should(Succeed()) + + csvNames, _, _ := unstructured.NestedStringSlice( + installPlans.Items[0].Object, "spec", "clusterServiceVersionNames", + ) + + if len(csvNames) != 2 { + KubectlTarget("-n", opPolTestNS, "delete", "-f", subscriptionsYAML, "--ignore-not-found") + KubectlTarget("-n", opPolTestNS, "delete", "installplans", "--all") + KubectlTarget("-n", opPolTestNS, "create", "-f", subscriptionsYAML) + } + + g.Expect(csvNames).To(ConsistOf("argocd-operator.v0.9.1", "strimzi-cluster-operator.v0.35.0")) + }, olmWaitTimeout, 1).Should(Succeed()) + + By("Creating an OperatorPolicy to adopt the argocd-operator Subscription") + createObjWithParent( + parentPolicyYAML, parentPolicyName, opPolArgoCDYAML, testNamespace, gvrPolicy, gvrOperatorPolicy, + ) + + By("Checking the OperatorPolicy InstallPlan is not approved because of strimzi-cluster-operator.v0.35.0") + check( + "argocd-operator", + true, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "InstallPlan", + APIVersion: "operators.coreos.com/v1alpha1", + Metadata: policyv1.ObjectMetadata{ + Namespace: opPolTestNS, + }, + }, + Compliant: "Compliant", + Reason: "The InstallPlan is RequiresApproval", + }}, + metav1.Condition{ + Type: "InstallPlanCompliant", + Status: metav1.ConditionTrue, + Reason: "InstallPlanRequiresApproval", + Message: "an InstallPlan to update to [argocd-operator.v0.9.1, " + + "strimzi-cluster-operator.v0.35.0] " + + "is available for approval but approval for [strimzi-cluster-operator.v0.35.0] is required", + }, + "an InstallPlan to update to .* is available for approval", + ) + + By("Creating an OperatorPolicy to adopt the strimzi-cluster-operator Subscription") + createObjWithParent( + parentPolicyYAML, parentPolicyName, opPolKafkaYAML, testNamespace, gvrPolicy, gvrOperatorPolicy, + ) + + By("Verifying the initial installPlan for startingCSV was approved") + Eventually(func(g Gomega) { + installPlans, err := targetK8sDynamic.Resource(gvrInstallPlan).Namespace(opPolTestNS).List( + ctx, metav1.ListOptions{}, + ) + g.Expect(err).ToNot(HaveOccurred()) + + var approvedInstallPlan bool + + // OLM often creates duplicate InstallPlans with the same generation. They are the same but OLM + // concurrency can encounter race conditions, so just see that one of them is approved by + // the policies. + for _, installPlan := range installPlans.Items { + csvNames, _, _ := unstructured.NestedStringSlice( + installPlan.Object, "spec", "clusterServiceVersionNames", + ) + + sort.Strings(csvNames) + + if !reflect.DeepEqual( + csvNames, []string{"argocd-operator.v0.9.1", "strimzi-cluster-operator.v0.35.0"}, + ) { + continue + } + + approved, _, _ := unstructured.NestedBool(installPlan.Object, "spec", "approved") + if approved { + approvedInstallPlan = true + + break + } + } + + g.Expect(approvedInstallPlan).To(BeTrue(), "Expect an InstallPlan for startingCSV to be approved") + }, olmWaitTimeout, 1).Should(Succeed()) + }) + }) }) diff --git a/test/resources/case38_operator_install/multiple-subscriptions.yaml b/test/resources/case38_operator_install/multiple-subscriptions.yaml new file mode 100644 index 00000000..faf9d7d9 --- /dev/null +++ b/test/resources/case38_operator_install/multiple-subscriptions.yaml @@ -0,0 +1,31 @@ +apiVersion: operators.coreos.com/v1 +kind: OperatorGroup +metadata: + name: operator-policy-testns-abcdefg +spec: + targetNamespaces: + - operator-policy-testns +--- +apiVersion: operators.coreos.com/v1alpha1 +kind: Subscription +metadata: + name: argocd-operator +spec: + name: argocd-operator + installPlanApproval: Manual + channel: alpha + source: operatorhubio-catalog + sourceNamespace: olm + startingCSV: "argocd-operator.v0.9.1" +--- +apiVersion: operators.coreos.com/v1alpha1 +kind: Subscription +metadata: + name: strimzi-kafka-operator +spec: + name: strimzi-kafka-operator + installPlanApproval: Manual + channel: stable + source: operatorhubio-catalog + sourceNamespace: olm + startingCSV: "strimzi-cluster-operator.v0.35.0" diff --git a/test/resources/case38_operator_install/operator-policy-argocd.yaml b/test/resources/case38_operator_install/operator-policy-argocd.yaml new file mode 100644 index 00000000..b836c66d --- /dev/null +++ b/test/resources/case38_operator_install/operator-policy-argocd.yaml @@ -0,0 +1,24 @@ +apiVersion: policy.open-cluster-management.io/v1beta1 +kind: OperatorPolicy +metadata: + name: argocd-operator + annotations: + policy.open-cluster-management.io/parent-policy-compliance-db-id: "124" + policy.open-cluster-management.io/policy-compliance-db-id: "64" + ownerReferences: + - apiVersion: policy.open-cluster-management.io/v1 + kind: Policy + name: parent-policy + uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation +spec: + remediationAction: enforce + severity: medium + complianceType: musthave + subscription: + name: argocd-operator + namespace: operator-policy-testns + channel: alpha + startingCSV: "argocd-operator.v0.9.1" + source: operatorhubio-catalog + sourceNamespace: olm + upgradeApproval: None diff --git a/test/resources/case38_operator_install/operator-policy-strimzi-kafka-operator.yaml b/test/resources/case38_operator_install/operator-policy-strimzi-kafka-operator.yaml new file mode 100644 index 00000000..55eebbb9 --- /dev/null +++ b/test/resources/case38_operator_install/operator-policy-strimzi-kafka-operator.yaml @@ -0,0 +1,24 @@ +apiVersion: policy.open-cluster-management.io/v1beta1 +kind: OperatorPolicy +metadata: + name: strimzi-kafka-operator + annotations: + policy.open-cluster-management.io/parent-policy-compliance-db-id: "124" + policy.open-cluster-management.io/policy-compliance-db-id: "64" + ownerReferences: + - apiVersion: policy.open-cluster-management.io/v1 + kind: Policy + name: parent-policy + uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation +spec: + remediationAction: enforce + severity: medium + complianceType: musthave + subscription: + name: strimzi-kafka-operator + namespace: operator-policy-testns + channel: stable + startingCSV: "strimzi-cluster-operator.v0.35.0" + source: operatorhubio-catalog + sourceNamespace: olm + upgradeApproval: None