Skip to content

fix: pull should return 404 when image not exists #2914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 27, 2019
Merged

fix: pull should return 404 when image not exists #2914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion apis/server/image_bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
Copy link
Contributor

@fuweid fuweid Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well. if we write something into response, the http code will be 200 which can never be changed.
so we use jsonstream to return error. the client needs to parse the json stream to get error.

Copy link
Contributor

@fuweid fuweid Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you pull the No. 5 layer and get something wrong like connection refuse, your change is not helpful. it always be 200, not 500

}
metrics.ImageSuccessActionsCounter.WithLabelValues(label).Inc()
return nil
Expand Down
24 changes: 14 additions & 10 deletions ctrd/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Copy link
Contributor

@fuweid fuweid Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check the status and make sure that it is 404

Copy link
Contributor Author

@ZYecho ZYecho Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when resolveImage return err, means that it can't find available ref to use.

}

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,
Expand Down
10 changes: 7 additions & 3 deletions ctrd/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions ctrd/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package ctrd
import (
"context"
"crypto/tls"
"fmt"
"net"
"net/http"
"net/url"
Expand Down Expand Up @@ -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
Copy link
Contributor

@zhuangqh zhuangqh Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer errors.Wrapf(errtypes.ErrNotfound, "there is no available image reference after trying %+q", refs) here
util function just directly return error, log or not should be decided by upper.

Copy link
Contributor Author

@ZYecho ZYecho Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I change to logrus.warnf, add an another log in upperand. But I think errtypes.ErrNotfound is enough here, user didn't care about how you find a available ref, just tell them not found.

Copy link
Contributor

@zhuangqh zhuangqh Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense

}

refToName := map[string]string{
Expand Down
9 changes: 8 additions & 1 deletion daemon/mgr/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions test/api_image_create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
8 changes: 4 additions & 4 deletions test/z_cli_daemon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down