From 21a279d352d079bb8852db617945a1a636296536 Mon Sep 17 00:00:00 2001 From: zhangyue Date: Tue, 18 Jun 2019 22:32:00 +0800 Subject: [PATCH] fix: pull should return 404 when image not exists Signed-off-by: zhangyue --- apis/server/image_bridge.go | 6 +++++- ctrd/image.go | 24 ++++++++++++++---------- ctrd/interface.go | 10 +++++++--- ctrd/utils.go | 4 ++-- daemon/mgr/image.go | 9 ++++++++- test/api_image_create_test.go | 13 +++++++++++++ test/z_cli_daemon_test.go | 8 ++++---- 7 files changed, 53 insertions(+), 21 deletions(-) diff --git a/apis/server/image_bridge.go b/apis/server/image_bridge.go index bf9b6b29e..ce4529f2a 100644 --- a/apis/server/image_bridge.go +++ b/apis/server/image_bridge.go @@ -14,6 +14,7 @@ import ( "github.com/alibaba/pouch/apis/metrics" "github.com/alibaba/pouch/apis/types" "github.com/alibaba/pouch/daemon/mgr" + "github.com/alibaba/pouch/pkg/errtypes" "github.com/alibaba/pouch/pkg/httputils" util_metrics "github.com/alibaba/pouch/pkg/utils/metrics" @@ -58,7 +59,10 @@ func (s *Server) pullImage(ctx context.Context, rw http.ResponseWriter, req *htt // Error information has be sent to client, so no need call resp.Write if err := s.ImageMgr.PullImage(ctx, image, &authConfig, newWriteFlusher(rw)); err != nil { logrus.Errorf("failed to pull image %s: %v", image, err) - return nil + if err == errtypes.ErrNotfound { + return httputils.NewHTTPError(err, http.StatusNotFound) + } + return err } metrics.ImageSuccessActionsCounter.WithLabelValues(label).Inc() return nil diff --git a/ctrd/image.go b/ctrd/image.go index 36031fe60..11f445b59 100644 --- a/ctrd/image.go +++ b/ctrd/image.go @@ -200,7 +200,7 @@ func (c *Client) PushImage(ctx context.Context, ref string, authConfig *types.Au pushTracker := docker.NewInMemoryTracker() - resolver, _, err := c.getResolver(ctx, authConfig, ref, []string{ref}, docker.ResolverOptions{ + resolver, _, err := c.ResolveImage(ctx, ref, []string{ref}, authConfig, docker.ResolverOptions{ Tracker: pushTracker, }) if err != nil { @@ -251,21 +251,25 @@ func (c *Client) PushImage(ctx context.Context, ref string, authConfig *types.Au return nil } -// FetchImage fetches image content from the remote repository. -func (c *Client) FetchImage(ctx context.Context, name string, refs []string, authConfig *types.AuthConfig, stream *jsonstream.JSONStream) (containerd.Image, error) { - wrapperCli, err := c.Get(ctx) +// ResolveImage attempts to resolve the image reference into a available reference and resolver. +func (c *Client) ResolveImage(ctx context.Context, nameRef string, refs []string, authConfig *types.AuthConfig, opts docker.ResolverOptions) (remotes.Resolver, string, error) { + resolver, availableRef, err := c.getResolver(ctx, authConfig, nameRef, refs, opts) if err != nil { - return nil, fmt.Errorf("failed to get a containerd grpc client: %v", err) + logrus.Errorf("image ref not found %s", nameRef) + return nil, "", err } - resolver, availableRef, err := c.getResolver(ctx, authConfig, name, refs, docker.ResolverOptions{}) + return resolver, availableRef, nil +} + +// FetchImage fetches image content from the remote repository. +func (c *Client) FetchImage(ctx context.Context, resolver remotes.Resolver, availableRef string, authConfig *types.AuthConfig, stream *jsonstream.JSONStream) (containerd.Image, error) { + wrapperCli, err := c.Get(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get a containerd grpc client: %v", err) } - logrus.Infof("pulling image name %v reference %v", name, availableRef) - - ongoing := newJobs(name) + ongoing := newJobs(availableRef) options := []containerd.RemoteOpt{ containerd.WithSchema1Conversion, diff --git a/ctrd/interface.go b/ctrd/interface.go index a37a7c574..40bc369bb 100644 --- a/ctrd/interface.go +++ b/ctrd/interface.go @@ -13,8 +13,10 @@ import ( containerdtypes "github.com/containerd/containerd/api/types" ctrdmetaimages "github.com/containerd/containerd/images" "github.com/containerd/containerd/mount" + "github.com/containerd/containerd/remotes" + "github.com/containerd/containerd/remotes/docker" "github.com/containerd/containerd/snapshots" - digest "github.com/opencontainers/go-digest" + "github.com/opencontainers/go-digest" ) // APIClient defines common methods of containerd api client @@ -77,8 +79,10 @@ type ImageAPIClient interface { GetImage(ctx context.Context, ref string) (containerd.Image, error) // ListImages returns the list of containerd.Image filtered by the given conditions. ListImages(ctx context.Context, filter ...string) ([]containerd.Image, error) - // FetchImage fetchs image content by the given reference. - FetchImage(ctx context.Context, name string, refs []string, authConfig *types.AuthConfig, stream *jsonstream.JSONStream) (containerd.Image, error) + // FetchImage fetches image content by the given reference. + FetchImage(ctx context.Context, resolver remotes.Resolver, ref string, authConfig *types.AuthConfig, stream *jsonstream.JSONStream) (containerd.Image, error) + // ResolveImage attempts to resolve the image reference into a available reference and resolver. + ResolveImage(ctx context.Context, nameRef string, refs []string, authConfig *types.AuthConfig, opts docker.ResolverOptions) (remotes.Resolver, string, error) // RemoveImage removes the image by the given reference. RemoveImage(ctx context.Context, ref string) error // ImportImage creates a set of images by tarstream. diff --git a/ctrd/utils.go b/ctrd/utils.go index ea94ad847..4526a3fbe 100644 --- a/ctrd/utils.go +++ b/ctrd/utils.go @@ -3,7 +3,6 @@ package ctrd import ( "context" "crypto/tls" - "fmt" "net" "net/http" "net/url" @@ -166,7 +165,8 @@ func (c *Client) getResolver(ctx context.Context, authConfig *types.AuthConfig, } if availableRef == "" { - return nil, "", fmt.Errorf("there is no available image reference after trying %+q", refs) + logrus.Warnf("there is no available image reference after trying %+q", refs) + return nil, "", errtypes.ErrNotfound } refToName := map[string]string{ diff --git a/daemon/mgr/image.go b/daemon/mgr/image.go index 648ff9bc3..127451de8 100644 --- a/daemon/mgr/image.go +++ b/daemon/mgr/image.go @@ -29,6 +29,7 @@ import ( "github.com/containerd/containerd/content" ctrdmetaimages "github.com/containerd/containerd/images" "github.com/containerd/containerd/platforms" + "github.com/containerd/containerd/remotes/docker" "github.com/opencontainers/go-digest" ocispec "github.com/opencontainers/image-spec/specs-go/v1" pkgerrors "github.com/pkg/errors" @@ -217,7 +218,13 @@ func (mgr *ImageManager) PullImage(ctx context.Context, ref string, authConfig * fullRefs := mgr.LookupImageReferences(ref) namedRef = reference.TrimTagForDigest(reference.WithDefaultTagIfMissing(namedRef)) - img, err := mgr.client.FetchImage(pctx, namedRef.String(), fullRefs, authConfig, stream) + resolver, availableRef, err := mgr.client.ResolveImage(ctx, namedRef.String(), fullRefs, authConfig, docker.ResolverOptions{}) + if err != nil { + return err + } + logrus.Infof("pulling image name %v reference %v", namedRef.String(), availableRef) + + img, err := mgr.client.FetchImage(pctx, resolver, availableRef, authConfig, stream) if err != nil { writeStream(err) return err diff --git a/test/api_image_create_test.go b/test/api_image_create_test.go index 570d7b425..03d29dfad 100644 --- a/test/api_image_create_test.go +++ b/test/api_image_create_test.go @@ -47,6 +47,19 @@ func (suite *APIImageCreateSuite) TestImageCreateNil(c *check.C) { CheckRespStatus(c, resp, 400) } +// TestImageCreateNonExistentImage tests pulling a non-existent image. +func (suite *APIImageCreateSuite) TestImageCreateNonExistentImage(c *check.C) { + q := url.Values{} + image := "qwefghjm:zxcvbn_efgh_nonexist" + + q.Add("fromImage", image) + query := request.WithQuery(q) + + resp, err := request.Post("/images/create", query) + c.Assert(err, check.IsNil) + CheckRespStatus(c, resp, 404) +} + // TestImageCreateWithoutTag tests creating an image without tag, will use "latest" by default. func (suite *APIImageCreateSuite) TestImageCreateWithoutTag(c *check.C) { q := url.Values{} diff --git a/test/z_cli_daemon_test.go b/test/z_cli_daemon_test.go index 37b17d765..97fa85acc 100644 --- a/test/z_cli_daemon_test.go +++ b/test/z_cli_daemon_test.go @@ -367,14 +367,14 @@ func (suite *PouchDaemonSuite) TestDaemonLabelNeg(c *check.C) { func (suite *PouchDaemonSuite) TestDaemonDefaultRegistry(c *check.C) { dcfg, err := StartDefaultDaemonDebug( "--default-registry", - "reg.docker.alibaba-inc.com", + "registry.hub.docker.com", "--default-registry-namespace", - "base") + "library") c.Assert(err, check.IsNil) // Check pull image with default registry using the registry specified in daemon. - result := RunWithSpecifiedDaemon(dcfg, "pull", "hello-world") - err = util.PartialEqual(result.Combined(), "reg.docker.alibaba-inc.com/base/hello-world") + result := RunWithSpecifiedDaemon(dcfg, "pull", "nginx:latest") + err = util.PartialEqual(result.Combined(), "registry.hub.docker.com/library/nginx:latest") c.Assert(err, check.IsNil) defer dcfg.KillDaemon()