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.

Sign up for GitHub

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

Jump to bottom

✨[WIP]feat: Add the proposal about the helm chart autogenerate plugin. #3632

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+271 −0
Open

✨[WIP]feat: Add the proposal about the helm chart autogenerate plugin. #3632

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6432ab
Add the proposal about the helm chart autogenerate plugin.
dashanji Sep 24, 2023
bfbf651
Improve the design via comments.
dashanji Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
271 changes: 271 additions & 0 deletions designs/helm-chart-autogenerate-plugin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,271 @@
| Authors | Creation Date | Status | Extra |
|---------------|---------------|-------------|---|
| @name | date | Implementeble | - |

# Allow users package Operator with Helm Charts

This proposal aims to introduce an optional mechanism that allows users to generate a Helm Chart from their Kubebuilder-scaffolded project. This will enable them to effectively package and distribute their solutions.

To achieve this goal, we can introduce a Helm Chart plugin (i.e. helm-chart/v1alpha), which will provide the scaffolds. Alternatively, we could implement a command to generate the Helm Chart from a provided Kubebuilder project.

## Example

camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
**Via a new Optional Plugin**

By running `kubebuilder init --plugins=go/v4,helm-chart/v1alpha` or `kubebuilder edit --plugins=helm-chart/v1alpha`, the scaffolds of Helm Chart to package the Operator would be created in the project.

Example:
```shell
$ tree
.
..
├── PROJECT
...
├── deploy
│ └── helm-chart // The helm chart with all values will be scaffold under a directory of the project
...
```

**Via a new command (Alternative Option)**
By running the following command, the plugin will generate a helm chart from the specific kustomize directory and output it to the directory specified by the `--output` flag.

```shell
kubebuilder alpha generate-helm-chart --from=<path> --output=<path>
```
Comment on lines +29 to +34
Copy link
Member

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


## Open Questions [optional]

TBF
Copy link
Member

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


## Summary

