Add more validation to the OperatorPolicy

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(
@@ -830,7 +880,8 @@ func (r *OperatorPolicyReconciler) handleCSV(ctx context.Context,
  // need to report lack of existing CSV
  err := r.updateStatus(ctx, policy, noCSVCond, noExistingCSVObj)
  if err != nil {
- return nil, fmt.Errorf("error updating the status for Deployments: %w", err)
+ return nil, fmt.Errorf("error updating the status for ClusterServiceVersion "+
+ " with nonexistent Subscription: %w", err)
  }
 
  return nil, err
@@ -842,7 +893,7 @@ func (r *OperatorPolicyReconciler) handleCSV(ctx context.Context,
  if sub.Status.InstalledCSV == "" {
  err := r.updateStatus(ctx, policy, noCSVCond, noExistingCSVObj)
  if err != nil {
- return nil, fmt.Errorf("error updating the status for Deployments: %w", err)
+ return nil, fmt.Errorf("error updating the status for ClusterServiceVersion yet to be installed: %w", err)
  }
 
  return nil, err
@@ -893,10 +944,10 @@ func (r *OperatorPolicyReconciler) handleDeployment(
  // need to report lack of existing Deployments
  err := r.updateStatus(ctx, policy, noDeploymentsCond, noExistingDeploymentObj)
  if err != nil {
- return fmt.Errorf("error updating the status for Deployments: %w", err)
+ return fmt.Errorf("error updating the status for nonexistent Deployments: %w", err)
  }
 
- return err
+ return nil
  }
 
  OpLog := ctrl.LoggerFrom(ctx)
@@ -961,7 +1012,7 @@ func (r *OperatorPolicyReconciler) handleCatalogSource(
  // Note: existing related objects will not be removed by this status update
  err := r.updateStatus(ctx, policy, invalidCausingUnknownCond("CatalogSource"))
  if err != nil {
- return fmt.Errorf("error updating the status when the could not be determined: %w", err)
+ return fmt.Errorf("error updating the status for CatalogSource with nonexistent Subscription: %w", err)
  }
 
  return nil
@@ -1005,7 +1056,7 @@ func (r *OperatorPolicyReconciler) handleCatalogSource(
  isUnhealthy = (CatalogSrcState != CatalogSourceReady)
  }
 
- err = r.updateStatus(ctx, policy, catalogSourceFindCond(isUnhealthy, isMissing),
+ err = r.updateStatus(ctx, policy, catalogSourceFindCond(isUnhealthy, isMissing, catalogName),
  catalogSourceObj(catalogName, catalogNS, isUnhealthy, isMissing))
  if err != nil {
  return fmt.Errorf("error updating the status for a CatalogSource: %w", err)

controllers/operatorpolicy_status.go

@@ -643,7 +643,7 @@ var noDeploymentsCond = metav1.Condition{
 
 // catalogSourceFindCond is a conditionally compliant condition with reason
 // based on the `isUnhealthy` and `isMissing` parameters
-func catalogSourceFindCond(isUnhealthy bool, isMissing bool) metav1.Condition {
+func catalogSourceFindCond(isUnhealthy bool, isMissing bool, name string) metav1.Condition {
  status := metav1.ConditionFalse
  reason := "CatalogSourcesFound"
  message := "CatalogSource was found"
@@ -657,7 +657,7 @@ func catalogSourceFindCond(isUnhealthy bool, isMissing bool) metav1.Condition {
  if isMissing {
  status = metav1.ConditionTrue
  reason = "CatalogSourcesNotFound"
- message = "CatalogSource was not found"
+ message = "CatalogSource '" + name + "' was not found"
  }
 
  return metav1.Condition{

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)
@@ -809,9 +811,9 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun
  Type: "CatalogSourcesUnhealthy",
  Status: metav1.ConditionTrue,
  Reason: "CatalogSourcesNotFound",
- Message: "CatalogSource was not found",
+ Message: "CatalogSource 'fakeName' was not found",
  },
- "CatalogSource was not found",
+ "CatalogSource 'fakeName' was not found",
  )
  })
  It("Should remain NonCompliant when CatalogSource fails", func() {
@@ -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",
+ )
+ })
+ })
 })