From 07127f2352e9793a8471bfd9d1e15e97d69bf81b Mon Sep 17 00:00:00 2001 From: Jaydip Gabani Date: Thu, 19 Sep 2024 11:22:48 -0700 Subject: [PATCH] chore: adding common function for error reporting for constraint (#3486) Signed-off-by: Jaydip Gabani Co-authored-by: Rita Zhang --- .../constraint/constraint_controller.go | 93 ++++---------- .../constraint/constraint_controller_test.go | 120 ++++++++++++++++++ 2 files changed, 145 insertions(+), 68 deletions(-) diff --git a/pkg/controller/constraint/constraint_controller.go b/pkg/controller/constraint/constraint_controller.go index 967456d0d23..089d3cb1e42 100644 --- a/pkg/controller/constraint/constraint_controller.go +++ b/pkg/controller/constraint/constraint_controller.go @@ -39,6 +39,7 @@ import ( "github.com/open-policy-agent/gatekeeper/v3/pkg/readiness" "github.com/open-policy-agent/gatekeeper/v3/pkg/util" "github.com/open-policy-agent/gatekeeper/v3/pkg/watch" + errorpkg "github.com/pkg/errors" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1" corev1 "k8s.io/api/core/v1" @@ -306,20 +307,12 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R if c, err := r.cfClient.GetConstraint(instance); err != nil || !reflect.DeepEqual(instance, c) { err := util.ValidateEnforcementAction(enforcementAction, instance.Object) if err != nil { - 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 report error for validation of enforcement action") - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not validate enforcement actions") } generateVAPB, VAPEnforcementActions, err := shouldGenerateVAPB(*DefaultGenerateVAPB, enforcementAction, instance) if err != nil { - log.Error(err, "could not determine if VAPBinding should be generated") - 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 report error when determining if VAPBinding should be generated") - } - return reconcile.Result{}, err + log.Error(err, "could not determine if ValidatingAdmissionPolicyBinding should be generated") + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not determine if ValidatingAdmissionPolicyBinding should be generated") } isAPIEnabled := false var groupVersion *schema.GroupVersion @@ -329,19 +322,12 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R if generateVAPB { if !isAPIEnabled { log.Error(ErrValidatingAdmissionPolicyAPIDisabled, "Cannot generate ValidatingAdmissionPolicyBinding", "constraint", instance.GetName()) - status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: fmt.Sprintf("%s, cannot generate ValidatingAdmissionPolicyBinding", ErrValidatingAdmissionPolicyAPIDisabled.Error())}) - if err2 := r.writer.Update(ctx, status); err2 != nil { - log.Error(err2, "could not update constraint status error when ValidatingAdmissionPolicy API is not enabled") - } + _ = r.reportErrorOnConstraintStatus(ctx, status, ErrValidatingAdmissionPolicyAPIDisabled, "cannot generate ValidatingAdmissionPolicyBinding") generateVAPB = false } else { unversionedCT := &templates.ConstraintTemplate{} if err := r.scheme.Convert(ct, unversionedCT, nil); err != nil { - 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 converting ConstraintTemplate to unversioned") - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not convert ConstraintTemplate to unversioned") } hasVAP, err := ShouldGenerateVAP(unversionedCT) switch { @@ -349,17 +335,11 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R 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") - } + _ = r.reportErrorOnConstraintStatus(ctx, status, err, "could not determine if ConstraintTemplate is configured to generate ValidatingAdmissionPolicy") generateVAPB = false 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") - } + _ = r.reportErrorOnConstraintStatus(ctx, status, ErrVAPConditionsNotSatisfied, "Cannot generate ValidatingAdmissionPolicyBinding") generateVAPB = false default: } @@ -370,11 +350,7 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R if generateVAPB && groupVersion != nil { currentVapBinding, err := vapBindingForVersion(*groupVersion) if err != nil { - 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 get vapbinding version") - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not get ValidatingAdmissionPolicyBinding API version") } vapBindingName := fmt.Sprintf("gatekeeper-%s", instance.GetName()) log.Info("check if vapbinding exists", "vapBindingName", vapBindingName) @@ -386,20 +362,12 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R } transformedVapBinding, err := transform.ConstraintToBinding(instance, VAPEnforcementActions) if err != nil { - 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 report transform vapbinding error", "vapBindingName", vapBindingName) - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not transform constraint to ValidatingAdmissionPolicyBinding") } newVapBinding, err := getRunTimeVAPBinding(groupVersion, transformedVapBinding, currentVapBinding) if err != nil { - 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 get VAPBinding object with runtime group version", "vapBindingName", vapBindingName) - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not get ValidatingAdmissionPolicyBinding object with runtime group version") } if err := controllerutil.SetControllerReference(instance, newVapBinding, r.scheme); err != nil { @@ -409,20 +377,12 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R if currentVapBinding == nil { log.Info("creating vapbinding") if err := r.writer.Create(ctx, newVapBinding); err != nil { - 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 report creating vapbinding error status") - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, fmt.Sprintf("could not create ValidatingAdmissionPolicyBinding: %s", vapBindingName)) } } else if !reflect.DeepEqual(currentVapBinding, newVapBinding) { log.Info("updating vapbinding") if err := r.writer.Update(ctx, newVapBinding); err != nil { - 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 report update vapbinding error status") - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, fmt.Sprintf("could not update ValidatingAdmissionPolicyBinding: %s", vapBindingName)) } } } @@ -431,11 +391,7 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R if !generateVAPB && groupVersion != nil { currentVapBinding, err := vapBindingForVersion(*groupVersion) if err != nil { - 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 get vapbinding version") - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not get ValidatingAdmissionPolicyBinding API version") } vapBindingName := fmt.Sprintf("gatekeeper-%s", instance.GetName()) log.Info("check if vapbinding exists", "vapBindingName", vapBindingName) @@ -448,11 +404,7 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R if currentVapBinding != nil { log.Info("deleting vapbinding") if err := r.writer.Delete(ctx, currentVapBinding); err != nil { - 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 report delete vapbinding error status") - } - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, fmt.Sprintf("could not delete ValidatingAdmissionPolicyBinding: %s", vapBindingName)) } } } @@ -461,12 +413,8 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R enforcementAction: enforcementAction, status: metrics.ErrorStatus, }) - 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 report constraint error status") - } reportMetrics = true - return reconcile.Result{}, err + return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not cache constraint") } logAddition(r.log, instance, enforcementAction) } @@ -617,6 +565,15 @@ func (r *ReconcileConstraint) cacheConstraint(ctx context.Context, instance *uns return nil } +func (r *ReconcileConstraint) reportErrorOnConstraintStatus(ctx context.Context, status *constraintstatusv1beta1.ConstraintPodStatus, err error, message string) error { + status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: fmt.Sprintf("%s: %s", message, err)}) + if err2 := r.writer.Update(ctx, status); err2 != nil { + log.Error(err2, message, "error", "could not update constraint status") + return errorpkg.Wrapf(err, fmt.Sprintf("%s, could not update constraint status: %s", message, err2)) + } + return err +} + func NewConstraintsCache() *ConstraintsCache { return &ConstraintsCache{ cache: make(map[string]tags), diff --git a/pkg/controller/constraint/constraint_controller_test.go b/pkg/controller/constraint/constraint_controller_test.go index ca042ead9a4..87acc788ea1 100644 --- a/pkg/controller/constraint/constraint_controller_test.go +++ b/pkg/controller/constraint/constraint_controller_test.go @@ -1,6 +1,7 @@ package constraint import ( + "context" "errors" "reflect" "strings" @@ -12,12 +13,14 @@ import ( celSchema "github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/k8scel/schema" regoSchema "github.com/open-policy-agent/frameworks/constraint/pkg/client/drivers/rego/schema" "github.com/open-policy-agent/frameworks/constraint/pkg/core/templates" + constraintstatusv1beta1 "github.com/open-policy-agent/gatekeeper/v3/apis/status/v1beta1" "github.com/open-policy-agent/gatekeeper/v3/pkg/metrics" "github.com/open-policy-agent/gatekeeper/v3/pkg/target" "github.com/open-policy-agent/gatekeeper/v3/pkg/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" ) func makeTemplateWithRegoAndCELEngine(vapGenerationVal *bool) *templates.ConstraintTemplate { @@ -534,3 +537,120 @@ func TestShouldGenerateVAP(t *testing.T) { }) } } + +func TestReportErrorOnConstraintStatus(t *testing.T) { + tests := []struct { + name string + status *constraintstatusv1beta1.ConstraintPodStatus + err error + message string + updateErr error + expectedError error + expectedStatusErrorLen int + expectedStatusError []constraintstatusv1beta1.Error + }{ + { + name: "successful update", + status: &constraintstatusv1beta1.ConstraintPodStatus{ + Status: constraintstatusv1beta1.ConstraintPodStatusStatus{}, + }, + err: errors.New("test error"), + message: "test message", + updateErr: nil, + expectedError: errors.New("test error"), + expectedStatusErrorLen: 1, + expectedStatusError: []constraintstatusv1beta1.Error{ + { + Message: "test error", + }, + }, + }, + { + name: "update error", + status: &constraintstatusv1beta1.ConstraintPodStatus{ + Status: constraintstatusv1beta1.ConstraintPodStatusStatus{}, + }, + err: errors.New("test error"), + message: "test message", + updateErr: errors.New("update error"), + expectedError: errors.New("test message, could not update constraint status: update error: test error"), + expectedStatusErrorLen: 1, + expectedStatusError: []constraintstatusv1beta1.Error{ + { + Message: "test error", + }, + }, + }, + { + name: "append status error", + status: &constraintstatusv1beta1.ConstraintPodStatus{ + Status: constraintstatusv1beta1.ConstraintPodStatusStatus{ + Errors: []constraintstatusv1beta1.Error{ + { + Message: "existing error", + }, + }, + }, + }, + err: errors.New("test error"), + message: "test message", + updateErr: nil, + expectedError: errors.New("test error"), + expectedStatusErrorLen: 2, + expectedStatusError: []constraintstatusv1beta1.Error{ + { + Message: "existing error", + }, + { + Message: "test error", + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + writer := &fakeWriter{updateErr: tt.updateErr} + r := &ReconcileConstraint{ + writer: writer, + } + + err := r.reportErrorOnConstraintStatus(context.TODO(), tt.status, tt.err, tt.message) + if err == nil || err.Error() != tt.expectedError.Error() { + t.Errorf("expected error %v, got %v", tt.expectedError, err) + } + + if len(tt.status.Status.Errors) != tt.expectedStatusErrorLen { + t.Errorf("expected %d error in status, got %d", tt.expectedStatusErrorLen, len(tt.status.Status.Errors)) + } + + if reflect.DeepEqual(tt.status.Status.Errors, tt.expectedStatusError) { + t.Errorf("expected status errors %v, got %v", tt.expectedStatusError, tt.status.Status.Errors) + } + }) + } +} + +type fakeWriter struct { + updateErr error +} + +func (f *fakeWriter) Update(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + return f.updateErr +} + +func (f *fakeWriter) Create(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return nil +} + +func (f *fakeWriter) Delete(_ context.Context, _ client.Object, _ ...client.DeleteOption) error { + return nil +} + +func (f *fakeWriter) Patch(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + return nil +} + +func (f *fakeWriter) DeleteAllOf(_ context.Context, _ client.Object, _ ...client.DeleteAllOfOption) error { + return nil +}