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

Verify read access while selecting best run-image mirror #1024

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
6 changes: 3 additions & 3 deletions acceptance/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ func TestAnalyzer(t *testing.T) {
analyzeDaemonFixtures = analyzeTest.targetDaemon.fixtures
analyzeRegFixtures = analyzeTest.targetRegistry.fixtures

for _, platformAPI := range api.Platform.Supported {
spec.Run(t, "acceptance-analyzer/"+platformAPI.String(), testAnalyzerFunc(platformAPI.String()), spec.Parallel(), spec.Report(report.Terminal{}))
for _, platformAPI := range []string{"0.12"} {
c0d1ngm0nk3y marked this conversation as resolved.
Show resolved Hide resolved
spec.Run(t, "acceptance-analyzer/"+platformAPI, testAnalyzerFunc(platformAPI), spec.Parallel(), spec.Report(report.Terminal{}))
}
}

Expand Down Expand Up @@ -271,7 +271,7 @@ func testAnalyzerFunc(platformAPI string) func(t *testing.T, when spec.G, it spe
output, err := cmd.CombinedOutput()

h.AssertNotNil(t, err)
expected := "ensure registry read access to some-run-image-from-run-toml"
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
expected := "failed to find accessible run image"
h.AssertStringContains(t, string(output), expected)
})
})
Expand Down
2 changes: 1 addition & 1 deletion acceptance/creator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func testCreatorFunc(platformAPI string) func(t *testing.T, when spec.G, it spec
output, err := cmd.CombinedOutput()

h.AssertNotNil(t, err)
expected := "ensure registry read access to some-run-image-from-run-toml"
expected := "failed to resolve inputs: failed to find accessible run image"
h.AssertStringContains(t, string(output), expected)
})
})
Expand Down
2 changes: 1 addition & 1 deletion cmd/lifecycle/rebaser.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func (r *rebaseCmd) setAppImage() error {
}

