Repository: uber/makisu
Pull request #258 (2019-09-15 17:56:37)

Add test for pulling images with dup layers
Test for #203.

Also need to refactor existing test setup a little bit.


Patch:
diff --git a/Makefile b/Makefile
index d4cbc0e..fd2be47 100644
--- a/Makefile
+++ b/Makefile
@@ -87,8 +87,10 @@ env: test/python/requirements.txt
 images:
 	docker build -t $(REGISTRY)/makisu:$(PACKAGE_VERSION) -f Dockerfile .
 	docker tag $(REGISTRY)/makisu:$(PACKAGE_VERSION) makisu:$(PACKAGE_VERSION)
+	docker tag $(REGISTRY)/makisu:$(PACKAGE_VERSION) makisu:latest
 	docker build -t $(REGISTRY)/makisu-alpine:$(PACKAGE_VERSION) -f Dockerfile.alpine .
 	docker tag $(REGISTRY)/makisu-alpine:$(PACKAGE_VERSION) makisu-alpine:$(PACKAGE_VERSION)
+	docker tag $(REGISTRY)/makisu-alpine:$(PACKAGE_VERSION) makisu-alpine:latest
 
 publish: images
 	docker push $(REGISTRY)/makisu:$(PACKAGE_VERSION)
diff --git a/lib/registry/client.go b/lib/registry/client.go
index 649b59d..efbdde0 100644
--- a/lib/registry/client.go
+++ b/lib/registry/client.go
@@ -119,13 +119,14 @@ func (c DockerRegistryClient) Pull(tag string) (*image.DistributionManifest, err
 
 	multiError := utils.NewMultiErrors()
 	workers := concurrency.NewWorkerPool(c.config.Concurrency)
-	layerSets := make(map[string]interface{})
+	layerSet := make(map[string]interface{})
 	for _, layer := range manifest.GetLayerDigests() {
 		l := layer
-		if _, ok := layerSets[l.Hex()]; ok {
+		if _, ok := layerSet[l.Hex()]; ok {
+			// Duplicate layer.
 			continue
 		} else {
-			layerSets[l.Hex()] = struct{}{}
+			layerSet[l.Hex()] = struct{}{}
 		}
 		workers.Do(func() {
 			if _, err := c.PullLayer(l); err != nil {
@@ -172,13 +173,14 @@ func (c DockerRegistryClient) Push(tag string) error {
 
 	multiError := utils.NewMultiErrors()
 	workers := concurrency.NewWorkerPool(c.config.Concurrency)
-	layerSets := make(map[string]interface{})
+	layerSet := make(map[string]interface{})
 	for _, layer := range manifest.GetLayerDigests() {
 		l := layer
-		if _, ok := layerSets[l.Hex()]; ok {
+		if _, ok := layerSet[l.Hex()]; ok {
+			// Duplicate layer.
 			continue
 		} else {
-			layerSets[l.Hex()] = struct{}{}
+			layerSet[l.Hex()] = struct{}{}
 		}
 		workers.Do(func() {
 			if err := c.PushLayer(l); err != nil {
@@ -330,7 +332,6 @@ func (c DockerRegistryClient) pullLayerHelper(
 	}
 	defer resp.Body.Close()
 
-	// TODO: handle concurrent download of the same file.
 	if err := c.store.Layers.CreateDownloadFile(layerDigest.Hex(), 0); err != nil {
 		return nil, fmt.Errorf("create layer file: %s", err)
 	}
diff --git a/lib/registry/pull_fixture.go b/lib/registry/pull_fixture.go
index 6b83cd4..41c2b82 100644
--- a/lib/registry/pull_fixture.go
+++ b/lib/registry/pull_fixture.go
@@ -13,28 +13,59 @@ import (
 	"github.com/uber/makisu/lib/utils/testutil"
 )
 
-// PullClientFixture returns a new registry client fixture that can handle
-// image pull requests.
-func PullClientFixture(ctx *context.BuildContext, testdataDir string) (*DockerRegistryClient, error) {
-	image := image.MustParseName(fmt.Sprintf("localhost:5055/%s:%s", testutil.SampleImageRepoName, testutil.SampleImageTag))
+const (
+	_testFileDirAlpine    = "../../testdata/files/alpine"
+	_testFileDirAlpineDup = "../../testdata/files/alpine_dup"
+)
+
+// PullClientFixture returns a new registry client fixture that can handle image
+// pull requests using a local alpine test image.
+func PullClientFixtureWithAlpine(ctx *context.BuildContext) (*DockerRegistryClient, error) {
+	return PullClientFixture(ctx,
+		filepath.Join(_testFileDirAlpine, "test_distribution_manifest"),
+		filepath.Join(_testFileDirAlpine, "test_image_config"),
+		filepath.Join(_testFileDirAlpine, "test_layer.tar"))
+}
+
+// PullClientFixture returns a new registry client fixture that can handle image
+// pull requests using a local alpine test image that contains duplicate layers.
+func PullClientFixtureWithAlpineDup(ctx *context.BuildContext) (*DockerRegistryClient, error) {
+	return PullClientFixture(ctx,
+		filepath.Join(_testFileDirAlpineDup, "test_distribution_manifest"),
+		filepath.Join(_testFileDirAlpine, "test_image_config"),
+		filepath.Join(_testFileDirAlpine, "test_layer.tar"))
+}
+
+// PullClientFixture returns a new registry client fixture that can handle image
+// pull requests.
+func PullClientFixture(
+	ctx *context.BuildContext, manifestPath, imageConfigPath, layerTarPath string,
+) (*DockerRegistryClient, error) {
+
+	imageName := image.MustParseName(
+		fmt.Sprintf("localhost:5055/%s:%s", testutil.SampleImageRepoName, testutil.SampleImageTag))
 	cli := &http.Client{
 		Transport: pullTransportFixture{
-			image:       image,
-			testdataDir: testdataDir,
+			imageName:       imageName,
+			manifestPath:    manifestPath,
+			imageConfigPath: imageConfigPath,
+			layerTarPath:    layerTarPath,
 		},
 	}
-	c := NewWithClient(ctx.ImageStore, image.GetRegistry(), image.GetRepository(), cli)
+	c := NewWithClient(ctx.ImageStore, imageName.GetRegistry(), imageName.GetRepository(), cli)
 	c.config.Security.TLS.Client.Disabled = true
 	return c, nil
 }
 
 type pullTransportFixture struct {
-	image       image.Name
-	testdataDir string
+	imageName       image.Name
+	manifestPath    string
+	imageConfigPath string
+	layerTarPath    string
 }
 
 func (t pullTransportFixture) manifestResponse() (*http.Response, error) {
-	manifest, err := os.Open(filepath.Join(t.testdataDir, "files/test_distribution_manifest"))
+	manifest, err := os.Open(t.manifestPath)
 	if err != nil {
 		return nil, err
 	}
@@ -48,7 +79,7 @@ func (t pullTransportFixture) manifestResponse() (*http.Response, error) {
 }
 
 func (t pullTransportFixture) imageConfigResponse() (*http.Response, error) {
-	imageConfig, err := os.Open(filepath.Join(t.testdataDir, "files/test_image_config"))
+	imageConfig, err := os.Open(t.imageConfigPath)
 	if err != nil {
 		return nil, err
 	}
@@ -60,7 +91,7 @@ func (t pullTransportFixture) imageConfigResponse() (*http.Response, error) {
 }
 
 func (t pullTransportFixture) layerResponse() (*http.Response, error) {
-	layerTar, err := os.Open(filepath.Join(t.testdataDir, "files/test_layer.tar"))
+	layerTar, err := os.Open(t.layerTarPath)
 	if err != nil {
 		return nil, err
 	}
@@ -72,8 +103,10 @@ func (t pullTransportFixture) layerResponse() (*http.Response, error) {
 }
 
 func (t pullTransportFixture) RoundTrip(r *http.Request) (*http.Response, error) {
-	repoURL := fmt.Sprintf("http://%s/v2/%s", t.image.GetRegistry(), t.image.GetRepository())
-	manifestURL := fmt.Sprintf("%s/manifests/%s", repoURL, t.image.GetTag())
+	repoURL := fmt.Sprintf(
+		"http://%s/v2/%s", t.imageName.GetRegistry(), t.imageName.GetRepository())
+	manifestURL := fmt.Sprintf(
+		"%s/manifests/%s", repoURL, t.imageName.GetTag())
 	imageConfigURL := repoURL + "/blobs/sha256:" + testutil.SampleImageConfigDigest
 	layerTarURL := repoURL + "/blobs/sha256:" + testutil.SampleLayerTarDigest
 	url := r.URL.String()
diff --git a/lib/storage/fixtures.go b/lib/storage/fixtures.go
index e32e28e..93f2765 100644
--- a/lib/storage/fixtures.go
+++ b/lib/storage/fixtures.go
@@ -48,7 +48,7 @@ func StoreFixture() (*ImageStore, func()) {
 func StoreFixtureWithSampleImage() (*ImageStore, func()) {
 	store, c := StoreFixture()
 
-	manifestData, err := ioutil.ReadFile("../../testdata/files/test_distribution_manifest")
+	manifestData, err := ioutil.ReadFile("../../testdata/files/alpine/test_distribution_manifest")
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -68,7 +68,7 @@ func StoreFixtureWithSampleImage() (*ImageStore, func()) {
 		log.Fatal(err)
 	}
 
-	imageConfigData, err := ioutil.ReadFile("../../testdata/files/test_image_config")
+	imageConfigData, err := ioutil.ReadFile("../../testdata/files/alpine/test_image_config")
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -85,7 +85,7 @@ func StoreFixtureWithSampleImage() (*ImageStore, func()) {
 		log.Fatal(err)
 	}
 
-	layerTarData, err := ioutil.ReadFile("../../testdata/files/test_layer.tar")
+	layerTarData, err := ioutil.ReadFile("../../testdata/files/alpine/test_layer.tar")
 	if err != nil {
 		log.Fatal(err)
 	}


Test patch:
diff --git a/lib/builder/step/from_step_test.go b/lib/builder/step/from_step_test.go
index 7aeb54b..9764fe2 100644
--- a/lib/builder/step/from_step_test.go
+++ b/lib/builder/step/from_step_test.go
@@ -17,6 +17,7 @@ package step
 import (
 	"encoding/json"
 	"io/ioutil"
+	"path/filepath"
 	"testing"
 
 	"github.com/uber/makisu/lib/context"
@@ -129,7 +130,11 @@ func TestFromStepRegularFlow(t *testing.T) {
 	ctx, cleanup := context.BuildContextFixture()
 	defer cleanup()
 
-	p, err := registry.PullClientFixture(ctx, "../../../testdata")
+	testFileDirAlpine := "../../../testdata/files/alpine"
+	p, err := registry.PullClientFixture(ctx,
+		filepath.Join(testFileDirAlpine, "test_distribution_manifest"),
+		filepath.Join(testFileDirAlpine, "test_image_config"),
+		filepath.Join(testFileDirAlpine, "test_layer.tar"))
 	require.NoError(err)
 
 	step, err := NewFromStep("", "fakeregistry.dev/library/alpine:latest", "")
@@ -150,7 +155,7 @@ func TestFromStepRegularFlow(t *testing.T) {
 	// Generate config.
 	conf, err := step.UpdateCtxAndConfig(ctx, nil)
 	require.NoError(err)
-	expectedConfBytes, err := ioutil.ReadFile("../../../testdata/files/test_image_config")
+	expectedConfBytes, err := ioutil.ReadFile("../../../testdata/files/alpine/test_image_config")
 	require.NoError(err)
 	var expectedConf image.Config
 	require.NoError(json.Unmarshal(expectedConfBytes, &expectedConf))
diff --git a/lib/registry/client_test.go b/lib/registry/client_test.go
index e206b73..e252d9b 100644
--- a/lib/registry/client_test.go
+++ b/lib/registry/client_test.go
@@ -16,6 +16,7 @@ package registry
 
 import (
 	"io/ioutil"
+	"path"
 	"testing"
 
 	"github.com/uber/makisu/lib/context"
@@ -25,14 +26,12 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-var _testdata = "../../testdata"
-
 func TestPullManifest(t *testing.T) {
 	require := require.New(t)
 	ctx, cleanup := context.BuildContextFixture()
 	defer cleanup()
 
-	p, err := PullClientFixture(ctx, _testdata)
+	p, err := PullClientFixtureWithAlpine(ctx)
 	require.NoError(err)
 
 	// Pull manifest.
@@ -45,7 +44,26 @@ func TestPullImage(t *testing.T) {
 	ctx, cleanup := context.BuildContextFixture()
 	defer cleanup()
 
-	p, err := PullClientFixture(ctx, _testdata)
+	p, err := PullClientFixtureWithAlpine(ctx)
+	require.NoError(err)
+
+	// Pull image.
+	_, err = p.Pull(testutil.SampleImageTag)
+	require.NoError(err)
+
+	_, err = p.store.Layers.GetStoreFileStat("393ccd5c4dd90344c9d725125e13f636ce0087c62f5ca89050faaacbb9e3ed5b")
+	require.NoError(err)
+
+	_, err = p.store.Manifests.GetStoreFileStat(testutil.SampleImageRepoName, testutil.SampleImageTag)
+	require.NoError(err)
+}
+
+func TestPullImageWithDuplicateLayers(t *testing.T) {
+	require := require.New(t)
+	ctx, cleanup := context.BuildContextFixture()
+	defer cleanup()
+
+	p, err := PullClientFixtureWithAlpineDup(ctx)
 	require.NoError(err)
 
 	// Pull image.
@@ -59,16 +77,16 @@ func TestPullImage(t *testing.T) {
 	require.NoError(err)
 }
 
-func TestPullWithExistingLayer(t *testing.T) {
+func TestPullWithExistingLayerInCache(t *testing.T) {
 	require := require.New(t)
 	ctx, cleanup := context.BuildContextFixture()
 	defer cleanup()
 
-	p, err := PullClientFixture(ctx, _testdata)
+	p, err := PullClientFixtureWithAlpine(ctx)
 	require.NoError(err)
 
 	// Put layer in store first.
-	layerTarData, err := ioutil.ReadFile("../../testdata/files/test_layer.tar")
+	layerTarData, err := ioutil.ReadFile(path.Join(_testFileDirAlpine, "test_layer.tar"))
 	require.NoError(err)
 	err = ctx.ImageStore.Layers.CreateDownloadFile("393ccd5c4dd90344c9d725125e13f636ce0087c62f5ca89050faaacbb9e3ed5b", 0)
 	require.NoError(err)
@@ -93,7 +111,7 @@ func TestManifestExists(t *testing.T) {
 	ctx, cleanup := context.BuildContextFixture()
 	defer cleanup()
 
-	p, err := PullClientFixture(ctx, _testdata)
+	p, err := PullClientFixtureWithAlpine(ctx)
 	require.NoError(err)
 
 	exists, err := p.manifestExists(testutil.SampleImageTag)
@@ -106,7 +124,7 @@ func TestLayerExists(t *testing.T) {
 	ctx, cleanup := context.BuildContextFixture()
 	defer cleanup()
 
-	p, err := PullClientFixture(ctx, _testdata)
+	p, err := PullClientFixtureWithAlpine(ctx)
 	require.NoError(err)
 
 	exists, err := p.layerExists("sha256:" + testutil.SampleLayerTarDigest)
diff --git a/testdata/files/test_distribution_manifest b/testdata/files/alpine/test_distribution_manifest
similarity index 100%
rename from testdata/files/test_distribution_manifest
rename to testdata/files/alpine/test_distribution_manifest
diff --git a/testdata/files/test_image_config b/testdata/files/alpine/test_image_config
similarity index 100%
rename from testdata/files/test_image_config
rename to testdata/files/alpine/test_image_config
diff --git a/testdata/files/test_layer.tar b/testdata/files/alpine/test_layer.tar
similarity index 100%
rename from testdata/files/test_layer.tar
rename to testdata/files/alpine/test_layer.tar
diff --git a/testdata/files/alpine_dup/test_distribution_manifest b/testdata/files/alpine_dup/test_distribution_manifest
new file mode 100644
index 0000000..f95d96e
--- /dev/null
+++ b/testdata/files/alpine_dup/test_distribution_manifest
@@ -0,0 +1,21 @@
+{
+   "schemaVersion": 2,
+   "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
+   "config": {
+      "mediaType": "application/vnd.docker.container.image.v1+json",
+      "size": 2940,
+      "digest": "sha256:a052f56e596097698ac74bb4b03607f2dd6bc026751878ff5d57a74bb043f098"
+   },
+   "layers": [
+      {
+         "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
+         "size": 1902063,
+         "digest": "sha256:393ccd5c4dd90344c9d725125e13f636ce0087c62f5ca89050faaacbb9e3ed5b"
+      },
+      {
+         "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
+         "size": 1902063,
+         "digest": "sha256:393ccd5c4dd90344c9d725125e13f636ce0087c62f5ca89050faaacbb9e3ed5b"
+      }
+   ]
+}