Fixing non-deterministic layer and metadata ordering during pack builder create

internal/builder/builder.go

@@ -9,6 +9,7 @@ import (
 	"path"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -81,6 +82,12 @@ type orderTOML struct {
 	OrderExt dist.Order `toml:"order-extensions,omitempty"`
 }
 
+type toAdd struct {
+	tarPath string
+	diffID  string
+	module  buildpack.BuildModule
+}
+
 // FromImage constructs a builder from a builder image
 func FromImage(img imgutil.Image) (*Builder, error) {
 	var metadata Metadata
@@ -439,12 +446,6 @@ func (b *Builder) Save(logger logging.Logger, creatorMetadata CreatorMetadata) e
 // Helpers
 
 func (b *Builder) addModules(kind string, logger logging.Logger, tmpDir string, image imgutil.Image, additionalModules []buildpack.BuildModule, layers dist.ModuleLayers) error {
-	type toAdd struct {
-		tarPath string
-		diffID  string
-		module  buildpack.BuildModule
-	}
-
 	collectionToAdd := map[string]toAdd{}
 	for i, module := range additionalModules {
 		// create directory
@@ -507,7 +508,10 @@ func (b *Builder) addModules(kind string, logger logging.Logger, tmpDir string,
 		}
 	}
 
-	for _, module := range collectionToAdd {
+	// Fixes 1453
+	keys := sortKeys(collectionToAdd)
+	for _, k := range keys {
+		module := collectionToAdd[k]
 		logger.Debugf("Adding %s %s (diffID=%s)", kind, style.Symbol(module.module.Descriptor().Info().FullName()), module.diffID)
 		if err := image.AddLayerWithDiffID(module.tarPath, module.diffID); err != nil {
 			return errors.Wrapf(err,
@@ -920,3 +924,12 @@ func (b *Builder) whiteoutLayer(tmpDir string, i int, bpInfo dist.ModuleInfo) (s
 
 	return fh.Name(), nil
 }
+
+func sortKeys(collection map[string]toAdd) []string {
+	keys := make([]string, 0, len(collection))
+	for k := range collection {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	return keys
+}

internal/builder/builder_test.go

@@ -10,6 +10,7 @@ import (
 	"path"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"testing"
 
 	"github.com/buildpacks/imgutil"
@@ -47,6 +48,7 @@ func testBuilder(t *testing.T, when spec.G, it spec.S) {
 		bp1v1          buildpack.BuildModule
 		bp1v2          buildpack.BuildModule
 		bp2v1          buildpack.BuildModule
+		bp2v2          buildpack.BuildModule
 		ext1v1         buildpack.BuildModule
 		ext1v2         buildpack.BuildModule
 		ext2v1         buildpack.BuildModule
@@ -848,6 +850,61 @@ func testBuilder(t *testing.T, when spec.G, it spec.S) {
 					})
 				})
 			})
+
+			when("modules are added in random order", func() {
+				var fakeLayerImage *h.FakeAddedLayerImage
+
+				it.Before(func() {
+					var err error
+					fakeLayerImage = &h.FakeAddedLayerImage{Image: baseImage}
+					subject, err = builder.New(fakeLayerImage, "some/builder")
+					h.AssertNil(t, err)
+					subject.SetLifecycle(mockLifecycle)
+
+					bp2v2, err = ifakes.NewFakeBuildpack(dist.BuildpackDescriptor{
+						WithAPI: api.MustParse("0.2"),
+						WithInfo: dist.ModuleInfo{
+							ID:      "buildpack-2-id",
+							Version: "buildpack-2-version-2",
+						},
+						WithStacks: []dist.Stack{{
+							ID:     "some.stack.id",
+							Mixins: []string{"build:mixinA", "run:mixinB"},
+						}},
+					}, 0644)
+					h.AssertNil(t, err)
+				})
+
+				it("layers are written ordered by buildpacks ID & Version", func() {
+					// add buildpacks in a random order
+					subject.AddBuildpack(bp2v2)
+					subject.AddBuildpack(bp1v2)
+					subject.AddBuildpack(bp1v1)
+					subject.AddBuildpack(bp2v1)
+					h.AssertNil(t, subject.Save(logger, builder.CreatorMetadata{}))
+
+					layers := fakeLayerImage.AddedLayersOrder()
+					h.AssertEq(t, len(layers), 4)
+					h.AssertTrue(t, strings.Contains(layers[0], h.LayerFileName(bp1v1)))
+					h.AssertTrue(t, strings.Contains(layers[1], h.LayerFileName(bp1v2)))
+					h.AssertTrue(t, strings.Contains(layers[2], h.LayerFileName(bp2v1)))
+					h.AssertTrue(t, strings.Contains(layers[3], h.LayerFileName(bp2v2)))
+				})
+
+				it("extensions are written ordered by buildpacks ID & Version", func() {
+					// add buildpacks in a random order
+					subject.AddBuildpack(ext2v1)
+					subject.AddBuildpack(ext1v2)
+					subject.AddBuildpack(ext1v1)
+					h.AssertNil(t, subject.Save(logger, builder.CreatorMetadata{}))
+
+					layers := fakeLayerImage.AddedLayersOrder()
+					h.AssertEq(t, len(layers), 3)
+					h.AssertTrue(t, strings.Contains(layers[0], h.LayerFileName(ext1v1)))
+					h.AssertTrue(t, strings.Contains(layers[1], h.LayerFileName(ext1v2)))
+					h.AssertTrue(t, strings.Contains(layers[2], h.LayerFileName(ext2v1)))
+				})
+			})
 		})
 
 		when("#SetLifecycle", func() {

pkg/client/create_builder.go

@@ -3,6 +3,8 @@ package client
 import (
 	"context"
 	"fmt"
+	"sort"
+	"strings"
 
 	"github.com/Masterminds/semver"
 	"github.com/buildpacks/imgutil"
@@ -11,7 +13,6 @@ import (
 	pubbldr "github.com/buildpacks/pack/builder"
 	"github.com/buildpacks/pack/internal/builder"
 	"github.com/buildpacks/pack/internal/paths"
-	"github.com/buildpacks/pack/internal/strings"
 	"github.com/buildpacks/pack/internal/style"
 	"github.com/buildpacks/pack/pkg/buildpack"
 	"github.com/buildpacks/pack/pkg/image"
@@ -267,6 +268,15 @@ func (c *Client) addConfig(ctx context.Context, kind string, config pubbldr.Modu
 		}
 	}
 
+	// Fixes 1453
+	sort.Slice(depBPs, func(i, j int) bool {
+		compareId := strings.Compare(depBPs[i].Descriptor().Info().ID, depBPs[j].Descriptor().Info().ID)
+		if compareId == 0 {
+			return strings.Compare(depBPs[i].Descriptor().Info().Version, depBPs[j].Descriptor().Info().Version) <= 0
+		}
+		return compareId < 0
+	})
+
 	for _, module := range append([]buildpack.BuildModule{mainBP}, depBPs...) {
 		switch kind {
 		case buildpack.KindBuildpack:

pkg/client/create_builder_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/buildpacks/imgutil/fakes"
@@ -745,6 +746,80 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
 				h.AssertEq(t, bldr.LifecycleDescriptor().APIs.Buildpack.Deprecated.AsStrings(), []string{"0.2", "0.3"})
 				h.AssertNotContains(t, out.String(), "is using deprecated Buildpacks API version")
 			})
+
+			when("Buildpack dependencies are provided", func() {
+				var (
+					bp1v1          buildpack.BuildModule
+					bp1v2          buildpack.BuildModule
+					bp2v1          buildpack.BuildModule
+					bp2v2          buildpack.BuildModule
+					fakeLayerImage *h.FakeAddedLayerImage
+					err            error
+				)
+				it.Before(func() {
+					fakeLayerImage = &h.FakeAddedLayerImage{Image: fakeBuildImage}
+				})
+
+				var prepareBuildpackDependencies = func() []buildpack.BuildModule {
+					bp1v1Blob := blob.NewBlob(filepath.Join("testdata", "buildpack-non-deterministic", "buildpack-1-version-1"))
+					bp1v2Blob := blob.NewBlob(filepath.Join("testdata", "buildpack-non-deterministic", "buildpack-1-version-2"))
+					bp2v1Blob := blob.NewBlob(filepath.Join("testdata", "buildpack-non-deterministic", "buildpack-2-version-1"))
+					bp2v2Blob := blob.NewBlob(filepath.Join("testdata", "buildpack-non-deterministic", "buildpack-2-version-2"))
+
+					bp1v1, err = buildpack.FromBuildpackRootBlob(bp1v1Blob, archive.DefaultTarWriterFactory())
+					h.AssertNil(t, err)
+
+					bp1v2, err = buildpack.FromBuildpackRootBlob(bp1v2Blob, archive.DefaultTarWriterFactory())
+					h.AssertNil(t, err)
+
+					bp2v1, err = buildpack.FromBuildpackRootBlob(bp2v1Blob, archive.DefaultTarWriterFactory())
+					h.AssertNil(t, err)
+
+					bp2v2, err = buildpack.FromBuildpackRootBlob(bp2v2Blob, archive.DefaultTarWriterFactory())
+					h.AssertNil(t, err)
+
+					return []buildpack.BuildModule{bp2v2, bp2v1, bp1v1, bp1v2}
+				}
+
+				var successfullyCreateDeterministicBuilder = func() {
+					t.Helper()
+
+					err := subject.CreateBuilder(context.TODO(), opts)
+					h.AssertNil(t, err)
+					h.AssertEq(t, fakeLayerImage.IsSaved(), true)
+				}
+
+				it("should add dependencies buildpacks layers order by ID and version", func() {
+					mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", gomock.Any()).Return(fakeLayerImage, nil)
+					prepareFetcherWithRunImages()
+					opts.Config.Buildpacks[0].URI = "https://example.fake/bp-one-with-api-4.tgz"
+					opts.Config.Extensions[0].URI = "https://example.fake/ext-one-with-api-9.tgz"
+					bpDependencies := prepareBuildpackDependencies()
+
+					buildpackBlob := blob.NewBlob(filepath.Join("testdata", "buildpack-api-0.4"))
+					bp, err := buildpack.FromBuildpackRootBlob(buildpackBlob, archive.DefaultTarWriterFactory())
+					h.AssertNil(t, err)
+					mockBuildpackDownloader.EXPECT().Download(gomock.Any(), "https://example.fake/bp-one-with-api-4.tgz", gomock.Any()).Return(bp, bpDependencies, nil)
+
+					extensionBlob := blob.NewBlob(filepath.Join("testdata", "extension-api-0.9"))
+					extension, err := buildpack.FromExtensionRootBlob(extensionBlob, archive.DefaultTarWriterFactory())
+					h.AssertNil(t, err)
+					mockBuildpackDownloader.EXPECT().Download(gomock.Any(), "https://example.fake/ext-one-with-api-9.tgz", gomock.Any()).Return(extension, nil, nil)
+
+					successfullyCreateDeterministicBuilder()
+
+					layers := fakeLayerImage.AddedLayersOrder()
+					// Main buildpack + 4 dependencies + 1 extension
+					h.AssertEq(t, len(layers), 6)
+
+					// [0] bp.one.1.2.3.tar - main buildpack
+					h.AssertTrue(t, strings.Contains(layers[1], h.LayerFileName(bp1v1)))
+					h.AssertTrue(t, strings.Contains(layers[2], h.LayerFileName(bp1v2)))
+					h.AssertTrue(t, strings.Contains(layers[3], h.LayerFileName(bp2v1)))
+					h.AssertTrue(t, strings.Contains(layers[4], h.LayerFileName(bp2v2)))
+					// [5] ext.one.1.2.3.tar - extension
+				})
+			})
 		})
 
 		it("supports directory buildpacks", func() {

pkg/client/testdata/buildpack-non-deterministic/buildpack-1-version-1/bin/build

@@ -0,0 +1 @@
+build-contents

pkg/client/testdata/buildpack-non-deterministic/buildpack-1-version-1/buildpack.toml

@@ -0,0 +1,10 @@
+api = "0.4"
+
+[buildpack]
+id = "buildpack-1-id"
+version = "buildpack-1-version-1"
+homepage = "http://non-deterministic.buildpack-1"
+
+[[stacks]]
+id = "some.stack.id"
+mixins = ["mixinX", "build:mixinY", "run:mixinZ"]

pkg/client/testdata/buildpack-non-deterministic/buildpack-1-version-2/bin/build

@@ -0,0 +1 @@
+build-contents

pkg/client/testdata/buildpack-non-deterministic/buildpack-1-version-2/buildpack.toml

@@ -0,0 +1,11 @@
+api = "0.4"
+
+[buildpack]
+id = "buildpack-1-id"
+version = "buildpack-1-version-2"
+homepage = "http://non-deterministic.buildpack-1"
+
+[[stacks]]
+id = "some.stack.id"
+mixins = ["mixinX", "build:mixinY", "run:mixinZ"]
+

pkg/client/testdata/buildpack-non-deterministic/buildpack-2-version-1/bin/build

@@ -0,0 +1 @@
+build-contents

pkg/client/testdata/buildpack-non-deterministic/buildpack-2-version-1/buildpack.toml

@@ -0,0 +1,10 @@
+api = "0.4"
+
+[buildpack]
+id = "buildpack-2-id"
+version = "buildpack-2-version-1"
+homepage = "http://non-deterministic.buildpack-2"
+
+[[stacks]]
+id = "some.stack.id"
+mixins = ["mixinX", "build:mixinY", "run:mixinZ"]

pkg/client/testdata/buildpack-non-deterministic/buildpack-2-version-2/bin/build

@@ -0,0 +1 @@
+build-contents

pkg/client/testdata/buildpack-non-deterministic/buildpack-2-version-2/buildpack.toml

@@ -0,0 +1,10 @@
+api = "0.4"
+
+[buildpack]
+id = "buildpack-2-id"
+version = "buildpack-2-version-2"
+homepage = "http://non-deterministic.buildpack-2"
+
+[[stacks]]
+id = "some.stack.id"
+mixins = ["mixinX", "build:mixinY", "run:mixinZ"]

testhelpers/testhelpers.go

@@ -21,6 +21,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/buildpacks/imgutil/fakes"
+
 	dockertypes "github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/client"
@@ -843,3 +845,21 @@ func MockWriterAndOutput() (*color.Console, func() string) {
 		return b.String()
 	}
 }
+
+func LayerFileName(bp buildpack.BuildModule) string {
+	return fmt.Sprintf("%s.%s.tar", bp.Descriptor().Info().ID, bp.Descriptor().Info().Version)
+}
+
+type FakeAddedLayerImage struct {
+	*fakes.Image
+	addedLayersOrder []string
+}
+
+func (f *FakeAddedLayerImage) AddedLayersOrder() []string {
+	return f.addedLayersOrder
+}
+
+func (f *FakeAddedLayerImage) AddLayerWithDiffID(path, diffID string) error {
+	f.addedLayersOrder = append(f.addedLayersOrder, path)
+	return f.Image.AddLayerWithDiffID(path, diffID)
+}