Remove path from admission, more response helpers

This removes the path functionality from admission webhooks, since it's
not strictly needed.  Relevant metrics are moved up to the webhook
server, and path is added to register.  This also introduces a couple of
helpers that should make code a bit clearer when writing admission
webhooks (Allowed, Denied, Patched), and an alias file to avoid
importing both webhook and webhook/admission.

example/main.go

@@ -32,7 +32,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/runtime/signals"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 var log = logf.Log.WithName("example-controller")
@@ -78,17 +77,6 @@ func main() {
 	}
 
 	// Setup webhooks
-	entryLog.Info("setting up webhooks")
-	mutatingWebhook := &admission.Webhook{
-		Path: "/mutate-pods",
-		Handler: &podAnnotator{},
-	}
-
-	validatingWebhook := &admission.Webhook{
-		Path: "/validate-pods",
-		Handler: &podValidator{},
-	}
-
 	entryLog.Info("setting up webhook server")
 	hookServer := &webhook.Server{
 		Port:    9876,
@@ -97,11 +85,8 @@ func main() {
 	mgr.Add(hookServer)
 
 	entryLog.Info("registering webhooks to the webhook server")
-	err = hookServer.Register(mutatingWebhook, validatingWebhook)
-	if err != nil {
-		entryLog.Error(err, "unable to register webhooks in the admission server")
-		os.Exit(1)
-	}
+	hookServer.Register("/mutate-pods", webhook.Admission{&podAnnotator{}})
+	hookServer.Register("/validate-pods", webhook.Admission{&podValidator{}})
 
 	entryLog.Info("starting manager")
 	if err := mgr.Start(signals.SetupSignalHandler()); err != nil {

example/mutatingwebhook.go

@@ -32,9 +32,6 @@ type podAnnotator struct {
 	decoder admission.Decoder
 }
 
-// Implement admission.Handler so the controller can handle admission request.
-var _ admission.Handler = &podAnnotator{}
-
 // podAnnotator adds an annotation to every incoming pods.
 func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admission.Response {
 	pod := &corev1.Pod{}
@@ -44,10 +41,10 @@ func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admiss
 		return admission.ErrorResponse(http.StatusBadRequest, err)
 	}
 
-	err = a.mutatePodsFn(ctx, pod)
-	if err != nil {
-		return admission.ErrorResponse(http.StatusInternalServerError, err)
+	if pod.Annotations == nil {
+		pod.Annotations = map[string]string{}
 	}
+	pod.Annotations["example-mutating-admission-webhook"] = "foo"
 
 	marshaledPod, err := json.Marshal(pod)
 	if err != nil {
@@ -57,15 +54,6 @@ func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admiss
 	return admission.PatchResponseFromRaw(req.AdmissionRequest.Object.Raw, marshaledPod)
 }
 
-// mutatePodsFn add an annotation to the given pod
-func (a *podAnnotator) mutatePodsFn(ctx context.Context, pod *corev1.Pod) error {
-	if pod.Annotations == nil {
-		pod.Annotations = map[string]string{}
-	}
-	pod.Annotations["example-mutating-admission-webhook"] = "foo"
-	return nil
-}
-
 // podAnnotator implements inject.Client.
 // A client will be automatically injected.
 

example/validatingwebhook.go

@@ -32,9 +32,6 @@ type podValidator struct {
 	decoder admission.Decoder
 }
 
-// Implement admission.Handler so the controller can handle admission request.
-var _ admission.Handler = &podValidator{}
-
 // podValidator admits a pod iff a specific annotation exists.
 func (v *podValidator) Handle(ctx context.Context, req admission.Request) admission.Response {
 	pod := &corev1.Pod{}
@@ -44,26 +41,16 @@ func (v *podValidator) Handle(ctx context.Context, req admission.Request) admiss
 		return admission.ErrorResponse(http.StatusBadRequest, err)
 	}
 
-	allowed, reason, err := v.validatePodsFn(ctx, pod)
-	if err != nil {
-		return admission.ErrorResponse(http.StatusInternalServerError, err)
-	}
-	return admission.ValidationResponse(allowed, reason)
-}
-
-func (v *podValidator) validatePodsFn(ctx context.Context, pod *corev1.Pod) (bool, string, error) {
 	key := "example-mutating-admission-webhook"
 	anno, found := pod.Annotations[key]
-	switch {
-	case !found:
-		return found, fmt.Sprintf("failed to find annotation with key: %q", key), nil
-	case found && anno == "foo":
-		return found, "", nil
-	case found && anno != "foo":
-		return false,
-			fmt.Sprintf("the value associate with key %q is expected to be %q, but got %q", key, "foo", anno), nil
+	if !found {
+		return admission.Denied(fmt.Sprintf("missing annotation %s", key))
+	}
+	if anno != "foo" {
+		return admission.Denied(fmt.Sprintf("annotation %s did not have value %q", key, "foo"))
 	}
-	return false, "", nil
+
+	return admission.Allowed("")
 }
 
 // podValidator implements inject.Client.

pkg/webhook/admission/http.go

@@ -24,33 +24,24 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
-	"time"
 
 	"k8s.io/api/admission/v1beta1"
 	admissionv1beta1 "k8s.io/api/admission/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics"
 )
 
-var admissionv1beta1scheme = runtime.NewScheme()
-var admissionv1beta1schemecodecs = serializer.NewCodecFactory(admissionv1beta1scheme)
+var admissionScheme = runtime.NewScheme()
+var admissionCodecs = serializer.NewCodecFactory(admissionScheme)
 
 func init() {
-	addToScheme(admissionv1beta1scheme)
-}
-
-func addToScheme(scheme *runtime.Scheme) {
-	utilruntime.Must(admissionv1beta1.AddToScheme(scheme))
+	utilruntime.Must(admissionv1beta1.AddToScheme(admissionScheme))
 }
 
 var _ http.Handler = &Webhook{}
 
-func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	startTS := time.Now()
-	defer metrics.RequestLatency.WithLabelValues(wh.Path).Observe(time.Now().Sub(startTS).Seconds())
-
+func (wh Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	var body []byte
 	var err error
 
@@ -85,7 +76,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		// avoid an extra copy
 		Request: &req.AdmissionRequest,
 	}
-	if _, _, err := admissionv1beta1schemecodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil {
+	if _, _, err := admissionCodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil {
 		log.Error(err, "unable to decode the request")
 		reviewResponse = ErrorResponse(http.StatusBadRequest, err)
 		wh.writeResponse(w, reviewResponse)
@@ -98,14 +89,6 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 }
 
 func (wh *Webhook) writeResponse(w io.Writer, response Response) {
-	if response.Result.Code != 0 {
-		if response.Result.Code == http.StatusOK {
-			metrics.TotalRequests.WithLabelValues(wh.Path, "true").Inc()
-		} else {
-			metrics.TotalRequests.WithLabelValues(wh.Path, "false").Inc()
-		}
-	}
-
 	encoder := json.NewEncoder(w)
 	responseAdmissionReview := v1beta1.AdmissionReview{
 		Response: &response.AdmissionResponse,

pkg/webhook/admission/response.go

@@ -25,6 +25,23 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+// Allowed constructs a response indicating that the given operation
+// is allowed (without any patches).
+func Allowed(reason string) Response {
+	return ValidationResponse(true, reason)
+}
+
+func Denied(reason string) Response {
+	return ValidationResponse(false, reason)
+}
+
+func Patched(reason string, patches ...jsonpatch.JsonPatchOperation) Response {
+	resp := Allowed(reason)
+	resp.Patches = patches
+
+	return resp
+}
+
 // ErrorResponse creates a new Response for error-handling a request.
 func ErrorResponse(code int32, err error) Response {
 	return Response{

pkg/webhook/admission/webhook.go

@@ -18,13 +18,37 @@ package admission
 
 import (
 	"context"
-	"errors"
 	"net/http"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
+	"github.com/appscode/jsonpatch"
+	admissionv1beta1 "k8s.io/api/admission/v1beta1"
 )
 
+// Request defines the input for an admission handler.
+// It contains information to identify the object in
+// question (group, version, kind, resource, subresource,
+// name, namespace), as well as the operation in question
+// (e.g. Get, Create, etc), and the object itself.
+type Request struct {
+	admissionv1beta1.AdmissionRequest
+}
+
+// Response is the output of an admission handler.
+// It contains a response indicating if a given
+// operation is allowed, as well as a set of patches
+// to mutate the object in the case of a mutating admission handler.
+type Response struct {
+	// Patches are the JSON patches for mutating webhooks.
+	// Using this instead of setting Response.Patch to minimize
+	// overhead of serialization and deserialization.
+	Patches []jsonpatch.JsonPatchOperation
+	// AdmissionResponse is the raw admission response.
+	// The Patch field in it will be overwritten by the listed patches.
+	admissionv1beta1.AdmissionResponse
+}
+
 // Handler can handle an AdmissionRequest.
 type Handler interface {
 	Handle(context.Context, Request) Response
@@ -42,16 +66,11 @@ func (f HandlerFunc) Handle(ctx context.Context, req Request) Response {
 
 // Webhook represents each individual webhook.
 type Webhook struct {
-	// Path is the path this webhook will serve.
-	Path string
 	// Handler actually processes an admission request returning whether it was allowed or denied,
 	// and potentially patches to apply to the handler.
 	Handler Handler
 }
 
-// Webhook implements Handler interface.
-var _ Handler = &Webhook{}
-
 // Handle processes AdmissionRequest.
 // If the webhook is mutating type, it delegates the AdmissionRequest to each handler and merge the patches.
 // If the webhook is validating type, it delegates the AdmissionRequest to each handler and
@@ -73,22 +92,6 @@ func (w *Webhook) Handle(ctx context.Context, req Request) Response {
 	return resp
 }
 
-// GetPath returns the path that the webhook registered.
-func (w *Webhook) GetPath() string {
-	return w.Path
-}
-
-// Validate validates if the webhook is valid.
-func (w *Webhook) Validate() error {
-	if len(w.Path) == 0 {
-		return errors.New("field Path should not be empty")
-	}
-	if w.Handler == nil {
-		return errors.New("field Handler should not be empty")
-	}
-	return nil
-}
-
 // InjectFunc injects the field setter into the webhook.
 func (w *Webhook) InjectFunc(f inject.Func) error {
 	// inject directly into the handlers.  It would be more correct

pkg/webhook/admission/webhook_test.go

@@ -99,28 +99,4 @@ var _ = Describe("Admission Webhooks", func() {
 		Expect(resp.Result).NotTo(BeNil())
 		Expect(resp.Result.Message).To(Equal("Ground Control to Major Tom"))
 	})
-
-	Context("validation", func() {
-		It("should accept a properly populated webhook", func() {
-			wh := &Webhook{
-				Path: "/somepath",
-				Handler: &fakeHandler{},
-			}
-			Expect(wh.Validate()).To(Succeed())
-		})
-
-		It("should fail when missing a path", func() {
-			wh := &Webhook{
-				Handler: &fakeHandler{},
-			}
-			Expect(wh.Validate()).NotTo(Succeed())
-		})
-
-		It("should fail when missing a handler", func() {
-			wh := &Webhook{
-				Path: "/somepath",
-			}
-			Expect(wh.Validate()).NotTo(Succeed())
-		})
-	})
 })