manifest: rework error handling

This patch reworks and updates the error handling in the docker manifest
command to be more inline with modern go principles.

Specifically, we use the new errors.Is function, along with errors.Wrap,
to preserve error hierarchy instead of using manual string operations.
This allows us to simplify various testing components, as well as
simplifying the implementation of manifest merging later.

Signed-off-by: Justin Chadwell <me@jedevc.com>

Author: jedevc

cli/command/manifest/annotate.go

@@ -5,7 +5,7 @@ import (
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
-	"github.com/docker/cli/cli/manifest/store"
+	"github.com/docker/cli/cli/manifest/types"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -60,7 +60,7 @@ func runManifestAnnotate(dockerCli command.Cli, opts annotateOptions) error {
 	manifestStore := dockerCli.ManifestStore()
 	imageManifest, err := manifestStore.Get(targetRef, imgRef)
 	switch {
-	case store.IsNotFound(err):
+	case errors.Is(err, types.ErrManifestNotFound):
 		return fmt.Errorf("manifest for image %s does not exist in %s", opts.image, opts.target)
 	case err != nil:
 		return err

cli/command/manifest/create_list.go

@@ -6,7 +6,7 @@ import (
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
-	"github.com/docker/cli/cli/manifest/store"
+	"github.com/docker/cli/cli/manifest/types"
 	"github.com/docker/docker/registry"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -50,7 +50,7 @@ func createManifestList(dockerCli command.Cli, args []string, opts createOpts) e
 	manifestStore := dockerCli.ManifestStore()
 	_, err = manifestStore.GetList(targetRef)
 	switch {
-	case store.IsNotFound(err):
+	case errors.Is(err, types.ErrManifestNotFound):
 		// New manifest list
 	case err != nil:
 		return err

cli/command/manifest/create_test.go

@@ -99,16 +99,16 @@ func TestManifestCreateNoManifest(t *testing.T) {
 	cli.SetManifestStore(store)
 	cli.SetRegistryClient(&fakeRegistryClient{
 		getManifestFunc: func(_ context.Context, ref reference.Named) (manifesttypes.ImageManifest, error) {
-			return manifesttypes.ImageManifest{}, errors.Errorf("No such image: %v", ref)
+			return manifesttypes.ImageManifest{}, errors.Wrapf(manifesttypes.ErrManifestNotFound, "%q does not exist", ref)
 		},
 		getManifestListFunc: func(ctx context.Context, ref reference.Named) ([]manifesttypes.ImageManifest, error) {
-			return nil, errors.Errorf("No such manifest: %s", ref)
+			return nil, errors.Wrapf(manifesttypes.ErrManifestNotFound, "%q does not exist", ref)
 		},
 	})
 
 	cmd := newCreateListCommand(cli)
 	cmd.SetArgs([]string{"example.com/list:v1", "example.com/alpine:3.0"})
 	cmd.SetOut(io.Discard)
 	err := cmd.Execute()
-	assert.Error(t, err, "No such image: example.com/alpine:3.0")
+	assert.Error(t, err, `"example.com/alpine:3.0" does not exist: manifest not found`)
 }

cli/command/manifest/inspect_test.go

@@ -70,7 +70,7 @@ func TestInspectCommandLocalManifestNotFound(t *testing.T) {
 	cmd.SetOut(io.Discard)
 	cmd.SetArgs([]string{"example.com/list:v1", "example.com/alpine:3.0"})
 	err := cmd.Execute()
-	assert.Error(t, err, "No such manifest: example.com/alpine:3.0")
+	assert.Error(t, err, `"example.com/alpine:3.0" does not exist: manifest not found`)
 }
 
 func TestInspectCommandNotFound(t *testing.T) {
@@ -79,19 +79,19 @@ func TestInspectCommandNotFound(t *testing.T) {
 	cli := test.NewFakeCli(nil)
 	cli.SetManifestStore(store)
 	cli.SetRegistryClient(&fakeRegistryClient{
-		getManifestFunc: func(_ context.Context, _ reference.Named) (types.ImageManifest, error) {
-			return types.ImageManifest{}, errors.New("missing")
+		getManifestFunc: func(ctx context.Context, ref reference.Named) (types.ImageManifest, error) {
+			return types.ImageManifest{}, errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", ref)
 		},
 		getManifestListFunc: func(ctx context.Context, ref reference.Named) ([]types.ImageManifest, error) {
-			return nil, errors.Errorf("No such manifest: %s", ref)
+			return nil, errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", ref)
 		},
 	})
 
 	cmd := newInspectCommand(cli)
 	cmd.SetOut(io.Discard)
 	cmd.SetArgs([]string{"example.com/alpine:3.0"})
 	err := cmd.Execute()
-	assert.Error(t, err, "No such manifest: example.com/alpine:3.0")
+	assert.Error(t, err, `"example.com/alpine:3.0" does not exist: manifest not found`)
 }
 
 func TestInspectCommandLocalManifest(t *testing.T) {

cli/command/manifest/rm_test.go

@@ -37,8 +37,8 @@ func TestRmSeveralManifests(t *testing.T) {
 
 	_, search1 := cli.ManifestStore().GetList(list1)
 	_, search2 := cli.ManifestStore().GetList(list2)
-	assert.Error(t, search1, "No such manifest: example.com/first:1")
-	assert.Error(t, search2, "No such manifest: example.com/second:2")
+	assert.Error(t, search1, `"example.com/first:1" does not exist: manifest not found`)
+	assert.Error(t, search2, `"example.com/second:2" does not exist: manifest not found`)
 }
 
 // attempt to remove a manifest list which was never created
@@ -57,8 +57,8 @@ func TestRmManifestNotCreated(t *testing.T) {
 	cmd.SetArgs([]string{"example.com/first:1", "example.com/second:2"})
 	cmd.SetOut(io.Discard)
 	err = cmd.Execute()
-	assert.Error(t, err, "No such manifest: example.com/first:1")
+	assert.Error(t, err, `"example.com/first:1" does not exist: manifest not found`)
 
 	_, err = cli.ManifestStore().GetList(list2)
-	assert.Error(t, err, "No such manifest: example.com/second:2")
+	assert.Error(t, err, `"example.com/second:2" does not exist: manifest not found`)
 }

cli/command/manifest/util.go

@@ -4,9 +4,9 @@ import (
 	"context"
 
 	"github.com/docker/cli/cli/command"
-	"github.com/docker/cli/cli/manifest/store"
 	"github.com/docker/cli/cli/manifest/types"
 	"github.com/docker/distribution/reference"
+	"github.com/pkg/errors"
 )
 
 type osArch struct {
@@ -70,15 +70,26 @@ func normalizeReference(ref string) (reference.Named, error) {
 // getManifest from the local store, and fallback to the remote registry if it
 // doesn't exist locally
 func getManifest(ctx context.Context, dockerCli command.Cli, listRef, namedRef reference.Named, insecure bool) (types.ImageManifest, error) {
-	data, err := dockerCli.ManifestStore().Get(listRef, namedRef)
-	switch {
-	case store.IsNotFound(err):
-		return dockerCli.RegistryClient(insecure).GetManifest(ctx, namedRef)
-	case err != nil:
-		return types.ImageManifest{}, err
-	case len(data.Raw) == 0:
-		return dockerCli.RegistryClient(insecure).GetManifest(ctx, namedRef)
-	default:
-		return data, nil
+	// load from the local store
+	if listRef != nil {
+		data, err := dockerCli.ManifestStore().Get(listRef, namedRef)
+		if err == nil {
+			return data, nil
+		} else if !errors.Is(err, types.ErrManifestNotFound) {
+			return types.ImageManifest{}, err
+		}
 	}
+
+	// load from the remote registry
+	client := dockerCli.RegistryClient(insecure)
+	if client != nil {
+		data, err := client.GetManifest(ctx, namedRef)
+		if err == nil {
+			return data, nil
+		} else if !errors.Is(err, types.ErrManifestNotFound) {
+			return types.ImageManifest{}, err
+		}
+	}
+
+	return types.ImageManifest{}, errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", namedRef)
 }

cli/manifest/store/store.go

@@ -2,7 +2,6 @@ package store
 
 import (
 	"encoding/json"
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -49,7 +48,7 @@ func (s *fsStore) getFromFilename(ref reference.Reference, filename string) (typ
 	bytes, err := os.ReadFile(filename)
 	switch {
 	case os.IsNotExist(err):
-		return types.ImageManifest{}, newNotFoundError(ref.String())
+		return types.ImageManifest{}, errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", ref.String())
 	case err != nil:
 		return types.ImageManifest{}, err
 	}
@@ -93,7 +92,7 @@ func (s *fsStore) GetList(listRef reference.Reference) ([]types.ImageManifest, e
 	case err != nil:
 		return nil, err
 	case filenames == nil:
-		return nil, newNotFoundError(listRef.String())
+		return nil, errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", listRef.String())
 	}
 
 	manifests := []types.ImageManifest{}
@@ -152,28 +151,3 @@ func makeFilesafeName(ref string) string {
 	fileName := strings.Replace(ref, ":", "-", -1)
 	return strings.Replace(fileName, "/", "_", -1)
 }
-
-type notFoundError struct {
-	object string
-}
-
-func newNotFoundError(ref string) *notFoundError {
-	return &notFoundError{object: ref}
-}
-
-func (n *notFoundError) Error() string {
-	return fmt.Sprintf("No such manifest: %s", n.object)
-}
-
-// NotFound interface
-func (n *notFoundError) NotFound() {}
-
-// IsNotFound returns true if the error is a not found error
-func IsNotFound(err error) bool {
-	_, ok := err.(notFound)
-	return ok
-}
-
-type notFound interface {
-	NotFound()
-}

cli/manifest/store/store_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/docker/cli/cli/manifest/types"
 	"github.com/docker/distribution/reference"
 	"github.com/google/go-cmp/cmp"
+	"github.com/pkg/errors"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 )
@@ -62,7 +63,7 @@ func TestStoreSaveAndGet(t *testing.T) {
 		listRef     reference.Reference
 		manifestRef reference.Reference
 		expected    types.ImageManifest
-		expectedErr string
+		expectedErr error
 	}{
 		{
 			listRef:     listRef,
@@ -72,22 +73,21 @@ func TestStoreSaveAndGet(t *testing.T) {
 		{
 			listRef:     listRef,
 			manifestRef: ref("exist:does-not"),
-			expectedErr: "No such manifest: exist:does-not",
+			expectedErr: types.ErrManifestNotFound,
 		},
 		{
 			listRef:     ref("list:does-not-exist"),
 			manifestRef: ref("manifest:does-not-exist"),
-			expectedErr: "No such manifest: manifest:does-not-exist",
+			expectedErr: types.ErrManifestNotFound,
 		},
 	}
 
 	for _, testcase := range testcases {
 		testcase := testcase
 		t.Run(testcase.manifestRef.String(), func(t *testing.T) {
 			actual, err := store.Get(testcase.listRef, testcase.manifestRef)
-			if testcase.expectedErr != "" {
-				assert.Error(t, err, testcase.expectedErr)
-				assert.Check(t, IsNotFound(err))
+			if testcase.expectedErr != nil {
+				assert.Check(t, errors.Is(err, types.ErrManifestNotFound))
 				return
 			}
 			assert.NilError(t, err)
@@ -117,6 +117,5 @@ func TestStoreGetListDoesNotExist(t *testing.T) {
 	store := NewStore(t.TempDir())
 	listRef := ref("list")
 	_, err := store.GetList(listRef)
-	assert.Error(t, err, "No such manifest: list")
-	assert.Check(t, IsNotFound(err))
+	assert.Check(t, errors.Is(err, types.ErrManifestNotFound))
 }

cli/manifest/types/types.go

@@ -13,6 +13,16 @@ import (
 	"github.com/pkg/errors"
 )
 
+var (
+	// ErrManifestNotFound is an error returned when a requested manifest or
+	// manifest list cannot be found in the store or the registry
+	ErrManifestNotFound = errors.New("manifest not found")
+
+	// ErrUnknownType is an error returned when a manifest was found, but it
+	// was of an unrecognized type
+	ErrUnknownType = errors.New("unknown manifest type")
+)
+
 // ImageManifest contains info to output for a manifest object.
 type ImageManifest struct {
 	Ref        *SerializableNamed

cli/registry/client/fetcher.go

@@ -43,9 +43,9 @@ func fetchManifest(ctx context.Context, repo distribution.Repository, ref refere
 		}
 		return imageManifest, nil
 	case *manifestlist.DeserializedManifestList:
-		return types.ImageManifest{}, errors.Errorf("%s is a manifest list", ref)
+		return types.ImageManifest{}, errors.Wrapf(types.ErrManifestNotFound, "%q is a manifest list", ref)
 	}
-	return types.ImageManifest{}, errors.Errorf("%s is not a manifest", ref)
+	return types.ImageManifest{}, errors.Wrapf(types.ErrUnknownType, "%q does not exist", ref)
 }
 
 func fetchList(ctx context.Context, repo distribution.Repository, ref reference.Named) ([]types.ImageManifest, error) {
@@ -55,14 +55,16 @@ func fetchList(ctx context.Context, repo distribution.Repository, ref reference.
 	}
 
 	switch v := manifest.(type) {
+	case *schema2.DeserializedManifest, *ocischema.DeserializedManifest:
+		return nil, errors.Wrapf(types.ErrManifestNotFound, "%q is not a manifest list", ref)
 	case *manifestlist.DeserializedManifestList:
 		imageManifests, err := pullManifestList(ctx, ref, repo, *v)
 		if err != nil {
 			return nil, err
 		}
 		return imageManifests, nil
 	default:
-		return nil, errors.Errorf("unsupported manifest format: %v", v)
+		return nil, types.ErrUnknownType
 	}
 }
 
@@ -74,7 +76,7 @@ func getManifest(ctx context.Context, repo distribution.Repository, ref referenc
 
 	dgst, opts, err := getManifestOptionsFromReference(ref)
 	if err != nil {
-		return nil, errors.Errorf("image manifest for %q does not exist", ref)
+		return nil, errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", ref)
 	}
 	return manSvc.Get(ctx, dgst, opts...)
 }
@@ -195,7 +197,7 @@ func pullManifestList(ctx context.Context, ref reference.Named, repo distributio
 		case *ocischema.DeserializedManifest:
 			imageManifest, err = pullManifestOCISchema(ctx, manifestRef, repo, *v)
 		default:
-			err = errors.Errorf("unsupported manifest type: %T", manifest)
+			err = types.ErrUnknownType
 		}
 		if err != nil {
 			return nil, err
@@ -287,7 +289,7 @@ func (c *client) iterateEndpoints(ctx context.Context, namedRef reference.Named,
 			return nil
 		}
 	}
-	return newNotFoundError(namedRef.String())
+	return errors.Wrapf(types.ErrManifestNotFound, "%q does not exist", namedRef)
 }
 
 // allEndpoints returns a list of endpoints ordered by priority (v2, https, v1).
@@ -310,18 +312,3 @@ func allEndpoints(namedRef reference.Named, insecure bool) ([]registry.APIEndpoi
 	logrus.Debugf("endpoints for %s: %v", namedRef, endpoints)
 	return endpoints, err
 }
-
-func newNotFoundError(ref string) *notFoundError {
-	return &notFoundError{err: errors.New("no such manifest: " + ref)}
-}
-
-type notFoundError struct {
-	err error
-}
-
-func (n *notFoundError) Error() string {
-	return n.err.Error()
-}
-
-// NotFound satisfies interface github.com/docker/docker/errdefs.ErrNotFound
-func (n *notFoundError) NotFound() {}