Helm chart improvements#1802
Conversation
Signed-off-by: babykart <babykart@gmail.com>
Signed-off-by: babykart <babykart@gmail.com>
Signed-off-by: babykart <babykart@gmail.com>
Signed-off-by: babykart <babykart@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Garnet Helm chart to use Kubernetes-native ServiceAccount token automounting and to expose Service IP family configuration for cluster networking.
Changes:
- Replaces
serviceAccount.tokenwithserviceAccount.automount. - Removes the custom ServiceAccount token Secret template.
- Adds
service.ipFamiliesandservice.ipFamilyPolicyvalues and renders them on the main Service.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
charts/garnet/values.yaml |
Updates ServiceAccount values and adds Service IP family defaults. |
charts/garnet/templates/token.yaml |
Removes the generated ServiceAccount token Secret. |
charts/garnet/templates/serviceaccount.yaml |
Adds automountServiceAccountToken to created ServiceAccounts. |
charts/garnet/templates/service.yaml |
Adds IP family fields to the main Service spec. |
Comments suppressed due to low confidence (3)
charts/garnet/values.yaml:120
- The generated chart README is not updated for these new service values, so users will not see
service.ipFamiliesorservice.ipFamilyPolicyin the published values table unless the README is regenerated with helm-docs.
# -- Service port
port: 6379
# -- Service ipFamilies
ipFamilies:
- IPv4
charts/garnet/templates/token.yaml:1
- Deleting this template removes the
serviceAccount.tokenbehavior entirely.automountServiceAccountTokencontrols Pod token volume mounting but does not create the named ServiceAccount token Secret that existing users withserviceAccount.token: truemay consume, so this is a breaking chart API change unless a compatibility path or migration note is provided.
charts/garnet/templates/service.yaml:17 - These fields are emitted even when the corresponding values are empty or unset, which renders null values such as
ipFamilies:oripFamilyPolicy:rather than omitting the fields. Kubernetes expects valid values here, so users cannot fall back to cluster defaults without producing an invalid Service manifest.
ipFamilies:
{{- with .Values.service.ipFamilies }}
{{- toYaml . | nindent 4 }}
{{- end }}
ipFamilyPolicy: {{ .Values.service.ipFamilyPolicy }}
| automountServiceAccountToken: {{ .Values.serviceAccount.automount | default false }} | ||
| {{- end }} |
There was a problem hiding this comment.
This behavior is intentional and follows Helm’s standard object model for ServiceAccounts. The serviceAccount.* values are explicitly scoped to resources provisioned by the chart. When serviceAccount.create: false (default), the chart assumes an external SA is managed independently, so automount is correctly ignored. This decoupled pattern is a widely adopted Helm/Kubernetes best practice to prevent conflicts with existing infrastructure. As you noted, workload-level token control should indeed be applied at the Pod spec level or pre-configured on external SAs. The implementation fully aligns with established Helm chart conventions.
Signed-off-by: babykart <babykart@gmail.com>
|
Thanks @babykart, it seems there is still one comment on this PR to be responded to before we can go ahead. |
values.schema.jsonfile.