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

Additional exportable layers #181

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

samj1912
Copy link
Member

@samj1912 samj1912 commented Aug 13, 2021

Signed-off-by: Sambhav Kothari skothari44@bloomberg.net

Readable

@samj1912 samj1912 requested a review from a team as a code owner August 13, 2021 19:24
@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)

# Drawbacks
[drawbacks]: #drawbacks

N/A.
Copy link
Member

Choose a reason for hiding this comment

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

I actually think one of the drawbacks is any buildpack using this feature is highly tied to the underlying stack image. With #167, does this make it harder for buildpacks to have that tight coupling? How does one know what VOLUMEs are required before building?

@hone
Copy link
Member

hone commented Aug 18, 2021

What happens if the VOLUME is not set in the stack image and the buildpack wants to export it?

@hone
Copy link
Member

hone commented Aug 18, 2021

I do think the use cases are compelling, but my concerns are around exposing this information upfront and I think this impacts #167.

# Optionally users can override the default application workspace
# by specifying it through the WORKDIR directive in the build
# and run images.
WORKDIR /app
Copy link
Member

Choose a reason for hiding this comment

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

Should we instead use CNB_APP_DIR for this? Although workdir=appdir in the buildpack API, in the platform API we don't use the working directory to indicate this and instead use an environment variable.

USER ${CNB_USER_ID}:${CNB_GROUP_ID}
```

Buildpacks will be passed a list of volumes through the `CNB_EXPORT_VOLUMES` variable and this variable would be a JSON list. Buildpacks may read this environment variable during `detect` or `build`.
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
Buildpacks will be passed a list of volumes through the `CNB_EXPORT_VOLUMES` variable and this variable would be a JSON list. Buildpacks may read this environment variable during `detect` or `build`.
Buildpacks will be passed a list of volumes through the `CNB_EXPORT_DIRS` variable and this variable would be a JSON list. Buildpacks may read this environment variable during `detect` or `build`.

Given we are exporting the contents of these build-time volumes into real layers in the resulting app image maybe this makes more sense from the buildpack perspective? It is irrelevent to the buildpack that these are build time volumes.

Users would be able to define stack images with `VOLUME` directives in Dockerfiles.


For example a build/run base image may look like -
Copy link
Member

Choose a reason for hiding this comment

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

Is it confusing to add these volumes to the run image, given that when we extend the run image to create the app image these directories will have buildpack-contributed contents and thus will not behave as volumes?

If did wanted these as volumes on the run image (to prevent folks from extending the run image in a way that breaks rebase) I think the exporter should remove these volumes from the final app image.


# Drawbacks
[drawbacks]: #drawbacks

Copy link
Member

Choose a reason for hiding this comment

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

It would be hard for a simple platform like tekton to implement this.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible this could open a new attack vector? I can't think of anything specific, but i wonder what's possible if a buildpack can drop files in /etc/ or /usr/bin/ for example.

@hone
Copy link
Member

hone commented Sep 15, 2021

@samj1912 will update with an alternative proposal from the last WG.

@samj1912 samj1912 force-pushed the volumes branch 2 times, most recently from 373d6f8 to 60a6756 Compare September 16, 2021 13:32

The lifecycle changes would involve exporting the files present in the above locations which should be similar to the logic that currently exists for exporting application workspace. Buildpacks could also take advantage of `slices` to specify paths in these additional directories that should exist as separate layers.

The lifecycle `analysis` phase would also be responsible for validating that the above list of `export_dirs` is valid for the provided `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.

How would analyzer validate this? We would need to be able to do this quickly, so some sort of pre-processing would need to occur.

Copy link
Member Author

Choose a reason for hiding this comment

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

Answered below. It would make it fairly slow if the platform has not put additional metadata in the run image.

# Unresolved Questions
[unresolved-questions]: #unresolved-questions

TBD.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any security ramifications for certain paths? Do platforms or builders need a way to deny paths?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be good to have a block list of paths.

