Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explode flattened buildpacks when pack build #1787

Merged
merged 39 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
735bfc8
Avoid writing duplicate tar files for flattened buildpacks
jjbustamante May 25, 2023
362052a
WIP
jjbustamante May 26, 2023
03808b7
Implementing logic split flattened modules into different tar files, and
jjbustamante May 30, 2023
f5cce3c
skipping --flatten test on previous versions of pack
jjbustamante Jun 15, 2023
c456691
buildpack directories not supported on windows
jjbustamante Jun 15, 2023
f938d7e
WIP - logging again, it seems we have a concurrency issue
jjbustamante Jun 15, 2023
36fcb55
fixes
jjbustamante Jun 15, 2023
c8e0705
removing some structs that were duplicated
jjbustamante Jun 16, 2023
47f76c4
Fixing weird behavior in the acceptance tests
jjbustamante Jun 20, 2023
12126f3
Applying review suggestions from Joe Kimmel
jjbustamante Jun 20, 2023
f23c0df
Apply suggestions from code review
jjbustamante Jun 20, 2023
39b275e
fixing assertions in flattened buildpack tests
jjbustamante Jun 20, 2023
3903d96
fixing unit tests assertions
jjbustamante Jun 21, 2023
1255cc0
Apply suggestions from code review
jjbustamante Jun 21, 2023
d38479d
refactoring the implementation of ToNLayerTar method to use the recur…
jjbustamante Jun 22, 2023
1a47dd7
Merge branch 'main' into fix/flatten-tar-extras-2
jjbustamante Jun 22, 2023
ff30cd4
adding some feedback from review
jjbustamante Jun 22, 2023
1adc59e
fixing path errors on windows
jjbustamante Jun 22, 2023
dceef0d
trying to fix the path issue on windows on a different way, cleaning …
jjbustamante Jun 23, 2023
7b867ad
trying to fix the path issue on windows on a different way, cleaning …
jjbustamante Jun 23, 2023
0325a3d
attempt to fix windows errors
jjbustamante Jun 23, 2023
c3c11cc
fixing windows issue
jjbustamante Jun 23, 2023
144a88f
adding test case for windows
jjbustamante Jun 23, 2023
39f29d2
Removing skips on Windows
natalieparellano Jun 26, 2023
b17287b
for -> from
natalieparellano Jun 26, 2023
31060e0
fixing windows issue
jjbustamante Jun 26, 2023
6f50a75
Cleanups from PR review
natalieparellano Jun 27, 2023
e7c0a12
Fix more stuff
natalieparellano Jun 27, 2023
cead534
Update comment
natalieparellano Jun 27, 2023
e14d0d9
Variable rename and pend tests to see if acceptance is passing
natalieparellano Jun 27, 2023
03116fe
Fix units by not writing parent headers twice
natalieparellano Jun 27, 2023
84ee822
Try to fix Windows
natalieparellano Jun 27, 2023
d5cc7d9
Other small cleanups
natalieparellano Jun 28, 2023
e73699a
merging suggestions from Natalie and Joe to simplify the code
jjbustamante Jun 28, 2023
744aefc
Merge branch 'main' into fix/flatten-tar-extras-2
jjbustamante Jun 28, 2023
c5aa199
removing unused code
jjbustamante Jun 28, 2023
d5ab961
adding test coverage
jjbustamante Jun 29, 2023
f956222
Attempt to fix CodeQL errors
jjbustamante Jun 29, 2023
d9a254b
removing Focus
jjbustamante Jun 30, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions acceptance/acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2253,6 +2253,92 @@ include = [ "*.jar", "media/mountain.jpg", "/media/person.png", ]
})
})
})