The Helm Chart created by the project should accurately reflect the default values utilized in Kubebuilder. Furthermore, any newly generated CRDs or Webhooks should be seamlessly integrated into the Helm Chart. To accomplish this, the new plugin needs to incorporate all the subCommands available to the [kustomize plugin](https://book.kubebuilder.io/plugins/kustomize-v2). Additionally, the edit feature, similar to what is available in the optional [Grafana plugin](https://book.kubebuilder.io/plugins/grafana-v1-alpha), should be integrated to enable modifications in projects that were previously created.

Thus, the init subCommand should take charge of creating the Helm Chart directory and scaffolding the base of the Helm Chart, based on the default scaffold provided by Kustomize. This means that the same default values, scaffolded to the manager and proxy, along with the options to enable metrics via Prometheus, cert-manager, and webhooks, should be disabled (commented out) by default, as seen in the [config/default/kustomization.yaml](https://github.com/kubernetes-sigs/kubebuilder/blob/f4744670e6fc8ed29f87161d39a8f2f3838c27f4/testdata/project-v4/config/default/kustomization.yaml#L21-L27) file.

Subsequently, if a user decides to scaffold webhooks, we should uncomment the relevant sections. Please refer to PRs https://github.com/kubernetes-sigs/kubebuilder/pull/3627 and https://github.com/kubernetes-sigs/kubebuilder/pull/3629 for examples of this implementation.

Lastly, when new API(s) (CRDs) are introduced, they must be added to the Helm Chart as well. As a result, the Makefile targets make generate and make manifest should be modified when the proposed plugin is in use to ensure that CRDs are accurately copied and pasted into the Helm Chart's respective directory.

To allow users to customize their Helm Charts, we should avoid overwriting files within the Helm Chart, with the exception of CRDs/webhooks, as changes made to these must be mirrored in the Helm Chart. However, what is scaffolded by the init/edit should remain primarily unchanged. We would only uncomment the options as described above.

## Motivation

Kubebuilder users currently face the challenges of lacking an officially supported distribution mechanism for Helm Chart. They seek to:

- Harness the power of Helm Chart as a package manager for the Operator, enabling seamless adaptation to diverse deployment environments.
- Take advantage of Helm's dependency management capabilities to simplify the installation process of Operator dependencies, such as cert-manager.
- Seamlessly integrate with Helm's ecosystem, including FluxCD, to efficiently manage the Operator.

However, Kubebuilder does not offer any utilities to facilitate the distribution of projects currently. One could clone the entire project and utilize the Makefile targets to apply the solution on clusters. Consequently, this proposal aims to introduce a method that allows Kubebuilder users to easily distribute their projects through Helm Charts, a strategy that many similar many well-known operators contains the distribution of helm charts:

- [mongodb](https://artifacthub.io/packages/helm/mongodb-helm-charts/community-operator
)
- [cert-manager](https://cert-manager.io/v1.6-docs/installation/helm/#1-add-the-jetstack-helm-repository)
- [prometheus](https://bitnami.com/stack/prometheus-operator/helm)
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
- [aws-load-balancer-controller](https://github.com/kubernetes-sigs/aws-load-balancer-controller/tree/main/helm/aws-load-balancer-controller)


**NOTE** For further context see the [discussion topic](https://github.com/kubernetes-sigs/kubebuilder/discussions/3074)

### Goals

- Allow Kubebuilder users distribute their projects using Helm easily.
- Make the best effort to preserve any customizations made to the Helm Charts by the users.
Copy link
Member

@camilamacedo86 camilamacedo86 Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- Make the best effort to preserve any customizations made to the Helm Charts by the users.
- Make the best effort to preserve any customizations made to the Helm Charts by the users - which means we will skip syncs in the values.ymal.
- Stick with Helm layout definitions and externalize into the relevant values-only options to distribute the default scaffold done by Kubebuilder. We should follow https://helm.sh/docs/chart_best_practices

I think we need to add something like that here.

Copy link

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 the templates/*.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 in templates/ 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:

kind: Deployment
# (...)
spec:
  template:
    spec:
      image: foo/bar:latest

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:

kind: Deployment
# (...)
spec:
  template:
    spec:
      image: foo/bar:latest
      containers:
      - name: not-important-for-the-example
        {{- if .Values.env }}
        env:
          {{- $k, $v := range .Values.env }}
          - name: {{ $k }}
            value: {{ $v }}
          {{- end }}
        {{- end }}

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):

kind: Deployment
# (...)
spec:
  template:
    spec:
      {{/* +kubebuilder:scaffolding:image */}}
      image: foo/bar:latest
      containers:
      - name: not-important-for-the-example
        {{- if .Values.env }}
        env:
          {{- $k, $v := range .Values.env }}
          - name: {{ $k }}
            value: {{ $v }}
          {{- end }}
        {{- end }}


### Non-Goals

- Converting any Kustomize configuration to Helm Charts like [helmify](https://github.com/arttor/helmify) does.
- Support the deprecated plugins. This option should be supported from go/v4 and kustomize/v2
- Introduce support for Helm in addition to Kustomize, or replace Kustomize with Helm entirely, similar to the approach taken by Operator-SDK, thereby allowing users to utilize Helm Charts to build their Operators.

Copy link
Member

@camilamacedo86 camilamacedo86 Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- Attend standard practices that deviate from HelmCharts layout, definition, or conventions to workaround its limitations
- Provide values and options to allow users to perform customizations on the HelmChart of features that are not used by default in the Operator layout

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree.

## Proposal

camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
The new optional plugin will create a new directory, `deploy/helm-chart`, where the scaffolding of the project should be constructed as follows:
//TODO: We need a better definition
// We must create a Helm Chart with the default Kubebuilder scaffold to add here
```shell
❯ tree deploy/helm-chart
helm-chart
├── Chart.yaml
├── crds
│ ├── <CRDs YAML files generate under config/crds/>
Comment on lines +91 to +92
Copy link

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"

Copy link
Member

Choose a reason for hiding this comment

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

in helm was not what we wanted as it precluded us from every upgrading the CRD

What do you mean with this one?
Could you please give an example?

Copy link

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 updates

Copy link
Member

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:

  • Versioning CRDs: When making significant changes to a CRD schema, consider versioning the CRD. This allows you to deploy a new version of the CRD without affecting existing resources that use the old schema.
  • Helm Hooks: You could use Helm hooks to manage the lifecycle of CRDs. For example, a pre-upgrade hook could apply changes to a CRD before the rest of the Helm chart is processed.

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.

Copy link

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.

Copy link
Member

@camilamacedo86 camilamacedo86 Dec 8, 2023 •
edited
Loading

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.

Copy link

Choose a reason for hiding this comment

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

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.

Totally agree you cant please everyone, however an option for either the crd/ folder or just put them in templates/ seems easy enough low friction implementation wise to offer it as an option that i believe that will account for 90%+ of user needs.

Copy link
Member

@camilamacedo86 camilamacedo86 Dec 8, 2023 •
edited
Loading

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.

Copy link

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.

^ 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.

Agreed.

Copy link
Member

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 of helm.

I would suggest supporting one or both of the following options:

  1. As was suggested by @Jay-Madden, the better choice is to put the CRDs into the 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.
  2. Scaffold separate charts for the APIs.

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

├── templates
│ ├── deployment.yaml
│ ├── _helpers.tpl
│ ├── role_binding.yaml
│ ├── role.yaml
│ ├── <all under config/rbac less the edit and view roles which are helpers for admins>
└── values.yaml
```

To reduce the user adaptation, the command
leverage the existing kustomize default directory and create the similar layout of the helm chart from it. The generated `values.yaml` is as follows:
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved

```yaml
# global configurations
nameOverride: ""

fullnameOverride: ""

## CRD configuration under the `config/crd` directory
crd:
## Whether the `resources` field contains `crd` in the `config/default/kustomization.yaml` file
create: false

## RBAC configuration under the `config/rbac` directory
rbac:
## Whether the `resources` field contains `rbac` in the `config/default/kustomization.yaml` file
create: true
serviceAccountName: "controller-manager"


## Manager configuration referent to `config/manager/manager.yaml`
manager:
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
## The Security Context of the manager Pod.
## More info: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-pod
podSecurityContext:
runAsNonRoot: true

## Configure the kubeRbacProxy container.
kubeRbacProxy:
image:
registry: gcr.io
repository: "kubebuilder/kube-rbac-proxy"
tag: v0.13.0
pullPolicy: IfNotPresent
Copy link
Member

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


## Configure the resources.
## More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
resources:
limits:
cpu: 500m
memory: 128Mi
requests:
cpu: 5m
memory: 64Mi

## Configure the controller container.
controller:
image:
registry: docker.io
repository: ""
tag: latest
pullPolicy: IfNotPresent

## The environment variables of the controller container.
## More info: https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/
env: []
Copy link
Member

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.

Copy link

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?

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.


## Configure extra options for liveness probe.
## More info: https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/#configure-probes
livenessProbe:
enabled: true
httpGet:
path: /healthz
port: 8081
initialDelaySeconds: 15
periodSeconds: 20

readinessProbe:
enabled: true
httpGet:
path: /readyz
port: 8081
initialDelaySeconds: 5
periodSeconds: 10

## Configure the resources.
## More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
resources:
limits:
cpu: 500m
memory: 128Mi
requests:
cpu: 10m
memory: 64Mi

## Configure the scheduling rules.
## More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#affinity-and-anti-affinity
affinity: {}

## More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#nodeselector
nodeSelector: {}

## More info: https://kubernetes.io/docs/concepts/configuration/taint-and-toleration
tolerations: {}
Comment on lines +188 to +196
Copy link
Member

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


## Configure the rbac roles.
serviceAccountName: ""
Copy link
Member

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


## Webhook configuration under the `config/webhook` directory
webhook:
## Whether the `resources` field contains `webhook` in the `config/default/kustomization.yaml` file
enabled: true
## More info: https://kubernetes.io/docs/concepts/services-networking/service/
service:
ports:
- name: webhook-server
protocol: TCP
containerPort: 9443

metrics:
enabled: true
service:
type: ClusterIP
ports:
- port: 8443
protocol: TCP

## Configure the cert-manager dependency.
certmanager:
enabled: true
installCRDs: true
## Add the owner reference to the certificate.
extraArgs:
- --enable-certificate-owner-ref=true

## Prometheus configuration under the `config/prometheus` directory
prometheus:
## Whether the `resources` field contains `prometheus` in the `config/default/kustomization.yaml` file
enabled: false

## The domain name of Kubernetes cluster
## More info: https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/
clusterDomain: cluster.local
```

### User Stories

- As a developer, I want to be able to generate a helm chart from a kustomize directory so that I can distribute the helm chart to my users. Also, I want the
generation to be as simple as possible without the need to write any additional duplicate files.

- As a user, I want the helm chart can cover all potential configurations when I deploy it on the Kubernetes cluster.

Copy link

Choose a reason for hiding this comment

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

Suggested change
- As a platform engineer, I want to be able to manage different versions and configurations of a controller/operator across multiple clusters and environments based on the same distribution artifact (Helm Chart), with versioning and dependency locking for supply chain security.

### Implementation Details/Notes/Constraints [optional]

TBF

### Risks and Mitigations

**Difficulty in Maintaining the Solution**

Maintaining the solution may prove challenging in the long term, particularly if it does not gain community adoption and, consequently, collaboration. To mitigate this risk, the proposal aims to introduce an optional alpha plugin or to implement it through an alpha command. This approach provides us with greater flexibility to make adjustments or, if necessary, to deprecate the feature without definitively compromising support.

Additionally, it is crucial to cover the solution with end-to-end tests to ensure that, despite any changes, the Helm Chart remains deployable and continues to function well, as is the practice with our other plugins and solutions.

### Proof of Concept [optional]

Refer to the open source tool
[helmify](https://github.com/arttor/helmify).


## Drawbacks

**Generation of Deployable Helm Chart:**

There might be challenges in generating a deployable Helm Chart without human intervention, similar to the current process for Kubebuilder projects.

## Alternatives

TBF
Copy link
Member

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