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,77 @@ 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 errors.New(
+ "the subscription defaults could not be determined because the PackageManifest didn't specify a catalog",
+ )
+ }
+
+ 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 errors.New(
+ "the default channel could not be determined because the PackageManifest didn't specify one",
+ )
+ }
+
+ 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