when("build --buildpack <flattened buildpack>", func() {
var (
tmpDir string
flattenedPackageName string
simplePackageConfigFixtureName = "package.toml"
)

generateAggregatePackageToml := func(buildpackURI, nestedPackageName, operatingSystem string) string {
t.Helper()
packageTomlFile, err := os.CreateTemp(tmpDir, "package_aggregate-*.toml")
assert.Nil(err)

pack.FixtureManager().TemplateFixtureToFile(
"package_aggregate.toml",
packageTomlFile,
map[string]interface{}{
"BuildpackURI": buildpackURI,
"PackageName": nestedPackageName,
"OS": operatingSystem,
},
)

assert.Nil(packageTomlFile.Close())
return packageTomlFile.Name()
}

it.Before(func() {
h.SkipIf(t, !pack.SupportsFeature(invoke.BuildpackFlatten), "")
h.SkipIf(t, imageManager.HostOS() == "windows", "buildpack directories not supported on windows")

var err error
tmpDir, err = os.MkdirTemp("", "buildpack-package-flattened-tests")
assert.Nil(err)

buildpackManager = buildpacks.NewBuildModuleManager(t, assert)
buildpackManager.PrepareBuildModules(tmpDir, buildpacks.BpSimpleLayersParent, buildpacks.BpSimpleLayers)

// set up a flattened buildpack
packageTomlPath := generatePackageTomlWithOS(t, assert, pack, tmpDir, simplePackageConfigFixtureName, imageManager.HostOS())
nestedPackageName := "test/flattened-package-" + h.RandString(10)
nestedPackage := buildpacks.NewPackageImage(
t,
pack,
nestedPackageName,
packageTomlPath,
buildpacks.WithRequiredBuildpacks(buildpacks.BpSimpleLayers),
)
buildpackManager.PrepareBuildModules(tmpDir, nestedPackage)
assertImage.ExistsLocally(nestedPackageName)

aggregatePackageToml := generateAggregatePackageToml("simple-layers-parent-buildpack.tgz", nestedPackageName, imageManager.HostOS())
flattenedPackageName = "test/package-" + h.RandString(10)

_ = pack.RunSuccessfully(
"buildpack", "package", flattenedPackageName,
"-c", aggregatePackageToml,
"--flatten",
)

assertImage.ExistsLocally(flattenedPackageName)
assertImage.HasLengthLayers(flattenedPackageName, 1)
})

it.After(func() {
assert.Nil(os.RemoveAll(tmpDir))
imageManager.CleanupImages(flattenedPackageName)
})

when("--flatten", func() {
it("does not write duplicate tar files when creating the ephemeral builder", func() {
output := pack.RunSuccessfully(
"build", repoName,
"-p", filepath.Join("testdata", "mock_app"),
"--buildpack", fmt.Sprintf("docker://%s", flattenedPackageName),
"--builder", builderName,
)
// buildpack returning an empty tar file is non-deterministic,
// but we expect one of them to throw the warning
h.AssertContainsMatch(t, output, "Buildpack '(simple/layers@simple-layers-version|simple/layers/parent@simple-layers-parent-version)' is a component of a flattened buildpack that will be added elsewhere, skipping...")

// simple/layers BP exists on the builder and in the flattened buildpack
h.AssertContainsMatch(t, output, "Buildpack 'simple/layers@simple-layers-version' already exists on builder with same contents, skipping...")
})
})
})
})

