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

Added spec changes for Image Extension #267

Closed
wants to merge 19 commits into from

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Nov 3, 2021

WIP specification of buildpacks/rfcs#173

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner
Copy link
Member Author

jkutner commented Nov 17, 2021

@sclevine @natalieparellano PTAL and let me know if this is on the right track


## Image Extension API Version

This document specifies Image Extension API version `0.1`.
Copy link
Member Author

Choose a reason for hiding this comment

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

Per @sclevine, we should not introduce a new API version, but instead tie this to one of the existing APIs. We also need to address this in the extension.toml

buildpack.md Outdated Show resolved Hide resolved
Correspondingly, each `/bin/build` executable:

- MAY read from the `<app>` directory.
- MUST NOT write to the `<app>` directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- MUST NOT write to the `<app>` directory.
- MUST NOT write to the `<app>` directory.

Is this one of those places where we hope they won't but can't do anything if they do?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jabrown85 i think it depends on the platform. Like, if it's running the extension in it's own container, it might be able to exclude <app> dir changes from it's snapshot. But if it's all running on the same filesystem/volume it won't be able to.

This isn't actually a suggested change right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, not a suggested change. I must have clicked the wrong button :)

- MAY log output from the build process to `stdout`.
- MAY emit error, warning, or debug messages to `stderr`.
- SHOULD write BOM (Bill-of-Materials) entries to `<output>/launch.toml` describing any contributions to the app image.
- MAY write key-value pairs to `<output>/launch.toml` that are provided as build args to run.Dockerfile or Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the first time run.Dockerfile is mentioned, should we omit this specific file name here in favor of keeping that in the image-extensions.md? Just refer to Dockerfile here in a generic sense maybe...

image-extension.md Outdated Show resolved Hide resolved
All `Dockerfile`s are provided with `base_image` and `build_id` args.
The `base_image` arg allows the Dockerfile to reference the original base image.
The `build_id` arg allows the Dockerfile to invalidate the cache after a certain layer and must be defaulted to `0`. The executor of the Dockerfile will provide the `build_id` as a UUID (this eliminates the need to track this variable).
When the `$build_id` arg is referenced in a `RUN` instruction, all subsequent layers will be rebuilt on the next build (as the value will change).
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 a section on how a platform should use this $build_id. I kind of forgot how/why it is done this way and when a platform would ever be able to safely use the same UUID.

[-plan <plan>]
```

##### Inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may need /workspace | CNB_APP_DIR as well, since they are allowed/expected to read from that directory.

buildpack.md Outdated Show resolved Hide resolved
Image extensions participate in the buildpack [detection](#detector) process, with the same `UID`, `GID`, and interface for `/bin/detect`. However:
- Detection is optional for extensions, and they are assumed to pass detection when it is not present. A `/bin/detect` that exits with a 0 exit code passes detection, and fails otherwise.
- Extensions MUST only output `provides` entries to the build plan. They MUST NOT output `requires`.
- Extensions MUST all precede buildpacks in [order](#ordertoml-toml) definitions.
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/buildpacks/samples/pull/113/files#r726206827 talks about creating a separate [[order-ext]] in builder.toml - would that translate into a separate file?

buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
buildpack.md Outdated Show resolved Hide resolved
Comment on lines +499 to +501
- MAY modify or delete any existing `<output>` directories.
- MAY create new `<output>` directories.
- MAY name any new `<output>` directories without restrictions except those imposed by the filesystem and the ones noted below.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure what to make of these bullets. I think they could be removed (when talking about <layers> they make sense but we don't really care about the output directory for extensions, only the files contained there).

platform.md Outdated Show resolved Hide resolved
[-exts <exts>] \
[-output <output>] \
[-log-level <log-level>] \
[-plan <plan>]
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

platform.md Outdated Show resolved Hide resolved
platform.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved
`Dockerfile`s MUST be applied to their corresponding base images after all extensions are executed and before any regular buildpacks are executed. `Dockerfile`s MUST be applied in the order determined during buildpack detection.

All `Dockerfile`s are provided with `base_image` and `build_id` args.
The `base_image` arg allows the Dockerfile to reference the original base image.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `base_image` arg allows the Dockerfile to reference the original base image.
The `base_image` arg MUST be the original base image.


All `Dockerfile`s are provided with `base_image` and `build_id` args.
The `base_image` arg allows the Dockerfile to reference the original base image.
The `build_id` arg allows the Dockerfile to invalidate the cache after a certain layer and must be defaulted to `0`. The executor of the Dockerfile will provide the `build_id` as a UUID (this eliminates the need to track this variable).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `build_id` arg allows the Dockerfile to invalidate the cache after a certain layer and must be defaulted to `0`. The executor of the Dockerfile will provide the `build_id` as a UUID (this eliminates the need to track this variable).
The `build_id` arg MUST be a UUID. This allows the Dockerfile to invalidate the cache after a certain layer.

image-extension.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved
image-extension.md Outdated Show resolved Hide resolved

### Files

#### `extension.toml` (TOML)
Copy link
Member

Choose a reason for hiding this comment

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

We should also mention (somewhere) that meta-buildpacks may not contain extensions.

@natalieparellano
Copy link
Member

natalieparellano commented Dec 9, 2021

Just making a quick note for posterity on what we'll probably still want to add:

  • maybe make more clear the distinction between extending the build image vs. the run image (something like "image-extender MAY run within the context of the build container to extend the build image directly prior to buildpack execution"); we could also clarify that extending the build image and extending the run image are likely different phases (or phase executions) that could happen in parallel
  • probably some updates to the distribution spec for builder changes
  • updates to the platform spec around rebase (e.g., use of the --force flag)
  • resolution of current questions around the presence of /cnb/image/genpkgs in the runtime base image
    (EDIT: added)
  • what metadata (if any) should go on the image so that data from previous build extensions can be restored?

jkutner and others added 17 commits December 13, 2021 08:44
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Copy link
Member

natalieparellano commented Dec 15, 2021

I started working on a POC lifecycle that can:

  • Run /bin/detect for extensions as part of regular detect
  • Run /bin/build for extensions as a new invocation of build
    Together with the "extender" POC that @cmoulliard is working on, this could help uncover the remaining uncertainties that we'll need to resolve in the spec. Some questions and comments so far are noted here and listed below:
  • Questions around how extensions should be identified in order.toml and group.toml (see RFC comment)
  • What should be the format of the file that we’re using to pass the Dockerfile paths & args to the extender? And what should be the path (currently using /layers/config/metadata.toml, but we probably want a separate file for extensions and buildpacks)
  • optional=false is the default value for buildpacks, but optional=true is the default value for extensions - how to handle this cleanly?
  • We need the build phase for extensions to output plan.toml so that we can filter the plan for buildpacks
  • What should be the default parent dir for extensions output - maybe /layers/config/ext?

@hone
Copy link
Member

hone commented Mar 23, 2022

@jkutner I'm going to closes this in lieu of #298. Feel free to re-open if that's not the case.

@hone hone closed this Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants