Skip to content

Commit

Permalink
Merge pull request #1933 from buildpacks/fix/run-image-multi-arch
Browse files Browse the repository at this point in the history 
Ensure the downloaded os/arch always matches the expected os/arch
  • Loading branch information
@jkutner
jkutner authored Oct 30, 2023
2 parents a2b862c + e2df802 commit 13802c2
Show file tree
Hide file tree
Showing 8 changed files with 106 additions and 38 deletions.
15 changes: 13 additions & 2 deletions pkg/buildpack/downloader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ type DownloadOptions struct {
// The OS of the builder image
ImageOS string

// The OS/Architecture to download
Platform string

// Deprecated: the older alternative to buildpack URI
ImageName string

Expand Down Expand Up @@ -102,7 +105,11 @@ func (c *buildpackDownloader) Download(ctx context.Context, moduleURI string, op
case PackageLocator:
imageName := ParsePackageLocator(moduleURI)
c.logger.Debugf("Downloading %s from image: %s", kind, style.Symbol(imageName))
mainBP, depBPs, err = extractPackaged(ctx, kind, imageName, c.imageFetcher, image.FetchOptions{Daemon: opts.Daemon, PullPolicy: opts.PullPolicy})
mainBP, depBPs, err = extractPackaged(ctx, kind, imageName, c.imageFetcher, image.FetchOptions{
Daemon: opts.Daemon,
PullPolicy: opts.PullPolicy,
Platform: opts.Platform,
})
if err != nil {
return nil, nil, errors.Wrapf(err, "extracting from registry %s", style.Symbol(moduleURI))
}
Expand All @@ -113,7 +120,11 @@ func (c *buildpackDownloader) Download(ctx context.Context, moduleURI string, op
return nil, nil, errors.Wrapf(err, "locating in registry: %s", style.Symbol(moduleURI))
}

mainBP, depBPs, err = extractPackaged(ctx, kind, address, c.imageFetcher, image.FetchOptions{Daemon: opts.Daemon, PullPolicy: opts.PullPolicy})
mainBP, depBPs, err = extractPackaged(ctx, kind, address, c.imageFetcher, image.FetchOptions{
Daemon: opts.Daemon,
PullPolicy: opts.PullPolicy,
Platform: opts.Platform,
})
if err != nil {
return nil, nil, errors.Wrapf(err, "extracting from registry %s", style.Symbol(moduleURI))
}
Expand Down
24 changes: 15 additions & 9 deletions pkg/buildpack/downloader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,12 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
downloadOptions = buildpack.DownloadOptions{ImageOS: "linux"}
)

shouldFetchPackageImageWith := func(demon bool, pull image.PullPolicy) {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), packageImage.Name(), image.FetchOptions{Daemon: demon, PullPolicy: pull}).Return(packageImage, nil)
shouldFetchPackageImageWith := func(demon bool, pull image.PullPolicy, platform string) {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), packageImage.Name(), image.FetchOptions{
Daemon: demon,
PullPolicy: pull,
Platform: platform,
}).Return(packageImage, nil)
}

when("package image lives in cnb registry", func() {
Expand All @@ -141,11 +145,12 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
downloadOptions = buildpack.DownloadOptions{
RegistryName: "some-registry",
ImageOS: "linux",
Platform: "linux/amd64",
Daemon: true,
PullPolicy: image.PullAlways,
}

shouldFetchPackageImageWith(true, image.PullAlways)
shouldFetchPackageImageWith(true, image.PullAlways, "linux/amd64")
mainBP, _, err := buildpackDownloader.Download(context.TODO(), "urn:cnb:registry:example/[email protected]", downloadOptions)
h.AssertNil(t, err)
h.AssertEq(t, mainBP.Descriptor().Info().ID, "example/foo")
Expand All @@ -161,7 +166,7 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
PullPolicy: image.PullAlways,
}

shouldFetchPackageImageWith(true, image.PullAlways)
shouldFetchPackageImageWith(true, image.PullAlways, "")
mainBP, _, err := buildpackDownloader.Download(context.TODO(), "example/[email protected]", downloadOptions)
h.AssertNil(t, err)
h.AssertEq(t, mainBP.Descriptor().Info().ID, "example/foo")
Expand All @@ -185,10 +190,11 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
Daemon: true,
PullPolicy: image.PullAlways,
ImageOS: "linux",
Platform: "linux/amd64",
ImageName: "some/package:tag",
}

shouldFetchPackageImageWith(true, image.PullAlways)
shouldFetchPackageImageWith(true, image.PullAlways, "linux/amd64")
mainBP, _, err := buildpackDownloader.Download(context.TODO(), "", downloadOptions)
h.AssertNil(t, err)
h.AssertEq(t, mainBP.Descriptor().Info().ID, "example/foo")
Expand All @@ -204,7 +210,7 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
PullPolicy: image.PullAlways,
}

shouldFetchPackageImageWith(true, image.PullAlways)
shouldFetchPackageImageWith(true, image.PullAlways, "")
mainBP, _, err := buildpackDownloader.Download(context.TODO(), "", downloadOptions)
h.AssertNil(t, err)
h.AssertEq(t, mainBP.Descriptor().Info().ID, "example/foo")
Expand All @@ -220,7 +226,7 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
PullPolicy: image.PullAlways,
}

shouldFetchPackageImageWith(false, image.PullAlways)
shouldFetchPackageImageWith(false, image.PullAlways, "")
mainBP, _, err := buildpackDownloader.Download(context.TODO(), "", downloadOptions)
h.AssertNil(t, err)
h.AssertEq(t, mainBP.Descriptor().Info().ID, "example/foo")
Expand All @@ -234,7 +240,7 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
Daemon: false,
PullPolicy: image.PullAlways,
}
shouldFetchPackageImageWith(false, image.PullAlways)
shouldFetchPackageImageWith(false, image.PullAlways, "")
mainBP, _, err := buildpackDownloader.Download(context.TODO(), packageImage.Name(), downloadOptions)
h.AssertNil(t, err)
h.AssertEq(t, mainBP.Descriptor().Info().ID, "example/foo")
Expand All @@ -250,7 +256,7 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
PullPolicy: image.PullNever,
}

shouldFetchPackageImageWith(false, image.PullNever)
shouldFetchPackageImageWith(false, image.PullNever, "")
mainBP, _, err := buildpackDownloader.Download(context.TODO(), "", downloadOptions)
h.AssertNil(t, err)
h.AssertEq(t, mainBP.Descriptor().Info().ID, "example/foo")
Expand Down
49 changes: 32 additions & 17 deletions pkg/client/build.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,14 +318,28 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
return errors.Wrapf(err, "failed to fetch builder image '%s'", builderRef.Name())
}

builderOS, err := rawBuilderImage.OS()
if err != nil {
return errors.Wrapf(err, "getting builder OS")
}

builderArch, err := rawBuilderImage.Architecture()
if err != nil {
return errors.Wrapf(err, "getting builder architecture")
}

bldr, err := c.getBuilder(rawBuilderImage)
if err != nil {
return errors.Wrapf(err, "invalid builder %s", style.Symbol(opts.Builder))
}

runImageName := c.resolveRunImage(opts.RunImage, imgRegistry, builderRef.Context().RegistryStr(), bldr.DefaultRunImage(), opts.AdditionalMirrors, opts.Publish)

fetchOptions := image.FetchOptions{Daemon: !opts.Publish, PullPolicy: opts.PullPolicy}
fetchOptions := image.FetchOptions{
Daemon: !opts.Publish,
PullPolicy: opts.PullPolicy,
Platform: fmt.Sprintf("%s/%s", builderOS, builderArch),
}
if opts.Layout() {
targetRunImagePath, err := layout.ParseRefToPath(runImageName)
if err != nil {
Expand Down Expand Up @@ -361,11 +375,6 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
return err
}

imgOS, err := rawBuilderImage.OS()
if err != nil {
return errors.Wrapf(err, "getting builder OS")
}

// Default mode: if the TrustBuilder option is not set, trust the suggested builders.
if opts.TrustBuilder == nil {
opts.TrustBuilder = IsSuggestedBuilderFunc
Expand Down Expand Up @@ -396,15 +405,14 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
lifecycleImageName = fmt.Sprintf("%s:%s", internalConfig.DefaultLifecycleImageRepo, lifecycleVersion.String())
}

imgArch, err := rawBuilderImage.Architecture()
if err != nil {
return errors.Wrapf(err, "getting builder architecture")
}

lifecycleImage, err := c.imageFetcher.Fetch(
ctx,
lifecycleImageName,
image.FetchOptions{Daemon: true, PullPolicy: opts.PullPolicy, Platform: fmt.Sprintf("%s/%s", imgOS, imgArch)},
image.FetchOptions{
Daemon: true,
PullPolicy: opts.PullPolicy,
Platform: fmt.Sprintf("%s/%s", builderOS, builderArch),
},
)
if err != nil {
return fmt.Errorf("fetching lifecycle image: %w", err)
Expand Down Expand Up @@ -455,7 +463,7 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
if !c.experimental {
return fmt.Errorf("experimental features must be enabled when builder contains image extensions")
}
if imgOS == "windows" {
if builderOS == "windows" {
return fmt.Errorf("builder contains image extensions which are not supported for Windows builds")
}
if !(opts.PullPolicy == image.PullAlways) {
Expand All @@ -467,7 +475,7 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
opts.ContainerConfig.Volumes = appendLayoutVolumes(opts.ContainerConfig.Volumes, pathsConfig)
}

processedVolumes, warnings, err := processVolumes(imgOS, opts.ContainerConfig.Volumes)
processedVolumes, warnings, err := processVolumes(builderOS, opts.ContainerConfig.Volumes)
if err != nil {
return err
}
Expand Down Expand Up @@ -1024,13 +1032,19 @@ func (c *Client) fetchBuildpack(ctx context.Context, bp string, relativeBaseDir
Version: version,
}
default:
imageOS, err := builderImage.OS()
builderOS, err := builderImage.OS()
if err != nil {
return nil, nil, errors.Wrapf(err, "getting builder OS")
}

builderArch, err := builderImage.Architecture()
if err != nil {
return nil, nil, errors.Wrapf(err, "getting OS from %s", style.Symbol(builderImage.Name()))
return nil, nil, errors.Wrapf(err, "getting builder architecture")
}
downloadOptions := buildpack.DownloadOptions{
RegistryName: registry,
ImageOS: imageOS,
ImageOS: builderOS,
Platform: fmt.Sprintf("%s/%s", builderOS, builderArch),
RelativeBaseDir: relativeBaseDir,
Daemon: !publish,
PullPolicy: pullPolicy,
Expand Down Expand Up @@ -1068,6 +1082,7 @@ func (c *Client) fetchBuildpackDependencies(ctx context.Context, bp string, pack
mainBP, deps, err := c.buildpackDownloader.Download(ctx, dep.URI, buildpack.DownloadOptions{
RegistryName: downloadOptions.RegistryName,
ImageOS: downloadOptions.ImageOS,
Platform: downloadOptions.Platform,
Daemon: downloadOptions.Daemon,
PullPolicy: downloadOptions.PullPolicy,
RelativeBaseDir: filepath.Join(bp, packageCfg.Buildpack.URI),
Expand Down
11 changes: 7 additions & 4 deletions pkg/client/build_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,8 @@ api = "0.2"
Version: "child.buildpack.version",
},
})
args := fakeImageFetcher.FetchCalls[fakePackage.Name()]
h.AssertEq(t, args.Platform, "linux/amd64")
})

it("fails when no metadata label on package", func() {
Expand Down Expand Up @@ -2085,11 +2087,12 @@ api = "0.2"
}))
h.AssertEq(t, fakeLifecycle.Opts.Publish, true)

args := fakeImageFetcher.FetchCalls["default/run"]
h.AssertEq(t, args.Daemon, false)

args = fakeImageFetcher.FetchCalls[defaultBuilderName]
args := fakeImageFetcher.FetchCalls[defaultBuilderName]
h.AssertEq(t, args.Daemon, true)

args = fakeImageFetcher.FetchCalls["default/run"]
h.AssertEq(t, args.Daemon, false)
h.AssertEq(t, args.Platform, "linux/amd64")
})

