From 21676edd5208f55ac44aaaed3de59695d10bf076 Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <liggitt@google.com>
Date: Wed, 31 Aug 2022 11:49:34 -0400
Subject: [PATCH] Improve kubectl display of invalid errors

Kubernetes-commit: 6c549d75a8d951ec43ecd5394b2634c1a40e5dd1
---
 pkg/cmd/util/helpers.go      | 28 +++++++++++++++++--
 pkg/cmd/util/helpers_test.go | 54 ++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/pkg/cmd/util/helpers.go b/pkg/cmd/util/helpers.go
index 40ec0f19..784dc736 100644
--- a/pkg/cmd/util/helpers.go
+++ b/pkg/cmd/util/helpers.go
@@ -133,6 +133,20 @@ func CheckDiffErr(err error) {
 	})
 }
 
+// isInvalidReasonStatusError returns true if this is an API Status error with reason=Invalid.
+// This is distinct from generic 422 errors we want to fall back to generic error handling.
+func isInvalidReasonStatusError(err error) bool {
+	if !apierrors.IsInvalid(err) {
+		return false
+	}
+	statusError, isStatusError := err.(*apierrors.StatusError)
+	if !isStatusError {
+		return false
+	}
+	status := statusError.Status()
+	return status.Reason == metav1.StatusReasonInvalid
+}
+
 // checkErr formats a given error as a string and calls the passed handleErr
 // func with that string and an kubectl exit code.
 func checkErr(err error, handleErr func(string, int)) {
@@ -148,16 +162,26 @@ func checkErr(err error, handleErr func(string, int)) {
 	switch {
 	case err == ErrExit:
 		handleErr("", DefaultErrorExitCode)
-	case apierrors.IsInvalid(err):
-		details := err.(*apierrors.StatusError).Status().Details
+	case isInvalidReasonStatusError(err):
+		status := err.(*apierrors.StatusError).Status()
+		details := status.Details
 		s := "The request is invalid"
 		if details == nil {
+			// if we have no other details, include the message from the server if present
+			if len(status.Message) > 0 {
+				s += ": " + status.Message
+			}
 			handleErr(s, DefaultErrorExitCode)
 			return
 		}
 		if len(details.Kind) != 0 || len(details.Name) != 0 {
 			s = fmt.Sprintf("The %s %q is invalid", details.Kind, details.Name)
+		} else if len(status.Message) > 0 && len(details.Causes) == 0 {
+			// only append the message if we have no kind/name details and no causes,
+			// since default invalid error constructors duplicate that information in the message
+			s += ": " + status.Message
 		}
+
 		if len(details.Causes) > 0 {
 			errs := statusCausesToAggrError(details.Causes)
 			handleErr(MultilineError(s+": ", errs), DefaultErrorExitCode)
diff --git a/pkg/cmd/util/helpers_test.go b/pkg/cmd/util/helpers_test.go
index 6a695fd2..2c436e6e 100644
--- a/pkg/cmd/util/helpers_test.go
+++ b/pkg/cmd/util/helpers_test.go
@@ -348,6 +348,60 @@ func TestCheckInvalidErr(t *testing.T) {
 			"The request is invalid",
 			DefaultErrorExitCode,
 		},
+		// invalid error that that includes a message but no details
+		{
+			&errors.StatusError{metav1.Status{
+				Status: metav1.StatusFailure,
+				Code:   http.StatusUnprocessableEntity,
+				Reason: metav1.StatusReasonInvalid,
+				// Details is nil.
+				Message: "Some message",
+			}},
+			"The request is invalid: Some message",
+			DefaultErrorExitCode,
+		},
+		// webhook response that sets code=422 with no reason
+		{
+			&errors.StatusError{metav1.Status{
+				Status:  "Failure",
+				Message: `admission webhook "my.webhook" denied the request without explanation`,
+				Code:    422,
+			}},
+			`Error from server: admission webhook "my.webhook" denied the request without explanation`,
+			DefaultErrorExitCode,
+		},
+		// webhook response that sets code=422 with no reason and non-nil details
+		{
+			&errors.StatusError{metav1.Status{
+				Status:  "Failure",
+				Message: `admission webhook "my.webhook" denied the request without explanation`,
+				Code:    422,
+				Details: &metav1.StatusDetails{},
+			}},
+			`Error from server: admission webhook "my.webhook" denied the request without explanation`,
+			DefaultErrorExitCode,
+		},
+		// source-wrapped webhook response that sets code=422 with no reason
+		{
+			AddSourceToErr("creating", "configmap.yaml", &errors.StatusError{metav1.Status{
+				Status:  "Failure",
+				Message: `admission webhook "my.webhook" denied the request without explanation`,
+				Code:    422,
+			}}),
+			`Error from server: error when creating "configmap.yaml": admission webhook "my.webhook" denied the request without explanation`,
+			DefaultErrorExitCode,
+		},
+		// webhook response that sets reason=Invalid and code=422 and a message
+		{
+			&errors.StatusError{metav1.Status{
+				Status:  "Failure",
+				Reason:  "Invalid",
+				Message: `admission webhook "my.webhook" denied the request without explanation`,
+				Code:    422,
+			}},
+			`The request is invalid: admission webhook "my.webhook" denied the request without explanation`,
+			DefaultErrorExitCode,
+		},
 	})
 }