From a92a3c0ae5ea6884dac1480296a429eedcb2a0b8 Mon Sep 17 00:00:00 2001 From: Janelle Law Date: Mon, 11 Aug 2025 14:41:16 -0700 Subject: [PATCH] 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 .v. 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 --- controllers/operatorpolicy_controller.go | 114 ++++++++++++------ controllers/operatorpolicy_controller_test.go | 112 +++++++++++++++++ test/resources/unit/mtc-oadp-installplan.yaml | 18 +++ 3 files changed, 205 insertions(+), 39 deletions(-) create mode 100644 test/resources/unit/mtc-oadp-installplan.yaml diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index ada6f6a8..e4ad7276 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -2247,49 +2247,36 @@ 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 `.v`. The -// returned set is of package names and not the CSV (i.e. no version in it). +// 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, packageName string, + installPlan *operatorv1alpha1.InstallPlan, targetCSV string, ) sets.Set[string] { - packageDependencies := 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 +2291,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) + } + } + } + + 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, ok := packageDependencies[startingCSV] + if !ok || packages == nil { + return dependencyCSVs + } + + for pkg := range packages { + if csv, ok := packageToCSV[pkg]; ok { + dependencyCSVs.Insert(csv) } } - return packageDependencies + if len(dependencyCSVs) == 0 { + // The startingCSV's dependencies are already satisfied + // by other installed CSVs. No additional CSVs need approval. + return dependencyCSVs + } + + for csv := range dependencyCSVs { + dependencyCSVs = dependencyCSVs.Union(getDependencyCSVs(csv, packageToCSV, packageDependencies)) + } + + return dependencyCSVs } func (r *OperatorPolicyReconciler) mustnothaveInstallPlan( diff --git a/controllers/operatorpolicy_controller_test.go b/controllers/operatorpolicy_controller_test.go index 2c6ff9bb..b717d83f 100644 --- a/controllers/operatorpolicy_controller_test.go +++ b/controllers/operatorpolicy_controller_test.go @@ -334,6 +334,118 @@ 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{ + "starting-operator": "starting-operator.v7.8.9", + "first-dependency-operator": "primary-operator.v1.1.1", + "second-dependency-operator": "secondary-operator.v2.2.2", + } + + happyPackageDependencies := map[string]sets.Set[string]{ + "starting-operator.v7.8.9": func() sets.Set[string] { + s := sets.New[string]() + s.Insert("first-dependency-operator") + + return s + }(), + "primary-operator.v1.1.1": func() sets.Set[string] { + s := sets.New[string]() + s.Insert("second-dependency-operator") + + return s + }(), + "secondary-operator.v2.2.2": sets.New[string](), + } + + packageDependenciesAlreadyInstalled := map[string]sets.Set[string]{ + "starting-operator.v7.8.9": func() sets.Set[string] { + s := sets.New[string]() + s.Insert("first-dependency-operator") + + return s + }(), + } + dependencyCSVsAlreadyInstalled := map[string]string{ + "irrelevant-uninstalled-operator": "irrelevant-uninstalled-operator.v1.2.3", + } + + expectedCSVs := sets.Set[string]{} + + dependencyCSVs := getDependencyCSVs("starting-operator.v7.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( + "starting-operator.v7.8.9", dependencyCSVsAlreadyInstalled, 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(), ", ")) + } + + dependencyCSVs = getDependencyCSVs("starting-operator.v7.8.9", happyPackageToCSV, happyPackageDependencies) + + expectedCSVs.Insert("primary-operator.v1.1.1") + expectedCSVs.Insert("secondary-operator.v2.2.2") + + if !dependencyCSVs.Equal(expectedCSVs) { + t.Fatalf( + "Expected dependency CSVs 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{ diff --git a/test/resources/unit/mtc-oadp-installplan.yaml b/test/resources/unit/mtc-oadp-installplan.yaml new file mode 100644 index 00000000..144bcf86 --- /dev/null +++ b/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"}}]}'