From 916f838cf0c6a024804bdd5ebb2122cfc9763aec Mon Sep 17 00:00:00 2001 From: Jaydipkumar Arvindbhai Gabani Date: Wed, 14 Aug 2024 14:00:31 -0700 Subject: [PATCH] fix: fixing error reporting for templates without CEL, cherry-pick (#3493) (#3495) --- .../constraint/constraint_controller.go | 20 ++--- .../constraint/constraint_controller_test.go | 4 +- .../constrainttemplate_controller.go | 2 +- .../constrainttemplate_controller_test.go | 81 +++++++++++++++++++ 4 files changed, 94 insertions(+), 13 deletions(-) diff --git a/pkg/controller/constraint/constraint_controller.go b/pkg/controller/constraint/constraint_controller.go index c56158d00ca..967456d0d23 100644 --- 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 index 5bd5b7dfcc4..ca042ead9a4 100644 --- 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 index 062aac2a34e..cfc728eacac 100644 --- 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 index afd9efdbbbd..f52d4fdc664 100644 --- 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{