Dockerfiles POC #802

natalieparellano · 2022-02-15T22:34:18Z

This PR updates #786 and integrates the work done in #797. It also pulls in code from https://github.com/redhat-buildpacks/poc/tree/main/kaniko/code.

In theory, using this branch it is possible to perform a build that uses extensions to extend the build and/or run image and get an app image that is based off the extended run image.

To see it in action, run test-extender.sh (use REGISTRY_HOST to configure the registry where the extended images should be saved).

Changes of note:

extender/spec.md for proposed platform <-> extender interface
new cmd/extender package
new extender package
- the DockerfileApplier interface (this could allow platforms to supply their own implementation, using extender as a library to provide CNB logic)
changes to detector.go and builder.go
changes to buildpack package:
- buildpack/build.go
- buildpack/detect.go
- buildpack/dockerfile.go
- buildpack/extension.go

Significant work still to be done:

kaniko implementation:
- Enable and verify caching
- Enable "ignore paths" provided by the end user
- When extending the build image, don't push and pull the image between Dockerfile builds (we can just parse the commands in the Dockerfile)
- Decide if we need an in-process registry
Decide what build.Dockerfiles should be considered "valid" (e.g., they must start with ARG #{base_image})
Set the correct value of io.buildpacks.rebasable
Run genpkgs
Exclude genpkgs from the final image
...much more

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

…pply to both build and run will be repeated in the list. Signed-off-by: Natalie Arellano <[email protected]>

Signed-off-by: Anthony Emengo <[email protected]>

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

Signed-off-by: Jesse Brown <[email protected]>

…t a final image Signed-off-by: Natalie Arellano <[email protected]>

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

Add argument for target image Signed-off-by: Natalie Arellano <[email protected]>

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

jabrown85

This looks like a great start to me!

jabrown85 · 2022-02-15T22:40:07Z

.gitignore

+extender/testdata/layers/group.toml
+extender/testdata/layers/plan.toml
+extender/testdata/layers/report.toml
+extender/testdata/layers/samples_*/*


nit: newline

Suggested change

extender/testdata/layers/samples_*/*

extender/testdata/layers/samples_*/*

jabrown85 · 2022-02-15T22:52:49Z

cmd/lifecycle/builder.go

 	if err != nil {
 		return cmd.FailErrCode(err, ba.platform.CodeFor(platform.BuildError), "build")
 	}

 	builder := &lifecycle.Builder{
 		AppDir:         ba.appDir,
-		LayersDir:      ba.layersDir,
+		LayersDir:      ba.layersDir, // TODO: when there are extensions, this should default to something other than /layers - maybe /layers/config/ext


Is that up to the platform? I guess having a different default is probably good for documentation kind of reason.

jabrown85 · 2022-02-15T22:53:18Z

cmd/lifecycle/detector.go

-		da.buildpacksDir,
-		da.platform,
-	)
+	detector, err := lifecycle.NewDetector(buildpack.DetectConfig{


nit: the old formatting was better IMO

jabrown85 · 2022-02-15T22:55:37Z

cmd/lifecycle/detector.go

-			if err != nil {
-				return cmd.FailErr(err, fmt.Sprintf("lookup buildpack.toml for buildpack '%s'", groupBp.String()))
+			var buildable buildpack.Buildable
+			if groupBp.Extension {


Future note: This feels like a place we could improve the API. If we had an api like multistore.Lookup(groupBp) could we remove the branching here and anywhere else we need to lookup?

jabrown85 · 2022-02-15T22:59:36Z

exporter.go

@@ -284,6 +291,67 @@ func (e *Exporter) addAppLayers(opts ExportOptions, slices []layers.Slice, meta
 	return nil
 }

+func (e *Exporter) addExtenderLayers(opts ExportOptions, meta *platform.LayersMetadata) error {


I think we can remove this entirely if we say that extensions produce images in registries, at least. If we intend on supporting non-registry flows, then we would want to be able to capture and use these layers as the run-image during exporter.

I don't know if we've made that decision. I'm fine leaving this here as POC idea until we do, though it is incorrect in implementation.

jabrown85 · 2022-02-15T23:05:12Z

extender/testdata/cnb/buildpacks/samples_use_curl/0.0.1/buildpack.toml

+name = "Buildpack using curl"
+
+[[stacks]]
+id = "*"


nit:newline

Suggested change

id = "*"

id = "*"

jabrown85 · 2022-02-15T23:10:13Z

extender/testdata/cnb/ext/samples_curl/0.0.1/bin/build

+FROM \${base_image}
+
+# TODO: why wasn't this needed before?
+USER root


I believe this was not needed before because we were executing the instructions on the builder which was running as root.

This raises a good question/concern. If, for run extensions, we apply Dockerfile after Dockerfile and the last Dockerfile leaves the USER root as the last instruction...is that ok? That feels wrong as one of the pillars of CNB is that the resulting image should not be root IMO. It gets a bit muddier when you think about full run image replacements as well. Does the final image need to be using the same CNB_USER_ID as the original run image reference to make things happy/correct?

I'm sure @sclevine has some thoughts on this...

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

BarDweller · 2022-02-22T14:49:29Z

Ran into an issue where if the Dockerfile being processed attempts to pull any new images, the pull failed because kaniko had a null/empty default platform, resulting in errors like..

time="2022-02-17T18:34:55Z" level=info msg="Retrieving image manifest redhat/ubi8-minimal:latest"
time="2022-02-17T18:34:55Z" level=info msg="Retrieving image redhat/ubi8-minimal:latest from registry index.docker.io"
time="2022-02-17T18:34:55Z" level=error msg="Error parsing the serverURL" error="docker-credential-ecr-login can only be used with Amazon Elastic Container Registry." serverURL=index.docker.io
ERROR: no child with platform  in index redhat/ubi8-minimal:latest

(Note the empty platform name in the final ERROR output)

Did some digging, found kaniko treats the CustomPlatform option differently to all it's others, and essentially requires it to be set to the default platform if the value is not set by a user.

Have created a quick PR for the branch of this PR to fix that.. #804

Signed-off-by: Ozzy Osborne <[email protected]>

Kaniko will now push cache layerse into the registry at `$REGISTRY_HOST/extended/buildimage/cache` and `$REGISTRY_HOST/extended/runimage/cache` - allowing for faster builds when the layer can be re-used. Signed-off-by: Jesse Brown <[email protected]>

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

See buildpacks/spec#286 - this was patched in a newer lifecycle, but is missing in the POC Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano · 2022-04-14T16:17:46Z

Some thoughts on next steps...

While the POC seems to work, there are a number of features missing. Some are procedural / cosmetic, but there are a few harder problems to solve:

isolating registry credentials from Dockerfiles
running syft on the extended image (see related comment)
validations on the extended image (is it rebasable, did the user change, etc.)

For isolating registry credentials, the platform would need to pull all the base images from the registry before starting the extender container. The base images could be provided to kaniko in either a local registry, or in a cache directory - but the latter seems not performant as kaniko expects to find the layers in a single tar, which would make it difficult to re-use layers across images. We could see about PR'ing kaniko the ability to lookup layers from a blob store.

Another problem is outputs - kaniko can output to either a registry or to an oci layout directory. The former might be more convenient as it is readily chain-able as an input to the next Dockerfile. But there are other headaches associated with creating a local registry (such as networking) that would need to be thought through.

The following diagram illustrates how this might work. It assumes we are using a local registry but the idea is the same if we try to go the other route (changing how --cache-dir behaves in kaniko, making in chain-able). The other discussions currently underway in the project (1, 2) might influence which route we decide to take. cc @jjbustamante @jromero @samj1912

natalieparellano · 2022-04-14T16:19:07Z

Also just to add - buildpacks/spec#298 (comment) proposes a phased implementation that would let us get something out that would deliver value while we work out some of these thornier issues.

…uilder (#845) Signed-off-by: Natalie Arellano <[email protected]>

…deps (#844) Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano · 2022-06-29T19:14:53Z

Closing as I think this POC has served its purpose. For further details on the "real" implementation please follow buildpacks/rfcs#224

natalieparellano and others added 26 commits December 13, 2021 16:53

WIP

c0df511

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

WIP

006e371

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

WIP

a32e8e6

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

WIP

8f9a532

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

WIP

7787bec

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

Fix

8cec26a

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

Rename Buildpack -> Buildable

1b07d9e

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

Dockerfile has a type that is either build or run. Dockerfiles that a…

ad323ee

…pply to both build and run will be repeated in the list. Signed-off-by: Natalie Arellano <[email protected]>

Move poc code into here

cb587b3

Signed-off-by: Anthony Emengo <[email protected]>

wip: Anthony + Natalie: merging in charles POC kaniko

115e7f3

Signed-off-by: Anthony Emengo <[email protected]>

Add script to test extender and run it for extending the build image

d5fb974

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

Add comment

f11101e

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

wip

d8e99a9

Signed-off-by: Jesse Brown <[email protected]>

fixup! wip

87ff4de

Signed-off-by: Jesse Brown <[email protected]>

store kaniko snapshots in different directories

d97defe

Signed-off-by: Jesse Brown <[email protected]>

more wip work

1f2d774

Signed-off-by: Jesse Brown <[email protected]>

fixup! more wip work

0f6fb26

Signed-off-by: Jesse Brown <[email protected]>

even more work

03a9503

Signed-off-by: Jesse Brown <[email protected]>

turn off cache

9dd5d4b

Signed-off-by: Jesse Brown <[email protected]>

Build test works end-to-end to extend build and run images, and expor…

0f8fbc1

…t a final image Signed-off-by: Natalie Arellano <[email protected]>

Copy fixtures from samples repo

b83ffc3

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

Pass flags from extender through to builder

279c6e6

Add argument for target image Signed-off-by: Natalie Arellano <[email protected]>

Update .gitignore

28209d3

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

Delete ignored files

bd0ce7d

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

Update local registry default port

0bdd2fe

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

Update .gitignore and remove unneeded files

012221c

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

natalieparellano changed the title ~~Extender lib~~ Dockerfiles POC Feb 15, 2022

jabrown85 approved these changes Feb 15, 2022

View reviewed changes

Remove unneeded files

80a06f4

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

Add default value for CustomPlatform (#804)

9b34d15

Signed-off-by: Ozzy Osborne <[email protected]>

This was referenced Feb 25, 2022

Dockerfiles: run /bin/detect and /bin/build for extensions #786

Closed

Introduce extend PHASE that builds a single application provided Dockerfile w/ kaniko #791

Closed

WIP: export kaniko layers on run image #797

Closed

natalieparellano mentioned this pull request Mar 2, 2022

Fix: order.toml has an order-ext table #812

Merged

natalieparellano added 3 commits March 3, 2022 11:43

Cache image for kaniko is configurable (#813)

84c9f13

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

order.toml has an order-ext table (#812)

24ad818

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

Remove bom check for newer buildpacks

2ee8594

See buildpacks/spec#286 - this was patched in a newer lifecycle, but is missing in the POC Signed-off-by: Natalie Arellano <[email protected]>

This was referenced Apr 7, 2022

Try to bring kaniko implementation up-to-date with the latest lifecycle redhat-buildpacks/poc#19

Closed

Project for Dockerfiles proof of concept buildpacks/samples#113

Closed

Allow platforms to provide run image sboms buildpacks/rfcs#211

Closed

natalieparellano mentioned this pull request Apr 14, 2022

Add spec changes for image extensions buildpacks/spec#298

Closed

natalieparellano added 2 commits April 18, 2022 10:17

Extender should source env from extended build image before running b…

597e31d

…uilder (#845) Signed-off-by: Natalie Arellano <[email protected]>

Generate phase should update build plan to remove extension-provided …

3bc4c8e

…deps (#844) Signed-off-by: Natalie Arellano <[email protected]>

natalieparellano mentioned this pull request Apr 18, 2022

Spike: Dockerfiles POC #709

Closed

natalieparellano closed this Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dockerfiles POC #802

Dockerfiles POC #802

natalieparellano commented Feb 15, 2022 •

edited

Loading

jabrown85 left a comment

jabrown85 Feb 15, 2022

jabrown85 Feb 15, 2022

jabrown85 Feb 15, 2022

jabrown85 Feb 15, 2022

jabrown85 Feb 15, 2022

jabrown85 Feb 15, 2022

jabrown85 Feb 15, 2022

BarDweller commented Feb 22, 2022

natalieparellano commented Apr 14, 2022

natalieparellano commented Apr 14, 2022

natalieparellano commented Jun 29, 2022

	extender/testdata/layers/samples_/
	extender/testdata/layers/samples_/

Dockerfiles POC #802

Dockerfiles POC #802

Conversation

natalieparellano commented Feb 15, 2022 • edited Loading

jabrown85 left a comment

Choose a reason for hiding this comment

jabrown85 Feb 15, 2022

Choose a reason for hiding this comment

jabrown85 Feb 15, 2022

Choose a reason for hiding this comment

jabrown85 Feb 15, 2022

Choose a reason for hiding this comment

jabrown85 Feb 15, 2022

Choose a reason for hiding this comment

jabrown85 Feb 15, 2022

Choose a reason for hiding this comment

jabrown85 Feb 15, 2022

Choose a reason for hiding this comment

jabrown85 Feb 15, 2022

Choose a reason for hiding this comment

BarDweller commented Feb 22, 2022

natalieparellano commented Apr 14, 2022

natalieparellano commented Apr 14, 2022

natalieparellano commented Jun 29, 2022

natalieparellano commented Feb 15, 2022 •

edited

Loading