Merge pull request #54734 from janetkuo/webhook-error

Automatic merge from submit-queue (batch tested with PRs 53645, 54734, 54586, 55015, 54688). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Improve webhook error message

**What this PR does / why we need it**:
Currently, apiserver only prints message of review status returned by a rejecting webhook controller. If the message is empty, users will see this in event message: 
`create Pod <pod-name> failed error:<empty-string>`. Hook name should be included in the error message as well.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: 

**Special notes for your reviewer**: @kubernetes/sig-api-machinery-bugs 

**Release note**:

```release-note
NONE
```

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",

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,
 	}
 }
 

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)
+		}
+	}
+}