Repository: operator-framework/operator-lifecycle-manager
Pull request #3069 (2023-10-18 16:47:29)

Do not derive installplan.spec.clusterServiceNames from bundle IDs
Problem: 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 recently updated to complete the unpacking jobs prior to creating the installPlan, which means that the two sources can cause duplicate entries to appear in the array.

Solution: Now that OLM creates the installPlan after successfully unpacking the bundles, we no longer need to derive the names of the CSV from the bundle metadata.


openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/operator-framework/operator-lifecycle-manager/pull/3069#" title="Author self-approved">awgreene</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=operator-framework%2Foperator-lifecycle-manager).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/operator-framework/operator-lifecycle-manager/blob/master/OWNERS)~~ [awgreene]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/operator-framework/operator-lifecycle-manager/pull/3069#" title="Author self-approved">awgreene</a>*, *<a href="https://github.com/operator-framework/operator-lifecycle-manager/pull/3069#pullrequestreview-1685693938" title="LGTM">tmshort</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=operator-framework%2Foperator-lifecycle-manager).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/operator-framework/operator-lifecycle-manager/blob/master/OWNERS)~~ [awgreene,tmshort]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
awgreene: /hold
openshift-ci[bot]: New changes are detected. LGTM label has been removed.
tmshort: /lgtm
awgreene: /unhold

Patch:
diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go
index aa6ce24ec..57f749546 100644
--- a/pkg/controller/operators/catalog/operator.go
+++ b/pkg/controller/operators/catalog/operator.go
@@ -1593,9 +1593,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)


Test patch:
diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go
index 4dcc6499c..d07e8e606 100644
--- a/pkg/controller/operators/catalog/operator_test.go
+++ b/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)