Merge pull request #77022 from liggitt/webhook-error-success

Ensure 4xx+ response codes from webhook rejections
kubernetes · May 4, 2019 · aefa6d4492dd014c27cfc6f08ee5fe94384f87a3 · aefa6d4
2 parents fd5560e + 5007643
commit aefa6d4492dd014c27cfc6f08ee5fe94384f87a3
Unified Split

Showing with 94 additions and 3 deletions.

+10 −0 staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go

+84 −3 staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go
diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go
@@ -18,6 +18,7 @@ package errors
 import (
 	"fmt"
+  
+	"net/http"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -32,6 +33,15 @@ func ToStatusErr(webhookName string, result *metav1.Status) *apierrors.StatusErr
 		result = &metav1.Status{Status: metav1.StatusFailure}
 	}
+  
+	// Make sure we don't return < 400 status codes along with a rejection
+  
+	if result.Code < http.StatusBadRequest {
+  
+		result.Code = http.StatusBadRequest
+  
+	}
+  
+	// Make sure we don't return "" or "Success" status along with a rejection
+  
+	if result.Status == "" || result.Status == metav1.StatusSuccess {
+  
+		result.Status = metav1.StatusFailure
+  
+	}
+  
+
 	switch {
 	case len(result.Message) > 0:
 		result.Message = fmt.Sprintf("%s: %s", deniedBy, result.Message)

diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror_test.go
@@ -27,28 +27,36 @@ 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
+  
+		name           string
+  
+		result         *metav1.Status
+  
+		expectedError  string
+  
+		expectedCode   int32
+  
+		expectedStatus string
 	}{
 		{
 			"nil result",
 			nil,
 			deniedBy + " without explanation",
+  
+			400,
+  
+			metav1.StatusFailure,
 		},
 		{
 			"only message",
 			&metav1.Status{
 				Message: "you shall not pass",
 			},
 			deniedBy + ": you shall not pass",
+  
+			400,
+  
+			metav1.StatusFailure,
 		},
 		{
 			"only reason",
 			&metav1.Status{
 				Reason: metav1.StatusReasonForbidden,
 			},
 			deniedBy + ": Forbidden",
+  
+			400,
+  
+			metav1.StatusFailure,
 		},
 		{
 			"message and reason",
@@ -57,17 +65,90 @@ func TestToStatusErr(t *testing.T) {
 				Reason:  metav1.StatusReasonForbidden,
 			},
 			deniedBy + ": you shall not pass",
+  
+			400,
+  
+			metav1.StatusFailure,
 		},
 		{
 			"no message, no reason",
 			&metav1.Status{},
 			deniedBy + " without explanation",
+  
+			400,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"custom 4xx status code",
+  
+			&metav1.Status{Code: 401},
+  
+			deniedBy + " without explanation",
+  
+			401,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"custom 5xx status code",
+  
+			&metav1.Status{Code: 500},
+  
+			deniedBy + " without explanation",
+  
+			500,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"200 status code",
+  
+			&metav1.Status{Code: 200},
+  
+			deniedBy + " without explanation",
+  
+			400,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"300 status code",
+  
+			&metav1.Status{Code: 300},
+  
+			deniedBy + " without explanation",
+  
+			400,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"399 status code",
+  
+			&metav1.Status{Code: 399},
+  
+			deniedBy + " without explanation",
+  
+			400,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"missing status",
+  
+			&metav1.Status{},
+  
+			deniedBy + " without explanation",
+  
+			400,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"success status overridden",
+  
+			&metav1.Status{Status: metav1.StatusSuccess},
+  
+			deniedBy + " without explanation",
+  
+			400,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"failure status preserved",
+  
+			&metav1.Status{Status: metav1.StatusFailure},
+  
+			deniedBy + " without explanation",
+  
+			400,
+  
+			metav1.StatusFailure,
+  
+		},
+  
+		{
+  
+			"custom status preserved",
+  
+			&metav1.Status{Status: "custom"},
+  
+			deniedBy + " without explanation",
+  
+			400,
+  
+			"custom",
 		},
 	}
 	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)
 		}
+  
+		if err.ErrStatus.Code != test.expectedCode {
+  
+			t.Errorf("%s: expected code %d, got %d", test.name, test.expectedCode, err.ErrStatus.Code)
+  
+		}
+  
+		if err.ErrStatus.Status != test.expectedStatus {
+  
+			t.Errorf("%s: expected code %q, got %q", test.name, test.expectedStatus, err.ErrStatus.Status)
+  
+		}
 	}
 }