fix: fixing error reporting for templates without CEL (#3493)

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

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
  }

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",

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)

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{