[v1.22.x] (bugfix): approve InstallPlan only if it is a new InstallPlan (#5924)

* fix installplan approval bug

Signed-off-by: Bryce Palmer <[email protected]>

* add changelog

Signed-off-by: Bryce Palmer <[email protected]>

* fix sanity issues

Signed-off-by: Bryce Palmer <[email protected]>

Co-authored-by: Bryce Palmer <[email protected]>

Author: openshift-cherrypick-robot

changelog/fragments/rbu-ip-approval-bugfix.yaml

@@ -0,0 +1,17 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      For `operator-sdk run bundle-upgrade`: fix a bug that caused `InstallPlan`s occasionally not being approved when attempting to upgrade a bundle.  
+
+    # kind is one of:
+    # - addition
+    # - change
+    # - deprecation
+    # - removal
+    # - bugfix
+    kind: "bugfix"
+
+    # Is this a breaking change?
+    breaking: false
+

internal/olm/operator/registry/operator_installer.go

@@ -31,6 +31,8 @@ import (
 
 	olmclient "github.com/operator-framework/operator-sdk/internal/olm/client"
 	"github.com/operator-framework/operator-sdk/internal/olm/operator"
+
+	corev1 "k8s.io/api/core/v1"
 )
 
 type OperatorInstaller struct {
@@ -345,11 +347,17 @@ func (o OperatorInstaller) waitForInstallPlan(ctx context.Context, sub *v1alpha1
 		Name:      sub.GetName(),
 	}
 
+	// Get the previous InstallPlanRef
+	prevIPRef := corev1.ObjectReference{}
+	if sub.Status.InstallPlanRef != nil {
+		prevIPRef = *sub.Status.InstallPlanRef
+	}
+
 	ipCheck := wait.ConditionFunc(func() (done bool, err error) {
 		if err := o.cfg.Client.Get(ctx, subKey, sub); err != nil {
 			return false, err
 		}
-		if sub.Status.InstallPlanRef != nil {
+		if sub.Status.InstallPlanRef != nil && sub.Status.InstallPlanRef.Name != prevIPRef.Name {
 			return true, nil
 		}
 		return false, nil

internal/olm/operator/registry/operator_installer_test.go

@@ -32,6 +32,9 @@ import (
 	"github.com/operator-framework/operator-sdk/internal/olm/operator"
 )
 
+const name = "fakeName"
+const namespace = "fakeNS"
+
 var _ = Describe("OperatorInstaller", func() {
 	Describe("NewOperatorInstaller", func() {
 		It("should create an OperatorInstaller", func() {
@@ -161,47 +164,47 @@ var _ = Describe("OperatorInstaller", func() {
 			oi.cfg.Client = fake.NewClientBuilder().WithScheme(sch).WithObjects(
 				&v1alpha1.InstallPlan{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "fakeName",
-						Namespace: "fakeNS",
+						Name:      name,
+						Namespace: namespace,
 					},
 				},
 			).Build()
 
 			ip := &v1alpha1.InstallPlan{}
 			ipKey := types.NamespacedName{
-				Namespace: "fakeNS",
-				Name:      "fakeName",
+				Namespace: namespace,
+				Name:      name,
 			}
 
 			err := oi.cfg.Client.Get(context.TODO(), ipKey, ip)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(ip.Name).To(Equal("fakeName"))
-			Expect(ip.Namespace).To(Equal("fakeNS"))
+			Expect(ip.Name).To(Equal(name))
+			Expect(ip.Namespace).To(Equal(namespace))
 
 			// Test
 			sub := &v1alpha1.Subscription{
 				Status: v1alpha1.SubscriptionStatus{
 					InstallPlanRef: &corev1.ObjectReference{
-						Name:      "fakeName",
-						Namespace: "fakeNS",
+						Name:      name,
+						Namespace: namespace,
 					},
 				},
 			}
 			err = oi.approveInstallPlan(context.TODO(), sub)
 			Expect(err).ToNot(HaveOccurred())
 			err = oi.cfg.Client.Get(context.TODO(), ipKey, ip)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(ip.Name).To(Equal("fakeName"))
-			Expect(ip.Namespace).To(Equal("fakeNS"))
+			Expect(ip.Name).To(Equal(name))
+			Expect(ip.Namespace).To(Equal(namespace))
 			Expect(ip.Spec.Approved).To(Equal(true))
 		})
 		It("should return an error if the install plan does not exist.", func() {
 			oi.cfg.Client = fake.NewClientBuilder().WithScheme(sch).Build()
 			sub := &v1alpha1.Subscription{
 				Status: v1alpha1.SubscriptionStatus{
 					InstallPlanRef: &corev1.ObjectReference{
-						Name:      "fakeName",
-						Namespace: "fakeNS",
+						Name:      name,
+						Namespace: namespace,
 					},
 				},
 			}
@@ -224,8 +227,8 @@ var _ = Describe("OperatorInstaller", func() {
 			cfg.Client = fake.NewClientBuilder().WithScheme(sch).Build()
 
 			oi = NewOperatorInstaller(cfg)
-			oi.StartingCSV = "fakeName"
-			oi.cfg.Namespace = "fakeNS"
+			oi.StartingCSV = name
+			oi.cfg.Namespace = namespace
 		})
 		It("should return an error if the subscription does not exist.", func() {
 			sub := newSubscription(oi.StartingCSV, oi.cfg.Namespace, withCatalogSource("duplicate", oi.cfg.Namespace))
@@ -235,23 +238,66 @@ var _ = Describe("OperatorInstaller", func() {
 			Expect(err.Error()).Should(ContainSubstring("install plan is not available for the subscription"))
 
 		})
-		It("should return if subscription has an install plan.", func() {
+		It("should return if subscription has an install plan and previous install plan is nil", func() {
+			name := name
+			namespace := namespace
+			prevSub := &v1alpha1.Subscription{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      name,
+					Namespace: namespace,
+				},
+			}
+
+			sub := &v1alpha1.Subscription{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      name,
+					Namespace: namespace,
+				},
+				Status: v1alpha1.SubscriptionStatus{
+					InstallPlanRef: &corev1.ObjectReference{
+						Name:      name,
+						Namespace: namespace,
+					},
+				},
+			}
+			err := oi.cfg.Client.Create(context.TODO(), sub)
+			Expect(err).ToNot(HaveOccurred())
+
+			err = oi.waitForInstallPlan(context.TODO(), prevSub)
+			Expect(err).ToNot(HaveOccurred())
+		})
+		It("should return if subscription has an install plan and is different than previous install plan", func() {
+			name := name
+			namespace := namespace
+			prevSub := &v1alpha1.Subscription{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      name,
+					Namespace: namespace,
+				},
+				Status: v1alpha1.SubscriptionStatus{
+					InstallPlanRef: &corev1.ObjectReference{
+						Name:      name + "diff",
+						Namespace: namespace + "diff",
+					},
+				},
+			}
+
 			sub := &v1alpha1.Subscription{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "fakeName",
-					Namespace: "fakeNS",
+					Name:      name,
+					Namespace: namespace,
 				},
 				Status: v1alpha1.SubscriptionStatus{
 					InstallPlanRef: &corev1.ObjectReference{
-						Name:      "fakeName",
-						Namespace: "fakeNS",
+						Name:      name,
+						Namespace: namespace,
 					},
 				},
 			}
 			err := oi.cfg.Client.Create(context.TODO(), sub)
 			Expect(err).ToNot(HaveOccurred())
 
-			err = oi.waitForInstallPlan(context.TODO(), sub)
+			err = oi.waitForInstallPlan(context.TODO(), prevSub)
 			Expect(err).ToNot(HaveOccurred())
 		})
 	})
@@ -550,7 +596,7 @@ var _ = Describe("OperatorInstaller", func() {
 			Expect(err).To(HaveOccurred())
 		})
 		It("should return nothing if namespace does not match", func() {
-			oi.cfg.Namespace = "fakens"
+			oi.cfg.Namespace = namespace
 			_ = createOperatorGroupHelper(context.TODO(), client, "og1", "atestns")
 			grp, found, err := oi.getOperatorGroup(context.TODO())
 			Expect(grp).To(BeNil())