Skip to content

[RFC0031] Support for System Cloud Native Buildpacks #3898

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

pbusko
Copy link
Contributor

@pbusko pbusko commented Jul 23, 2024

  • A short explanation of the proposed change:

Support for System (admin) Cloud Native Buildpacks

  • An explanation of the use cases your change solves

Implementation of the RFC0031

This PR requires changes to the cloud_controller_worker config, to include the default_app_lifecycle property with the default value buildpack, similar to https://github.com/cloudfoundry/capi-release/blob/b8afc27dbfbb4dc3749485cb23ab9bf65801e26b/jobs/cloud_controller_ng/templates/cloud_controller_ng.yml.erb#L542

  • Links to any other associated PRs

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@pbusko pbusko changed the title [RFC0031] CNB System Buildpacks [RFC0031] Support for System Cloud Native Buildpacks Jul 23, 2024
@pbusko pbusko force-pushed the cnb-system-buildpacks branch 5 times, most recently from 788251d to b55a772 Compare July 25, 2024 14:27
@pbusko pbusko marked this pull request as ready for review July 25, 2024 14:56
@pbusko pbusko force-pushed the cnb-system-buildpacks branch from b55a772 to 29d7904 Compare September 23, 2024 06:53
@pbusko pbusko force-pushed the cnb-system-buildpacks branch from 29d7904 to 27210e1 Compare September 30, 2024 07:30
@pbusko pbusko force-pushed the cnb-system-buildpacks branch from 27210e1 to ca076ad Compare October 29, 2024 08:12
@modulo11 modulo11 force-pushed the cnb-system-buildpacks branch from 8862fab to c27a9a9 Compare October 31, 2024 09:15
@pbusko pbusko force-pushed the cnb-system-buildpacks branch from c27a9a9 to b43a8e4 Compare February 3, 2025 09:09
@pbusko pbusko force-pushed the cnb-system-buildpacks branch from b43a8e4 to 0cc1cac Compare February 20, 2025 09:52
@pbusko pbusko force-pushed the cnb-system-buildpacks branch from 0cc1cac to 5ba3373 Compare April 15, 2025 07:30
@sethboyles
Copy link
Member

@pbusko @nicolasbender thanks for the PR. We'd like to get this merged in soon, if possible. Is there anything left to do on this PR before its ready?

Can you help me understand the right format for uploading a CNB? I've tried the format specified here but I get the error CNBGenericBuildFailed - cnb: generic build failure. Maybe you could provide me with a working cnb zip so I can understand what I'm doing wrong?

@sethboyles
Copy link
Member

When repackaging a CNB as a zip file I simply get the CNBGenericBuildFailed - cnb: generic build failure error. However, when repackaging as a tgz file I get the following error:

ERROR: failed to create 'order.toml', error: open /tmp/buildpacks/order.toml: permission denied
Error: generic build failure

@sethboyles
Copy link
Member

Upgraded to latest diego-release and am getting different errors:

With a tgz file:

Staging app and tracing logs...
   Downloading node...
   Downloaded node (67M)
   Cell f0e5d726-a296-4916-a5fb-6271cefee1aa creating container for instance 454792ee-c2d0-40c4-9226-71274c380442
   Security group rules were updated
   Cell f0e5d726-a296-4916-a5fb-6271cefee1aa successfully created container for instance 454792ee-c2d0-40c4-9226-71274c380442
   Downloading app package...
   Downloading build artifacts cache...
   Downloaded app package (1.2M)
   Downloaded build artifacts cache (51.7M)
   Using buildpacks: file:///tmp/buildpacks/56d2e45307d1d346
   Error: downloading buildpacks failed
   ERROR: failed to download buildpacks, error: extracting from file:///tmp/buildpacks/56d2e45307d1d346: inspecting buildpack blob: failed to get next tar entry: open /tmp/buildpacks/56d2e45307d1d346/._.: permission denied
   Exit status 232
   Cell f0e5d726-a296-4916-a5fb-6271cefee1aa stopping instance 454792ee-c2d0-40c4-9226-71274c380442
   Cell f0e5d726-a296-4916-a5fb-6271cefee1aa destroying container for instance 454792ee-c2d0-40c4-9226-71274c380442
