From 3d8bc6a99c11a49a43d3db858e2f8b7c4592b65c Mon Sep 17 00:00:00 2001 From: Glyn Normington Date: Thu, 20 Sep 2018 10:36:43 +0100 Subject: [PATCH] riff image load Fixes https://github.com/projectriff/riff/issues/806 --- Makefile | 1 + cmd/commands/image.go | 43 ++- cmd/commands/wiring.go | 1 + docs/riff_image.md | 1 + docs/riff_image_load.md | 33 ++ docs/riff_image_push.md | 2 + pkg/core/fixtures/image_client/complete.yaml | 4 + .../fixtures/image_client/incomplete.yaml | 4 + pkg/core/image_client.go | 41 ++- pkg/core/image_client_test.go | 306 ++++++++++++++++++ pkg/docker/docker.go | 12 +- pkg/docker/mocks/Docker.go | 59 ++++ 12 files changed, 493 insertions(+), 14 deletions(-) create mode 100644 docs/riff_image_load.md create mode 100644 pkg/core/fixtures/image_client/complete.yaml create mode 100644 pkg/core/fixtures/image_client/incomplete.yaml create mode 100644 pkg/core/image_client_test.go create mode 100644 pkg/docker/mocks/Docker.go diff --git a/Makefile b/Makefile index 634c568f6..ecbd8e9b3 100644 --- a/Makefile +++ b/Makefile @@ -24,6 +24,7 @@ gen-mocks: mockery -output pkg/core/vendor_mocks -outpkg vendor_mocks -dir vendor/k8s.io/client-go/kubernetes/typed/core/v1 -name NamespaceInterface mockery -output pkg/core/vendor_mocks -outpkg vendor_mocks -dir vendor/k8s.io/client-go/kubernetes/typed/core/v1 -name ServiceAccountInterface mockery -output pkg/core/vendor_mocks -outpkg vendor_mocks -dir vendor/k8s.io/client-go/kubernetes/typed/core/v1 -name SecretInterface + mockery -output pkg/docker/mocks -outpkg mocks -dir pkg/docker -name Docker install: build cp $(OUTPUT) $(GOBIN) diff --git a/cmd/commands/image.go b/cmd/commands/image.go index fe80cc05f..6e0a05286 100644 --- a/cmd/commands/image.go +++ b/cmd/commands/image.go @@ -119,6 +119,46 @@ func ImageRelocate(c *core.Client) *cobra.Command { return command } +func ImageLoad(c *core.ImageClient) *cobra.Command { + options := core.LoadAndTagImagesOptions{} + + command := &cobra.Command{ + Use: "load", + Short: "Load and tag docker images", + Long: "Load the set of images identified by the provided image manifest into a docker daemon.\n\n" + + "NOTE: This command requires the `docker` command line tool, as well as a (local) docker daemon.\n\n"+ + "SEE ALSO: To load, tag, and push images, use `riff image push`.", + Example: ` riff image load --images=riff-distro-xx/image-manifest.yaml`, + PreRunE: func(cmd *cobra.Command, args []string) error { + // FIXME: these flags should not apply to this command: https://github.com/projectriff/riff/issues/743 + if cmd.Flags().Changed("kubeconfig") { + return errors.New("the 'kubeconfig' flag is not supported by the 'image load' command") + } + m, _ := cmd.Flags().GetString("master") + if len(m) > 0 { + return errors.New("the 'master' flag is not supported by the 'image load' command") + } + + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { + err := (*c).LoadAndTagImages(options) + if err != nil { + return err + } + + PrintSuccessfulCompletion(cmd) + return nil + }, + } + command.Flags().StringVarP(&options.Images, "images", "i", "", "path of an image manifest of image names to be loaded") + command.MarkFlagRequired("images") + command.MarkFlagFilename("images", "yml", "yaml") + + return command +} + + func ImagePush(c *core.ImageClient) *cobra.Command { options := core.PushImagesOptions{} @@ -126,7 +166,8 @@ func ImagePush(c *core.ImageClient) *cobra.Command { Use: "push", Short: "Push (relocated) docker image names to an image registry", Long: "Push the set of images identified by the provided image manifest into a remote registry, for later consumption by `riff system install`.\n\n" + - "NOTE: This command requires the `docker` command line tool, as well as a (local) docker daemon and will load and tag the images using that daemon.", + "NOTE: This command requires the `docker` command line tool, as well as a (local) docker daemon and will load and tag the images using that daemon.\n\n"+ + "SEE ALSO: To load and tag images, but not push them, use `riff image load`.", Example: ` riff image push --images=riff-distro-xx/image-manifest.yaml`, RunE: func(cmd *cobra.Command, args []string) error { err := (*c).PushImages(options) diff --git a/cmd/commands/wiring.go b/cmd/commands/wiring.go index 22aca218f..0e4255606 100644 --- a/cmd/commands/wiring.go +++ b/cmd/commands/wiring.go @@ -135,6 +135,7 @@ See https://projectriff.io and https://github.com/knative/docs`, image := Image() image.AddCommand( ImageRelocate(&client), + ImageLoad(&imageClient), ImagePush(&imageClient), ) diff --git a/docs/riff_image.md b/docs/riff_image.md index d03031b3d..36aada8ea 100644 --- a/docs/riff_image.md +++ b/docs/riff_image.md @@ -15,6 +15,7 @@ Interact with docker images ### SEE ALSO * [riff](riff.md) - Commands for creating and managing function resources +* [riff image load](riff_image_load.md) - Load and tag docker images * [riff image push](riff_image_push.md) - Push (relocated) docker image names to an image registry * [riff image relocate](riff_image_relocate.md) - Relocate docker image names to another registry diff --git a/docs/riff_image_load.md b/docs/riff_image_load.md new file mode 100644 index 000000000..f1be4aab0 --- /dev/null +++ b/docs/riff_image_load.md @@ -0,0 +1,33 @@ +## riff image load + +Load and tag docker images + +### Synopsis + +Load the set of images identified by the provided image manifest into a docker daemon. + +NOTE: This command requires the `docker` command line tool, as well as a (local) docker daemon. + +SEE ALSO: To load, tag, and push images, use the image push command. + +``` +riff image load [flags] +``` + +### Examples + +``` + riff image load --images=riff-distro-xx/image-manifest.yaml +``` + +### Options + +``` + -h, --help help for load + -i, --images string path of an image manifest of image names to be loaded +``` + +### SEE ALSO + +* [riff image](riff_image.md) - Interact with docker images + diff --git a/docs/riff_image_push.md b/docs/riff_image_push.md index e5fa5b515..3c61cd7b0 100644 --- a/docs/riff_image_push.md +++ b/docs/riff_image_push.md @@ -8,6 +8,8 @@ Push the set of images identified by the provided image manifest into a remote r NOTE: This command requires the `docker` command line tool, as well as a (local) docker daemon and will load and tag the images using that daemon. +SEE ALSO: To load and tag images but not push them, use the image load command. + ``` riff image push [flags] ``` diff --git a/pkg/core/fixtures/image_client/complete.yaml b/pkg/core/fixtures/image_client/complete.yaml new file mode 100644 index 000000000..6291aaf3c --- /dev/null +++ b/pkg/core/fixtures/image_client/complete.yaml @@ -0,0 +1,4 @@ +Images: + a/b: "1" + c/d: "2" +manifestVersion: "0.1" diff --git a/pkg/core/fixtures/image_client/incomplete.yaml b/pkg/core/fixtures/image_client/incomplete.yaml new file mode 100644 index 000000000..91a0e809d --- /dev/null +++ b/pkg/core/fixtures/image_client/incomplete.yaml @@ -0,0 +1,4 @@ +manifestVersion: 0.1 +images: + "a/b": "1" + "c/d": diff --git a/pkg/core/image_client.go b/pkg/core/image_client.go index 174b39f16..1ee4d384c 100644 --- a/pkg/core/image_client.go +++ b/pkg/core/image_client.go @@ -26,6 +26,7 @@ import ( ) type ImageClient interface { + LoadAndTagImages(options LoadAndTagImagesOptions) error PushImages(options PushImagesOptions) error PullImages(options PullImagesOptions) error } @@ -34,6 +35,10 @@ type PushImagesOptions struct { Images string } +type LoadAndTagImagesOptions struct { + Images string +} + type PullImagesOptions struct { Images string Output string @@ -44,22 +49,42 @@ type imageClient struct { docker docker.Docker } -// TODO: provide unit test +func (c *imageClient) LoadAndTagImages(options LoadAndTagImagesOptions) error { + _, err := c.loadAndTagImages(options.Images) + return err +} + func (c *imageClient) PushImages(options PushImagesOptions) error { - imManifest, err := NewImageManifest(options.Images) + imManifest, err := c.loadAndTagImages(options.Images) if err != nil { return err } - distroLocation := filepath.Dir(options.Images) - for name, digest := range imManifest.Images { - filename := filepath.Join(distroLocation, "images", string(digest)) - if err := c.docker.PushImage(string(name), string(digest), filename); err != nil { + for name, _ := range imManifest.Images { + if err := c.docker.PushImage(string(name)); err != nil { return err } } return nil } +func (c *imageClient) loadAndTagImages(imageManifest string) (*ImageManifest, error) { + imManifest, err := NewImageManifest(imageManifest) + if err != nil { + return nil, err + } + distroLocation := filepath.Dir(imageManifest) + for name, digest := range imManifest.Images { + if digest == "" { + return nil, fmt.Errorf("image manifest %s does not specify a digest for image %s", imageManifest, name) + } + filename := filepath.Join(distroLocation, "images", string(digest)) + if err := c.docker.LoadAndTagImage(string(name), string(digest), filename); err != nil { + return nil, err + } + } + return imManifest, nil +} + func (c *imageClient) PullImages(options PullImagesOptions) error { originalManifest, err := NewImageManifest(options.Images) if err != nil { @@ -74,7 +99,7 @@ func (c *imageClient) PullImages(options PullImagesOptions) error { newManifestPath = filepath.Join(options.Output, "image-manifest.yaml") imagesDir = filepath.Join(options.Output, "images") } - if _, err := os.Stat(imagesDir); err != nil && os.IsNotExist(err) { + if _, err := os.Stat(imagesDir); err != nil { if err2 := os.MkdirAll(imagesDir, outputDirPermissions); err2 != nil { return err2 } @@ -86,7 +111,7 @@ func (c *imageClient) PullImages(options PullImagesOptions) error { if newSha, err := c.docker.PullImage(string(name), imagesDir); err != nil { return err } else if newSha != string(sha) && sha != "" && !options.ContinueOnMismatch { - return fmt.Errorf("image %q had digest %v in the original manifest, but the pulled version now has digest %s", name, sha, newSha) + return fmt.Errorf("image %q had digest %v in the original manifest, but the pulled version has digest %s", name, sha, newSha) } else { newManifest.Images[name] = imageDigest(newSha) } diff --git a/pkg/core/image_client_test.go b/pkg/core/image_client_test.go new file mode 100644 index 000000000..1b4be27d0 --- /dev/null +++ b/pkg/core/image_client_test.go @@ -0,0 +1,306 @@ +/* + * Copyright 2018 The original author or authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package core_test + +import ( + "errors" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "github.com/projectriff/riff/pkg/core" + "github.com/projectriff/riff/pkg/docker/mocks" + "github.com/stretchr/testify/mock" + "io/ioutil" + "os" + "path/filepath" +) + +var _ = Describe("ImageClient", func() { + var ( + imageClient core.ImageClient + mockDocker *mocks.Docker + testError error + ) + + BeforeEach(func() { + mockDocker = new(mocks.Docker) + testError = errors.New("test error") + }) + + JustBeforeEach(func() { + imageClient = core.NewImageClient(mockDocker) + }) + + AfterEach(func() { + mockDocker.AssertExpectations(GinkgoT()) + }) + + Describe("LoadAndTagImages", func() { + var ( + options core.LoadAndTagImagesOptions + err error + ) + + JustBeforeEach(func() { + err = imageClient.LoadAndTagImages(options) + }) + + Context("when the manifest has a digest for each image", func() { + BeforeEach(func() { + options.Images = "fixtures/image_client/complete.yaml" + mockDocker.On("LoadAndTagImage", "a/b", "1", "fixtures/image_client/images/1").Return(nil) + mockDocker.On("LoadAndTagImage", "c/d", "2", "fixtures/image_client/images/2").Return(nil) + }) + + It("should succeed", func() { + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Context("when the manifest has a missing digest", func() { + BeforeEach(func() { + options.Images = "fixtures/image_client/incomplete.yaml" + mockDocker.On("LoadAndTagImage", "a/b", "1", "fixtures/image_client/images/1").Return(nil).Maybe() + }) + + It("should succeed", func() { + Expect(err).To(MatchError("image manifest fixtures/image_client/incomplete.yaml does not specify a digest for image c/d")) + }) + }) + + Context("when the docker client returns an error", func() { + BeforeEach(func() { + mockDocker.On("LoadAndTagImage", mock.Anything, mock.Anything, mock.Anything).Return(testError).Once() + options.Images = "fixtures/image_client/complete.yaml" + }) + + It("should propagate the error", func() { + Expect(err).To(MatchError(testError)) + }) + }) + + Context("when the image manifest cannot be read", func() { + BeforeEach(func() { + options.Images = "no/such" + }) + + It("should return a suitable error", func() { + Expect(err).To(MatchError("error reading image manifest file: open no/such: no such file or directory")) + }) + }) + }) + + Describe("PushImages", func() { + var ( + options core.PushImagesOptions + err error + ) + + JustBeforeEach(func() { + err = imageClient.PushImages(options) + }) + + Context("when the manifest has a digest for each image", func() { + BeforeEach(func() { + options.Images = "fixtures/image_client/complete.yaml" + mockDocker.On("LoadAndTagImage", mock.Anything, mock.Anything, mock.Anything).Return(nil).Twice() + mockDocker.On("PushImage", "a/b").Return(nil) + mockDocker.On("PushImage", "c/d").Return(nil) + }) + + It("should succeed", func() { + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Context("when the manifest has a missing digest", func() { + BeforeEach(func() { + options.Images = "fixtures/image_client/incomplete.yaml" + mockDocker.On("LoadAndTagImage", "a/b", "1", "fixtures/image_client/images/1").Return(nil).Maybe() + }) + + It("should succeed", func() { + Expect(err).To(MatchError("image manifest fixtures/image_client/incomplete.yaml does not specify a digest for image c/d")) + }) + }) + + Context("when the docker client returns an error", func() { + BeforeEach(func() { + mockDocker.On("LoadAndTagImage", mock.Anything, mock.Anything, mock.Anything).Return(nil).Twice() + mockDocker.On("PushImage", mock.Anything).Return(testError).Once() + options.Images = "fixtures/image_client/complete.yaml" + }) + + It("should propagate the error", func() { + Expect(err).To(MatchError(testError)) + }) + }) + + Context("when the image manifest cannot be read", func() { + BeforeEach(func() { + options.Images = "no/such" + }) + + It("should return a suitable error", func() { + Expect(err).To(MatchError("error reading image manifest file: open no/such: no such file or directory")) + }) + }) + }) + + Describe("PullImages", func() { + var ( + options core.PullImagesOptions + workDir string + imagesDir string + err error + expectedImageManifest *core.ImageManifest + ) + + BeforeEach(func() { + // Ensure optional options do not leak from one test to another + options.Output = "" + options.ContinueOnMismatch = false + + // Avoid tests updating fixtures + workDir, err = ioutil.TempDir("", "image_client_test") + Expect(err).NotTo(HaveOccurred()) + options.Images = copyFile("fixtures/image_client/complete.yaml", workDir) + imagesDir = filepath.Join(workDir, "images") + expectedImageManifest = core.EmptyImageManifest() + expectedImageManifest.Images["a/b"] = "1" + expectedImageManifest.Images["c/d"] = "2" + }) + + AfterEach(func() { + err = os.RemoveAll(workDir) + Expect(err).NotTo(HaveOccurred()) + }) + + JustBeforeEach(func() { + err = imageClient.PullImages(options) + }) + + Context("when the returned digests match those in the manifest", func() { + BeforeEach(func() { + mockDocker.On("PullImage", "a/b", imagesDir).Return("1", nil) + mockDocker.On("PullImage", "c/d", imagesDir).Return("2", nil) + }) + + It("should succeed", func() { + Expect(err).NotTo(HaveOccurred()) + }) + + It("should output the correct image manifest", func() { + Expect(actualImageManifest(options.Images)).To(Equal(expectedImageManifest)) + }) + }) + + Context("when the returned digests conflict with those in the manifest", func() { + BeforeEach(func() { + mockDocker.On("PullImage", "a/b", imagesDir).Return("1", nil).Maybe() + mockDocker.On("PullImage", "c/d", imagesDir).Return("3", nil) + }) + + Context("when conflicts are not allowed", func() { + It("should fail with a suitable error", func() { + Expect(err).To(MatchError(`image "c/d" had digest 2 in the original manifest, but the pulled version has digest 3`)) + }) + }) + + Context("when conflicts are allowed", func() { + BeforeEach(func() { + options.ContinueOnMismatch = true + expectedImageManifest.Images["c/d"] = "3" + }) + + It("should succeed", func() { + Expect(err).NotTo(HaveOccurred()) + }) + + It("should output the correct image manifest", func() { + Expect(actualImageManifest(options.Images)).To(Equal(expectedImageManifest)) + }) + }) + }) + + Context("when output directory is specified", func() { + BeforeEach(func() { + options.Output = filepath.Join(workDir, "image_client_test_output") + imagesOutputDir := filepath.Join(options.Output, "images") + mockDocker.On("PullImage", "a/b", imagesOutputDir).Return("1", nil) + mockDocker.On("PullImage", "c/d", imagesOutputDir).Return("2", nil) + }) + + It("should succeed", func() { + Expect(err).NotTo(HaveOccurred()) + }) + + It("should output the correct image manifest", func() { + Expect(actualImageManifest(filepath.Join(options.Output, "image-manifest.yaml"))).To(Equal(expectedImageManifest)) + }) + }) + + Context("when output images directory cannot be created", func() { + BeforeEach(func() { + options.Output = filepath.Join(workDir, "image_client_test_output") + err = ioutil.WriteFile(options.Output, []byte{1}, 0644) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should return a suitable error", func() { + Expect(err).To(HaveOccurred()) + if _, ok := err.(*os.PathError); !ok { + Fail("Unexpected error type") + } + }) + }) + + Context("when the docker client returns an error", func() { + BeforeEach(func() { + mockDocker.On("PullImage", mock.Anything, mock.Anything).Return("", testError).Once() + }) + + It("should propagate the error", func() { + Expect(err).To(MatchError(testError)) + }) + }) + + Context("when the image manifest cannot be read", func() { + BeforeEach(func() { + options.Images = "no/such" + }) + + It("should return a suitable error", func() { + Expect(err).To(MatchError("error reading image manifest file: open no/such: no such file or directory")) + }) + }) + }) +}) + +func copyFile(src string, destDir string) string { + contents, err := ioutil.ReadFile(src) + Expect(err).NotTo(HaveOccurred()) + dest := filepath.Join(destDir, filepath.Base(src)) + err = ioutil.WriteFile(dest, contents, 0644) + Expect(err).NotTo(HaveOccurred()) + return dest +} + +func actualImageManifest(path string) *core.ImageManifest { + m, err := core.NewImageManifest(path) + Expect(err).NotTo(HaveOccurred()) + return m +} diff --git a/pkg/docker/docker.go b/pkg/docker/docker.go index 9beaf9511..7a99d90b3 100644 --- a/pkg/docker/docker.go +++ b/pkg/docker/docker.go @@ -29,7 +29,8 @@ import ( ) type Docker interface { - PushImage(name string, digest string, file string) error + LoadAndTagImage(name string, digest string, file string) error + PushImage(name string) error PullImage(name string, directory string) (digest string, err error) } @@ -41,13 +42,14 @@ type processDocker struct { stderr io.Writer } -func (pd *processDocker) PushImage(name string, digest string, file string) error { +func (pd *processDocker) LoadAndTagImage(name string, digest string, file string) error { if err := pd.exec(5*time.Minute, "image", "load", "-i", file); err != nil { return err } - if err := pd.exec(1*time.Second, "image", "tag", digest, name); err != nil { - return err - } + return pd.exec(1*time.Second, "image", "tag", digest, name) +} + +func (pd *processDocker) PushImage(name string) error { return pd.exec(10*time.Minute, "push", name) } diff --git a/pkg/docker/mocks/Docker.go b/pkg/docker/mocks/Docker.go new file mode 100644 index 000000000..2799dadb8 --- /dev/null +++ b/pkg/docker/mocks/Docker.go @@ -0,0 +1,59 @@ +// Code generated by mockery v1.0.0. DO NOT EDIT. + +package mocks + +import mock "github.com/stretchr/testify/mock" + +// Docker is an autogenerated mock type for the Docker type +type Docker struct { + mock.Mock +} + +// LoadAndTagImage provides a mock function with given fields: name, digest, file +func (_m *Docker) LoadAndTagImage(name string, digest string, file string) error { + ret := _m.Called(name, digest, file) + + var r0 error + if rf, ok := ret.Get(0).(func(string, string, string) error); ok { + r0 = rf(name, digest, file) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// PullImage provides a mock function with given fields: name, directory +func (_m *Docker) PullImage(name string, directory string) (string, error) { + ret := _m.Called(name, directory) + + var r0 string + if rf, ok := ret.Get(0).(func(string, string) string); ok { + r0 = rf(name, directory) + } else { + r0 = ret.Get(0).(string) + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(name, directory) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// PushImage provides a mock function with given fields: name +func (_m *Docker) PushImage(name string) error { + ret := _m.Called(name) + + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { + r0 = rf(name) + } else { + r0 = ret.Error(0) + } + + return r0 +}