Add more validation to the OperatorPolicy

The `ValidPolicySpec` condition now reports when the subscription or
operatorGroup in the policy have unknown fields. It also reports if the
InstallPlanApproval value in the subscription is not correct.

That condition also reports when the namespace for the subscription
does not exist, and the controller now watches that namespace so that if
it is created (or deleted), the policy will be reconciled and the status
will be updated.

Refs:
 - https://issues.redhat.com/browse/ACM-9993

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

controllers/operatorpolicy_controller.go

@@ -4,6 +4,7 @@
 package controllers
 
 import (
+ "bytes"
  "context"
  "encoding/json"
  "errors"
@@ -40,6 +41,11 @@ const (
 )
 
 var (
+ namespaceGVK = schema.GroupVersionKind{
+ Group: "",
+ Version: "v1",
+ Kind: "Namespace",
+ }
  subscriptionGVK = schema.GroupVersionKind{
  Group: "operators.coreos.com",
  Version: "v1alpha1",
@@ -223,6 +229,18 @@ func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *p
  validationErrors = append(validationErrors, ogErr)
  }
 
+ watcher := opPolIdentifier(policy.Namespace, policy.Name)
+
+ gotNamespace, err := r.DynamicWatcher.Get(watcher, namespaceGVK, "", opGroupNS)
+ if err != nil {
+ return sub, opGroup, fmt.Errorf("error getting operator namespace: %w", err)
+ }
+
+ if gotNamespace == nil {
+ validationErrors = append(validationErrors,
+ fmt.Errorf("the operator namespace ('%v') does not exist", opGroupNS))
+ }
+
  return sub, opGroup, r.updateStatus(ctx, policy, validationCond(validationErrors))
 }
 
@@ -255,18 +273,35 @@ func buildSubscription(
  return nil, fmt.Errorf("the namespace '%v' used for the subscription is not a valid namespace identifier", ns)
  }
 
- spec := new(operatorv1alpha1.SubscriptionSpec)
+ // This field is not actually in the subscription spec
+ delete(sub, "namespace")
 
- err = json.Unmarshal(policy.Spec.Subscription.Raw, spec)
+ subSpec, err := json.Marshal(sub)
  if err != nil {
  return nil, fmt.Errorf("the policy spec.subscription is invalid: %w", err)
  }
 
+ // Use a decoder to find fields that were erroneously set by the user.
+ dec := json.NewDecoder(bytes.NewReader(subSpec))
+ dec.DisallowUnknownFields()
+
+ spec := new(operatorv1alpha1.SubscriptionSpec)
+
+ if err := dec.Decode(spec); err != nil {
+ return nil, fmt.Errorf("the policy spec.subscription is invalid: %w", err)
+ }
+
  subscription.SetGroupVersionKind(subscriptionGVK)
  subscription.ObjectMeta.Name = spec.Package
  subscription.ObjectMeta.Namespace = ns
  subscription.Spec = spec
 
+ // This is not validated by the CRD, so validate it here to prevent unexpected behavior.
+ if !(spec.InstallPlanApproval == "Manual" || spec.InstallPlanApproval == "Automatic") {
+ return nil, fmt.Errorf("the policy spec.subscription.installPlanApproval ('%v') is invalid: "+
+ "must be 'Automatic' or 'Manual'", spec.InstallPlanApproval)
+ }
+
  // If the policy is in `enforce` mode and the allowed CSVs are restricted,
  // the InstallPlanApproval will be set to Manual so that upgrades can be controlled.
  if policy.Spec.RemediationAction.IsEnforce() && len(policy.Spec.Versions) > 0 {
@@ -302,7 +337,7 @@ func buildOperatorGroup(
  }
 
  if specifiedNS, ok := opGroup["namespace"].(string); ok && specifiedNS != "" {
- if specifiedNS != namespace {
+ if specifiedNS != namespace && namespace != "" {
  return nil, fmt.Errorf("the namespace specified in spec.operatorGroup ('%v') must match "+
  "the namespace used for the subscription ('%v')", specifiedNS, namespace)
  }
@@ -313,9 +348,22 @@ func buildOperatorGroup(
  return nil, fmt.Errorf("name is required in spec.operatorGroup")
  }
 
+ // These fields are not actually in the operatorGroup spec
+ delete(opGroup, "name")
+ delete(opGroup, "namespace")
+
+ opGroupSpec, err := json.Marshal(opGroup)
+ if err != nil {
+ return nil, fmt.Errorf("the policy spec.operatorGroup is invalid: %w", err)
+ }
+
+ // Use a decoder to find fields that were erroneously set by the user.
+ dec := json.NewDecoder(bytes.NewReader(opGroupSpec))
+ dec.DisallowUnknownFields()
+
  spec := new(operatorv1.OperatorGroupSpec)
 
- if err := json.Unmarshal(policy.Spec.OperatorGroup.Raw, spec); err != nil {
+ if err := dec.Decode(spec); err != nil {
  return nil, fmt.Errorf("the policy spec.operatorGroup is invalid: %w", err)
  }
 
@@ -331,12 +379,14 @@ func (r *OperatorPolicyReconciler) handleOpGroup(
 ) error {
  watcher := opPolIdentifier(policy.Namespace, policy.Name)
 
- if desiredOpGroup == nil {
+ if desiredOpGroup == nil || desiredOpGroup.Namespace == "" {
  // Note: existing related objects will not be removed by this status update
  err := r.updateStatus(ctx, policy, invalidCausingUnknownCond("OperatorGroup"))
  if err != nil {
  return fmt.Errorf("error updating the status when the OperatorGroup could not be determined: %w", err)
  }
+
+ return nil
  }
 
  foundOpGroups, err := r.DynamicWatcher.List(
@@ -896,7 +946,7 @@ func (r *OperatorPolicyReconciler) handleDeployment(
  return fmt.Errorf("error updating the status for Deployments: %w", err)
  }
 
- return err
+ return nil
  }
 
  OpLog := ctrl.LoggerFrom(ctx)

test/e2e/case38_install_operator_test.go

@@ -53,22 +53,24 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun
  g.Expect(policy.Status.ComplianceState).To(Equal(policyv1.NonCompliant))
  }
 
- matchingRelated := policy.Status.RelatedObjsOfKind(expectedRelatedObjs[0].Object.Kind)
- g.Expect(matchingRelated).To(HaveLen(len(expectedRelatedObjs)))
-
- for _, expectedRelObj := range expectedRelatedObjs {
- foundMatchingName := false
- unnamed := expectedRelObj.Object.Metadata.Name == ""
-
- for _, actualRelObj := range matchingRelated {
- if unnamed || actualRelObj.Object.Metadata.Name == expectedRelObj.Object.Metadata.Name {
- foundMatchingName = true
- g.Expect(actualRelObj.Compliant).To(Equal(expectedRelObj.Compliant))
- g.Expect(actualRelObj.Reason).To(Equal(expectedRelObj.Reason))
+ if len(expectedRelatedObjs) != 0 {
+ matchingRelated := policy.Status.RelatedObjsOfKind(expectedRelatedObjs[0].Object.Kind)
+ g.Expect(matchingRelated).To(HaveLen(len(expectedRelatedObjs)))
+
+ for _, expectedRelObj := range expectedRelatedObjs {
+ foundMatchingName := false
+ unnamed := expectedRelObj.Object.Metadata.Name == ""
+
+ for _, actualRelObj := range matchingRelated {
+ if unnamed || actualRelObj.Object.Metadata.Name == expectedRelObj.Object.Metadata.Name {
+ foundMatchingName = true
+ g.Expect(actualRelObj.Compliant).To(Equal(expectedRelObj.Compliant))
+ g.Expect(actualRelObj.Reason).To(Equal(expectedRelObj.Reason))
+ }
  }
- }
 
- g.Expect(foundMatchingName).To(BeTrue())
+ g.Expect(foundMatchingName).To(BeTrue())
+ }
  }
 
  idx, actualCondition := policy.Status.GetCondition(expectedCondition.Type)
@@ -1097,4 +1099,140 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun
  )
  })
  })
+ Describe("Testing OperatorPolicy validation messages", Ordered, func() {
+ const (
+ opPolYAML = "../resources/case38_operator_install/operator-policy-validity-test.yaml"
+ opPolName = "oppol-validity-test"
+ subName = "project-quay"
+ )
+
+ BeforeAll(func() {
+ utils.Kubectl("create", "ns", opPolTestNS)
+ DeferCleanup(func() {
+ utils.Kubectl("delete", "ns", opPolTestNS)
+ utils.Kubectl("delete", "ns", "nonexist-testns")
+ })
+
+ createObjWithParent(parentPolicyYAML, parentPolicyName,
+ opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
+ })
+
+ It("Should initially report unknown fields", func() {
+ check(
+ opPolName,
+ true,
+ []policyv1.RelatedObject{},
+ metav1.Condition{
+ Type: "ValidPolicySpec",
+ Status: metav1.ConditionFalse,
+ Reason: "InvalidPolicySpec",
+ Message: `spec.subscription is invalid: json: unknown field "actually"`,
+ },
+ `the status of the Subscription could not be determined because the policy is invalid`,
+ )
+ check(
+ opPolName,
+ true,
+ []policyv1.RelatedObject{},
+ metav1.Condition{
+ Type: "ValidPolicySpec",
+ Status: metav1.ConditionFalse,
+ Reason: "InvalidPolicySpec",
+ Message: `spec.operatorGroup is invalid: json: unknown field "foo"`,
+ },
+ `the status of the OperatorGroup could not be determined because the policy is invalid`,
+ )
+ })
+ It("Should report about the invalid installPlanApproval value", func() {
+ // remove the "unknown" fields
+ utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
+ `[{"op": "remove", "path": "/spec/operatorGroup/foo"}, `+
+ `{"op": "remove", "path": "/spec/subscription/actually"}]`)
+ check(
+ opPolName,
+ true,
+ []policyv1.RelatedObject{},
+ metav1.Condition{
+ Type: "ValidPolicySpec",
+ Status: metav1.ConditionFalse,
+ Reason: "InvalidPolicySpec",
+ Message: "spec.subscription.installPlanApproval ('Incorrect') is invalid: " +
+ "must be 'Automatic' or 'Manual'",
+ },
+ "NonCompliant",
+ )
+ })
+ It("Should report about the namespaces not matching", func() {
+ // Fix the `installPlanApproval` value
+ utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
+ `[{"op": "replace", "path": "/spec/subscription/installPlanApproval", "value": "Automatic"}]`)
+ check(
+ opPolName,
+ true,
+ []policyv1.RelatedObject{},
+ metav1.Condition{
+ Type: "ValidPolicySpec",
+ Status: metav1.ConditionFalse,
+ Reason: "InvalidPolicySpec",
+ Message: "the namespace specified in spec.operatorGroup ('operator-policy-testns') must match " +
+ "the namespace used for the subscription ('nonexist-testns')",
+ },
+ "NonCompliant",
+ )
+ })
+ It("Should report about the namespace not existing", func() {
+ // Fix the namespace mismatch by removing the operator group spec
+ utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
+ `[{"op": "remove", "path": "/spec/operatorGroup"}]`)
+ check(
+ opPolName,
+ true,
+ []policyv1.RelatedObject{{
+ Object: policyv1.ObjectResource{
+ Kind: "Subscription",
+ APIVersion: "operators.coreos.com/v1alpha1",
+ Metadata: policyv1.ObjectMetadata{
+ Name: subName,
+ Namespace: "nonexist-testns",
+ },
+ },
+ Compliant: "NonCompliant",
+ Reason: "Resource not found but should exist",
+ }},
+ metav1.Condition{
+ Type: "ValidPolicySpec",
+ Status: metav1.ConditionFalse,
+ Reason: "InvalidPolicySpec",
+ Message: "the operator namespace ('nonexist-testns') does not exist",
+ },
+ "NonCompliant",
+ )
+ })
+ It("Should update the status after the namespace is created", func() {
+ utils.Kubectl("create", "namespace", "nonexist-testns")
+ check(
+ opPolName,
+ true,
+ []policyv1.RelatedObject{{
+ Object: policyv1.ObjectResource{
+ Kind: "Subscription",
+ APIVersion: "operators.coreos.com/v1alpha1",
+ Metadata: policyv1.ObjectMetadata{
+ Name: subName,
+ Namespace: "nonexist-testns",
+ },
+ },
+ Compliant: "NonCompliant",
+ Reason: "Resource not found but should exist",
+ }},
+ metav1.Condition{
+ Type: "ValidPolicySpec",
+ Status: metav1.ConditionTrue,
+ Reason: "PolicyValidated",
+ Message: "the policy spec is valid",
+ },
+ "the policy spec is valid",
+ )
+ })
+ })
 })

test/resources/case38_operator_install/operator-policy-validity-test.yaml

@@ -0,0 +1,31 @@
+apiVersion: policy.open-cluster-management.io/v1beta1
+kind: OperatorPolicy
+metadata:
+ name: oppol-validity-test
+ 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
+ operatorGroup: # optional
+ foo: bar
+ name: scoped-operator-group
+ namespace: operator-policy-testns
+ targetNamespaces:
+ - operator-policy-testns
+ subscription:
+ actually: incorrect
+ channel: stable-3.8
+ name: project-quay
+ namespace: nonexist-testns
+ installPlanApproval: Incorrect
+ source: operatorhubio-catalog
+ sourceNamespace: olm
+ startingCSV: quay-operator.v3.8.1