From 8211f51fab9341465677019bfc51b05d5f7b9c46 Mon Sep 17 00:00:00 2001 From: mprahl Date: Thu, 11 Apr 2024 17:09:31 -0400 Subject: [PATCH] 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 --- controllers/operatorpolicy_controller.go | 101 +++++++++++++++++- main.go | 1 + test/e2e/case38_install_operator_test.go | 93 ++++++++++++++++ .../operator-policy-all-defaults.yaml | 20 ++++ ...erator-policy-defaults-invalid-source.yaml | 21 ++++ 5 files changed, 232 insertions(+), 4 deletions(-) create mode 100644 test/resources/case38_operator_install/operator-policy-all-defaults.yaml create mode 100644 test/resources/case38_operator_install/operator-policy-defaults-invalid-source.yaml diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 3cee26ab..1b840d12 100644 --- a/controllers/operatorpolicy_controller.go +++ b/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 diff --git a/main.go b/main.go index 804223aa..d83fa9e0 100644 --- a/main.go +++ b/main.go @@ -434,6 +434,7 @@ func main() { OpReconciler := controllers.OperatorPolicyReconciler{ Client: mgr.GetClient(), + DynamicClient: targetK8sDynamicClient, DynamicWatcher: watcher, InstanceName: instanceName, DefaultNamespace: opts.operatorPolDefaultNS, diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 08c5c58b..3ed4324a 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/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" diff --git a/test/resources/case38_operator_install/operator-policy-all-defaults.yaml b/test/resources/case38_operator_install/operator-policy-all-defaults.yaml new file mode 100644 index 00000000..afa54a78 --- /dev/null +++ b/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 diff --git a/test/resources/case38_operator_install/operator-policy-defaults-invalid-source.yaml b/test/resources/case38_operator_install/operator-policy-defaults-invalid-source.yaml new file mode 100644 index 00000000..8d472ab4 --- /dev/null +++ b/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