CNBDownloadBuildpackFailed - cnb: downloading buildpacks failed: This may be caused by a wrong url, invalid buildpack or invalid credentials. Check the staging logs for more details.
FAILED

With a zip file:

Staging app and tracing logs...
   Downloading node...
   Downloaded node (67M)
   Cell f0e5d726-a296-4916-a5fb-6271cefee1aa creating container for instance ba0fc857-9fed-4802-b149-65754ac0f776
   Security group rules were updated
   Cell f0e5d726-a296-4916-a5fb-6271cefee1aa successfully created container for instance ba0fc857-9fed-4802-b149-65754ac0f776
   Downloading build artifacts cache...
   Downloading app package...
   Downloaded app package (1.2M)
   Downloaded build artifacts cache (51.7M)
   Using buildpacks: file:///tmp/buildpacks/0c70243571233494
   ERROR: failed to download buildpacks, error: extracting from file:///tmp/buildpacks/0c70243571233494: reading buildpack: reading buildpack.toml: could not find entry path 'buildpack.toml': not exist
   Error: downloading buildpacks failed
   Exit status 232
   Cell f0e5d726-a296-4916-a5fb-6271cefee1aa stopping instance ba0fc857-9fed-4802-b149-65754ac0f776
   Cell f0e5d726-a296-4916-a5fb-6271cefee1aa destroying container for instance ba0fc857-9fed-4802-b149-65754ac0f776
CNBDownloadBuildpackFailed - cnb: downloading buildpacks failed: This may be caused by a wrong url, invalid buildpack or invalid credentials. Check the staging logs for more details.
FAILED

@sethboyles
Copy link
Member

I was able to get this to work by uploading the artifact from the docker repo instead:

docker pull gcr.io/paketo-buildpacks/nodejs
docker save gcr.io/paketo-buildpacks/nodejs:latest | gzip > ~/node-image.tar.gz
cf update-buildpack node -p ~/node-image.tar.gz

This has a few extra files compared to the file published to Github. According to the RFC, I think the cnb from the Github repo should work--do you know why it doesn't?

(as a side note--should we allow direct uploading of cnb files, without needing to rename/repackage them as zips or tars?)

@pbusko
Copy link
Contributor Author

pbusko commented Apr 23, 2025

Hi @sethboyles,

The artifact must be a gzipped tarball with an OCI layout inside. It was decided based on the fact that CC must select one of the supported media types by bbs for download & extraction. The name of the file should not matter, since we changed the media type detection from the file extension comparison to reading magic bytes from the file header.

@sethboyles
Copy link
Member

sethboyles commented Apr 23, 2025

@pbusko Ok. When I upload a cnb file directly, I get an error that it is not a zip file.

Also, do you why the downloads from the Github releases pages don't work? Even if I rename them as zip files. They appear to fit the OCI format specified in the RFC, but I get the error mentioned above

I think we've got a fix that will allow us to upload CNB files directly.

@sethboyles
Copy link
Member

sethboyles commented Apr 24, 2025

@pbusko are there any outstanding items that are blocking this PR? Any unimplemented aspects that you are aware of?

@pbusko
Copy link
Contributor Author

pbusko commented Apr 24, 2025

@sethboyles this PR is pending review since Jul/August 2024

Also, do you why the downloads from the Github releases pages don't work? Even if I rename them as zip files. They appear to fit the OCI format specified in the RFC, but I get the error mentioned above

As I mentioned before, the name of the file should not matter, since CAPI checks the first bytes to determine the file type. Also it is stated in the RFC, the file must be a gzipped tarball (not zip). The .cnb files attached to the GitHub releases are just tarballs without compression:

➜  ~ wget -q https://github.com/paketo-buildpacks/nodejs/releases/download/v7.6.2/nodejs-7.6.2.cnb
➜  ~ file nodejs-7.6.2.cnb
nodejs-7.6.2.cnb: POSIX tar archive

nicolasbender and others added 6 commits April 24, 2025 09:47
@pbusko pbusko force-pushed the cnb-system-buildpacks branch from 5ba3373 to cc3ac62 Compare April 24, 2025 07:47
tomkennedy513 added a commit to tomkennedy513/cloud_controller_ng that referenced this pull request Apr 24, 2025
- This is a follow on to cloudfoundry#3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format