// for older platforms, we find the best mirror for the run image as this point
r.RunImageRef, err = md.Stack.BestRunImageMirror(registry)
r.RunImageRef, err = md.Stack.BestRunImageMirror(registry, r.LifecycleInputs.AccessChecker)
if err != nil {
return err
}
Expand Down
82 changes: 67 additions & 15 deletions platform/files.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,14 @@ import (
"github.com/buildpacks/lifecycle/internal/fsutil"

"github.com/BurntSushi/toml"
"github.com/google/go-containerregistry/pkg/authn"
"github.com/google/go-containerregistry/pkg/name"
"github.com/pkg/errors"

"github.com/buildpacks/imgutil/remote"

"github.com/buildpacks/lifecycle/api"
"github.com/buildpacks/lifecycle/auth"
"github.com/buildpacks/lifecycle/buildpack"
"github.com/buildpacks/lifecycle/internal/encoding"
"github.com/buildpacks/lifecycle/launch"
Expand Down Expand Up @@ -412,38 +416,86 @@ type RunImageForExport struct {
Mirrors []string `toml:"mirrors" json:"mirrors,omitempty"`
}

func (rm *RunImageForExport) BestRunImageMirror(registry string) (string, error) {
type ImageStrategy interface {
CheckReadAccess(repo string, keychain authn.Keychain) (bool, error)
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
}

type RemoteImageStrategy struct{}

var _ ImageStrategy = &RemoteImageStrategy{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why we need this global

Copy link
Contributor

@c0d1ngm0nk3y c0d1ngm0nk3y Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natalieparellano Actually, it is used in the main package as well (rebaser.go, analyzer.go)


func (a *RemoteImageStrategy) CheckReadAccess(repo string, keychain authn.Keychain) (bool, error) {
img, err := remote.NewImage(repo, keychain)
if err != nil {
return false, errors.Wrap(err, "failed to parse image reference")
}

return img.CheckReadAccess(), nil
}

type LocalImageStrategy struct{}

var _ ImageStrategy = &LocalImageStrategy{}

func (a *LocalImageStrategy) CheckReadAccess(_ string, _ authn.Keychain) (bool, error) {
return true, nil
}

func (rm *RunImageForExport) BestRunImageMirror(registry string, accessChecker ImageStrategy) (string, error) {
if rm.Image == "" {
return "", errors.New("missing run-image metadata")
}

runImageMirrors := []string{rm.Image}
runImageMirrors = append(runImageMirrors, rm.Mirrors...)
runImageRef, err := byRegistry(registry, runImageMirrors)

keychain, err := auth.DefaultKeychain(runImageMirrors...)
if err != nil {
return "", errors.Wrap(err, "failed to find run image")
return "", errors.Wrap(err, "unable to create keychain")
}
return runImageRef, nil
}

func (sm *StackMetadata) BestRunImageMirror(registry string) (string, error) {
return sm.RunImage.BestRunImageMirror(registry)
}
runImageRef := byRegistry(registry, runImageMirrors, accessChecker, keychain)
if runImageRef != "" {
return runImageRef, nil
}

for _, image := range runImageMirrors {
ok, err := accessChecker.CheckReadAccess(image, keychain)
if err != nil {
return "", err
}

func byRegistry(reg string, imgs []string) (string, error) {
if len(imgs) < 1 {
return "", errors.New("no images provided to search")
if ok {
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
return image, nil
}
}

for _, img := range imgs {
ref, err := name.ParseReference(img, name.WeakValidation)
return "", errors.New("failed to find accessible run image")
}

func (sm *StackMetadata) BestRunImageMirror(registry string, accessChecker ImageStrategy) (string, error) {
return sm.RunImage.BestRunImageMirror(registry, accessChecker)
}

func byRegistry(reg string, repos []string, accessChecker ImageStrategy, keychain authn.Keychain) string {
for _, repo := range repos {
ref, err := name.ParseReference(repo, name.WeakValidation)
if err != nil {
continue
}
if reg == ref.Context().RegistryStr() {
return img, nil
ok, err := accessChecker.CheckReadAccess(repo, keychain)
if err != nil {
return ""
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
}

if ok {
return repo
}
}
}
return imgs[0], nil

return ""
}

func ReadStack(stackPath string, logger log.Logger) (StackMetadata, error) {
Expand Down
17 changes: 12 additions & 5 deletions platform/files_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/buildpacks/lifecycle/api"
"github.com/buildpacks/lifecycle/buildpack"
"github.com/buildpacks/lifecycle/platform"
"github.com/buildpacks/lifecycle/platform/testhelpers"
h "github.com/buildpacks/lifecycle/testhelpers"
)

Expand Down Expand Up @@ -119,33 +120,39 @@ func testFiles(t *testing.T, when spec.G, it spec.S) {

when("repoName is dockerhub", func() {
it("returns the dockerhub image", func() {
name, err := stackMD.BestRunImageMirror("index.docker.io")
name, err := stackMD.BestRunImageMirror("index.docker.io", &testhelpers.SimpleImageStrategy{})
h.AssertNil(t, err)
h.AssertEq(t, name, "myorg/myrepo")
})
})

when("registry is gcr.io", func() {
it("returns the gcr.io image", func() {
name, err := stackMD.BestRunImageMirror("gcr.io")
name, err := stackMD.BestRunImageMirror("gcr.io", &testhelpers.SimpleImageStrategy{})
h.AssertNil(t, err)
h.AssertEq(t, name, "gcr.io/org/repo")
})

when("registry is zonal.gcr.io", func() {
it("returns the gcr image", func() {
name, err := stackMD.BestRunImageMirror("zonal.gcr.io")
name, err := stackMD.BestRunImageMirror("zonal.gcr.io", &testhelpers.SimpleImageStrategy{})
h.AssertNil(t, err)
h.AssertEq(t, name, "zonal.gcr.io/org/repo")
})
})

when("registry is missingzone.gcr.io", func() {
it("returns the run image", func() {
name, err := stackMD.BestRunImageMirror("missingzone.gcr.io")
name, err := stackMD.BestRunImageMirror("missingzone.gcr.io", &testhelpers.SimpleImageStrategy{})
h.AssertNil(t, err)
h.AssertEq(t, name, "first.com/org/repo")
})

it("returns the first readable mirror", func() {
name, err := stackMD.BestRunImageMirror("missingzone.gcr.io", &testhelpers.StubImageStrategy{AllowedRepo: "zonal.gcr.io"})
h.AssertNil(t, err)
h.AssertEq(t, name, "zonal.gcr.io/org/repo")
})
})
})

Expand All @@ -155,7 +162,7 @@ func testFiles(t *testing.T, when spec.G, it spec.S) {
})

it("skips over it", func() {
name, err := stackMD.BestRunImageMirror("gcr.io")
name, err := stackMD.BestRunImageMirror("gcr.io", &testhelpers.SimpleImageStrategy{})
h.AssertNil(t, err)
h.AssertEq(t, name, "gcr.io/myorg/myrepo")
})
Expand Down
1 change: 1 addition & 0 deletions platform/lifecycle_inputs.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
// Fields are the cumulative total of inputs across all lifecycle phases and all supported Platform APIs.
type LifecycleInputs struct {
PlatformAPI *api.Version
AccessChecker ImageStrategy
AnalyzedPath string
AppDir string
BuildConfigDir string
Expand Down
2 changes: 2 additions & 0 deletions platform/resolve_analyze_inputs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ func testResolveAnalyzeInputs(platformAPI string) func(t *testing.T, when spec.G
inputs.OutputImageRef = "some-output-image" // satisfy validation
logHandler = memory.New()
logger = &log.Logger{Handler: logHandler}
inputs.UseDaemon = true // to prevent access checking of run images
})

when("latest Platform API(s)", func() {
Expand Down Expand Up @@ -124,6 +125,7 @@ func testResolveAnalyzeInputs(platformAPI string) func(t *testing.T, when spec.G
it.Before(func() {
h.SkipIf(t, api.MustParse(platformAPI).LessThan("0.7"), "")
inputs.RunImageRef = "some-run-image" // satisfy validation
inputs.UseDaemon = false
})

when("provided destination tags are on different registries", func() {
Expand Down
2 changes: 2 additions & 0 deletions platform/resolve_create_inputs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func testResolveCreateInputs(platformAPI string) func(t *testing.T, when spec.G,
inputs.RunImageRef = "some-run-image" // satisfy validation
logHandler = memory.New()
logger = &log.Logger{Handler: logHandler}
inputs.UseDaemon = true // to prevent read access check for run image
})

when("latest Platform API(s)", func() {
Expand Down Expand Up @@ -122,6 +123,7 @@ func testResolveCreateInputs(platformAPI string) func(t *testing.T, when spec.G,
when("Platform API >= 0.7", func() {
it.Before(func() {
h.SkipIf(t, api.MustParse(platformAPI).LessThan("0.7"), "")
inputs.UseDaemon = false
})

when("provided destination tags are on different registries", func() {
Expand Down
15 changes: 9 additions & 6 deletions platform/resolve_inputs.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ var (
)

func ResolveInputs(phase LifecyclePhase, i *LifecycleInputs, logger log.Logger) error {
if i.UseDaemon || i.UseLayout {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is LocalimageStrategy correct for Daemon run images in all cases?

I thought pack build <blah> --publish , for instance, would read the run image from the target registry to place the app layers onto without pulling it down.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, but in the --publish case i.UseDaemon would be false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this comment

If the export target is the daemon, then we also expect to find the run image in a daemon. We don't allow "mixing and matching" although maybe (someday) we should.

But does --publish not imply that you export to a registry and not the daemon?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as i.UseDaemon is false I think we are good.

i.AccessChecker = &LocalImageStrategy{}
} else {
i.AccessChecker = &RemoteImageStrategy{}
}

// order of operations is important
ops := []LifecycleInputsOperation{UpdatePlaceholderPaths, ResolveAbsoluteDirPaths}
switch phase {
Expand Down Expand Up @@ -178,11 +184,8 @@ func fillRunImageFromRunTOMLIfNeeded(i *LifecycleInputs, logger log.Logger) erro
if len(runMD.Images) == 0 {
return errors.New(ErrRunImageRequiredWhenNoRunMD)
}
i.RunImageRef, err = runMD.Images[0].BestRunImageMirror(targetRegistry)
if err != nil {
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
return errors.New(ErrRunImageRequiredWhenNoRunMD)
}
return nil
i.RunImageRef, err = runMD.Images[0].BestRunImageMirror(targetRegistry, i.AccessChecker)
return err
}

// fillRunImageFromStackTOMLIfNeeded updates the provided lifecycle inputs to include the run image from stack.toml if the run image input it is missing.
Expand All @@ -199,7 +202,7 @@ func fillRunImageFromStackTOMLIfNeeded(i *LifecycleInputs, logger log.Logger) er
if err != nil {
return err
}
i.RunImageRef, err = stackMD.BestRunImageMirror(targetRegistry)
i.RunImageRef, err = stackMD.BestRunImageMirror(targetRegistry, i.AccessChecker)
if err != nil {
return errors.New(ErrRunImageRequiredWhenNoStackMD)
}
Expand Down
44 changes: 44 additions & 0 deletions platform/testhelpers/image_strategy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package testhelpers

import (
"strings"

"github.com/google/go-containerregistry/pkg/authn"

"github.com/buildpacks/lifecycle/platform"
)

type TestResource struct {
Repo string
}

var _ authn.Resource = &TestResource{}

func (t *TestResource) String() string {
return t.Repo
}

func (t *TestResource) RegistryStr() string {
return t.Repo
}

type SimpleImageStrategy struct{}

var _ platform.ImageStrategy = &SimpleImageStrategy{}
Copy link
Member

@natalieparellano natalieparellano Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused - why do we need this global (and why do we override it since it lives on inputs.AccessChecker)?


func (t *SimpleImageStrategy) CheckReadAccess(repo string, keychain authn.Keychain) (bool, error) {
resource := &TestResource{Repo: repo}
_, err := keychain.Resolve(resource)

return (err == nil), err
}

type StubImageStrategy struct {
AllowedRepo string
}

var _ platform.ImageStrategy = &StubImageStrategy{}

func (s *StubImageStrategy) CheckReadAccess(repo string, _ authn.Keychain) (bool, error) {
return strings.Contains(repo, s.AllowedRepo), nil
}