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

RFC: Introduce Buildpack/ClusterBuildpack Resources #931

Merged
merged 7 commits into from
Sep 1, 2022

Conversation

matthewmcnew
Copy link
Collaborator

@matthewmcnew matthewmcnew commented Mar 8, 2022

@dumez-k dumez-k self-requested a review March 8, 2022 15:08
- group:
- id: paketo-buildpacks/java
```
The `paketo-buildpacks/java` buildpack referenced in the Builder order would be sourced from the Buildpack resource within the `my-builder` namespace or the ClusterBuildpack resource that provides the `paketo-buildpacks/java` with the highest version number as defined by semver. If multiple Buildpack resources provide the same selected Buildpack the Builder should reconcile with an error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this get you into a state where someone else creates a buildpack or clusterbuildpack providing the same id and version that would inadvertently cause your builder to error out

Copy link
Contributor

Choose a reason for hiding this comment

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

If multiple Buildpack resources provide the same selected Buildpack the Builder should reconcile with an error.

Maybe buildpacks should fail to reconcile if a buildpack with the same version/id combo already exists.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is a risk but, inherent if a builder is letting its versions float without direct references to a buildpack.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe buildpacks should fail to reconcile if a buildpack with the same version/id combo already exists.

I think that would be difficult to enforce.

@matthewmcnew matthewmcnew marked this pull request as ready for review May 25, 2022 01:21
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2022

Codecov Report

Merging #931 (7738995) into main (783820c) will increase coverage by 0.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #931      +/-   ##
==========================================
+ Coverage   69.42%   69.77%   +0.34%     
==========================================
  Files         119      119              
  Lines        5370     5574     +204     
==========================================
+ Hits         3728     3889     +161     
- Misses       1285     1305      +20     
- Partials      357      380      +23     
Impacted Files Coverage Δ
pkg/duckbuilder/duck_builder.go 73.33% <0.00%> (-26.67%) ⬇️
pkg/registry/client.go 60.97% <0.00%> (-8.59%) ⬇️
pkg/config/lifecycle_provider.go 75.92% <0.00%> (-7.72%) ⬇️
pkg/apis/build/v1alpha2/image_conversion.go 88.37% <0.00%> (-6.00%) ⬇️
pkg/reconciler/clusterstack/clusterstack.go 55.17% <0.00%> (-3.01%) ⬇️
pkg/reconciler/clusterstore/clusterstore.go 55.17% <0.00%> (-3.01%) ⬇️
pkg/reconciler/build/build.go 70.07% <0.00%> (-2.73%) ⬇️
pkg/reconciler/sourceresolver/sourceresolver.go 43.63% <0.00%> (-2.52%) ⬇️
pkg/reconciler/builder/builder.go 44.11% <0.00%> (-2.04%) ⬇️
pkg/reconciler/clusterbuilder/clusterbuilder.go 44.11% <0.00%> (-2.04%) ⬇️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 783820c...7738995. Read the comment docs.



**Prior Art:**
* [kapp controller](https://carvel.dev/kapp-controller/) models available Package versions as indepdent Package resources.
* [kapp controller](https://carvel.dev/kapp-controller/) models available Package versions as independent Package resources.

**Risks:**
This will require introducing a new api version within kpack. Future kpack releases will need to maintain support for ClusterStore resources until the kpack apis which contain the ClusterStore are removed.
Copy link

Choose a reason for hiding this comment

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

Another risk that should be considered is the possible need for a conversion webhook or some mechanism to allow migration from clusterstores to clusterbuildpacks when clusterstores are removed. at minimal documentation needs to be provided to ease the process for users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This RFC does not propose the ClusterStore to be removed so, migration of resources is outside the scope of the RFC. I will add this as a risk though.

rfcs/0000-buildpack-resource.md Outdated Show resolved Hide resolved
rfcs/0000-buildpack-resource.md Outdated Show resolved Hide resolved

Some kpack clusters may be operated in an environment where different personas manage available buildpacks from the persona that manages the builder config. This is especially likely in an enterprise environment where only blessed buildpacks are available to developers but, developers can modify their buildpack usage by owning namespace scoped Builders. This can be restricted via RBAC with a distinct Buildpack resource but, would not be possible if Builders directly referenced buildpack images as developers could then use any Buildpack.

These distinct personas may also exist in non-enterprise settings where buildpacks and builders have independent lifecycles. For example, the kpack build cluster is a multitenant kpack cluster that provides up-to-date dependencies for multiple teams by continually updating new paketo dependencies. The kpack components are built with a kpack specific namespace Builder that utilizes the [Paketo Go Buildpack](https://github.com/paketo-buildpacks/go) alongside a custom buildpack to build libgit2. The kpack namespace scoped Builder continually pulls in new versions of the Paketo Go buildpack. If the buildpack images were configured on the Builder resource the process that updated the Paketo Go Buildpack would also need to update the kpack Custom Builder. This would be unnecessarily complicated and may not even be possible if the cluster operator does not know all the uses of a Buildpack within the cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be unnecessarily complicated and may not even be possible if the cluster operator does not know all the uses of a Buildpack within the cluster.

Good point. Thank you for enumerating the drawbacks of having the Builder reference the buildpack images.

Copy link
Collaborator

@tomkennedy513 tomkennedy513 left a comment

Choose a reason for hiding this comment

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

This is fantastic. Great work @matthewmcnew

Copy link
Contributor

@stormqueen1990 stormqueen1990 left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

rfcs/0000-buildpack-resource.md Outdated Show resolved Hide resolved
@matthewmcnew matthewmcnew requested a review from mgibson1121 June 30, 2022 18:37
@matthewmcnew matthewmcnew requested a review from mvalliath June 30, 2022 18:37
@jjbustamante jjbustamante self-requested a review June 30, 2022 20:26
Copy link
Contributor

@jjbustamante jjbustamante left a comment

Choose a reason for hiding this comment

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

Seems good to me!


The status of the ClusterBuildpack should match the status of the Buildpack resource.

##### ClusterStore Resources
Copy link

@mgibson1121 mgibson1121 Jun 30, 2022

Choose a reason for hiding this comment

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

Will there be any migration concerns? One problem that jumped to mind is that this may increase complexity for getting started compared to configuring a ClusterStore. Although the user has to configure one less resource in this new world, they can currently pretty much copy and paste several lines of paketo buildpackages in one yaml file to configure the clusterstore. From this RFC it seems like the user would have to configure a buildpack/clusterbuildpack for each buildpackage they want to reference in a builder/clusterbuilder.

Most likely out of scope of this RFC, but perhaps it's additional impetus for the project to consider a more streamlined buildpack and stack bootstrapping process

Copy link
Collaborator Author

@matthewmcnew matthewmcnew Jul 5, 2022

Choose a reason for hiding this comment

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

One problem that jumped to mind is that this may increase complexity for getting started compared to configuring a ClusterStore

I can add this to the RFC but, I hope that this will actually help simplify onboarding. Currently, because the Clusterstore is a monolithic resource users essentially need to use the kp cli and a published descriptor to be used alongside the cli. However, a Buildpack resource enables the Buildpacks to be managed independently which means a descriptor is not needed. Instead users can directly apply a preconfigured set of Buildpack/Builder/Stack resources with kubectl apply to get started fast. (This does require this WIP RFC to be implemented as well #932)

Copy link

@AidanDelaney AidanDelaney left a comment

Choose a reason for hiding this comment

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

This is really great!

@tcdowney
Copy link
Contributor

tcdowney commented Aug 1, 2022

Just want to comment saying that I am excited for this update. As a user of kpack / "platform author" of platforms using kpack, these two points really resonate with me.

It is common for a buildpack to be used within multiple Builders or ClusterBuilders on a kpack cluster. Externalizing the specification of a Buildpack location within one resource on the Cluster removes unnecessary duplication. Updating a buildpack can occur with the create/update of a single resource which will propagate out to multiple Builder resources on a cluster.

Some kpack clusters may be operated in an environment where different personas manage available buildpacks from the persona that manages the builder config. This is especially likely in an enterprise environment where only blessed buildpacks are available to developers but, developers can modify their buildpack usage by owning namespace scoped Builders. This can be restricted via RBAC with a distinct Buildpack resource but, would not be possible if Builders directly referenced buildpack images as developers could then use any Buildpack.

We had a use case where one team managed a set of the "blessed" buildplacks for the platform and we wanted to provide a few ancillary buildpacks around them. We didn't want to just copy the "blessed" ClusterStore wholesale into our supertset ClusterStore because we wanted to be able to track and consume updates to those buildpacks as they happened. I was originally thinking we needed someway to specify ClusterStore on a per-buildpack basis in the (Cluster)Builder, but this solution is more elegant and should meet our needs. Thanks!

@matthewmcnew matthewmcnew reopened this Sep 1, 2022
@matthewmcnew matthewmcnew merged commit 79b4727 into main Sep 1, 2022
@matthewmcnew matthewmcnew deleted the buildpack-resource branch September 1, 2022 19:23
@pviraj59
Copy link
Contributor

solves #964

@chenbh chenbh mentioned this pull request Dec 1, 2022
3 tasks
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.