From d7aee90f36644e5b6c998127701596b73a6dfa6f Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 31 Jan 2019 09:51:55 -0500 Subject: [PATCH 1/3] deduplicate aggregated errors when generating Error() string --- .../k8s.io/apimachinery/pkg/util/errors/BUILD | 1 + .../apimachinery/pkg/util/errors/errors.go | 23 ++++++++++--- .../pkg/util/errors/errors_test.go | 32 +++++++++++++++++++ 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/errors/BUILD b/staging/src/k8s.io/apimachinery/pkg/util/errors/BUILD index c1de3109a4d..25d90f31cc1 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/errors/BUILD +++ b/staging/src/k8s.io/apimachinery/pkg/util/errors/BUILD @@ -20,6 +20,7 @@ go_library( ], importmap = "k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/errors", importpath = "k8s.io/apimachinery/pkg/util/errors", + deps = ["//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library"], ) filegroup( diff --git a/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go b/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go index 88e937679da..b05b6aee1aa 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go @@ -19,6 +19,8 @@ package errors import ( "errors" "fmt" + + "k8s.io/apimachinery/pkg/util/sets" ) // MessageCountMap contains occurrence for each error message. @@ -67,12 +69,23 @@ func (agg aggregate) Error() string { if len(agg) == 1 { return agg[0].Error() } - result := fmt.Sprintf("[%s", agg[0].Error()) - for i := 1; i < len(agg); i++ { - result += fmt.Sprintf(", %s", agg[i].Error()) + seenerrs := sets.NewString() + result := "" + for _, err := range agg { + msg := err.Error() + if seenerrs.Has(msg) { + continue + } + seenerrs.Insert(msg) + if len(seenerrs) > 1 { + result += ", " + } + result += msg } - result += "]" - return result + if len(seenerrs) == 1 { + return result + } + return "[" + result + "]" } // Errors is part of the Aggregate interface. diff --git a/staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go b/staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go index 0ad3967d285..79edc5cfcf2 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go @@ -147,6 +147,38 @@ func TestPluralAggregate(t *testing.T) { } } +func TestDedupeAggregate(t *testing.T) { + var slice []error = []error{fmt.Errorf("abc"), fmt.Errorf("abc")} + var agg Aggregate + + agg = NewAggregate(slice) + if agg == nil { + t.Errorf("expected non-nil") + } + if s := agg.Error(); s != "abc" { + t.Errorf("expected 'abc', got %q", s) + } + if s := agg.Errors(); len(s) != 2 { + t.Errorf("expected two-elements slice, got %#v", s) + } +} + +func TestDedupePluralAggregate(t *testing.T) { + var slice []error = []error{fmt.Errorf("abc"), fmt.Errorf("abc"), fmt.Errorf("123")} + var agg Aggregate + + agg = NewAggregate(slice) + if agg == nil { + t.Errorf("expected non-nil") + } + if s := agg.Error(); s != "[abc, 123]" { + t.Errorf("expected '[abc, 123]', got %q", s) + } + if s := agg.Errors(); len(s) != 3 { + t.Errorf("expected three-elements slice, got %#v", s) + } +} + func TestFilterOut(t *testing.T) { testCases := []struct { err error From 6da24b591e8190ae1f98e21d4586c642f1c97e8d Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 31 Jan 2019 09:59:02 -0500 Subject: [PATCH 2/3] Flatten aggregated error when generating Error() string --- .../apimachinery/pkg/util/errors/errors.go | 21 ++++++++++-- .../pkg/util/errors/errors_test.go | 32 +++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go b/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go index b05b6aee1aa..62a73f34ebe 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go @@ -71,23 +71,38 @@ func (agg aggregate) Error() string { } seenerrs := sets.NewString() result := "" - for _, err := range agg { + agg.visit(func(err error) { msg := err.Error() if seenerrs.Has(msg) { - continue + return } seenerrs.Insert(msg) if len(seenerrs) > 1 { result += ", " } result += msg - } + }) if len(seenerrs) == 1 { return result } return "[" + result + "]" } +func (agg aggregate) visit(f func(err error)) { + for _, err := range agg { + switch err := err.(type) { + case aggregate: + err.visit(f) + case Aggregate: + for _, nestedErr := range err.Errors() { + f(nestedErr) + } + default: + f(err) + } + } +} + // Errors is part of the Aggregate interface. func (agg aggregate) Errors() []error { return []error(agg) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go b/staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go index 79edc5cfcf2..d70a4d51a0b 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/errors/errors_test.go @@ -179,6 +179,38 @@ func TestDedupePluralAggregate(t *testing.T) { } } +func TestFlattenAndDedupeAggregate(t *testing.T) { + var slice []error = []error{fmt.Errorf("abc"), fmt.Errorf("abc"), NewAggregate([]error{fmt.Errorf("abc")})} + var agg Aggregate + + agg = NewAggregate(slice) + if agg == nil { + t.Errorf("expected non-nil") + } + if s := agg.Error(); s != "abc" { + t.Errorf("expected 'abc', got %q", s) + } + if s := agg.Errors(); len(s) != 3 { + t.Errorf("expected three-elements slice, got %#v", s) + } +} + +func TestFlattenAggregate(t *testing.T) { + var slice []error = []error{fmt.Errorf("abc"), fmt.Errorf("abc"), NewAggregate([]error{fmt.Errorf("abc"), fmt.Errorf("def"), NewAggregate([]error{fmt.Errorf("def"), fmt.Errorf("ghi")})})} + var agg Aggregate + + agg = NewAggregate(slice) + if agg == nil { + t.Errorf("expected non-nil") + } + if s := agg.Error(); s != "[abc, def, ghi]" { + t.Errorf("expected '[abc, def, ghi]', got %q", s) + } + if s := agg.Errors(); len(s) != 3 { + t.Errorf("expected three-elements slice, got %#v", s) + } +} + func TestFilterOut(t *testing.T) { testCases := []struct { err error From fe549a5a17884434c1a1eff7d5229fdffb9a9cf9 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 31 Jan 2019 09:59:48 -0500 Subject: [PATCH 3/3] Return authentication webhook error message --- .../plugin/pkg/authenticator/token/webhook/webhook.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go index cf0a83b5d97..e13985d726b 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go +++ b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go @@ -19,6 +19,7 @@ package webhook import ( "context" + "errors" "time" authentication "k8s.io/api/authentication/v1beta1" @@ -120,7 +121,11 @@ func (w *WebhookTokenAuthenticator) AuthenticateToken(ctx context.Context, token r.Status = result.Status if !r.Status.Authenticated { - return nil, false, nil + var err error + if len(r.Status.Error) != 0 { + err = errors.New(r.Status.Error) + } + return nil, false, err } var extra map[string][]string