Skip to content

Commit

Permalink
Ensure read access when resolving run image location
Browse files Browse the repository at this point in the history
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
  • Loading branch information
pbusko committed Jan 3, 2024
1 parent c08b289 commit 4566bb5
Show file tree
Hide file tree
Showing 11 changed files with 179 additions and 17 deletions.
19 changes: 19 additions & 0 deletions internal/fakes/fake_access_checker.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package fakes

type FakeAccessChecker struct {
RegistriesToFail []string
}

func NewFakeAccessChecker() *FakeAccessChecker {
return &FakeAccessChecker{}
}

func (f *FakeAccessChecker) Check(repo string) bool {
for _, toFail := range f.RegistriesToFail {
if toFail == repo {
return false
}
}

return true
}
2 changes: 1 addition & 1 deletion pkg/client/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
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)
runImageName := c.resolveRunImage(opts.RunImage, imgRegistry, builderRef.Context().RegistryStr(), bldr.DefaultRunImage(), opts.AdditionalMirrors, opts.Publish, c.accessChecker)

fetchOptions := image.FetchOptions{
Daemon: !opts.Publish,
Expand Down
3 changes: 3 additions & 0 deletions pkg/client/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func testBuild(t *testing.T, when spec.G, it spec.S) {
subject *Client
fakeImageFetcher *ifakes.FakeImageFetcher
fakeLifecycle *ifakes.FakeLifecycle
fakeAccessChecker *ifakes.FakeAccessChecker
defaultBuilderStackID = "some.stack.id"
defaultWindowsBuilderStackID = "some.windows.stack.id"
defaultBuilderImage *fakes.Image
Expand All @@ -80,6 +81,7 @@ func testBuild(t *testing.T, when spec.G, it spec.S) {
var err error

fakeImageFetcher = ifakes.NewFakeImageFetcher()
fakeAccessChecker = ifakes.NewFakeAccessChecker()
fakeLifecycle = &ifakes.FakeLifecycle{}

tmpDir, err = os.MkdirTemp("", "build-test")
Expand Down Expand Up @@ -136,6 +138,7 @@ func testBuild(t *testing.T, when spec.G, it spec.S) {
logger: logger,
imageFetcher: fakeImageFetcher,
downloader: blobDownloader,
accessChecker: fakeAccessChecker,
lifecycleExecutor: fakeLifecycle,
docker: docker,
buildpackDownloader: buildpackDownloader,
Expand Down
20 changes: 20 additions & 0 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ type BuildpackDownloader interface {
Download(ctx context.Context, buildpackURI string, opts buildpack.DownloadOptions) (buildpack.BuildModule, []buildpack.BuildModule, error)
}

//go:generate mockgen -package testmocks -destination ../testmocks/mock_access_checker.go github.com/buildpacks/pack/pkg/client AccessChecker

// AccessChecker is an interface for checking remote images for read access
type AccessChecker interface {
Check(repo string) bool
}

// Client is an orchestration object, it contains all parameters needed to
// build an app image using Cloud Native Buildpacks.
// All settings on this object should be changed through ClientOption functions.
Expand All @@ -90,6 +97,7 @@ type Client struct {
keychain authn.Keychain
imageFactory ImageFactory
imageFetcher ImageFetcher
accessChecker AccessChecker
downloader BlobDownloader
lifecycleExecutor LifecycleExecutor
buildpackDownloader BuildpackDownloader
Expand Down Expand Up @@ -125,6 +133,14 @@ func WithFetcher(f ImageFetcher) Option {
}
}

// WithAccessChecker supply your own AccessChecker.
// A AccessChecker returns true if an image is accessible for reading.
func WithAccessChecker(f AccessChecker) Option {
return func(c *Client) {
c.accessChecker = f
}
}

// WithDownloader supply your own downloader.
// A Downloader is used to gather buildpacks from both remote urls, or local sources.
func WithDownloader(d BlobDownloader) Option {
Expand Down Expand Up @@ -225,6 +241,10 @@ func NewClient(opts ...Option) (*Client, error) {
}
}

if client.accessChecker == nil {
client.accessChecker = image.NewAccessChecker(client.logger, client.keychain)
}

if client.buildpackDownloader == nil {
client.buildpackDownloader = buildpack.NewDownloader(
client.logger,
Expand Down
10 changes: 10 additions & 0 deletions pkg/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/sclevine/spec"
"github.com/sclevine/spec/report"

"github.com/buildpacks/pack/pkg/image"
"github.com/buildpacks/pack/pkg/logging"
"github.com/buildpacks/pack/pkg/testmocks"
h "github.com/buildpacks/pack/testhelpers"
Expand Down Expand Up @@ -122,4 +123,13 @@ func testClient(t *testing.T, when spec.G, it spec.S) {
h.AssertEq(t, cl.registryMirrors, registryMirrors)
})
})

when("#WithAccessChecker", func() {
it("uses AccessChecker provided", func() {
ac := &image.Checker{}
cl, err := NewClient(WithAccessChecker(ac))
h.AssertNil(t, err)
h.AssertSameInstance(t, cl.accessChecker, ac)
})
})
}
21 changes: 15 additions & 6 deletions pkg/client/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (c *Client) parseTagReference(imageName string) (name.Reference, error) {
return ref, nil
}

func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, runImageMetadata builder.RunImageMetadata, additionalMirrors map[string][]string, publish bool) string {
func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, runImageMetadata builder.RunImageMetadata, additionalMirrors map[string][]string, publish bool, accessChecker AccessChecker) string {
if runImage != "" {
c.logger.Debugf("Using provided run-image %s", style.Symbol(runImage))
return runImage
Expand All @@ -44,6 +44,7 @@ func (c *Client) resolveRunImage(runImage, imgRegistry, bldrRegistry string, run
runImageMetadata.Image,
runImageMetadata.Mirrors,
additionalMirrors[runImageMetadata.Image],
accessChecker,
)

switch {
Expand Down Expand Up @@ -107,8 +108,8 @@ func contains(slc []string, v string) bool {
return false
}

func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string) string {
runImageList := append(append(append([]string{}, preferredMirrors...), runImage), mirrors...)
func getBestRunMirror(registry string, runImage string, mirrors []string, preferredMirrors []string, accessChecker AccessChecker) string {
runImageList := filterImageList(append(append(append([]string{}, preferredMirrors...), runImage), mirrors...), accessChecker)
for _, img := range runImageList {
ref, err := name.ParseReference(img, name.WeakValidation)
if err != nil {
Expand All @@ -119,9 +120,17 @@ func getBestRunMirror(registry string, runImage string, mirrors []string, prefer
}
}

if len(preferredMirrors) > 0 {
return preferredMirrors[0]
return runImageList[0]
}

func filterImageList(imageList []string, accessChecker AccessChecker) []string {
var accessibleImages []string

for i, img := range imageList {
if accessChecker.Check(img) {
accessibleImages = append(accessibleImages, imageList[i])
}
}

return runImage
return accessibleImages
}
35 changes: 28 additions & 7 deletions pkg/client/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/sclevine/spec/report"

"github.com/buildpacks/pack/internal/builder"
ifakes "github.com/buildpacks/pack/internal/fakes"
"github.com/buildpacks/pack/pkg/logging"
h "github.com/buildpacks/pack/testhelpers"
)
Expand All @@ -31,6 +32,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
gcrRegistry string
gcrRunMirror string
stackInfo builder.StackMetadata
accessChecker *ifakes.FakeAccessChecker
assert = h.NewAssertionManager(t)
)

Expand All @@ -54,27 +56,28 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
},
},
}
accessChecker = ifakes.NewFakeAccessChecker()
})

