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

WIP: export kaniko layers on run image #797

Closed
wants to merge 7 commits into from

Conversation

jabrown85
Copy link
Contributor

@jabrown85 jabrown85 commented Jan 20, 2022

Loop through run-image extensions and execute them one by one

@jabrown85 jabrown85 requested a review from a team as a code owner January 20, 2022 22:38
@jabrown85 jabrown85 force-pushed the jab/wip-export-on-run-image branch from 30f032a to 2ba9b07 Compare January 20, 2022 22:39
Comment on lines +38 to +41
rm -rf $SAMPLES_REPO_PATH/kaniko-run
mkdir -p $SAMPLES_REPO_PATH/kaniko-run
rm -rf $SAMPLES_REPO_PATH/layers-run/kaniko
mkdir -p $SAMPLES_REPO_PATH/layers-run/kaniko
Copy link
Member

Choose a reason for hiding this comment

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

I guess a next step in this effort could be to try to make these directories useful for caching purposes (though alternative approaches to caching such as spinning up a registry process were also discussed).

# args: <kaniko|buildah> <build|run> <base-image>

echo ">>>>>>>>>> Running extend on run image..."
# TODO: we should probably not have to mount /cnb/ext? Can we copy the static Dockerfiles to layers?
Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a good idea to me. The lifecycle currently just leaves the Dockerfiles where the extensions wrote them, but copying them sounds like a logical next step.

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
@jabrown85 jabrown85 force-pushed the jab/wip-export-on-run-image branch from 15a9ed0 to 9dd5d4b Compare January 31, 2022 16:37
// if err := e.addExtenderLayers(opts, &meta); err != nil {
// return platform.ExportReport{}, err
// }

// buildpack-provided layers
if err := e.addBuildpackLayers(opts, &meta); err != nil {

Choose a reason for hiding this comment

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

Will this function only be used to extract the Run or Build/Run layers ? If the function will only be used to extract the Run layers, then it should be renamed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function already existed and only handles buildpack layers. I actually could remove the exporter changes and addExtenderLayers entirely. But I didn't do so yet depending on how we plan on moving forward. My plan was to assume a new run image reference came out of the run extension and was already pushed to the remote registry. We could alternatively have the run extensions save a run image as OCI Layout/Tarball and then have exporter build with those layers (or even push up the run image to the remote registry).

Choose a reason for hiding this comment

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

Add then a TODO to review later or better document what this step of the code is doing ?

func (e *Exporter) addExtenderLayers(opts ExportOptions, meta *platform.LayersMetadata) error {
e.Logger.Info("Adding extender layers")

path := filepath.Join("/layers-run/kaniko")

Choose a reason for hiding this comment

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

Do you plan to extract the layers for every build(pack) build or do we plan to move the already extracted layers to a cache and reuse them for next build ?

// We're in "build" mode, untar layers to root filesystem: /
for _, layerFile := range layerFiles {
workingDirectory := "/"
tarPath := "/layers/kaniko/" + layerFile

Choose a reason for hiding this comment

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

We should maybe avoid to hard code kaniko but pass it as parameter as it could be possible that another solution could be supported in the future (buildah, ...)

panic(err)
}
}

// Run the build for buildpacks with lowered privileges.
// We must assume that this extender is run as root.
cmd := exec.Command("/cnb/lifecycle/builder", "-app", "/workspace", "-log-level", "debug")

Choose a reason for hiding this comment

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

Do you plan to log an error message for the end user to be informed that a root privileged are needed ?

Remark: Why do you say that Run the build for buildpacks with lowered privileges. as comment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the first question, I do assume we will need to introduce "must run as root" error message like we do elsewhere (creator does this). I didn't add the comment you are asking about, but the idea is that we want to run normal buildpacks after build extensions but in the same process. The /cnb/lifecycle/builder should refuse to run as root, so the ext binary will need to ensure they aren't running as root.

@natalieparellano natalieparellano mentioned this pull request Feb 15, 2022
@natalieparellano
Copy link
Member

@jabrown85 I'm going to close this as all the work is captured in #802

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.

3 participants