refactor: Move setting up Obj to old obj on Delete logic to target ha…

…ndler (#3511)

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>
Co-authored-by: Avinash Patnala <avinashpatnala@google.com>
Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com>

pkg/gator/verify/runner.go

@@ -361,9 +361,6 @@ func (r *Runner) validateAndReviewAdmissionReviewRequest(ctx context.Context, c
 	}
 
 	req := &admission.Request{AdmissionRequest: *ar.Request}
-	if err := util.SetObjectOnDelete(req); err != nil {
-		return nil, fmt.Errorf("%w: %w", gator.ErrInvalidK8sAdmissionReview, err)
-	}
 
 	arr := target.AugmentedReview{
 		AdmissionRequest: &req.AdmissionRequest,

pkg/gator/verify/runner_test.go

@@ -10,8 +10,10 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	constraintclient "github.com/open-policy-agent/frameworks/constraint/pkg/client"
+	clienterrors "github.com/open-policy-agent/frameworks/constraint/pkg/client/errors"
 	"github.com/open-policy-agent/gatekeeper/v3/pkg/gator"
 	"github.com/open-policy-agent/gatekeeper/v3/pkg/gator/fixtures"
+	"github.com/open-policy-agent/gatekeeper/v3/pkg/target"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/utils/ptr"
 )
@@ -1155,7 +1157,7 @@ func TestRunner_Run(t *testing.T) {
 							{Name: "invalid admission review object", Error: gator.ErrInvalidK8sAdmissionReview},
 							{Name: "missing admission request object", Error: gator.ErrMissingK8sAdmissionRequest},
 							{Name: "no objects to review", Error: gator.ErrNoObjectForReview},
-							{Name: "no oldObject on delete", Error: gator.ErrInvalidK8sAdmissionReview},
+							{Name: "no oldObject on delete", Error: &clienterrors.ErrorMap{target.Name: constraintclient.ErrReview}},
 						},
 					},
 					{

pkg/target/target.go

@@ -18,6 +18,9 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
+// nolint: revive // Moved error out of pkg/webhook/admission; needs capitalization for backwards compat.
+var ErrOldObjectIsNil = errors.New("oldObject cannot be nil for DELETE operations")
+
 // Name is the name of Gatekeeper's Kubernetes validation target.
 const Name = "admission.k8s.gatekeeper.sh"
 
@@ -127,6 +130,10 @@ func (h *K8sValidationTarget) handleReview(obj interface{}) (bool, *gkReview, er
 		return false, nil, nil
 	}
 
+	if err := setObjectOnDelete(review); err != nil {
+		return false, nil, err
+	}
+
 	return true, review, nil
 }
 
@@ -249,3 +256,24 @@ func (h *K8sValidationTarget) ToMatcher(u *unstructured.Unstructured) (constrain
 func (h *K8sValidationTarget) GetCache() handler.Cache {
 	return &h.cache
 }
+
+// setObjectOnDelete enforces that we use at least K8s API v1.15.0+ on DELETE operations
+// and copies over the oldObject into the Object field for the given AdmissionRequest.
+func setObjectOnDelete(review *gkReview) error {
+	if review.AdmissionRequest.Operation == admissionv1.Delete {
+		// oldObject is the existing object.
+		// It is null for DELETE operations in API servers prior to v1.15.0.
+		// https://github.com/kubernetes/website/pull/14671
+		if review.AdmissionRequest.OldObject.Raw == nil {
+			return ErrOldObjectIsNil
+		}
+
+		// For admission webhooks registered for DELETE operations on k8s built APIs or CRDs,
+		// the apiserver now sends the existing object as admissionRequest.Request.OldObject to the webhook
+		// object is the new object being admitted.
+		// It is null for DELETE operations.
+		// https://github.com/kubernetes/kubernetes/pull/76346
+		review.AdmissionRequest.Object = review.AdmissionRequest.OldObject
+	}
+	return nil
+}

pkg/target/target_test.go

@@ -3,6 +3,7 @@ package target
 import (
 	"encoding/json"
 	"errors"
+	"reflect"
 	"sync"
 	"testing"
 
@@ -1149,3 +1150,69 @@ func newNsCache() *nsCache {
 		cache: make(map[string]*corev1.Namespace),
 	}
 }
+
+func TestHandleReviewForDelete(t *testing.T) {
+	testCases := []struct {
+		name          string
+		req           interface{}
+		checkEquality bool
+		wantErr       error
+	}{
+		{
+			name: "request not on delete",
+			req: admissionv1.AdmissionRequest{
+				Operation: "CREATE",
+				Object:    runtime.RawExtension{Raw: matchedRawData()},
+			},
+			checkEquality: false,
+			wantErr:       nil,
+		},
+		{
+			name: "err on request and nil object",
+			req: admissionv1.AdmissionRequest{
+				Operation: "DELETE",
+			},
+			wantErr: ErrOldObjectIsNil,
+		},
+		{
+			name: "handle ok oldObject not nil",
+			req: admissionv1.AdmissionRequest{
+				Operation: "DELETE",
+				OldObject: runtime.RawExtension{
+					Raw: []byte{'a', 'b', 'c'},
+				},
+			},
+			checkEquality: true,
+			wantErr:       nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			target := &K8sValidationTarget{}
+
+			_, review, err := target.HandleReview(tc.req)
+
+			if tc.wantErr != nil {
+				if !errors.Is(tc.wantErr, err) {
+					t.Fatalf("error did not match what was expected\n want: %v \n got: %v \n", tc.wantErr, err)
+				}
+			}
+
+			gkr, ok := review.(*gkReview)
+			if !ok {
+				t.Fatalf("test %v: HandleReview failed to return gkReview object", tc.name)
+			}
+
+			if tc.checkEquality {
+				// open box: make sure that the OldObject field has been copied into the Object field
+				if !reflect.DeepEqual(gkr.AdmissionRequest.OldObject, gkr.AdmissionRequest.Object) {
+					t.Fatal("oldObject and object need to match")
+				}
+			}
+		})
+	}
+}

