diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index cbae7146..a1ac8c84 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -288,8 +288,7 @@ func TestGetKubeconfig(t *testing.T) { }, wantError: true, wantStderr: here.Doc(` - Error: invalid api group suffix: 1 error(s): - - a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') + Error: invalid api group suffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') `), }, { diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 41607e4e..822b9f82 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -132,8 +132,7 @@ func TestLoginOIDCCommand(t *testing.T) { }, wantError: true, wantStderr: here.Doc(` - Error: invalid concierge parameters: invalid api group suffix: 1 error(s): - - a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') + Error: invalid concierge parameters: invalid api group suffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') `), }, { diff --git a/cmd/pinniped/cmd/login_static_test.go b/cmd/pinniped/cmd/login_static_test.go index 0152dbd0..5d0114cb 100644 --- a/cmd/pinniped/cmd/login_static_test.go +++ b/cmd/pinniped/cmd/login_static_test.go @@ -142,8 +142,7 @@ func TestLoginStaticCommand(t *testing.T) { }, wantError: true, wantStderr: here.Doc(` - Error: invalid concierge parameters: invalid api group suffix: 1 error(s): - - a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') + Error: invalid concierge parameters: invalid api group suffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') `), }, { diff --git a/internal/config/concierge/config_test.go b/internal/config/concierge/config_test.go index 31b7a356..4073d96d 100644 --- a/internal/config/concierge/config_test.go +++ b/internal/config/concierge/config_test.go @@ -197,7 +197,7 @@ func TestFromPath(t *testing.T) { credentialIssuer: pinniped-config apiService: pinniped-api `), - wantError: "validate apiGroupSuffix: 1 error(s):\n- a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", + wantError: "validate apiGroupSuffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", }, } for _, test := range tests { diff --git a/internal/config/supervisor/config_test.go b/internal/config/supervisor/config_test.go index 7576822c..7fb1acc8 100644 --- a/internal/config/supervisor/config_test.go +++ b/internal/config/supervisor/config_test.go @@ -72,7 +72,7 @@ func TestFromPath(t *testing.T) { names: defaultTLSCertificateSecret: my-secret-name `), - wantError: "validate apiGroupSuffix: 1 error(s):\n- a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", + wantError: "validate apiGroupSuffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", }, } for _, test := range tests { diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index e0f87b3c..4ba54cc7 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -12,6 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/clock" + "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/util/retry" "k8s.io/klog/v2" @@ -20,7 +21,6 @@ import ( configinformers "go.pinniped.dev/generated/1.20/client/supervisor/informers/externalversions/config/v1alpha1" pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/multierror" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" ) @@ -107,7 +107,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro } } - errs := multierror.New() + var errs []error federationDomainIssuers := make([]*provider.FederationDomainIssuer, 0) for _, federationDomain := range federationDomains { @@ -123,7 +123,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro configv1alpha1.DuplicateFederationDomainStatusCondition, "Duplicate issuer: "+federationDomain.Spec.Issuer, ); err != nil { - errs.Add(fmt.Errorf("could not update status: %w", err)) + errs = append(errs, fmt.Errorf("could not update status: %w", err)) } continue } @@ -138,7 +138,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro configv1alpha1.SameIssuerHostMustUseSameSecretFederationDomainStatusCondition, "Issuers with the same DNS hostname (address not including port) must use the same secretName: "+issuerURLToHostnameKey(issuerURL), ); err != nil { - errs.Add(fmt.Errorf("could not update status: %w", err)) + errs = append(errs, fmt.Errorf("could not update status: %w", err)) } continue } @@ -152,7 +152,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro configv1alpha1.InvalidFederationDomainStatusCondition, "Invalid: "+err.Error(), ); err != nil { - errs.Add(fmt.Errorf("could not update status: %w", err)) + errs = append(errs, fmt.Errorf("could not update status: %w", err)) } continue } @@ -164,7 +164,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro configv1alpha1.SuccessFederationDomainStatusCondition, "Provider successfully created", ); err != nil { - errs.Add(fmt.Errorf("could not update status: %w", err)) + errs = append(errs, fmt.Errorf("could not update status: %w", err)) continue } @@ -173,7 +173,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro c.providerSetter.SetProviders(federationDomainIssuers...) - return errs.ErrOrNil() + return errors.NewAggregate(errs) } func (c *federationDomainWatcherController) updateStatus( diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index e370a560..e7324c42 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go @@ -6,6 +6,7 @@ package supervisorconfig import ( "context" "errors" + "fmt" "net/url" "reflect" "sync" @@ -320,7 +321,7 @@ func TestSync(t *testing.T) { it("sets the provider that it could actually update in the API", func() { startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) - r.EqualError(err, "1 error(s):\n- could not update status: some update error") + r.EqualError(err, "could not update status: some update error") provider1, err := provider.NewFederationDomainIssuer(federationDomain1.Spec.Issuer) r.NoError(err) @@ -339,7 +340,7 @@ func TestSync(t *testing.T) { it("returns an error", func() { startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) - r.EqualError(err, "1 error(s):\n- could not update status: some update error") + r.EqualError(err, "could not update status: some update error") federationDomain1.Status.Status = v1alpha1.SuccessFederationDomainStatusCondition federationDomain1.Status.Message = "Provider successfully created" @@ -455,7 +456,7 @@ func TestSync(t *testing.T) { it("returns an error", func() { startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) - r.EqualError(err, "1 error(s):\n- could not update status: some update error") + r.EqualError(err, "could not update status: some update error") federationDomain.Status.Status = v1alpha1.SuccessFederationDomainStatusCondition federationDomain.Status.Message = "Provider successfully created" @@ -491,7 +492,7 @@ func TestSync(t *testing.T) { it("returns the get error", func() { startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) - r.EqualError(err, "1 error(s):\n- could not update status: get failed: some get error") + r.EqualError(err, "could not update status: get failed: some get error") federationDomain.Status.Status = v1alpha1.SuccessFederationDomainStatusCondition federationDomain.Status.Message = "Provider successfully created" @@ -606,7 +607,7 @@ func TestSync(t *testing.T) { it("sets the provider that it could actually update in the API", func() { startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) - r.EqualError(err, "1 error(s):\n- could not update status: some update error") + r.EqualError(err, "could not update status: some update error") validProvider, err := provider.NewFederationDomainIssuer(validFederationDomain.Spec.Issuer) r.NoError(err) @@ -623,7 +624,7 @@ func TestSync(t *testing.T) { it("returns an error", func() { startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) - r.EqualError(err, "1 error(s):\n- could not update status: some update error") + r.EqualError(err, "could not update status: some update error") validFederationDomain.Status.Status = v1alpha1.SuccessFederationDomainStatusCondition validFederationDomain.Status.Message = "Provider successfully created" @@ -761,22 +762,20 @@ func TestSync(t *testing.T) { }) when("we cannot talk to the API", func() { + var count int it.Before(func() { pinnipedAPIClient.PrependReactor( "get", "federationdomains", func(_ coretesting.Action) (bool, runtime.Object, error) { - return true, nil, errors.New("some get error") + count++ + return true, nil, fmt.Errorf("some get error %d", count) }, ) }) it("returns the get errors", func() { - expectedError := here.Doc(` - 3 error(s): - - could not update status: get failed: some get error - - could not update status: get failed: some get error - - could not update status: get failed: some get error`) + expectedError := here.Doc(`[could not update status: get failed: some get error 1, could not update status: get failed: some get error 2, could not update status: get failed: some get error 3]`) startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) r.EqualError(err, expectedError) @@ -947,23 +946,20 @@ func TestSync(t *testing.T) { }) when("we cannot talk to the API", func() { + var count int it.Before(func() { pinnipedAPIClient.PrependReactor( "get", "federationdomains", func(_ coretesting.Action) (bool, runtime.Object, error) { - return true, nil, errors.New("some get error") + count++ + return true, nil, fmt.Errorf("some get error %d", count) }, ) }) it("returns the get errors", func() { - expectedError := here.Doc(` - 4 error(s): - - could not update status: get failed: some get error - - could not update status: get failed: some get error - - could not update status: get failed: some get error - - could not update status: get failed: some get error`) + expectedError := here.Doc(`[could not update status: get failed: some get error 1, could not update status: get failed: some get error 2, could not update status: get failed: some get error 3, could not update status: get failed: some get error 4]`) startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) r.EqualError(err, expectedError) diff --git a/internal/groupsuffix/groupsuffix.go b/internal/groupsuffix/groupsuffix.go index bdd1bad1..39f556a7 100644 --- a/internal/groupsuffix/groupsuffix.go +++ b/internal/groupsuffix/groupsuffix.go @@ -9,11 +9,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/validation" "go.pinniped.dev/internal/constable" "go.pinniped.dev/internal/kubeclient" - "go.pinniped.dev/internal/multierror" ) const ( @@ -127,17 +127,17 @@ func Unreplace(baseAPIGroup, apiGroupSuffix string) (string, bool) { // makes sure that the provided apiGroupSuffix is a valid DNS-1123 subdomain with at least one dot, // to match Kubernetes behavior. func Validate(apiGroupSuffix string) error { - err := multierror.New() + var errs []error //nolint: prealloc if len(strings.Split(apiGroupSuffix, ".")) < 2 { - err.Add(constable.Error("must contain '.'")) + errs = append(errs, constable.Error("must contain '.'")) } errorStrings := validation.IsDNS1123Subdomain(apiGroupSuffix) for _, errorString := range errorStrings { errorString := errorString - err.Add(constable.Error(errorString)) + errs = append(errs, constable.Error(errorString)) } - return err.ErrOrNil() + return errors.NewAggregate(errs) } diff --git a/internal/groupsuffix/groupsuffix_test.go b/internal/groupsuffix/groupsuffix_test.go index e649e1d7..f9c599e0 100644 --- a/internal/groupsuffix/groupsuffix_test.go +++ b/internal/groupsuffix/groupsuffix_test.go @@ -487,19 +487,19 @@ func TestValidate(t *testing.T) { }, { apiGroupSuffix: "no-dots", - wantErrorPrefix: "1 error(s):\n- must contain '.'", + wantErrorPrefix: "must contain '.'", }, { apiGroupSuffix: ".starts.with.dot", - wantErrorPrefix: "1 error(s):\n- a lowercase RFC 1123 subdomain must consist", + wantErrorPrefix: "a lowercase RFC 1123 subdomain must consist", }, { apiGroupSuffix: "ends.with.dot.", - wantErrorPrefix: "1 error(s):\n- a lowercase RFC 1123 subdomain must consist", + wantErrorPrefix: "a lowercase RFC 1123 subdomain must consist", }, { apiGroupSuffix: ".multiple-issues.because-this-string-is-longer-than-the-253-character-limit-of-a-dns-1123-identifier-" + chars(253), - wantErrorPrefix: "2 error(s):\n- must be no more than 253 characters\n- a lowercase RFC 1123 subdomain must consist", + wantErrorPrefix: "[must be no more than 253 characters, a lowercase RFC 1123 subdomain must consist", }, } for _, test := range tests { diff --git a/internal/multierror/multierror.go b/internal/multierror/multierror.go deleted file mode 100644 index 5f87107f..00000000 --- a/internal/multierror/multierror.go +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -// Package multierror provides a type that can translate multiple errors into a Go error interface. -// -// A common use of this package is as follows. -// errs := multierror.New() -// for i := range stuff { -// err := doThing(i) -// errs.Add(err) -// } -// return errs.ErrOrNil() -package multierror - -import ( - "fmt" - "strings" -) - -// formatFunc is a function used to format the string representing of a MultiError. It is used in the -// Error() function. -// -// It is marked out here to indicate how we could potentially extend MultiError in the future to -// support more styles of converting from a list of error's to a string. -//nolint: gochecknoglobals -var formatFunc func(errs MultiError, sb *strings.Builder) = defaultFormat - -// MultiError holds a list of error's, that could potentially be empty. -// -// Use New() to create a MultiError. -type MultiError []error - -// New returns an empty MultiError. -func New() MultiError { - return make([]error, 0) -} - -// Add adds an error to the MultiError. The provided err must not be nil. -func (m *MultiError) Add(err error) { - *m = append(*m, err) -} - -// Error implements the error.Error() interface method. -func (m MultiError) Error() string { - sb := strings.Builder{} - formatFunc(m, &sb) - return sb.String() -} - -// ErrOrNil returns either nil, if there are no errors in this MultiError, or an error, otherwise. -func (m MultiError) ErrOrNil() error { - if len(m) > 0 { - return m - } - return nil -} - -func defaultFormat(errs MultiError, sb *strings.Builder) { - _, _ = fmt.Fprintf(sb, "%d error(s):", len(errs)) - for _, err := range errs { - _, _ = fmt.Fprintf(sb, "\n- %s", err.Error()) - } -} diff --git a/internal/multierror/multierror_test.go b/internal/multierror/multierror_test.go deleted file mode 100644 index cb96771e..00000000 --- a/internal/multierror/multierror_test.go +++ /dev/null @@ -1,24 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package multierror - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestMultierror(t *testing.T) { - errs := New() - - require.Nil(t, errs.ErrOrNil()) - - errs.Add(errors.New("some error 1")) - require.EqualError(t, errs.ErrOrNil(), "1 error(s):\n- some error 1") - - errs.Add(errors.New("some error 2")) - errs.Add(errors.New("some error 3")) - require.EqualError(t, errs.ErrOrNil(), "3 error(s):\n- some error 1\n- some error 2\n- some error 3") -} diff --git a/internal/testutil/fakekubeapi/fakekubeapi.go b/internal/testutil/fakekubeapi/fakekubeapi.go index 66450f58..55f3ac8b 100644 --- a/internal/testutil/fakekubeapi/fakekubeapi.go +++ b/internal/testutil/fakekubeapi/fakekubeapi.go @@ -30,6 +30,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/errors" kubescheme "k8s.io/client-go/kubernetes/scheme" restclient "k8s.io/client-go/rest" aggregatorclientscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" @@ -37,7 +38,6 @@ import ( pinnipedconciergeclientsetscheme "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned/scheme" pinnipedsupervisorclientsetscheme "go.pinniped.dev/generated/1.20/client/supervisor/clientset/versioned/scheme" "go.pinniped.dev/internal/httputil/httperr" - "go.pinniped.dev/internal/multierror" ) // Start starts an httptest.Server (with TLS) that pretends to be a Kube API server. @@ -107,7 +107,7 @@ func decodeObj(r *http.Request) (runtime.Object, error) { } var obj runtime.Object - multiErr := multierror.New() + var errs []error //nolint: prealloc codecsThatWeUseInOurCode := []runtime.NegotiatedSerializer{ kubescheme.Codecs, aggregatorclientscheme.Codecs, @@ -119,9 +119,9 @@ func decodeObj(r *http.Request) (runtime.Object, error) { if err == nil { return obj, nil } - multiErr.Add(err) + errs = append(errs, err) } - return nil, multiErr.ErrOrNil() + return nil, errors.NewAggregate(errs) } func tryDecodeObj( diff --git a/pkg/conciergeclient/conciergeclient_test.go b/pkg/conciergeclient/conciergeclient_test.go index ddbe5025..bdf27364 100644 --- a/pkg/conciergeclient/conciergeclient_test.go +++ b/pkg/conciergeclient/conciergeclient_test.go @@ -112,7 +112,7 @@ func TestNew(t *testing.T) { WithEndpoint("https://example.com"), WithAPIGroupSuffix(""), }, - wantErr: "invalid api group suffix: 2 error(s):\n- must contain '.'\n- a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", + wantErr: "invalid api group suffix: [must contain '.', a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')]", }, { name: "invalid api group suffix", @@ -121,7 +121,7 @@ func TestNew(t *testing.T) { WithEndpoint("https://example.com"), WithAPIGroupSuffix(".starts.with.dot"), }, - wantErr: "invalid api group suffix: 1 error(s):\n- a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", + wantErr: "invalid api group suffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", }, { name: "valid",