Extender changes for run image extension #1022

natalieparellano · 2023-02-24T21:45:48Z

Fixes #998

Branching off other feature branch for now, will repoint when that is merged

- The logic to update the default path for TOML files was repeated across phases - In general it is safe to provide default values for inputs that might not be relevant to the current phase, as these will be ignored when constructing a new service for the phase; e.g., platform.LifecycleInputs.OrderPath will be ignored when constructing a lifecycle.Exporter - As more inputs are shared across phases (e.g., analyzed.toml is now an input to the detect phase), duplicating the logic for providing default values is becoming more cumbersome Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>

…= 0.10 Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>

…kage Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>

… the platform spec Signed-off-by: Natalie Arellano <[email protected]>

…mage.extend = <true or false, default false> Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>

… should fail if the selected base image is not found in run.toml. Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>

- digest ref for run image - target data for run image Additionally the restorer will download the run image manifest & config when extend is true Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>

Because we change the media types to be oci types (vs docker types) this changes the digest of the image Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>

imgutil/layout/sparse modifies the image media types which we don't want Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano · 2023-03-28T18:25:40Z

platform/lifecycle_inputs.go

+	var skipLayers bool
+	if boolEnv(EnvSkipLayers) || boolEnv(EnvSkipRestore) {
+		skipLayers = true
+	}


Unrelated bug

natalieparellano · 2023-03-28T18:26:37Z

internal/selective/write.go

