Merge pull request #30313 from juanvallejo/jvallejo_kube-remove-duplicate-error-msgs

Automatic merge from submit-queue

remove duplicate errors from aggregate error outputs

release-label-none

Duplicate error messages are sometimes shown when displaying aggregate errors:

`$ kubectl label pod/database-1-fn0r7 qwer1345%$$#=self`
```
* metadata.labels: Invalid value: "qwer1345%5602#": name part must match the regex ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9] (e.g. 'MyName' or 'my.name' or '123-abc')
* metadata.labels: Invalid value: "qwer1345%5602#": name part must match the regex ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9] (e.g. 'MyName' or 'my.name' or '123-abc')
* metadata.labels: Invalid value: "qwer1345%5602#": name part must match the regex ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9] (e.g. 'MyName' or 'my.name' or '123-abc')
```

This patch removes any duplicate messages (adjacent or not) that appear in the final list of errors.

pkg/kubectl/cmd/util/helpers.go

@@ -19,6 +19,7 @@ package util
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -27,7 +28,7 @@ import (
 	"strings"
 	"time"
 
-	"k8s.io/kubernetes/pkg/api/errors"
+	kerrors "k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/api/meta"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/apimachinery/registered"
@@ -38,6 +39,7 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl/resource"
 	"k8s.io/kubernetes/pkg/runtime"
 	utilerrors "k8s.io/kubernetes/pkg/util/errors"
+	"k8s.io/kubernetes/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/util/strategicpatch"
 
 	"github.com/evanphx/json-patch"
@@ -59,10 +61,10 @@ type debugError interface {
 // souce is the filename or URL to the template file(*.json or *.yaml), or stdin to use to handle the resource.
 func AddSourceToErr(verb string, source string, err error) error {
 	if source != "" {
-		if statusError, ok := err.(errors.APIStatus); ok {
+		if statusError, ok := err.(kerrors.APIStatus); ok {
 			status := statusError.Status()
 			status.Message = fmt.Sprintf("error when %s %q: %v", verb, source, status.Message)
-			return &errors.StatusError{ErrStatus: status}
+			return &kerrors.StatusError{ErrStatus: status}
 		}
 		return fmt.Errorf("error when %s %q: %v", verb, source, err)
 	}
@@ -118,11 +120,12 @@ func checkErr(pref string, err error, handleErr func(string)) {
 		return
 	}
 
-	if errors.IsInvalid(err) {
+	if kerrors.IsInvalid(err) {
-		details := err.(*errors.StatusError).Status().Details
+		details := err.(*kerrors.StatusError).Status().Details
 		prefix := fmt.Sprintf("%sThe %s %q is invalid.\n", pref, details.Kind, details.Name)
 		errs := statusCausesToAggrError(details.Causes)
 		handleErr(MultilineError(prefix, errs))
+		return
 	}
 
 	if noMatch, ok := err.(*meta.NoResourceMatchError); ok {
@@ -142,9 +145,11 @@ func checkErr(pref string, err error, handleErr func(string)) {
 	// handle multiline errors
 	if clientcmd.IsConfigurationInvalid(err) {
 		handleErr(MultilineError(fmt.Sprintf("%sError in configuration: ", pref), err))
+		return
 	}
 	if agg, ok := err.(utilerrors.Aggregate); ok && len(agg.Errors()) > 0 {
 		handleErr(MultipleErrors(pref, agg.Errors()))
+		return
 	}
 
 	msg, ok := StandardErrorMessage(err)
@@ -158,9 +163,16 @@ func checkErr(pref string, err error, handleErr func(string)) {
 }
 
 func statusCausesToAggrError(scs []unversioned.StatusCause) utilerrors.Aggregate {
-	errs := make([]error, len(scs))
+	errs := make([]error, 0, len(scs))
-	for i, sc := range scs {
+	errorMsgs := sets.NewString()
-		errs[i] = fmt.Errorf("%s: %s", sc.Field, sc.Message)
+	for _, sc := range scs {
+		// check for duplicate error messages and skip them
+		msg := fmt.Sprintf("%s: %s", sc.Field, sc.Message)
+		if errorMsgs.Has(msg) {
+			continue
+		}
+		errorMsgs.Insert(msg)
+		errs = append(errs, errors.New(msg))
 	}
 	return utilerrors.NewAggregate(errs)
 }
@@ -175,7 +187,7 @@ func StandardErrorMessage(err error) (string, bool) {
 	if debugErr, ok := err.(debugError); ok {
 		glog.V(4).Infof(debugErr.DebugError())
 	}
-	status, isStatus := err.(errors.APIStatus)
+	status, isStatus := err.(kerrors.APIStatus)
 	switch {
 	case isStatus:
 		switch s := status.Status(); {
@@ -184,7 +196,7 @@ func StandardErrorMessage(err error) (string, bool) {
 		default:
 			return fmt.Sprintf("Error from server: %s", err.Error()), true
 		}
-	case errors.IsUnexpectedObjectError(err):
+	case kerrors.IsUnexpectedObjectError(err):
 		return fmt.Sprintf("Server returned an unexpected response: %s", err.Error()), true
 	}
 	switch t := err.(type) {
@@ -503,7 +515,7 @@ func GetThirdPartyGroupVersions(discovery discovery.DiscoveryInterface) ([]unver
 	groupList, err := discovery.ServerGroups()
 	if err != nil {
 		// On forbidden or not found, just return empty lists.
-		if errors.IsForbidden(err) || errors.IsNotFound(err) {
+		if kerrors.IsForbidden(err) || kerrors.IsNotFound(err) {
 			return result, gvks, nil
 		}
 

pkg/kubectl/cmd/util/helpers_test.go

@@ -220,21 +220,39 @@ func TestCheckInvalidErr(t *testing.T) {
 	}{
 		{
 			errors.NewInvalid(api.Kind("Invalid1"), "invalidation", field.ErrorList{field.Invalid(field.NewPath("field"), "single", "details")}),
-			`Error from server: Invalid1 "invalidation" is invalid: field: Invalid value: "single": details`,
+			`error: The Invalid1 "invalidation" is invalid. field: Invalid value: "single": details`,
 		},
 		{
 			errors.NewInvalid(api.Kind("Invalid2"), "invalidation", field.ErrorList{field.Invalid(field.NewPath("field1"), "multi1", "details"), field.Invalid(field.NewPath("field2"), "multi2", "details")}),
-			`Error from server: Invalid2 "invalidation" is invalid: [field1: Invalid value: "multi1": details, field2: Invalid value: "multi2": details]`,
+			`error: The Invalid2 "invalidation" is invalid. * field1: Invalid value: "multi1": details, * field2: Invalid value: "multi2": details`,
 		},
 		{
 			errors.NewInvalid(api.Kind("Invalid3"), "invalidation", field.ErrorList{}),
-			`Error from server: Invalid3 "invalidation" is invalid: <nil>`,
+			`error: The Invalid3 "invalidation" is invalid. %!s(<nil>)`,
+		},
+		{
+			errors.NewInvalid(api.Kind("Invalid4"), "invalidation", field.ErrorList{field.Invalid(field.NewPath("field4"), "multi4", "details"), field.Invalid(field.NewPath("field4"), "multi4", "details")}),
+			`error: The Invalid4 "invalidation" is invalid. field4: Invalid value: "multi4": details`,
 		},
 	}
 
 	var errReturned string
 	errHandle := func(err string) {
-		errReturned = err
+		for _, v := range strings.Split(err, "\n") {
+			separator := " "
+			if errReturned == "" || v == "" {
+				separator = ""
+			} else if !strings.HasSuffix(errReturned, ".") {
+				separator = ", "
+			}
+			errReturned = fmt.Sprintf("%s%s%s", errReturned, separator, v)
+		}
+		if !strings.HasPrefix(errReturned, "error: ") {
+			errReturned = fmt.Sprintf("error: %s", errReturned)
+		}
+		if strings.HasSuffix(errReturned, ", ") {
+			errReturned = errReturned[:len(errReturned)-len(" ,")]
+		}
 	}
 
 	for _, test := range tests {
@@ -243,6 +261,7 @@ func TestCheckInvalidErr(t *testing.T) {
 		if errReturned != test.expected {
 			t.Fatalf("Got: %s, expected: %s", errReturned, test.expected)
 		}
+		errReturned = ""
 	}
 }