Generated helm chart should align with other charts from ecosystem

[calind]

What do you want to happen?

Currently, the generated helm resources do not follow the conventions in the helm ecosystem, which makes them somewhat "alien" and hard to customize.

Here are a few examples:

• Resource names are not templated by {{ include "chart.fullname" . }}. In Helm charts, usually resource names take the form:

metadata:
  name: {{ include "chart.fullname" . }}
  labels:
    {{- include "chart.labels" . | nindent 4 }}

• Resources are missing features and customization options (like nodeSelector, tolerations). A chart scaffolded with helm create chart provides these default values:

# Default values for chart.
# This is a YAML-formatted file.
# Declare variables to be passed into your templates.

# This will set the replicaset count more information can be found here: https://kubernetes.io/docs/concepts/workloads/controllers/replicaset/
replicaCount: 1

# This sets the container image more information can be found here: https://kubernetes.io/docs/concepts/containers/images/
image:
  repository: nginx
  # This sets the pull policy for images.
  pullPolicy: IfNotPresent
  # Overrides the image tag whose default is the chart appVersion.
  tag: ""

# This is for the secrets for pulling an image from a private repository more information can be found here: https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/
imagePullSecrets: []
# This is to override the chart name.
nameOverride: ""
fullnameOverride: ""

# This section builds out the service account more information can be found here: https://kubernetes.io/docs/concepts/security/service-accounts/
serviceAccount:
  # Specifies whether a service account should be created
  create: true
  # Automatically mount a ServiceAccount's API credentials?
  automount: true
  # Annotations to add to the service account
  annotations: {}
  # The name of the service account to use.
  # If not set and create is true, a name is generated using the fullname template
  name: ""

# This is for setting Kubernetes Annotations to a Pod.
# For more information checkout: https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
podAnnotations: {}
# This is for setting Kubernetes Labels to a Pod.
# For more information checkout: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/
podLabels: {}

podSecurityContext: {}
  # fsGroup: 2000

securityContext: {}
  # capabilities:
  #   drop:
  #   - ALL
  # readOnlyRootFilesystem: true
  # runAsNonRoot: true
  # runAsUser: 1000

# This is for setting up a service more information can be found here: https://kubernetes.io/docs/concepts/services-networking/service/
service:
  # This sets the service type more information can be found here: https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types
  type: ClusterIP
  # This sets the ports more information can be found here: https://kubernetes.io/docs/concepts/services-networking/service/#field-spec-ports
  port: 80

# This block is for setting up the ingress for more information can be found here: https://kubernetes.io/docs/concepts/services-networking/ingress/
ingress:
  enabled: false
  className: ""
  annotations: {}
    # kubernetes.io/ingress.class: nginx
    # kubernetes.io/tls-acme: "true"
  hosts:
    - host: chart-example.local
      paths:
        - path: /
          pathType: ImplementationSpecific
  tls: []
  #  - secretName: chart-example-tls
  #    hosts:
  #      - chart-example.local

resources: {}
  # We usually recommend not to specify default resources and to leave this as a conscious
  # choice for the user. This also increases chances charts run on environments with little
  # resources, such as Minikube. If you do want to specify resources, uncomment the following
  # lines, adjust them as necessary, and remove the curly braces after 'resources:'.
  # limits:
  #   cpu: 100m
  #   memory: 128Mi
  # requests:
  #   cpu: 100m
  #   memory: 128Mi

# This is to setup the liveness and readiness probes more information can be found here: https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/
livenessProbe:
  httpGet:
    path: /
    port: http
readinessProbe:
  httpGet:
    path: /
    port: http

# This section is for setting up autoscaling more information can be found here: https://kubernetes.io/docs/concepts/workloads/autoscaling/
autoscaling:
  enabled: false
  minReplicas: 1
  maxReplicas: 100
  targetCPUUtilizationPercentage: 80
  # targetMemoryUtilizationPercentage: 80

# Additional volumes on the output Deployment definition.
volumes: []
# - name: foo
#   secret:
#     secretName: mysecret
#     optional: false

# Additional volumeMounts on the output Deployment definition.
volumeMounts: []
# - name: foo
#   mountPath: "/etc/foo"
#   readOnly: true

nodeSelector: {}

tolerations: []

affinity: {}

My point here is that the helm chart generator should use helm create chart as a starting point, at least regarding the structure of the values file and the way it names resources.

Extra Labels

No response

[camilamacedo86]

Hi @calind

Thank you for your feedback.
First, we need to keep in mind that we cannot add to the HelmChart something that does not exist with the scaffold produced in the YAML/kustomize at all.

Then, keeping the above in mind, please help us identify what aspects of your perspective we should change.
Could you please list them out with the motivations?

[calind]

Hi @camilamacedo86,

First, we need to keep in mind that we cannot add to the HelmChart something that does not exist with the scaffold produced in the YAML/kustomize at all.

I agree that we cannot/should not add things that are not scaffolded with kustomize.

Moving on to the changes:

