Refactor admission webhook (#595)

- decouple the webhool server from the controller
- move tests to respective files

Gopkg.lock

@@ -1356,7 +1356,6 @@
     "k8s.io/client-go/kubernetes",
     "k8s.io/client-go/kubernetes/fake",
     "k8s.io/client-go/kubernetes/scheme",
-    "k8s.io/client-go/kubernetes/typed/admissionregistration/v1beta1",
     "k8s.io/client-go/kubernetes/typed/core/v1",
     "k8s.io/client-go/plugin/pkg/client/auth/gcp",
     "k8s.io/client-go/rest",

webhook/resource_admission_controller.go

@@ -0,0 +1,325 @@
+/*
+Copyright 2019 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package webhook
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"fmt"
+	"sort"
+	"strings"
+
+	"github.com/markbates/inflect"
+	"github.com/mattbaird/jsonpatch"
+	"go.uber.org/zap"
+	admissionv1beta1 "k8s.io/api/admission/v1beta1"
+	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/kubernetes"
+
+	"knative.dev/pkg/apis"
+	"knative.dev/pkg/apis/duck"
+	"knative.dev/pkg/kmp"
+	"knative.dev/pkg/logging"
+)
+
+// ResourceCallback defines a signature for resource specific (Route, Configuration, etc.)
+// handlers that can validate and mutate an object. If non-nil error is returned, object mutation
+// is denied. Mutations should be appended to the patches operations.
+type ResourceCallback func(patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error
+
+// ResourceDefaulter defines a signature for resource specific (Route, Configuration, etc.)
+// handlers that can set defaults on an object. If non-nil error is returned, object mutation
+// is denied. Mutations should be appended to the patches operations.
+type ResourceDefaulter func(patches *[]jsonpatch.JsonPatchOperation, crd GenericCRD) error
+
+// GenericCRD is the interface definition that allows us to perform the generic
+// CRD actions like deciding whether to increment generation and so forth.
+type GenericCRD interface {
+	apis.Defaultable
+	apis.Validatable
+	runtime.Object
+}
+
+type ResourceAdmissionController struct {
+	Handlers map[schema.GroupVersionKind]GenericCRD
+	Options  ControllerOptions
+
+	DisallowUnknownFields bool
+}
+
+func (ac *ResourceAdmissionController) Admit(ctx context.Context, request *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse {
+	logger := logging.FromContext(ctx)
+	switch request.Operation {
+	case admissionv1beta1.Create, admissionv1beta1.Update:
+	default:
+		logger.Infof("Unhandled webhook operation, letting it through %v", request.Operation)
+		return &admissionv1beta1.AdmissionResponse{Allowed: true}
+	}
+
+	patchBytes, err := ac.mutate(ctx, request)
+	if err != nil {
+		return makeErrorStatus("mutation failed: %v", err)
+	}
+	logger.Infof("Kind: %q PatchBytes: %v", request.Kind, string(patchBytes))
+
+	return &admissionv1beta1.AdmissionResponse{
+		Patch:   patchBytes,
+		Allowed: true,
+		PatchType: func() *admissionv1beta1.PatchType {
+			pt := admissionv1beta1.PatchTypeJSONPatch
+			return &pt
+		}(),
+	}
+}
+
+func (ac *ResourceAdmissionController) Register(ctx context.Context, kubeClient kubernetes.Interface, caCert []byte) error {
+	client := kubeClient.AdmissionregistrationV1beta1().MutatingWebhookConfigurations()
+	logger := logging.FromContext(ctx)
+	failurePolicy := admissionregistrationv1beta1.Fail
+
+	var rules []admissionregistrationv1beta1.RuleWithOperations
+	for gvk := range ac.Handlers {
+		plural := strings.ToLower(inflect.Pluralize(gvk.Kind))
+
+		rules = append(rules, admissionregistrationv1beta1.RuleWithOperations{
+			Operations: []admissionregistrationv1beta1.OperationType{
+				admissionregistrationv1beta1.Create,
+				admissionregistrationv1beta1.Update,
+			},
+			Rule: admissionregistrationv1beta1.Rule{
+				APIGroups:   []string{gvk.Group},
+				APIVersions: []string{gvk.Version},
+				Resources:   []string{plural + "/*"},
+			},
+		})
+	}
+
+	// Sort the rules by Group, Version, Kind so that things are deterministically ordered.
+	sort.Slice(rules, func(i, j int) bool {
+		lhs, rhs := rules[i], rules[j]
+		if lhs.APIGroups[0] != rhs.APIGroups[0] {
+			return lhs.APIGroups[0] < rhs.APIGroups[0]
+		}
+		if lhs.APIVersions[0] != rhs.APIVersions[0] {
+			return lhs.APIVersions[0] < rhs.APIVersions[0]
+		}
+		return lhs.Resources[0] < rhs.Resources[0]
+	})
+
+	webhook := &admissionregistrationv1beta1.MutatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: ac.Options.WebhookName,
+		},
+		Webhooks: []admissionregistrationv1beta1.Webhook{{
+			Name:  ac.Options.WebhookName,
+			Rules: rules,
+			ClientConfig: admissionregistrationv1beta1.WebhookClientConfig{
+				Service: &admissionregistrationv1beta1.ServiceReference{
+					Namespace: ac.Options.Namespace,
+					Name:      ac.Options.ServiceName,
+				},
+				CABundle: caCert,
+			},
+			FailurePolicy: &failurePolicy,
+		}},
+	}
+
+	// Set the owner to our deployment.
+	deployment, err := kubeClient.Apps().Deployments(ac.Options.Namespace).Get(ac.Options.DeploymentName, metav1.GetOptions{})
+	if err != nil {
+		return fmt.Errorf("failed to fetch our deployment: %v", err)
+	}
+	deploymentRef := metav1.NewControllerRef(deployment, deploymentKind)
+	webhook.OwnerReferences = append(webhook.OwnerReferences, *deploymentRef)
+
+	// Try to create the webhook and if it already exists validate webhook rules.
+	_, err = client.Create(webhook)
+	if err != nil {
+		if !apierrors.IsAlreadyExists(err) {
+			return fmt.Errorf("failed to create a webhook: %v", err)
+		}
+		logger.Info("Webhook already exists")
+		configuredWebhook, err := client.Get(ac.Options.WebhookName, metav1.GetOptions{})
+		if err != nil {
+			return fmt.Errorf("error retrieving webhook: %v", err)
+		}
+		if ok, err := kmp.SafeEqual(configuredWebhook.Webhooks, webhook.Webhooks); err != nil {
+			return fmt.Errorf("error diffing webhooks: %v", err)
+		} else if !ok {
+			logger.Info("Updating webhook")
+			// Set the ResourceVersion as required by update.
+			webhook.ObjectMeta.ResourceVersion = configuredWebhook.ObjectMeta.ResourceVersion
+			if _, err := client.Update(webhook); err != nil {
+				return fmt.Errorf("failed to update webhook: %s", err)
+			}
+		} else {
+			logger.Info("Webhook is already valid")
+		}
+	} else {
+		logger.Info("Created a webhook")
+	}
+	return nil
+}
+
+func (ac *ResourceAdmissionController) mutate(ctx context.Context, req *admissionv1beta1.AdmissionRequest) ([]byte, error) {
+	kind := req.Kind
+	newBytes := req.Object.Raw
+	oldBytes := req.OldObject.Raw
+	// Why, oh why are these different types...
+	gvk := schema.GroupVersionKind{
+		Group:   kind.Group,
+		Version: kind.Version,
+		Kind:    kind.Kind,
+	}
+
+	logger := logging.FromContext(ctx)
+	handler, ok := ac.Handlers[gvk]
+	if !ok {
+		logger.Errorf("Unhandled kind: %v", gvk)
+		return nil, fmt.Errorf("unhandled kind: %v", gvk)
+	}
+
+	// nil values denote absence of `old` (create) or `new` (delete) objects.
+	var oldObj, newObj GenericCRD
+
+	if len(newBytes) != 0 {
+		newObj = handler.DeepCopyObject().(GenericCRD)
+		newDecoder := json.NewDecoder(bytes.NewBuffer(newBytes))
+		if ac.DisallowUnknownFields {
+			newDecoder.DisallowUnknownFields()
+		}
+		if err := newDecoder.Decode(&newObj); err != nil {
+			return nil, fmt.Errorf("cannot decode incoming new object: %v", err)
+		}
+	}
+	if len(oldBytes) != 0 {
+		oldObj = handler.DeepCopyObject().(GenericCRD)
+		oldDecoder := json.NewDecoder(bytes.NewBuffer(oldBytes))
+		if ac.DisallowUnknownFields {
+			oldDecoder.DisallowUnknownFields()
+		}
+		if err := oldDecoder.Decode(&oldObj); err != nil {
+			return nil, fmt.Errorf("cannot decode incoming old object: %v", err)
+		}
+	}
+	var patches duck.JSONPatch
+
+	var err error
+	// Skip this step if the type we're dealing with is a duck type, since it is inherently
+	// incomplete and this will patch away all of the unspecified fields.
+	if _, ok := newObj.(duck.Populatable); !ok {
+		// Add these before defaulting fields, otherwise defaulting may cause an illegal patch
+		// because it expects the round tripped through Golang fields to be present already.
+		rtp, err := roundTripPatch(newBytes, newObj)
+		if err != nil {
+			return nil, fmt.Errorf("cannot create patch for round tripped newBytes: %v", err)
+		}
+		patches = append(patches, rtp...)
+	}
+
+	// Set up the context for defaulting and validation
+	if oldObj != nil {
+		// Copy the old object and set defaults so that we don't reject our own
+		// defaulting done earlier in the webhook.
+		oldObj = oldObj.DeepCopyObject().(GenericCRD)
+		oldObj.SetDefaults(ctx)
+
+		s, ok := oldObj.(apis.HasSpec)
+		if ok {
+			SetUserInfoAnnotations(s, ctx, req.Resource.Group)
+		}
+
+		if req.SubResource == "" {
+			ctx = apis.WithinUpdate(ctx, oldObj)
+		} else {
+			ctx = apis.WithinSubResourceUpdate(ctx, oldObj, req.SubResource)
+		}
+	} else {
+		ctx = apis.WithinCreate(ctx)
+	}
+	ctx = apis.WithUserInfo(ctx, &req.UserInfo)
+
+	// Default the new object.
+	if patches, err = setDefaults(ctx, patches, newObj); err != nil {
+		logger.Errorw("Failed the resource specific defaulter", zap.Error(err))
+		// Return the error message as-is to give the defaulter callback
+		// discretion over (our portion of) the message that the user sees.
+		return nil, err
+	}
+
+	if patches, err = ac.setUserInfoAnnotations(ctx, patches, newObj, req.Resource.Group); err != nil {
+		logger.Errorw("Failed the resource user info annotator", zap.Error(err))
+		return nil, err
+	}
+
+	// None of the validators will accept a nil value for newObj.
+	if newObj == nil {
+		return nil, errMissingNewObject
+	}
+	if err := validate(ctx, newObj); err != nil {
+		logger.Errorw("Failed the resource specific validation", zap.Error(err))
+		// Return the error message as-is to give the validation callback
+		// discretion over (our portion of) the message that the user sees.
+		return nil, err
+	}
+
+	return json.Marshal(patches)
+}
+
+func (ac *ResourceAdmissionController) setUserInfoAnnotations(ctx context.Context, patches duck.JSONPatch, new GenericCRD, groupName string) (duck.JSONPatch, error) {
+	if new == nil {
+		return patches, nil
+	}
+	nh, ok := new.(apis.HasSpec)
+	if !ok {
+		return patches, nil
+	}
+
+	b, a := new.DeepCopyObject().(apis.HasSpec), nh
+
+	SetUserInfoAnnotations(nh, ctx, groupName)
+
+	patch, err := duck.CreatePatch(b, a)
+	if err != nil {
+		return nil, err
+	}
+	return append(patches, patch...), nil
+}
+
+// roundTripPatch generates the JSONPatch that corresponds to round tripping the given bytes through
+// the Golang type (JSON -> Golang type -> JSON). Because it is not always true that
+// bytes == json.Marshal(json.Unmarshal(bytes)).
+//
+// For example, if bytes did not contain a 'spec' field and the Golang type specifies its 'spec'
+// field without omitempty, then by round tripping through the Golang type, we would have added
+// `'spec': {}`.
+func roundTripPatch(bytes []byte, unmarshalled interface{}) (duck.JSONPatch, error) {
+	if unmarshalled == nil {
+		return duck.JSONPatch{}, nil
+	}
+	marshaledBytes, err := json.Marshal(unmarshalled)
+	if err != nil {
+		return nil, fmt.Errorf("cannot marshal interface: %v", err)
+	}
+	return jsonpatch.CreatePatch(bytes, marshaledBytes)
+}

webhook/resource_admission_controller_deprecated_test.go

@@ -504,8 +504,8 @@ func TestStrictValidation(t *testing.T) {
 				ctx = apis.DisallowDeprecated(ctx)
 			}
 
-			_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
+			_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
-			resp := ac.admit(ctx, tc.req)
+			resp := ac.Admit(ctx, tc.req)
 
 			if len(tc.wantErrs) > 0 {
 				for _, err := range tc.wantErrs {
@@ -534,8 +534,8 @@ func TestStrictValidation_Spec_Create(t *testing.T) {
 
 	ctx := apis.DisallowDeprecated(TestContextWithLogger(t))
 
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
-	resp := ac.admit(ctx, req)
+	resp := ac.Admit(ctx, req)
 
 	expectFailsWith(t, resp, "must not set")
 	expectFailsWith(t, resp, "spec.field")
@@ -558,8 +558,8 @@ func TestStrictValidation_Spec_Update(t *testing.T) {
 
 	ctx := apis.DisallowDeprecated(TestContextWithLogger(t))
 
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
-	resp := ac.admit(ctx, req)
+	resp := ac.Admit(ctx, req)
 
 	expectFailsWith(t, resp, "must not update")
 	expectFailsWith(t, resp, "spec.field")

webhook/resource_admission_controller_test.go

@@ -0,0 +1,626 @@
+/*
+Copyright 2017 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package webhook
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"reflect"
+	"sort"
+	"strings"
+	"testing"
+
+	// "knative.dev/pkg/apis/duck"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/mattbaird/jsonpatch"
+	admissionv1beta1 "k8s.io/api/admission/v1beta1"
+	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
+	appsv1 "k8s.io/api/apps/v1"
+	authenticationv1 "k8s.io/api/authentication/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
+	fakekubeclientset "k8s.io/client-go/kubernetes/fake"
+	"knative.dev/pkg/apis"
+
+	. "knative.dev/pkg/logging/testing"
+	. "knative.dev/pkg/testing"
+)
+
+func newNonRunningTestResourceAdmissionController(t *testing.T, options ControllerOptions) (
+	kubeClient *fakekubeclientset.Clientset,
+	ac *ResourceAdmissionController) {
+	t.Helper()
+	// Create fake clients
+	kubeClient = fakekubeclientset.NewSimpleClientset()
+
+	ac = NewTestResourceAdmissionController(options)
+	return
+}
+
+func TestDeleteAllowed(t *testing.T) {
+	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Delete,
+	}
+
+	if resp := ac.Admit(TestContextWithLogger(t), req); !resp.Allowed {
+		t.Fatal("Unexpected denial of delete")
+	}
+}
+
+func TestConnectAllowed(t *testing.T) {
+	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Connect,
+	}
+
+	resp := ac.Admit(TestContextWithLogger(t), req)
+	if !resp.Allowed {
+		t.Fatalf("Unexpected denial of connect")
+	}
+}
+
+func TestUnknownKindFails(t *testing.T) {
+	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Garbage",
+		},
+	}
+
+	expectFailsWith(t, ac.Admit(TestContextWithLogger(t), req), "unhandled kind")
+}
+
+func TestUnknownVersionFails(t *testing.T) {
+	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1beta2",
+			Kind:    "Resource",
+		},
+	}
+	expectFailsWith(t, ac.Admit(TestContextWithLogger(t), req), "unhandled kind")
+}
+
+func TestUnknownFieldFails(t *testing.T) {
+	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+	}
+
+	marshaled, err := json.Marshal(map[string]interface{}{
+		"spec": map[string]interface{}{
+			"foo": "bar",
+		},
+	})
+	if err != nil {
+		panic("failed to marshal resource")
+	}
+	req.Object.Raw = marshaled
+
+	expectFailsWith(t, ac.Admit(TestContextWithLogger(t), req),
+		`mutation failed: cannot decode incoming new object: json: unknown field "foo"`)
+}
+
+func TestAdmitCreates(t *testing.T) {
+	tests := []struct {
+		name      string
+		setup     func(context.Context, *Resource)
+		rejection string
+		patches   []jsonpatch.JsonPatchOperation
+	}{{
+		name: "test simple creation (alpha, no diff)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.TypeMeta.APIVersion = "v1alpha1"
+			r.SetDefaults(ctx)
+			r.Annotations = map[string]string{
+				"pkg.knative.dev/creator":      user1,
+				"pkg.knative.dev/lastModifier": user1,
+			}
+		},
+		patches: []jsonpatch.JsonPatchOperation{},
+	}, {
+		name: "test simple creation (beta, no diff)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.TypeMeta.APIVersion = "v1beta1"
+			r.SetDefaults(ctx)
+			r.Annotations = map[string]string{
+				"pkg.knative.dev/creator":      user1,
+				"pkg.knative.dev/lastModifier": user1,
+			}
+		},
+		patches: []jsonpatch.JsonPatchOperation{},
+	}, {
+		name: "test simple creation (with defaults)",
+		setup: func(ctx context.Context, r *Resource) {
+		},
+		patches: []jsonpatch.JsonPatchOperation{{
+			Operation: "add",
+			Path:      "/metadata/annotations",
+			Value: map[string]interface{}{
+				"pkg.knative.dev/creator":      user1,
+				"pkg.knative.dev/lastModifier": user1,
+			},
+		}, {
+			Operation: "add",
+			Path:      "/spec/fieldThatsImmutableWithDefault",
+			Value:     "this is another default value",
+		}, {
+			Operation: "add",
+			Path:      "/spec/fieldWithDefault",
+			Value:     "I'm a default.",
+		}},
+	}, {
+		name: "test simple creation (with defaults around annotations)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.Annotations = map[string]string{
+				"foo": "bar",
+			}
+		},
+		patches: []jsonpatch.JsonPatchOperation{{
+			Operation: "add",
+			Path:      "/metadata/annotations/pkg.knative.dev~1creator",
+			Value:     user1,
+		}, {
+			Operation: "add",
+			Path:      "/metadata/annotations/pkg.knative.dev~1lastModifier",
+			Value:     user1,
+		}, {
+			Operation: "add",
+			Path:      "/spec/fieldThatsImmutableWithDefault",
+			Value:     "this is another default value",
+		}, {
+			Operation: "add",
+			Path:      "/spec/fieldWithDefault",
+			Value:     "I'm a default.",
+		}},
+	}, {
+		name: "test simple creation (with partially overridden defaults)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.Spec.FieldThatsImmutableWithDefault = "not the default"
+		},
+		patches: []jsonpatch.JsonPatchOperation{{
+			Operation: "add",
+			Path:      "/metadata/annotations",
+			Value: map[string]interface{}{
+				"pkg.knative.dev/creator":      user1,
+				"pkg.knative.dev/lastModifier": user1,
+			},
+		}, {
+			Operation: "add",
+			Path:      "/spec/fieldWithDefault",
+			Value:     "I'm a default.",
+		}},
+	}, {
+		name: "test simple creation (webhook corrects user annotation)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+			// THIS IS NOT WHO IS CREATING IT, IT IS LIES!
+			r.Annotations = map[string]string{
+				"pkg.knative.dev/lastModifier": user2,
+			}
+		},
+		patches: []jsonpatch.JsonPatchOperation{{
+			Operation: "replace",
+			Path:      "/metadata/annotations/pkg.knative.dev~1lastModifier",
+			Value:     user1,
+		}, {
+			Operation: "add",
+			Path:      "/metadata/annotations/pkg.knative.dev~1creator",
+			Value:     user1,
+		}},
+	}, {
+		name: "with bad field",
+		setup: func(ctx context.Context, r *Resource) {
+			// Put a bad value in.
+			r.Spec.FieldWithValidation = "not what's expected"
+		},
+		rejection: "invalid value",
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			r := createResource("a name")
+			ctx := apis.WithinCreate(apis.WithUserInfo(
+				TestContextWithLogger(t),
+				&authenticationv1.UserInfo{Username: user1}))
+
+			// Setup the resource.
+			tc.setup(ctx, r)
+
+			_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+			resp := ac.Admit(ctx, createCreateResource(ctx, r))
+
+			if tc.rejection == "" {
+				expectAllowed(t, resp)
+				expectPatches(t, resp.Patch, tc.patches)
+			} else {
+				expectFailsWith(t, resp, tc.rejection)
+			}
+		})
+	}
+}
+
+func createCreateResource(ctx context.Context, r *Resource) *admissionv1beta1.AdmissionRequest {
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+		UserInfo: *apis.GetUserInfo(ctx),
+	}
+	marshaled, err := json.Marshal(r)
+	if err != nil {
+		panic("failed to marshal resource")
+	}
+	req.Object.Raw = marshaled
+	req.Resource.Group = "pkg.knative.dev"
+	return req
+}
+
+func TestAdmitUpdates(t *testing.T) {
+	tests := []struct {
+		name      string
+		setup     func(context.Context, *Resource)
+		mutate    func(context.Context, *Resource)
+		rejection string
+		patches   []jsonpatch.JsonPatchOperation
+	}{{
+		name: "test simple update (no diff)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+		},
+		mutate: func(ctx context.Context, r *Resource) {
+			// If we don't change anything, the updater
+			// annotation doesn't change.
+		},
+		patches: []jsonpatch.JsonPatchOperation{},
+	}, {
+		name: "test simple update (update updater annotation)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+		},
+		mutate: func(ctx context.Context, r *Resource) {
+			// When we change the spec, the updater
+			// annotation changes.
+			r.Spec.FieldWithDefault = "not the default"
+		},
+		patches: []jsonpatch.JsonPatchOperation{{
+			Operation: "replace",
+			Path:      "/metadata/annotations/pkg.knative.dev~1lastModifier",
+			Value:     user2,
+		}},
+	}, {
+		name: "test simple update (annotation change doesn't change updater)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+		},
+		mutate: func(ctx context.Context, r *Resource) {
+			// When we change an annotation, the updater doesn't change.
+			r.Annotations["foo"] = "bar"
+		},
+		patches: []jsonpatch.JsonPatchOperation{},
+	}, {
+		name: "test that updates dropping immutable defaults are filled back in",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+			r.Spec.FieldThatsImmutableWithDefault = ""
+		},
+		mutate: func(ctx context.Context, r *Resource) {
+			r.Spec.FieldThatsImmutableWithDefault = ""
+		},
+		patches: []jsonpatch.JsonPatchOperation{{
+			Operation: "add",
+			Path:      "/spec/fieldThatsImmutableWithDefault",
+			Value:     "this is another default value",
+		}},
+	}, {
+		name: "bad mutation (immutable)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+		},
+		mutate: func(ctx context.Context, r *Resource) {
+			r.Spec.FieldThatsImmutableWithDefault = "something different"
+		},
+		rejection: "Immutable field changed",
+	}, {
+		name: "bad mutation (validation)",
+		setup: func(ctx context.Context, r *Resource) {
+			r.SetDefaults(ctx)
+		},
+		mutate: func(ctx context.Context, r *Resource) {
+			r.Spec.FieldWithValidation = "not what's expected"
+		},
+		rejection: "invalid value",
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			old := createResource("a name")
+			ctx := TestContextWithLogger(t)
+
+			old.Annotations = map[string]string{
+				"pkg.knative.dev/creator":      user1,
+				"pkg.knative.dev/lastModifier": user1,
+			}
+
+			tc.setup(ctx, old)
+
+			new := old.DeepCopy()
+
+			// Mutate the resource using the update context as user2
+			ctx = apis.WithUserInfo(apis.WithinUpdate(ctx, old),
+				&authenticationv1.UserInfo{Username: user2})
+			tc.mutate(ctx, new)
+
+			_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+			resp := ac.Admit(ctx, createUpdateResource(ctx, old, new))
+
+			if tc.rejection == "" {
+				expectAllowed(t, resp)
+				expectPatches(t, resp.Patch, tc.patches)
+			} else {
+				expectFailsWith(t, resp, tc.rejection)
+			}
+		})
+	}
+}
+
+func createUpdateResource(ctx context.Context, old, new *Resource) *admissionv1beta1.AdmissionRequest {
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Update,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+		UserInfo: *apis.GetUserInfo(ctx),
+	}
+	marshaled, err := json.Marshal(new)
+	if err != nil {
+		panic("failed to marshal resource")
+	}
+	req.Object.Raw = marshaled
+	marshaledOld, err := json.Marshal(old)
+	if err != nil {
+		panic("failed to marshal resource")
+	}
+	req.OldObject.Raw = marshaledOld
+	req.Resource.Group = "pkg.knative.dev"
+	return req
+}
+
+func TestValidCreateResourceSucceedsWithRoundTripAndDefaultPatch(t *testing.T) {
+	req := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "InnerDefaultResource",
+		},
+	}
+	req.Object.Raw = createInnerDefaultResourceWithoutSpec(t)
+
+	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	resp := ac.Admit(TestContextWithLogger(t), req)
+	expectAllowed(t, resp)
+	expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{
+		Operation: "add",
+		Path:      "/spec",
+		Value:     map[string]interface{}{},
+	}, {
+		Operation: "add",
+		Path:      "/spec/fieldWithDefault",
+		Value:     "I'm a default.",
+	}})
+}
+
+func createInnerDefaultResourceWithoutSpec(t *testing.T) []byte {
+	t.Helper()
+	r := InnerDefaultResource{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: testNamespace,
+			Name:      "a name",
+		},
+	}
+	// Remove the 'spec' field of the generated JSON by marshaling it to JSON, parsing that as a
+	// generic map[string]interface{}, removing 'spec', and marshaling it again.
+	origBytes, err := json.Marshal(r)
+	if err != nil {
+		t.Fatalf("Error marshaling origBytes: %v", err)
+	}
+	var q map[string]interface{}
+	if err := json.Unmarshal(origBytes, &q); err != nil {
+		t.Fatalf("Error unmarshaling origBytes: %v", err)
+	}
+	delete(q, "spec")
+	b, err := json.Marshal(q)
+	if err != nil {
+		t.Fatalf("Error marshaling q: %v", err)
+	}
+	return b
+}
+
+func createInnerDefaultResourceWithSpecAndStatus(t *testing.T, spec *InnerDefaultSpec, status *InnerDefaultStatus) []byte {
+	t.Helper()
+	r := InnerDefaultResource{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: testNamespace,
+			Name:      "a name",
+		},
+	}
+	if spec != nil {
+		r.Spec = *spec
+	}
+	if status != nil {
+		r.Status = *status
+	}
+
+	b, err := json.Marshal(r)
+	if err != nil {
+		t.Fatalf("Error marshaling bytes: %v", err)
+	}
+	return b
+}
+
+func TestValidWebhook(t *testing.T) {
+	kubeClient, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	createDeployment(kubeClient)
+	err := ac.Register(TestContextWithLogger(t), kubeClient, []byte{})
+	if err != nil {
+		t.Fatalf("Failed to create webhook: %s", err)
+	}
+}
+
+func TestUpdatingWebhook(t *testing.T) {
+	kubeClient, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	webhook := &admissionregistrationv1beta1.MutatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: ac.Options.WebhookName,
+		},
+		Webhooks: []admissionregistrationv1beta1.Webhook{{
+			Name:         ac.Options.WebhookName,
+			Rules:        []admissionregistrationv1beta1.RuleWithOperations{{}},
+			ClientConfig: admissionregistrationv1beta1.WebhookClientConfig{},
+		}},
+	}
+
+	createDeployment(kubeClient)
+	createWebhook(kubeClient, webhook)
+	err := ac.Register(TestContextWithLogger(t), kubeClient, []byte{})
+	if err != nil {
+		t.Fatalf("Failed to create webhook: %s", err)
+	}
+
+	currentWebhook, _ := kubeClient.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Get(ac.Options.WebhookName, metav1.GetOptions{})
+	if reflect.DeepEqual(currentWebhook.Webhooks, webhook.Webhooks) {
+		t.Fatalf("Expected webhook to be updated")
+	}
+}
+
+func createDeployment(kubeClient kubernetes.Interface) {
+	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "whatever",
+			Namespace: "knative-something",
+		},
+	}
+	kubeClient.Apps().Deployments("knative-something").Create(deployment)
+}
+
+func createWebhook(kubeClient kubernetes.Interface, webhook *admissionregistrationv1beta1.MutatingWebhookConfiguration) {
+	client := kubeClient.AdmissionregistrationV1beta1().MutatingWebhookConfigurations()
+	_, err := client.Create(webhook)
+	if err != nil {
+		panic(fmt.Sprintf("failed to create test webhook: %s", err))
+	}
+}
+
+func expectAllowed(t *testing.T, resp *admissionv1beta1.AdmissionResponse) {
+	t.Helper()
+	if !resp.Allowed {
+		t.Errorf("Expected allowed, but failed with %+v", resp.Result)
+	}
+}
+
+func expectFailsWith(t *testing.T, resp *admissionv1beta1.AdmissionResponse, contains string) {
+	t.Helper()
+	if resp.Allowed {
+		t.Error("Expected denial, got allowed")
+		return
+	}
+	if !strings.Contains(resp.Result.Message, contains) {
+		t.Errorf("Expected failure containing %q got %q", contains, resp.Result.Message)
+	}
+}
+
+func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) {
+	t.Helper()
+	var got []jsonpatch.JsonPatchOperation
+
+	err := json.Unmarshal(a, &got)
+	if err != nil {
+		t.Errorf("Failed to unmarshal patches: %s", err)
+		return
+	}
+
+	// Give the patch a deterministic ordering.
+	// Technically this can change the meaning, but the ordering is otherwise unstable
+	// and difficult to test.
+	sort.Slice(e, func(i, j int) bool {
+		lhs, rhs := e[i], e[j]
+		if lhs.Operation != rhs.Operation {
+			return lhs.Operation < rhs.Operation
+		}
+		return lhs.Path < rhs.Path
+	})
+	sort.Slice(got, func(i, j int) bool {
+		lhs, rhs := got[i], got[j]
+		if lhs.Operation != rhs.Operation {
+			return lhs.Operation < rhs.Operation
+		}
+		return lhs.Path < rhs.Path
+	})
+
+	// Even though diff is useful, seeing the whole objects
+	// one under another helps a lot.
+	t.Logf("Got Patches:  %#v", got)
+	t.Logf("Want Patches: %#v", e)
+	if diff := cmp.Diff(e, got, cmpopts.EquateEmpty()); diff != "" {
+		t.Logf("diff Patches: %v", diff)
+		t.Errorf("expectPatches (-want, +got) = %s", diff)
+	}
+}
+
+func setUserAnnotation(userC, userU string) jsonpatch.JsonPatchOperation {
+	return jsonpatch.JsonPatchOperation{
+		Operation: "add",
+		Path:      "/metadata/annotations",
+		Value: map[string]interface{}{
+			"pkg.knative.dev/creator":      userC,
+			"pkg.knative.dev/lastModifier": userU,
+		},
+	}
+}
+
+func NewTestResourceAdmissionController(options ControllerOptions) *ResourceAdmissionController {
+	// Use different versions and domains, for coverage.
+	handlers := newHandlers()
+	return &ResourceAdmissionController{
+		Handlers:              handlers,
+		Options:               options,
+		DisallowUnknownFields: true,
+	}
+}

webhook/webhook.go

@@ -17,7 +17,6 @@ limitations under the License.
 package webhook
 
 import (
-	"bytes"
 	"context"
 	"crypto/tls"
 	"crypto/x509"
@@ -25,30 +24,22 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"sort"
-	"strings"
 	"time"
 
 	"go.uber.org/zap"
 
 	"knative.dev/pkg/apis"
 	"knative.dev/pkg/apis/duck"
-	"knative.dev/pkg/kmp"
 	"knative.dev/pkg/logging"
 	"knative.dev/pkg/logging/logkey"
 
-	"github.com/markbates/inflect"
-	"github.com/mattbaird/jsonpatch"
 	admissionv1beta1 "k8s.io/api/admission/v1beta1"
-	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
-	clientadmissionregistrationv1beta1 "k8s.io/client-go/kubernetes/typed/admissionregistration/v1beta1"
 )
 
 const (
@@ -105,34 +96,15 @@ type ControllerOptions struct {
 	StatsReporter StatsReporter
 }
 
-// ResourceCallback defines a signature for resource specific (Route, Configuration, etc.)
-// handlers that can validate and mutate an object. If non-nil error is returned, object creation
-// is denied. Mutations should be appended to the patches operations.
-type ResourceCallback func(patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error
-
-// ResourceDefaulter defines a signature for resource specific (Route, Configuration, etc.)
-// handlers that can set defaults on an object. If non-nil error is returned, object creation
-// is denied. Mutations should be appended to the patches operations.
-type ResourceDefaulter func(patches *[]jsonpatch.JsonPatchOperation, crd GenericCRD) error
-
 // AdmissionController implements the external admission webhook for validation of
 // pilot configuration.
 type AdmissionController struct {
-	Client   kubernetes.Interface
+	Client                      kubernetes.Interface
-	Options  ControllerOptions
+	Options                     ControllerOptions
-	Handlers map[schema.GroupVersionKind]GenericCRD
+	Logger                      *zap.SugaredLogger
-	Logger   *zap.SugaredLogger
+	resourceAdmissionController ResourceAdmissionController
 
-	WithContext           func(context.Context) context.Context
+	WithContext func(context.Context) context.Context
-	DisallowUnknownFields bool
-}
-
-// GenericCRD is the interface definition that allows us to perform the generic
-// CRD actions like deciding whether to increment generation and so forth.
-type GenericCRD interface {
-	apis.Defaultable
-	apis.Validatable
-	runtime.Object
 }
 
 // NewAdmissionController constructs an AdmissionController
@@ -153,12 +125,15 @@ func NewAdmissionController(
 	}
 
 	return &AdmissionController{
-		Client:                client,
+		Client:  client,
-		Options:               opts,
+		Options: opts,
-		Handlers:              handlers,
+		resourceAdmissionController: ResourceAdmissionController{
-		Logger:                logger,
+			Handlers:              handlers,
-		WithContext:           ctx,
+			Options:               opts,
-		DisallowUnknownFields: disallowUnknownFields,
+			DisallowUnknownFields: disallowUnknownFields,
+		},
+		Logger:      logger,
+		WithContext: ctx,
 	}, nil
 }
 
@@ -238,7 +213,7 @@ func getOrGenerateKeyCertsFromSecret(ctx context.Context, client kubernetes.Inte
 // validate performs validation on the provided "new" CRD.
 // For legacy purposes, this also does apis.Immutable validation,
 // which is deprecated and will be removed in a future release.
-func validate(ctx context.Context, new GenericCRD) error {
+func validate(ctx context.Context, new apis.Validatable) error {
 	if apis.IsInUpdate(ctx) {
 		old := apis.GetBaseline(ctx)
 		if immutableNew, ok := new.(apis.Immutable); ok {
@@ -317,8 +292,7 @@ func (ac *AdmissionController) Run(stop <-chan struct{}) error {
 
 	select {
 	case <-time.After(ac.Options.RegistrationDelay):
-		cl := ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations()
+		if err := ac.resourceAdmissionController.Register(ctx, ac.Client, caCert); err != nil {
-		if err := ac.register(ctx, cl, caCert); err != nil {
 			logger.Errorw("failed to register webhook", zap.Error(err))
 			return err
 		}
@@ -343,97 +317,6 @@ func (ac *AdmissionController) Run(stop <-chan struct{}) error {
 	}
 }
 
-// Register registers the external admission webhook for pilot
-// configuration types.
-func (ac *AdmissionController) register(
-	ctx context.Context, client clientadmissionregistrationv1beta1.MutatingWebhookConfigurationInterface, caCert []byte) error { // nolint: lll
-	logger := logging.FromContext(ctx)
-	failurePolicy := admissionregistrationv1beta1.Fail
-
-	var rules []admissionregistrationv1beta1.RuleWithOperations
-	for gvk := range ac.Handlers {
-		plural := strings.ToLower(inflect.Pluralize(gvk.Kind))
-
-		rules = append(rules, admissionregistrationv1beta1.RuleWithOperations{
-			Operations: []admissionregistrationv1beta1.OperationType{
-				admissionregistrationv1beta1.Create,
-				admissionregistrationv1beta1.Update,
-			},
-			Rule: admissionregistrationv1beta1.Rule{
-				APIGroups:   []string{gvk.Group},
-				APIVersions: []string{gvk.Version},
-				Resources:   []string{plural + "/*"},
-			},
-		})
-	}
-
-	// Sort the rules by Group, Version, Kind so that things are deterministically ordered.
-	sort.Slice(rules, func(i, j int) bool {
-		lhs, rhs := rules[i], rules[j]
-		if lhs.APIGroups[0] != rhs.APIGroups[0] {
-			return lhs.APIGroups[0] < rhs.APIGroups[0]
-		}
-		if lhs.APIVersions[0] != rhs.APIVersions[0] {
-			return lhs.APIVersions[0] < rhs.APIVersions[0]
-		}
-		return lhs.Resources[0] < rhs.Resources[0]
-	})
-
-	webhook := &admissionregistrationv1beta1.MutatingWebhookConfiguration{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: ac.Options.WebhookName,
-		},
-		Webhooks: []admissionregistrationv1beta1.Webhook{{
-			Name:  ac.Options.WebhookName,
-			Rules: rules,
-			ClientConfig: admissionregistrationv1beta1.WebhookClientConfig{
-				Service: &admissionregistrationv1beta1.ServiceReference{
-					Namespace: ac.Options.Namespace,
-					Name:      ac.Options.ServiceName,
-				},
-				CABundle: caCert,
-			},
-			FailurePolicy: &failurePolicy,
-		}},
-	}
-
-	// Set the owner to our deployment.
-	deployment, err := ac.Client.Apps().Deployments(ac.Options.Namespace).Get(ac.Options.DeploymentName, metav1.GetOptions{})
-	if err != nil {
-		return fmt.Errorf("failed to fetch our deployment: %v", err)
-	}
-	deploymentRef := metav1.NewControllerRef(deployment, deploymentKind)
-	webhook.OwnerReferences = append(webhook.OwnerReferences, *deploymentRef)
-
-	// Try to create the webhook and if it already exists validate webhook rules.
-	_, err = client.Create(webhook)
-	if err != nil {
-		if !apierrors.IsAlreadyExists(err) {
-			return fmt.Errorf("failed to create a webhook: %v", err)
-		}
-		logger.Info("Webhook already exists")
-		configuredWebhook, err := client.Get(ac.Options.WebhookName, metav1.GetOptions{})
-		if err != nil {
-			return fmt.Errorf("error retrieving webhook: %v", err)
-		}
-		if ok, err := kmp.SafeEqual(configuredWebhook.Webhooks, webhook.Webhooks); err != nil {
-			return fmt.Errorf("error diffing webhooks: %v", err)
-		} else if !ok {
-			logger.Info("Updating webhook")
-			// Set the ResourceVersion as required by update.
-			webhook.ObjectMeta.ResourceVersion = configuredWebhook.ObjectMeta.ResourceVersion
-			if _, err := client.Update(webhook); err != nil {
-				return fmt.Errorf("failed to update webhook: %s", err)
-			}
-		} else {
-			logger.Info("Webhook is already valid")
-		}
-	} else {
-		logger.Info("Created a webhook")
-	}
-	return nil
-}
-
 // ServeHTTP implements the external admission webhook for mutating
 // serving resources.
 func (ac *AdmissionController) ServeHTTP(w http.ResponseWriter, r *http.Request) {
@@ -468,7 +351,7 @@ func (ac *AdmissionController) ServeHTTP(w http.ResponseWriter, r *http.Request)
 		ctx = ac.WithContext(ctx)
 	}
 
-	reviewResponse := ac.admit(ctx, review.Request)
+	reviewResponse := ac.resourceAdmissionController.Admit(ctx, review.Request)
 	var response admissionv1beta1.AdmissionReview
 	if reviewResponse != nil {
 		response.Response = reviewResponse
@@ -497,174 +380,6 @@ func makeErrorStatus(reason string, args ...interface{}) *admissionv1beta1.Admis
 	}
 }
 
-func (ac *AdmissionController) admit(ctx context.Context, request *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse {
-	logger := logging.FromContext(ctx)
-	switch request.Operation {
-	case admissionv1beta1.Create, admissionv1beta1.Update:
-	default:
-		logger.Infof("Unhandled webhook operation, letting it through %v", request.Operation)
-		return &admissionv1beta1.AdmissionResponse{Allowed: true}
-	}
-
-	patchBytes, err := ac.mutate(ctx, request)
-	if err != nil {
-		return makeErrorStatus("mutation failed: %v", err)
-	}
-	logger.Infof("Kind: %q PatchBytes: %v", request.Kind, string(patchBytes))
-
-	return &admissionv1beta1.AdmissionResponse{
-		Patch:   patchBytes,
-		Allowed: true,
-		PatchType: func() *admissionv1beta1.PatchType {
-			pt := admissionv1beta1.PatchTypeJSONPatch
-			return &pt
-		}(),
-	}
-}
-
-func (ac *AdmissionController) mutate(ctx context.Context, req *admissionv1beta1.AdmissionRequest) ([]byte, error) {
-	kind := req.Kind
-	newBytes := req.Object.Raw
-	oldBytes := req.OldObject.Raw
-	// Why, oh why are these different types...
-	gvk := schema.GroupVersionKind{
-		Group:   kind.Group,
-		Version: kind.Version,
-		Kind:    kind.Kind,
-	}
-
-	logger := logging.FromContext(ctx)
-	handler, ok := ac.Handlers[gvk]
-	if !ok {
-		logger.Errorf("Unhandled kind: %v", gvk)
-		return nil, fmt.Errorf("unhandled kind: %v", gvk)
-	}
-
-	// nil values denote absence of `old` (create) or `new` (delete) objects.
-	var oldObj, newObj GenericCRD
-
-	if len(newBytes) != 0 {
-		newObj = handler.DeepCopyObject().(GenericCRD)
-		newDecoder := json.NewDecoder(bytes.NewBuffer(newBytes))
-		if ac.DisallowUnknownFields {
-			newDecoder.DisallowUnknownFields()
-		}
-		if err := newDecoder.Decode(&newObj); err != nil {
-			return nil, fmt.Errorf("cannot decode incoming new object: %v", err)
-		}
-	}
-	if len(oldBytes) != 0 {
-		oldObj = handler.DeepCopyObject().(GenericCRD)
-		oldDecoder := json.NewDecoder(bytes.NewBuffer(oldBytes))
-		if ac.DisallowUnknownFields {
-			oldDecoder.DisallowUnknownFields()
-		}
-		if err := oldDecoder.Decode(&oldObj); err != nil {
-			return nil, fmt.Errorf("cannot decode incoming old object: %v", err)
-		}
-	}
-	var patches duck.JSONPatch
-
-	var err error
-	// Skip this step if the type we're dealing with is a duck type, since it is inherently
-	// incomplete and this will patch away all of the unspecified fields.
-	if _, ok := newObj.(duck.Populatable); !ok {
-		// Add these before defaulting fields, otherwise defaulting may cause an illegal patch
-		// because it expects the round tripped through Golang fields to be present already.
-		rtp, err := roundTripPatch(newBytes, newObj)
-		if err != nil {
-			return nil, fmt.Errorf("cannot create patch for round tripped newBytes: %v", err)
-		}
-		patches = append(patches, rtp...)
-	}
-
-	// Set up the context for defaulting and validation
-	if oldObj != nil {
-		// Copy the old object and set defaults so that we don't reject our own
-		// defaulting done earlier in the webhook.
-		oldObj = oldObj.DeepCopyObject().(GenericCRD)
-		oldObj.SetDefaults(ctx)
-
-		s, ok := oldObj.(apis.HasSpec)
-		if ok {
-			SetUserInfoAnnotations(s, ctx, req.Resource.Group)
-		}
-
-		if req.SubResource == "" {
-			ctx = apis.WithinUpdate(ctx, oldObj)
-		} else {
-			ctx = apis.WithinSubResourceUpdate(ctx, oldObj, req.SubResource)
-		}
-	} else {
-		ctx = apis.WithinCreate(ctx)
-	}
-	ctx = apis.WithUserInfo(ctx, &req.UserInfo)
-
-	// Default the new object.
-	if patches, err = setDefaults(ctx, patches, newObj); err != nil {
-		logger.Errorw("Failed the resource specific defaulter", zap.Error(err))
-		// Return the error message as-is to give the defaulter callback
-		// discretion over (our portion of) the message that the user sees.
-		return nil, err
-	}
-
-	if patches, err = ac.setUserInfoAnnotations(ctx, patches, newObj, req.Resource.Group); err != nil {
-		logger.Errorw("Failed the resource user info annotator", zap.Error(err))
-		return nil, err
-	}
-
-	// None of the validators will accept a nil value for newObj.
-	if newObj == nil {
-		return nil, errMissingNewObject
-	}
-	if err := validate(ctx, newObj); err != nil {
-		logger.Errorw("Failed the resource specific validation", zap.Error(err))
-		// Return the error message as-is to give the validation callback
-		// discretion over (our portion of) the message that the user sees.
-		return nil, err
-	}
-
-	return json.Marshal(patches)
-}
-
-func (ac *AdmissionController) setUserInfoAnnotations(ctx context.Context, patches duck.JSONPatch, new GenericCRD, groupName string) (duck.JSONPatch, error) {
-	if new == nil {
-		return patches, nil
-	}
-	nh, ok := new.(apis.HasSpec)
-	if !ok {
-		return patches, nil
-	}
-
-	b, a := new.DeepCopyObject().(apis.HasSpec), nh
-
-	SetUserInfoAnnotations(nh, ctx, groupName)
-
-	patch, err := duck.CreatePatch(b, a)
-	if err != nil {
-		return nil, err
-	}
-	return append(patches, patch...), nil
-}
-
-// roundTripPatch generates the JSONPatch that corresponds to round tripping the given bytes through
-// the Golang type (JSON -> Golang type -> JSON). Because it is not always true that
-// bytes == json.Marshal(json.Unmarshal(bytes)).
-//
-// For example, if bytes did not contain a 'spec' field and the Golang type specifies its 'spec'
-// field without omitempty, then by round tripping through the Golang type, we would have added
-// `'spec': {}`.
-func roundTripPatch(bytes []byte, unmarshalled interface{}) (duck.JSONPatch, error) {
-	if unmarshalled == nil {
-		return duck.JSONPatch{}, nil
-	}
-	marshaledBytes, err := json.Marshal(unmarshalled)
-	if err != nil {
-		return nil, fmt.Errorf("cannot marshal interface: %v", err)
-	}
-	return jsonpatch.CreatePatch(bytes, marshaledBytes)
-}
-
 func generateSecret(ctx context.Context, options *ControllerOptions) (*corev1.Secret, error) {
 	serverKey, serverCert, caCert, err := CreateCerts(ctx, options.ServiceName, options.Namespace)
 	if err != nil {

webhook/webhook_integration_test.go

@@ -464,19 +464,19 @@ func testSetup(t *testing.T) (*AdmissionController, string, error) {
 
 	defaultOpts := newDefaultOptions()
 	defaultOpts.Port = port
-	_, ac := newNonRunningTestAdmissionController(t, defaultOpts)
+	kubeClient, ac := newNonRunningTestAdmissionController(t, defaultOpts)
 
-	nsErr := createNamespace(t, ac.Client, metav1.NamespaceSystem)
+	nsErr := createNamespace(t, kubeClient, metav1.NamespaceSystem)
 	if nsErr != nil {
 		return nil, "", nsErr
 	}
 
-	cMapsErr := createTestConfigMap(t, ac.Client)
+	cMapsErr := createTestConfigMap(t, kubeClient)
 	if cMapsErr != nil {
 		return nil, "", cMapsErr
 	}
 
-	createDeployment(ac)
+	createDeployment(kubeClient)
 	resetMetrics()
 	return ac, fmt.Sprintf("0.0.0.0:%d", port), nil
 }
@@ -484,17 +484,17 @@ func testSetup(t *testing.T) (*AdmissionController, string, error) {
 func TestSetupWebhookHTTPServerError(t *testing.T) {
 	defaultOpts := newDefaultOptions()
 	defaultOpts.Port = -1 // invalid port
-	_, ac := newNonRunningTestAdmissionController(t, defaultOpts)
+	kubeClient, ac := newNonRunningTestAdmissionController(t, defaultOpts)
 
-	nsErr := createNamespace(t, ac.Client, metav1.NamespaceSystem)
+	nsErr := createNamespace(t, kubeClient, metav1.NamespaceSystem)
 	if nsErr != nil {
 		t.Fatalf("testSetup() = %v", nsErr)
 	}
-	cMapsErr := createTestConfigMap(t, ac.Client)
+	cMapsErr := createTestConfigMap(t, kubeClient)
 	if cMapsErr != nil {
 		t.Fatalf("testSetup() = %v", cMapsErr)
 	}
-	createDeployment(ac)
+	createDeployment(kubeClient)
 
 	stopCh := make(chan struct{})
 	errCh := make(chan error)

webhook/webhook_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 The Knative Authors
+Copyright 2019 The Knative Authors
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -17,34 +17,22 @@ limitations under the License.
 package webhook
 
 import (
-	"context"
 	"crypto/tls"
-	"encoding/json"
 	"encoding/pem"
 	"fmt"
 	"net"
-	"reflect"
-	"sort"
 	"strings"
 	"testing"
 	"time"
 
-	// "knative.dev/pkg/apis/duck"
-	"golang.org/x/sync/errgroup"
-
 	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
-	"github.com/mattbaird/jsonpatch"
 	"go.uber.org/zap"
-	admissionv1beta1 "k8s.io/api/admission/v1beta1"
+	"golang.org/x/sync/errgroup"
 	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
-	appsv1 "k8s.io/api/apps/v1"
-	authenticationv1 "k8s.io/api/authentication/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
 	fakekubeclientset "k8s.io/client-go/kubernetes/fake"
-	"knative.dev/pkg/apis"
 
 	. "knative.dev/pkg/logging/testing"
 	. "knative.dev/pkg/testing"
@@ -60,6 +48,31 @@ func newDefaultOptions() ControllerOptions {
 	}
 }
 
+func newHandlers() map[schema.GroupVersionKind]GenericCRD {
+	return map[schema.GroupVersionKind]GenericCRD{
+		{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		}: &Resource{},
+		{
+			Group:   "pkg.knative.dev",
+			Version: "v1beta1",
+			Kind:    "Resource",
+		}: &Resource{},
+		{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "InnerDefaultResource",
+		}: &InnerDefaultResource{},
+		{
+			Group:   "pkg.knative.io",
+			Version: "v1alpha1",
+			Kind:    "InnerDefaultResource",
+		}: &InnerDefaultResource{},
+	}
+}
+
 const (
 	testNamespace    = "test-namespace"
 	testResourceName = "test-resource"
@@ -81,482 +94,9 @@ func newNonRunningTestAdmissionController(t *testing.T, options ControllerOption
 	return
 }
 
-func TestDeleteAllowed(t *testing.T) {
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-
-	req := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Delete,
-	}
-
-	if resp := ac.admit(TestContextWithLogger(t), req); !resp.Allowed {
-		t.Fatal("Unexpected denial of delete")
-	}
-}
-
-func TestConnectAllowed(t *testing.T) {
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-
-	req := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Connect,
-	}
-
-	resp := ac.admit(TestContextWithLogger(t), req)
-	if !resp.Allowed {
-		t.Fatalf("Unexpected denial of connect")
-	}
-}
-
-func TestUnknownKindFails(t *testing.T) {
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-
-	req := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Create,
-		Kind: metav1.GroupVersionKind{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "Garbage",
-		},
-	}
-
-	expectFailsWith(t, ac.admit(TestContextWithLogger(t), req), "unhandled kind")
-}
-
-func TestUnknownVersionFails(t *testing.T) {
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-	req := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Create,
-		Kind: metav1.GroupVersionKind{
-			Group:   "pkg.knative.dev",
-			Version: "v1beta2",
-			Kind:    "Resource",
-		},
-	}
-	expectFailsWith(t, ac.admit(TestContextWithLogger(t), req), "unhandled kind")
-}
-
-func TestUnknownFieldFails(t *testing.T) {
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-	req := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Create,
-		Kind: metav1.GroupVersionKind{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "Resource",
-		},
-	}
-
-	marshaled, err := json.Marshal(map[string]interface{}{
-		"spec": map[string]interface{}{
-			"foo": "bar",
-		},
-	})
-	if err != nil {
-		panic("failed to marshal resource")
-	}
-	req.Object.Raw = marshaled
-
-	expectFailsWith(t, ac.admit(TestContextWithLogger(t), req),
-		`mutation failed: cannot decode incoming new object: json: unknown field "foo"`)
-}
-
-func TestAdmitCreates(t *testing.T) {
-	tests := []struct {
-		name      string
-		setup     func(context.Context, *Resource)
-		rejection string
-		patches   []jsonpatch.JsonPatchOperation
-	}{{
-		name: "test simple creation (alpha, no diff)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.TypeMeta.APIVersion = "v1alpha1"
-			r.SetDefaults(ctx)
-			r.Annotations = map[string]string{
-				"pkg.knative.dev/creator":      user1,
-				"pkg.knative.dev/lastModifier": user1,
-			}
-		},
-		patches: []jsonpatch.JsonPatchOperation{},
-	}, {
-		name: "test simple creation (beta, no diff)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.TypeMeta.APIVersion = "v1beta1"
-			r.SetDefaults(ctx)
-			r.Annotations = map[string]string{
-				"pkg.knative.dev/creator":      user1,
-				"pkg.knative.dev/lastModifier": user1,
-			}
-		},
-		patches: []jsonpatch.JsonPatchOperation{},
-	}, {
-		name: "test simple creation (with defaults)",
-		setup: func(ctx context.Context, r *Resource) {
-		},
-		patches: []jsonpatch.JsonPatchOperation{{
-			Operation: "add",
-			Path:      "/metadata/annotations",
-			Value: map[string]interface{}{
-				"pkg.knative.dev/creator":      user1,
-				"pkg.knative.dev/lastModifier": user1,
-			},
-		}, {
-			Operation: "add",
-			Path:      "/spec/fieldThatsImmutableWithDefault",
-			Value:     "this is another default value",
-		}, {
-			Operation: "add",
-			Path:      "/spec/fieldWithDefault",
-			Value:     "I'm a default.",
-		}},
-	}, {
-		name: "test simple creation (with defaults around annotations)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.Annotations = map[string]string{
-				"foo": "bar",
-			}
-		},
-		patches: []jsonpatch.JsonPatchOperation{{
-			Operation: "add",
-			Path:      "/metadata/annotations/pkg.knative.dev~1creator",
-			Value:     user1,
-		}, {
-			Operation: "add",
-			Path:      "/metadata/annotations/pkg.knative.dev~1lastModifier",
-			Value:     user1,
-		}, {
-			Operation: "add",
-			Path:      "/spec/fieldThatsImmutableWithDefault",
-			Value:     "this is another default value",
-		}, {
-			Operation: "add",
-			Path:      "/spec/fieldWithDefault",
-			Value:     "I'm a default.",
-		}},
-	}, {
-		name: "test simple creation (with partially overridden defaults)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.Spec.FieldThatsImmutableWithDefault = "not the default"
-		},
-		patches: []jsonpatch.JsonPatchOperation{{
-			Operation: "add",
-			Path:      "/metadata/annotations",
-			Value: map[string]interface{}{
-				"pkg.knative.dev/creator":      user1,
-				"pkg.knative.dev/lastModifier": user1,
-			},
-		}, {
-			Operation: "add",
-			Path:      "/spec/fieldWithDefault",
-			Value:     "I'm a default.",
-		}},
-	}, {
-		name: "test simple creation (webhook corrects user annotation)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-			// THIS IS NOT WHO IS CREATING IT, IT IS LIES!
-			r.Annotations = map[string]string{
-				"pkg.knative.dev/lastModifier": user2,
-			}
-		},
-		patches: []jsonpatch.JsonPatchOperation{{
-			Operation: "replace",
-			Path:      "/metadata/annotations/pkg.knative.dev~1lastModifier",
-			Value:     user1,
-		}, {
-			Operation: "add",
-			Path:      "/metadata/annotations/pkg.knative.dev~1creator",
-			Value:     user1,
-		}},
-	}, {
-		name: "with bad field",
-		setup: func(ctx context.Context, r *Resource) {
-			// Put a bad value in.
-			r.Spec.FieldWithValidation = "not what's expected"
-		},
-		rejection: "invalid value",
-	}}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			r := createResource("a name")
-			ctx := apis.WithinCreate(apis.WithUserInfo(
-				TestContextWithLogger(t),
-				&authenticationv1.UserInfo{Username: user1}))
-
-			// Setup the resource.
-			tc.setup(ctx, r)
-
-			_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-			resp := ac.admit(ctx, createCreateResource(ctx, r))
-
-			if tc.rejection == "" {
-				expectAllowed(t, resp)
-				expectPatches(t, resp.Patch, tc.patches)
-			} else {
-				expectFailsWith(t, resp, tc.rejection)
-			}
-		})
-	}
-}
-
-func createCreateResource(ctx context.Context, r *Resource) *admissionv1beta1.AdmissionRequest {
-	req := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Create,
-		Kind: metav1.GroupVersionKind{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "Resource",
-		},
-		UserInfo: *apis.GetUserInfo(ctx),
-	}
-	marshaled, err := json.Marshal(r)
-	if err != nil {
-		panic("failed to marshal resource")
-	}
-	req.Object.Raw = marshaled
-	req.Resource.Group = "pkg.knative.dev"
-	return req
-}
-
-func TestAdmitUpdates(t *testing.T) {
-	tests := []struct {
-		name      string
-		setup     func(context.Context, *Resource)
-		mutate    func(context.Context, *Resource)
-		rejection string
-		patches   []jsonpatch.JsonPatchOperation
-	}{{
-		name: "test simple update (no diff)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-		},
-		mutate: func(ctx context.Context, r *Resource) {
-			// If we don't change anything, the updater
-			// annotation doesn't change.
-		},
-		patches: []jsonpatch.JsonPatchOperation{},
-	}, {
-		name: "test simple update (update updater annotation)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-		},
-		mutate: func(ctx context.Context, r *Resource) {
-			// When we change the spec, the updater
-			// annotation changes.
-			r.Spec.FieldWithDefault = "not the default"
-		},
-		patches: []jsonpatch.JsonPatchOperation{{
-			Operation: "replace",
-			Path:      "/metadata/annotations/pkg.knative.dev~1lastModifier",
-			Value:     user2,
-		}},
-	}, {
-		name: "test simple update (annotation change doesn't change updater)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-		},
-		mutate: func(ctx context.Context, r *Resource) {
-			// When we change an annotation, the updater doesn't change.
-			r.Annotations["foo"] = "bar"
-		},
-		patches: []jsonpatch.JsonPatchOperation{},
-	}, {
-		name: "test that updates dropping immutable defaults are filled back in",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-			r.Spec.FieldThatsImmutableWithDefault = ""
-		},
-		mutate: func(ctx context.Context, r *Resource) {
-			r.Spec.FieldThatsImmutableWithDefault = ""
-		},
-		patches: []jsonpatch.JsonPatchOperation{{
-			Operation: "add",
-			Path:      "/spec/fieldThatsImmutableWithDefault",
-			Value:     "this is another default value",
-		}},
-	}, {
-		name: "bad mutation (immutable)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-		},
-		mutate: func(ctx context.Context, r *Resource) {
-			r.Spec.FieldThatsImmutableWithDefault = "something different"
-		},
-		rejection: "Immutable field changed",
-	}, {
-		name: "bad mutation (validation)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-		},
-		mutate: func(ctx context.Context, r *Resource) {
-			r.Spec.FieldWithValidation = "not what's expected"
-		},
-		rejection: "invalid value",
-	}}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			old := createResource("a name")
-			ctx := TestContextWithLogger(t)
-
-			old.Annotations = map[string]string{
-				"pkg.knative.dev/creator":      user1,
-				"pkg.knative.dev/lastModifier": user1,
-			}
-
-			tc.setup(ctx, old)
-
-			new := old.DeepCopy()
-
-			// Mutate the resource using the update context as user2
-			ctx = apis.WithUserInfo(apis.WithinUpdate(ctx, old),
-				&authenticationv1.UserInfo{Username: user2})
-			tc.mutate(ctx, new)
-
-			_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-			resp := ac.admit(ctx, createUpdateResource(ctx, old, new))
-
-			if tc.rejection == "" {
-				expectAllowed(t, resp)
-				expectPatches(t, resp.Patch, tc.patches)
-			} else {
-				expectFailsWith(t, resp, tc.rejection)
-			}
-		})
-	}
-}
-
-func createUpdateResource(ctx context.Context, old, new *Resource) *admissionv1beta1.AdmissionRequest {
-	req := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Update,
-		Kind: metav1.GroupVersionKind{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "Resource",
-		},
-		UserInfo: *apis.GetUserInfo(ctx),
-	}
-	marshaled, err := json.Marshal(new)
-	if err != nil {
-		panic("failed to marshal resource")
-	}
-	req.Object.Raw = marshaled
-	marshaledOld, err := json.Marshal(old)
-	if err != nil {
-		panic("failed to marshal resource")
-	}
-	req.OldObject.Raw = marshaledOld
-	req.Resource.Group = "pkg.knative.dev"
-	return req
-}
-
-func TestValidCreateResourceSucceedsWithRoundTripAndDefaultPatch(t *testing.T) {
-	req := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Create,
-		Kind: metav1.GroupVersionKind{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "InnerDefaultResource",
-		},
-	}
-	req.Object.Raw = createInnerDefaultResourceWithoutSpec(t)
-
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-	resp := ac.admit(TestContextWithLogger(t), req)
-	expectAllowed(t, resp)
-	expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{
-		Operation: "add",
-		Path:      "/spec",
-		Value:     map[string]interface{}{},
-	}, {
-		Operation: "add",
-		Path:      "/spec/fieldWithDefault",
-		Value:     "I'm a default.",
-	}})
-}
-
-func createInnerDefaultResourceWithoutSpec(t *testing.T) []byte {
-	t.Helper()
-	r := InnerDefaultResource{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: testNamespace,
-			Name:      "a name",
-		},
-	}
-	// Remove the 'spec' field of the generated JSON by marshaling it to JSON, parsing that as a
-	// generic map[string]interface{}, removing 'spec', and marshaling it again.
-	origBytes, err := json.Marshal(r)
-	if err != nil {
-		t.Fatalf("Error marshaling origBytes: %v", err)
-	}
-	var q map[string]interface{}
-	if err := json.Unmarshal(origBytes, &q); err != nil {
-		t.Fatalf("Error unmarshaling origBytes: %v", err)
-	}
-	delete(q, "spec")
-	b, err := json.Marshal(q)
-	if err != nil {
-		t.Fatalf("Error marshaling q: %v", err)
-	}
-	return b
-}
-
-func createInnerDefaultResourceWithSpecAndStatus(t *testing.T, spec *InnerDefaultSpec, status *InnerDefaultStatus) []byte {
-	t.Helper()
-	r := InnerDefaultResource{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: testNamespace,
-			Name:      "a name",
-		},
-	}
-	if spec != nil {
-		r.Spec = *spec
-	}
-	if status != nil {
-		r.Status = *status
-	}
-
-	b, err := json.Marshal(r)
-	if err != nil {
-		t.Fatalf("Error marshaling bytes: %v", err)
-	}
-	return b
-}
-
-func TestValidWebhook(t *testing.T) {
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-	createDeployment(ac)
-	ac.register(TestContextWithLogger(t), ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations(), []byte{})
-	_, err := ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Get(ac.Options.WebhookName, metav1.GetOptions{})
-	if err != nil {
-		t.Fatalf("Failed to create webhook: %s", err)
-	}
-}
-
-func TestUpdatingWebhook(t *testing.T) {
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
-	webhook := &admissionregistrationv1beta1.MutatingWebhookConfiguration{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: ac.Options.WebhookName,
-		},
-		Webhooks: []admissionregistrationv1beta1.Webhook{{
-			Name:         ac.Options.WebhookName,
-			Rules:        []admissionregistrationv1beta1.RuleWithOperations{{}},
-			ClientConfig: admissionregistrationv1beta1.WebhookClientConfig{},
-		}},
-	}
-
-	createDeployment(ac)
-	createWebhook(ac, webhook)
-	ac.register(TestContextWithLogger(t), ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations(), []byte{})
-	currentWebhook, _ := ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Get(ac.Options.WebhookName, metav1.GetOptions{})
-	if reflect.DeepEqual(currentWebhook.Webhooks, webhook.Webhooks) {
-		t.Fatalf("Expected webhook to be updated")
-	}
-}
-
 func TestRegistrationStopChanFire(t *testing.T) {
 	opts := newDefaultOptions()
-	_, ac := newNonRunningTestAdmissionController(t, opts)
+	kubeClient, ac := newNonRunningTestAdmissionController(t, opts)
 	webhook := &admissionregistrationv1beta1.MutatingWebhookConfiguration{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: ac.Options.WebhookName,
@@ -569,7 +109,7 @@ func TestRegistrationStopChanFire(t *testing.T) {
 			},
 		},
 	}
-	createWebhook(ac, webhook)
+	createWebhook(kubeClient, webhook)
 
 	ac.Options.RegistrationDelay = 1 * time.Minute
 	stopCh := make(chan struct{})
@@ -591,7 +131,7 @@ func TestRegistrationStopChanFire(t *testing.T) {
 }
 
 func TestRegistrationForAlreadyExistingWebhook(t *testing.T) {
-	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
+	kubeClient, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
 	webhook := &admissionregistrationv1beta1.MutatingWebhookConfiguration{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: ac.Options.WebhookName,
@@ -604,7 +144,7 @@ func TestRegistrationForAlreadyExistingWebhook(t *testing.T) {
 			},
 		},
 	}
-	createWebhook(ac, webhook)
+	createWebhook(kubeClient, webhook)
 
 	ac.Options.RegistrationDelay = 1 * time.Millisecond
 	stopCh := make(chan struct{})
@@ -641,8 +181,8 @@ func TestCertConfigurationForAlreadyGeneratedSecret(t *testing.T) {
 		t.Fatalf("Failed to create secret: %v", err)
 	}
 
-	createNamespace(t, ac.Client, metav1.NamespaceSystem)
+	createNamespace(t, kubeClient, metav1.NamespaceSystem)
-	createTestConfigMap(t, ac.Client)
+	createTestConfigMap(t, kubeClient)
 
 	tlsConfig, caCert, err := configureCerts(ctx, kubeClient, &ac.Options)
 	if err != nil {
@@ -677,8 +217,8 @@ func TestCertConfigurationForGeneratedSecret(t *testing.T) {
 	kubeClient, ac := newNonRunningTestAdmissionController(t, opts)
 
 	ctx := TestContextWithLogger(t)
-	createNamespace(t, ac.Client, metav1.NamespaceSystem)
+	createNamespace(t, kubeClient, metav1.NamespaceSystem)
-	createTestConfigMap(t, ac.Client)
+	createTestConfigMap(t, kubeClient)
 
 	tlsConfig, caCert, err := configureCerts(ctx, kubeClient, &ac.Options)
 	if err != nil {
@@ -709,115 +249,8 @@ func TestSettingWebhookClientAuth(t *testing.T) {
 	}
 }
 
-func createDeployment(ac *AdmissionController) {
-	deployment := &appsv1.Deployment{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "whatever",
-			Namespace: "knative-something",
-		},
-	}
-	ac.Client.Apps().Deployments("knative-something").Create(deployment)
-}
-
-func createWebhook(ac *AdmissionController, webhook *admissionregistrationv1beta1.MutatingWebhookConfiguration) {
-	client := ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations()
-	_, err := client.Create(webhook)
-	if err != nil {
-		panic(fmt.Sprintf("failed to create test webhook: %s", err))
-	}
-}
-
-func expectAllowed(t *testing.T, resp *admissionv1beta1.AdmissionResponse) {
-	t.Helper()
-	if !resp.Allowed {
-		t.Errorf("Expected allowed, but failed with %+v", resp.Result)
-	}
-}
-
-func expectFailsWith(t *testing.T, resp *admissionv1beta1.AdmissionResponse, contains string) {
-	t.Helper()
-	if resp.Allowed {
-		t.Error("Expected denial, got allowed")
-		return
-	}
-	if !strings.Contains(resp.Result.Message, contains) {
-		t.Errorf("Expected failure containing %q got %q", contains, resp.Result.Message)
-	}
-}
-
-func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) {
-	t.Helper()
-	var got []jsonpatch.JsonPatchOperation
-
-	err := json.Unmarshal(a, &got)
-	if err != nil {
-		t.Errorf("Failed to unmarshal patches: %s", err)
-		return
-	}
-
-	// Give the patch a deterministic ordering.
-	// Technically this can change the meaning, but the ordering is otherwise unstable
-	// and difficult to test.
-	sort.Slice(e, func(i, j int) bool {
-		lhs, rhs := e[i], e[j]
-		if lhs.Operation != rhs.Operation {
-			return lhs.Operation < rhs.Operation
-		}
-		return lhs.Path < rhs.Path
-	})
-	sort.Slice(got, func(i, j int) bool {
-		lhs, rhs := got[i], got[j]
-		if lhs.Operation != rhs.Operation {
-			return lhs.Operation < rhs.Operation
-		}
-		return lhs.Path < rhs.Path
-	})
-
-	// Even though diff is useful, seeing the whole objects
-	// one under another helps a lot.
-	t.Logf("Got Patches:  %#v", got)
-	t.Logf("Want Patches: %#v", e)
-	if diff := cmp.Diff(e, got, cmpopts.EquateEmpty()); diff != "" {
-		t.Logf("diff Patches: %v", diff)
-		t.Errorf("expectPatches (-want, +got) = %s", diff)
-	}
-}
-
-func setUserAnnotation(userC, userU string) jsonpatch.JsonPatchOperation {
-	return jsonpatch.JsonPatchOperation{
-		Operation: "add",
-		Path:      "/metadata/annotations",
-		Value: map[string]interface{}{
-			"pkg.knative.dev/creator":      userC,
-			"pkg.knative.dev/lastModifier": userU,
-		},
-	}
-}
-
 func NewTestAdmissionController(client kubernetes.Interface, options ControllerOptions,
 	logger *zap.SugaredLogger) (*AdmissionController, error) {
-	// Use different versions and domains, for coverage.
+	handlers := newHandlers()
-	handlers := map[schema.GroupVersionKind]GenericCRD{
-		{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "Resource",
-		}: &Resource{},
-		{
-			Group:   "pkg.knative.dev",
-			Version: "v1beta1",
-			Kind:    "Resource",
-		}: &Resource{},
-		{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "InnerDefaultResource",
-		}: &InnerDefaultResource{},
-		{
-			Group:   "pkg.knative.io",
-			Version: "v1alpha1",
-			Kind:    "InnerDefaultResource",
-		}: &InnerDefaultResource{},
-	}
 	return NewAdmissionController(client, options, handlers, logger, nil, true)
 }