From 948bd7bbc18b343161120b365c089528f8fc0550 Mon Sep 17 00:00:00 2001
From: Janet Kuo <chiachenk@google.com>
Date: Fri, 27 Oct 2017 14:59:54 -0700
Subject: [PATCH] Add hook information when rejecting a request

---
 .../pkg/admission/plugin/webhook/BUILD        |  1 +
 .../pkg/admission/plugin/webhook/admission.go | 27 ++++++++--
 .../plugin/webhook/admission_test.go          | 53 +++++++++++++++++++
 3 files changed, 76 insertions(+), 5 deletions(-)

diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD
index 5a29c15d29a..a0ec3b05546 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/BUILD
@@ -51,6 +51,7 @@ go_test(
         "//vendor/k8s.io/api/admissionregistration/v1alpha1:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
index 33b6dd0a23c..7f1108bea04 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission.go
@@ -218,11 +218,11 @@ func (a *GenericAdmissionWebhook) Admit(attr admission.Attributes) error {
 				}
 
 				glog.Warningf("Failed calling webhook, failing closed %v: %v", hook.Name, err)
-				errCh <- err
+				errCh <- apierrors.NewInternalError(err)
 				return
 			}
 
-			glog.Warningf("rejected by webhook %v %t: %v", hook.Name, err, err)
+			glog.Warningf("rejected by webhook %q: %#v", hook.Name, err)
 			errCh <- err
 		}(&hooks[i])
 	}
@@ -273,12 +273,29 @@ func (a *GenericAdmissionWebhook) callHook(ctx context.Context, h *v1alpha1.Exte
 		return nil
 	}
 
-	if response.Status.Result == nil {
-		return fmt.Errorf("admission webhook %q denied the request without explanation", h.Name)
+	return toStatusErr(h.Name, response.Status.Result)
+}
+
+// toStatusErr returns a StatusError with information about the webhook controller
+func toStatusErr(name string, result *metav1.Status) *apierrors.StatusError {
+	deniedBy := fmt.Sprintf("admission webhook %q denied the request", name)
+	const noExp = "without explanation"
+
+	if result == nil {
+		result = &metav1.Status{Status: metav1.StatusFailure}
+	}
+
+	switch {
+	case len(result.Message) > 0:
+		result.Message = fmt.Sprintf("%s: %s", deniedBy, result.Message)
+	case len(result.Reason) > 0:
+		result.Message = fmt.Sprintf("%s: %s", deniedBy, result.Reason)
+	default:
+		result.Message = fmt.Sprintf("%s %s", deniedBy, noExp)
 	}
 
 	return &apierrors.StatusError{
-		ErrStatus: *response.Status.Result,
+		ErrStatus: *result,
 	}
 }
 
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go
index 391f16f9476..5124cb3a09b 100644
--- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/admission_test.go
@@ -30,6 +30,7 @@ import (
 	"k8s.io/api/admission/v1alpha1"
 	registrationv1alpha1 "k8s.io/api/admissionregistration/v1alpha1"
 	api "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/apiserver/pkg/admission"
@@ -280,6 +281,9 @@ func TestAdmit(t *testing.T) {
 					t.Errorf(" expected an error saying %q, but got %v", tt.errorContains, err)
 				}
 			}
+			if _, isStatusErr := err.(*apierrors.StatusError); err != nil && !isStatusErr {
+				t.Errorf("%s: expected a StatusError, got %T", name, err)
+			}
 		})
 	}
 }
@@ -333,3 +337,52 @@ type fakeAuthenticationInfoResolver struct {
 func (c *fakeAuthenticationInfoResolver) ClientConfigFor(server string) (*rest.Config, error) {
 	return c.restConfig, nil
 }
+
+func TestToStatusErr(t *testing.T) {
+	hookName := "foo"
+	deniedBy := fmt.Sprintf("admission webhook %q denied the request", hookName)
+	tests := []struct {
+		name          string
+		result        *metav1.Status
+		expectedError string
+	}{
+		{
+			"nil result",
+			nil,
+			deniedBy + " without explanation",
+		},
+		{
+			"only message",
+			&metav1.Status{
+				Message: "you shall not pass",
+			},
+			deniedBy + ": you shall not pass",
+		},
+		{
+			"only reason",
+			&metav1.Status{
+				Reason: metav1.StatusReasonForbidden,
+			},
+			deniedBy + ": Forbidden",
+		},
+		{
+			"message and reason",
+			&metav1.Status{
+				Message: "you shall not pass",
+				Reason:  metav1.StatusReasonForbidden,
+			},
+			deniedBy + ": you shall not pass",
+		},
+		{
+			"no message, no reason",
+			&metav1.Status{},
+			deniedBy + " without explanation",
+		},
+	}
+	for _, test := range tests {
+		err := toStatusErr(hookName, test.result)
+		if err == nil || err.Error() != test.expectedError {
+			t.Errorf("%s: expected an error saying %q, but got %v", test.name, test.expectedError, err)
+		}
+	}
+}