when("passed specific run image", func() {
it("selects that run image", func() {
runImgFlag := "flag/passed-run-image"
runImageName := subject.resolveRunImage(runImgFlag, defaultRegistry, "", stackInfo.RunImage, nil, false)
runImageName := subject.resolveRunImage(runImgFlag, defaultRegistry, "", stackInfo.RunImage, nil, false, accessChecker)
assert.Equal(runImageName, runImgFlag)
})
})

when("publish is true", func() {
it("defaults to run-image in registry publishing to", func() {
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true)
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true, accessChecker)
assert.Equal(runImageName, gcrRunMirror)
})

it("prefers config defined run image mirror to stack defined run image mirror", func() {
configMirrors := map[string][]string{
runImageName: {defaultRegistry + "/unique-run-img"},
}
runImageName := subject.resolveRunImage("", defaultRegistry, "", stackInfo.RunImage, configMirrors, true)
runImageName := subject.resolveRunImage("", defaultRegistry, "", stackInfo.RunImage, configMirrors, true, accessChecker)
assert.NotEqual(runImageName, defaultMirror)
assert.Equal(runImageName, defaultRegistry+"/unique-run-img")
})
Expand All @@ -83,7 +86,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
configMirrors := map[string][]string{
runImageName: {defaultRegistry + "/unique-run-img"},
}
runImageName := subject.resolveRunImage("", "test.registry.io", "", stackInfo.RunImage, configMirrors, true)
runImageName := subject.resolveRunImage("", "test.registry.io", "", stackInfo.RunImage, configMirrors, true, accessChecker)
assert.NotEqual(runImageName, defaultMirror)
assert.Equal(runImageName, defaultRegistry+"/unique-run-img")
})
Expand All @@ -92,7 +95,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
// If publish is false, we are using the local daemon, and want to match to the builder registry
when("publish is false", func() {
it("defaults to run-image in registry publishing to", func() {
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, false)
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, false, accessChecker)
assert.Equal(runImageName, defaultMirror)
assert.NotEqual(runImageName, gcrRunMirror)
})
Expand All @@ -101,7 +104,7 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
configMirrors := map[string][]string{
runImageName: {defaultRegistry + "/unique-run-img"},
}
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, configMirrors, false)
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, configMirrors, false, accessChecker)
assert.NotEqual(runImageName, defaultMirror)
assert.Equal(runImageName, defaultRegistry+"/unique-run-img")
})
Expand All @@ -110,10 +113,28 @@ func testCommon(t *testing.T, when spec.G, it spec.S) {
configMirrors := map[string][]string{
runImageName: {defaultRegistry + "/unique-run-img"},
}
runImageName := subject.resolveRunImage("", defaultRegistry, "test.registry.io", stackInfo.RunImage, configMirrors, false)
runImageName := subject.resolveRunImage("", defaultRegistry, "test.registry.io", stackInfo.RunImage, configMirrors, false, accessChecker)
assert.NotEqual(runImageName, defaultMirror)
assert.Equal(runImageName, defaultRegistry+"/unique-run-img")
})
})

