fix: fixing error reporting for templates without CEL, cherry-pick (#…

…3493) (#3495)
open-policy-agent · Aug 14, 2024 · 916f838 · 916f838
1 parent 3f9ba17
commit 916f838
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 13 deletions.
diff --git a/pkg/controller/constraint/constraint_controller.go b/pkg/controller/constraint/constraint_controller.go
@@ -64,10 +64,10 @@ import (
 )
 
 var (
-	log                                     = logf.Log.V(logging.DebugLevel).WithName("controller").WithValues(logging.Process, "constraint_controller")
-	discoveryErr                            *apiutil.ErrResourceDiscoveryFailed
-	DefaultGenerateVAPB                     = flag.Bool("default-create-vap-binding-for-constraints", false, "Create VAPBinding resource for constraint of the template containing VAP-style CEL source. Allowed values are false: do not create Validating Admission Policy Binding, true: create Validating Admission Policy Binding.")
-	DefaultGenerateVAP                      = flag.Bool("default-create-vap-for-templates", false, "Create VAP resource for template containing VAP-style CEL source. Allowed values are false: do not create Validating Admission Policy unless generateVAP: true is set on constraint template explicitly, true: create Validating Admission Policy unless generateVAP: false is set on constraint template explicitly.")
+	log                 = logf.Log.V(logging.DebugLevel).WithName("controller").WithValues(logging.Process, "constraint_controller")
+	discoveryErr        *apiutil.ErrResourceDiscoveryFailed
+	DefaultGenerateVAPB = flag.Bool("default-create-vap-binding-for-constraints", false, "Create VAPBinding resource for constraint of the template containing VAP-style CEL source. Allowed values are false: do not create Validating Admission Policy Binding, true: create Validating Admission Policy Binding.")
+	DefaultGenerateVAP  = flag.Bool("default-create-vap-for-templates", false, "Create VAP resource for template containing VAP-style CEL source. Allowed values are false: do not create Validating Admission Policy unless generateVAP: true is set on constraint template explicitly, true: create Validating Admission Policy unless generateVAP: false is set on constraint template explicitly.")
 )
 
 var (
@@ -344,21 +344,24 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
 						return reconcile.Result{}, err
 					}
 					hasVAP, err := ShouldGenerateVAP(unversionedCT)
-					if err != nil {
+					switch {
+					case errors.Is(err, celSchema.ErrCodeNotDefined):
+						generateVAPB = false
+					case err != nil:
 						log.Error(err, "could not determine if ConstraintTemplate is configured to generate ValidatingAdmissionPolicy", "constraint", instance.GetName(), "constraint_template", ct.GetName())
 						status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: err.Error()})
 						if err2 := r.writer.Update(ctx, status); err2 != nil {
 							log.Error(err2, "could not update constraint status error when determining if ConstraintTemplate is configured to generate ValidatingAdmissionPolicy")
 						}
 						generateVAPB = false
-					}
-					if !hasVAP {
+					case !hasVAP:
 						log.Error(ErrVAPConditionsNotSatisfied, "Cannot generate ValidatingAdmissionPolicyBinding", "constraint", instance.GetName(), "constraint_template", ct.GetName())
 						status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: fmt.Sprintf("%s, cannot generate ValidatingAdmissionPolicyBinding", ErrVAPConditionsNotSatisfied.Error())})
 						if err2 := r.writer.Update(ctx, status); err2 != nil {
 							log.Error(err2, "could not update constraint status error when conditions are not satisfied to generate ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding")
 						}
 						generateVAPB = false
+					default:
 					}
 				}
 			}