pkg/webhook/policy.go

@@ -143,12 +143,6 @@ func (h *validationHandler) Handle(ctx context.Context, req admission.Request) a
 		return admission.Allowed("Gatekeeper does not self-manage")
 	}
 
-	if err := util.SetObjectOnDelete(&req); err != nil {
-		vResp := admission.Denied(err.Error())
-		vResp.Result.Code = http.StatusInternalServerError
-		return vResp
-	}
-
 	if userErr, err := h.validateGatekeeperResources(ctx, &req); err != nil {
 		var code int32
 		if userErr {
@@ -582,29 +576,33 @@ func (h *validationHandler) reviewRequest(ctx context.Context, req *admission.Re
 		return nil, fmt.Errorf("failed to create augmentedReview: %w", err)
 	}
 
-	// Convert the request's generator resource to unstructured for expansion
-	obj := &unstructured.Unstructured{}
-	if _, _, err := deserializer.Decode(req.Object.Raw, nil, obj); err != nil {
-		return nil, fmt.Errorf("error decoding generator resource %s: %w", req.Name, err)
-	}
-	obj.SetNamespace(req.Namespace)
-	obj.SetGroupVersionKind(
-		schema.GroupVersionKind{
-			Group:   req.Kind.Group,
-			Version: req.Kind.Version,
-			Kind:    req.Kind.Kind,
-		})
-
-	// Expand the generator and apply mutators to the resultant resources
-	// The base object is not mutated, so we do not need to specify its source
-	base := &mutationtypes.Mutable{
-		Object:    obj,
-		Namespace: review.Namespace,
-		Username:  req.AdmissionRequest.UserInfo.Username,
-	}
-	resultants, err := h.expansionSystem.Expand(base)
-	if err != nil {
-		return nil, fmt.Errorf("unable to expand object: %w", err)
+	resultants := []*expansion.Resultant{}
+	// Skip the expansion if admissionRequest.Obj is nil.
+	if req.AdmissionRequest.Object.Raw != 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 {
+			return nil, fmt.Errorf("error decoding generator resource %s: %w", req.Name, err)
+		}
+		obj.SetNamespace(req.Namespace)
+		obj.SetGroupVersionKind(
+			schema.GroupVersionKind{
+				Group:   req.Kind.Group,
+				Version: req.Kind.Version,
+				Kind:    req.Kind.Kind,
+			})
+
+		// Expand the generator and apply mutators to the resultant resources
+		// The base object is not mutated, so we do not need to specify its source
+		base := &mutationtypes.Mutable{
+			Object:    obj,
+			Namespace: review.Namespace,
+			Username:  req.AdmissionRequest.UserInfo.Username,
+		}
+		resultants, err = h.expansionSystem.Expand(base)
+		if err != nil {
+			return nil, fmt.Errorf("unable to expand object: %w", err)
+		}
 	}
 
 	trace, dump := h.tracingLevel(ctx, req)