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.

Sign up for GitHub

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

Jump to bottom

Pull run image using digest reference in analyzed.toml (not image name from extensions) #2127

Merged
merged 7 commits into from
May 20, 2024
Merged

Pull run image using digest reference in analyzed.toml (not image name from extensions) #2127

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3343b9b
Pull run image using digest reference in analyzed.toml (not image nam…
natalieparellano Apr 16, 2024
2c98edd
Fix unit
natalieparellano Apr 29, 2024
328fce6
Use the run image name to determine if the run image changed,
natalieparellano Apr 29, 2024
b30f3e5
Fallback to run image name if we fail to pull run image by reference
natalieparellano Apr 29, 2024
75b16b6
Pull the new run image by name before restore, and by identifier afte…
natalieparellano May 14, 2024
3350bc9
Fix acceptance
natalieparellano May 17, 2024
6405ff4
Merge branch 'main' into fix/run-image-pull
jjbustamante May 20, 2024
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
3 changes: 2 additions & 1 deletion acceptance/acceptance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,8 @@ func testAcceptance(
h.SkipIf(t, !lifecycle.SupportsFeature(config.RunImageExtensions), "")
})

it("uses the 5 phases, and tries to pull the new run image before restore", func() {
// FIXME: now that we pull the run image AFTER the restore phases, the restorer fails to access the non-existent run image when it does restore checks
Copy link
Member

Choose a reason for hiding this comment

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

@natalieparellano What about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the acceptance tests need some more extensive re-working to make this test possible again (we need to fill in the fixture with the registry repo name for the run image before starting the test). Maybe we could make an issue for it, and update the comment to reference it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I managed to fix the acceptance test, and in so doing uncovered that I was doing wrong things previously, so thank you.

it.Pend("uses the 5 phases, and tries to pull the new run image before restore", func() {
output, _ := pack.Run(
"build", repoName,
"-p", filepath.Join("testdata", "mock_app"),
Expand Down
55 changes: 37 additions & 18 deletions internal/build/lifecycle_execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,25 +240,27 @@
}
}

l.logger.Info(style.Step("RESTORING"))
if l.opts.ClearCache && l.PlatformAPI().LessThan("0.10") {
l.logger.Info("Skipping 'restore' due to clearing cache")
} else if err := l.Restore(ctx, buildCache, kanikoCache, phaseFactory); err != nil {
return err
}

Check warning on line 248 in internal/build/lifecycle_execution.go

View check run for this annotation

Codecov / codecov/patch

internal/build/lifecycle_execution.go#L247-L248 

Added lines #L247 - L248 were not covered by tests

var (
ephemeralRunImage string
err error
)
currentRunImage := l.runImageAfterExtensions()
if l.runImageChanged() || l.hasExtensionsForRun() {
if currentRunImage == "" { // sanity check
return nil
if ephemeralRunImage, err = l.opts.FetchRunImageWithLifecycleLayer(l.runImageIdentifierAfterExtensions()); err != nil {
// If the run image was switched by extensions, the run image reference as written by the __restorer__ will be a digest reference
// that is pullable from a registry.
// However, if the run image is only extended (not switched), the run image reference as written by the __analyzer__ may be an image identifier
// (in the daemon case), and will not be pullable.
if ephemeralRunImage, err = l.opts.FetchRunImageWithLifecycleLayer(l.runImageNameAfterExtensions()); err != nil {
return err
}

Check warning on line 262 in internal/build/lifecycle_execution.go

View check run for this annotation

Codecov / codecov/patch

internal/build/lifecycle_execution.go#L256-L262 

Added lines #L256 - L262 were not covered by tests
}
if ephemeralRunImage, err = l.opts.FetchRunImageWithLifecycleLayer(currentRunImage); err != nil {
return err
}
}

l.logger.Info(style.Step("RESTORING"))
if l.opts.ClearCache && l.PlatformAPI().LessThan("0.10") {
l.logger.Info("Skipping 'restore' due to clearing cache")
} else if err := l.Restore(ctx, buildCache, kanikoCache, phaseFactory); err != nil {
return err
}

group, _ := errgroup.WithContext(context.TODO())
Expand Down Expand Up @@ -490,7 +492,7 @@
registryImages = append(registryImages, l.opts.BuilderImage)
}
if l.runImageChanged() || l.hasExtensionsForRun() {
registryImages = append(registryImages, l.runImageAfterExtensions())
registryImages = append(registryImages, l.runImageNameAfterExtensions())
}
if l.hasExtensionsForBuild() || l.hasExtensionsForRun() {
kanikoCacheBindOp = WithBinds(fmt.Sprintf("%s:%s", kanikoCache.Name(), l.mountPaths.kanikoCacheDir()))
Expand Down Expand Up @@ -915,25 +917,42 @@
return amd.RunImage.Extend
}

func (l *LifecycleExecution) runImageAfterExtensions() string {
func (l *LifecycleExecution) runImageIdentifierAfterExtensions() string {
if !l.hasExtensions() {
return l.opts.RunImage
}

Check warning on line 923 in internal/build/lifecycle_execution.go

View check run for this annotation

Codecov / codecov/patch

internal/build/lifecycle_execution.go#L922-L923 

Added lines #L922 - L923 were not covered by tests
var amd files.Analyzed
if _, err := toml.DecodeFile(filepath.Join(l.tmpDir, "analyzed.toml"), &amd); err != nil {
l.logger.Warnf("failed to parse analyzed.toml file, assuming run image identifier did not change: %s", err)
return l.opts.RunImage
}

Check warning on line 928 in internal/build/lifecycle_execution.go

View check run for this annotation

Codecov / codecov/patch

internal/build/lifecycle_execution.go#L926-L928 

Added lines #L926 - L928 were not covered by tests
if amd.RunImage == nil || amd.RunImage.Reference == "" {
// this shouldn't be reachable
l.logger.Warnf("found no run image in analyzed.toml file, assuming run image identifier did not change...")
return l.opts.RunImage
}
return amd.RunImage.Reference
}

func (l *LifecycleExecution) runImageNameAfterExtensions() string {
if !l.hasExtensions() {
return l.opts.RunImage
}
var amd files.Analyzed
if _, err := toml.DecodeFile(filepath.Join(l.tmpDir, "analyzed.toml"), &amd); err != nil {
l.logger.Warnf("failed to parse analyzed.toml file, assuming run image did not change: %s", err)
l.logger.Warnf("failed to parse analyzed.toml file, assuming run image name did not change: %s", err)

Check warning on line 943 in internal/build/lifecycle_execution.go

View check run for this annotation

Codecov / codecov/patch

internal/build/lifecycle_execution.go#L943 

Added line #L943 was not covered by tests
return l.opts.RunImage
}
if amd.RunImage == nil || amd.RunImage.Image == "" {
// this shouldn't be reachable
l.logger.Warnf("found no run image in analyzed.toml file, assuming run image did not change...")
l.logger.Warnf("found no run image in analyzed.toml file, assuming run image name did not change...")
return l.opts.RunImage
}
return amd.RunImage.Image
}

func (l *LifecycleExecution) runImageChanged() bool {
currentRunImage := l.runImageAfterExtensions()
currentRunImage := l.runImageNameAfterExtensions()
return currentRunImage != "" && currentRunImage != l.opts.RunImage
}

Expand Down
15 changes: 10 additions & 5 deletions internal/build/lifecycle_execution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
configProvider *build.PhaseConfigProvider

extensionsForBuild, extensionsForRun bool
extensionsRunImage string
extensionsRunImageName string
extensionsRunImageIdentifier string
useCreatorWithExtensions bool
)

Expand Down Expand Up @@ -149,8 +150,11 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
if extensionsForRun {
amd.RunImage.Extend = true
}
if extensionsRunImage != "" {
amd.RunImage.Image = extensionsRunImage
if extensionsRunImageName != "" {
amd.RunImage.Image = extensionsRunImageName
}
if extensionsRunImageIdentifier != "" {
amd.RunImage.Reference = extensionsRunImageIdentifier
}
f, err := os.Create(filepath.Join(tmpDir, "analyzed.toml"))
h.AssertNil(t, err)
Expand Down Expand Up @@ -660,14 +664,15 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
})

when("does not match provided run image", func() {
extensionsRunImage = "some-new-run-image"
extensionsRunImageName = "some-new-run-image"
extensionsRunImageIdentifier = "some-new-run-image-identifier"

it("pulls the new run image", func() {
err := lifecycle.Run(context.Background(), func(execution *build.LifecycleExecution) build.PhaseFactory {
return fakePhaseFactory
})
h.AssertNil(t, err)
h.AssertEq(t, fakeFetcher.calledWithArgAtCall[0], "some-new-run-image")
h.AssertEq(t, fakeFetcher.calledWithArgAtCall[0], "some-new-run-image-identifier")
h.AssertEq(t, fakeFetcher.callCount, 1)
})
})
Expand Down
Loading