when("builder is untrusted", func() {
Expand Down
12 changes: 9 additions & 3 deletions pkg/client/create_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,14 +256,20 @@ func (c *Client) addExtensionsToBuilder(ctx context.Context, opts CreateBuilderO
func (c *Client) addConfig(ctx context.Context, kind string, config pubbldr.ModuleConfig, opts CreateBuilderOptions, bldr *builder.Builder) error {
c.logger.Debugf("Looking up %s %s", kind, style.Symbol(config.DisplayString()))

imageOS, err := bldr.Image().OS()
builderOS, err := bldr.Image().OS()
if err != nil {
return errors.Wrapf(err, "getting OS from %s", style.Symbol(bldr.Image().Name()))
return errors.Wrapf(err, "getting builder OS")
}
builderArch, err := bldr.Image().Architecture()
if err != nil {
return errors.Wrapf(err, "getting builder architecture")
}

mainBP, depBPs, err := c.buildpackDownloader.Download(ctx, config.URI, buildpack.DownloadOptions{
Daemon: !opts.Publish,
ImageName: config.ImageName,
ImageOS: imageOS,
ImageOS: builderOS,
Platform: fmt.Sprintf("%s/%s", builderOS, builderArch),
ModuleKind: kind,
PullPolicy: opts.PullPolicy,
RegistryName: opts.Registry,
Expand Down
14 changes: 12 additions & 2 deletions pkg/client/create_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -843,12 +843,22 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
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)
mockBuildpackDownloader.EXPECT().Download(gomock.Any(), "https://example.fake/bp-one-with-api-4.tgz", gomock.Any()).DoAndReturn(
func(ctx context.Context, buildpackURI string, opts buildpack.DownloadOptions) (buildpack.BuildModule, []buildpack.BuildModule, error) {
// test options
h.AssertEq(t, opts.Platform, "linux/amd64")
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)
mockBuildpackDownloader.EXPECT().Download(gomock.Any(), "https://example.fake/ext-one-with-api-9.tgz", gomock.Any()).DoAndReturn(
func(ctx context.Context, buildpackURI string, opts buildpack.DownloadOptions) (buildpack.BuildModule, []buildpack.BuildModule, error) {
// test options
h.AssertEq(t, opts.Platform, "linux/amd64")
return extension, nil, nil
})

successfullyCreateDeterministicBuilder()

Expand Down
17 changes: 16 additions & 1 deletion pkg/client/rebase.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package client

import (
"context"
"fmt"
"os"
"path/filepath"

Expand Down Expand Up @@ -60,6 +61,16 @@ func (c *Client) Rebase(ctx context.Context, opts RebaseOptions) error {
return err
}

appOS, err := appImage.OS()
if err != nil {
return errors.Wrapf(err, "getting app OS")
}

appArch, err := appImage.Architecture()
if err != nil {
return errors.Wrapf(err, "getting app architecture")
}

var md files.LayersMetadataCompat
if ok, err := dist.GetLabel(appImage, platform.LifecycleMetadataLabel, &md); err != nil {
return err
Expand Down Expand Up @@ -90,7 +101,11 @@ func (c *Client) Rebase(ctx context.Context, opts RebaseOptions) error {
return errors.New("run image must be specified")
}

baseImage, err := c.imageFetcher.Fetch(ctx, runImageName, image.FetchOptions{Daemon: !opts.Publish, PullPolicy: opts.PullPolicy})
baseImage, err := c.imageFetcher.Fetch(ctx, runImageName, image.FetchOptions{
Daemon: !opts.Publish,
PullPolicy: opts.PullPolicy,
Platform: fmt.Sprintf("%s/%s", appOS, appArch),
})
if err != nil {
return err
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/client/rebase_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,8 @@ func testRebase(t *testing.T, when spec.G, it spec.S) {
h.AssertEq(t, fakeAppImage.Base(), "some/run")
lbl, _ := fakeAppImage.Label("io.buildpacks.lifecycle.metadata")
h.AssertContains(t, lbl, `"runImage":{"topLayer":"remote-top-layer-sha","reference":"remote-digest"`)
args := fakeImageFetcher.FetchCalls["some/run"]
h.AssertEq(t, args.Platform, "linux/amd64")
})
})
})
Expand Down

0 comments on commit 13802c2

Please sign in to comment.