From fa4b13b97ecbd5ece5caf4c4a7a36cc9e3364eba Mon Sep 17 00:00:00 2001 From: awgreene Date: Sat, 25 Apr 2020 14:59:20 -0400 Subject: [PATCH 1/2] 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. --- pkg/controller/registry/resolver/evolver.go | 6 +- .../registry/resolver/resolver_test.go | 43 ++++++- test/e2e/catalog_e2e_test.go | 111 ++++++++++++++++++ 3 files changed, 156 insertions(+), 4 deletions(-) diff --git a/pkg/controller/registry/resolver/evolver.go b/pkg/controller/registry/resolver/evolver.go index 28b8e35aec..a023ff42a7 100644 --- a/pkg/controller/registry/resolver/evolver.go +++ b/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 } diff --git a/pkg/controller/registry/resolver/resolver_test.go b/pkg/controller/registry/resolver/resolver_test.go index 66e2d30269..69d865a596 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, @@ -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) { 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 { From e11d5665f59fb1051dbf15045307e91c3bc86283 Mon Sep 17 00:00:00 2001 From: Evan Date: Fri, 24 Apr 2020 13:13:43 -0400 Subject: [PATCH 2/2] fix(resolver): Transferring API Ownership Problem: In an upgrade where v1 of Operator A provides an API and v1 of Operator B depends on the API, where the ownership of the API is transferred from Operator A to Operator B during the upgrade, OLM may run into a situation where Operator B is updated first. This causes OLM to fail to calculate the generation because multiple operator provide the same API. Solution: Add and remove updates as a set to prevent the situation described above. --- pkg/controller/registry/resolver/evolver.go | 21 ++++++-- .../registry/resolver/evolver_test.go | 40 +++++++++++++++ .../registry/resolver/resolver_test.go | 49 +++++++++++++++++++ 3 files changed, 105 insertions(+), 5 deletions(-) diff --git a/pkg/controller/registry/resolver/evolver.go b/pkg/controller/registry/resolver/evolver.go index a023ff42a7..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,15 +73,21 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error { return errors.Wrap(err, "error parsing bundle") } o.SetReplaces(op.Identifier()) + updates[op.Identifier()] = o + } - // Remove the old operator and the APIs it provides before adding the new operator to the generation. - e.gen.RemoveOperator(op) + // remove any operators we found updates for + for old := range updates { + e.gen.RemoveOperator(e.gen.Operators().Snapshot()[old]) + } - // Add the new operator and the APIs it provides to the generation. - if err := e.gen.AddOperator(o); err != nil { + // 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") } } + 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 69d865a596..c6b3559627 100644 --- a/pkg/controller/registry/resolver/resolver_test.go +++ b/pkg/controller/registry/resolver/resolver_test.go @@ -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{