From 7f8b46075a16ca4337f0a29941b584aa2fee2787 Mon Sep 17 00:00:00 2001 From: Alex Pana <8968914+acpana@users.noreply.github.com> Date: Thu, 19 Oct 2023 19:16:06 +0000 Subject: [PATCH 1/2] fix: support DELETE configs validation Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com> --- pkg/webhook/policy.go | 14 +++++++++----- pkg/webhook/policy_test.go | 38 ++++++++++++++++++++++++++------------ 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/pkg/webhook/policy.go b/pkg/webhook/policy.go index be6830957a2..fb87a55e64d 100644 --- a/pkg/webhook/policy.go +++ b/pkg/webhook/policy.go @@ -349,15 +349,15 @@ func (h *validationHandler) validateGatekeeperResources(ctx context.Context, req if err := h.validateConfigResource(req); err != nil { return true, err } - case req.AdmissionRequest.Kind.Group == mutationsGroup && req.AdmissionRequest.Kind.Kind == "AssignMetadata": + case gvk.Group == mutationsGroup && gvk.Kind == "AssignMetadata": return h.validateAssignMetadata(req) - case req.AdmissionRequest.Kind.Group == mutationsGroup && req.AdmissionRequest.Kind.Kind == "Assign": + case gvk.Group == mutationsGroup && gvk.Kind == "Assign": return h.validateAssign(req) - case req.AdmissionRequest.Kind.Group == mutationsGroup && req.AdmissionRequest.Kind.Kind == "ModifySet": + case gvk.Group == mutationsGroup && gvk.Kind == "ModifySet": return h.validateModifySet(req) - case req.AdmissionRequest.Kind.Group == mutationsGroup && req.AdmissionRequest.Kind.Kind == "AssignImage": + case gvk.Group == mutationsGroup && gvk.Kind == "AssignImage": return h.validateAssignImage(req) - case req.AdmissionRequest.Kind.Group == externalDataGroup && req.AdmissionRequest.Kind.Kind == "Provider": + case gvk.Group == externalDataGroup && gvk.Kind == "Provider": return h.validateProvider(req) } @@ -449,6 +449,10 @@ func (h *validationHandler) validateExpansionTemplate(req *admission.Request) (b } func (h *validationHandler) validateConfigResource(req *admission.Request) error { + if req.Operation == admissionv1.Delete && req.Name == "" { + return nil // Allow the general DELETE of "/apis/config.gatekeeper.sh/v1alpha1/namespaces//configs" + } + if req.Name != keys.Config.Name { return fmt.Errorf("config resource must have name 'config'") } diff --git a/pkg/webhook/policy_test.go b/pkg/webhook/policy_test.go index d3c87f8ede7..67fd515f219 100644 --- a/pkg/webhook/policy_test.go +++ b/pkg/webhook/policy_test.go @@ -811,36 +811,50 @@ func TestGetValidationMessages(t *testing.T) { func TestValidateConfigResource(t *testing.T) { tc := []struct { - TestName string - Name string - Err bool + name string + rName string + deleteOp bool + expectErr bool }{ { - TestName: "Wrong name", - Name: "FooBar", - Err: true, + name: "Wrong name", + rName: "FooBar", + expectErr: true, }, { - TestName: "Correct name", - Name: "config", + name: "Correct name", + rName: "config", + }, + { + name: "Delete operation with no name", + deleteOp: true, + }, + { + name: "Delete operation with name", + deleteOp: true, + rName: "abc", + expectErr: true, }, } for _, tt := range tc { - t.Run(tt.TestName, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { handler := validationHandler{log: log} req := &admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{ - Name: tt.Name, + Name: tt.rName, }, } + if tt.deleteOp { + req.AdmissionRequest.Operation = admissionv1.Delete + } err := handler.validateConfigResource(req) - if tt.Err && err == nil { + if tt.expectErr && err == nil { t.Errorf("Expected error but received nil") } - if !tt.Err && err != nil { + if !tt.expectErr && err != nil { t.Errorf("Did not expect error but received: %v", err) } }) From 2bbef83c60a537e4322d29af768ed78ecc290e62 Mon Sep 17 00:00:00 2001 From: Alex Pana <8968914+acpana@users.noreply.github.com> Date: Mon, 23 Oct 2023 18:27:25 +0000 Subject: [PATCH 2/2] refactor: skip validation ffor all delete operations Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com> --- pkg/webhook/policy.go | 9 +++++---- pkg/webhook/policy_test.go | 4 +++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/pkg/webhook/policy.go b/pkg/webhook/policy.go index fb87a55e64d..6d4eaebd5cb 100644 --- a/pkg/webhook/policy.go +++ b/pkg/webhook/policy.go @@ -336,6 +336,11 @@ func (h *validationHandler) getValidationMessages(res []*rtypes.Result, req *adm // validateGatekeeperResources returns whether an issue is user error (vs internal) and any errors // validating internal resources. func (h *validationHandler) validateGatekeeperResources(ctx context.Context, req *admission.Request) (bool, error) { + if req.Operation == admissionv1.Delete && req.Name == "" { + // Allow the general DELETE of resources like "/apis/config.gatekeeper.sh/v1alpha1/namespaces//configs" + return true, nil + } + gvk := req.AdmissionRequest.Kind switch { @@ -449,10 +454,6 @@ func (h *validationHandler) validateExpansionTemplate(req *admission.Request) (b } func (h *validationHandler) validateConfigResource(req *admission.Request) error { - if req.Operation == admissionv1.Delete && req.Name == "" { - return nil // Allow the general DELETE of "/apis/config.gatekeeper.sh/v1alpha1/namespaces//configs" - } - if req.Name != keys.Config.Name { return fmt.Errorf("config resource must have name 'config'") } diff --git a/pkg/webhook/policy_test.go b/pkg/webhook/policy_test.go index 67fd515f219..024ce6382b3 100644 --- a/pkg/webhook/policy_test.go +++ b/pkg/webhook/policy_test.go @@ -11,6 +11,7 @@ import ( "github.com/open-policy-agent/frameworks/constraint/pkg/core/templates" rtypes "github.com/open-policy-agent/frameworks/constraint/pkg/types" "github.com/open-policy-agent/gatekeeper/v3/apis/config/v1alpha1" + configv1alpha1 "github.com/open-policy-agent/gatekeeper/v3/apis/config/v1alpha1" "github.com/open-policy-agent/gatekeeper/v3/pkg/controller/config/process" "github.com/open-policy-agent/gatekeeper/v3/pkg/expansion" "github.com/open-policy-agent/gatekeeper/v3/pkg/mutation" @@ -843,13 +844,14 @@ func TestValidateConfigResource(t *testing.T) { req := &admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{ Name: tt.rName, + Kind: metav1.GroupVersionKind(configv1alpha1.GroupVersion.WithKind("Config")), }, } if tt.deleteOp { req.AdmissionRequest.Operation = admissionv1.Delete } - err := handler.validateConfigResource(req) + _, err := handler.validateGatekeeperResources(context.Background(), req) if tt.expectErr && err == nil { t.Errorf("Expected error but received nil")