Merge pull request #31353 from juanvallejo/jvallejo_fix-duplicate-errors-kubectl-set-env

Automatic merge from submit-queue

fix duplicate validation/field/errors

**Release note**:

``` release-note
release-note-none
```

Related PR: https://github.com/kubernetes/kubernetes/pull/30313

PR #30313 fixed duplicate errors for invalid aggregate errors in
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/util/helpers.go

However, duplicate aggregate errors that went through
https://github.com/kubernetes/kubernetes/blob/master/pkg/util/validation/field/errors.go
were not affected by that patch.

This patch adds duplicate aggregate error checking to
`pkg/util/validation/field/errors.go`
##### Before

`$ kubectl set env rc/idling-echo-1 test-abc=1234`

```
error: ReplicationController "idling-echo-1" is invalid:
[spec.template.spec.containers[0].env[0].name: Invalid value:
"test-abc": must be a C identifier (matching regex
[A-Za-z_][A-Za-z0-9_]*): e.g. "my_name" or "MyName",
spec.template.spec.containers[1].env[0].name: Invalid value: "test-abc":
must be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g.
"my_name" or "MyName", spec.template.spec.containers[0].env[0].name:
Invalid value: "test-abc": must be a C identifier (matching regex
[A-Za-z_][A-Za-z0-9_]*): e.g. "my_name" or "MyName",
spec.template.spec.containers[1].env[0].name: Invalid value: "test-abc":
must be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g.
"my_name" or "MyName"]
```

`$ kubectl set env rc/node-1 test-abc=1234`

```
error: ReplicationController "idling-echo-1" is invalid:
[spec.template.spec.containers[0].env[0].name: Invalid value:
"test-abc": must be a C identifier (matching regex
[A-Za-z_][A-Za-z0-9_]*): e.g. "my_name" or "MyName",
spec.template.spec.containers[1].env[0].name: Invalid value: "test-abc":
must be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g.
"my_name" or "MyName"]
```
##### After

`$ kubectl set env rc/idling-echo-1 test-abc=1234`

```
error: ReplicationController "idling-echo-1" is invalid:
[spec.template.spec.containers[0].env[0].name: Invalid value:
"test-abc": must be a C identifier (matching regex
[A-Za-z_][A-Za-z0-9_]*): e.g. "my_name" or "MyName",
spec.template.spec.containers[1].env[0].name: Invalid value: "test-abc":
must be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g.
"my_name" or "MyName"]
```

`$ kubectl set env rc/node-1 test-abc=1234`

```
error: ReplicationController "node-1" is invalid:
spec.template.spec.containers[0].env[0].name: Invalid value: "test-abc":
must be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g.
"my_name" or "MyName"
```

pkg/util/validation/field/BUILD

@@ -17,7 +17,10 @@ go_library(
         "path.go",
     ],
     tags = ["automanaged"],
-    deps = ["//pkg/util/errors:go_default_library"],
+    deps = [
+        "//pkg/util/errors:go_default_library",
+        "//pkg/util/sets:go_default_library",
+    ],
 )
 
 go_test(

pkg/util/validation/field/errors.go

@@ -22,6 +22,7 @@ import (
 	"strings"
 
 	utilerrors "k8s.io/kubernetes/pkg/util/errors"
+	"k8s.io/kubernetes/pkg/util/sets"
 )
 
 // Error is an implementation of the 'error' interface, which represents a
@@ -201,9 +202,15 @@ func NewErrorTypeMatcher(t ErrorType) utilerrors.Matcher {
 
 // ToAggregate converts the ErrorList into an errors.Aggregate.
 func (list ErrorList) ToAggregate() utilerrors.Aggregate {
-	errs := make([]error, len(list))
+	errs := make([]error, 0, len(list))
-	for i := range list {
+	errorMsgs := sets.NewString()
-		errs[i] = list[i]
+	for _, err := range list {
+		msg := fmt.Sprintf("%v", err)
+		if errorMsgs.Has(msg) {
+			continue
+		}
+		errorMsgs.Insert(msg)
+		errs = append(errs, err)
 	}
 	return utilerrors.NewAggregate(errs)
 }

pkg/util/validation/field/errors_test.go

@@ -99,22 +99,46 @@ func TestErrorUsefulMessage(t *testing.T) {
 }
 
 func TestToAggregate(t *testing.T) {
-	testCases := []ErrorList{
+	testCases := struct {
+		ErrList         []ErrorList
+		NumExpectedErrs []int
+	}{
+		[]ErrorList{
 			nil,
 			{},
 			{Invalid(NewPath("f"), "v", "d")},
+			{Invalid(NewPath("f"), "v", "d"), Invalid(NewPath("f"), "v", "d")},
 			{Invalid(NewPath("f"), "v", "d"), InternalError(NewPath(""), fmt.Errorf("e"))},
+		},
+		[]int{
+			0,
+			0,
+			1,
+			1,
+			2,
+		},
 	}
-	for i, tc := range testCases {
+
+	if len(testCases.ErrList) != len(testCases.NumExpectedErrs) {
+		t.Errorf("Mismatch: length of NumExpectedErrs does not match length of ErrList")
+	}
+	for i, tc := range testCases.ErrList {
 		agg := tc.ToAggregate()
+		numErrs := 0
+
+		if agg != nil {
+			numErrs = len(agg.Errors())
+		}
+		if numErrs != testCases.NumExpectedErrs[i] {
+			t.Errorf("[%d] Expected %d, got %d", i, testCases.NumExpectedErrs[i], numErrs)
+		}
+
 		if len(tc) == 0 {
 			if agg != nil {
 				t.Errorf("[%d] Expected nil, got %#v", i, agg)
 			}
 		} else if agg == nil {
 			t.Errorf("[%d] Expected non-nil", i)
-		} else if len(tc) != len(agg.Errors()) {
-			t.Errorf("[%d] Expected %d, got %d", i, len(tc), len(agg.Errors()))
 		}
 	}
 }