Merge pull request #1203 from rfredette/ocpbugs-53424-multiple-installplans

OCPBUGS-53424: Wait for install plans to enter the "Requires Approval" phase before approving them

Author: openshift-merge-bot[bot]

pkg/operator/controller/gatewayclass/controller.go

@@ -81,13 +81,14 @@ func NewUnmanaged(mgr manager.Manager, config Config) (controller.Controller, er
 		}
 		return false
 	})
-	// Check the approved status of an InstallPlan. The ingress operator only needs to potentially move install plans
-	// from Approved=false to Approved=true, so we can filter out all approved plans at the Watch() level.
-	isInstallPlanApproved := predicate.NewPredicateFuncs(func(o client.Object) bool {
+	// Check if an InstallPlan is ready for approval. This requires that both the spec.approved field is false and that
+	// the status.phase is "RequiresApproval" to make sure OLM is done modifying the InstallPlan before it can be
+	// approved.
+	isInstallPlanReadyForApproval := predicate.NewPredicateFuncs(func(o client.Object) bool {
 		installPlan := o.(*operatorsv1alpha1.InstallPlan)
-		return installPlan.Spec.Approved
+		return !installPlan.Spec.Approved && installPlan.Status.Phase == operatorsv1alpha1.InstallPlanPhaseRequiresApproval
 	})
-	if err := c.Watch(source.Kind[client.Object](operatorCache, &operatorsv1alpha1.InstallPlan{}, reconciler.enqueueRequestForSomeGatewayClass(), isOurInstallPlan, predicate.Not(isInstallPlanApproved))); err != nil {
+	if err := c.Watch(source.Kind[client.Object](operatorCache, &operatorsv1alpha1.InstallPlan{}, reconciler.enqueueRequestForSomeGatewayClass(), isOurInstallPlan, isInstallPlanReadyForApproval)); err != nil {
 		return nil, err
 	}
 

pkg/operator/controller/gatewayclass/subscription.go

@@ -158,22 +158,45 @@ func (r *reconciler) currentInstallPlan(ctx context.Context) (bool, *operatorsv1
 	if installPlans == nil || len(installPlans.Items) == 0 {
 		return false, nil, nil
 	}
+	var currentInstallPlan *operatorsv1alpha1.InstallPlan
+	multipleInstallPlans := false
 	for _, installPlan := range installPlans.Items {
 		if len(installPlan.OwnerReferences) == 0 || len(installPlan.Spec.ClusterServiceVersionNames) == 0 {
 			continue
 		}
+		ownerRefMatches := false
 		for _, ownerRef := range installPlan.OwnerReferences {
 			if ownerRef.UID == subscription.UID {
-				for _, csvName := range installPlan.Spec.ClusterServiceVersionNames {
-					if csvName == r.config.GatewayAPIOperatorVersion {
-						return true, &installPlan, nil
-					}
+				ownerRefMatches = true
+				break
+			}
+		}
+		if !ownerRefMatches {
+			continue
+		}
+		// Ignore InstallPlans not in the "RequiresApproval" state. OLM may not be done setting them up.
+		if installPlan.Status.Phase != operatorsv1alpha1.InstallPlanPhaseRequiresApproval {
+			continue
+		}
+		for _, csvName := range installPlan.Spec.ClusterServiceVersionNames {
+			if csvName == r.config.GatewayAPIOperatorVersion {
+				// Keep the newest InstallPlan to return at the end of the loop.
+				if currentInstallPlan == nil {
+					currentInstallPlan = &installPlan
+					break
+				}
+				multipleInstallPlans = true
+				if currentInstallPlan.ObjectMeta.CreationTimestamp.Before(&installPlan.ObjectMeta.CreationTimestamp) {
+					currentInstallPlan = &installPlan
+					break
 				}
 			}
 		}
 	}
-	// No valid InstallPlan found.
-	return false, nil, nil
+	if multipleInstallPlans {
+		log.Info(fmt.Sprintf("found multiple valid InstallPlans. using %s because it's the newest", currentInstallPlan.Name))
+	}
+	return (currentInstallPlan != nil), currentInstallPlan, nil
 }
 
 // desiredInstallPlan returns a version of the expected InstallPlan that is approved.