Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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]feat: Add the proposal about the helm chart autogenerate plugin. #3632
base: master
Are you sure you want to change the base?
✨[WIP]feat: Add the proposal about the helm chart autogenerate plugin. #3632
Changes from all commits
d6432ab
bfbf651
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to move this one to Alternative section
Also, add the pros and cons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IHMO: We need to add here: https://github.com/kubernetes-sigs/kubebuilder/pull/3632/files#r1420716711
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add something like that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think skipping syncs in
values.yaml
also kind of implies in skipping at least part of syncs in thetemplates/*.yaml
files, or use markers or some other mechanism so we clearly mark which parts kubebuilder can and can't change.My point is that if the user has custom variables in
values.yaml
, it necessarily means they also changed something intemplates/
to get that value, optionally transform it and render in the resulting manifest.Not sure if worth mentioning in the proposal, but just wanted to make sure we're on the same page here.
Example
The name of the image would probably be sourced from the Makefile/env, and inserted directly into a
templates/deployment.yaml
:Let's assume our values file doesn't provide a way for users to pass in env vars. The user would do something like this in the template:
When syncing the template again, we can't remove the user customization (or this gets pretty useless pretty quickly -- if I have to write my own tooling to make any kind of customization to run on top of kubebuilder, why don't I just write all of this tooling without a Helm Chart plugin in the first place?).
On the other hand, we must make sure the image is up-to-date.
What I mentioned about markers was something like (just an example, not an actual suggestion for how the marker should be named, placed, etc):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IHMO: We need to add more one here.
Otherwise, we will end up in the business to start to discuss how company/team A does and how company/team B does when we cannot attend to everybody. So, we need to stick with Helm definitions and official docs to generate the HelmChart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some concern needs to be given to how this is done. In our experience converting our kubebuilder projects to helm charts the CRD folder in helm was not what we wanted as it precluded us from every upgrading the CRD again with future changes. We ended up treating the CRD as another file in templates to facilitate upgrades. this is an approach used by several operators today however with the obvious drawback of "if you uninstall the operator you uninstall the crds"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with this one?
Could you please give an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once helm installs a CRD from the
crd/
folder it will skip it if it detects that the CRD is already installed.This meant we were unable to change the crd schema after initial installation. We just put the CRD definition in the
templates/
folder to facilitate actual updatesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Jay-Madden
It seems a good point. it seems a good approach but that means that the lifecycle of the CRD is tied to the Helm release. This could potentially lead to issues if the Helm chart is deleted or if different releases try to manage the same CRD. Also, another approaches seems that are:
So, I think we need an Open Question in the proposal
and we need POC and investigate the pros and cons of each case scenario
as check how people has been doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a newer user of kubebuilder, I am very excited about this proposal!
An additional approach for CRDs could be a separate helm chart for crds, alongside the main chart for deploying an operator. The AWS Karpenter Operator uses this approach and it has allowed full control of both CRD versions and operator version in automated production environments using helm technologies without some of the drawbacks like deleting crds when operator chart is deleted.
This approach also plays nicely with tools like terraform and ArgoCD alike.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helm typically uses the crd/ directory as the default location for Custom Resource Definitions (CRDs). In my opinion, if we're developing a plugin to distribute Operators via Helm, it should adapt to the Helm layout.
However, it's important to recognize that preferences vary across teams and companies; we can't satisfy everyone. Our best approach is to adhere to Helm's standard layout and guidelines.
From my experience, and as observed in many Helm Charts distributing Operators (such as CertManager), it's crucial to ensure that CRDs are applied before installing or upgrading a Helm Chart. This practice is generally achievable with any solution (terraform, argocd etc). However, accommodating those variations to try to work around the Helm limitations may be beyond our scope.
^ PS.: IMO, we need to add it in the OpenQuestion, and something like the above would be the answer. The plugin will package the Operator following the Helm's layout definition - we cannot attend to everybody's deviations and personal preferences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree you cant please everyone, however an option for either the
crd/
folder or just put them intemplates/
seems easy enough low friction implementation wise to offer it as an option that i believe that will account for 90%+ of user needs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Jay-Madden
Helm best practices: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you
That is what we should follow up
If a user wants to change things, then they can tool their releases to change places where the CRD will be distributed as they please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the best practices push, and I also get the point @Jay-Madden makes that this seems simple to implement and would cover a very substantial amount of use-cases, where the best practices wouldn't. A nice-to-have feat.
I think that this can be done as a further improvement if/when folks request it as a feature, and there is no need to support this on a first version. Let's get this out of the paper first.
Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chiming in with my two cents on this crucial question.
The helm project essentially punted on CRD handling. Method 1 (linked above) is essentially that helm will create the CRDs on the initial install... and never touch them again. Last I checked, it will silently ignore CRDs in upgraded chart versions, even if they differ from what is installed. This is potentially surprising and unexpected behavior, and I think it would be wise for the kubebuilder project to avoid supporting or endorsing it.
I believe the basis for this decision by the helm project is that upgrading CRDs is a potentially destructive operation, so the maintainers opted for a safe implementation to avoid
helm
itself being blamed for a CRD update the broke a cluster. The design rather puts the onus on cluster administrators to upgrade the CRDs fully out-of-band ofhelm
.I would suggest supporting one or both of the following options:
templates/
directory. This will cause helm to treat the CRDs like any other object and it will upgrade them as expected. The one downside to this approach is that it means that you cannot put both a CRD and a CR of that type in the same chart. Helm is unable to create the CRD, wait for it to be registered, and then proceed with the CR creation in a single release install/upgrade. This leads me to option 2.Option 2 is actually its own can of worms, but one that likely needs to be opened sooner rather than later. If a CRD specified a conversion webhook, then the "API chart" would actually need to contain the CRDs and the webhook service/workload (and in many cases, it would also make sense to include validating/mutating webhooks in an "API chart"). This structure would then have implications on the scaffolding of the actual go project itself because it would need to scaffold separate main modules and image builds for the webhooks and controllers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one we need put the data that we have in : https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/config/default/manager_auth_proxy_patch.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have our default Kubebuilder Operator layout or any plugin?
If not, then I think we need to remove it.
If yes, we need to add a comment here to justify why/when we need/use this one.
Or goal is just package the Operator in HelmChart
Any extra thing or customizations that might be done on top shows out of our scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that here we should only take inputs we already have in kustomize, for example? Inputs that kubebuilder currently already knows how to work with.
I think that makes sense, just clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. The values must be exactly only what we have in the default scaffold
We need to stick with each otherwise will be very hard we maintain and make it work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need put the same value from: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/config/manager/manager.yaml#L101
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think we need disccuss the command option
ALso, add the pros and cons of each one of them