@@ -59,7 +59,7 @@ func (l Path) writeImage(img v1.Image) error {
 	if err != nil {
 		return err
 	}
-	if err := l.WriteBlob(cfgName, io.NopCloser(bytes.NewReader(cfgBlob))); err != nil {


Not sure why this stood out to me

Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano · 2023-03-28T18:28:49Z

acceptance/extender_test.go

+		if platformAPI.LessThan("0.10") {
+			continue
+		}


Speeds the tests up a little

natalieparellano · 2023-03-28T18:30:27Z

acceptance/extender_test.go

-					h.AssertStringDoesNotContain(t, secondOutput, "ca-certificates")            // shows that cache layer was used
-					h.AssertStringContains(t, secondOutput, "Hello Extensions buildpack\ncurl") // output by buildpack, shows that curl is still installed in the unpacked cached layer
+					h.AssertStringDoesNotContain(t, secondOutput, "ca-certificates")                                                             // shows that first cache layer was used
+					h.AssertStringDoesNotContain(t, secondOutput, "No cached layer found for cmd RUN apt-get update && apt-get install -y tree") // shows that second cache layer was used


Verifies we've fixed a bug that exists today where we only pull from the cache on the first Dockerfile (it has to do with non-reproducible image create time).

Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano · 2023-03-28T18:57:57Z

extender.go

 }

 //go:generate mockgen -package testmock -destination testmock/dockerfile_applier.go github.com/buildpacks/lifecycle DockerfileApplier
 type DockerfileApplier interface {
-	Apply(workspace string, digest string, dockerfiles []extend.Dockerfile, options extend.Options) error
+	ImageFor(reference string) (v1.Image, error)
+	Apply(dockerfile extend.Dockerfile, toBaseImage v1.Image, withBuildOptions extend.Options, logger log.Logger) (v1.Image, error)


Pretty significant refactor, now instead of taking a list of Dockerfiles the applier takes a single Dockerfile. This allows us to consolidate all the CNB-spec logic in the lifecycle package without it bleeding into internal/extend/kaniko.

natalieparellano · 2023-03-28T18:59:09Z

extender.go

+		return fmt.Errorf("getting image hash: %w", err)
+	}
+	toPath := filepath.Join(e.ExtendedDir, "run", imageHash.String())
+	layoutPath, err := selective.Write(toPath, empty.Index) // FIXME: this should use the imgutil layout/sparse package instead, but for some reason sparse.NewImage().Save() fails when the provided base image is already sparse


This was a bummer to discover. When we update imgutil we can remove some logic (e.g., topLayer) from this file.

natalieparellano · 2023-03-28T19:00:53Z

internal/extend/kaniko/dockerfile_applier.go

-func NewDockerfileApplier(logger log.Logger) *DockerfileApplier {
-	return &DockerfileApplier{
-		logger: logger,
+func (a *DockerfileApplier) Cleanup() error {


This could cause problems when images are extended in parallel (if the kaniko directory is re-used, which it might be). It may be safer to move this logic to the exporter.

natalieparellano · 2023-03-28T19:01:55Z

@jabrown85 @joe-kimmel-vmw this one is finally ready for review :)

joe-kimmel-vmw

I left you a couple questions but idk, i'm not actually an approver anyhow 😆 feel free to ignore me, or tell me why i'm wrong / what i'm missing

joe-kimmel-vmw · 2023-03-28T20:29:46Z

acceptance/extender_test.go

+					h.AssertStringDoesNotContain(t, secondOutput, "Did not find cache key, pulling remote image...")
+					h.AssertStringDoesNotContain(t, secondOutput, "Error while retrieving image from cache: oci")
+					h.AssertStringDoesNotContain(t, secondOutput, "ca-certificates")                                                             // shows that first cache layer was used
+					h.AssertStringDoesNotContain(t, secondOutput, "No cached layer found for cmd RUN apt-get update && apt-get install -y tree") // shows that second cache layer was used


I appreciate the options are few here, but I'm thinking that somebody could come along later and change a log string, and not realize that they've invalidated a unit test (because if that string changes, then the log output will never include it). there's no great options but things that come to mind include:

a test that asserts that the message does occur, with some comments explaining that if this test is failing then some other maintenance is needed also wherever this string occurs.

make the string be a constant in the module. it can be 'private'/lowercase if you also put the unit test in that module, which i think there's Opinions about in some circles but technically files that end in _test.go aren't compiled or linked into the executable targets anyhow?

find a non-log-output indicator. (I'm guessing if there was a good one we'd be using it already tho)

You are so right.

For line 3, ca-certificates, the positive assertion actually does exist here. For line 4, No cached layer found..., I'll add it in a43a606. For these lines, we could maybe add some sort of time-based assertion (since using the cache will be faster) and also I think they are also covered by this line since generating a new layer will cause the image SHA to be different.

For lines 1-2, there's probably no good option, and maybe we don't even need these tests anymore. In development I was trying to make sure that I used a particular code path but the end result of either was the same. For now I added constants in 007da92.

joe-kimmel-vmw · 2023-03-28T21:59:22Z

extender.go

+			group.Go(func() error {
+				return copyLayer(currentLayer, toPath)
+			})
+		case currentHash.String() == origTopLayerHash:


I can't tell if i'm just old fashioned here, but it's confusing to me to see a switch statement that is looking at different variables. in my head switch is "on" a single variable and we check different value-cases of that variable, but i guess i'm just not hip to the golang idioms.

joe-kimmel-vmw · 2023-03-28T22:01:37Z

extender.go

+
+	var (
+		configFile *v1.ConfigFile
+		rebasable  = true // for now, we don't require the initial base image to have io.buildpacks.rebasable=true


that's gonna be a tough one right? bc if we don't start that way, then technically we'd break backwards compatibility to ever require it? is the issue that basically nobody's gonna decorate their images like that, so we kind of just say "the absence of rebasable=false label is true"?

I guess i'm questioning whether the comment should imply that in the future we'll really have a default of rebaseable false, given that we're starting with a default of rebaseable true. are we skirting a spec change by saying "yeah yeah, we'll do it later"?? 😆

Fair point - I'l remove "for now" because I don't think we'll ever actually make this a requirement.

joe-kimmel-vmw · 2023-03-28T22:10:15Z

extender.go

+			buildOptions,
+			logger,
+		); err != nil {
+			return nil, fmt.Errorf("applying dockerfile to image: %w", err)


if/when we return partway through this loop what does that mean for the user's docker images? are they going to be partially extended and in an inconsistent or unusable state? is there a way to ... "roll back the transaction"? or maybe indicate to the user "our condolences for the loss of your docker image. you have a backup, right?" Or is the UX cool, here?

Great question! It should be okay - we're using the filesystem as our export target. Specifically, we're writing to the kaniko dir as a "scratch" workspace and only copying the image layers to <extended>/run/<image> after all Dockerfiles have been applied.

joe-kimmel-vmw · 2023-03-28T22:26:53Z

platform/files.go

@@ -76,7 +76,7 @@ type OSDistribution struct {
 	Version string `json:"version" toml:"version"`
 }

-// Satisfies treats optional fields (ArchVariant and Distributions) as wildcards if empty, returns true if
+// IsSatisfiedBy treats optional fields (ArchVariant and Distributions) as wildcards if empty, returns true if


You can thank my IDE :)

Signed-off-by: Natalie Arellano <[email protected]>

joe-kimmel-vmw · 2023-03-29T16:48:18Z

acceptance/extender_test.go

@@ -39,6 +39,11 @@ var (
 	extendTest           *PhaseTest
 )

+const (
+	msgPullingRemoteImage          = "Did not find cache key, pulling remote image"
+	msgErrRetrievingImageFromCache = "Error while retrieving image from cache: oci"


i see -- ideally imo these constants would be used in the (production) code that generates the logs, and then consumed from the tests, so that it's clear that those log lines are relied on someplace else outside the immediate domain, and the tests would update effortlessly with the constants.

Ohhhh I see... those log lines are emitted by kaniko :/

natalieparellano added 27 commits February 14, 2023 13:03

Read values from environment

433bf3c

Signed-off-by: Natalie Arellano <[email protected]>

Buildpack API: run.Dockerfiles are allowed instructions on versions >…

f40616e

…= 0.10 Signed-off-by: Natalie Arellano <[email protected]>

Platform API: the detector accepts a new -run flag

29a94a6

Signed-off-by: Natalie Arellano <[email protected]>

Move responsibility for validating Dockerfiles into the buildpack pac…

569f985

…kage Signed-off-by: Natalie Arellano <[email protected]>

When verifying Dockerfiles, return the new base image name if necessary

c72a10e

Signed-off-by: Natalie Arellano <[email protected]>

When determining the new runtime base image, use criteria outlined in…

4c5a564

… the platform spec Signed-off-by: Natalie Arellano <[email protected]>

Platform API: the schema of analyzed.toml is updated to include run-i…

01ab4b6

…mage.extend = <true or false, default false> Signed-off-by: Natalie Arellano <[email protected]>

TESTME: Update analyzed.toml with new run image if needed

9ba40e3

Signed-off-by: Natalie Arellano <[email protected]>

If extensions are used to switch the runtime base image, the detector…

eea3182

… should fail if the selected base image is not found in run.toml. Signed-off-by: Natalie Arellano <[email protected]>

Add fixture to test re-writing of analyzed.toml

647a3db

Signed-off-by: Natalie Arellano <[email protected]>

Move updating analyzed.toml into lifecycle package for easier testing

9157796

Signed-off-by: Natalie Arellano <[email protected]>

Platform API: the restorer will update analyzed.toml with:

62411e8

- digest ref for run image - target data for run image Additionally the restorer will download the run image manifest & config when extend is true Signed-off-by: Natalie Arellano <[email protected]>

Update acceptance/extender_test.go

362e755

Signed-off-by: Natalie Arellano <[email protected]>

The extender accepts new -extended and -kind flags

6cd5a89

Signed-off-by: Natalie Arellano <[email protected]>

Bring back selective package

97470cd

Because we change the media types to be oci types (vs docker types) this changes the digest of the image Signed-off-by: Natalie Arellano <[email protected]>

Add acceptance test for run image extension

9971560

Signed-off-by: Natalie Arellano <[email protected]>

WIP

8013390

Signed-off-by: Natalie Arellano <[email protected]>

Refactor

d546169

Signed-off-by: Natalie Arellano <[email protected]>

WIP: break me up into separate commits

3a55e4e

Signed-off-by: Natalie Arellano <[email protected]>

Validate extend config during buildpack/generate

c3f74aa

Signed-off-by: Natalie Arellano <[email protected]>

Address some TODOs and refactor

ee09eb7

Signed-off-by: Natalie Arellano <[email protected]>

Set io.buildpacks.rebasable

50c23a7

Signed-off-by: Natalie Arellano <[email protected]>

Merge branch 'main' into runext/restore-997

15c4c06

Fix merge and restore selective package

8917ca0

imgutil/layout/sparse modifies the image media types which we don't want Signed-off-by: Natalie Arellano <[email protected]>

Update analyzed.toml with digest reference or target data if needed

ea13f10

Signed-off-by: Natalie Arellano <[email protected]>

Merge branch 'main' into runext/detect-996

d0ce558

natalieparellano changed the title ~~Runext/extend 998~~ Extender changes for run image extension Mar 1, 2023

natalieparellano added 2 commits March 1, 2023 12:38

Fix acceptance

9ba92d1

Signed-off-by: Natalie Arellano <[email protected]>

Merge branch 'main' into runext/restore-997

6487081

natalieparellano added 4 commits March 1, 2023 13:29

Merge branch 'runext/detect-996' into runext/restore-997

76c7284

Don't redefine -layers

b8f029f

Signed-off-by: Natalie Arellano <[email protected]>

Merge branch 'runext/detect-996' into runext/restore-997

2d78993

Merge branch 'runext/restore-997' into runext/extend-998

7a37dfb

Base automatically changed from runext/restore-997 to main March 28, 2023 15:23

natalieparellano added 3 commits March 28, 2023 12:28

Merge branch 'main' into runext/extend-998

4bf7786

Save extended run image under <extended>/run

523ce2e

Signed-off-by: Natalie Arellano <[email protected]>

Check for nil config and make the loop simpler

63d5178

Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano marked this pull request as ready for review March 28, 2023 18:24

natalieparellano requested a review from a team as a code owner March 28, 2023 18:24

natalieparellano commented Mar 28, 2023

View reviewed changes

Fix ineffectual assignment to err

e7f0bc6

Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano commented Mar 28, 2023

View reviewed changes

Cleanup extender tests

21f9560

Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano commented Mar 28, 2023

View reviewed changes

joe-kimmel-vmw reviewed Mar 28, 2023

View reviewed changes

natalieparellano added 4 commits March 29, 2023 11:11

Fix typo

74eddd9

Signed-off-by: Natalie Arellano <[email protected]>

Add positive assertion

a43a606

Signed-off-by: Natalie Arellano <[email protected]>

Make constants for asserted log lines

007da92

Signed-off-by: Natalie Arellano <[email protected]>

Update comment

924351f

Signed-off-by: Natalie Arellano <[email protected]>

joe-kimmel-vmw reviewed Mar 29, 2023

View reviewed changes

natalieparellano merged commit 21fb915 into main Mar 31, 2023

natalieparellano deleted the runext/extend-998 branch March 31, 2023 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extender changes for run image extension #1022

Extender changes for run image extension #1022

natalieparellano commented Feb 24, 2023

natalieparellano Mar 28, 2023

natalieparellano Mar 28, 2023

natalieparellano Mar 28, 2023

natalieparellano Mar 28, 2023

natalieparellano Mar 28, 2023

natalieparellano Mar 28, 2023

natalieparellano Mar 28, 2023

natalieparellano commented Mar 28, 2023

joe-kimmel-vmw left a comment

joe-kimmel-vmw Mar 28, 2023

natalieparellano Mar 29, 2023 •

edited

Loading

joe-kimmel-vmw Mar 28, 2023

joe-kimmel-vmw Mar 28, 2023

natalieparellano Mar 29, 2023

joe-kimmel-vmw Mar 28, 2023

natalieparellano Mar 29, 2023

joe-kimmel-vmw Mar 28, 2023

natalieparellano Mar 29, 2023

joe-kimmel-vmw Mar 29, 2023

natalieparellano Mar 29, 2023

Extender changes for run image extension #1022

Extender changes for run image extension #1022

Conversation

natalieparellano commented Feb 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natalieparellano commented Mar 28, 2023

joe-kimmel-vmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natalieparellano Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natalieparellano Mar 29, 2023 •

edited

Loading