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

Ensure the downloaded os/arch always matches the expected os/arch #1933

Merged
merged 3 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 13 additions & 2 deletions pkg/buildpack/downloader.go
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -318,14 +318,28 @@
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")
}

Check warning on line 324 in pkg/client/build.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/build.go#L323-L324

Added lines #L323 - L324 were not covered by tests

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

Check warning on line 329 in pkg/client/build.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/build.go#L328-L329

Added lines #L328 - L329 were not covered by tests

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 @@
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 @@
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 @@
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 @@
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 @@
Version: version,
}
default:
imageOS, err := builderImage.OS()
builderOS, err := builderImage.OS()
if err != nil {
return nil, nil, errors.Wrapf(err, "getting builder OS")
}

Check warning on line 1038 in pkg/client/build.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/build.go#L1037-L1038

Added lines #L1037 - L1038 were not covered by tests

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")

Check warning on line 1042 in pkg/client/build.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/build.go#L1042

Added line #L1042 was not covered by tests
}
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 @@
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -256,14 +256,20 @@
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")

Check warning on line 261 in pkg/client/create_builder.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/create_builder.go#L261

Added line #L261 was not covered by tests
}
builderArch, err := bldr.Image().Architecture()
if err != nil {
return errors.Wrapf(err, "getting builder architecture")
}

Check warning on line 266 in pkg/client/create_builder.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/create_builder.go#L265-L266

Added lines #L265 - L266 were not covered by tests

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
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

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

Expand Down Expand Up @@ -60,6 +61,16 @@
return err
}

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

Check warning on line 67 in pkg/client/rebase.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/rebase.go#L66-L67

Added lines #L66 - L67 were not covered by tests

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

Check warning on line 72 in pkg/client/rebase.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/rebase.go#L71-L72

Added lines #L71 - L72 were not covered by tests

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 @@
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
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
Loading