chore: adding common function for error reporting for constraint (#3486)

Signed-off-by: Jaydip Gabani <[email protected]> Co-authored-by: Rita Zhang <[email protected]>
open-policy-agent · Sep 19, 2024 · 07127f2 · 07127f2
1 parent 8d1d7b3
commit 07127f2
Show file tree

Hide file tree

Showing 2 changed files with 145 additions and 68 deletions.
diff --git 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,37 +322,24 @@ 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 {
 					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")
-						}
+						_ = 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
@@ -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
+}