Merge pull request #596 from tmshort/OCPBUGS-17408

OCPBUGS-17408: Do not derive installplan.spec.clusterServiceNames from bundle IDs

Author: openshift-ci[bot]

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go

@@ -1536,9 +1536,7 @@ func (o *Operator) createInstallPlan(namespace string, gen int, subs []*v1alpha1
 		}
 		catalogSourceMap[s.Resource.CatalogSource] = struct{}{}
 	}
-	for _, b := range bundleLookups {
-		csvNames = append(csvNames, b.Identifier)
-	}
+
 	catalogSources := []string{}
 	for s := range catalogSourceMap {
 		catalogSources = append(catalogSources, s)

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go

@@ -77,6 +77,70 @@ func (m *mockTransitioner) ExecutePlan(plan *v1alpha1.InstallPlan) error {
 	return m.err
 }
 
+func TestCreateInstallPlanHasExpectedClusterServiceVersionNames(t *testing.T) {
+	namespace := "foo"
+	tests := []struct {
+		testName                           string
+		steps                              []*v1alpha1.Step
+		bundleLookups                      []v1alpha1.BundleLookup
+		expectedClusterServiceVersionNames []string
+	}{
+		/******************************************************************************
+		Historically, when creating an installPlan it's spec.ClusterServiceVersionNames
+		was derived from two sources:
+		   1. The names of CSVs found in "steps" of the installPlan's status.plan
+		   2. The metadata associated with the bundle image
+
+		These sources couldn't result in duplicate entries as the unpacking job would
+		finish after the installPlan was created and the steps weren't populated until
+		the unpacking job finished.
+
+		OLM was later updated to complete the unpacking jobs prior to creating
+		the installPlan, which caused CSVs to be listed twice as the createInstallPlan
+		function was called with steps and a bundle.
+		*****************************************************************************/
+		{
+			testName: "Check that CSVs are not listed twice if steps and bundles are provided",
+			steps: []*v1alpha1.Step{{
+				Resolving: "csv",
+				Resource: v1alpha1.StepResource{
+					CatalogSource:          "catalog",
+					CatalogSourceNamespace: namespace,
+					Group:                  "operators.coreos.com",
+					Version:                "v1alpha1",
+					Kind:                   "ClusterServiceVersion",
+					Name:                   "csvA",
+					Manifest:               toManifest(t, csv("csvA", namespace, nil, nil)),
+				},
+				Status: v1alpha1.StepStatusUnknown,
+			}},
+			bundleLookups: []v1alpha1.BundleLookup{
+				{
+					Identifier: "csvA",
+				},
+			},
+			expectedClusterServiceVersionNames: []string{"csvA"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.testName, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.TODO())
+			defer cancel()
+
+			op, err := NewFakeOperator(ctx, namespace, []string{namespace})
+			require.NoError(t, err)
+
+			_, err = op.createInstallPlan(namespace, 0, nil, v1alpha1.ApprovalAutomatic, tt.steps, tt.bundleLookups)
+			require.NoError(t, err)
+
+			ipList, err := op.client.OperatorsV1alpha1().InstallPlans(namespace).List(ctx, metav1.ListOptions{})
+			require.NoError(t, err)
+			require.Len(t, ipList.Items, 1)
+			require.Equal(t, tt.expectedClusterServiceVersionNames, ipList.Items[0].Spec.ClusterServiceVersionNames)
+		})
+	}
+}
+
 func TestTransitionInstallPlan(t *testing.T) {
 	errMsg := "transition test error"
 	err := errors.New(errMsg)