fix: error on deleting GK resources when delete operation is enabled CP(#3921) (#3929)

JaydipGabani · web-flow · commit d71d9bfcd54f · 2025-04-25T09:16:05.000-07:00
Signed-off-by: Jaydip Gabani &lt;gabanijaydip@gmail.com&gt;
diff --git a/pkg/webhook/policy.go b/pkg/webhook/policy.go
@@ -241,8 +241,9 @@ func (h *validationHandler) getValidationMessages(res []*rtypes.Result, req *adm
 
 	if len(res) > 0 && (*logDenies || *emitAdmissionEvents) {
 		resourceName = req.AdmissionRequest.Name
-		if req.AdmissionRequest.Object.Raw != nil {
-			if _, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, obj); err == nil {
+		rawObj := getReqObject(req)
+		if rawObj != nil {
+			if _, _, err := deserializer.Decode(rawObj, nil, obj); err == nil {
 				// On a CREATE operation, the client may omit name and
 				// rely on the server to generate the name.
 				if len(resourceName) == 0 {
@@ -399,7 +400,7 @@ func (h *validationHandler) validateGatekeeperResources(ctx context.Context, req
 // The returned boolean is only true if error is non-nil and is a result of user
 // error.
 func (h *validationHandler) validateTemplate(ctx context.Context, req *admission.Request) (bool, error) {
-	templ, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, nil)
+	templ, _, err := deserializer.Decode(getReqObject(req), nil, nil)
 	if err != nil {
 		return false, err
 	}
@@ -419,9 +420,16 @@ func (h *validationHandler) validateTemplate(ctx context.Context, req *admission
 	return false, nil
 }
 
+func getReqObject(req *admission.Request) []byte {
+	if req.AdmissionRequest.Operation == admissionv1.Delete {
+		return req.AdmissionRequest.OldObject.Raw
+	}
+	return req.AdmissionRequest.Object.Raw
+}
+
 func (h *validationHandler) validateConstraint(req *admission.Request) (bool, error) {
 	obj := &unstructured.Unstructured{}
-	if _, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, obj); err != nil {
+	if _, _, err := deserializer.Decode(getReqObject(req), nil, obj); err != nil {
 		return false, err
 	}
 	if err := h.opa.ValidateConstraint(obj); err != nil {
@@ -447,7 +455,7 @@ func (h *validationHandler) validateConstraint(req *admission.Request) (bool, er
 }
 
 func (h *validationHandler) validateExpansionTemplate(req *admission.Request) (bool, error) {
-	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, nil)
+	obj, _, err := deserializer.Decode(getReqObject(req), nil, nil)
 	if err != nil {
 		return false, err
 	}
@@ -471,7 +479,7 @@ func (h *validationHandler) validateConfigResource(req *admission.Request) error
 }
 
 func (h *validationHandler) validateAssignMetadata(req *admission.Request) (bool, error) {
-	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, nil)
+	obj, _, err := deserializer.Decode(getReqObject(req), nil, nil)
 	if err != nil {
 		return false, err
 	}
@@ -488,7 +496,7 @@ func (h *validationHandler) validateAssignMetadata(req *admission.Request) (bool
 }
 
 func (h *validationHandler) validateAssign(req *admission.Request) (bool, error) {
-	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, nil)
+	obj, _, err := deserializer.Decode(getReqObject(req), nil, nil)
 	if err != nil {
 		return false, err
 	}
@@ -505,7 +513,7 @@ func (h *validationHandler) validateAssign(req *admission.Request) (bool, error)
 }
 
 func (h *validationHandler) validateAssignImage(req *admission.Request) (bool, error) {
-	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, nil)
+	obj, _, err := deserializer.Decode(getReqObject(req), nil, nil)
 	if err != nil {
 		return false, err
 	}
@@ -522,7 +530,7 @@ func (h *validationHandler) validateAssignImage(req *admission.Request) (bool, e
 }
 
 func (h *validationHandler) validateModifySet(req *admission.Request) (bool, error) {
-	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, nil)
+	obj, _, err := deserializer.Decode(getReqObject(req), nil, nil)
 	if err != nil {
 		return false, err
 	}
@@ -539,7 +547,7 @@ func (h *validationHandler) validateModifySet(req *admission.Request) (bool, err
 }
 
 func (h *validationHandler) validateProvider(req *admission.Request) (bool, error) {
-	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, nil)
+	obj, _, err := deserializer.Decode(getReqObject(req), nil, nil)
 	if err != nil {
 		return false, err
 	}
@@ -578,10 +586,11 @@ func (h *validationHandler) reviewRequest(ctx context.Context, req *admission.Re
 
 	resultants := []*expansion.Resultant{}
 	// Skip the expansion if admissionRequest.Obj is nil.
-	if req.AdmissionRequest.Object.Raw != nil {
+	rawObj := getReqObject(req)
+	if rawObj != nil {
 		// Convert the request's generator resource to unstructured for expansion
 		obj := &unstructured.Unstructured{}
-		if _, _, err := deserializer.Decode(req.Object.Raw, nil, obj); err != nil {
+		if _, _, err := deserializer.Decode(rawObj, nil, obj); err != nil {
 			return nil, fmt.Errorf("error decoding generator resource %s: %w", req.Name, err)
 		}
 		obj.SetNamespace(req.Namespace)
diff --git a/pkg/webhook/policy_test.go b/pkg/webhook/policy_test.go
@@ -1,6 +1,7 @@
 package webhook
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"testing"
@@ -526,42 +527,63 @@ func TestConstraintValidation(t *testing.T) {
 		Template      *templates.ConstraintTemplate
 		Constraint    string
 		ErrorExpected bool
+		Operation     admissionv1.Operation
 	}{
 		{
 			Name:          "Valid Constraint labelselector",
 			Template:      validRegoTemplate(),
 			Constraint:    goodLabelSelector,
 			ErrorExpected: false,
+			Operation:     admissionv1.Create,
 		},
 		{
 			Name:          "Invalid Constraint labelselector",
 			Template:      validRegoTemplate(),
 			Constraint:    badLabelSelector,
 			ErrorExpected: true,
+			Operation:     admissionv1.Create,
 		},
 		{
 			Name:          "Valid Constraint namespaceselector",
 			Template:      validRegoTemplate(),
 			Constraint:    goodNamespaceSelector,
 			ErrorExpected: false,
+			Operation:     admissionv1.Create,
 		},
 		{
 			Name:          "Invalid Constraint namespaceselector",
 			Template:      validRegoTemplate(),
 			Constraint:    badNamespaceSelector,
 			ErrorExpected: true,
+			Operation:     admissionv1.Create,
 		},
 		{
 			Name:          "Valid Constraint enforcementaction",
 			Template:      validRegoTemplate(),
 			Constraint:    goodEnforcementAction,
 			ErrorExpected: false,
+			Operation:     admissionv1.Create,
 		},
 		{
 			Name:          "Invalid Constraint enforcementaction",
 			Template:      validRegoTemplate(),
 			Constraint:    badEnforcementAction,
 			ErrorExpected: true,
+			Operation:     admissionv1.Create,
+		},
+		{
+			Name:          "Valid Constraint labelselector with delete operation",
+			Template:      validRegoTemplate(),
+			Constraint:    goodLabelSelector,
+			ErrorExpected: false,
+			Operation:     admissionv1.Delete,
+		},
+		{
+			Name:          "Invalid Constraint labelselector with delete operation",
+			Template:      validRegoTemplate(),
+			Constraint:    badLabelSelector,
+			ErrorExpected: true,
+			Operation:     admissionv1.Delete,
 		},
 	}
 	for _, tt := range tc {
@@ -592,11 +614,19 @@ func TestConstraintValidation(t *testing.T) {
 						Version: "v1beta1",
 						Kind:    "K8sGoodRego",
 					},
-					Object: runtime.RawExtension{
-						Raw: b,
-					},
+					Operation: tt.Operation,
+					Name:      "constraint",
 				},
 			}
+			if tt.Operation == admissionv1.Delete {
+				review.OldObject = runtime.RawExtension{
+					Raw: b,
+				}
+			} else {
+				review.Object = runtime.RawExtension{
+					Raw: b,
+				}
+			}
 			_, err = handler.validateGatekeeperResources(ctx, review)
 			if err != nil && !tt.ErrorExpected {
 				t.Errorf("err = %s; want nil", err)
@@ -1077,6 +1107,55 @@ func TestValidateProvider(t *testing.T) {
 	}
 }
 
+func TestGetReqObject(t *testing.T) {
+	tests := []struct {
+		name           string
+		operation      admissionv1.Operation
+		objectRaw      []byte
+		oldObjectRaw   []byte
+		expectedOutput []byte
+	}{
+		{
+			name:           "Create operation",
+			operation:      admissionv1.Create,
+			objectRaw:      []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "test-pod"}}`),
+			oldObjectRaw:   nil,
+			expectedOutput: []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "test-pod"}}`),
+		},
+		{
+			name:           "Update operation",
+			operation:      admissionv1.Update,
+			objectRaw:      []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "updated-pod"}}`),
+			oldObjectRaw:   nil,
+			expectedOutput: []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "updated-pod"}}`),
+		},
+		{
+			name:           "Delete operation",
+			operation:      admissionv1.Delete,
+			objectRaw:      nil,
+			oldObjectRaw:   []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "deleted-pod"}}`),
+			expectedOutput: []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "deleted-pod"}}`),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			req := &admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					Operation: tt.operation,
+					Object:    runtime.RawExtension{Raw: tt.objectRaw},
+					OldObject: runtime.RawExtension{Raw: tt.oldObjectRaw},
+				},
+			}
+
+			result := getReqObject(req)
+			if !bytes.Equal(result, tt.expectedOutput) {
+				t.Errorf("getAnyObject() = %s, want %s", string(result), string(tt.expectedOutput))
+			}
+		})
+	}
+}
+
 // converts runtime.Object to runtime.RawExtension.
 func convertToRawExtension(obj runtime.Object) (*runtime.RawExtension, error) {
 	re := &runtime.RawExtension{}
