From a7286e8afaaa137a0cff7dc845a88a9339c5f696 Mon Sep 17 00:00:00 2001 From: juanvallejo Date: Wed, 24 Aug 2016 12:38:02 -0400 Subject: [PATCH] fix duplicate validation/field/errors 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 | 5 ++- pkg/util/validation/field/errors.go | 13 ++++++-- pkg/util/validation/field/errors_test.go | 40 +++++++++++++++++++----- 3 files changed, 46 insertions(+), 12 deletions(-) diff --git a/pkg/util/validation/field/BUILD b/pkg/util/validation/field/BUILD index 81339eb6499..91f2a6beca2 100644 --- a/pkg/util/validation/field/BUILD +++ b/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( diff --git a/pkg/util/validation/field/errors.go b/pkg/util/validation/field/errors.go index b4a6c5cd598..227a49eea36 100644 --- a/pkg/util/validation/field/errors.go +++ b/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)) - for i := range list { - errs[i] = list[i] + errs := make([]error, 0, len(list)) + errorMsgs := sets.NewString() + 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) } diff --git a/pkg/util/validation/field/errors_test.go b/pkg/util/validation/field/errors_test.go index da8baa37e36..023939c8f66 100644 --- a/pkg/util/validation/field/errors_test.go +++ b/pkg/util/validation/field/errors_test.go @@ -99,22 +99,46 @@ func TestErrorUsefulMessage(t *testing.T) { } func TestToAggregate(t *testing.T) { - testCases := []ErrorList{ - nil, - {}, - {Invalid(NewPath("f"), "v", "d")}, - {Invalid(NewPath("f"), "v", "d"), InternalError(NewPath(""), fmt.Errorf("e"))}, + 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())) } } }