feat(dgraph): support existingSecret for ACL, encryption, and backup admin credentials

[mlwelles]

Add existingSecret for the ACL HMAC, encryption key, and backup-admin password, so key material never renders through Helm values or release state. Default-off.

Stacked on fix-statefulset-latent-bugs; will retarget to main once that merges.

Part of splitting #140 into per-area PRs. The merge of all split PRs reproduces #140's tree byte-for-byte. #140 is being closed as superseded.

[matthewmcneely]

secretFile and keyFile are dead — grep -rn 'secretFile\|keyFile' charts/dgraph/templates/ returns nothing. Same class as #141, in the same series.

The bigger problem is that the prose here and at :398 (and :412/:416 for keyFile) describes a mechanism the chart doesn't implement:

Filename (and Secret key) of the HMAC secret mounted at /dgraph/acl/<secretFile>.
The Secret must contain the secretFile key.

Neither volume has an items: selector, so the whole Secret is projected and the on-disk filenames are the Secret's own key names — the acl.file map key on the chart path, whatever the operator's Secret carries on the existingSecret path. Nothing consults secretFile in either case.

That matters operationally rather than cosmetically: the chart never emits --acl_secret_file, so the operator hand-writes /dgraph/acl/<filename> into extraFlags or configFile. values.yaml naming the wrong filename gets them a wrong flag and an alpha that can't find its key.

Two clean ways out. Wire the keys — add items: [{key: <secretFile>, path: <secretFile>}] inside the existingSecret branch only, which makes the path deterministic, makes both values live, narrows the projection, and turns a wrong key name into a loud kubelet mount failure. Or delete both keys and reword the comments to say the whole Secret mounts at /dgraph/acl and /dgraph/enc, and the operator's key names become the filenames.

Whichever you pick, don't apply items: unconditionally. acl.file is a documented free-form filename-to-data map, so someone with file: {my_custom_hmac: ...} would get a volume demanding hmac_secret_file from a Secret that lacks it, and the pod would never start.

Worth noting one side effect of the bare mount on the existingSecret path specifically: every key of an externally managed Secret lands as a file in the alpha container, and ExternalSecrets/Terraform Secrets routinely carry several. That surface is new — before this PR the volume could only reference a chart-owned Secret whose contents the user authored key by key.

[matthewmcneely]

Correction on this one — I reviewed #149 standalone and got the stack wrong. #150, stacked directly on this branch, consumes both keys:

--acl "secret-file=/dgraph/acl/{{ .Values.alpha.acl.secretFile }};"
--encryption "key-file=/dgraph/enc/{{ .Values.alpha.encryption.keyFile }};"

So they are introduced here and used one PR up, which is a normal split rather than the #141 dead-key pattern I compared it to. Disregard the "delete them and reword" option, and the prose at :393/:398 becomes an accurate statement of a real requirement once #150 lands, not a description of something unimplemented.

Two things from that comment survive, and one of them gets sharper.

The items: suggestion is worth more now, not less. #150 hardcodes the path to /dgraph/acl/<secretFile>, so the Secret has to carry a key of exactly that name. Nothing enforces that today, and without items: a mismatch surfaces as a runtime failure rather than a no-op.

The warning about not applying items: unconditionally stops being hypothetical and becomes a live breakage in #150. acl.file is a free-form filename-to-data map, so anyone with file: {my_custom_hmac: ...} who leaves secretFile at its default gets a flag pointing at /dgraph/acl/hmac_secret_file, which does not exist. Whatever guard you add wants to cover that case. I am still pinning down exactly what alpha does at that point — crash or silently start without ACL — and will follow up on #150 with the answer, since it decides how urgent this is.

The note about every key of an externally managed Secret landing as a file inside the alpha container is unaffected.

[matthewmcneely]

You updated this message to name backups.admin.existingSecret, but the README still teaches the old rule in three places, so the chart now errors with a key its own docs never mention:

• README:251 — "Login user password for backups (required if ACL enabled)"
• README:614 — "backups.admin.password (required) - the corresponding password for that user will need to be specified."
• README:45 — the v25 breaking-change note, "backups.admin.password must be explicitly set"

An operator using existingSecret who follows :251 or :614 also sets backups.admin.password. The chart then ignores it while it sits in the Helm release values — exactly the exposure this feature exists to prevent.