export_dirs = ["/opt", "/home/cnb/.config"]
```

The above would only be valid for normal buildpacks and not meta-buildpacks. Based on the `volumes` field, a platform would be responsible for validating that the `build` and `run` image do not have any content on these paths. Additionally, the builder image would be tagged with a label `io.buildpacks.export_dirs` with the list of paths that need to be exported.
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 above would only be valid for normal buildpacks and not meta-buildpacks. Based on the `volumes` field, a platform would be responsible for validating that the `build` and `run` image do not have any content on these paths. Additionally, the builder image would be tagged with a label `io.buildpacks.export_dirs` with the list of paths that need to be exported.
The above would only be valid for normal buildpacks and not meta-buildpacks. Based on the `export_dirs` field, a platform would be responsible for validating that the `build` and `run` image do not have any content on these paths. Additionally, the builder image would be tagged with a label `io.buildpacks.export_dirs` with the list of paths that need to be exported.


The lifecycle changes would involve exporting the files present in the above locations which should be similar to the logic that currently exists for exporting application workspace. Buildpacks could also take advantage of `slices` to specify paths in these additional directories that should exist as separate layers.

The lifecycle `analysis` phase would also be responsible for validating that the above list of `export_dirs` is valid for the provided `run` image.
Copy link
Member

Choose a reason for hiding this comment

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

How?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would make the analysis phase fairly expensive. There are a couple of ways -

  1. During the builder creation, the platform add a label to the run images with a flag making sure that the buildpacks are compatible with the run image.
  2. The other option is to download the run images and check all the paths across the layers to confirm that the specified folders are empty.

Not ideal, but we could skip the validation as discussed in the WG if the operator trusts the builder to be valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could validation be just the Deny path list that is set at builder creation time and stored as a label? Or is that not validating the image in the way you would expect?

[buildpack]
id = "<buildpack ID>"
name = "<buildpack name>"
export_dirs = ["/opt", "/home/cnb/.config"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we worried about this coupling between a buildpack and the stack? Does this hurt portability of buildpacks between stacks?

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably will, but it is an optional feature to implement. If a buildpack is aiming for portability it could choose not to do things this way.

@hone
Copy link
Member

hone commented Sep 29, 2021

@samj1912 @sclevine @hone will meet to discuss the multiple input directory proposal before moving forward with this one.

@hone
Copy link
Member

hone commented Oct 6, 2021

@samj1912 will update with single input / multiple output proposal from the discussion.

@hone hone unassigned ekcasey Oct 7, 2021
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@hone hone added the status/blocked RFC is blocked by some other outstanding dependency. label Dec 8, 2021
@hone hone removed the status/blocked RFC is blocked by some other outstanding dependency. label Dec 15, 2021
# Motivation
[motivation]: #motivation

The main motivation behind this RFC is to unlock exporting application images that require libraries or software to be installed in certain directories apart from the default application directory and these installations are rebase safe and self-contained. This unlocks use cases where buildpacks may install software in `/opt` directories (for AWS Lambda extensions or other common standalone software) for example or preserve some settings in a `/home/$USER/.config` for the output application. This provides a rebase safe extension point without major changes to the buildpacks API.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the motivation is to allow for "libraries or software that install in certain directories", it goes at odds with requiring that "these exportable volumes be empty". After all, we can't guarantee that these "libraries or software" aren't installing to / or something.


```json
[
{"name": "user-config-dir", "value": "/home/cnb/.config"},
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this mapping concept being done by the base image author.

Could this list be something buildpack authors could get at and then name layers that match user-config-dir that gets exported to /home/cnb/.config during export phase? Or perhaps a key in the layer.toml has to be set so it is more explicit.

The requirement for an empty dir and symlinking for builder could potentially be avoided. A buildpack author could put a new binary in the resulting app image's /usr/bin by putting the binary in /layers/<bp-dir>/usr-bin/. If a new stack is used that doesn't support it, the layer would still exist on the resulting app image but not in the overridden path.

Giving the buildpack a dictionary so they can easily check if "usr/bin" is available to use would allow buildpack authors to either put the binary in "usr/bin" or do more work like altering PATH and whatnot to get it to work on other base images.

{
  "usr/bin" : "usr-bin",
  "~/.config": "user-config-dir"
}

# What it is
[what-it-is]: #what-it-is

Buildpack authors would be able to buildpacks with `buildpack.exportable-volumes` keys in their `buildpack.toml` files.
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
Buildpack authors would be able to buildpacks with `buildpack.exportable-volumes` keys in their `buildpack.toml` files.
Buildpack authors would be able define a set of `buildpack.exportable-volumes` keys in their `buildpack.toml` files.

@samj1912 samj1912 marked this pull request as draft February 16, 2022 19:24
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.

None yet

9 participants