Fix Operator Generation code

Problem:
If an operator is being upgraded that provides a required API whose GVK
has not changed since the previous version of the operator and
uses a skipRange instead of the Spec.Replaces field, OLM will:
* Add the new operator to the generation, and marking the APIs it
  provides as "present".
* Remove the old operator from the generation, marking the APIs it
  provides as "absent", despite being provided by the new version of the
  operator.
* Attempt to resolve the "missing" APIs, overwriting the new version
  of the operator with a copy that  does not have its Spec.Replaces field
  set.

This causes OLM to fail the upgrade, where the old operator's CSV will
not be replaced and the new operators CSV will run into an intercepting
API Provider issue.

Solution:
Update OLM to remove the old operator from the current generation before
adding the new operator to the generation.

Author: awgreene

pkg/controller/registry/resolver/evolver.go

@@ -68,10 +68,14 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error {
 			return errors.Wrap(err, "error parsing bundle")
 		}
 		o.SetReplaces(op.Identifier())
+
+		// Remove the old operator and the APIs it provides before adding the new operator to the generation.
+		e.gen.RemoveOperator(op)
+
+		// Add the new operator and the APIs it provides to the generation.
 		if err := e.gen.AddOperator(o); err != nil {
 			return errors.Wrap(err, "error calculating generation changes due to new bundle")
 		}
-		e.gen.RemoveOperator(op)
 	}
 	return nil
 }

pkg/controller/registry/resolver/resolver_test.go