Signed-off-by: Tom Kennedy <[email protected]>
tomkennedy513 added a commit to tomkennedy513/cloud_controller_ng that referenced this pull request Apr 24, 2025
- This is a follow on to cloudfoundry#3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format

Signed-off-by: Tom Kennedy <[email protected]>
tomkennedy513 added a commit to tomkennedy513/cloud_controller_ng that referenced this pull request Apr 24, 2025
- This is a follow on to cloudfoundry#3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format

Signed-off-by: Tom Kennedy <[email protected]>
tomkennedy513 added a commit to tomkennedy513/cloud_controller_ng that referenced this pull request Apr 24, 2025
- This is a follow on to cloudfoundry#3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format

Signed-off-by: Tom Kennedy <[email protected]>
@sethboyles
Copy link
Member

@pbusko thanks for clarifying on the gzip tarball/zip and fixing some of the tests.

Uploading a non-gzip or zip results in this error:

Uploaded buildpack file is invalid: heroku_ruby_linux-arm64.cnb is not a zip or gzip archive

And uploading a zip succeeds--it only fails in the staging process. I'm currently seeing two paths forward with that aspect:

  1. Improve the error messaging and reject outright zip files on upload
  2. Add support for zip files

I think we are leaning toward number 1 as of now.

I see that we also need a capi-release PR for the default_app_lifecycle property.

We are happy to implement these changes (and other minor polishing aspects) if you are OK with it. Thoughts?

@pbusko
Copy link
Contributor Author

pbusko commented Apr 24, 2025

@sethboyles I agree that the right path is a set of more precise checks on CAPI side during the buildpack upload process. In order to support both, gzipped tarballs and zip archives, we would need to preserve the information about the buildpack format, so that the media type in the generated diego actions (which download buildpacks from blobstore) is correct.

Also tar format is the adopted format for OCI packages, which speaks in favor of this option as well.

We are happy to implement these changes (and other minor polishing aspects) if you are OK with it.

Absolutely, feel free to add what you see fit

Co-authored-by: Nicolas Bender <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>
@pbusko pbusko force-pushed the cnb-system-buildpacks branch from e9fbe7c to d3b75b6 Compare April 25, 2025 06:40
tomkennedy513 added a commit to tomkennedy513/cloud_controller_ng that referenced this pull request Apr 25, 2025
- This is a follow on to cloudfoundry#3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format

Signed-off-by: Tom Kennedy <[email protected]>

module VCAP::CloudController
class BuildpackCreateMessage < MetadataBaseMessage
MAX_BUILDPACK_NAME_LENGTH = 250
MAX_STACK_LENGTH = 250

register_allowed_keys %i[name stack position enabled locked]
register_allowed_keys %i[name stack position enabled locked lifecycle]
Copy link
Contributor

@Samze Samze Apr 29, 2025

Choose a reason for hiding this comment

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

We will need to update v3 docs for these changes

Copy link
Member

Choose a reason for hiding this comment

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

@@ -128,7 +127,7 @@ def app_lifecycle_hash
cnb_lifecycle_data
elsif requested?(:docker)
docker_lifecycle_data
else
elsif requested?(:buildpacks) || requested?(:buildpack) || requested?(:stack)
Copy link
Member

Choose a reason for hiding this comment

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

I've been debugging an issue where if you push a cnb app where the lifecycle is not specified in the manifest, but a buildpack is, then you get the following error:

cf push node-cnb --buildpack node
For application 'node-cnb': Lifecycle type cannot be changed from cnb to buildpack
FAILED

I believe it's because of this line, since specifying a buildpack doesnt necessarily mean the app is using the buildpack lifecycle. The same would be true for stack I believe. Not sure how to resolve this yet, I will keep looking into it tomorrow.

@sethboyles
Copy link
Member

I believe rebasing main into this branch will resolve some of the backwards compatibility test errors: fb1d344

sethboyles pushed a commit that referenced this pull request May 5, 2025
- This is a follow on to #3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format

Signed-off-by: Tom Kennedy <[email protected]>
sethboyles pushed a commit that referenced this pull request May 5, 2025
- This is a follow on to #3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format

Signed-off-by: Tom Kennedy <[email protected]>
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.

6 participants