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

Add RFC for extension layers #301

Merged

Conversation

c0d1ngm0nk3y
Copy link
Contributor

@c0d1ngm0nk3y c0d1ngm0nk3y commented Oct 9, 2023

@buildpack-bot
Copy link
Member

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

@pbusko pbusko force-pushed the extension-distroless branch 2 times, most recently from ff21dd8 to 17369c5 Compare October 13, 2023 13:39
@loewenstein
Copy link
Contributor

@natalieparellano how do we manage to get feedback? We don't seem to be able to request a review.

Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

Thank you @c0d1ngm0nk3y for the RFC. I've added a few comments to try to understand it better.


This change allows extensions to possess their own layers to utilize during the generation/extend process. Additionally, it ensures that extension output does not inadvertently interfere with other extension or buildpack layers during the build, and it does not unintentionally become part of the final application image.

This would allow distroless run images to be extended.
Copy link
Member

Choose a reason for hiding this comment

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

IIUC this would allow an extension to use the package manager from the build image to download packages for the run image (by copying the package manager to the layers dir).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to see n example extension laid out in the RFC

Copy link
Contributor

Choose a reason for hiding this comment

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

For instance, if we try to extend a distroless image, there's no way to extend it meaningfully with additional packages/files by the means available in the run image itself (sometimes even coreutils are not present). The extension could populate the extension layer with necessary files/packages/binaries and manipulate them from the generated Dockerfile in the context of a run image.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great - lets get that example added into the RFC. An example extension + Dockerfile that makes use of the files in the context. It'll help communicate the idea to a broader group

text/0000-extension-layer.md Outdated Show resolved Hide resolved
text/0000-extension-layer.md Outdated Show resolved Hide resolved
text/0000-extension-layer.md Outdated Show resolved Hide resolved
text/0000-extension-layer.md Outdated Show resolved Hide resolved
text/0000-extension-layer.md Show resolved Hide resolved
text/0000-extension-layer.md Show resolved Hide resolved
@pbusko
Copy link
Contributor

pbusko commented Oct 30, 2023

@jabrown85 @natalieparellano the RFC has been updated with the proposed changes.

Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @c0d1ngm0nk3y. The proposed changes make sense and should be relatively easy to implement in the lifecycle. I wonder if there's a better term compared to "layers" so as not to create confusion with respect to buildpack layers. The purpose, lifecycle, etc of these context directories is very different.

text/0000-extension-layer.md Outdated Show resolved Hide resolved
text/0000-extension-layer.md Outdated Show resolved Hide resolved
text/0000-extension-layer.md Outdated Show resolved Hide resolved
text/0000-extension-layer.md Outdated Show resolved Hide resolved
text/0000-extension-layer.md Outdated Show resolved Hide resolved
@loewenstein
Copy link
Contributor

I wonder what the state of this is? @natalieparellano is there anything missing from your point of view? @pbusko are we on top of all provided review feedback?

@natalieparellano
Copy link
Member

I wonder what the state of this is? @natalieparellano is there anything missing from your point of view? @pbusko are we on top of all provided review feedback?

I left two new comments (here and here) but otherwise I think this is ready. If we don't evolve it any further after this week we can put it up for votes.

@pbusko
Copy link
Contributor

pbusko commented Nov 22, 2023

I left two new comments (here and here) but otherwise I think this is ready. If we don't evolve it any further after this week we can put it up for votes.

The comments have been addressed

@loewenstein
Copy link
Contributor

I wonder what the state of this is? @natalieparellano is there anything missing from your point of view? @pbusko are we on top of all provided review feedback?

I left two new comments (here and here) but otherwise I think this is ready. If we don't evolve it any further after this week we can put it up for votes.

@natalieparellano are you putting up for vote?

@loewenstein
Copy link
Contributor

@jabrown85 could you help bringing this to a vote and getting it merged. That would be great. I hesitate to immediately start working on spec and implementation pull requests - inviting people to vote might lead to more review comments that lead to changes, but I would really like to see buildpacks/spec#378 closed and 0.13 released as soon as possible.

[how-it-works]: #how-it-works

- Before execution of the `./bin/generate`, the lifecycle will create a distinct writable layer `$CNB_GENERATED_DIR/<extension-id>` for each extension which passed detection.
- The `$CNB_GENERATED_DIR/<extension-id>` is provided to the `./bin/generate` as `<output>` (`$CNB_OUTPUT_DIR`) directory.
Copy link
Member

Choose a reason for hiding this comment

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

Currently the extender is looking for Dockerfiles in <generated>/run/<image extension ID>/Dockerfile and <generated>/build/<image extension ID>/Dockerfile (where <generated> defaults to <layers>/generated). Should we change the Platform API so that the extender looks for <generated>/<image extension ID>/run.Dockerfile, or should the lifecycle re-arrange the files after the generate phase? The former might be simpler in the long run, but platforms such as pack look for the presence of files in <generated>/build and <generated>/run to infer that extension is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go with writing to <generated>/<image extension ID>/run.Dockerfile and/or <generated>/<image extension ID>/build.Dockerfile, as suggested in this RFC. Platforms such as pack might have a fallback mechanism, instead of adding more complex logic on the lifecycle side.

Copy link
Member

Choose a reason for hiding this comment

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

@pbusko I'm in favor of removing this complexity from the lifecycle. Could we add an extend boolean to the [build-image] table in analyzed.toml, to match [run-image]:

[run-image]
  image = "<image>"
  reference = "<image reference>"
  extend = false

[build-image]
  reference = "<image reference>"
  extend = false

This will give platforms a simple way to check if extension is needed.

@pbusko
Copy link
Contributor

pbusko commented Dec 1, 2023

@natalieparellano please let us know if there's something still unclear/blocking the RFC from being merged

@loewenstein
Copy link
Contributor

@natalieparellano @jabrown85 anything blocking a vote, merge and implementation?

@natalieparellano
Copy link
Member

@jabrown85 could you give this one a read?

pbusko and others added 7 commits December 14, 2023 13:33
Co-authored-by: Ralf Pannemans <[email protected]>
Signed-off-by: Ralf Pannemans <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>
Signed-off-by: Pavel Busko <[email protected]>
Signed-off-by: Pavel Busko <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]>
Signed-off-by: Pavel Busko <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]>
Signed-off-by: Pavel Busko <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]>
Signed-off-by: Pavel Busko <[email protected]>
Co-authored-by: Natalie Arellano <[email protected]>
Signed-off-by: Pavel Busko <[email protected]>
Signed-off-by: Pavel Busko <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>
@natalieparellano
Copy link
Member

Let's merge this one, it was moved into voting exactly one week ago...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants