Fix: overwrite existing buildpacks correctly

[fatiiates]

Summary

According to the issue, the --buildpack flag was not correctly overwriting the buildpack. So I created a whiteout layer to fix this. When it is understood that the buildpacks have the same ID and version, a tarball is created with that directory whiteout and added between these layers. Since it is added as a child layer from the newly added buildpack, only those in the same directory in the lower layer will appear as whiteout.

Output

Following the steps below, the files in the ls output should be completely new buildpack files passed as parameters in the --buildpack flag.

1. git clone https://github.com/buildpacks/samples && cd samples
2. touch buildpacks/java-maven/buildpack-in-builder
3. pack builder create testcase --config builders/alpine/builder.toml
4. rm buildpacks/java-maven/buildpack-in-builder && touch buildpacks/java-maven/buildpack-local
5. echo -e '#!/usr/bin/env bash\nls -al "${CNB_BUILDPACK_DIR}"\nexit 1' > buildpacks/java-maven/bin/detect
6. pack build sample-app --builder testcase --buildpack buildpacks/java-maven/ --path apps/java-maven/ --verbose

Before

After

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

Related

Resolves #1322

[aemengo]

Thanks so much for tackling this issue! A lot of us are on holiday, so please forgive us if you don't get a review until next week.

[fatiiates]

It's okay @aemengo, have a nice holiday.

[dfreilich]

Thanks for the PR, @fatiiates! This definitely looks like it's on the right path. Is there any chance you could add a test for this case?

[dfreilich]

nit: remove extra line

[dfreilich]

Can you make that a file var, similar to cnbDir?

[fatiiates]

I will deal with them ASAP.

[dfreilich]

nit: Can you remove this line?

[fatiiates]

I changed that line, but I doubt I got it right :)

[dfreilich]

Hey @fatiiates - for tests, we would be looking for a unit test that tests this functionality. I would look for inspiration at these current tests on that part of the code, which test the contents of the layers saved:

pack/internal/builder/builder_test.go

Lines 639 to 752 in 4f04f34

	when("saving with duplicated buildpacks", func() {
	it("adds a single buildpack to the builder image", func() {
	subject.AddBuildpack(bp1v1)
	subject.AddBuildpack(bp2v1)
	subject.AddBuildpack(bp1v1)
	err := subject.Save(logger, builder.CreatorMetadata{})
	h.AssertNil(t, err)
	h.AssertEq(t, baseImage.IsSaved(), true)
	// Expect 6 layers from the following locations:
	// - 1 from defaultDirsLayer
	// - 1 from lifecycleLayer
	// - 2 from buildpacks
	// - 1 from orderLayer
	// - 1 from stackLayer
	h.AssertEq(t, baseImage.NumberOfAddedLayers(), 6)
	})
	when("duplicated buildpack, has different contents", func() {
	var bp1v1Alt buildpack.Buildpack
	it.Before(func() {
	var err error
	bp1v1Alt, err = ifakes.NewFakeBuildpack(dist.BuildpackDescriptor{
	API: api.MustParse("0.2"),
	Info: dist.BuildpackInfo{
	ID: "buildpack-1-id",
	Version: "buildpack-1-version-1",
	},
	Stacks: []dist.Stack{{
	ID: "some.stack.id",
	Mixins: []string{"mixinX", "mixinY"},
	}},
	}, 0644, ifakes.WithExtraBuildpackContents("coolbeans", "a file cool as beans"))
	h.AssertNil(t, err)
	})
	it("uses the last buildpack", func() {
	logger := logging.NewLogWithWriters(&outBuf, &outBuf, logging.WithVerbose())
	subject.AddBuildpack(bp1v1)
	subject.AddBuildpack(bp1v1Alt)
	err := subject.Save(logger, builder.CreatorMetadata{})
	h.AssertNil(t, err)
	h.AssertEq(t, baseImage.IsSaved(), true)
	// Expect 5 layers from the following locations:
	// - 1 from defaultDirsLayer
	// - 1 from lifecycleLayer
	// - 1 from buildpacks
	// - 1 from orderLayer
	// - 1 from stackLayer
	h.AssertEq(t, baseImage.NumberOfAddedLayers(), 5)
	oldSha256 := "4dc0072c61fc2bd7118bbc93a432eae0012082de094455cf0a9fed20e3c44789"
	newSha256 := "29cb2bce4c2350f0e86f3dd30fa3810beb409b910126a18651de750f457fedfb"
	if runtime.GOOS == "windows" {
	newSha256 = "eaed4a1617bba5738ae5672f6aefda8add7abb2f8630c75dc97a6232879d4ae4"
	}
	h.AssertContains(t, outBuf.String(), fmt.Sprintf(`buildpack 'buildpack-1-id@buildpack-1-version-1' was previously defined with different contents and will be overwritten
	- previous diffID: 'sha256:%s'
	- using diffID: 'sha256:%s'`, oldSha256, newSha256))
	layer, err := baseImage.FindLayerWithPath(filepath.Join("/cnb", "buildpacks", "buildpack-1-id", "buildpack-1-version-1", "coolbeans"))
	h.AssertNil(t, err)
	bpLayer, err := os.Open(layer)
	h.AssertNil(t, err)
	defer bpLayer.Close()
	hsh := sha256.New()
	_, err = io.Copy(hsh, bpLayer)
	h.AssertNil(t, err)
	h.AssertEq(t, newSha256, fmt.Sprintf("%x", hsh.Sum(nil)))
	})
	})
	when("adding buildpack that already exists on the image", func() {
	it("skips adding buildpack that already exists", func() {
	logger := logging.NewLogWithWriters(&outBuf, &outBuf, logging.WithVerbose())
	diffID := "4dc0072c61fc2bd7118bbc93a432eae0012082de094455cf0a9fed20e3c44789"
	bpLayer := dist.BuildpackLayers{
	"buildpack-1-id": map[string]dist.BuildpackLayerInfo{
	"buildpack-1-version-1": {
	API: api.MustParse("0.2"),
	Stacks: nil,
	Order: nil,
	LayerDiffID: fmt.Sprintf("sha256:%s", diffID),
	Homepage: "",
	},
	},
	}
	bpLayerString, err := json.Marshal(bpLayer)
	h.AssertNil(t, err)
	h.AssertNil(t, baseImage.SetLabel(
	dist.BuildpackLayersLabel,
	string(bpLayerString),
	))
	subject.AddBuildpack(bp1v1)
	err = subject.Save(logger, builder.CreatorMetadata{})
	h.AssertNil(t, err)
	fmt.Println(outBuf.String())
	expectedLog := "Buildpack 'buildpack-1-id@buildpack-1-version-1' already exists on builder with same contents, skipping..."
	h.AssertContains(t, outBuf.String(), expectedLog)
	})
	})
	})

Specifically, we should change it by adding a test which tries to add two buildpacks which have the same ID and version, but have different contents, and then verify that the contents of the 2nd buildpack, and not the 1st buildpack, are there. Does that make sense?

[fatiiates]

Hey @dfreilich, thanks for the explanation. Everything is clearer now. And yes what you said makes perfect sense, that's how I'll do it.

[dfreilich]

Beautiful! If you could just fix the DCO (you can see instructions for that by clicking on the link in the checks), we'll be happy to merge this in

[dfreilich]

Merged in by #1362. Thank you for the help, @fatiiates !

Sorry for the hold-up with it, we had to figure out how to deal with some cases of a Windows image, which took me a bit of time.

[fatiiates]

It's okay, I saw the other PR you opened but I couldn't help because I have final exams. Good news that the bug has been fixed! Congrats to both of us!