Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Sep 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d9ef2f4
adding common function for error reporting for constraint
JaydipGabani Aug 12, 2024
b0c222c
Merge branch 'master' into report-constraint-error
JaydipGabani Sep 12, 2024
c787ed6
Merge branch 'master' into report-constraint-error
JaydipGabani Sep 16, 2024
6c93e0c
adding tests
JaydipGabani Sep 17, 2024
c4f5400
setting reportMetrics to true
JaydipGabani Sep 18, 2024
f751249
Merge branch 'master' into report-constraint-error
ritazh Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 25 additions & 69 deletions pkg/controller/constraint/constraint_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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:
}
Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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))
}
}
}
Expand All @@ -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)
Expand All @@ -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))
}
}
}
Expand All @@ -461,12 +413,7 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to the reportMetrics flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the flag back. This was not supposed to be removed. Good catch!

return reconcile.Result{}, err
return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not cache constraint")
}
logAddition(r.log, instance, enforcementAction)
}
Expand Down Expand Up @@ -617,6 +564,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),
Expand Down
120 changes: 120 additions & 0 deletions pkg/controller/constraint/constraint_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package constraint

import (
"context"
"errors"
"reflect"
"strings"
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
Loading