when("inspecting builder", func() {
Expand Down
7 changes: 7 additions & 0 deletions acceptance/assertions/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ func (a ImageAssertionManager) HasLabelWithData(image, label, data string) {
a.assert.Contains(label, data)
}

func (a ImageAssertionManager) HasLengthLayers(image string, length int) {
a.testObject.Helper()
inspect, err := a.imageManager.InspectLocal(image)
a.assert.Nil(err)
a.assert.TrueWithMessage(len(inspect.RootFS.Layers) == length, fmt.Sprintf("expected image to have %d layers, found %d", length, len(inspect.RootFS.Layers)))
}

func (a ImageAssertionManager) RunsWithOutput(image string, expectedOutputs ...string) {
a.testObject.Helper()
containerName := "test-" + h.RandString(10)
Expand Down
4 changes: 4 additions & 0 deletions acceptance/invoke/pack.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ const (
BuildImageExtensions
RunImageExtensions
StackValidation
BuildpackFlatten
)

var featureTests = map[Feature]func(i *PackInvoker) bool{
Expand All @@ -250,6 +251,9 @@ var featureTests = map[Feature]func(i *PackInvoker) bool{
StackValidation: func(i *PackInvoker) bool {
return !i.atLeast("v0.30.0")
},
BuildpackFlatten: func(i *PackInvoker) bool {
return i.atLeast("v0.30.0")
},
}

func (i *PackInvoker) SupportsFeature(f Feature) bool {
Expand Down
177 changes: 116 additions & 61 deletions internal/builder/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package builder
import (
"archive/tar"
"bytes"
e "errors"
"fmt"
"io"
"os"
Expand All @@ -16,7 +17,6 @@ import (

"github.com/BurntSushi/toml"
"github.com/buildpacks/imgutil"
v1 "github.com/google/go-containerregistry/pkg/v1"
"github.com/pkg/errors"

"github.com/buildpacks/pack/builder"
Expand Down Expand Up @@ -47,6 +47,8 @@ const (
workspaceDir = "/workspace"
layersDir = "/layers"

emptyTarDiffID = "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"

metadataLabel = "io.buildpacks.builder.metadata"
stackLabel = "io.buildpacks.stack.id"

Expand Down Expand Up @@ -88,7 +90,8 @@ type orderTOML struct {
OrderExt dist.Order `toml:"order-extensions,omitempty"`
}

type toAdd struct {
// moduleWithDiffID is a Build Module which content was written on disk in a tar file and the content hash was calculated
type moduleWithDiffID struct {
tarPath string
diffID string
module buildpack.BuildModule
Expand Down Expand Up @@ -559,62 +562,18 @@ func (b *Builder) Save(logger logging.Logger, creatorMetadata CreatorMetadata) e
// Helpers

func (b *Builder) addExplodedModules(kind string, logger logging.Logger, tmpDir string, image imgutil.Image, additionalModules []buildpack.BuildModule, layers dist.ModuleLayers) error {
collectionToAdd := map[string]toAdd{}

type modInfo struct {
info dist.ModuleInfo
layerTar string
diffID v1.Hash
err error
collectionToAdd := map[string]moduleWithDiffID{}
splitAdditionalModules, errs := splitBuildModules(kind, tmpDir, additionalModules, logger)
if len(errs) > 0 {
return e.Join(errs...)
}

lids := make([]chan modInfo, len(additionalModules))
for i := range lids {
lids[i] = make(chan modInfo, 1)
}

for i, module := range additionalModules {
go func(i int, module buildpack.BuildModule) {
// create directory
modTmpDir := filepath.Join(tmpDir, fmt.Sprintf("%s-%s", kind, strconv.Itoa(i)))
if err := os.MkdirAll(modTmpDir, os.ModePerm); err != nil {
lids[i] <- modInfo{err: errors.Wrapf(err, "creating %s temp dir", kind)}
}

// create tar file
layerTar, err := buildpack.ToLayerTar(modTmpDir, module)
if err != nil {
lids[i] <- modInfo{err: err}
}

// generate diff id
diffID, err := dist.LayerDiffID(layerTar)
info := module.Descriptor().Info()
if err != nil {
lids[i] <- modInfo{err: errors.Wrapf(err,
"getting content hashes for %s %s",
kind,
style.Symbol(info.FullName()),
)}
}
lids[i] <- modInfo{
info: info,
layerTar: layerTar,
diffID: diffID,
}
}(i, module)
}

for i, module := range additionalModules {
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
mi := <-lids[i]
if mi.err != nil {
return mi.err
}
info, diffID, layerTar := mi.info, mi.diffID, mi.layerTar
for i, additionalModule := range splitAdditionalModules {
info, diffID, layerTar, module := additionalModule.module.Descriptor().Info(), additionalModule.diffID, additionalModule.tarPath, additionalModule.module

// check against builder layers
if existingInfo, ok := layers[info.ID][info.Version]; ok {
if existingInfo.LayerDiffID == diffID.String() {
if existingInfo.LayerDiffID == diffID {
logger.Debugf("%s %s already exists on builder with same contents, skipping...", istrings.Title(kind), style.Symbol(info.FullName()))
continue
} else {
Expand All @@ -628,23 +587,23 @@ func (b *Builder) addExplodedModules(kind string, logger logging.Logger, tmpDir
}
}

logger.Debugf(ModuleOnBuilderMessage, kind, style.Symbol(info.FullName()), style.Symbol(existingInfo.LayerDiffID), style.Symbol(diffID.String()))
logger.Debugf(ModuleOnBuilderMessage, kind, style.Symbol(info.FullName()), style.Symbol(existingInfo.LayerDiffID), style.Symbol(diffID))
}

// check against other modules to be added
if otherAdditionalMod, ok := collectionToAdd[info.FullName()]; ok {
if otherAdditionalMod.diffID == diffID.String() {
if otherAdditionalMod.diffID == diffID {
logger.Debugf("%s %s with same contents is already being added, skipping...", istrings.Title(kind), style.Symbol(info.FullName()))
continue
}

logger.Debugf(ModulePreviouslyDefinedMessage, kind, style.Symbol(info.FullName()), style.Symbol(otherAdditionalMod.diffID), style.Symbol(diffID.String()))
logger.Debugf(ModulePreviouslyDefinedMessage, kind, style.Symbol(info.FullName()), style.Symbol(otherAdditionalMod.diffID), style.Symbol(diffID))
}

// note: if same id@version is in additionalModules, last one wins (see warnings above)
collectionToAdd[info.FullName()] = toAdd{
collectionToAdd[info.FullName()] = moduleWithDiffID{
tarPath: layerTar,
diffID: diffID.String(),
diffID: diffID,
module: module,
}
}
Expand All @@ -669,7 +628,7 @@ func (b *Builder) addExplodedModules(kind string, logger logging.Logger, tmpDir
}

func (b *Builder) addFlattenedModules(kind string, logger logging.Logger, tmpDir string, image imgutil.Image, flattenModules [][]buildpack.BuildModule, layers dist.ModuleLayers) ([]buildpack.BuildModule, error) {
collectionToAdd := map[string]toAdd{}
collectionToAdd := map[string]moduleWithDiffID{}
var (
buildModuleExcluded []buildpack.BuildModule
finalTarPath string
Expand All @@ -696,7 +655,7 @@ func (b *Builder) addFlattenedModules(kind string, logger logging.Logger, tmpDir
}

for _, module := range additionalModules {
collectionToAdd[module.Descriptor().Info().FullName()] = toAdd{
collectionToAdd[module.Descriptor().Info().FullName()] = moduleWithDiffID{
tarPath: finalTarPath,
diffID: diffID.String(),
module: module,
Expand Down Expand Up @@ -1175,11 +1134,107 @@ func (b *Builder) whiteoutLayer(tmpDir string, i int, bpInfo dist.ModuleInfo) (s
return fh.Name(), nil
}

func sortKeys(collection map[string]toAdd) []string {
func sortKeys(collection map[string]moduleWithDiffID) []string {
keys := make([]string, 0, len(collection))
for k := range collection {
keys = append(keys, k)
}
sort.Strings(keys)
return keys
}

// splitBuildModules takes the given Build Modules and concurrently read their internal tar files. In
// case a flattened module is found, it will split each flattened buildpack into an individual module and skip all the empty ones
// returns an explodedBuildModule array with the Build Module information but also the diffId and the tar file on disk
func splitBuildModules(kind, tmpDir string, additionalModules []buildpack.BuildModule, logger logging.Logger) ([]moduleWithDiffID, []error) {
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
modInfoChans := make([]chan modInfo, len(additionalModules))
for i := range modInfoChans {
modInfoChans[i] = make(chan modInfo, 1)
}

for i, module := range additionalModules {
go func(i int, module buildpack.BuildModule) {
modTmpDir := filepath.Join(tmpDir, fmt.Sprintf("%s-%s", kind, strconv.Itoa(i)))
if err := os.MkdirAll(modTmpDir, os.ModePerm); err != nil {
modInfoChans[i] <- handleError(module, err, fmt.Sprintf("creating %s temp dir %s", kind, modTmpDir))
}
moduleTars, err := buildpack.ToNLayerTar(modTmpDir, module)
if err != nil {
modInfoChans[i] <- handleError(module, err, fmt.Sprintf("creating %s tar file at path %s", module.Descriptor().Info().FullName(), modTmpDir))
}
modInfoChans[i] <- modInfo{moduleTars: moduleTars}
}(i, module)
}

var result []moduleWithDiffID
var errs []error

// skip the flattened buildpacks that returned an empty tar file, their contents
// are included in a different Build Module that was split
for i, module := range additionalModules {
mi := <-modInfoChans[i]
if mi.err != nil {
errs = append(errs, mi.err)
continue
}
// moduleTar could be an individual Buildpack or flattened Buildpack that writes an empty tar on disk
for _, moduleTar := range mi.moduleTars {
explodedMod := moduleWithDiffID{tarPath: moduleTar.Path()}
diffID, err := dist.LayerDiffID(moduleTar.Path())
if err != nil {
errs = append(errs, errors.Wrapf(err, "calculating layer diffID for path %s", moduleTar.Path()))
continue
}
if diffID.String() == emptyTarDiffID {
logger.Debugf("%s %s is a component of a flattened buildpack that will be added elsewhere, skipping...", istrings.Title(kind), style.Symbol(moduleTar.Info().FullName()))
continue // we don't need to keep empty tars
}
explodedMod.diffID = diffID.String()
if moduleTar.Info().FullName() == fmt.Sprintf("%s@%s", module.Descriptor().EscapedID(), module.Descriptor().Info().Version) ||
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
moduleTar.Info().FullName() == module.Descriptor().Info().FullName() {
jjbustamante marked this conversation as resolved.
Show resolved Hide resolved
explodedMod.module = module
} else {
// we need to match the exploded modules with its corresponding BuildModule.
// this is important when flattened modules where included
for _, additionalModule := range additionalModules {
if namesMatch(additionalModule, moduleTar) {
explodedMod.module = additionalModule
break
}
}
}
result = append(result, explodedMod)
}
}

return result, errs
}

func handleError(module buildpack.BuildModule, err error, message string) modInfo {
moduleTar := errModuleTar{
module: module,
}
return modInfo{moduleTars: []buildpack.ModuleTar{moduleTar}, err: errors.Wrap(err, message)}
}

func namesMatch(module buildpack.BuildModule, moduleOnDisk buildpack.ModuleTar) bool {
return moduleOnDisk.Info().FullName() == fmt.Sprintf("%s@%s", module.Descriptor().EscapedID(), module.Descriptor().Info().Version) ||
moduleOnDisk.Info().FullName() == module.Descriptor().Info().FullName()
}

type modInfo struct {
moduleTars []buildpack.ModuleTar
err error
}

type errModuleTar struct {
module buildpack.BuildModule
}

func (e errModuleTar) Info() dist.ModuleInfo {
return e.module.Descriptor().Info()
}

func (e errModuleTar) Path() string {
return ""
}
4 changes: 4 additions & 0 deletions internal/fakes/fake_buildpack.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,7 @@ func (b *fakeBuildpack) Open() (io.ReadCloser, error) {

return tarBuilder.Reader(archive.DefaultTarWriterFactory()), nil
}

func (b *fakeBuildpack) ContainsFlattenedModules() bool {
return false
}
Loading