Helm Chart: Adding support for secrets enabled multi-cluster#4938
Helm Chart: Adding support for secrets enabled multi-cluster#4938cmcgalliard wants to merge 12 commits intokubernetes-sigs:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cmcgalliard The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
Welcome @cmcgalliard! |
There was a problem hiding this comment.
Pull request overview
Adds Helm chart support for configuring Headlamp multi-cluster access via one or more kubeconfig files stored in Kubernetes Secrets, by mounting each secret and building a KUBECONFIG env var pointing at the mounted files.
Changes:
- Introduces
config.kubeconfigSecretsin chart values and documents how to use it. - Updates the Deployment template to mount each referenced secret and set
KUBECONFIGaccordingly. - Adds Helm helper templates to detect/configure kubeconfig secret usage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| charts/headlamp/values.yaml | Adds config.kubeconfigSecrets values entry with inline documentation. |
| charts/headlamp/templates/deployment.yaml | Mounts kubeconfig secrets as volumes/volumeMounts and sets KUBECONFIG env var. |
| charts/headlamp/templates/_helpers.tpl | Adds helpers to build the KUBECONFIG path and detect whether secrets are configured. |
| charts/headlamp/README.md | Documents multi-cluster configuration via kubeconfig secrets. |
| | Key | Type | Default | Description | | ||
| |-----|------|---------|-------------| | ||
| | config.kubeconfigSecrets | list | `[]` | List of secrets containing kubeconfig files | | ||
| | config.kubeconfigSecrets[].secretName | string | required | Name of the secret containing kubeconfig | | ||
| | config.kubeconfigSecrets[].key | string | `"config"` | Key within the secret data (optional, defaults to "config") | | ||
|
|
There was a problem hiding this comment.
The multi-cluster table is malformed Markdown: the header and rows start with || instead of |, so it won’t render as a proper table. Please fix the leading pipes for the table in this section.
There was a problem hiding this comment.
I don't see a line where this is the case
| {{- if include "headlamp.hasKubeconfigSecrets" . }} | ||
| - name: KUBECONFIG | ||
| value: {{ include "headlamp.kubeconfigPath" . | quote }} | ||
| {{- end }} | ||
| {{- if .Values.env }} | ||
| {{- toYaml .Values.env | nindent 12 }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- else }} | ||
| env: | ||
| {{- if include "headlamp.hasKubeconfigSecrets" . }} | ||
| - name: KUBECONFIG | ||
| value: {{ include "headlamp.kubeconfigPath" . | quote }} | ||
| {{- end }} |
There was a problem hiding this comment.
This adds KUBECONFIG support via config.kubeconfigSecrets, but the chart doesn’t currently enforce the documented requirement that config.inCluster must be false. Consider adding a template validation (e.g., required/fail) when kubeconfigSecrets is non-empty and inCluster is true, so Helm install fails fast instead of silently ignoring the mounted kubeconfigs.
There was a problem hiding this comment.
I am still not sure whether the lack of the in-cluster kubeconfig mapping here was intentional. I also asked about this in the CNCF Slack. It would be nice if someone from the project could let me know what you all would like :)
| # Example: | ||
| # kubeconfigSecrets: | ||
| # - secretName: prod-cluster-kubeconfig | ||
| # key: config # optional, defaults to "config" | ||
| # - secretName: dev-cluster-kubeconfig | ||
| # key: kubeconfig | ||
| # - secretName: staging-cluster-kubeconfig |
There was a problem hiding this comment.
The commented example under config.kubeconfigSecrets is missing the surrounding config: key/indentation, which can be confusing when users copy/paste it into their values file. Please adjust the example to reflect the actual nesting under config:.
| # Example: | |
| # kubeconfigSecrets: | |
| # - secretName: prod-cluster-kubeconfig | |
| # key: config # optional, defaults to "config" | |
| # - secretName: dev-cluster-kubeconfig | |
| # key: kubeconfig | |
| # - secretName: staging-cluster-kubeconfig | |
| # Example (in values.yaml): | |
| # config: | |
| # kubeconfigSecrets: | |
| # - secretName: prod-cluster-kubeconfig | |
| # key: config # optional, defaults to "config" | |
| # - secretName: dev-cluster-kubeconfig | |
| # key: kubeconfig | |
| # - secretName: staging-cluster-kubeconfig |
There was a problem hiding this comment.
Adding config: would be more confusing than helpful, as it would add a duplicate line if someone were to just uncomment the code.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Corey McGalliard <59486473+cmcgalliard@users.noreply.github.com>
|
Tested this evening after adjusting for code review and it seems to work as expected. |
| - name: KUBECONFIG | ||
| value: {{ include "headlamp.kubeconfigPath" . | quote }} | ||
| {{- end }} | ||
| {{- if .Values.env }} | ||
| {{- toYaml .Values.env | nindent 12 }} |
There was a problem hiding this comment.
If users also set .Values.env with a KUBECONFIG entry, this template will emit duplicate env: entries with the same name (one from kubeconfigSecrets, one from toYaml .Values.env). That can lead to unpredictable results during apply/patch. Consider either (a) only setting KUBECONFIG when it’s not already present in .Values.env, or (b) explicitly failing with a clear message when both are configured.
There was a problem hiding this comment.
I think handling this edge case will make the helm difficult to read - most folks will not use both if they get this far, IMO.
Summary
This simplifies the process of configuring kubeconfigs in secrets for Headlamp. This functionality already exists; the Helm chart just doesn't make it easy to configure.
Changes
Steps to Test
Create 1+ secret containing a kubeconfig
Configure the kubeconfigSecrets
Screenshots (if applicable)
Notes for the Reviewer
I considered changing it so we could use the in-cluster kubeconfig, but I'm not sure whether you all have intentionally chosen this pattern.