From 44df9674df798f17d19330a28048539edb4349f9 Mon Sep 17 00:00:00 2001 From: kevinrizza Date: Thu, 18 Jun 2020 07:05:16 -0400 Subject: [PATCH] Bug 1847540: resolve only default channels Non deterministic behavior was introduced where, in some cases, the dependency resolver would select a channel entry from the registry that was not part of the default channel and create a subscription on that channel. This bugfix updates the registry client's filter function to ignore non default channel entries. --- pkg/controller/registry/registry_client.go | 52 +++++++++++++++++----- test/e2e/subscription_e2e_test.go | 8 +++- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/pkg/controller/registry/registry_client.go b/pkg/controller/registry/registry_client.go index 9fe3ef9bc3..170f887db2 100644 --- a/pkg/controller/registry/registry_client.go +++ b/pkg/controller/registry/registry_client.go @@ -86,9 +86,9 @@ func (rc *Client) FindBundleThatProvides(ctx context.Context, group, version, ki if err != nil { return nil, err } - entry := rc.filterChannelEntries(it, excludedPkgName) + entry := rc.filterChannelEntries(ctx, it, excludedPkgName) if entry == nil { - return nil, fmt.Errorf("Unable to find a channel entry which doesn't belong to package %s", excludedPkgName) + return nil, fmt.Errorf("Unable to find a channel entry which doesn't belong to package %s's default channel", excludedPkgName) } bundle, err := rc.Client.Registry.GetBundle(ctx, ®istryapi.GetBundleRequest{PkgName: entry.PackageName, ChannelName: entry.ChannelName, CsvName: entry.BundleName}) if err != nil { @@ -97,25 +97,53 @@ func (rc *Client) FindBundleThatProvides(ctx context.Context, group, version, ki return bundle, nil } -// FilterChannelEntries filters out a channel entries that provide the requested +// FilterChannelEntries filters out channel entries that provide the requested // API and come from the same package with original operator and returns the -// first entry on the list -func (rc *Client) filterChannelEntries(it *ChannelEntryIterator, excludedPkgName string) *opregistry.ChannelEntry { +// first entry on the list from the default channel of that package +func (rc *Client) filterChannelEntries(ctx context.Context, it *ChannelEntryIterator, excludedPkgName string) *opregistry.ChannelEntry { + defChannels := make(map[string]string, 0) + var entries []*opregistry.ChannelEntry for e := it.Next(); e != nil; e = it.Next() { if e.PackageName != excludedPkgName { - entry := &opregistry.ChannelEntry{ - PackageName: e.PackageName, - ChannelName: e.ChannelName, - BundleName: e.BundleName, - Replaces: e.Replaces, + // keep track of the default channel for each package + if _, ok := defChannels[e.PackageName]; !ok { + defChannel, err := rc.getDefaultPackageChannel(ctx, e.PackageName) + if err != nil { + continue + } + defChannels[e.PackageName] = defChannel + } + + // only add entry to the list if the entry is in the default channel + if e.ChannelName == defChannels[e.PackageName] { + entry := &opregistry.ChannelEntry{ + PackageName: e.PackageName, + ChannelName: e.ChannelName, + BundleName: e.BundleName, + Replaces: e.Replaces, + } + entries = append(entries, entry) } - entries = append(entries, entry) } } - if entries != nil { + if len(entries) > 0 { return entries[0] } return nil } + +// GetDefaultPackageChannel uses registry client to get the default +// channel name for a given package name +func (rc *Client) getDefaultPackageChannel(ctx context.Context, pkgName string) (string, error) { + pkg, err := rc.Client.Registry.GetPackage(ctx, ®istryapi.GetPackageRequest{Name: pkgName}) + if err != nil { + return "", err + } + if pkg == nil { + return "", fmt.Errorf("package %s not found in registry", pkgName) + } + + return pkg.DefaultChannelName, nil +} diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go index 4158357079..d1d4a04fc6 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/test/e2e/subscription_e2e_test.go @@ -1349,6 +1349,8 @@ var _ = Describe("Subscription", func() { csvB := newCSV("nginx-b-dep", testNamespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{crd}, nil, depNamedStrategy) // csvC provides CRD2 in the different catalogsource with csvA (apackage) csvC := newCSV("nginx-c-dep", testNamespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{crd2}, nil, depNamedStrategy2) + // csvD provides CRD1 in the same catalogsource with csvA (apackage) + csvD := newCSV("nginx-d-dep", testNamespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{crd}, nil, depNamedStrategy) // Create PackageManifests 1 // Contain csvA, ABC and B @@ -1364,6 +1366,7 @@ var _ = Describe("Subscription", func() { { PackageName: packageName2, Channels: []registry.PackageChannel{ + {Name: alphaChannel, CurrentCSVName: csvD.GetName()}, {Name: stableChannel, CurrentCSVName: csvB.GetName()}, }, DefaultChannelName: stableChannel, @@ -1383,7 +1386,7 @@ var _ = Describe("Subscription", func() { } catalogSourceName := genName("catsrc") - catsrc, cleanup := createInternalCatalogSource(kubeClient, crClient, catalogSourceName, testNamespace, manifests, []apiextensions.CustomResourceDefinition{crd, crd2}, []v1alpha1.ClusterServiceVersion{csvA, csvABC, csvB}) + catsrc, cleanup := createInternalCatalogSource(kubeClient, crClient, catalogSourceName, testNamespace, manifests, []apiextensions.CustomResourceDefinition{crd, crd2}, []v1alpha1.ClusterServiceVersion{csvA, csvABC, csvB, csvD}) defer cleanup() // Ensure that the catalog source is resolved before we create a subscription. @@ -1429,6 +1432,9 @@ var _ = Describe("Subscription", func() { // Ensure csvABC is not installed _, err = crClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Get(context.TODO(), csvABC.Name, metav1.GetOptions{}) require.Error(GinkgoT(), err) + // Ensure csvD is not installed -- this implies the dependent subscription selected the default channel + _, err = crClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Get(context.TODO(), csvD.Name, metav1.GetOptions{}) + require.Error(GinkgoT(), err) }) })