Merge pull request #395 from enj/enj/i/remove_multierror

Remove multierror package and migrate callers to k8s.io/apimachinery/pkg/util/errors.NewAggregate

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):
+				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])?)*')
-				- 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])?)*')
 			`),
 		},
 		{

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):
+				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])?)*')
-				- 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])?)*')
 			`),
 		},
 		{

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):
+				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])?)*')
-				- 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])?)*')
 			`),
 		},
 		{

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 {

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 {

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(

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(`
+					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]`)
-						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`)
 					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(`
+					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]`)
-						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`)
 					startInformersAndController()
 					err := controllerlib.TestSync(t, subject, *syncContext)
 					r.EqualError(err, expectedError)

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)
 }

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 {

internal/multierror/multierror.go

@@ -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())
-	}
-}

internal/multierror/multierror_test.go

@@ -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")
-}

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(

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",