@@ -120,7 +120,7 @@ func TestNamespaceResolver(t *testing.T) {
 				},
 				lookups: []v1alpha1.BundleLookup{
 					{
-						Path: "quay.io/test/bundle@sha256:abcd",
+						Path:       "quay.io/test/bundle@sha256:abcd",
 						Identifier: "b.v1",
 						CatalogSourceRef: &corev1.ObjectReference{
 							Namespace: catalog.Namespace,
@@ -234,9 +234,9 @@ func TestNamespaceResolver(t *testing.T) {
 				steps: [][]*v1alpha1.Step{},
 				lookups: []v1alpha1.BundleLookup{
 					{
-						Path:     "quay.io/test/bundle@sha256:abcd",
+						Path:       "quay.io/test/bundle@sha256:abcd",
 						Identifier: "a.v2",
-						Replaces: "a.v1",
+						Replaces:   "a.v1",
 						CatalogSourceRef: &corev1.ObjectReference{
 							Namespace: catalog.Namespace,
 							Name:      catalog.Name,
@@ -425,6 +425,43 @@ func TestNamespaceResolver(t *testing.T) {
 				},
 			},
 		},
+		{
+			// This test uses logic that implements the FakeSourceQuerier to ensure
+			// that the required API is provided by the new Operator.
+			//
+			// Background:
+			// OLM used to add the new operator to the generation before removing
+			// the old operator from the generation. The logic that removes an operator
+			// from the current generation removes the APIs it provides from the list of
+			// "available" APIs. This caused OLM to search for an operator that provides the API.
+			// If the operator that provides the API uses a skipRange rather than the Spec.Replaces
+			// field, the Replaces field is set to an empty string, causing OLM to fail to upgrade.
+			name: "InstalledSubs/ExistingOperators/OldCSVsReplaced",
+			clusterState: []runtime.Object{
+				existingSub(namespace, "a.v1", "a", "alpha", catalog),
+				existingSub(namespace, "b.v1", "b", "beta", catalog),
+				existingOperator(namespace, "a.v1", "a", "alpha", "", nil, Requires1, nil, nil),
+				existingOperator(namespace, "b.v1", "b", "beta", "", Provides1, nil, nil, nil),
+			},
+			querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
+				catalog: {
+					bundle("a.v1", "a", "alpha", "", nil, nil, nil, nil),
+					bundle("a.v2", "a", "alpha", "a.v1", nil, Requires1, nil, nil),
+					bundle("b.v1", "b", "beta", "", Provides1, nil, nil, nil),
+					bundle("b.v2", "b", "beta", "b.v1", Provides1, nil, nil, nil),
+				},
+			}),
+			out: out{
+				steps: [][]*v1alpha1.Step{
+					bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, Requires1, nil, nil), namespace, "", catalog),
+					bundleSteps(bundle("b.v2", "b", "beta", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog),
+				},
+				subs: []*v1alpha1.Subscription{
+					updatedSub(namespace, "a.v2", "a", "alpha", catalog),
+					updatedSub(namespace, "b.v2", "b", "beta", catalog),
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

test/e2e/catalog_e2e_test.go

@@ -7,6 +7,7 @@ import (
 	"net"
 	"reflect"
 	"testing"
+	"strings"
 	"time"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/version"
@@ -923,6 +924,117 @@ func TestCatalogImageUpdate(t *testing.T) {
 	subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subChecker)
 	require.NoError(t, err)
 	require.NotNil(t, subscription)
+	})
+	It("Dependency has correct replaces field", func() {
+		// Create a CatalogSource that contains the busybox v1 and busybox-dependency v1 images
+		// Create a Subscription for busybox v1, which has a dependency on busybox-dependency v1.
+		// Wait for the busybox and busybox2 Subscriptions to succeed
+		// Wait for the CSVs to succeed
+		// Update the catalog to point to an image that contains the busybox v2 and busybox-dependency v2 images.
+		// Wait for the new Subscriptions to succeed and check if they include the new CSVs
+		// Wait for the CSVs to succeed and confirm that the have the correct Spec.Replaces fields.
+		defer cleaner.NotifyTestComplete(GinkgoT(), true)
+
+		sourceName := genName("catalog-")
+		packageName := "busybox"
+		channelName := "alpha"
+
+		catSrcImage := "quay.io/olmtest/busybox-dependencies-index"
+
+		// Create gRPC CatalogSource
+		source := &v1alpha1.CatalogSource{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       v1alpha1.CatalogSourceKind,
+				APIVersion: v1alpha1.CatalogSourceCRDAPIVersion,
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      sourceName,
+				Namespace: testNamespace,
+			},
+			Spec: v1alpha1.CatalogSourceSpec{
+				SourceType: v1alpha1.SourceTypeGrpc,
+				Image:      catSrcImage + ":1.0.0",
+			},
+		}
+
+		crc := newCRClient(GinkgoT())
+		source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.TODO(), source, metav1.CreateOptions{})
+		require.NoError(GinkgoT(), err)
+		defer func() {
+			require.NoError(GinkgoT(), crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Delete(context.TODO(), source.GetName(), metav1.DeleteOptions{}))
+		}()
+
+		// Create a Subscription for busybox
+		subscriptionName := genName("sub-")
+		cleanupSubscription := createSubscriptionForCatalog(GinkgoT(), crc, source.GetNamespace(), subscriptionName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic)
+		defer cleanupSubscription()
+
+		// Wait for the Subscription to succeed
+		subscription, err := fetchSubscription(GinkgoT(), crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker)
+		require.NoError(GinkgoT(), err)
+		require.NotNil(GinkgoT(), subscription)
+		require.Equal(GinkgoT(), subscription.Status.InstalledCSV, "busybox.v1.0.0")
+
+		// Confirm that a subscription was created for busybox-dependency
+		subscriptionList, err := crc.OperatorsV1alpha1().Subscriptions(source.GetNamespace()).List(context.TODO(), metav1.ListOptions{})
+		require.NoError(GinkgoT(), err)
+		dependencySubscriptionName := ""
+		for _, sub := range subscriptionList.Items {
+			if strings.HasPrefix(sub.GetName(), "busybox-dependency") {
+				dependencySubscriptionName = sub.GetName()
+			}
+		}
+
+		require.NotEmpty(GinkgoT(), dependencySubscriptionName)
+		// Wait for the Subscription to succeed
+		subscription, err = fetchSubscription(GinkgoT(), crc, testNamespace, dependencySubscriptionName, subscriptionStateAtLatestChecker)
+		require.NoError(GinkgoT(), err)
+		require.NotNil(GinkgoT(), subscription)
+		require.Equal(GinkgoT(), subscription.Status.InstalledCSV, "busybox-dependency.v1.0.0")
+
+		// Update the catalog image
+		err = wait.PollImmediate(pollInterval, pollDuration, func() (bool, error) {
+			existingSource, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(context.TODO(), sourceName, metav1.GetOptions{})
+			if err != nil {
+				return false, err
+			}
+			existingSource.Spec.Image = catSrcImage + ":2.0.0"
+
+			source, err = crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Update(context.TODO(), existingSource, metav1.UpdateOptions{})
+			if err == nil {
+				return true, nil
+			}
+			return false, nil
+		})
+		require.NoError(GinkgoT(), err)
+
+		// Wait for the busybox v2 Subscription to succeed
+		subChecker := func(sub *v1alpha1.Subscription) bool {
+			return sub.Status.InstalledCSV == "busybox.v2.0.0"
+		}
+		subscription, err = fetchSubscription(GinkgoT(), crc, testNamespace, subscriptionName, subChecker)
+		require.NoError(GinkgoT(), err)
+		require.NotNil(GinkgoT(), subscription)
+
+		// Wait for busybox v2 csv to succeed and check the replaces field
+		csv, err := fetchCSV(GinkgoT(), crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
+		require.NoError(GinkgoT(), err)
+		require.Equal(GinkgoT(), "busybox.v1.0.0", csv.Spec.Replaces)
+
+		// Wait for the busybox-dependency v2 Subscription to succeed
+		subChecker = func(sub *v1alpha1.Subscription) bool {
+			return sub.Status.InstalledCSV == "busybox-dependency.v2.0.0"
+		}
+		subscription, err = fetchSubscription(GinkgoT(), crc, testNamespace, dependencySubscriptionName, subChecker)
+		require.NoError(GinkgoT(), err)
+		require.NotNil(GinkgoT(), subscription)
+
+		// Wait for busybox-dependency v2 csv to succeed and check the replaces field
+		csv, err = fetchCSV(GinkgoT(), crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
+		require.NoError(GinkgoT(), err)
+		require.Equal(GinkgoT(), "busybox-dependency.v1.0.0", csv.Spec.Replaces)
+	})
+})
 
 	// Wait for csv to succeed
 	csv, err := fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)