when("desirable run-image is not accessible", func() {
it.Before(func() {
accessChecker.RegistriesToFail = []string{
gcrRunMirror,
stackInfo.RunImage.Image,
}
})

it.After(func() {
accessChecker.RegistriesToFail = nil
})

it("selects the first accessible run-image", func() {
runImageName := subject.resolveRunImage("", gcrRegistry, defaultRegistry, stackInfo.RunImage, nil, true, accessChecker)
assert.Equal(runImageName, defaultMirror)
})
})
})
}
4 changes: 3 additions & 1 deletion pkg/client/rebase.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ func (c *Client) Rebase(ctx context.Context, opts RebaseOptions) error {
"",
runImageMD,
opts.AdditionalMirrors,
opts.Publish)
opts.Publish,
c.accessChecker,
)

if runImageName == "" {
return errors.New("run image must be specified")
Expand Down
7 changes: 5 additions & 2 deletions pkg/client/rebase_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func testRebase(t *testing.T, when spec.G, it spec.S) {
when("#Rebase", func() {
var (
fakeImageFetcher *ifakes.FakeImageFetcher
fakeAccessChecker *ifakes.FakeAccessChecker
subject *Client
fakeAppImage *fakes.Image
fakeRunImage *fakes.Image
Expand All @@ -37,6 +38,7 @@ func testRebase(t *testing.T, when spec.G, it spec.S) {

it.Before(func() {
fakeImageFetcher = ifakes.NewFakeImageFetcher()
fakeAccessChecker = ifakes.NewFakeAccessChecker()

fakeAppImage = fakes.NewImage("some/app", "", &fakeIdentifier{name: "app-image"})
h.AssertNil(t, fakeAppImage.SetLabel("io.buildpacks.lifecycle.metadata",
Expand All @@ -54,8 +56,9 @@ func testRebase(t *testing.T, when spec.G, it spec.S) {

fakeLogger := logging.NewLogWithWriters(&out, &out)
subject = &Client{
logger: fakeLogger,
imageFetcher: fakeImageFetcher,
logger: fakeLogger,
imageFetcher: fakeImageFetcher,
accessChecker: fakeAccessChecker,
}
})

Expand Down
41 changes: 41 additions & 0 deletions pkg/image/access_checker.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package image

import (
"github.com/buildpacks/imgutil/remote"
"github.com/google/go-containerregistry/pkg/authn"

"github.com/buildpacks/pack/pkg/logging"
)

type Checker struct {
logger logging.Logger
keychain authn.Keychain
}

func NewAccessChecker(logger logging.Logger, keychain authn.Keychain) *Checker {
checker := &Checker{
logger: logger,
keychain: keychain,
}

if checker.keychain == nil {
checker.keychain = authn.DefaultKeychain
}

Check warning on line 23 in pkg/image/access_checker.go

View check run for this annotation

Codecov / codecov/patch

pkg/image/access_checker.go#L22-L23

Added lines #L22 - L23 were not covered by tests

return checker
}

func (c *Checker) Check(repo string) bool {
img, err := remote.NewImage(repo, c.keychain)
if err != nil {
return false
}

Check warning on line 32 in pkg/image/access_checker.go

View check run for this annotation

Codecov / codecov/patch

pkg/image/access_checker.go#L31-L32

Added lines #L31 - L32 were not covered by tests

if ok, err := img.CheckReadAccess(); ok {
c.logger.Debugf("CheckReadAccess succeeded for the run image %s", repo)
return true

Check warning on line 36 in pkg/image/access_checker.go

View check run for this annotation

Codecov / codecov/patch

pkg/image/access_checker.go#L35-L36

Added lines #L35 - L36 were not covered by tests
} else {
c.logger.Debugf("CheckReadAccess failed for the run image %s, error: %s", repo, err.Error())
return false
}
}
34 changes: 34 additions & 0 deletions pkg/image/access_checker_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package image_test

import (
"bytes"
"testing"

"github.com/buildpacks/lifecycle/auth"
"github.com/sclevine/spec"
"github.com/sclevine/spec/report"

"github.com/buildpacks/pack/pkg/image"
"github.com/buildpacks/pack/pkg/logging"
h "github.com/buildpacks/pack/testhelpers"
)

func TestChecker(t *testing.T) {
spec.Run(t, "Checker", testChecker, spec.Report(report.Terminal{}))
}

func testChecker(t *testing.T, when spec.G, it spec.S) {
when("#Check", func() {
it("fails when checking dummy image", func() {
buf := &bytes.Buffer{}

keychain, err := auth.DefaultKeychain("pack.test/dummy")
h.AssertNil(t, err)

ic := image.NewAccessChecker(logging.NewSimpleLogger(buf), keychain)

h.AssertFalse(t, ic.Check("pack.test/dummy"))
h.AssertContains(t, buf.String(), "DEBUG: CheckReadAccess failed for the run image pack.test/dummy")
})
})
}

0 comments on commit 4566bb5

Please sign in to comment.