diff --git a/pkg/controller/registry/resolver/evolver.go b/pkg/controller/registry/resolver/evolver.go index 28b8e35aec..95099e13bb 100644 --- a/pkg/controller/registry/resolver/evolver.go +++ b/pkg/controller/registry/resolver/evolver.go @@ -51,8 +51,13 @@ func (e *NamespaceGenerationEvolver) Evolve(add map[OperatorSourceInfo]struct{}) } func (e *NamespaceGenerationEvolver) checkForUpdates() error { + // maps the old operator identifier to the new operator + updates := EmptyOperatorSet() + // take a snapshot of the current generation so that we don't update the same operator twice in one resolution - for _, op := range e.gen.Operators().Snapshot() { + snapshot := e.gen.Operators().Snapshot() + + for _, op := range snapshot { // only check for updates if we have sourceinfo if op.SourceInfo() == &ExistingOperator { continue @@ -68,11 +73,21 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error { return errors.Wrap(err, "error parsing bundle") } o.SetReplaces(op.Identifier()) - if err := e.gen.AddOperator(o); err != nil { + updates[op.Identifier()] = o + } + + // remove any operators we found updates for + for old := range updates { + e.gen.RemoveOperator(e.gen.Operators().Snapshot()[old]) + } + + // add the new operators we found + for _, new := range updates { + if err := e.gen.AddOperator(new); err != nil { return errors.Wrap(err, "error calculating generation changes due to new bundle") } - e.gen.RemoveOperator(op) } + return nil } diff --git a/pkg/controller/registry/resolver/evolver_test.go b/pkg/controller/registry/resolver/evolver_test.go index ad810e3291..be6316d757 100644 --- a/pkg/controller/registry/resolver/evolver_test.go +++ b/pkg/controller/registry/resolver/evolver_test.go @@ -411,6 +411,46 @@ func TestNamespaceGenerationEvolver(t *testing.T) { "original"), ), }, + { + name: "OwnershipTransfer", + fields: fields{ + querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{ + CatalogKey{"catsrc", "catsrc-namespace"}: { + bundle("depender.v2", "depender", "channel", "depender.v1", + APISet{opregistry.APIKey{"g", "v", "k", "ks"}: {}}, EmptyAPISet(), EmptyAPISet(), EmptyAPISet()), + bundle("original.v2", "o", "c", "original", EmptyAPISet(), EmptyAPISet(), EmptyAPISet(), EmptyAPISet()), + }, + }), + gen: NewGenerationFromOperators( + NewFakeOperatorSurface("original", "o", "c", "", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil, nil), + NewFakeOperatorSurface("depender.v1", "depender", "channel", "", "catsrc", "", nil, []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil), + ), + }, + args: args{}, + wantGen: NewGenerationFromOperators( + NewFakeOperatorSurface("original.v2", "o", "c", "original", "catsrc", "", nil, nil, nil, nil), + NewFakeOperatorSurface("depender.v2", "depender", "channel", "depender.v1", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil, nil), + ), + }, + { + name: "PicksOlderProvider", + fields: fields{ + querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{ + CatalogKey{"catsrc", "catsrc-namespace"}: { + bundle("original", "o", "c", "", APISet{opregistry.APIKey{"g", "v", "k", "ks"}: {}}, EmptyAPISet(), EmptyAPISet(), EmptyAPISet()), + bundle("original.v2", "o", "c", "original", EmptyAPISet(), EmptyAPISet(), EmptyAPISet(), EmptyAPISet()), + }, + }), + gen: NewGenerationFromOperators( + NewFakeOperatorSurface("depender.v1", "depender", "channel", "", "catsrc", "", nil, []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil), + ), + }, + args: args{}, + wantGen: NewGenerationFromOperators( + NewFakeOperatorSurface("original", "o", "c", "", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}},nil, nil, nil), + NewFakeOperatorSurface("depender.v1", "depender", "channel", "", "catsrc", "", nil,[]opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil), + ), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/controller/registry/resolver/resolver_test.go b/pkg/controller/registry/resolver/resolver_test.go index 66e2d30269..c6b3559627 100644 --- a/pkg/controller/registry/resolver/resolver_test.go +++ b/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, @@ -409,6 +409,55 @@ func TestNamespaceResolver(t *testing.T) { }, }, }, + { + // This test verifies that ownership of an api can be migrated between two operators + name: "OwnedAPITransfer", + clusterState: []runtime.Object{ + existingSub(namespace, "a.v1", "a", "alpha", catalog), + existingOperator(namespace, "a.v1", "a", "alpha", "", Provides1, nil, nil, nil), + existingSub(namespace, "b.v1", "b", "alpha", catalog), + existingOperator(namespace, "b.v1", "b", "alpha", "", nil, Requires1, nil, nil), + }, + querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{ + catalog: { + bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil), + bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil), + }, + }), + out: out{ + steps: [][]*v1alpha1.Step{ + bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil), namespace, "", catalog), + bundleSteps(bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog), + }, + subs: []*v1alpha1.Subscription{ + updatedSub(namespace, "a.v2", "a", "alpha", catalog), + updatedSub(namespace, "b.v2", "b", "alpha", catalog), + }, + }, + }, + { + name: "PicksOlderProvider", + clusterState: []runtime.Object{ + newSub(namespace, "b", "alpha", catalog), + }, + querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{ + catalog: { + bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil), + bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil), + bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil), + }, + }), + out: out{ + steps: [][]*v1alpha1.Step{ + bundleSteps(bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil), namespace, "", catalog), + bundleSteps(bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil), namespace, "", catalog), + subSteps(namespace, "a.v1", "a", "alpha", catalog), + }, + subs: []*v1alpha1.Subscription{ + updatedSub(namespace, "b.v1", "b", "alpha", catalog), + }, + }, + }, { name: "InstalledSub/UpdateInHead/SkipRange", clusterState: []runtime.Object{ @@ -425,6 +474,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) { diff --git a/test/e2e/catalog_e2e_test.go b/test/e2e/catalog_e2e_test.go index 8de7491bb7..0391157efb 100644 --- a/test/e2e/catalog_e2e_test.go +++ b/test/e2e/catalog_e2e_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net" "reflect" + "strings" "testing" "time" @@ -941,6 +942,116 @@ func TestCatalogImageUpdate(t *testing.T) { } } +func TestDependencySkipRange(t *testing.T) { + // 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(t, 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(t) + source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(source) + require.NoError(t, err) + defer func() { + require.NoError(t, crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Delete(source.GetName(), &metav1.DeleteOptions{})) + }() + + // Create a Subscription for busybox + subscriptionName := genName("sub-") + cleanupSubscription := createSubscriptionForCatalog(t, crc, source.GetNamespace(), subscriptionName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic) + defer cleanupSubscription() + + // Wait for the Subscription to succeed + subscription, err := fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker) + require.NoError(t, err) + require.NotNil(t, subscription) + require.Equal(t, subscription.Status.InstalledCSV, "busybox.v1.0.0") + + // Confirm that a subscription was created for busybox-dependency + subscriptionList, err := crc.OperatorsV1alpha1().Subscriptions(source.GetNamespace()).List(metav1.ListOptions{}) + require.NoError(t, err) + dependencySubscriptionName := "" + for _, sub := range subscriptionList.Items { + if strings.HasPrefix(sub.GetName(), "busybox-dependency") { + dependencySubscriptionName = sub.GetName() + } + } + + require.NotEmpty(t, dependencySubscriptionName) + // Wait for the Subscription to succeed + subscription, err = fetchSubscription(t, crc, testNamespace, dependencySubscriptionName, subscriptionStateAtLatestChecker) + require.NoError(t, err) + require.NotNil(t, subscription) + require.Equal(t, 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(sourceName, metav1.GetOptions{}) + if err != nil { + return false, err + } + existingSource.Spec.Image = catSrcImage + ":2.0.0" + + source, err = crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Update(existingSource) + if err == nil { + return true, nil + } + return false, nil + }) + require.NoError(t, 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(t, crc, testNamespace, subscriptionName, subChecker) + require.NoError(t, err) + require.NotNil(t, subscription) + + // Wait for busybox v2 csv to succeed and check the replaces field + csv, err := fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker) + require.NoError(t, err) + require.Equal(t, "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(t, crc, testNamespace, dependencySubscriptionName, subChecker) + require.NoError(t, err) + require.NotNil(t, subscription) + + // Wait for busybox-dependency v2 csv to succeed and check the replaces field + csv, err = fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker) + require.NoError(t, err) + require.Equal(t, "busybox-dependency.v1.0.0", csv.Spec.Replaces) +} + func getOperatorDeployment(c operatorclient.ClientInterface, namespace string, operatorLabels labels.Set) (*appsv1.Deployment, error) { deployments, err := c.ListDeploymentsWithLabels(namespace, operatorLabels) if err != nil || deployments == nil || len(deployments.Items) != 1 {