Handle CSV approvals with unconventional package names

ref: https://issues.redhat.com/browse/ACM-20500

Remove the assumption that dependency CSV names always match the
format of <package name>.v<version>.

When approving multiple CSVs in an install plan, inspect the bundles
for required package names before recursively gathering dependency CSVs for approval.

Signed-off-by: Janelle Law <[email protected]>

Author: jan-law

controllers/operatorpolicy_controller.go

@@ -2247,49 +2247,34 @@ func getApprovedCSVs(
 		return nil
 	}
 
-	packageDependencies := getBundleDependencies(installPlan, subscriptionCSV, "")
-
-	approvedCSVs := sets.Set[string]{}
-	approvedCSVs.Insert(subscriptionCSV)
-
-	// Approve all CSVs that are dependencies of the subscription CSV.
-	for _, packageDependency := range packageDependencies.UnsortedList() {
-		for _, csvName := range installPlan.Spec.ClusterServiceVersionNames {
-			if strings.HasPrefix(csvName, packageDependency+".v") {
-				approvedCSVs.Insert(csvName)
-			}
-		}
-	}
+	approvedCSVs := getBundleDependencies(installPlan, subscriptionCSV)
 
 	return approvedCSVs
 }
 
-// getBundleDependencies recursively gets the dependencies from a target CSV or package name. If the package name is
-// provided instead of the target CSV, then it assumes the CSVs are in the format of `<package>.v<version>`. The
-// returned set is of package names and not the CSV (i.e. no version in it).
-func getBundleDependencies(
-	installPlan *operatorv1alpha1.InstallPlan, targetCSV string, packageName string,
-) sets.Set[string] {
-	packageDependencies := sets.Set[string]{}
+// getBundleDependencies reads the install plan for required package dependencies.
+// The package names are mapped to their CSV identifiers before recursively evaluating the dependency chain.
+// The returned set contains the target CSV and all of its dependency CSVs.
+func getBundleDependencies(installPlan *operatorv1alpha1.InstallPlan, targetCSV string) sets.Set[string] {
+	approvedCSVs := sets.Set[string]{}
 
-	if targetCSV == "" && packageName == "" {
-		return packageDependencies
+	if targetCSV == "" {
+		return approvedCSVs
 	}
 
-	for i, bundle := range installPlan.Status.BundleLookups {
-		if targetCSV != "" && bundle.Identifier != targetCSV {
-			continue
-		}
+	approvedCSVs.Insert(targetCSV)
 
-		if packageName != "" && !strings.HasPrefix(bundle.Identifier, packageName+".v") {
-			continue
-		}
+	// stores the mapping of package names to unapproved CSV identifiers
+	packageToCSV := make(map[string]string)
+	// stores the required package names for each unapproved CSV
+	packageDependencies := make(map[string]sets.Set[string])
 
+	for i, bundle := range installPlan.Status.BundleLookups {
 		if bundle.Properties == "" {
-			break
+			continue
 		}
 
-		props := map[string]interface{}{}
+		props := map[string]any{}
 
 		err := json.Unmarshal([]byte(bundle.Properties), &props)
 		if err != nil {
@@ -2304,37 +2289,86 @@ func getBundleDependencies(
 			break
 		}
 
-		propsList, ok := props["properties"].([]interface{})
+		propsList, ok := props["properties"].([]any)
 		if !ok {
-			break
+			continue
 		}
 
 		for _, prop := range propsList {
-			propMap, ok := prop.(map[string]interface{})
+			propMap, ok := prop.(map[string]any)
+			if !ok {
+				continue
+			}
+
+			propType, ok := propMap["type"].(string)
 			if !ok {
 				continue
 			}
 
-			if propMap["type"] != "olm.package.required" {
+			if propType != "olm.package" && propType != "olm.package.required" {
 				continue
 			}
 
-			propValue, ok := propMap["value"].(map[string]interface{})
+			propValue, ok := propMap["value"].(map[string]any)
 			if !ok {
 				continue
 			}
 
-			depPackageName, ok := propValue["packageName"].(string)
-			if !ok || depPackageName == "" {
+			pkgName, ok := propValue["packageName"].(string)
+			if !ok || pkgName == "" {
 				continue
 			}
 
-			packageDependencies.Insert(depPackageName)
-			packageDependencies = packageDependencies.Union(getBundleDependencies(installPlan, "", depPackageName))
+			switch propType {
+			case "olm.package":
+				packageToCSV[pkgName] = bundle.Identifier
+			case "olm.package.required":
+				if packageDependencies[bundle.Identifier] == nil {
+					packageDependencies[bundle.Identifier] = sets.Set[string]{}
+				}
+
+				packageDependencies[bundle.Identifier].Insert(pkgName)
+			}
 		}
 	}
 
-	return packageDependencies
+	approvedCSVs = approvedCSVs.Union(getDependencyCSVs(targetCSV, &packageToCSV, &packageDependencies))
+
+	return approvedCSVs
+}
+
+// getDependencyCSVs recursively converts the package dependencies to their CSV identifiers
+func getDependencyCSVs(
+	startingCSV string, packageToCSV *map[string]string, packageDependencies *map[string]sets.Set[string],
+) sets.Set[string] {
+	dependencyCSVs := sets.Set[string]{}
+
+	if startingCSV == "" || packageToCSV == nil || packageDependencies == nil {
+		return dependencyCSVs
+	}
+
+	packages := (*packageDependencies)[startingCSV]
+	if packages == nil {
+		return dependencyCSVs
+	}
+
+	for pkg := range packages {
+		if csv, ok := (*packageToCSV)[pkg]; ok {
+			dependencyCSVs.Insert(csv)
+		}
+	}
+
+	// This CSV's package dependencies are already satisfied
+	// by installed CSVs, so no additional CSVs need approval.
+	if len(dependencyCSVs) == 0 {
+		return dependencyCSVs
+	}
+
+	for csv := range dependencyCSVs {
+		dependencyCSVs = dependencyCSVs.Union(getDependencyCSVs(csv, packageToCSV, packageDependencies))
+	}
+
+	return dependencyCSVs
 }
 
 func (r *OperatorPolicyReconciler) mustnothaveInstallPlan(

controllers/operatorpolicy_controller_test.go

@@ -260,7 +260,7 @@ func TestMessageIncludesSubscription(t *testing.T) {
 	}
 }
 
-func TestGetApprovedCSVs(t *testing.T) {
+func TestGetApprovedCSVsWithMultipleCSVs(t *testing.T) {
 	odfIPRaw, err := os.ReadFile("../test/resources/unit/odf-installplan.yaml")
 	if err != nil {
 		t.Fatalf("Encountered an error when reading the odf-installplan.yaml: %v", err)
@@ -334,6 +334,112 @@ func TestGetApprovedCSVs(t *testing.T) {
 	}
 }
 
+func TestGetApprovedCSVsWithPackageNameLookup(t *testing.T) {
+	mtcOadpIPRaw, err := os.ReadFile("../test/resources/unit/mtc-oadp-installplan.yaml")
+	if err != nil {
+		t.Fatalf("Encountered an error when reading the mtc-oadp-installplan.yaml: %v", err)
+	}
+
+	installPlan := &operatorv1alpha1.InstallPlan{}
+
+	err = yaml.Unmarshal(mtcOadpIPRaw, installPlan)
+	if err != nil {
+		t.Fatalf("Encountered an error when unmarshaling the mtc-oadp-installplan.yaml: %v", err)
+	}
+
+	policy := &policyv1beta1.OperatorPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "mtc-operator",
+		},
+		Spec: policyv1beta1.OperatorPolicySpec{
+			RemediationAction: "enforce",
+			ComplianceType:    "musthave",
+			Severity:          "medium",
+			UpgradeApproval:   "Automatic",
+			Versions:          []string{"mtc-operator.v1.8.9"},
+		},
+	}
+
+	subscription := &operatorv1alpha1.Subscription{
+		Status: operatorv1alpha1.SubscriptionStatus{
+			CurrentCSV: "mtc-operator.v1.8.9",
+		},
+	}
+
+	csvs := getApprovedCSVs(policy, subscription, installPlan)
+
+	expectedCSVs := sets.Set[string]{}
+	expectedCSVs.Insert("mtc-operator.v1.8.9")
+	expectedCSVs.Insert("oadp-operator.v1.5.0")
+
+	if !csvs.Equal(expectedCSVs) {
+		t.Fatalf(
+			"Expected both MTC and OADP CSVs to be approved. Got: %s, Expected: %s",
+			strings.Join(csvs.UnsortedList(), ", "),
+			strings.Join(expectedCSVs.UnsortedList(), ", "),
+		)
+	}
+}
+
+func TestGetDependencyCSVs(t *testing.T) {
+	happyPackageToCSV := map[string]string{
+		"mtc-operator":  "mtc-operator.v1.8.9",
+		"oadp-operator": "oadp-operator.v1.5.0",
+	}
+
+	happyPackageDependencies := map[string]sets.Set[string]{
+		"mtc-operator.v1.8.9": func() sets.Set[string] {
+			s := sets.New[string]()
+			s.Insert("oadp-operator")
+
+			return s
+		}(),
+		"oadp-operator.v1.5.0": sets.New[string](),
+	}
+
+	packageDependenciesAlreadyInstalled := map[string]sets.Set[string]{
+		"mtc-operator.v1.8.9": sets.New[string](),
+	}
+	dependencyCSVsAlreadyInstalled := map[string]string{
+		"irrelevant-operator": "irrelevant-operator.v1.2.3",
+	}
+
+	expectedCSVs := sets.Set[string]{}
+
+	dependencyCSVs := getDependencyCSVs("mtc-operator.v1.8.9", nil, nil)
+	if !dependencyCSVs.Equal(expectedCSVs) {
+		t.Fatalf("Expected no dependency CSVs to be approved. Got: %s, Expected: %s",
+			strings.Join(dependencyCSVs.UnsortedList(), ", "),
+			strings.Join(expectedCSVs.UnsortedList(), ", "))
+	}
+
+	dependencyCSVs = getDependencyCSVs(
+		"mtc-operator.v1.8.9", &dependencyCSVsAlreadyInstalled, &happyPackageDependencies)
+	if !dependencyCSVs.Equal(expectedCSVs) {
+		t.Fatalf("Expected no dependency CSVs to be approved. Got: %s, Expected: %s",
+			strings.Join(dependencyCSVs.UnsortedList(), ", "),
+			strings.Join(expectedCSVs.UnsortedList(), ", "))
+	}
+
+	dependencyCSVs = getDependencyCSVs("mtc-operator.v1.8.9", &happyPackageToCSV, &packageDependenciesAlreadyInstalled)
+	if !dependencyCSVs.Equal(expectedCSVs) {
+		t.Fatalf("Expected no dependency CSVs to be approved. Got: %s, Expected: %s",
+			strings.Join(dependencyCSVs.UnsortedList(), ", "),
+			strings.Join(expectedCSVs.UnsortedList(), ", "))
+	}
+
+	expectedCSVs.Insert("oadp-operator.v1.5.0")
+
+	dependencyCSVs = getDependencyCSVs("mtc-operator.v1.8.9", &happyPackageToCSV, &happyPackageDependencies)
+
+	if !dependencyCSVs.Equal(expectedCSVs) {
+		t.Fatalf(
+			"Expected OADP CSV to be approved. Got: %s, Expected: %s",
+			strings.Join(dependencyCSVs.UnsortedList(), ", "),
+			strings.Join(expectedCSVs.UnsortedList(), ", "))
+	}
+}
+
 func TestCanonicalizeVersions(t *testing.T) {
 	policy := &policyv1beta1.OperatorPolicy{
 		Spec: policyv1beta1.OperatorPolicySpec{

test/resources/unit/mtc-oadp-installplan.yaml

@@ -0,0 +1,18 @@
+apiVersion: operators.coreos.com/v1alpha1
+kind: InstallPlan
+metadata:
+  name: install-mtc-oadp
+  namespace: openshift-adp
+spec:
+  approval: Manual
+  approved: false
+  clusterServiceVersionNames:
+  - mtc-operator.v1.8.9
+  - oadp-operator.v1.5.0
+  generation: 1
+status:
+  bundleLookups:
+  - identifier: oadp-operator.v1.5.0
+    properties: '{"properties":[{"type":"olm.gvk","value":{"group":"oadp.openshift.io","kind":"CloudStorage","version":"v1alpha1"}},{"type":"olm.package","value":{"packageName":"redhat-oadp-operator","version":"1.5.0"}}]}'
+  - identifier: mtc-operator.v1.8.9
+    properties: '{"properties":[{"type":"olm.gvk","value":{"group":"migration.openshift.io","kind":"DirectImageMigration","version":"v1alpha1"}},{"type":"olm.package","value":{"packageName":"mtc-operator","version":"1.8.9"}},{"type":"olm.package.required","value":{"packageName":"redhat-oadp-operator","versionRange":">=1.2.3"}}]}'