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.

Author: DirectXMan12

example/main.go

@@ -32,7 +32,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager/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,
@@ -100,11 +88,8 @@ func main() {
 	}
 
 	entryLog.Info("registering webhooks to the webhook server")
-	err = hookServer.Register(mutatingWebhook, validatingWebhook)
-	if err != nil {
-		entryLog.Error(err, "unable to setup 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/inject.go

@@ -16,7 +16,7 @@ limitations under the License.
 
 package admission
 
-// Decoder is used by the ControllerManager to inject decoder into webhook handlers.
+// DecoderInjector is used by the ControllerManager to inject decoder into webhook handlers.
 type DecoderInjector interface {
 	InjectDecoder(Decoder) error
 }

pkg/webhook/admission/response.go

@@ -25,6 +25,28 @@ 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)
+}
+
+// Denied constructs a response indicating that the given operation
+// is not allowed.
+func Denied(reason string) Response {
+	return ValidationResponse(false, reason)
+}
+
+// Patched constructs a response indicating that the given operation is
+// allowed, and that the target object should be modified by the given
+// JSONPatch operations.
+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/response_test.go

@@ -29,8 +29,95 @@ import (
 )
 
 var _ = Describe("Admission Webhook Response Helpers", func() {
+	Describe("Allowed", func() {
+		It("should return an 'allowed' response", func() {
+			Expect(Allowed("")).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: true,
+					},
+				},
+			))
+		})
+
+		It("should populate a status with a reason when a reason is given", func() {
+			Expect(Allowed("acceptable")).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: true,
+						Result: &metav1.Status{
+							Reason: "acceptable",
+						},
+					},
+				},
+			))
+		})
+	})
+
+	Describe("Denied", func() {
+		It("should return a 'not allowed' response", func() {
+			Expect(Denied("")).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: false,
+					},
+				},
+			))
+		})
+
+		It("should populate a status with a reason when a reason is given", func() {
+			Expect(Denied("UNACCEPTABLE!")).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: false,
+						Result: &metav1.Status{
+							Reason: "UNACCEPTABLE!",
+						},
+					},
+				},
+			))
+		})
+	})
+
+	Describe("Patched", func() {
+		ops := []jsonpatch.JsonPatchOperation{
+			{
+				Operation: "replace",
+				Path:      "/spec/selector/matchLabels",
+				Value:     map[string]string{"foo": "bar"},
+			},
+			{
+				Operation: "delete",
+				Path:      "/spec/replicas",
+			},
+		}
+		It("should return an 'allowed' response with the given patches", func() {
+			Expect(Patched("", ops...)).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: true,
+					},
+					Patches: ops,
+				},
+			))
+		})
+		It("should populate a status with a reason when a reason is given", func() {
+			Expect(Patched("some changes", ops...)).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: true,
+						Result: &metav1.Status{
+							Reason: "some changes",
+						},
+					},
+					Patches: ops,
+				},
+			))
+		})
+	})
+
 	Describe("ErrorResponse", func() {
-		It("should return the response with an error", func() {
+		It("should return a denied response with an error", func() {
 			err := errors.New("this is an error")
 			expected := Response{
 				AdmissionResponse: admissionv1beta1.AdmissionResponse{
@@ -47,30 +134,65 @@ var _ = Describe("Admission Webhook Response Helpers", func() {
 	})
 
 	Describe("ValidationResponse", func() {
-		It("should return the response with an admission decision", func() {
-			expected := Response{
-				AdmissionResponse: admissionv1beta1.AdmissionResponse{
-					Allowed: true,
-					Result: &metav1.Status{
-						Reason: metav1.StatusReason("allow to admit"),
+		It("should populate a status with a reason when a reason is given", func() {
+			By("checking that a message is populated for 'allowed' responses")
+			Expect(ValidationResponse(true, "acceptable")).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: true,
+						Result: &metav1.Status{
+							Reason: "acceptable",
+						},
 					},
 				},
-			}
-			resp := ValidationResponse(true, "allow to admit")
-			Expect(resp).To(Equal(expected))
+			))
+
+			By("checking that a message is populated for 'denied' responses")
+			Expect(ValidationResponse(false, "UNACCEPTABLE!")).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: false,
+						Result: &metav1.Status{
+							Reason: "UNACCEPTABLE!",
+						},
+					},
+				},
+			))
+		})
+
+		It("should return an admission decision", func() {
+			By("checking that it returns a 'denied' response when allowed is false")
+			Expect(ValidationResponse(true, "")).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: true,
+					},
+				},
+			))
+
+			By("checking that it returns an 'allowed' response when allowed is true")
+			Expect(ValidationResponse(false, "")).To(Equal(
+				Response{
+					AdmissionResponse: admissionv1beta1.AdmissionResponse{
+						Allowed: false,
+					},
+				},
+			))
 		})
 	})
 
-	Describe("PatchResponse", func() {
-		It("should return the response with patches", func() {
+	Describe("PatchResponseFromRaw", func() {
+		It("should return an 'allowed' response with a patch of the diff between two sets of serialized JSON", func() {
 			expected := Response{
-				Patches: []jsonpatch.JsonPatchOperation{},
+				Patches: []jsonpatch.JsonPatchOperation{
+					{Operation: "replace", Path: "/a", Value: "bar"},
+				},
 				AdmissionResponse: admissionv1beta1.AdmissionResponse{
 					Allowed:   true,
 					PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(),
 				},
 			}
-			resp := PatchResponseFromRaw([]byte(`{}`), []byte(`{}`))
+			resp := PatchResponseFromRaw([]byte(`{"a": "foo"}`), []byte(`{"a": "bar"}`))
 			Expect(resp).To(Equal(expected))
 		})
 	})