@@ -561,9 +564,6 @@ func (r *ReconcileConstraint) getOrCreatePodStatus(ctx context.Context, constrai
 
 func ShouldGenerateVAP(ct *templates.ConstraintTemplate) (bool, error) {
 	source, err := celSchema.GetSourceFromTemplate(ct)
-	if errors.Is(err, celSchema.ErrCodeNotDefined) {
-		return false, nil
-	}
 	if err != nil {
 		return false, err
 	}

diff --git a/pkg/controller/constraint/constraint_controller_test.go b/pkg/controller/constraint/constraint_controller_test.go
@@ -443,14 +443,14 @@ func TestShouldGenerateVAP(t *testing.T) {
 			},
 			vapDefault: true,
 			expected:   false,
-			wantErr:    false,
+			wantErr:    true,
 		},
 		{
 			name:       "template with only Rego engine",
 			template:   makeTemplateWithRegoEngine(),
 			vapDefault: true,
 			expected:   false,
-			wantErr:    false,
+			wantErr:    true,
 		},
 		{
 			name:       "Rego and CEL template with generateVAP set to true",

diff --git a/pkg/controller/constrainttemplate/constrainttemplate_controller.go b/pkg/controller/constrainttemplate/constrainttemplate_controller.go
@@ -379,7 +379,7 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 		return reconcile.Result{}, err
 	}
 	generateVap, err := constraint.ShouldGenerateVAP(unversionedCT)
-	if err != nil {
+	if err != nil || !errors.Is(err, celSchema.ErrCodeNotDefined) {
 		logger.Error(err, "generateVap error")
 	}
 	logger.Info("generateVap", "r.generateVap", generateVap)

diff --git a/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go b/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go
@@ -536,6 +536,56 @@ func TestReconcile(t *testing.T) {
 		}
 	})
 
+	t.Run("Error should be present on constraint when VAP generation is off and VAPB generation is on for CEL templates", func(t *testing.T) {
+		suffix := "ErrorShouldBePresentOnConstraint"
+		logger.Info("Running test: Error should be present on constraint when VAP generation is off and VAPB generation is on")
+		constraint.DefaultGenerateVAP = ptr.To[bool](false)
+		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
+		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, nil)
+		cstr := newDenyAllCstr(suffix)
+		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
+		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
+
+		err = retry.OnError(testutils.ConstantRetry, func(_ error) bool {
+			return true
+		}, func() error {
+			return c.Create(ctx, cstr)
+		})
+		if err != nil {
+			logger.Error(err, "create cstr")
+			t.Fatal(err)
+		}
+
+		err := isConstraintStatuErrorAsExpected(ctx, c, suffix, true, "Conditions are not satisfied")
+		if err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("Error should not be present on constraint when VAP generation if off and VAPB generation is on for templates without CEL", func(t *testing.T) {
+		suffix := "ErrorShouldNotBePresentOnConstraint"
+		logger.Info("Running test: Error should not be present on constraint when VAP generation is off and VAPB generation is on for templates wihout CEL")
+		constraint.DefaultGenerateVAP = ptr.To[bool](false)
+		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
+		constraintTemplate := makeReconcileConstraintTemplate(suffix)
+		cstr := newDenyAllCstr(suffix)
+		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
+		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
+		err = retry.OnError(testutils.ConstantRetry, func(_ error) bool {
+			return true
+		}, func() error {
+			return c.Create(ctx, cstr)
+		})
+		if err != nil {
+			logger.Error(err, "create cstr")
+			t.Fatal(err)
+		}
+		err := isConstraintStatuErrorAsExpected(ctx, c, suffix, false, "")
+		if err != nil {
+			t.Fatal(err)
+		}
+	})
+
 	t.Run("VapBinding should not be created without generateVap intent in CT", func(t *testing.T) {
 		suffix := "VapBindingShouldNotBeCreatedWithoutGenerateVapIntent"
 		logger.Info("Running test: VapBinding should not be created without generateVap intent in CT")
@@ -1337,6 +1387,37 @@ func constraintEnforced(ctx context.Context, c client.Client, suffix string) err
 	})
 }
 
+func isConstraintStatuErrorAsExpected(ctx context.Context, c client.Client, suffix string, wantErr bool, errMsg string) error {
+	return retry.OnError(testutils.ConstantRetry, func(_ error) bool {
+		return true
+	}, func() error {
+		cstr := newDenyAllCstr(suffix)
+		err := c.Get(ctx, types.NamespacedName{Name: "denyallconstraint"}, cstr)
+		if err != nil {
+			return err
+		}
+		status, err := getCByPodStatus(cstr)
+		if err != nil {
+			return err
+		}
+
+		switch {
+		case wantErr && len(status.Errors) == 0:
+			return fmt.Errorf("expected error not found in status")
+		case !wantErr && len(status.Errors) > 0:
+			return fmt.Errorf("unexpected error found in status")
+		case wantErr:
+			for _, e := range status.Errors {
+				if strings.Contains(e.Message, errMsg) {
+					return nil
+				}
+			}
+			return fmt.Errorf("expected error not found in status")
+		}
+		return nil
+	})
+}
+
 func newDenyAllCstr(suffix string) *unstructured.Unstructured {
 	cstr := &unstructured.Unstructured{}
 	cstr.SetGroupVersionKind(schema.GroupVersionKind{