Set default subscription values when not specified

This sets the default channel, source, and sourceNamespace when not
provided in the policy.

Relates:
https://issues.redhat.com/browse/ACM-10561

Signed-off-by: mprahl <mprahl@users.noreply.github.com>

controllers/operatorpolicy_controller.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/client-go/dynamic"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -85,11 +86,18 @@ var (
 		Version: "v1alpha1",
 		Kind:    "InstallPlan",
 	}
+	packageManifestGVR = schema.GroupVersionResource{
+		Group:    "packages.operators.coreos.com",
+		Version:  "v1",
+		Resource: "packagemanifests",
+	}
+	ErrPackageManifest = errors.New("")
 )
 
 // OperatorPolicyReconciler reconciles a OperatorPolicy object
 type OperatorPolicyReconciler struct {
 	client.Client
+	DynamicClient    dynamic.Interface
 	DynamicWatcher   depclient.DynamicWatcher
 	InstanceName     string
 	DefaultNamespace string
@@ -209,7 +217,7 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
 
 	earlyComplianceEvents = make([]metav1.Condition, 0)
 
-	desiredSub, desiredOG, changed, err := r.buildResources(policy)
+	desiredSub, desiredOG, changed, err := r.buildResources(ctx, policy)
 	condChanged = changed
 
 	if err != nil {
@@ -297,13 +305,28 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
 //   - an error if an API call failed
 //
 // The built objects can be used to find relevant objects for a 'mustnothave' policy.
-func (r *OperatorPolicyReconciler) buildResources(policy *policyv1beta1.OperatorPolicy) (
+func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *policyv1beta1.OperatorPolicy) (
 	*operatorv1alpha1.Subscription, *operatorv1.OperatorGroup, bool, error,
 ) {
+	var returnedErr error
+
 	validationErrors := make([]error, 0)
 
 	sub, subErr := buildSubscription(policy, r.DefaultNamespace)
-	if subErr != nil {
+	if subErr == nil {
+		err := r.applySubscriptionDefaults(ctx, sub)
+		if err != nil {
+			sub = nil
+
+			// If it's a PackageManifest API error, then that means it should be returned for the Reconcile method
+			// to requeue the request. This is to workaround the PackageManifest API not supporting watches.
+			if errors.Is(err, ErrPackageManifest) {
+				returnedErr = err
+			}
+
+			validationErrors = append(validationErrors, err)
+		}
+	} else {
 		validationErrors = append(validationErrors, subErr)
 	}
 
@@ -329,7 +352,79 @@ func (r *OperatorPolicyReconciler) buildResources(policy *policyv1beta1.Operator
 		}
 	}
 
-	return sub, opGroup, updateStatus(policy, validationCond(validationErrors)), nil
+	return sub, opGroup, updateStatus(policy, validationCond(validationErrors)), returnedErr
+}
+
+// applySubscriptionDefaults will set the subscription channel, source, and sourceNamespace when they are unset by
+// utilizing the PackageManifest API.
+func (r *OperatorPolicyReconciler) applySubscriptionDefaults(
+	ctx context.Context, subscription *operatorv1alpha1.Subscription,
+) error {
+	subSpec := subscription.Spec
+
+	defaultsNeeded := subSpec.Channel == "" || subSpec.CatalogSource == "" || subSpec.CatalogSourceNamespace == ""
+
+	if !defaultsNeeded {
+		return nil
+	}
+
+	// PackageManifests come from an API server and not a Kubernetes resource, so the DynamicWatcher can't be used since
+	// it utilizes watches. The namespace doesn't have any meaning but is required.
+	packageManifest, err := r.DynamicClient.Resource(packageManifestGVR).Namespace("default").Get(
+		ctx, subSpec.Package, metav1.GetOptions{},
+	)
+	if err != nil {
+		if k8serrors.IsNotFound(err) {
+			return fmt.Errorf(
+				"%wthe subscription defaults could not be determined because the PackageManifest was not found",
+				ErrPackageManifest,
+			)
+		}
+
+		log.Error(err, "Failed to get the PackageManifest", "name", subSpec.Package)
+
+		return fmt.Errorf(
+			"%wthe subscription defaults could not be determined because the PackageManifest API returned an error",
+			ErrPackageManifest,
+		)
+	}
+
+	catalog, _, _ := unstructured.NestedString(packageManifest.Object, "status", "catalogSource")
+	catalogNamespace, _, _ := unstructured.NestedString(packageManifest.Object, "status", "catalogSourceNamespace")
+
+	if catalog == "" || catalogNamespace == "" {
+		return fmt.Errorf(
+			"%wthe subscription defaults could not be determined because the PackageManifest didn't specify a catalog",
+			ErrPackageManifest,
+		)
+	}
+
+	if (subSpec.CatalogSource != "" && subSpec.CatalogSource != catalog) ||
+		(subSpec.CatalogSourceNamespace != "" && subSpec.CatalogSourceNamespace != catalogNamespace) {
+		return errors.New(
+			"the subscription defaults could not be determined because the catalog specified in the policy does " +
+				"not match what was found in the PackageManifest on the cluster",
+		)
+	}
+
+	if subSpec.Channel == "" {
+		defaultChannel, _, _ := unstructured.NestedString(packageManifest.Object, "status", "defaultChannel")
+		if defaultChannel == "" {
+			return fmt.Errorf(
+				"%wthe default channel could not be determined because the PackageManifest didn't specify one",
+				ErrPackageManifest,
+			)
+		}
+
+		subSpec.Channel = defaultChannel
+	}
+
+	if subSpec.CatalogSource == "" || subSpec.CatalogSourceNamespace == "" {
+		subSpec.CatalogSource = catalog
+		subSpec.CatalogSourceNamespace = catalogNamespace
+	}
+
+	return nil
 }
 
 // buildSubscription bootstraps the subscription spec defined in the operator policy

main.go

@@ -434,6 +434,7 @@ func main() {
 
 		OpReconciler := controllers.OperatorPolicyReconciler{
 			Client:           mgr.GetClient(),
+			DynamicClient:    targetK8sDynamicClient,
 			DynamicWatcher:   watcher,
 			InstanceName:     instanceName,
 			DefaultNamespace: opts.operatorPolDefaultNS,

test/e2e/case38_install_operator_test.go

@@ -1,6 +1,7 @@
 package e2e
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"reflect"
@@ -136,6 +137,98 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 		ConsistentlyWithOffset(1, checkFunc, consistentlyDuration, 1).Should(Succeed())
 	}
 
+	Describe("Testing an all default operator policy", Ordered, func() {
+		const (
+			opPolYAML = "../resources/case38_operator_install/operator-policy-all-defaults.yaml"
+			opPolName = "oppol-all-defaults"
+			subName   = "project-quay"
+		)
+		BeforeAll(func() {
+			utils.Kubectl("create", "ns", opPolTestNS)
+			DeferCleanup(func() {
+				utils.Kubectl("delete", "ns", opPolTestNS)
+			})
+
+			createObjWithParent(parentPolicyYAML, parentPolicyName,
+				opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
+		})
+
+		It("Should create the Subscription with default values", func(ctx context.Context) {
+			By("Verifying the policy is compliant")
+			check(
+				opPolName,
+				false,
+				[]policyv1.RelatedObject{{
+					Object: policyv1.ObjectResource{
+						Kind:       "Subscription",
+						APIVersion: "operators.coreos.com/v1alpha1",
+					},
+					Compliant: "Compliant",
+					Reason:    "Resource found as expected",
+				}},
+				metav1.Condition{
+					Type:    "SubscriptionCompliant",
+					Status:  metav1.ConditionTrue,
+					Reason:  "SubscriptionMatches",
+					Message: "the Subscription matches what is required by the policy",
+				},
+				"the Subscription required by the policy was created",
+			)
+
+			By("Verifying the subscription has the correct defaults")
+			sub, err := clientManagedDynamic.Resource(gvrSubscription).Namespace(opPolTestNS).
+				Get(ctx, subName, metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+
+			channel, _, _ := unstructured.NestedString(sub.Object, "spec", "channel")
+			// This default will change so just check something was set.
+			Expect(channel).ToNot(BeEmpty())
+
+			source, _, _ := unstructured.NestedString(sub.Object, "spec", "source")
+			Expect(source).To(Equal("operatorhubio-catalog"))
+
+			sourceNamespace, _, _ := unstructured.NestedString(sub.Object, "spec", "sourceNamespace")
+			Expect(sourceNamespace).To(Equal("olm"))
+		})
+	})
+
+	Describe("Testing an operator policy with invalid partial defaults", Ordered, func() {
+		const (
+			opPolYAML = "../resources/case38_operator_install/operator-policy-defaults-invalid-source.yaml"
+			opPolName = "oppol-defaults-invalid-source"
+			subName   = "project-quay"
+		)
+		BeforeAll(func() {
+			utils.Kubectl("create", "ns", opPolTestNS)
+			DeferCleanup(func() {
+				utils.Kubectl("delete", "ns", opPolTestNS)
+			})
+
+			createObjWithParent(parentPolicyYAML, parentPolicyName,
+				opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
+		})
+
+		It("Should be NonCompliant specifying a source not matching the PackageManifest", func(ctx context.Context) {
+			By("Verifying the policy is noncompliant due to the invalid source")
+			Eventually(func(g Gomega) {
+				pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName,
+					opPolTestNS, true, eventuallyTimeout)
+				compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+
+				g.Expect(compliance).To(Equal("NonCompliant"))
+
+				expectedMsg := "the subscription defaults could not be determined because the catalog specified in " +
+					"the policy does not match what was found in the PackageManifest on the cluster"
+				events := utils.GetMatchingEvents(
+					clientManaged, opPolTestNS, parentPolicyName, "", expectedMsg, eventuallyTimeout,
+				)
+				g.Expect(events).NotTo(BeEmpty())
+			}, defaultTimeoutSeconds, 5, ctx).Should(Succeed())
+		})
+	})
+
 	Describe("Testing OperatorGroup behavior when it is not specified in the policy", Ordered, func() {
 		const (
 			opPolYAML        = "../resources/case38_operator_install/operator-policy-no-group.yaml"

test/resources/case38_operator_install/operator-policy-all-defaults.yaml

@@ -0,0 +1,20 @@
+apiVersion: policy.open-cluster-management.io/v1beta1
+kind: OperatorPolicy
+metadata:
+  name: oppol-all-defaults
+  annotations:
+    policy.open-cluster-management.io/parent-policy-compliance-db-id: "124"
+    policy.open-cluster-management.io/policy-compliance-db-id: "64"
+  ownerReferences:
+  - apiVersion: policy.open-cluster-management.io/v1
+    kind: Policy
+    name: parent-policy
+    uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation
+spec:
+  remediationAction: enforce
+  severity: medium
+  complianceType: musthave
+  subscription:
+    name: project-quay
+    namespace: operator-policy-testns
+    installPlanApproval: Manual

test/resources/case38_operator_install/operator-policy-defaults-invalid-source.yaml

@@ -0,0 +1,21 @@
+apiVersion: policy.open-cluster-management.io/v1beta1
+kind: OperatorPolicy
+metadata:
+  name: oppol-defaults-invalid-source
+  annotations:
+    policy.open-cluster-management.io/parent-policy-compliance-db-id: "124"
+    policy.open-cluster-management.io/policy-compliance-db-id: "64"
+  ownerReferences:
+  - apiVersion: policy.open-cluster-management.io/v1
+    kind: Policy
+    name: parent-policy
+    uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation
+spec:
+  remediationAction: inform
+  severity: medium
+  complianceType: musthave
+  subscription:
+    name: project-quay
+    namespace: operator-policy-testns
+    installPlanApproval: Manual
+    source: wrong