From b9d242037a3b4d66384060555b5fb4269ca4779e Mon Sep 17 00:00:00 2001 From: Ryan Fredette Date: Mon, 24 Mar 2025 14:42:31 -0400 Subject: [PATCH] Wait for install plans to enter the "Requires Approval" phase before approving them. Waiting until that point guarantees that there won't be simultaneous updates to the install plan, which can cause the install process to fail. Also, if multiple install plans exist for the same OSSM version, approve the most recently created one. This is part of the fix for OCPBUGS-53424 --- .../controller/gatewayclass/controller.go | 11 +- .../controller/gatewayclass/subscription.go | 35 ++- .../gatewayclass/subscription_test.go | 291 ++++++++++++++++++ 3 files changed, 326 insertions(+), 11 deletions(-) diff --git a/pkg/operator/controller/gatewayclass/controller.go b/pkg/operator/controller/gatewayclass/controller.go index 6113a44d04..e4918163d3 100644 --- a/pkg/operator/controller/gatewayclass/controller.go +++ b/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 } diff --git a/pkg/operator/controller/gatewayclass/subscription.go b/pkg/operator/controller/gatewayclass/subscription.go index 22ed7e09fa..2558436dd7 100644 --- a/pkg/operator/controller/gatewayclass/subscription.go +++ b/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. diff --git a/pkg/operator/controller/gatewayclass/subscription_test.go b/pkg/operator/controller/gatewayclass/subscription_test.go index 84693795c9..22eaf6d62b 100644 --- a/pkg/operator/controller/gatewayclass/subscription_test.go +++ b/pkg/operator/controller/gatewayclass/subscription_test.go @@ -3,6 +3,7 @@ package gatewayclass import ( "context" "testing" + "time" configv1 "github.com/openshift/api/config/v1" operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" @@ -40,6 +41,48 @@ func Test_ensureServiceMeshOperatorInstallPlan(t *testing.T) { expectUpdate: []client.Object{}, expectDelete: []client.Object{}, }, + { + // InstallPlan exists but is not yet approved but phase is incorrect. No changes expected. + name: "InstallPlan not approved with incorrect phase", + channel: "stable", + version: "servicemeshoperator.v1.0.0", + existingObjects: []runtime.Object{ + &operatorsv1alpha1.Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: operatorcontroller.ServiceMeshOperatorSubscriptionName().Namespace, + Name: operatorcontroller.ServiceMeshOperatorSubscriptionName().Name, + UID: "foobar", + }, + Spec: &operatorsv1alpha1.SubscriptionSpec{ + Channel: "stable", + InstallPlanApproval: operatorsv1alpha1.ApprovalManual, + Package: "servicemeshoperator", + CatalogSource: "redhat-operators", + CatalogSourceNamespace: "openshift-marketplace", + StartingCSV: "servicemeshoperator.v1.0.0", + }, + }, + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: false, + }, + }, + }, + expectCreate: []client.Object{}, + expectUpdate: []client.Object{}, + expectDelete: []client.Object{}, + }, { // InstallPlan exists but is already approved. No changes expected. name: "InstallPlan already approved", @@ -120,6 +163,9 @@ func Test_ensureServiceMeshOperatorInstallPlan(t *testing.T) { Approval: operatorsv1alpha1.ApprovalManual, Approved: false, }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, }, }, expectCreate: []client.Object{}, @@ -141,6 +187,9 @@ func Test_ensureServiceMeshOperatorInstallPlan(t *testing.T) { Approval: operatorsv1alpha1.ApprovalManual, Approved: true, }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, }, }, expectDelete: []client.Object{}, @@ -183,6 +232,9 @@ func Test_ensureServiceMeshOperatorInstallPlan(t *testing.T) { Approval: operatorsv1alpha1.ApprovalManual, Approved: false, }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, }, &operatorsv1alpha1.InstallPlan{ ObjectMeta: metav1.ObjectMeta{ @@ -201,6 +253,9 @@ func Test_ensureServiceMeshOperatorInstallPlan(t *testing.T) { Approval: operatorsv1alpha1.ApprovalManual, Approved: false, }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, }, }, expectCreate: []client.Object{}, @@ -222,6 +277,9 @@ func Test_ensureServiceMeshOperatorInstallPlan(t *testing.T) { Approval: operatorsv1alpha1.ApprovalManual, Approved: true, }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, }, }, expectDelete: []client.Object{}, @@ -264,6 +322,9 @@ func Test_ensureServiceMeshOperatorInstallPlan(t *testing.T) { Approval: operatorsv1alpha1.ApprovalManual, Approved: false, }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, }, }, expectCreate: []client.Object{}, @@ -306,12 +367,242 @@ func Test_ensureServiceMeshOperatorInstallPlan(t *testing.T) { Approval: operatorsv1alpha1.ApprovalManual, Approved: false, }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, }, }, expectCreate: []client.Object{}, expectUpdate: []client.Object{}, expectDelete: []client.Object{}, }, + { + // Multiple InstallPlans exist, all with the correct cluster service version and owner reference. Expect + // InstallPlan.Spec.Approved = true for only the most recent InstallPlan (by creation timestamp). + name: "Multiple valid InstallPlans, should approve latest", + channel: "stable", + version: "servicemeshoperator.v1.0.0", + existingObjects: []runtime.Object{ + &operatorsv1alpha1.Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: operatorcontroller.ServiceMeshOperatorSubscriptionName().Namespace, + Name: operatorcontroller.ServiceMeshOperatorSubscriptionName().Name, + UID: "foobar", + CreationTimestamp: metav1.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC), + }, + Spec: &operatorsv1alpha1.SubscriptionSpec{ + Channel: "stable", + InstallPlanApproval: operatorsv1alpha1.ApprovalManual, + Package: "servicemeshoperator", + CatalogSource: "redhat-operators", + CatalogSourceNamespace: "openshift-marketplace", + StartingCSV: "servicemeshoperator.v1.0.0", + }, + }, + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Install-foo", + Namespace: operatorcontroller.OpenshiftOperatorNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: false, + }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, + }, + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Install-bar", + Namespace: operatorcontroller.OpenshiftOperatorNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + CreationTimestamp: metav1.Date(2025, 1, 1, 2, 30, 0, 0, time.UTC), + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: false, + }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, + }, + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Install-baz", + Namespace: operatorcontroller.OpenshiftOperatorNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + CreationTimestamp: metav1.Date(2024, 12, 31, 23, 59, 0, 0, time.UTC), + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: false, + }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, + }, + }, + expectCreate: []client.Object{}, + expectUpdate: []client.Object{ + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Install-bar", + Namespace: operatorcontroller.OpenshiftOperatorNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + CreationTimestamp: metav1.Date(2025, 1, 1, 2, 30, 0, 0, time.UTC), + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: true, + }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, + }, + }, + expectDelete: []client.Object{}, + }, + { + // Multiple InstallPlans exist, all with the correct cluster service version and owner reference, but only + // one is in the "RequiresApproval" phase. Expect InstallPlan.Spec.Approved = true for only the one in the + // "RequiresApproval" phase. + name: "Multiple unapproved InstallPlans, only one in correct phase", + channel: "stable", + version: "servicemeshoperator.v1.0.0", + existingObjects: []runtime.Object{ + &operatorsv1alpha1.Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: operatorcontroller.ServiceMeshOperatorSubscriptionName().Namespace, + Name: operatorcontroller.ServiceMeshOperatorSubscriptionName().Name, + UID: "foobar", + CreationTimestamp: metav1.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC), + }, + Spec: &operatorsv1alpha1.SubscriptionSpec{ + Channel: "stable", + InstallPlanApproval: operatorsv1alpha1.ApprovalManual, + Package: "servicemeshoperator", + CatalogSource: "redhat-operators", + CatalogSourceNamespace: "openshift-marketplace", + StartingCSV: "servicemeshoperator.v1.0.0", + }, + }, + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Install-foo", + Namespace: operatorcontroller.OpenshiftOperatorNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: false, + }, + }, + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Install-bar", + Namespace: operatorcontroller.OpenshiftOperatorNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + CreationTimestamp: metav1.Date(2025, 1, 1, 2, 30, 0, 0, time.UTC), + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: false, + }, + }, + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Install-baz", + Namespace: operatorcontroller.OpenshiftOperatorNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + CreationTimestamp: metav1.Date(2024, 12, 31, 23, 59, 0, 0, time.UTC), + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: false, + }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, + }, + }, + expectCreate: []client.Object{}, + expectUpdate: []client.Object{ + &operatorsv1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Install-baz", + Namespace: operatorcontroller.OpenshiftOperatorNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + UID: "foobar", + }, + }, + CreationTimestamp: metav1.Date(2024, 12, 31, 23, 59, 0, 0, time.UTC), + }, + Spec: operatorsv1alpha1.InstallPlanSpec{ + ClusterServiceVersionNames: []string{ + "servicemeshoperator.v1.0.0", + }, + Approval: operatorsv1alpha1.ApprovalManual, + Approved: true, + }, + Status: operatorsv1alpha1.InstallPlanStatus{ + Phase: operatorsv1alpha1.InstallPlanPhaseRequiresApproval, + }, + }, + }, + expectDelete: []client.Object{}, + }, } scheme := runtime.NewScheme()