Separately, none of the new keys are documented at all: grep -c existingSecret charts/dgraph/README.md is 0, likewise passwordSecretKey. The values table has a row for every sibling (alpha.acl.file, alpha.encryption.file, backups.admin.password), so four live keys are undiscoverable. #147 in this series added its README rows, so the convention is current.

Ask: four table rows, an "unless backups.admin.existingSecret is set" caveat on :251 and :614, and a clause on :45. Hold off on rows for secretFile/keyFile until the values.yaml thread is resolved. An example under example_values/ alongside the existing alpha-acl-secrets.yaml would fit the repo's pattern nicely.

[matthewmcneely]

Partial correction: #150 adds four of the rows I said were missing — alpha.acl.secretFile, alpha.acl.existingSecret, alpha.encryption.keyFile, alpha.encryption.existingSecret.

Still missing once both land: backups.admin.existingSecret and backups.admin.passwordSecretKey.

The sharper half of the comment stands untouched. #150's README changes are confined to a new paragraph after :44 and the values-table rows around :185, so :251, :614, and :45 all still teach the old "password required" rule that existingSecret invalidates.

[matthewmcneely]

The parenthetical is false — passwordSecretKey does nothing on the chart's own Secret. It's read at exactly two places, cronjob-full.yaml:135 and cronjob-inc.yaml:135, both inside {{- if and .Values.alpha.acl.enabled .Values.backups.admin.existingSecret }}. The chart-minted path hardcodes the literal key in secrets.yaml:20:

$ helm template t charts/dgraph --set alpha.acl.enabled=true --set backups.full.enabled=true \
    --set backups.admin.password=s3cret --set backups.admin.passwordSecretKey=my_custom_key \
    -s templates/backups/secrets.yaml | tail -2
data:
  backup_admin_password: "czNjcmV0"        # my_custom_key appears nowhere in the full render

Suggested wording: "Key within existingSecret that holds the backup admin password. Ignored unless existingSecret is set; the chart's own backups Secret always uses the key backup_admin_password."

Don't fix it the other way by making secrets.yaml honor the key — cronjob-{full,inc}.yaml:76 hardcode cat /backup_secrets/backup_admin_password and the non-projected volume has no items: remap, so renaming would break the backup job.

[matthewmcneely]

No required or default here, so an explicitly empty value renders a null key that helm lint happily accepts and the API server rejects:

$ helm template t charts/dgraph --set alpha.acl.enabled=true --set backups.full.enabled=true \
    --set backups.admin.existingSecret=my-admin --set backups.admin.passwordSecretKey= \
    -s templates/backups/cronjob-full.yaml
                  items:
                  - key:                       # <- bare, parses to null
                    path: backup_admin_password

items[0].key: Required value at apply time, on both CronJobs.

Scoping this honestly: the key ships a working default, so it takes a deliberate blank rather than an omission — realistically a GitOps values pipeline emitting "" for an unset variable. And it fails closed and loud. What makes it worth the one-liner is internal consistency: this same PR wraps backups.admin.password in required and uses | default for both other new knobs.

{{ .Values.backups.admin.passwordSecretKey | default "backup_admin_password" }} in both templates, which also matches the hardcoded path: on the next line.

[matthewmcneely]

These are literal YAML comments rather than {{/* ... */}}, so they render into the manifest for every ACL or encryption user — including everyone who never touches existingSecret:

$ diff <(helm template t base --set alpha.acl.enabled=true) <(helm template t pr149 --set alpha.acl.enabled=true)
264a265,267
>           # existingSecret lets a pre-created Secret (e.g. one managed by Terraform or an
>           # external secrets operator) supply the HMAC key, so it never has to be rendered
>           # through Helm values; otherwise mount the Secret the chart renders from `acl.file`.

The parsed objects are identical, so no pod-template change and no rolling restart. The cost is manifest-text churn in helm get manifest, rendered-manifest GitOps repos, and helm-diff/Helmfile — this repo ships a helmfiles/ tree — plus permanently shipping "e.g. one managed by Terraform or an external secrets operator" into every user's cluster.

Same thing I raised on #145. Worth converting all ten lines while you're here: lines 284-287 on the base branch have the identical problem a few lines up, and a six-line patch would leave that one sitting there.