• We should template the resource names by {{ include "chart.fullname" . }}

  This is standard practice and expectation when templating for Helm, as it allows having multiple installs within the same cluster or more advanced deployment scenarios like blue/green, or purely to allow custom naming.

  Currently, all resource names are hardcoded to the {{ .ProjectName }} and cannot be customized. For this, we also need to add the chart.fullname helper. To give some examples, for a project called tomatino:

  • manager deployment name: tomatino-controller-manager -> {{ include "chart.fullname" . }}
  • metrics service: tomatino-controller-manager-metrics-service -> {{ include "chart.fullname" . }}-metrics
  • metrics service certificate: metrics-server-cert -> {{ include "chart.fullname" . }}-metrics-tls

  Outside of templates/manager/manager.yaml, the templates are getting rewritten, so modifying the generated templates is not an option for customizing this on your own.

• Template helpers should be namespaced to {{ .ProjectName }}.

  The helpers in the scaffolded _helpers.tpl are namespaced to the chart prefix (ie, chart.name, chart.labels). This is problematic, as in Helm, the namespace for all these helpers is flat, so it's impossible to have two operators under an umbrella chart, for example.

  As mentioned above, template names are global. As a result of this, if two templates are declared with the same name the last occurrence will be the one that is used. Since templates in subcharts are compiled together with top-level templates, it is best to name your templates with chart specific names. A popular naming convention is to prefix each defined template with the name of the chart: {{ define "mychart.labels" }}.

  https://helm.sh/docs/chart_template_guide/named_templates/

• metrics.enabled=false should disable the metrics server as well

  Currently, setting metrics.enabled=false requires manually setting the controllerManager.args and removing the --metrics-bind-address=:8443 arg. When automating, this becomes cumbersome, and instead of just flipping a switch on/off, you are required to juggle and account for other values as well.

  A simple way to solve this is to check for metrics.enabled in the manager deployment template and add the argument accordingly.

• Align with the values.yaml format, scaffolded by helm create chart

  This is more cosmetic, but I'm proposing it for completeness and for the fact that it allows some customizations, people using Helm are accustomed to. The scaffolded values.yaml using Kubebuilder's Helm plugin puts the majority of values under the controllerManager key, and for some settings uses custom namings. To give some examples:

  • controllerManager.replicas -> replicaCount
  • controllerManager.container.image.tag -> image.tag
  • controllerManager.container.image.repository -> image.repository
  • controllerManager.serviceAccountName is hardcoded, while in the helm scaffolded values, it is a map that offers more flexibility (ie, one can add annotations, required for integrating the controller with Google Cloud Workload Identity, for example)

• In the end, I would also propose to have something like a "correctness check", so that if you do a helm template PROJECT_NAME, you should get the same resources (and names) as the ones generated using kustomize.

[calind]

Another situation that I've found is that CRDs are placed in templates/crd folder, under a crd.enable flag. Helm handles CRDs on it's own, and has dedicated flags for them, like --skip-crds.

CRD YAML files should be placed in the crds/ directory inside of a chart. Multiple CRDs (separated by YAML start and end markers) may be placed in the same file. Helm will attempt to load all of the files in the CRD directory into Kubernetes.

CRD files cannot be templated. They must be plain YAML documents.

https://helm.sh/docs/topics/charts/#custom-resource-definitions-crds

[camilamacedo86]

Hi @calind

Thank you very much for your input, we will need to analyse all points one by one.

CRDs

Another situation that I've found is that CRDs are placed in templates/crd folder, under a crd.enable flag. Helm handles CRDs on it's own, and has dedicated flags for them, like --skip-crds.

The CRD are place in this the crd director: https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v4-with-plugins/dist/chart/templates/crd

• We had a big discussion about where should be the CRDs see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/helm-chart-autogenerate-plugin.md
• See also; 📖 Proposal : New Plugin to allow project distribution via helm charts #3632 (comment) mainly the convey: 📖 Proposal : New Plugin to allow project distribution via helm charts #3632 (comment)

So, please let us know if the above is not sufficient, how it is currently, and how you would like it to be, so that we can better understand your motivation. A dedicated issue (only for this) would be better for this one. Could you please raise it if that is the case?

Metrics Enabled

metrics.enabled=false should disable the metrics server as well

It seems like a bug. Could we have an issue only for this as well?
Therefore, it makes it easier for us to discuss each case scenario.

We should template the resource names by {{ include "chart.fullname" . }}

We have a fundamental problem because it manifests in the form of what is generated by controller-gen
Also, we have some files that we need to template: https://github.com/kubernetes-sigs/kubebuilder/tree/master/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates

See here other related requests: https://github.com/kubernetes-sigs/kubebuilder/milestone/39

Therefore, I think the first step is we try to use kustomize.yaml as input for the HelmCharts and try to drop any template that we have, see: #4833

Align with the values.YAML format, scaffolded by helm create chart

I agree that we need to change it in a way that is more aligned with common practices and helm expectations.

Also, if you would like to start proposing changes via PRs and contribute towards the Helm initiative, please feel free. I want you to know that all help is welcome. 🙌

Thank you a lot for your help and input. 🙌