Refactor webhook to allow adding conversion support (#989)

* Refactor webhook to allow adding conversion support

* pr feedback

* fix memory leak

* We can use mux.Handle

* move admission integration tests to separate file

webhook/admission.go

@@ -0,0 +1,94 @@
+/*
+Copyright 2020 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"
+	"net/http"
+	"time"
+
+	"go.uber.org/zap"
+	admissionv1beta1 "k8s.io/api/admission/v1beta1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"knative.dev/pkg/logging"
+	"knative.dev/pkg/logging/logkey"
+)
+
+// AdmissionController provides the interface for different admission controllers
+type AdmissionController interface {
+	// Path returns the path that this particular admission controller serves on.
+	Path() string
+
+	// Admit is the callback which is invoked when an HTTPS request comes in on Path().
+	Admit(context.Context, *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse
+}
+
+// MakeErrorStatus creates an 'BadRequest' error AdmissionResponse
+func MakeErrorStatus(reason string, args ...interface{}) *admissionv1beta1.AdmissionResponse {
+	result := apierrors.NewBadRequest(fmt.Sprintf(reason, args...)).Status()
+	return &admissionv1beta1.AdmissionResponse{
+		Result:  &result,
+		Allowed: false,
+	}
+}
+
+func admissionHandler(rootLogger *zap.SugaredLogger, stats StatsReporter, c AdmissionController) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		var ttStart = time.Now()
+		logger := rootLogger
+		logger.Infof("Webhook ServeHTTP request=%#v", r)
+
+		var review admissionv1beta1.AdmissionReview
+		if err := json.NewDecoder(r.Body).Decode(&review); err != nil {
+			http.Error(w, fmt.Sprintf("could not decode body: %v", err), http.StatusBadRequest)
+			return
+		}
+
+		logger = logger.With(
+			zap.String(logkey.Kind, review.Request.Kind.String()),
+			zap.String(logkey.Namespace, review.Request.Namespace),
+			zap.String(logkey.Name, review.Request.Name),
+			zap.String(logkey.Operation, string(review.Request.Operation)),
+			zap.String(logkey.Resource, review.Request.Resource.String()),
+			zap.String(logkey.SubResource, review.Request.SubResource),
+			zap.String(logkey.UserInfo, fmt.Sprint(review.Request.UserInfo)))
+
+		ctx := logging.WithLogger(r.Context(), logger)
+
+		var response admissionv1beta1.AdmissionReview
+		reviewResponse := c.Admit(ctx, review.Request)
+		logger.Infof("AdmissionReview for %#v: %s/%s response=%#v",
+			review.Request.Kind, review.Request.Namespace, review.Request.Name, reviewResponse)
+
+		if !reviewResponse.Allowed || reviewResponse.PatchType != nil || response.Response == nil {
+			response.Response = reviewResponse
+		}
+		response.Response.UID = review.Request.UID
+
+		if err := json.NewEncoder(w).Encode(response); err != nil {
+			http.Error(w, fmt.Sprintf("could encode response: %v", err), http.StatusInternalServerError)
+			return
+		}
+
+		if stats != nil {
+			// Only report valid requests
+			stats.ReportRequest(review.Request, response.Response, time.Since(ttStart))
+		}
+	}
+}

webhook/admission_integration_test.go

@@ -0,0 +1,267 @@
+/*
+Copyright 2020 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"
+	"io/ioutil"
+	"net/http"
+	"net/url"
+	"path"
+	"strings"
+	"testing"
+
+	"github.com/mattbaird/jsonpatch"
+	"golang.org/x/sync/errgroup"
+	admissionv1beta1 "k8s.io/api/admission/v1beta1"
+	authenticationv1 "k8s.io/api/authentication/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/metrics/metricstest"
+)
+
+type fixedAdmissionController struct {
+	path     string
+	response *admissionv1beta1.AdmissionResponse
+}
+
+var _ AdmissionController = (*fixedAdmissionController)(nil)
+
+func (fac *fixedAdmissionController) Path() string {
+	return fac.path
+}
+
+func (fac *fixedAdmissionController) Admit(ctx context.Context, req *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse {
+	return fac.response
+}
+
+func TestAdmissionEmptyRequestBody(t *testing.T) {
+	c := &fixedAdmissionController{
+		path:     "/bazinga",
+		response: &admissionv1beta1.AdmissionResponse{},
+	}
+
+	testEmptyRequestBody(t, c)
+}
+
+func TestAdmissionValidResponseForResource(t *testing.T) {
+	ac := &fixedAdmissionController{
+		path:     "/bazinga",
+		response: &admissionv1beta1.AdmissionResponse{},
+	}
+	wh, serverURL, ctx, cancel, err := testSetup(t, ac)
+	if err != nil {
+		t.Fatalf("testSetup() = %v", err)
+	}
+
+	eg, _ := errgroup.WithContext(ctx)
+	eg.Go(func() error { return wh.Run(ctx.Done()) })
+	defer func() {
+		cancel()
+		if err := eg.Wait(); err != nil {
+			t.Errorf("Unable to run controller: %s", err)
+		}
+	}()
+
+	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
+	if pollErr != nil {
+		t.Fatalf("waitForServerAvailable() = %v", err)
+	}
+	tlsClient, err := createSecureTLSClient(t, wh.Client, &wh.Options)
+	if err != nil {
+		t.Fatalf("createSecureTLSClient() = %v", err)
+	}
+
+	admissionreq := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+	}
+	testRev := createResource("testrev")
+	marshaled, err := json.Marshal(testRev)
+	if err != nil {
+		t.Fatalf("Failed to marshal resource: %s", err)
+	}
+
+	admissionreq.Resource.Group = "pkg.knative.dev"
+	admissionreq.Object.Raw = marshaled
+	rev := &admissionv1beta1.AdmissionReview{
+		Request: admissionreq,
+	}
+
+	reqBuf := new(bytes.Buffer)
+	err = json.NewEncoder(reqBuf).Encode(&rev)
+	if err != nil {
+		t.Fatalf("Failed to marshal admission review: %v", err)
+	}
+
+	u, err := url.Parse(fmt.Sprintf("https://%s", serverURL))
+	if err != nil {
+		t.Fatalf("bad url %v", err)
+	}
+
+	u.Path = path.Join(u.Path, ac.Path())
+
+	req, err := http.NewRequest("GET", u.String(), reqBuf)
+	if err != nil {
+		t.Fatalf("http.NewRequest() = %v", err)
+	}
+	req.Header.Add("Content-Type", "application/json")
+
+	response, err := tlsClient.Do(req)
+	if err != nil {
+		t.Fatalf("Failed to get response %v", err)
+	}
+
+	if got, want := response.StatusCode, http.StatusOK; got != want {
+		t.Errorf("Response status code = %v, wanted %v", got, want)
+	}
+
+	defer response.Body.Close()
+	responseBody, err := ioutil.ReadAll(response.Body)
+	if err != nil {
+		t.Fatalf("Failed to read response body %v", err)
+	}
+
+	reviewResponse := admissionv1beta1.AdmissionReview{}
+
+	err = json.NewDecoder(bytes.NewReader(responseBody)).Decode(&reviewResponse)
+	if err != nil {
+		t.Fatalf("Failed to decode response: %v", err)
+	}
+
+	metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName)
+}
+
+func TestAdmissionInvalidResponseForResource(t *testing.T) {
+	expectedError := "everything is fine."
+	ac := &fixedAdmissionController{
+		path:     "/booger",
+		response: MakeErrorStatus(expectedError),
+	}
+	wh, serverURL, ctx, cancel, err := testSetup(t, ac)
+	if err != nil {
+		t.Fatalf("testSetup() = %v", err)
+	}
+
+	eg, _ := errgroup.WithContext(ctx)
+	eg.Go(func() error { return wh.Run(ctx.Done()) })
+	defer func() {
+		cancel()
+		if err := eg.Wait(); err != nil {
+			t.Errorf("Unable to run controller: %s", err)
+		}
+	}()
+
+	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
+	if pollErr != nil {
+		t.Fatalf("waitForServerAvailable() = %v", err)
+	}
+	tlsClient, err := createSecureTLSClient(t, wh.Client, &wh.Options)
+	if err != nil {
+		t.Fatalf("createSecureTLSClient() = %v", err)
+	}
+
+	resource := createResource(testResourceName)
+
+	resource.Spec.FieldWithValidation = "not the right value"
+	marshaled, err := json.Marshal(resource)
+	if err != nil {
+		t.Fatalf("Failed to marshal resource: %s", err)
+	}
+
+	admissionreq := &admissionv1beta1.AdmissionRequest{
+		Operation: admissionv1beta1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+		UserInfo: authenticationv1.UserInfo{
+			Username: user1,
+		},
+	}
+
+	admissionreq.Resource.Group = "pkg.knative.dev"
+	admissionreq.Object.Raw = marshaled
+
+	rev := &admissionv1beta1.AdmissionReview{
+		Request: admissionreq,
+	}
+	reqBuf := new(bytes.Buffer)
+	err = json.NewEncoder(reqBuf).Encode(&rev)
+	if err != nil {
+		t.Fatalf("Failed to marshal admission review: %v", err)
+	}
+
+	u, err := url.Parse(fmt.Sprintf("https://%s", serverURL))
+	if err != nil {
+		t.Fatalf("bad url %v", err)
+	}
+
+	u.Path = path.Join(u.Path, ac.Path())
+
+	req, err := http.NewRequest("GET", u.String(), reqBuf)
+	if err != nil {
+		t.Fatalf("http.NewRequest() = %v", err)
+	}
+
+	req.Header.Add("Content-Type", "application/json")
+
+	response, err := tlsClient.Do(req)
+	if err != nil {
+		t.Fatalf("Failed to receive response %v", err)
+	}
+
+	if got, want := response.StatusCode, http.StatusOK; got != want {
+		t.Errorf("Response status code = %v, wanted %v", got, want)
+	}
+
+	defer response.Body.Close()
+	respBody, err := ioutil.ReadAll(response.Body)
+	if err != nil {
+		t.Fatalf("Failed to read response body %v", err)
+	}
+
+	reviewResponse := admissionv1beta1.AdmissionReview{}
+
+	err = json.NewDecoder(bytes.NewReader(respBody)).Decode(&reviewResponse)
+	if err != nil {
+		t.Fatalf("Failed to decode response: %v", err)
+	}
+
+	var respPatch []jsonpatch.JsonPatchOperation
+	err = json.Unmarshal(reviewResponse.Response.Patch, &respPatch)
+	if err == nil {
+		t.Fatalf("Expected to fail JSON unmarshal of resposnse")
+	}
+
+	if got, want := reviewResponse.Response.Result.Status, "Failure"; got != want {
+		t.Errorf("Response status = %v, wanted %v", got, want)
+	}
+
+	if !strings.Contains(reviewResponse.Response.Result.Message, expectedError) {
+		t.Errorf("Received unexpected response status message %s", reviewResponse.Response.Result.Message)
+	}
+
+	// Stats should be reported for requests that have admission disallowed
+	metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName)
+}

webhook/webhook.go

@@ -19,11 +19,9 @@ package webhook
 import (
 	"context"
 	"crypto/tls"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"net/http"
-	"time"
 
 	// Injection stuff
 	kubeclient "knative.dev/pkg/client/injection/kube/client"
@@ -31,12 +29,9 @@ import (
 
 	"go.uber.org/zap"
 	"golang.org/x/sync/errgroup"
-	admissionv1beta1 "k8s.io/api/admission/v1beta1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/kubernetes"
 	corelisters "k8s.io/client-go/listers/core/v1"
 	"knative.dev/pkg/logging"
-	"knative.dev/pkg/logging/logkey"
 	"knative.dev/pkg/system"
 	certresources "knative.dev/pkg/webhook/certificates/resources"
 )
@@ -63,30 +58,29 @@ type Options struct {
 	StatsReporter StatsReporter
 }
 
-// AdmissionController provides the interface for different admission controllers
-type AdmissionController interface {
-	// Path returns the path that this particular admission controller serves on.
-	Path() string
-
-	// Admit is the callback which is invoked when an HTTPS request comes in on Path().
-	Admit(context.Context, *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse
-}
-
 // Webhook implements the external webhook for validation of
 // resources and configuration.
 type Webhook struct {
 	Client  kubernetes.Interface
 	Options Options
 	Logger  *zap.SugaredLogger
-	admissionControllers map[string]AdmissionController
+
+	mux          http.ServeMux
 	secretlister corelisters.SecretLister
 }
 
 // New constructs a Webhook
 func New(
 	ctx context.Context,
-	admissionControllers []AdmissionController,
+	controllers []AdmissionController,
-) (*Webhook, error) {
+) (webhook *Webhook, err error) {
+
+	// ServeMux.Handle panics on duplicate paths
+	defer func() {
+		if r := recover(); r != nil {
+			err = fmt.Errorf("error creating webhook %v", r)
+		}
+	}()
 
 	client := kubeclient.Get(ctx)
 
@@ -111,35 +105,37 @@ func New(
 		opts.StatsReporter = reporter
 	}
 
-	// Build up a map of paths to admission controllers for routing handlers.
+	webhook = &Webhook{
-	acs := make(map[string]AdmissionController, len(admissionControllers))
-	for _, ac := range admissionControllers {
-		if _, ok := acs[ac.Path()]; ok {
-			return nil, fmt.Errorf("duplicate webhook for path: %v", ac.Path())
-		}
-		acs[ac.Path()] = ac
-	}
-
-	return &Webhook{
 		Client:       client,
 		Options:      *opts,
 		secretlister: secretInformer.Lister(),
-		admissionControllers: acs,
 		Logger:       logger,
-	}, nil
+	}
+
+	webhook.mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, fmt.Sprintf("no admission controller registered for: %s", r.URL.Path), http.StatusBadRequest)
+	})
+
+	for _, c := range controllers {
+		webhook.mux.Handle(
+			c.Path(),
+			admissionHandler(logger, opts.StatsReporter, c),
+		)
+	}
+	return
 }
 
 // Run implements the admission controller run loop.
-func (ac *Webhook) Run(stop <-chan struct{}) error {
+func (wh *Webhook) Run(stop <-chan struct{}) error {
-	logger := ac.Logger
+	logger := wh.Logger
 	ctx := logging.WithLogger(context.Background(), logger)
 
 	server := &http.Server{
-		Handler: ac,
+		Handler: wh,
-		Addr:    fmt.Sprintf(":%v", ac.Options.Port),
+		Addr:    fmt.Sprintf(":%d", wh.Options.Port),
 		TLSConfig: &tls.Config{
 			GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
-				secret, err := ac.secretlister.Secrets(system.Namespace()).Get(ac.Options.SecretName)
+				secret, err := wh.secretlister.Secrets(system.Namespace()).Get(wh.Options.SecretName)
 				if err != nil {
 					return nil, err
 				}
@@ -183,13 +179,7 @@ func (ac *Webhook) Run(stop <-chan struct{}) error {
 	}
 }
 
-// ServeHTTP implements the external admission webhook for mutating
+func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-// serving resources.
-func (ac *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	var ttStart = time.Now()
-	logger := ac.Logger
-	logger.Infof("Webhook ServeHTTP request=%#v", r)
-
 	// Verify the content type is accurate.
 	contentType := r.Header.Get("Content-Type")
 	if contentType != "application/json" {
@@ -197,55 +187,5 @@ func (ac *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	var review admissionv1beta1.AdmissionReview
+	wh.mux.ServeHTTP(w, r)
-	if err := json.NewDecoder(r.Body).Decode(&review); err != nil {
-		http.Error(w, fmt.Sprintf("could not decode body: %v", err), http.StatusBadRequest)
-		return
-	}
-
-	logger = logger.With(
-		zap.String(logkey.Kind, fmt.Sprint(review.Request.Kind)),
-		zap.String(logkey.Namespace, review.Request.Namespace),
-		zap.String(logkey.Name, review.Request.Name),
-		zap.String(logkey.Operation, fmt.Sprint(review.Request.Operation)),
-		zap.String(logkey.Resource, fmt.Sprint(review.Request.Resource)),
-		zap.String(logkey.SubResource, fmt.Sprint(review.Request.SubResource)),
-		zap.String(logkey.UserInfo, fmt.Sprint(review.Request.UserInfo)))
-	ctx := logging.WithLogger(r.Context(), logger)
-
-	c, ok := ac.admissionControllers[r.URL.Path]
-	if !ok {
-		http.Error(w, fmt.Sprintf("no admission controller registered for: %s", r.URL.Path), http.StatusBadRequest)
-		return
-	}
-
-	var response admissionv1beta1.AdmissionReview
-	reviewResponse := c.Admit(ctx, review.Request)
-	logger.Infof("AdmissionReview for %#v: %s/%s response=%#v",
-		review.Request.Kind, review.Request.Namespace, review.Request.Name, reviewResponse)
-
-	if !reviewResponse.Allowed {
-		response.Response = reviewResponse
-	} else if reviewResponse.PatchType != nil || response.Response == nil {
-		response.Response = reviewResponse
-	}
-	response.Response.UID = review.Request.UID
-
-	if err := json.NewEncoder(w).Encode(response); err != nil {
-		http.Error(w, fmt.Sprintf("could encode response: %v", err), http.StatusInternalServerError)
-		return
-	}
-
-	if ac.Options.StatsReporter != nil {
-		// Only report valid requests
-		ac.Options.StatsReporter.ReportRequest(review.Request, response.Response, time.Since(ttStart))
-	}
-}
-
-func MakeErrorStatus(reason string, args ...interface{}) *admissionv1beta1.AdmissionResponse {
-	result := apierrors.NewBadRequest(fmt.Sprintf(reason, args...)).Status()
-	return &admissionv1beta1.AdmissionResponse{
-		Result:  &result,
-		Allowed: false,
-	}
 }

webhook/webhook_integration_test.go

@@ -17,14 +17,10 @@ limitations under the License.
 package webhook
 
 import (
-	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/url"
-	"path"
 	"strings"
 	"testing"
 	"time"
@@ -33,10 +29,7 @@ import (
 	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/secret/fake"
 	"knative.dev/pkg/system"
 
-	"github.com/mattbaird/jsonpatch"
 	"golang.org/x/sync/errgroup"
-	admissionv1beta1 "k8s.io/api/admission/v1beta1"
-	authenticationv1 "k8s.io/api/authentication/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/metrics/metricstest"
 	pkgtest "knative.dev/pkg/testing"
@@ -57,21 +50,6 @@ func createResource(name string) *pkgtest.Resource {
 
 const testTimeout = time.Duration(10 * time.Second)
 
-type fixedAdmissionController struct {
-	path     string
-	response *admissionv1beta1.AdmissionResponse
-}
-
-var _ AdmissionController = (*fixedAdmissionController)(nil)
-
-func (fac *fixedAdmissionController) Path() string {
-	return fac.path
-}
-
-func (fac *fixedAdmissionController) Admit(ctx context.Context, req *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse {
-	return fac.response
-}
-
 func TestMissingContentType(t *testing.T) {
 	wh, serverURL, ctx, cancel, err := testSetup(t)
 	if err != nil {
@@ -125,8 +103,8 @@ func TestMissingContentType(t *testing.T) {
 	metricstest.CheckStatsNotReported(t, requestCountName, requestLatenciesName)
 }
 
-func TestEmptyRequestBody(t *testing.T) {
+func testEmptyRequestBody(t *testing.T, controller AdmissionController) {
-	wh, serverURL, ctx, cancel, err := testSetup(t)
+	wh, serverURL, ctx, cancel, err := testSetup(t, controller)
 	if err != nil {
 		t.Fatalf("testSetup() = %v", err)
 	}
@@ -150,7 +128,7 @@ func TestEmptyRequestBody(t *testing.T) {
 		t.Fatalf("createSecureTLSClient() = %v", err)
 	}
 
-	req, err := http.NewRequest("GET", fmt.Sprintf("https://%s", serverURL), nil)
+	req, err := http.NewRequest("GET", fmt.Sprintf("https://%s/bazinga", serverURL), nil)
 	if err != nil {
 		t.Fatalf("http.NewRequest() = %v", err)
 	}
@@ -177,213 +155,6 @@ func TestEmptyRequestBody(t *testing.T) {
 	}
 }
 
-func TestValidResponseForResource(t *testing.T) {
-	ac := &fixedAdmissionController{
-		path:     "/bazinga",
-		response: &admissionv1beta1.AdmissionResponse{},
-	}
-	wh, serverURL, ctx, cancel, err := testSetup(t, ac)
-	if err != nil {
-		t.Fatalf("testSetup() = %v", err)
-	}
-
-	eg, _ := errgroup.WithContext(ctx)
-	eg.Go(func() error { return wh.Run(ctx.Done()) })
-	defer func() {
-		cancel()
-		if err := eg.Wait(); err != nil {
-			t.Errorf("Unable to run controller: %s", err)
-		}
-	}()
-
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
-		t.Fatalf("waitForServerAvailable() = %v", err)
-	}
-	tlsClient, err := createSecureTLSClient(t, wh.Client, &wh.Options)
-	if err != nil {
-		t.Fatalf("createSecureTLSClient() = %v", err)
-	}
-
-	admissionreq := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Create,
-		Kind: metav1.GroupVersionKind{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "Resource",
-		},
-	}
-	testRev := createResource("testrev")
-	marshaled, err := json.Marshal(testRev)
-	if err != nil {
-		t.Fatalf("Failed to marshal resource: %s", err)
-	}
-
-	admissionreq.Resource.Group = "pkg.knative.dev"
-	admissionreq.Object.Raw = marshaled
-	rev := &admissionv1beta1.AdmissionReview{
-		Request: admissionreq,
-	}
-
-	reqBuf := new(bytes.Buffer)
-	err = json.NewEncoder(reqBuf).Encode(&rev)
-	if err != nil {
-		t.Fatalf("Failed to marshal admission review: %v", err)
-	}
-
-	u, err := url.Parse(fmt.Sprintf("https://%s", serverURL))
-	if err != nil {
-		t.Fatalf("bad url %v", err)
-	}
-
-	u.Path = path.Join(u.Path, ac.Path())
-
-	req, err := http.NewRequest("GET", u.String(), reqBuf)
-	if err != nil {
-		t.Fatalf("http.NewRequest() = %v", err)
-	}
-	req.Header.Add("Content-Type", "application/json")
-
-	response, err := tlsClient.Do(req)
-	if err != nil {
-		t.Fatalf("Failed to get response %v", err)
-	}
-
-	if got, want := response.StatusCode, http.StatusOK; got != want {
-		t.Errorf("Response status code = %v, wanted %v", got, want)
-	}
-
-	defer response.Body.Close()
-	responseBody, err := ioutil.ReadAll(response.Body)
-	if err != nil {
-		t.Fatalf("Failed to read response body %v", err)
-	}
-
-	reviewResponse := admissionv1beta1.AdmissionReview{}
-
-	err = json.NewDecoder(bytes.NewReader(responseBody)).Decode(&reviewResponse)
-	if err != nil {
-		t.Fatalf("Failed to decode response: %v", err)
-	}
-
-	metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName)
-}
-
-func TestInvalidResponseForResource(t *testing.T) {
-	expectedError := "everything is fine."
-	ac := &fixedAdmissionController{
-		path:     "/booger",
-		response: MakeErrorStatus(expectedError),
-	}
-	wh, serverURL, ctx, cancel, err := testSetup(t, ac)
-	if err != nil {
-		t.Fatalf("testSetup() = %v", err)
-	}
-
-	eg, _ := errgroup.WithContext(ctx)
-	eg.Go(func() error { return wh.Run(ctx.Done()) })
-	defer func() {
-		cancel()
-		if err := eg.Wait(); err != nil {
-			t.Errorf("Unable to run controller: %s", err)
-		}
-	}()
-
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
-		t.Fatalf("waitForServerAvailable() = %v", err)
-	}
-	tlsClient, err := createSecureTLSClient(t, wh.Client, &wh.Options)
-	if err != nil {
-		t.Fatalf("createSecureTLSClient() = %v", err)
-	}
-
-	resource := createResource(testResourceName)
-
-	resource.Spec.FieldWithValidation = "not the right value"
-	marshaled, err := json.Marshal(resource)
-	if err != nil {
-		t.Fatalf("Failed to marshal resource: %s", err)
-	}
-
-	admissionreq := &admissionv1beta1.AdmissionRequest{
-		Operation: admissionv1beta1.Create,
-		Kind: metav1.GroupVersionKind{
-			Group:   "pkg.knative.dev",
-			Version: "v1alpha1",
-			Kind:    "Resource",
-		},
-		UserInfo: authenticationv1.UserInfo{
-			Username: user1,
-		},
-	}
-
-	admissionreq.Resource.Group = "pkg.knative.dev"
-	admissionreq.Object.Raw = marshaled
-
-	rev := &admissionv1beta1.AdmissionReview{
-		Request: admissionreq,
-	}
-	reqBuf := new(bytes.Buffer)
-	err = json.NewEncoder(reqBuf).Encode(&rev)
-	if err != nil {
-		t.Fatalf("Failed to marshal admission review: %v", err)
-	}
-
-	u, err := url.Parse(fmt.Sprintf("https://%s", serverURL))
-	if err != nil {
-		t.Fatalf("bad url %v", err)
-	}
-
-	u.Path = path.Join(u.Path, ac.Path())
-
-	req, err := http.NewRequest("GET", u.String(), reqBuf)
-	if err != nil {
-		t.Fatalf("http.NewRequest() = %v", err)
-	}
-
-	req.Header.Add("Content-Type", "application/json")
-
-	response, err := tlsClient.Do(req)
-	if err != nil {
-		t.Fatalf("Failed to receive response %v", err)
-	}
-
-	if got, want := response.StatusCode, http.StatusOK; got != want {
-		t.Errorf("Response status code = %v, wanted %v", got, want)
-	}
-
-	defer response.Body.Close()
-	respBody, err := ioutil.ReadAll(response.Body)
-	if err != nil {
-		t.Fatalf("Failed to read response body %v", err)
-	}
-
-	reviewResponse := admissionv1beta1.AdmissionReview{}
-
-	err = json.NewDecoder(bytes.NewReader(respBody)).Decode(&reviewResponse)
-	if err != nil {
-		t.Fatalf("Failed to decode response: %v", err)
-	}
-
-	var respPatch []jsonpatch.JsonPatchOperation
-	err = json.Unmarshal(reviewResponse.Response.Patch, &respPatch)
-	if err == nil {
-		t.Fatalf("Expected to fail JSON unmarshal of resposnse")
-	}
-
-	if got, want := reviewResponse.Response.Result.Status, "Failure"; got != want {
-		t.Errorf("Response status = %v, wanted %v", got, want)
-	}
-
-	if !strings.Contains(reviewResponse.Response.Result.Message, expectedError) {
-		t.Errorf("Received unexpected response status message %s", reviewResponse.Response.Result.Message)
-	}
-
-	// Stats should be reported for requests that have admission disallowed
-	metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName)
-}
-
 func TestSetupWebhookHTTPServerError(t *testing.T) {
 	defaultOpts := newDefaultOptions()
 	defaultOpts.Port = -1 // invalid port