-
Notifications
You must be signed in to change notification settings - Fork 70
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 spec changes for image extensions #298
Conversation
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
…le.d Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]> Co-authored-by: Emily Casey <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
93bc21e
to
8511bc8
Compare
This is still a bit rough... I'm going to add some comments to call out things that might be interesting for the reader. This branch includes commits from |
During detection, an order definition MUST be resolved into individual buildpack implementations. | ||
During detection, an order definition for image extensions (if present) and an order definition for buildpacks MUST be resolved into individual buildpack implementations. | ||
|
||
Order definitions for image extensions MUST NOT contain nested orders. If an order definition for image extensions is present, it will be pre-pended to the order definition for buildpacks (as if the order definition for image extensions were an order buildpack). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence (while "accurate", I think) doesn't make much sense... I am not sure how to improve it without re-working the entire section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"During detection, order definitions MUST be resolved into individual buildpack implementations. If present, Image Extensions MAY provide an order definition. Buildpacks MUST provide an order definition."
Not sure if that covers the intent.. which itself kinda hints the original still has wiggle room.. (It's possible in my rephrasing to interpret that each extension would supply an order definition, or that there would be one for all extensions).
The original leaves bare the possibilty that an order defintiion for extensions is required, although the extensions themselves remain optional (as the binding of the 'if present' can be made to 'image extensions' rather than 'order definition').
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
This almost certainly needs more changes but I am marking it ready for review in the interest of getting feedback. |
- A directory containing application source code | ||
- A shell, if needed, | ||
|
||
For each buildpack in each group in order, the lifecycle MUST execute `/bin/detect`. | ||
For each buildpack implementation in each group in order, the lifecycle MUST execute `/bin/detect`. Image extensions and buildpacks are both buildpack implementations. For simplicity, the following section will refer to both as "buildpacks". Image extensions MUST always be optional during detection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we stop using the term "buildpack implementation"? Or at least swap the word order to "implementation buildpack" if we can't think of a better term for a non-meta-buildpack? Meta-buildpacks are still implementations of buildpacks, so I think it's difficult to understand these sections.
This could be a separate PR, but I feel like adding image extensions makes this even more confusing.
Some name ideas:
- atomic buildpacks
- executable buildpacks
- concrete buildpacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like concrete buildpacks.. has a nice construction theme to it ;p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see #300 - I went with "executable buildpack" because that seemed most natural according to the definition I used.
@@ -373,12 +386,14 @@ A buildpack's mixin requirements must be satisfied by the stack in one of the fo | |||
|
|||
#### Order Resolution | |||
|
|||
During detection, an order definition MUST be resolved into individual buildpack implementations. | |||
During detection, an order definition for image extensions (if present) and an order definition for buildpacks MUST be resolved into individual buildpack implementations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During detection, an order definition for image extensions (if present) and an order definition for buildpacks MUST be resolved into individual buildpack implementations. | |
During detection, order definitions for image extensions and buildpacks MUST be resolved into a single order definition containing only atomic buildpacks. |
During detection, an order definition MUST be resolved into individual buildpack implementations. | ||
During detection, an order definition for image extensions (if present) and an order definition for buildpacks MUST be resolved into individual buildpack implementations. | ||
|
||
Order definitions for image extensions MUST NOT contain nested orders. If an order definition for image extensions is present, it will be pre-pended to the order definition for buildpacks (as if the order definition for image extensions were an order buildpack). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order definitions for image extensions MUST NOT contain nested orders. If an order definition for image extensions is present, it will be pre-pended to the order definition for buildpacks (as if the order definition for image extensions were an order buildpack). | |
Order definitions for image extensions MUST NOT contain nested orders. | |
If an order definition for image extensions is present, it MUST be prepended to the order definition for buildpacks before the resolution process occurs. |
To make this more clear, I think we should define the following six terms early in the spec:
- Order buildpack
- Atomic buildpack
- Order image extension (not currently allowed)
- Atomic image extension
- Order build module (currently only order buildpack)
- Atomic build module (atomic buildpacks and image extensions)
The names are just suggestions.
Maybe it's easier to say that image extensions are buildpacks?
- Normal order buildpack
- Normal atomic buildpack
- Image extension order bulidpack (not currently allowed)
- Image extension atomic buildpack
- Order buildpack (only normal buildpacks allowed)
- Atomic buildpack (atomic normal and extension buildpacks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sclevine what is a build module? What is the difference between an order build module and an atomic build module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's hash this out in https://github.com/buildpacks/spec/pull/300/files - we can rebase this PR when that one is merged.
- A directory containing application source code | ||
- A shell, if needed, | ||
|
||
For each buildpack in each group in order, the lifecycle MUST execute `/bin/detect`. | ||
For each buildpack implementation in each group in order, the lifecycle MUST execute `/bin/detect`. Image extensions and buildpacks are both buildpack implementations. For simplicity, the following section will refer to both as "buildpacks". Image extensions MUST always be optional during detection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why image extensions MUST be optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sclevine could you elaborate?
Correspondingly, each `/bin/build` executable: | ||
|
||
- MAY read from the `<app>` directory. | ||
- MUST NOT write to the `<app>` directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How/where will we be enforcing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same way we enforce buildpacks not writing to other buildpacks' layers directory (wishful thinking) :)
This is something that we could add in theory, but it would to subject to the same concerns outlined in buildpacks/rfcs#155
ARG base_image | ||
FROM ${base_image} | ||
``` | ||
- MUST NOT contain any other `FROM` instructions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking to discussion here: https://github.com/buildpacks/rfcs/pull/173/files#r794629405
The concern is that we could end up switching to a base image that doesn't have buildpacks on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather have this as a 'SHOULD' than a MUST.. it's doable, IF you really know what you are doing, and closing that door with a MUST might end up closing an escape hatch that someone might need =)
I've tried swapping build images in the PoC (and copying the /cnb folder from the previous image via multistage) and it is doable, but I hit odd issues executing the build step after the extension step, that I suspect were related to that switch.. I suspect with enough care, with certain constraints it could be done.. eg, if the image switched to was built as a stack image that includes the buildpacks content, etc, etc, etc.
When extending the run image, a Dockerfile MAY indicate that it preserves the ability to rebase by including the following instruction: | ||
|
||
```bash | ||
LABEL io.buildpacks.rebasable=true | ||
``` | ||
|
||
For the final image to be rebasable, the provided run image and all applied Dockerfiles MUST indicate rebasability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen in case this label is set to true? What are the cases when an extension ran and the output image is still rebaseable?
One other thing I am unsure about is how we will uniquely identify a "family" of base images that can be rebased with one another.
I know we thought about replacing stack id with some other identifier that indicates the "family" of base images - how does that fit in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole rebase picture with extensions is still unclear to me. It would be nice if we can have some actual examples of cases where/how this can work with platforms like pack or kpack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking / hoping that an extension can still set rebaseable=true, where it modifies the build image (say to install runtimes, tools to build etc) .. but limits the runtime changes to just using 'FROM' to switch to a different run image.. I think this would still qualify to be rebaseable, as future run images can still be swapped in.. provided they still contain the required dependencies that were met by the substituted run image. In theory this allows for the existence of a generic builder image that tailors itself to install runtimes/tools etc, working in conjunction with a set of run images to cover the most common scenarios.. and maybe, also in conjunction with a generic (non rebaseable) run image to cover the edge cases where the common image was insufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, in the case where a Dockerfile is applied to the run image, but all it does is switch the run image (using a FROM
instruction):
- Should we publish a new "extended" run image (the
<target-image>
argument provided to theextender
)- Opinion: no, that seems confusing
- Should we run
genpkgs
?- Opinion: no, but we should make an effort to locate an sbom for the new run image (see Allow platforms to provide run image sboms rfcs#211)
Signed-off-by: Natalie Arellano <[email protected]> Co-authored-by: Stephen Levine <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Just to provide an update on this - I see this effort and the "run image SBOM" effort as sort of orthogonal at this point. The diagram below depicts what the path to implementation might look like: We had discussed a phased implementation for Dockerfiles previously and this seems to be prudent given that we're still working out some issues with kaniko. As a next step, I could update this PR (or open a new one) to capture only the changes needed for the first phase (the green box in the diagram above). |
After further discussion in 4/14 Office Hours, it seems that the simplest and best path forward for dealing with run image SBOMs is to make this the responsibility of the After the app image is exported, the
This will also simplify the lifecycle implementation. We can drop the notion of run image SBOM and |
Supersedes #267