Add volumes and volumeMounts values to the Helm chart for file-based credentials - #62
Conversation
…sed credentials Follow-up to #60. The chart now exposes `app.volumes` and `app.volumeMounts`, so a user of the published chart can mount a credentials file for `adminSecretFileRef` through `values.yaml`. `app.imagePullSecrets` moves onto the same pattern and loses its `- {}` default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
stplasim
left a comment
There was a problem hiding this comment.
Well done! Just two little things. One is just a doc thing.
P.s. sorry for the bad formatting, I reviewed this on mobile
| private static final String ENV_VAR_KUBECONFIG = "KUBECONFIG"; | ||
|
|
||
| /// Must match `quarkus.kubernetes.name`. | ||
| private static final String CONTAINER_NAME = "postgresql-operator"; |
There was a problem hiding this comment.
Nit: This is the third place we have the container name, after kubernetes.yml and quarkus.kubernetes.name. The constructor already injects quarkus.helm.name, so could this take quarkus.kubernetes.name the same way? Then a rename fails on the actual problem (Dekorate adding a second container) rather than on getName().
There was a problem hiding this comment.
No you are right, I'm going to inject quarkus.kubernetes.name and use that whenever possible.
| secretProviderClass: db-credentials | ||
| volumeMounts: | ||
| - name: db-credentials | ||
| mountPath: /mnt/secrets |
There was a problem hiding this comment.
Maybe it is just me but both new sections stop at the mount, no?
But the point of the PR is making adminSecretFileRef usable from the chart. Could you end each example with the matching path?
adminSecretFileRef:
path: /mnt/secrets/db-credentials.json
With the CSI one especially it's not obvious that the filename comes from objectAlias. And a line saying the Secrets Store CSI driver has to be installed first would save someone a confused pod.
There was a problem hiding this comment.
Thanks for carefully reading this 🙏🏼
I reworked the whole file, as since PR #60 it was really confusing and duplicated some stuff from the #### Mount the credentials file section partly again in ### Examples.
I moved these sections now under ## Examples and cleaned it up a bit.
…ame` to ensure consistency and prevent drift
Follow-up to #60.
adminSecretFileRefneeds the credentials file inside the operator Pod, but the Helm chart exposed no way to mount one.A user of the published chart had to patch the Deployment after
helm install.The chart now exposes
app.volumesandapp.volumeMounts.Both take raw Kubernetes syntax, so any volume source works, a Secrets Store CSI volume included.
That last case is the one #60 was added for.
Changes
operator/src/main/kubernetes/kubernetes.yml: baselinevolumes: [~], plus a namedcontainer with
volumeMounts: [~].operator/src/main/helm/values.yaml(new):[]defaults forapp.volumes,app.volumeMountsandapp.imagePullSecrets.operator/src/main/resources/application.yml: two newquarkus.helm.valuesentries,three
values-schemaentries, and a simplerimage-pull-secretsexpression.operator/src/test/java/it/aboutbits/postgresql/helm/HelmTest.java: extended assertions,plus a new test that renders the chart with a real volume.
docs/cluster-connection.md: a Secret example and a CSI example..gitignore: ignore the.kube/folder that the fabric8 Kubernetes client writesinto the module directory when the tests run.
Why the
[~]placeholdersquarkus-helmcan only replace a value node that already exists.quarkus.helm.values.<x>.pathsuses YamlPath, which never creates a key.An empty list
[]inkubernetes.ymldoes not survive either, because the fabric8 model marksPodSpec.volumes,PodSpec.imagePullSecretsandContainer.volumeMountswith@JsonInclude(NON_EMPTY).A list with one
nullelement does survive.operator/src/main/helm/values.yamlthen replaces the resulting- {}default with a real empty list.Upstream request: quarkiverse/quarkus-helm#453
Alternatives considered
The Debezium operator solves the same problem for
imagePullSecretsin debezium/debezium-operator#219.Two of its three tricks are the same as the ones here:
a
src/main/helm/values.yamlfor the[]default, and the description invalues-schema.properties.So those two choices are established practice, not invention.
The difference is how the missing key gets created. Debezium anchors a
quarkus.helm.expressionsentry on the existingserviceAccountNamescalar and appends a whole multi-line YAML block to it:We evaluated that and did not take it, for four reasons:
debezium-operatoris the service account name.Rename the service account and the rendered template silently loses it.
imagePullSecretsto an unrelated field.A reader of
application.ymlcannot tell whyserviceAccountNamecarries a pull-secret block.System.lineSeparator(), inapplyKnownPatterns, so the generated template depends on the build platform.volumeMountswould make all four worse, because that field sits inside a container list entry.It would need a second anchor on another container field, and it would hardcode that field's value as well.
The baseline
kubernetes.ymlkeeps each field at its own path, so the expression stays a one-liner and the indentation lives innindent.The other routes we tried
value-as-list: []orvalue: "[]"on the value entry.This would give the
[]default with no baseline file at all.It cannot work.
ValueReferenceConfig.valueAsList()is anOptional<List<String>>, and an empty list resolves toOptional.empty(), sotoValuefalls through.A string
"[]"reachesvalues.yamlas a quoted string and types the schema asstring.quarkus.kubernetes.secret-volumes.<n>.secret-namewith
optional=trueplusquarkus.kubernetes.mounts.<n>.path, orempty-dir-volumes.This does create the nodes. It also ships a concrete dummy volume in the default
values.yamland mounts it at runtime, so every user who does not need a volume still gets one.quarkus.helm.add-if-statement.It wraps a whole resource in a condition. It cannot add a field.
src/main/helm/templates/deployment.yaml.quarkus-helm keeps only the
{{- define }}blocks of a user template and prepends them.It does not replace or merge the generated Deployment, so the file adds nothing. Verified by build.
AdditionalHelmTemplateBuildItemwithReplacedResource("Deployment", ...).Possible since quarkus-helm 1.4.1.
It would mean hand-maintaining the entire Deployment template, and
quarkus.helm.values.<x>.pathsdo not apply to a replaced resource.Rejected as far more work than the placeholder.
Checked whether a newer version removes the need: it does not.
The relevant code is unchanged in quarkus-helm 1.4.1, 1.4.2 and 1.4.3, and in Quarkus kubernetes 3.36 through 3.39.2.
Upstream PRs #508 and #512 only act on build items that a Quarkus extension deployment module produces, so no
quarkus.helm.*property reaches them.Behaviour change for chart users
app.imagePullSecretsmoves onto the same pattern.Its default changes from
- {}to[], and the{{- if eq ... "- {}" }}guard is gone.helm templatenow rendersimagePullSecrets: []instead ofimagePullSecrets: null.Both are valid, and anyone who already sets the value is unaffected.
The old
- {}default was a trap: a user who copied it and appended a secret name shipped an invalid entry.Known trade-off
The generated chart README shows a blank Description for the three list values.
The descriptions live in
values.schema.jsoninstead.Cause: a key supplied through
src/main/helm/values.yamloverwrites theConfigReferencethat carries the description,and quarkus-helm merges the generated one with
putIfAbsent.An upstream fix is planned.
Four other rows in that table (
app.replicas,app.imagePullPolicyand the twoapp.resources.*values) are already blank for an unrelated reason.Three ways to get the descriptions back were weighed:
with
quarkus.helm.create-readme-file: falseand aHelmTestloop that asserts everyapp.*key appears in it.Rejected for now, because it moves 44 generated rows into a file that has to be maintained by hand.
[{}]defaults and the"- {}"guard expression.Rejected, because the
- {}default is a trap for anyone who copies it out ofhelm show values.Rejected, because
quarkus.helm.create-tar-file: truehas already packed the.tgzthat the release workflow uploads, so the task would have to repack it.The upstream fix is the chosen route, because it removes the cause instead of the symptom.