diff --git a/docker/docker_client.go b/docker/docker_client.go index 5c4530c8e6..1f8078bd08 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -18,6 +18,7 @@ import ( "github.com/containers/image/v5/docker/reference" "github.com/containers/image/v5/internal/iolimits" + "github.com/containers/image/v5/internal/multierr" "github.com/containers/image/v5/internal/set" "github.com/containers/image/v5/internal/useragent" "github.com/containers/image/v5/manifest" @@ -1011,11 +1012,7 @@ func (c *dockerClient) getExternalBlob(ctx context.Context, urls []string) (io.R if remoteErrors == nil { return nil, 0, nil // fallback to non-external blob } - err := fmt.Errorf("failed fetching external blob from all urls: %w", remoteErrors[0]) - for _, e := range remoteErrors[1:] { - err = fmt.Errorf("%s, %w", err, e) - } - return nil, 0, err + return nil, 0, fmt.Errorf("failed fetching external blob from all urls: %w", multierr.Format("", ", ", "", remoteErrors)) } func getBlobSize(resp *http.Response) int64 { diff --git a/go.mod b/go.mod index de22df636c..1410c4cd7d 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,6 @@ require ( github.com/docker/go-connections v0.5.0 github.com/go-openapi/strfmt v0.23.0 github.com/go-openapi/swag v0.23.0 - github.com/hashicorp/go-multierror v1.1.1 github.com/hashicorp/go-retryablehttp v0.7.5 github.com/klauspost/compress v1.17.7 github.com/klauspost/pgzip v1.2.6 @@ -91,6 +90,7 @@ require ( github.com/hashicorp/errwrap v1.1.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect github.com/hashicorp/go-hclog v1.2.0 // indirect + github.com/hashicorp/go-multierror v1.1.1 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/letsencrypt/boulder v0.0.0-20230907030200-6d76a0f91e1e // indirect diff --git a/internal/multierr/multierr.go b/internal/multierr/multierr.go new file mode 100644 index 0000000000..1341925c1d --- /dev/null +++ b/internal/multierr/multierr.go @@ -0,0 +1,34 @@ +package multierr + +import ( + "fmt" + "strings" +) + +// Format creates an error value from the input array (which should not be empty) +// If the input contains a single error value, it is returned as is. +// If there are multiple, they are formatted as a multi-error (with Unwrap() []error) with the provided initial, separator, and ending strings. +// +// Typical usage: +// +// var errs []error +// // … +// errs = append(errs, …) +// // … +// if errs != nil { return multierr.Format("Failures doing $FOO", "\n* ", "", errs)} +func Format(first, middle, last string, errs []error) error { + switch len(errs) { + case 0: + return fmt.Errorf("internal error: multierr.Format called with 0 errors") + case 1: + return errs[0] + default: + // We have to do this — and this function only really exists — because fmt.Errorf(format, errs...) is invalid: + // []error is not a valid parameter to a function expecting []any + anyErrs := make([]any, 0, len(errs)) + for _, e := range errs { + anyErrs = append(anyErrs, e) + } + return fmt.Errorf(first+"%w"+strings.Repeat(middle+"%w", len(errs)-1)+last, anyErrs...) + } +} diff --git a/internal/multierr/multierr_test.go b/internal/multierr/multierr_test.go new file mode 100644 index 0000000000..085be5dc16 --- /dev/null +++ b/internal/multierr/multierr_test.go @@ -0,0 +1,39 @@ +package multierr + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +type errA struct{} + +func (errA) Error() string { return "A" } + +func TestFormat(t *testing.T) { + errB := errors.New("B") + + // Single-item Format preserves the error type + res := Format("[", ",", "]", []error{errA{}}) + assert.Equal(t, "A", res.Error()) + var aTarget errA + assert.ErrorAs(t, res, &aTarget) + + // Single-item Format preserves the error identity + res = Format("[", ",", "]", []error{errB}) + assert.Equal(t, "B", res.Error()) + assert.ErrorIs(t, res, errB) + + // Multi-item Format preserves both + res = Format("[", ",", "]", []error{errA{}, errB}) + assert.Equal(t, "[A,B]", res.Error()) + assert.ErrorAs(t, res, &aTarget) + assert.ErrorIs(t, res, errB) + + // This is invalid, but make sure we don’t misleadingly suceeed + res = Format("[", ",", "]", []error{}) + assert.Error(t, res) + res = Format("[", ",", "]", nil) + assert.Error(t, res) +} diff --git a/openshift/openshift-copies.go b/openshift/openshift-copies.go index c6498f6ca5..5cce06806f 100644 --- a/openshift/openshift-copies.go +++ b/openshift/openshift-copies.go @@ -18,6 +18,7 @@ import ( "time" "dario.cat/mergo" + "github.com/containers/image/v5/internal/multierr" "github.com/containers/storage/pkg/homedir" "github.com/sirupsen/logrus" "golang.org/x/exp/slices" @@ -459,12 +460,6 @@ func (config *directClientConfig) getCluster() clientcmdCluster { return mergedClusterInfo } -// aggregateErr is a modified copy of k8s.io/apimachinery/pkg/util/errors.aggregate. -// This helper implements the error and Errors interfaces. Keeping it private -// prevents people from making an aggregate of 0 errors, which is not -// an error, but does satisfy the error interface. -type aggregateErr []error - // newAggregate is a modified copy of k8s.io/apimachinery/pkg/util/errors.NewAggregate. // NewAggregate converts a slice of errors into an Aggregate interface, which // is itself an implementation of the error interface. If the slice is empty, @@ -485,29 +480,9 @@ func newAggregate(errlist []error) error { if len(errs) == 0 { return nil } - return aggregateErr(errs) + return multierr.Format("[", ", ", "]", errs) } -// Error is a modified copy of k8s.io/apimachinery/pkg/util/errors.aggregate.Error. -// Error is part of the error interface. -func (agg aggregateErr) Error() string { - if len(agg) == 0 { - // This should never happen, really. - return "" - } - 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()) - } - result += "]" - return result -} - -// REMOVED: aggregateErr.Errors - // errConfigurationInvalid is a modified? copy of k8s.io/kubernetes/pkg/client/unversioned/clientcmd.errConfigurationInvalid. // errConfigurationInvalid is a set of errors indicating the configuration is invalid. type errConfigurationInvalid []error diff --git a/pkg/docker/config/config.go b/pkg/docker/config/config.go index c61065cb01..e62286ae06 100644 --- a/pkg/docker/config/config.go +++ b/pkg/docker/config/config.go @@ -13,6 +13,7 @@ import ( "strings" "github.com/containers/image/v5/docker/reference" + "github.com/containers/image/v5/internal/multierr" "github.com/containers/image/v5/internal/set" "github.com/containers/image/v5/pkg/sysregistriesv2" "github.com/containers/image/v5/types" @@ -20,7 +21,6 @@ import ( "github.com/containers/storage/pkg/ioutils" helperclient "github.com/docker/docker-credential-helpers/client" "github.com/docker/docker-credential-helpers/credentials" - "github.com/hashicorp/go-multierror" "github.com/sirupsen/logrus" ) @@ -231,7 +231,7 @@ func getCredentialsWithHomeDir(sys *types.SystemContext, key, homeDir string) (t return types.DockerAuthConfig{}, err } - var multiErr error + var multiErr []error for _, helper := range helpers { var ( creds types.DockerAuthConfig @@ -253,7 +253,7 @@ func getCredentialsWithHomeDir(sys *types.SystemContext, key, homeDir string) (t } if err != nil { logrus.Debugf("Error looking up credentials for %s in credential helper %s: %v", helperKey, helper, err) - multiErr = multierror.Append(multiErr, err) + multiErr = append(multiErr, err) continue } if creds != (types.DockerAuthConfig{}) { @@ -266,7 +266,7 @@ func getCredentialsWithHomeDir(sys *types.SystemContext, key, homeDir string) (t } } if multiErr != nil { - return types.DockerAuthConfig{}, multiErr + return types.DockerAuthConfig{}, multierr.Format("errors looking up credentials:\n\t* ", "\nt* ", "\n", multiErr) } logrus.Debugf("No credentials for %s found", key) @@ -313,7 +313,7 @@ func SetCredentials(sys *types.SystemContext, key, username, password string) (s } // Make sure to collect all errors. - var multiErr error + var multiErr []error for _, helper := range helpers { var desc string var err error @@ -345,14 +345,14 @@ func SetCredentials(sys *types.SystemContext, key, username, password string) (s } } if err != nil { - multiErr = multierror.Append(multiErr, err) + multiErr = append(multiErr, err) logrus.Debugf("Error storing credentials for %s in credential helper %s: %v", key, helper, err) continue } logrus.Debugf("Stored credentials for %s in credential helper %s", key, helper) return desc, nil } - return "", multiErr + return "", multierr.Format("Errors storing credentials\n\t* ", "\n\t* ", "\n", multiErr) } func unsupportedNamespaceErr(helper string) error { @@ -376,53 +376,56 @@ func RemoveAuthentication(sys *types.SystemContext, key string) error { return err } - var multiErr error isLoggedIn := false - removeFromCredHelper := func(helper string) { + removeFromCredHelper := func(helper string) error { if isNamespaced { logrus.Debugf("Not removing credentials because namespaced keys are not supported for the credential helper: %s", helper) - return + return nil } err := deleteCredsFromCredHelper(helper, key) if err == nil { logrus.Debugf("Credentials for %q were deleted from credential helper %s", key, helper) isLoggedIn = true - return + return nil } if credentials.IsErrCredentialsNotFoundMessage(err.Error()) { logrus.Debugf("Not logged in to %s with credential helper %s", key, helper) - return + return nil } - multiErr = multierror.Append(multiErr, fmt.Errorf("removing credentials for %s from credential helper %s: %w", key, helper, err)) + return fmt.Errorf("removing credentials for %s from credential helper %s: %w", key, helper, err) } + var multiErr []error for _, helper := range helpers { var err error switch helper { // Special-case the built-in helper for auth files. case sysregistriesv2.AuthenticationFileHelper: _, err = jsonEditor(sys, func(fileContents *dockerConfigFile) (bool, string, error) { + var helperErr error if innerHelper, exists := fileContents.CredHelpers[key]; exists { - removeFromCredHelper(innerHelper) + helperErr = removeFromCredHelper(innerHelper) } if _, ok := fileContents.AuthConfigs[key]; ok { isLoggedIn = true delete(fileContents.AuthConfigs, key) } - return true, "", multiErr + return true, "", helperErr }) if err != nil { - multiErr = multierror.Append(multiErr, err) + multiErr = append(multiErr, err) } // External helpers. default: - removeFromCredHelper(helper) + if err := removeFromCredHelper(helper); err != nil { + multiErr = append(multiErr, err) + } } } if multiErr != nil { - return multiErr + return multierr.Format("errors removing credentials\n\t* ", "\n\t*", "\n", multiErr) } if !isLoggedIn { return ErrNotLoggedIn @@ -439,7 +442,7 @@ func RemoveAllAuthentication(sys *types.SystemContext) error { return err } - var multiErr error + var multiErr []error for _, helper := range helpers { var err error switch helper { @@ -479,13 +482,16 @@ func RemoveAllAuthentication(sys *types.SystemContext) error { } if err != nil { logrus.Debugf("Error removing credentials from credential helper %s: %v", helper, err) - multiErr = multierror.Append(multiErr, err) + multiErr = append(multiErr, err) continue } logrus.Debugf("All credentials removed from credential helper %s", helper) } - return multiErr + if multiErr != nil { + return multierr.Format("errors removing all credentials:\n\t* ", "\n\t* ", "\n", multiErr) + } + return nil } // prepareForEdit processes sys and key (if keyRelevant) to return: diff --git a/pkg/shortnames/shortnames.go b/pkg/shortnames/shortnames.go index a15b2b56e1..17e30bc7b9 100644 --- a/pkg/shortnames/shortnames.go +++ b/pkg/shortnames/shortnames.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/containers/image/v5/docker/reference" + "github.com/containers/image/v5/internal/multierr" "github.com/containers/image/v5/pkg/sysregistriesv2" "github.com/containers/image/v5/types" "github.com/manifoldco/promptui" @@ -169,26 +170,17 @@ func (r *Resolved) Description() string { // Note that nil is returned if len(pullErrors) == 0. Otherwise, the amount of // pull errors must equal the amount of pull candidates. func (r *Resolved) FormatPullErrors(pullErrors []error) error { - if len(pullErrors) > 0 && len(pullErrors) != len(r.PullCandidates) { + if len(pullErrors) == 0 { + return nil + } + + if len(pullErrors) != len(r.PullCandidates) { pullErrors = append(slices.Clone(pullErrors), fmt.Errorf("internal error: expected %d instead of %d errors for %d pull candidates", len(r.PullCandidates), len(pullErrors), len(r.PullCandidates))) } - switch len(pullErrors) { - case 0: - return nil - case 1: - return pullErrors[0] - default: - var sb strings.Builder - sb.WriteString(fmt.Sprintf("%d errors occurred while pulling:", len(pullErrors))) - for _, e := range pullErrors { - sb.WriteString("\n * ") - sb.WriteString(e.Error()) - } - return errors.New(sb.String()) - } + return multierr.Format(fmt.Sprintf("%d errors occurred while pulling:\n * ", len(pullErrors)), "\n * ", "", pullErrors) } // PullCandidate is a resolved name. Once the Value has been used diff --git a/pkg/sysregistriesv2/shortnames.go b/pkg/sysregistriesv2/shortnames.go index 3a11542c62..13f6dfd7fd 100644 --- a/pkg/sysregistriesv2/shortnames.go +++ b/pkg/sysregistriesv2/shortnames.go @@ -9,6 +9,7 @@ import ( "github.com/BurntSushi/toml" "github.com/containers/image/v5/docker/reference" + "github.com/containers/image/v5/internal/multierr" "github.com/containers/image/v5/internal/rootless" "github.com/containers/image/v5/types" "github.com/containers/storage/pkg/homedir" @@ -297,11 +298,7 @@ func newShortNameAliasCache(path string, conf *shortNameAliasConf) (*shortNameAl } } if len(errs) > 0 { - err := errs[0] - for i := 1; i < len(errs); i++ { - err = fmt.Errorf("%v\n: %w", errs[i], err) - } - return nil, err + return nil, multierr.Format("", "\n", "", errs) } return &res, nil } diff --git a/signature/policy_eval_signedby.go b/signature/policy_eval_signedby.go index a4187735b7..5688678745 100644 --- a/signature/policy_eval_signedby.go +++ b/signature/policy_eval_signedby.go @@ -7,8 +7,8 @@ import ( "errors" "fmt" "os" - "strings" + "github.com/containers/image/v5/internal/multierr" "github.com/containers/image/v5/internal/private" "github.com/containers/image/v5/manifest" digest "github.com/opencontainers/go-digest" @@ -134,12 +134,7 @@ func (pr *prSignedBy) isRunningImageAllowed(ctx context.Context, image private.U case 1: summary = rejections[0] default: - var msgs []string - for _, e := range rejections { - msgs = append(msgs, e.Error()) - } - summary = PolicyRequirementError(fmt.Sprintf("None of the signatures were accepted, reasons: %s", - strings.Join(msgs, "; "))) + summary = PolicyRequirementError(multierr.Format("None of the signatures were accepted, reasons: ", "; ", "", rejections).Error()) } return false, summary } diff --git a/signature/policy_eval_sigstore.go b/signature/policy_eval_sigstore.go index dcf5592a8e..e877161fcd 100644 --- a/signature/policy_eval_sigstore.go +++ b/signature/policy_eval_sigstore.go @@ -10,8 +10,8 @@ import ( "errors" "fmt" "os" - "strings" + "github.com/containers/image/v5/internal/multierr" "github.com/containers/image/v5/internal/private" "github.com/containers/image/v5/internal/signature" "github.com/containers/image/v5/manifest" @@ -270,12 +270,7 @@ func (pr *prSigstoreSigned) isRunningImageAllowed(ctx context.Context, image pri case 1: summary = rejections[0] default: - var msgs []string - for _, e := range rejections { - msgs = append(msgs, e.Error()) - } - summary = PolicyRequirementError(fmt.Sprintf("None of the signatures were accepted, reasons: %s", - strings.Join(msgs, "; "))) + summary = PolicyRequirementError(multierr.Format("None of the signatures were accepted, reasons: ", "; ", "", rejections).Error()) } return false, summary }