Refactor pkg/v1/daemon (#989)

Use options for everything, including injecting clients for testing.

Add WithContext option.

Un-thunkify some of daemon.Image to make it a bit clearer.

Add more test coverage: actually caught one bug where we were returning
the wrong error if we failed to read from the daemon response body.

pkg/v1/daemon/image.go

@@ -19,103 +19,71 @@ import (
 	"context"
 	"io"
 	"io/ioutil"
+	"sync"
 
-	"github.com/docker/docker/client"
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/tarball"
 )
 
-// image accesses an image from a docker daemon
-type image struct {
-	v1.Image
-}
-
-var _ v1.Image = (*image)(nil)
-
 type imageOpener struct {
-	ref      name.Reference
+	ref name.Reference
+	ctx context.Context
+
 	buffered bool
 	client   Client
-}
-
-// ImageOption is a functional option for Image.
-type ImageOption func(*imageOpener) error
-
-func (i *imageOpener) Open() (v1.Image, error) {
-	var opener tarball.Opener
-	var err error
-	if i.buffered {
-		opener, err = i.bufferedOpener(i.ref)
-	} else {
-		opener, err = i.unbufferedOpener(i.ref)
-	}
-	if err != nil {
-		return nil, err
-	}
 
-	tb, err := tarball.Image(opener, nil)
-	if err != nil {
-		return nil, err
-	}
-	img := &image{
-		Image: tb,
-	}
-	return img, nil
+	once  sync.Once
+	bytes []byte
+	err   error
 }
 
-func (i *imageOpener) saveImage(ref name.Reference) (io.ReadCloser, error) {
-	return i.client.ImageSave(context.Background(), []string{ref.Name()})
+func (i *imageOpener) saveImage() (io.ReadCloser, error) {
+	return i.client.ImageSave(i.ctx, []string{i.ref.Name()})
 }
 
-func (i *imageOpener) bufferedOpener(ref name.Reference) (tarball.Opener, error) {
+func (i *imageOpener) bufferedOpener() (io.ReadCloser, error) {
 	// Store the tarball in memory and return a new reader into the bytes each time we need to access something.
-	rc, err := i.saveImage(ref)
-	if err != nil {
-		return nil, err
-	}
-	defer rc.Close()
+	i.once.Do(func() {
+		i.bytes, i.err = func() ([]byte, error) {
+			rc, err := i.saveImage()
+			if err != nil {
+				return nil, err
+			}
+			defer rc.Close()
+
+			return ioutil.ReadAll(rc)
+		}()
+	})
+
+	// Wrap the bytes in a ReadCloser so it looks like an opened file.
+	return ioutil.NopCloser(bytes.NewReader(i.bytes)), i.err
+}
 
-	imageBytes, err := ioutil.ReadAll(rc)
-	if err != nil {
-		return nil, err
+func (i *imageOpener) opener() tarball.Opener {
+	if i.buffered {
+		return i.bufferedOpener
 	}
-	// The tarball interface takes a function that it can call to return an opened reader-like object.
-	// Daemon comes from a set of bytes, so wrap them in a ReadCloser so it looks like an opened file.
-	return func() (io.ReadCloser, error) {
-		return ioutil.NopCloser(bytes.NewReader(imageBytes)), nil
-	}, nil
-}
 
-func (i *imageOpener) unbufferedOpener(ref name.Reference) (tarball.Opener, error) {
 	// To avoid storing the tarball in memory, do a save every time we need to access something.
-	return func() (io.ReadCloser, error) {
-		return i.saveImage(ref)
-	}, nil
+	return i.saveImage
 }
 
 // Image provides access to an image reference from the Docker daemon,
 // applying functional options to the underlying imageOpener before
 // resolving the reference into a v1.Image.
-func Image(ref name.Reference, options ...ImageOption) (v1.Image, error) {
-	i := &imageOpener{
-		ref:      ref,
-		buffered: true, // buffer by default
-	}
-	for _, option := range options {
-		if err := option(i); err != nil {
-			return nil, err
-		}
+func Image(ref name.Reference, options ...Option) (v1.Image, error) {
+	o, err := makeOptions(options...)
+	if err != nil {
+		return nil, err
 	}
 
-	if i.client == nil {
-		var err error
-		i.client, err = client.NewClientWithOpts(client.FromEnv)
-		if err != nil {
-			return nil, err
-		}
+	i := &imageOpener{
+		ref:      ref,
+		buffered: o.buffered,
+		client:   o.client,
+		ctx:      o.ctx,
 	}
-	i.client.NegotiateAPIVersion(context.Background())
 
-	return i.Open()
+	return tarball.Image(i.opener(), nil)
 }

pkg/v1/daemon/image_test.go

@@ -17,8 +17,11 @@ package daemon
 import (
 	"context"
 	"errors"
+	"fmt"
 	"io"
+	"io/ioutil"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/google/go-containerregistry/internal/compare"
@@ -29,48 +32,109 @@ import (
 
 var imagePath = "../tarball/testdata/test_image_1.tar"
 
-type MockImageSaver struct {
+type MockClient struct {
 	Client
 	path       string
 	negotiated bool
+
+	wantCtx context.Context
+
+	loadErr  error
+	loadBody io.ReadCloser
+
+	saveErr  error
+	saveBody io.ReadCloser
 }
 
-func (m *MockImageSaver) NegotiateAPIVersion(ctx context.Context) {
+func (m *MockClient) NegotiateAPIVersion(ctx context.Context) {
 	m.negotiated = true
 }
 
-func (m *MockImageSaver) ImageSave(_ context.Context, _ []string) (io.ReadCloser, error) {
+func (m *MockClient) ImageSave(_ context.Context, _ []string) (io.ReadCloser, error) {
 	if !m.negotiated {
 		return nil, errors.New("you forgot to call NegotiateAPIVersion before calling ImageSave")
+	}
 
+	if m.path != "" {
+		return os.Open(m.path)
 	}
-	return os.Open(m.path)
+
+	return m.saveBody, m.saveErr
 }
 
 func TestImage(t *testing.T) {
-	for _, opts := range [][]ImageOption{{
-		WithBufferedOpener(),
-		WithClient(&MockImageSaver{path: imagePath}),
+	for _, tc := range []struct {
+		name         string
+		buffered     bool
+		client       *MockClient
+		wantResponse string
+		wantErr      string
+	}{{
+		name: "success",
+		client: &MockClient{
+			path: imagePath,
+		},
+	}, {
+		name: "save err",
+		client: &MockClient{
+			saveBody: ioutil.NopCloser(strings.NewReader("Loaded")),
+			saveErr:  fmt.Errorf("locked and loaded"),
+		},
+		wantErr: "locked and loaded",
 	}, {
-		WithUnbufferedOpener(),
-		WithClient(&MockImageSaver{path: imagePath}),
+		name: "read err",
+		client: &MockClient{
+			saveBody: ioutil.NopCloser(&errReader{fmt.Errorf("goodbye, world")}),
+		},
+		wantErr: "goodbye, world",
 	}} {
-		img, err := tarball.ImageFromPath(imagePath, nil)
-		if err != nil {
-			t.Fatalf("error loading test image: %s", err)
-		}
+		run := func(t *testing.T) {
+			opts := []Option{WithClient(tc.client)}
+			if tc.buffered {
+				opts = append(opts, WithBufferedOpener())
+			} else {
+				opts = append(opts, WithUnbufferedOpener())
+			}
+			img, err := tarball.ImageFromPath(imagePath, nil)
+			if err != nil {
+				t.Fatalf("error loading test image: %s", err)
+			}
 
-		tag, err := name.NewTag("unused", name.WeakValidation)
-		if err != nil {
-			t.Fatalf("error creating test name: %s", err)
-		}
+			tag, err := name.NewTag("unused", name.WeakValidation)
+			if err != nil {
+				t.Fatalf("error creating test name: %s", err)
+			}
 
-		dmn, err := Image(tag, opts...)
-		if err != nil {
-			t.Fatalf("Error loading daemon image: %s", err)
-		}
-		if err := compare.Images(img, dmn); err != nil {
-			t.Errorf("compare.Images: %v", err)
+			dmn, err := Image(tag, opts...)
+			if err != nil {
+				if tc.wantErr == "" {
+					t.Errorf("Error loading daemon image: %s", err)
+				} else if !strings.Contains(err.Error(), tc.wantErr) {
+					t.Errorf("wanted %s to contain %s", err.Error(), tc.wantErr)
+				}
+				return
+			}
+			if err := compare.Images(img, dmn); err != nil {
+				t.Errorf("compare.Images: %v", err)
+			}
 		}
+
+		tc.buffered = true
+		t.Run(tc.name+" buffered", run)
+
+		tc.buffered = false
+		t.Run(tc.name+" unbuffered", run)
+	}
+}
+
+func TestImageDefaultClient(t *testing.T) {
+	wantErr := fmt.Errorf("bad client")
+	defaultClient = func() (Client, error) {
+		return nil, wantErr
+	}
+
+	_, err := Image(name.MustParseReference("unused"))
+	if err != wantErr {
+		t.Errorf("Image(): want %v; got %v", wantErr, err)
 	}
 }

pkg/v1/daemon/options.go

@@ -19,34 +19,71 @@ import (
 	"io"
 
 	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/client"
 )
 
-// WithBufferedOpener buffers the image.
-func WithBufferedOpener() ImageOption {
-	return func(i *imageOpener) error {
-		return i.setBuffered(true)
+type Option func(*options)
+
+type options struct {
+	ctx      context.Context
+	client   Client
+	buffered bool
+}
+
+var defaultClient = func() (Client, error) {
+	return client.NewClientWithOpts(client.FromEnv)
+}
+
+func makeOptions(opts ...Option) (*options, error) {
+	o := &options{
+		buffered: true,
+		ctx:      context.Background(),
+	}
+	for _, opt := range opts {
+		opt(o)
+	}
+
+	if o.client == nil {
+		client, err := defaultClient()
+		if err != nil {
+			return nil, err
+		}
+		o.client = client
 	}
+	o.client.NegotiateAPIVersion(o.ctx)
+
+	return o, nil
 }
 
-// WithUnbufferedOpener streams the image to avoid buffering.
-func WithUnbufferedOpener() ImageOption {
-	return func(i *imageOpener) error {
-		return i.setBuffered(false)
+// WithBufferedOpener buffers the image.
+func WithBufferedOpener() Option {
+	return func(o *options) {
+		o.buffered = true
 	}
 }
 
-func (i *imageOpener) setBuffered(buffer bool) error {
-	i.buffered = buffer
-	return nil
+// WithUnbufferedOpener streams the image to avoid buffering.
+func WithUnbufferedOpener() Option {
+	return func(o *options) {
+		o.buffered = false
+	}
 }
 
 // WithClient is a functional option to allow injecting a docker client.
 //
 // By default, github.com/docker/docker/client.FromEnv is used.
-func WithClient(client Client) ImageOption {
-	return func(i *imageOpener) error {
-		i.client = client
-		return nil
+func WithClient(client Client) Option {
+	return func(o *options) {
+		o.client = client
+	}
+}
+
+// WithContext is a functional option to pass through a context.Context.
+//
+// By default, context.Background() is used.
+func WithContext(ctx context.Context) Option {
+	return func(o *options) {
+		o.ctx = ctx
 	}
 }