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.

Author: kevinrizza

pkg/controller/registry/registry_client.go

@@ -86,9 +86,12 @@ func (rc *Client) FindBundleThatProvides(ctx context.Context, group, version, ki
 	if err != nil {
 		return nil, err
 	}
-	entry := rc.filterChannelEntries(it, excludedPkgName)
+	entry, err := rc.filterChannelEntries(ctx, it, excludedPkgName)
+	if err != nil {
+		return nil, err
+	}
 	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, &registryapi.GetBundleRequest{PkgName: entry.PackageName, ChannelName: entry.ChannelName, CsvName: entry.BundleName})
 	if err != nil {
@@ -97,25 +100,56 @@ 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, error) {
+	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 {
+					return nil, err
+				}
+				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 {
-		return entries[0]
+		return entries[0], nil
+	}
+	return nil, 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, &registryapi.GetPackageRequest{Name: pkgName})
+	if err != nil {
+		return "", err
+	}
+
+	var defChan string
+	if pkg != nil {
+		defChan = pkg.DefaultChannelName
+	} else {
+		return "", fmt.Errorf("package %s not found in registry", pkgName)
 	}
-	return nil
+	return defChan, nil
 }

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)
 	})
 })