From d313e99d539cf67d45f2053dd8fd4b4c83b2eb38 Mon Sep 17 00:00:00 2001 From: tada5hi Date: Mon, 24 Aug 2026 07:34:08 +0200 Subject: [PATCH 1/6] fix(authup): give the migration hook only what a hook can see, and scope useHelmHooks to ArgoCD Helm applies a pre-upgrade hook BEFORE the release manifest, so the migration Job can only reference objects that already exist from the PREVIOUS release. It referenced several that do not. Dropped from the Job (via a `hook` flag on the shared helpers, so Deployment and Job stay one source per architecture rule 10): - the provisioning volume. `migration run` never scans /provisioning: ProvisionerModule is registered by createApplication(), which only the start command calls. The upgrade that first set server.provisioning.files left the hook pod in ContainerCreating on "configmap not found" until it timed out. - REDIS and SMTP. The migration builds config + logger + database only, no cache and no mail module, and both Secrets are ordinary release resources. - CLIENT_SYSTEM_SECRET. Its key inside the chart-managed auth Secret is conditional, so flipping auth.systemClientEnabled on broke the hook the same way. Kept, deliberately: - the writable directory. Under the image's NODE_ENV=production the logger opens /http.log and /error.log before the first query, and an uncreatable path is a hard ENOENT, not a degradation. - the config file. `migration run` loads authup.server.core.conf unconditionally, and the db keys only the file can carry (ssl, socketPath, replication, extensions, poolSize) decide how it connects and what it creates. Dropping the mount would have migrated over a plaintext connection with no error. The Job now reads it from a hook-scoped COPY at hook-weight -5, which also fixes the quieter half: the release ConfigMap holds the PREVIOUS release's content when the hook runs. Same reasoning that already makes the Job inline configEnv. - SECRETS_ENCRYPTION_KEY, whose key is conditional too. Rule 6's fail-closed posture outranks the one-off break: a migration that ever touches a wrapped column must not run without the key. A write-once KEK gets its own upgrade. The helpers now take (dict "context" $ "hook" bool) and `required` the context. That guard is load-bearing: helm renders with missingkey=zero, so a call site passing a bare `.` would have read every guard as false and emitted writable + tmp only, silently dropping the config file again. Separately, useHelmHooks=false was documented "set false for ArgoCD / Flux". Flux's helm-controller runs a real helm upgrade and honours Helm hooks, so turning them off there applies the Job as an ordinary resource, and Job.spec.template is immutable: the next upgrade that changes the pod template fails to patch it. The flag is now scoped to ArgoCD in values.yaml, NOTES warns when it is set, and architecture rule 19 records why a content-hashed Job name is not the answer (helm orders a plain Job after the Deployment and does not wait for it, which is the ordering the Job exists to provide). Closes #17 Closes #18 --- .agents/architecture.md | 44 +++++- .agents/references/authup.md | 18 +++ .agents/testing.md | 29 ++++ DESIGN.md | 10 +- charts/authup/Chart.yaml | 6 + charts/authup/README.md | 2 +- charts/authup/ci/valkey-values.yaml | 7 + charts/authup/templates/NOTES.txt | 11 ++ charts/authup/templates/_server-env.tpl | 125 ++++++++++++++---- .../server/configmap-configuration.yaml | 2 +- .../configmap-migration-configuration.yaml | 39 ++++++ .../authup/templates/server/deployment.yaml | 6 +- .../templates/server/migration-job.yaml | 6 +- charts/authup/values.schema.json | 2 +- charts/authup/values.yaml | 5 +- 15 files changed, 269 insertions(+), 43 deletions(-) create mode 100644 charts/authup/templates/server/configmap-migration-configuration.yaml diff --git a/.agents/architecture.md b/.agents/architecture.md index 44f11e2..10675d0 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -41,14 +41,37 @@ editing templates or values. 9. **Component fullnames truncate the base BEFORE suffixing** (`trunc 52` then `-server` / `-ui` / engine suffix), so long release names cannot collapse every resource onto one identical name. -10. **The migration Job shares the deployment's env by construction.** - `authup.server.configEnv` (map) and `authup.server.secretEnv` (list) are - the single sources consumed by both `server/deployment.yaml` and +10. **The migration Job shares the deployment's env by construction, minus + what a hook cannot see.** `authup.server.configEnv` (map), + `authup.server.secretEnv` (list) and the two volume helpers are the single + sources consumed by both `server/deployment.yaml` and `server/migration-job.yaml`; the Job INLINES the config map (a pre-upgrade hook would otherwise run against the previous release's ConfigMap). The Job is pre-upgrade ONLY (never pre-install: hooks run before backing services exist; authup migrates at boot on fresh installs). With - `useHelmHooks=false` it renders ArgoCD `PreSync` hook annotations instead. + `useHelmHooks=false` it renders ArgoCD `PreSync` hook annotations instead, + which is an ArgoCD-only mode: see rule 19. + + Helm applies a pre-upgrade hook BEFORE the release manifest, so everything + the Job references must already exist from the PREVIOUS release. The three + helpers take a `hook` flag that drops what `migration run` does not read: + `REDIS`, `SMTP` (their Secrets are release resources, and the migration + builds no cache or mail module) and the provisioning mount (`ProvisionerModule` + is registered by the start command only). What stays, stays for a reason: + the writable directory, because under the image's `NODE_ENV=production` the + logger opens `/http.log` before the first query and an uncreatable + path is a hard ENOENT; and the config file, because `migration run` loads + `authup.server.core.conf` unconditionally and its file-only db keys (`ssl`, + `socketPath`, `replication`, `extensions`) decide how the migration connects. + The Job reads that file from a hook-scoped COPY + (`server/configmap-migration-configuration.yaml`, weight -5) for the same + reason it inlines the env: the release ConfigMap is either absent or one + release stale when the hook runs. `CLIENT_SYSTEM_SECRET` goes the same way: + its key inside the auth Secret is conditional. `SECRETS_ENCRYPTION_KEY` + deliberately does NOT, even though its key is conditional too and the + migration does not read it today: rule 6's fail-closed posture outranks the + one-off break, so a write-once KEK gets its own upgrade. `DB_PASSWORD` is the + other residual, and only when an upgrade also switches database engine. 11. **Checksum annotations roll pods on config or secret changes.** The server deployment checksums the env map plus every chart-managed secret it consumes (auth, external-db, redis, smtp, provisioning, configuration), @@ -114,6 +137,19 @@ editing templates or values. `validations.yaml` fails that combination; `route.matches` / `route.filters` are the raw passthroughs that express it (authup always serves at `/`, so the prefix must be matched AND rewritten away). +19. **`useHelmHooks=false` is an ArgoCD-only mode.** ArgoCD renders with + `helm template` and never executes Helm hooks, so it needs its own + `argocd.argoproj.io/hook` annotations. Flux is the opposite: helm-controller + runs a real `helm upgrade` and honours Helm hooks natively. Turning them off + there applies the migration Job as an ordinary release resource, and + `Job.spec.template` is immutable, so the next upgrade that touches the pod + template (image tag, `appVersion` label, a new env) fails to patch it. A + content-hashed Job name would make that apply-able but not correct: helm + orders a plain Job AFTER the Deployment and does not wait for it, which is + the ordering the Job exists to provide. So the value stays doc-scoped to + ArgoCD and NOTES warns when it is set. ArgoCD also maps Helm hooks onto its + own sync phases, so `true` works there as well; the flag only chooses which + annotation family drives the Job. ## Values conventions diff --git a/.agents/references/authup.md b/.agents/references/authup.md index 59b5118..f8ec748 100644 --- a/.agents/references/authup.md +++ b/.agents/references/authup.md @@ -65,6 +65,24 @@ unsupported per `.agents/architecture.md` in the monorepo), - server-core auto-runs migrations + provisioning at boot (`app/modules/database/module.ts`; no off-switch) -> generous startupProbe; optional pre-upgrade migration Job for multi-replica DDL serialization. +- `migration run` (`cli/commands/migration.ts`, `defineCLIMigrationCommand`) + builds only three modules: config, logger, database. No http, cache, mail, + identity or provisioning module. It therefore ignores `REDIS` / `SMTP`, and + never scans `/provisioning` (`ProvisionerModule` is registered by + `createApplication()`, i.e. the `start` command only) -> the chart drops all + three from the migration Job. +- `migration run` DOES read the config file, unconditionally: `createCLIConfigModule` + passes `fs: {}` (truthy) into `readConfig`, so `readConfigRawFromFS` runs + (`config/read/fs.ts`). Env wins per key, but the db keys typeorm-extension's + env reader does not name survive: `ssl`, `socketPath`, `replication`, + `poolSize`, `charset`, `extensions` (postgres `CREATE EXTENSION` during + `initialize()`), `entities`, `subscribers`. So the config file decides how the + migration connects and what it creates -> the chart MUST mount it on the Job. +- Under `NODE_ENV=production` (baked into the image) `migration run` needs the + writable directory before it touches the database: the logger adds winston + File transports for `/http.log` and `/error.log`, and the + transport does `mkdirSync` + open eagerly. An unwritable path is a hard ENOENT + failure of the command, not a degradation -> the Job keeps the writable mount. - Replicas > 1 without redis: per-process MemoryCache breaks auth codes, revocations, MFA challenges (`app/modules/cache/module.ts`) -> hard validation in the chart. diff --git a/.agents/testing.md b/.agents/testing.md index 8ffaf33..6eb44f9 100644 --- a/.agents/testing.md +++ b/.agents/testing.md @@ -83,6 +83,35 @@ flag, so all six read sites convert together: leave one raw and an umbrella-driv route renders unguarded. `ci/default-values.yaml` carries the false direction as the in-repo regression guard. +The pre-upgrade migration Job must stay narrower than the Deployment. Helm +applies a hook before the release manifest, so anything the Job references has +to exist from the previous release: + +```bash +helm template t charts/authup --set server.migration.enabled=true \ + --set valkey.enabled=true --set smtp.connectionString=smtp://u:p@mail:25 \ + --set auth.systemClientEnabled=true \ + --set server.provisioning.enabled=true --set 'server.provisioning.files.realms\.json=[]' \ + --set 'server.configuration=db: {ssl: true}' \ + -s templates/server/migration-job.yaml +``` + +The Job must show env `DB_PASSWORD` / `USER_ADMIN_PASSWORD` but NO `REDIS`, no +`SMTP` and no `CLIENT_SYSTEM_SECRET`; volumes `writable` / `tmp` / +`configuration` but NO `provisioning`; and the configuration volume must name +`-server-migration-configuration` +(the hook-scoped copy at weight -5), never `-server-configuration`. The +server Deployment in the same render must still carry all of them. Dropping the +config file from the Job is NOT a valid simplification: `migration run` reads it +and its file-only db keys (`ssl`, `socketPath`, `extensions`) govern the +connection, so a missing mount migrates over a plaintext connection instead of +failing. + +`useHelmHooks=false` must print the Flux/plain-helm warning in NOTES.txt, and +must not print it with hooks on. NOTES is not reachable through `helm template`, +so wrap it: copy the chart, append `templates/NOTES.txt` into a ConfigMap +template, render that. + Umbrella use is part of the contract: `global` must stay open. Render a throwaway parent chart with authup in `charts/` and an unrelated global (`global.myOrgKey`) whenever the schema generation changes; `ci/default-values.yaml` carries a stray diff --git a/DESIGN.md b/DESIGN.md index 0632940..378862f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -321,7 +321,15 @@ first-class templating, in v1. Job serializes DDL before new pods roll — recommended (and referenced by the replicas>1 validation) for multi-replica deployments, since MySQL DDL is non-transactional and concurrent boot migrations can race. -- `useHelmHooks: false` support (ArgoCD/Flux users get a plain Job). +- `useHelmHooks: false` support: ArgoCD only. ArgoCD renders with + `helm template` and never runs Helm hooks, so it gets `argocd.argoproj.io` + annotations instead. Flux runs a real `helm upgrade` and honours Helm hooks, + so a plain Job there hits the immutable `spec.template` on the next upgrade. +- The hook Job sees only the PREVIOUS release's ConfigMaps and Secrets, so it + carries a narrowed env/mount set (no REDIS, SMTP, CLIENT_SYSTEM_SECRET or + provisioning mount) plus a + hook-scoped copy of `authup.server.core.conf`, which `migration run` does + read. - Value reshuffles get authentik-style tripwires: a `deprecations.yaml` template fails loudly naming the moved key. BREAKING.md tracks migrations; chart versioning is independent SemVer (0.major.minor pre-1.0), `appVersion` tracks diff --git a/charts/authup/Chart.yaml b/charts/authup/Chart.yaml index f37db84..ea54876 100644 --- a/charts/authup/Chart.yaml +++ b/charts/authup/Chart.yaml @@ -37,3 +37,9 @@ annotations: description: Track authup v1.0.0-beta.63, whose image writes to /var/lib/authup - kind: added description: server.route.enabled / adminConsole.route.enabled accept a tpl-rendered string, so an umbrella chart can drive them from a global + - kind: fixed + description: The pre-upgrade migration Job no longer references release resources that do not exist when the hook runs; it drops the provisioning mount plus REDIS, SMTP and CLIENT_SYSTEM_SECRET, none of which migration run reads + - kind: fixed + description: The migration Job reads authup.server.core.conf from a hook-scoped copy, so it no longer runs against a missing or one-release-stale config file + - kind: changed + description: useHelmHooks=false is documented as ArgoCD-only; under Flux or plain helm it makes the Job a plain resource whose immutable pod template fails the next upgrade diff --git a/charts/authup/README.md b/charts/authup/README.md index d5e6879..1319eb6 100644 --- a/charts/authup/README.md +++ b/charts/authup/README.md @@ -484,7 +484,7 @@ Kubernetes: `>=1.25.0-0` | smtp.connectionString | string | `""` | SMTP connection string (smtp(s)://user:pass@host:port); stored in a chart-managed secret | | smtp.existingSecret | string | `""` | Existing secret holding the SMTP connection string (tpl-rendered) | | smtp.existingSecretKey | string | `"smtp-connection-string"` | Key inside smtp.existingSecret holding the connection string | -| useHelmHooks | bool | `true` | Render Job hook annotations (set false for ArgoCD / Flux) | +| useHelmHooks | bool | `true` | Render Helm hook annotations on the migration Job. Set false only for ArgoCD, which reads its own PreSync annotations instead (it also understands Helm hooks, so true works there too). Flux and plain helm need true: a plain Job's pod template is immutable, so the next upgrade cannot patch it. | | valkey.affinity | object | `{}` | Valkey affinity | | valkey.auth.password | string | `""` | Valkey password ("" = generate once, keep across upgrades) | | valkey.containerSecurityContext | object | `{"allowPrivilegeEscalation":false,"capabilities":{"drop":["ALL"]},"enabled":true,"runAsGroup":999,"runAsNonRoot":true,"runAsUser":999,"seccompProfile":{"type":"RuntimeDefault"}}` | Valkey container security context | diff --git a/charts/authup/ci/valkey-values.yaml b/charts/authup/ci/valkey-values.yaml index ccfa0a0..45bb0d5 100644 --- a/charts/authup/ci/valkey-values.yaml +++ b/charts/authup/ci/valkey-values.yaml @@ -8,6 +8,13 @@ server: replicaCount: 2 migration: enabled: true + # The only ci scenario setting server.configuration, so the release ConfigMap + # and the Deployment's mount get install coverage and the hook-scoped copy gets + # render coverage (ct never runs an upgrade, so no pre-upgrade hook is ever + # created on kind). Deliberately a no-op option: a db block here would fight + # the connection this scenario installs. + configuration: | + logger: true resources: requests: cpu: 50m diff --git a/charts/authup/templates/NOTES.txt b/charts/authup/templates/NOTES.txt index 1ee7dd2..a3e8c48 100644 --- a/charts/authup/templates/NOTES.txt +++ b/charts/authup/templates/NOTES.txt @@ -84,6 +84,17 @@ schema migrations are serialized in a pre-upgrade Job instead of racing at pod boot. {{- end }} +{{- if and .Values.server.migration.enabled (not .Values.useHelmHooks) }} + +WARNING: useHelmHooks=false renders the migration Job as a plain resource +carrying ArgoCD PreSync annotations, which only ArgoCD reads. Under Flux or +plain helm the Job is applied like any other resource, and a Job pod template +is immutable, so the next upgrade that changes it (new image tag, new env) +fails with "spec.template: field is immutable". Use useHelmHooks=true there. +ArgoCD maps Helm hook annotations onto its own sync phases, so true works for +ArgoCD too; false only changes which annotation family drives the Job. +{{- end }} + {{- if .Values.server.provisioning.enabled }} NOTE: "admin-console" and "account-console" are reserved client names: authup diff --git a/charts/authup/templates/_server-env.tpl b/charts/authup/templates/_server-env.tpl index 615628f..2cfa2f8 100644 --- a/charts/authup/templates/_server-env.tpl +++ b/charts/authup/templates/_server-env.tpl @@ -55,47 +55,67 @@ WRITABLE_DIRECTORY_PATH: "/var/lib/authup" {{/* Secret-backed server-core env entries (valueFrom.secretKeyRef list). Shared by the Deployment and the migration Job. +Usage: {{ include "authup.server.secretEnv" (dict "context" $ "hook" true) }} + +"hook" marks the pre-upgrade migration Job and drops REDIS and SMTP. Not +tidiness: both Secrets are ordinary release resources, and helm applies a +pre-upgrade hook BEFORE the release manifest, so the upgrade that first enables +valkey or SMTP would schedule a hook pod whose secretKeyRef target does not +exist yet (CreateContainerConfigError until the hook times out). `migration run` +builds config + logger + database only, no cache and no mail module, so neither +value is read there. CLIENT_SYSTEM_SECRET goes for the same reason: its key +inside the chart-managed auth Secret is conditional, so flipping +auth.systemClientEnabled on breaks the hook the same way. What stays: DB_PASSWORD +(the migration cannot run without it), USER_ADMIN_PASSWORD (unread, but its key +is unconditional, so it costs nothing) and the KEK (see its comment below). */}} {{- define "authup.server.secretEnv" -}} +{{- $ctx := required "authup.server.secretEnv: call it as (dict \"context\" $ \"hook\" bool)" .context -}} - name: DB_PASSWORD valueFrom: secretKeyRef: - name: {{ include "authup.database.secretName" . }} - key: {{ include "authup.database.passwordKey" . }} -{{- if include "authup.redis.enabled" . }} + name: {{ include "authup.database.secretName" $ctx }} + key: {{ include "authup.database.passwordKey" $ctx }} +{{- if and (not .hook) (include "authup.redis.enabled" $ctx) }} - name: REDIS valueFrom: secretKeyRef: - name: {{ include "authup.redis.secretName" . }} - key: {{ include "authup.redis.secretKey" . }} + name: {{ include "authup.redis.secretName" $ctx }} + key: {{ include "authup.redis.secretKey" $ctx }} {{- end }} -{{- if include "authup.smtp.enabled" . }} +{{- if and (not .hook) (include "authup.smtp.enabled" $ctx) }} - name: SMTP valueFrom: secretKeyRef: - name: {{ include "authup.smtp.secretName" . }} - key: {{ include "authup.smtp.secretKey" . }} + name: {{ include "authup.smtp.secretName" $ctx }} + key: {{ include "authup.smtp.secretKey" $ctx }} {{- end }} - name: USER_ADMIN_PASSWORD valueFrom: secretKeyRef: - name: {{ include "authup.auth.secretName" . }} - key: {{ .Values.auth.secretKeys.adminPasswordKey }} -{{- if .Values.auth.systemClientEnabled }} + name: {{ include "authup.auth.secretName" $ctx }} + key: {{ $ctx.Values.auth.secretKeys.adminPasswordKey }} +{{- if and (not .hook) $ctx.Values.auth.systemClientEnabled }} - name: CLIENT_SYSTEM_SECRET valueFrom: secretKeyRef: - name: {{ include "authup.auth.secretName" . }} - key: {{ .Values.auth.secretKeys.systemClientSecretKey }} + name: {{ include "authup.auth.secretName" $ctx }} + key: {{ $ctx.Values.auth.secretKeys.systemClientSecretKey }} {{- end }} -{{- if include "authup.auth.hasSecretsEncryptionKey" . }} +{{- if include "authup.auth.hasSecretsEncryptionKey" $ctx }} {{- /* Never optional: a silently missing KEK would boot authup into - plaintext-at-rest and defer unrecoverable decrypt failures. */}} + plaintext-at-rest and defer unrecoverable decrypt failures. Kept on the + hook for the same reason, even though its key is conditional and + `migration run` does not read it today: a migration that ever touches a + wrapped column must fail closed, not run without the key. The cost is + that enabling auth.secretsEncryptionKey and server.migration.enabled in + ONE upgrade schedules a hook pod referencing a key the release has not + written yet. Enable a write-once KEK on its own upgrade. */}} - name: SECRETS_ENCRYPTION_KEY valueFrom: secretKeyRef: - name: {{ include "authup.auth.secretName" . }} - key: {{ .Values.auth.secretKeys.secretsEncryptionKeyKey }} + name: {{ include "authup.auth.secretName" $ctx }} + key: {{ $ctx.Values.auth.secretKeys.secretsEncryptionKeyKey }} {{- end }} - name: npm_config_cache value: /tmp/.npm-cache @@ -104,18 +124,41 @@ Shared by the Deployment and the migration Job. {{/* Shared volumes / volumeMounts for the server container (writable dir, tmp, provisioning files, config file). +Usage: {{ include "authup.server.volumeMounts" (dict "context" $ "hook" true) }} +The `required` on .context is load-bearing: helm renders with missingkey=zero, so +a call site that passed a bare `.` would leave every guard below reading false and +emit writable+tmp only, silently dropping the config file. Failing the render is +the chart's posture everywhere else. + +"hook" marks the pre-upgrade migration Job. It drops the provisioning mount, +whose ConfigMap/Secret is an ordinary release resource that helm applies AFTER +the hook: the upgrade that first sets server.provisioning.files would leave the +hook pod in ContainerCreating on a "configmap not found" until it times out, and +`migration run` never reads those files anyway (ProvisionerModule is registered +by the start command only). The writable directory stays for BOTH: under the +image's NODE_ENV=production the logger opens /http.log and +/error.log before the migration touches the database, and an +uncreatable path is a hard ENOENT failure. + +The config file stays for both as well, and mounting it is not optional: +`migration run` loads authup.server.core.conf unconditionally, and the db keys +that only the file can carry (ssl, socketPath, replication, extensions, poolSize) +decide how the migration connects and what it creates. Dropping it would silently +migrate over a plaintext connection. The Job reads it from a hook-scoped copy +instead: see authup.server.configurationConfigMapName. */}} {{- define "authup.server.volumeMounts" -}} +{{- $ctx := required "authup.server.volumeMounts: call it as (dict \"context\" $ \"hook\" bool)" .context -}} - name: writable mountPath: /var/lib/authup - name: tmp mountPath: /tmp -{{- if and .Values.server.provisioning.enabled (or .Values.server.provisioning.files .Values.server.provisioning.existingConfigMap .Values.server.provisioning.existingSecret) }} +{{- if and (not .hook) $ctx.Values.server.provisioning.enabled (or $ctx.Values.server.provisioning.files $ctx.Values.server.provisioning.existingConfigMap $ctx.Values.server.provisioning.existingSecret) }} - name: provisioning mountPath: /var/lib/authup/provisioning readOnly: true {{- end }} -{{- if or .Values.server.configuration .Values.server.existingConfigmap }} +{{- if or $ctx.Values.server.configuration $ctx.Values.server.existingConfigmap }} - name: configuration mountPath: /usr/src/app/authup.server.core.conf subPath: authup.server.core.conf @@ -124,24 +167,25 @@ provisioning files, config file). {{- end -}} {{- define "authup.server.volumes" -}} +{{- $ctx := required "authup.server.volumes: call it as (dict \"context\" $ \"hook\" bool)" .context -}} - name: writable emptyDir: {} - name: tmp emptyDir: {} -{{- if and .Values.server.provisioning.enabled (or .Values.server.provisioning.files .Values.server.provisioning.existingConfigMap .Values.server.provisioning.existingSecret) }} +{{- if and (not .hook) $ctx.Values.server.provisioning.enabled (or $ctx.Values.server.provisioning.files $ctx.Values.server.provisioning.existingConfigMap $ctx.Values.server.provisioning.existingSecret) }} - name: provisioning - {{- if .Values.server.provisioning.existingSecret }} + {{- if $ctx.Values.server.provisioning.existingSecret }} secret: - secretName: {{ include "authup.tplvalues.render" (dict "value" .Values.server.provisioning.existingSecret "context" $) }} + secretName: {{ include "authup.tplvalues.render" (dict "value" $ctx.Values.server.provisioning.existingSecret "context" $ctx) }} {{- else }} configMap: - name: {{ include "authup.server.provisioningConfigMapName" . }} + name: {{ include "authup.server.provisioningConfigMapName" $ctx }} {{- end }} {{- end }} -{{- if or .Values.server.configuration .Values.server.existingConfigmap }} +{{- if or $ctx.Values.server.configuration $ctx.Values.server.existingConfigmap }} - name: configuration configMap: - name: {{ include "authup.server.configurationConfigMapName" . }} + name: {{ include "authup.server.configurationConfigMapName" (dict "context" $ctx "hook" .hook) }} {{- end }} {{- end -}} @@ -409,10 +453,35 @@ looks exactly like an un-themed page. {{- end -}} {{- end -}} +{{/* +ConfigMap carrying authup.server.core.conf for one consumer. +Usage: {{ include "authup.server.configurationConfigMapName" (dict "context" $ "hook" true) }} + +"hook" resolves to the migration Job's own copy (templates/server/configmap- +migration-configuration.yaml), which is itself a pre-upgrade hook and is +therefore created before the Job. Two reasons the Job cannot share the release +ConfigMap: on the upgrade that first sets server.configuration it does not exist +yet, and on every later upgrade it still holds the PREVIOUS release's content +when the hook runs. Same reasoning that makes the Job inline configEnv. +An operator-supplied existingConfigmap is not the chart's to copy: it lives +outside the release and already exists when the hook runs. +*/}} {{- define "authup.server.configurationConfigMapName" -}} -{{- if .Values.server.existingConfigmap -}} -{{- include "authup.tplvalues.render" (dict "value" .Values.server.existingConfigmap "context" $) -}} +{{- $ctx := required "authup.server.configurationConfigMapName: call it as (dict \"context\" $ \"hook\" bool)" .context -}} +{{- if $ctx.Values.server.existingConfigmap -}} +{{- include "authup.tplvalues.render" (dict "value" $ctx.Values.server.existingConfigmap "context" $ctx) -}} +{{- else if .hook -}} +{{- printf "%s-migration-configuration" (include "authup.server.fullname" $ctx) -}} {{- else -}} -{{- printf "%s-configuration" (include "authup.server.fullname" .) -}} +{{- printf "%s-configuration" (include "authup.server.fullname" $ctx) -}} +{{- end -}} {{- end -}} + +{{/* +Rendered content of authup.server.core.conf. One source for the release +ConfigMap and the hook copy, so the migration cannot run against a config file +that differs from the one the server pods get. +*/}} +{{- define "authup.server.configurationContent" -}} +{{- include "authup.tplvalues.render" (dict "value" .Values.server.configuration "context" $) -}} {{- end -}} diff --git a/charts/authup/templates/server/configmap-configuration.yaml b/charts/authup/templates/server/configmap-configuration.yaml index cd6c48c..ad31b31 100644 --- a/charts/authup/templates/server/configmap-configuration.yaml +++ b/charts/authup/templates/server/configmap-configuration.yaml @@ -8,5 +8,5 @@ metadata: annotations: {{- include "authup.annotations" (dict "context" $) | nindent 4 }} data: authup.server.core.conf: |- - {{- include "authup.tplvalues.render" (dict "value" .Values.server.configuration "context" $) | nindent 4 }} + {{- include "authup.server.configurationContent" . | nindent 4 }} {{- end }} diff --git a/charts/authup/templates/server/configmap-migration-configuration.yaml b/charts/authup/templates/server/configmap-migration-configuration.yaml new file mode 100644 index 0000000..ffd99de --- /dev/null +++ b/charts/authup/templates/server/configmap-migration-configuration.yaml @@ -0,0 +1,39 @@ +{{- if and .Values.server.enabled .Values.server.migration.enabled .Values.server.configuration (not .Values.server.existingConfigmap) }} +{{/* +The migration Job's own copy of authup.server.core.conf. + +`migration run` loads the config file unconditionally, and the db options only +the file can carry (ssl, socketPath, replication, extensions, poolSize) decide +how it connects and what it creates, so the Job has to mount it. It cannot mount +the release ConfigMap: a pre-upgrade hook runs BEFORE the release manifest is +applied, so that object either does not exist yet (the upgrade that first sets +server.configuration) or still holds the previous release's content. Same reason +the Job inlines the env ConfigMap instead of mounting it. + +Rendered from authup.server.configurationContent, the single source the release +ConfigMap uses too, so migration and server pods can never read different files. +Hook weight -5 puts it ahead of the Job's 0 (helm applies the hook-succeeded +delete policy only after every hook in the event has run, so it outlives the +Job); the ArgoCD branch mirrors that with sync-wave -5. +*/}} +apiVersion: v1 +kind: ConfigMap +metadata: + name: {{ include "authup.server.configurationConfigMapName" (dict "context" $ "hook" true) }} + namespace: {{ include "authup.namespace" . | quote }} + labels: {{- include "authup.labels" (dict "context" $ "component" "migration") | nindent 4 }} + annotations: + {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + {{- if .Values.useHelmHooks }} + helm.sh/hook: pre-upgrade + helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded + helm.sh/hook-weight: "-5" + {{- else }} + argocd.argoproj.io/hook: PreSync + argocd.argoproj.io/hook-delete-policy: BeforeHookCreation + argocd.argoproj.io/sync-wave: "-5" + {{- end }} +data: + authup.server.core.conf: |- + {{- include "authup.server.configurationContent" . | nindent 4 }} +{{- end }} diff --git a/charts/authup/templates/server/deployment.yaml b/charts/authup/templates/server/deployment.yaml index ee53043..f13f0b3 100644 --- a/charts/authup/templates/server/deployment.yaml +++ b/charts/authup/templates/server/deployment.yaml @@ -121,7 +121,7 @@ spec: - secretRef: name: {{ include "authup.tplvalues.render" (dict "value" .Values.server.extraEnvVarsSecret "context" $) }} {{- end }} - env: {{- include "authup.server.secretEnv" . | nindent 12 }} + env: {{- include "authup.server.secretEnv" (dict "context" $) | nindent 12 }} {{- if .Values.server.extraEnvVars }} {{- include "authup.tplvalues.render" (dict "value" .Values.server.extraEnvVars "context" $) | nindent 12 }} {{- end }} @@ -164,7 +164,7 @@ spec: {{- if .Values.server.lifecycleHooks }} lifecycle: {{- include "authup.tplvalues.render" (dict "value" .Values.server.lifecycleHooks "context" $) | nindent 12 }} {{- end }} - volumeMounts: {{- include "authup.server.volumeMounts" . | nindent 12 }} + volumeMounts: {{- include "authup.server.volumeMounts" (dict "context" $) | nindent 12 }} {{- if include "authup.server.themeMounted" . }} {{- include "authup.server.themeVolumeMounts" . | nindent 12 }} {{- end }} @@ -174,7 +174,7 @@ spec: {{- if .Values.server.sidecars }} {{- include "authup.tplvalues.render" (dict "value" .Values.server.sidecars "context" $) | nindent 8 }} {{- end }} - volumes: {{- include "authup.server.volumes" . | nindent 8 }} + volumes: {{- include "authup.server.volumes" (dict "context" $) | nindent 8 }} {{- if include "authup.server.themeMounted" . }} {{- include "authup.server.themeVolumes" . | nindent 8 }} {{- end }} diff --git a/charts/authup/templates/server/migration-job.yaml b/charts/authup/templates/server/migration-job.yaml index 06f4f46..f1bc051 100644 --- a/charts/authup/templates/server/migration-job.yaml +++ b/charts/authup/templates/server/migration-job.yaml @@ -69,7 +69,7 @@ spec: - name: {{ $key }} value: {{ $value | quote }} {{- end }} - {{- include "authup.server.secretEnv" . | nindent 12 }} + {{- include "authup.server.secretEnv" (dict "context" $ "hook" true) | nindent 12 }} {{- if .Values.server.extraEnvVars }} {{- include "authup.tplvalues.render" (dict "value" .Values.server.extraEnvVars "context" $) | nindent 12 }} {{- end }} @@ -77,11 +77,11 @@ spec: {{- if $resources }} resources: {{- toYaml $resources | nindent 12 }} {{- end }} - volumeMounts: {{- include "authup.server.volumeMounts" . | nindent 12 }} + volumeMounts: {{- include "authup.server.volumeMounts" (dict "context" $ "hook" true) | nindent 12 }} {{- if .Values.server.extraVolumeMounts }} {{- include "authup.tplvalues.render" (dict "value" .Values.server.extraVolumeMounts "context" $) | nindent 12 }} {{- end }} - volumes: {{- include "authup.server.volumes" . | nindent 8 }} + volumes: {{- include "authup.server.volumes" (dict "context" $ "hook" true) | nindent 8 }} {{- if .Values.server.extraVolumes }} {{- include "authup.tplvalues.render" (dict "value" .Values.server.extraVolumes "context" $) | nindent 8 }} {{- end }} diff --git a/charts/authup/values.schema.json b/charts/authup/values.schema.json index 7324034..51aaf40 100644 --- a/charts/authup/values.schema.json +++ b/charts/authup/values.schema.json @@ -3641,7 +3641,7 @@ }, "useHelmHooks": { "default": true, - "description": "Render Job hook annotations (set false for ArgoCD / Flux)", + "description": "Render Helm hook annotations on the migration Job. Set false only for\nArgoCD, which reads its own PreSync annotations instead (it also understands\nHelm hooks, so true works there too). Flux and plain helm need true: a plain\nJob's pod template is immutable, so the next upgrade cannot patch it.", "required": [], "title": "useHelmHooks", "type": "boolean" diff --git a/charts/authup/values.yaml b/charts/authup/values.yaml index f205f23..83e654d 100644 --- a/charts/authup/values.yaml +++ b/charts/authup/values.yaml @@ -34,7 +34,10 @@ commonLabels: {} commonAnnotations: {} # -- Extra objects to deploy (rendered through tpl; list of manifests or strings) extraDeploy: [] -# -- Render Job hook annotations (set false for ArgoCD / Flux) +# -- Render Helm hook annotations on the migration Job. Set false only for +# ArgoCD, which reads its own PreSync annotations instead (it also understands +# Helm hooks, so true works there too). Flux and plain helm need true: a plain +# Job's pod template is immutable, so the next upgrade cannot patch it. useHelmHooks: true diagnosticMode: # -- Start every container with a sleep command and disable probes (debugging) From 78e97085ef6ba12f538431a100cda2f89110dafd Mon Sep 17 00:00:00 2001 From: tada5hi Date: Mon, 24 Aug 2026 08:01:48 +0200 Subject: [PATCH 2/6] fix(authup): take USER_ADMIN_PASSWORD off the migration hook too, and correct the records A follow-up audit found the comment justifying its retention was wrong. It said the key is unconditional "so it costs nothing", but an unconditional KEY is not an existing OBJECT: authup.auth.secretName resolves to the chart-managed auth Secret, an ordinary release resource, so the upgrade that drops auth.existingSecret in favour of a chart-managed password schedules a hook pod whose secretKeyRef target does not exist yet. That is the same failure this branch removed for CLIENT_SYSTEM_SECRET, and `migration run` builds no identity or provisioning module, so the value was pure cost. The hook now carries exactly two secret-backed entries: DB_PASSWORD and, deliberately, the KEK. Record corrections, all found by the same audit: - architecture.md rule 10 said "three helpers" (it is four: configEnv is inlined rather than flagged) and narrowed the DB_PASSWORD residual to an engine switch. It also misses two residuals the flag cannot reach: serviceAccountName (the ServiceAccount renders under serviceAccount.create, and a missing one fails pod admission with no container status), and the extraEnvVarsCM / extraEnvVarsSecret / extraVolumes passthroughs, whose targets land after the hook when shipped through extraDeploy. - references/authup.md listed `entities` and `subscribers` as file-only db keys. DB_ENTITIES and DB_SUBSCRIBERS exist. The six that carry the argument for mounting the config file (ssl, socketPath, replication, poolSize, charset, extensions) are genuinely absent from typeorm-extension's env reader; the bullet now says how to re-derive the list. - testing.md's NOTES recipe was a trap: helm excludes templates/ from .Files, so the obvious `.Files.Get "templates/NOTES.txt"` wrapper renders empty and BOTH directions of the assertion pass. Replaced with the inlining recipe, verified to give 1 and 0. --- .agents/architecture.md | 30 ++++++++++++++++++------- .agents/references/authup.md | 7 ++++-- .agents/testing.md | 20 +++++++++++++---- DESIGN.md | 2 +- charts/authup/Chart.yaml | 2 +- charts/authup/templates/_server-env.tpl | 13 ++++++----- 6 files changed, 53 insertions(+), 21 deletions(-) diff --git a/.agents/architecture.md b/.agents/architecture.md index 10675d0..8dd6415 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -53,8 +53,10 @@ editing templates or values. which is an ArgoCD-only mode: see rule 19. Helm applies a pre-upgrade hook BEFORE the release manifest, so everything - the Job references must already exist from the PREVIOUS release. The three - helpers take a `hook` flag that drops what `migration run` does not read: + the Job references must already exist from the PREVIOUS release. Four + helpers take a `hook` flag (`secretEnv`, the two volume helpers and + `configurationConfigMapName`; `configEnv` does not, it is inlined instead) + and drop what `migration run` does not read: `REDIS`, `SMTP` (their Secrets are release resources, and the migration builds no cache or mail module) and the provisioning mount (`ProvisionerModule` is registered by the start command only). What stays, stays for a reason: @@ -66,12 +68,24 @@ editing templates or values. The Job reads that file from a hook-scoped COPY (`server/configmap-migration-configuration.yaml`, weight -5) for the same reason it inlines the env: the release ConfigMap is either absent or one - release stale when the hook runs. `CLIENT_SYSTEM_SECRET` goes the same way: - its key inside the auth Secret is conditional. `SECRETS_ENCRYPTION_KEY` - deliberately does NOT, even though its key is conditional too and the - migration does not read it today: rule 6's fail-closed posture outranks the - one-off break, so a write-once KEK gets its own upgrade. `DB_PASSWORD` is the - other residual, and only when an upgrade also switches database engine. + release stale when the hook runs. `USER_ADMIN_PASSWORD` and + `CLIENT_SYSTEM_SECRET` go the same way: no identity or provisioning module + on the migration path, and the auth Secret they read is itself a release + resource. `SECRETS_ENCRYPTION_KEY` deliberately does NOT, even though its + key is conditional too and the migration does not read it today: rule 6's + fail-closed posture outranks the one-off break, so a write-once KEK gets its + own upgrade. + + What the flag cannot reach, i.e. the residuals to keep in mind when adding + anything to the Job: `DB_PASSWORD` (the Secret behind it changes on an engine + switch, on adopting a built-in engine after `externalDatabase`, and on a + first inline `externalDatabase.password`, since `secret-db.yaml` is a release + resource too); the `serviceAccountName`, whose ServiceAccount renders only + under `serviceAccount.create`, so flipping that on fails pod ADMISSION with + no container status to read; and the `extraEnvVarsCM` / `extraEnvVarsSecret` + / `extraVolumes` passthroughs, whose targets are operator-owned unless the + operator ships them through `extraDeploy`, which renders them into the + release manifest and therefore after the hook. 11. **Checksum annotations roll pods on config or secret changes.** The server deployment checksums the env map plus every chart-managed secret it consumes (auth, external-db, redis, smtp, provisioning, configuration), diff --git a/.agents/references/authup.md b/.agents/references/authup.md index f8ec748..6c4110c 100644 --- a/.agents/references/authup.md +++ b/.agents/references/authup.md @@ -76,8 +76,11 @@ unsupported per `.agents/architecture.md` in the monorepo), (`config/read/fs.ts`). Env wins per key, but the db keys typeorm-extension's env reader does not name survive: `ssl`, `socketPath`, `replication`, `poolSize`, `charset`, `extensions` (postgres `CREATE EXTENSION` during - `initialize()`), `entities`, `subscribers`. So the config file decides how the - migration connects and what it creates -> the chart MUST mount it on the Job. + `initialize()`). So the config file decides how the migration connects and + what it creates -> the chart MUST mount it on the Job. (`entities` and + `subscribers` are NOT in that set: `DB_ENTITIES` / `DB_SUBSCRIBERS` exist. + Dump the real list with + `grep -rhoE "DB_[A-Z_]+" node_modules/typeorm-extension/dist | sort -u`.) - Under `NODE_ENV=production` (baked into the image) `migration run` needs the writable directory before it touches the database: the logger adds winston File transports for `/http.log` and `/error.log`, and the diff --git a/.agents/testing.md b/.agents/testing.md index 6eb44f9..d712e3e 100644 --- a/.agents/testing.md +++ b/.agents/testing.md @@ -96,8 +96,9 @@ helm template t charts/authup --set server.migration.enabled=true \ -s templates/server/migration-job.yaml ``` -The Job must show env `DB_PASSWORD` / `USER_ADMIN_PASSWORD` but NO `REDIS`, no -`SMTP` and no `CLIENT_SYSTEM_SECRET`; volumes `writable` / `tmp` / +The Job's only secret-backed env must be `DB_PASSWORD` (plus +`SECRETS_ENCRYPTION_KEY` when the KEK is set): no `REDIS`, no `SMTP`, no +`USER_ADMIN_PASSWORD`, no `CLIENT_SYSTEM_SECRET`. Volumes `writable` / `tmp` / `configuration` but NO `provisioning`; and the configuration volume must name `-server-migration-configuration` (the hook-scoped copy at weight -5), never `-server-configuration`. The @@ -109,8 +110,19 @@ failing. `useHelmHooks=false` must print the Flux/plain-helm warning in NOTES.txt, and must not print it with hooks on. NOTES is not reachable through `helm template`, -so wrap it: copy the chart, append `templates/NOTES.txt` into a ConfigMap -template, render that. +and `.Files.Get "templates/NOTES.txt"` does NOT work either (helm excludes +`templates/` from `.Files`, so the wrapper renders empty and BOTH directions +"pass"). Inline the raw template text into a generated template instead: + +```bash +cp -r charts/authup /tmp/nc +{ printf 'apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: notes\ndata:\n notes: |\n'; \ + sed 's/^/ /' /tmp/nc/templates/NOTES.txt; } > /tmp/nc/templates/zz-notes.yaml +helm template t /tmp/nc --set server.migration.enabled=true --set useHelmHooks=false \ + -s templates/zz-notes.yaml | grep -c 'useHelmHooks=false' # must be >0 +helm template t /tmp/nc --set server.migration.enabled=true \ + -s templates/zz-notes.yaml | grep -c 'useHelmHooks=false' # must be 0 +``` Umbrella use is part of the contract: `global` must stay open. Render a throwaway parent chart with authup in `charts/` and an unrelated global (`global.myOrgKey`) diff --git a/DESIGN.md b/DESIGN.md index 378862f..38cbee1 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -326,7 +326,7 @@ first-class templating, in v1. annotations instead. Flux runs a real `helm upgrade` and honours Helm hooks, so a plain Job there hits the immutable `spec.template` on the next upgrade. - The hook Job sees only the PREVIOUS release's ConfigMaps and Secrets, so it - carries a narrowed env/mount set (no REDIS, SMTP, CLIENT_SYSTEM_SECRET or + carries a narrowed env/mount set (DB_PASSWORD and the encryption key only, no provisioning mount) plus a hook-scoped copy of `authup.server.core.conf`, which `migration run` does read. diff --git a/charts/authup/Chart.yaml b/charts/authup/Chart.yaml index ea54876..dfbfa36 100644 --- a/charts/authup/Chart.yaml +++ b/charts/authup/Chart.yaml @@ -38,7 +38,7 @@ annotations: - kind: added description: server.route.enabled / adminConsole.route.enabled accept a tpl-rendered string, so an umbrella chart can drive them from a global - kind: fixed - description: The pre-upgrade migration Job no longer references release resources that do not exist when the hook runs; it drops the provisioning mount plus REDIS, SMTP and CLIENT_SYSTEM_SECRET, none of which migration run reads + description: The pre-upgrade migration Job no longer references release resources that do not exist when the hook runs; it drops the provisioning mount and every secret env migration run does not read (REDIS, SMTP, USER_ADMIN_PASSWORD, CLIENT_SYSTEM_SECRET), keeping only DB_PASSWORD and the encryption key - kind: fixed description: The migration Job reads authup.server.core.conf from a hook-scoped copy, so it no longer runs against a missing or one-release-stale config file - kind: changed diff --git a/charts/authup/templates/_server-env.tpl b/charts/authup/templates/_server-env.tpl index 2cfa2f8..9114f56 100644 --- a/charts/authup/templates/_server-env.tpl +++ b/charts/authup/templates/_server-env.tpl @@ -63,11 +63,12 @@ pre-upgrade hook BEFORE the release manifest, so the upgrade that first enables valkey or SMTP would schedule a hook pod whose secretKeyRef target does not exist yet (CreateContainerConfigError until the hook times out). `migration run` builds config + logger + database only, no cache and no mail module, so neither -value is read there. CLIENT_SYSTEM_SECRET goes for the same reason: its key -inside the chart-managed auth Secret is conditional, so flipping -auth.systemClientEnabled on breaks the hook the same way. What stays: DB_PASSWORD -(the migration cannot run without it), USER_ADMIN_PASSWORD (unread, but its key -is unconditional, so it costs nothing) and the KEK (see its comment below). +value is read there. USER_ADMIN_PASSWORD and CLIENT_SYSTEM_SECRET go too: the +migration builds no identity or provisioning module either, and the auth Secret +they read is itself a release resource (an upgrade dropping auth.existingSecret +for a chart-managed one creates it only AFTER the hook), on top of +CLIENT_SYSTEM_SECRET's key being conditional. The hook keeps exactly two: +DB_PASSWORD, without which the migration cannot run, and the KEK (see below). */}} {{- define "authup.server.secretEnv" -}} {{- $ctx := required "authup.server.secretEnv: call it as (dict \"context\" $ \"hook\" bool)" .context -}} @@ -90,11 +91,13 @@ is unconditional, so it costs nothing) and the KEK (see its comment below). name: {{ include "authup.smtp.secretName" $ctx }} key: {{ include "authup.smtp.secretKey" $ctx }} {{- end }} +{{- if not .hook }} - name: USER_ADMIN_PASSWORD valueFrom: secretKeyRef: name: {{ include "authup.auth.secretName" $ctx }} key: {{ $ctx.Values.auth.secretKeys.adminPasswordKey }} +{{- end }} {{- if and (not .hook) $ctx.Values.auth.systemClientEnabled }} - name: CLIENT_SYSTEM_SECRET valueFrom: From 3ec127409daccfe25677bc4d88ae6bdbf5849796 Mon Sep 17 00:00:00 2001 From: tada5hi Date: Mon, 24 Aug 2026 09:13:29 +0200 Subject: [PATCH 3/6] fix(authup): execute the migration hook in CI, cap the Job name, and fail on two config carriers Four follow-ups from the audit round, all decided on the PR. ct now runs upgrades (.github/configs/ct.yaml). Nothing in this repo had ever created a pre-upgrade hook on a cluster: `ct install` runs without --upgrade, so the migration Job and its hook-scoped ConfigMap were render-tested only, and the ordering claim rested on reading helm and gitops-engine source. `upgrade: true` gives two real `helm upgrade`s per ci values file, master's chart to this revision and this revision to itself. The first leg is the interesting one right now: master's ci/valkey-values.yaml has no server.configuration while this branch's does, so it IS the "first upgrade that introduces the config file with the migration hook on" scenario the hook-scoped copy exists for. ct skips that leg once a release bumps the middle digit (0.x reads that as breaking via a ~0.x.y constraint); the self-upgrade leg always runs. The kind job gets roughly 3x longer. The theme volume folds into the hook flag. It was a pair of deployment-only defines carved out for exactly the hook reason the flag now expresses, which left two calling conventions in one volumeMounts: block two lines apart, i.e. the most likely way the next mount lands on the wrong side. Net deletion, and the render is object-identical (it also drops two whitespace-only lines the old call-site guards emitted). themeEnv deliberately stays separate: it splits along a different axis, since configEnv is one define shared by the env ConfigMap and the Job's inlined env, and THEME_* has to stay in its reserved-key list. The migration Job name is capped at 63 (#21). Rule 9's trunc-52 base plus "-server" plus "-migration" reaches 69, and with no hand-written spec.selector the API server copies the Job name into the job-name / batch.kubernetes.io/ job-name pod labels, where a label VALUE stops at 63. A long release name therefore produced a Job the API server rejects, hanging the upgrade until the hook timeout. Rule 9 now records that it keeps names distinct, not short, and that 63 rather than 253 is the ceiling wherever a name becomes a label value or a DNS-1035 label. server.configuration together with server.existingConfigmap now fails the render (#23). The existing ConfigMap is the one that gets mounted, so the inline content was silently dropped, and that content is usually where db.ssl / socketPath / replication live: the operator believed TLS to the database was configured while the mounted file was somebody else's. Same shape as the auth.existingSecret guard right above it. Recorded in BREAKING.md. Closes #21 Closes #23 --- .agents/architecture.md | 13 +++- .agents/testing.md | 14 ++++ .github/configs/ct.yaml | 6 ++ charts/authup/BREAKING.md | 7 ++ charts/authup/Chart.yaml | 4 ++ charts/authup/templates/_server-env.tpl | 68 +++++++++---------- .../authup/templates/server/deployment.yaml | 6 -- .../templates/server/migration-job.yaml | 8 ++- charts/authup/templates/validations.yaml | 11 +++ 9 files changed, 91 insertions(+), 46 deletions(-) diff --git a/.agents/architecture.md b/.agents/architecture.md index 8dd6415..403135f 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -40,7 +40,12 @@ editing templates or values. Services within one release. 9. **Component fullnames truncate the base BEFORE suffixing** (`trunc 52` then `-server` / `-ui` / engine suffix), so long release names - cannot collapse every resource onto one identical name. + cannot collapse every resource onto one identical name. That rule keeps names + DISTINCT; it does not keep them SHORT. `trunc 52` plus a suffix exceeds 63, + and 63 (not 253) is the real ceiling wherever a name becomes a label value or + a DNS-1035 label: the migration Job is capped at 63 for that reason (the API + server copies its name into the `job-name` pod labels). Anything new that + suffixes a component fullname has to ask which ceiling applies to its kind. 10. **The migration Job shares the deployment's env by construction, minus what a hook cannot see.** `authup.server.configEnv` (map), `authup.server.secretEnv` (list) and the two volume helpers are the single @@ -56,7 +61,11 @@ editing templates or values. the Job references must already exist from the PREVIOUS release. Four helpers take a `hook` flag (`secretEnv`, the two volume helpers and `configurationConfigMapName`; `configEnv` does not, it is inlined instead) - and drop what `migration run` does not read: + and drop what `migration run` does not read. That flag is the ONE mechanism + for this: the theme volume used to be a pair of deployment-only defines + carved out for the same reason, and two conventions in one `volumeMounts:` + block is how the next mount ends up on the wrong side. `themeEnv` stays + separate because it splits along a different axis. Dropped: `REDIS`, `SMTP` (their Secrets are release resources, and the migration builds no cache or mail module) and the provisioning mount (`ProvisionerModule` is registered by the start command only). What stays, stays for a reason: diff --git a/.agents/testing.md b/.agents/testing.md index d712e3e..17fb950 100644 --- a/.agents/testing.md +++ b/.agents/testing.md @@ -49,6 +49,7 @@ helm template t charts/authup --set server.config.PUBLIC_URL=http://x # f helm template t charts/authup --set server.config.WRITABLE_DIRECTORY_PATH=/x # ditto; the chart pins this one to the path it mounts helm template t charts/authup --set 'server.route.enabled=yes' # flag that is neither true nor false helm template t charts/authup --set adminConsole.enabled=false --set adminConsole.route.enabled=yes # ditto: validated even with the component off +helm template t charts/authup --set 'server.configuration=logger: true' --set server.existingConfigmap=cm # both config carriers helm template t charts/authup --set server.theme.enabled=true # theme with no carrier helm template t charts/authup --set server.theme.enabled=true --set server.theme.title=X --set server.theme.existingConfigMap=cm # manifest + existing CM helm template t charts/authup --set server.theme.enabled=true --set server.theme.logo=logo.svg # asset outside assets/ @@ -108,6 +109,19 @@ and its file-only db keys (`ssl`, `socketPath`, `extensions`) govern the connection, so a missing mount migrates over a plaintext connection instead of failing. +Names have two ceilings, not one. Rule 9's `trunc 52` keeps component names +distinct, not short, and 63 is the limit wherever a name lands in a label value +or a DNS-1035 label. Check the migration Job at the longest release name helm +accepts: + +```bash +helm template $(python3 -c "print('n'*53)") charts/authup \ + --set server.migration.enabled=true | grep '^ name:' | awk '{print length($2), $2}' | sort -rn +``` + +The Job must be <= 63. ConfigMaps and Secrets may exceed it (253 applies), but a +Service may not. + `useHelmHooks=false` must print the Flux/plain-helm warning in NOTES.txt, and must not print it with hooks on. NOTES is not reachable through `helm template`, and `.Files.Get "templates/NOTES.txt"` does NOT work either (helm excludes diff --git a/.github/configs/ct.yaml b/.github/configs/ct.yaml index 8cb618c..b0c5757 100644 --- a/.github/configs/ct.yaml +++ b/.github/configs/ct.yaml @@ -3,6 +3,12 @@ target-branch: master chart-dirs: - charts helm-extra-args: --timeout 600s +# Runs two real `helm upgrade`s per ci values file: master's chart -> this +# revision, then this revision -> itself. Without it no pre-upgrade hook is ever +# created on a cluster, so the migration Job and its hook-scoped ConfigMap are +# render-tested only. ct skips the first leg once a release bumps the middle +# digit (0.x treats that as breaking); the self-upgrade leg always runs. +upgrade: true check-version-increment: false validate-maintainers: false lint-conf: .github/configs/lintconf.yaml diff --git a/charts/authup/BREAKING.md b/charts/authup/BREAKING.md index 3bd34a2..1fb757d 100644 --- a/charts/authup/BREAKING.md +++ b/charts/authup/BREAKING.md @@ -5,6 +5,13 @@ land on the middle digit. Every entry lists the value migrations required. ## Next release (unreleased) +- Setting BOTH `server.configuration` and `server.existingConfigmap` now fails + the render. It never worked: the existing ConfigMap is the one that gets + mounted, so the inline content was silently dropped, and that content is + typically where `db.ssl` / `socketPath` / `replication` live, i.e. how the + server pods and the pre-upgrade migration hook connect to the database. Move + the inline content into the referenced ConfigMap, or drop + `server.existingConfigmap`. - The writable directory moves from `/usr/src/app/writable` to `/var/lib/authup`, following the image (authup/authup#3474, shipped in v1.0.0-beta.63). The chart mounts an emptyDir there, so nothing persists across the change; only a diff --git a/charts/authup/Chart.yaml b/charts/authup/Chart.yaml index dfbfa36..56409fb 100644 --- a/charts/authup/Chart.yaml +++ b/charts/authup/Chart.yaml @@ -43,3 +43,7 @@ annotations: description: The migration Job reads authup.server.core.conf from a hook-scoped copy, so it no longer runs against a missing or one-release-stale config file - kind: changed description: useHelmHooks=false is documented as ArgoCD-only; under Flux or plain helm it makes the Job a plain resource whose immutable pod template fails the next upgrade + - kind: fixed + description: The migration Job name is capped at 63 characters, the limit that applies because Kubernetes copies it into the job-name pod labels; long release names previously produced an unschedulable Job that hung the upgrade + - kind: changed + description: Setting both server.configuration and server.existingConfigmap now fails the render instead of silently dropping the inline content, which is usually where db.ssl and socketPath live diff --git a/charts/authup/templates/_server-env.tpl b/charts/authup/templates/_server-env.tpl index 9114f56..eeb9fb3 100644 --- a/charts/authup/templates/_server-env.tpl +++ b/charts/authup/templates/_server-env.tpl @@ -149,6 +149,13 @@ that only the file can carry (ssl, socketPath, replication, extensions, poolSize decide how the migration connects and what it creates. Dropping it would silently migrate over a plaintext connection. The Job reads it from a hook-scoped copy instead: see authup.server.configurationConfigMapName. + +The theme volume rides the same flag. It used to be a pair of deployment-only +defines carved out for exactly this hook reason, which left two calling +conventions in deployment.yaml two lines apart; one mechanism is one thing to +get right. authup.server.themeEnv stays separate: it splits along a different +axis (configEnv is one define shared by the env ConfigMap and the Job's inlined +env, and THEME_* must stay in its reserved-key list either way). */}} {{- define "authup.server.volumeMounts" -}} {{- $ctx := required "authup.server.volumeMounts: call it as (dict \"context\" $ \"hook\" bool)" .context -}} @@ -167,6 +174,11 @@ instead: see authup.server.configurationConfigMapName. subPath: authup.server.core.conf readOnly: true {{- end }} +{{- if and (not .hook) (include "authup.server.themeMounted" $ctx) }} +- name: theme + mountPath: {{ include "authup.server.themeMountPath" $ctx }} + readOnly: true +{{- end }} {{- end -}} {{- define "authup.server.volumes" -}} @@ -190,15 +202,27 @@ instead: see authup.server.configurationConfigMapName. configMap: name: {{ include "authup.server.configurationConfigMapName" (dict "context" $ctx "hook" .hook) }} {{- end }} +{{- if and (not .hook) (include "authup.server.themeMounted" $ctx) }} +- name: theme + configMap: + name: {{ include "authup.server.themeConfigMapName" $ctx }} + {{- /* Whole-volume projection on purpose: a subPath mount is frozen + until the pod restarts, which would destroy authup's live theme + reload. */}} + {{- if $ctx.Values.server.theme.existingConfigMap }} + {{- with $ctx.Values.server.theme.existingConfigMapItems }} + items: {{- include "authup.tplvalues.render" (dict "value" . "context" $ctx) | nindent 6 }} + {{- end }} + {{- else }} + items: + {{- range $path := splitList "\n" (include "authup.server.themePaths" $ctx) }} + - key: {{ include "authup.server.themeConfigMapKey" $path }} + path: {{ $path }} + {{- end }} + {{- end }} +{{- end }} {{- end -}} -{{/* -Theme volume / volumeMount, deliberately NOT part of the shared server -helpers: the migration Job is a pre-upgrade HOOK, and hooks precede regular -resources, so on the upgrade that first enables theming it would reference a -ConfigMap that does not exist yet and hang. A migration run has no use for -the theme either way. -*/}} {{/* Theme environment, kept OUT of authup.server.configEnv for the same reason as the volume: the migration Job inlines configEnv, and pointing @@ -217,36 +241,6 @@ THEME_FRAGMENTS_ENABLED: {{ .Values.server.theme.fragmentsEnabled | toString | q {{- end }} {{- end -}} -{{- define "authup.server.themeVolumeMounts" -}} -{{- if include "authup.server.themeMounted" . }} -- name: theme - mountPath: {{ include "authup.server.themeMountPath" . }} - readOnly: true -{{- end }} -{{- end -}} - -{{- define "authup.server.themeVolumes" -}} -{{- if include "authup.server.themeMounted" . }} -- name: theme - configMap: - name: {{ include "authup.server.themeConfigMapName" . }} - {{- /* Whole-volume projection on purpose: a subPath mount is frozen - until the pod restarts, which would destroy authup's live theme - reload. */}} - {{- if .Values.server.theme.existingConfigMap }} - {{- with .Values.server.theme.existingConfigMapItems }} - items: {{- include "authup.tplvalues.render" (dict "value" . "context" $) | nindent 6 }} - {{- end }} - {{- else }} - items: - {{- range $path := splitList "\n" (include "authup.server.themePaths" $) }} - - key: {{ include "authup.server.themeConfigMapKey" $path }} - path: {{ $path }} - {{- end }} - {{- end }} -{{- end }} -{{- end -}} - {{/* Absolute path the theme volume is mounted at, and the value of THEME_DIRECTORY_PATH. A constant: the chart owns both ends. diff --git a/charts/authup/templates/server/deployment.yaml b/charts/authup/templates/server/deployment.yaml index f13f0b3..5c91523 100644 --- a/charts/authup/templates/server/deployment.yaml +++ b/charts/authup/templates/server/deployment.yaml @@ -165,9 +165,6 @@ spec: lifecycle: {{- include "authup.tplvalues.render" (dict "value" .Values.server.lifecycleHooks "context" $) | nindent 12 }} {{- end }} volumeMounts: {{- include "authup.server.volumeMounts" (dict "context" $) | nindent 12 }} - {{- if include "authup.server.themeMounted" . }} - {{- include "authup.server.themeVolumeMounts" . | nindent 12 }} - {{- end }} {{- if .Values.server.extraVolumeMounts }} {{- include "authup.tplvalues.render" (dict "value" .Values.server.extraVolumeMounts "context" $) | nindent 12 }} {{- end }} @@ -175,9 +172,6 @@ spec: {{- include "authup.tplvalues.render" (dict "value" .Values.server.sidecars "context" $) | nindent 8 }} {{- end }} volumes: {{- include "authup.server.volumes" (dict "context" $) | nindent 8 }} - {{- if include "authup.server.themeMounted" . }} - {{- include "authup.server.themeVolumes" . | nindent 8 }} - {{- end }} {{- if .Values.server.extraVolumes }} {{- include "authup.tplvalues.render" (dict "value" .Values.server.extraVolumes "context" $) | nindent 8 }} {{- end }} diff --git a/charts/authup/templates/server/migration-job.yaml b/charts/authup/templates/server/migration-job.yaml index f1bc051..80cb6be 100644 --- a/charts/authup/templates/server/migration-job.yaml +++ b/charts/authup/templates/server/migration-job.yaml @@ -10,7 +10,13 @@ against the previous release's ConfigMap. apiVersion: batch/v1 kind: Job metadata: - name: {{ printf "%s-migration" (include "authup.server.fullname" .) }} + {{- /* Capped at 63, not the 253 a Job name would otherwise allow: with no + hand-written spec.selector the API server copies this name into the + job-name / batch.kubernetes.io/job-name pod labels, and a LABEL VALUE + stops at 63. Rule 9's trunc-52 base plus "-server" plus "-migration" + reaches 69, so a long release name would make the hook unschedulable + and hang the upgrade. */}} + name: {{ printf "%s-migration" (include "authup.server.fullname" .) | trunc 63 | trimSuffix "-" }} namespace: {{ include "authup.namespace" . | quote }} labels: {{- include "authup.labels" (dict "context" $ "component" "migration") | nindent 4 }} annotations: diff --git a/charts/authup/templates/validations.yaml b/charts/authup/templates/validations.yaml index 7e8a499..65cf34d 100644 --- a/charts/authup/templates/validations.yaml +++ b/charts/authup/templates/validations.yaml @@ -34,6 +34,17 @@ store, token blocklist and MFA challenges fall back to a per-process memory cach {{- fail "authup: auth.existingSecret and inline auth values are mutually exclusive — when an existing secret is referenced, the inline adminPassword / systemClientSecret / secretsEncryptionKey values are unused; remove them to avoid a false sense of configuration." }} {{- end }} +{{/* +Same shape as the auth pair above, and worth its own guard because the value it +drops is usually load-bearing: server.configuration is where db.ssl / socketPath +/ replication live, and those decide how BOTH the server pods and the pre-upgrade +migration hook connect. Existing-wins silently would mean believing TLS to the +database is configured while the mounted file is the operator's. +*/}} +{{- if and .Values.server.enabled .Values.server.configuration .Values.server.existingConfigmap }} +{{- fail "authup: server.configuration and server.existingConfigmap are mutually exclusive. The existing ConfigMap is the one that gets mounted, so the inline content would be silently dropped: move it into that ConfigMap, or drop server.existingConfigmap." }} +{{- end }} + {{- if and .Values.adminConsole.enabled (not .Values.server.enabled) (not .Values.adminConsole.apiUrl) }} {{- fail "authup: adminConsole.enabled without server.enabled requires adminConsole.apiUrl (the browser-reachable URL of an external authup server-core)." }} {{- end }} From e6894d1726920fe7d31724995d5739acc9fcb748 Mon Sep 17 00:00:00 2001 From: tada5hi Date: Mon, 24 Aug 2026 09:18:59 +0200 Subject: [PATCH 4/6] fix(authup)!: derive component name budgets from the suffix so long release names install The admin-console Service reached 66 characters. A Service name is a DNS-1035 label, capped at 63, so any release whose fullname hit 50 characters (a release name of about 43) was rejected by the API server and could not install at all. No opt-in flag needed, unlike the migration Job in #21. authup.component.fullname is now the single implementation: it takes the suffix, computes `min 52 (63 - len(suffix) - 1)`, truncates the base to that, then suffixes. All five component helpers and the migration Job go through it, so the Job also stops needing a post-hoc trunc and renders a clean -server-migration instead of a mid-word -server-mig. `min 52` is the load-bearing half. The derived budget is WIDER than 52 for short suffixes, and widening renames resources on releases whose fullname lands between 53 and 55 characters. A renamed Secret carrying helm.sh/resource-policy: keep orphans the old one and generates a fresh admin password and system-client secret, i.e. a silent credential rotation on upgrade. The budget may only ever tighten. Verified rather than assumed: rendering every release-name length from 3 to 53 against origin/master, the name sets differ at no length where master was installable, and the branch is installable at every length. Master first becomes un-installable at 40 with the migration Job enabled, 43 without. Marked breaking because names do move for releases in that range, even though none of them can currently exist in a cluster. Closes #24 --- .agents/architecture.md | 29 ++++++++++----- .agents/testing.md | 24 +++++++++---- charts/authup/Chart.yaml | 2 +- charts/authup/templates/_helpers.tpl | 35 +++++++++++++++---- .../templates/server/migration-job.yaml | 16 +++++---- 5 files changed, 77 insertions(+), 29 deletions(-) diff --git a/.agents/architecture.md b/.agents/architecture.md index 403135f..d0aae71 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -38,14 +38,27 @@ editing templates or values. name + instance + component. `commonLabels` / `podLabels` must never leak into a selector. `app.kubernetes.io/component` separates the two services' Services within one release. -9. **Component fullnames truncate the base BEFORE suffixing** - (`trunc 52` then `-server` / `-ui` / engine suffix), so long release names - cannot collapse every resource onto one identical name. That rule keeps names - DISTINCT; it does not keep them SHORT. `trunc 52` plus a suffix exceeds 63, - and 63 (not 253) is the real ceiling wherever a name becomes a label value or - a DNS-1035 label: the migration Job is capped at 63 for that reason (the API - server copies its name into the `job-name` pod labels). Anything new that - suffixes a component fullname has to ask which ceiling applies to its kind. +9. **Component fullnames truncate the base BEFORE suffixing, on a budget + derived from the suffix.** `authup.component.fullname` + (`dict "context" $ "suffix" "server"`) is the single implementation; every + component name and the migration Job go through it. Truncating first is what + keeps names DISTINCT (a 63-char fullname would otherwise collapse every + component onto one name); deriving the budget is what keeps them LEGAL. + + The ceiling is 63, not the 253 a ConfigMap allows, wherever a name becomes a + DNS-1035 label (Service) or a label value (a Job name is copied into the + `job-name` pod labels). The old flat `trunc 52` ignored that: `-admin-console` + rendered a 66-char Service, so any release name from ~43 characters up could + not install at all, and appending `-migration` to the `-server` name reached + 69. Both are now `min 52 (63 - len(suffix) - 1)`. + + `min 52` is the load-bearing half. The derived budget is WIDER than 52 for + short suffixes, and widening RENAMES resources on releases whose fullname + lands between 53 and 55 characters. A renamed Secret carrying + `helm.sh/resource-policy: keep` orphans the old one and generates a new admin + password: a silent credential rotation on upgrade. **The budget may only ever + tighten**, which by construction touches only names too long to exist. Assert + that when changing it (see testing.md), do not assume it. 10. **The migration Job shares the deployment's env by construction, minus what a hook cannot see.** `authup.server.configEnv` (map), `authup.server.secretEnv` (list) and the two volume helpers are the single diff --git a/.agents/testing.md b/.agents/testing.md index 17fb950..ca6054b 100644 --- a/.agents/testing.md +++ b/.agents/testing.md @@ -109,18 +109,28 @@ and its file-only db keys (`ssl`, `socketPath`, `extensions`) govern the connection, so a missing mount migrates over a plaintext connection instead of failing. -Names have two ceilings, not one. Rule 9's `trunc 52` keeps component names -distinct, not short, and 63 is the limit wherever a name lands in a label value -or a DNS-1035 label. Check the migration Job at the longest release name helm -accepts: +Names have two ceilings, not one (rule 9). 63 applies to a Service (DNS-1035 +label) and to a Job (its name becomes a `job-name` label value); 253 applies to +ConfigMaps and Secrets. Audit every rendered name at the longest release name +helm accepts: ```bash helm template $(python3 -c "print('n'*53)") charts/authup \ - --set server.migration.enabled=true | grep '^ name:' | awk '{print length($2), $2}' | sort -rn + --set valkey.enabled=true --set server.migration.enabled=true | python3 -c " +import sys, yaml +for d in yaml.safe_load_all(sys.stdin): + if d and d['kind'] in ('Service','Job') and len(d['metadata']['name']) > 63: + print('OVER 63:', d['kind'], d['metadata']['name']) +" ``` -The Job must be <= 63. ConfigMaps and Secrets may exceed it (253 applies), but a -Service may not. +Must print nothing. The stronger property, and the one to assert whenever the +budget in `authup.component.fullname` changes, is that **no name changes for a +release that could already install**: render every release-name length 3..53 on +both `origin/master` and the branch, and check that the two name sets differ only +at lengths where master already emitted an over-63 Service or Job. Widening the +budget silently renames resources, and a renamed `resource-policy: keep` Secret +regenerates the admin password. `useHelmHooks=false` must print the Flux/plain-helm warning in NOTES.txt, and must not print it with hooks on. NOTES is not reachable through `helm template`, diff --git a/charts/authup/Chart.yaml b/charts/authup/Chart.yaml index 56409fb..cb90326 100644 --- a/charts/authup/Chart.yaml +++ b/charts/authup/Chart.yaml @@ -44,6 +44,6 @@ annotations: - kind: changed description: useHelmHooks=false is documented as ArgoCD-only; under Flux or plain helm it makes the Job a plain resource whose immutable pod template fails the next upgrade - kind: fixed - description: The migration Job name is capped at 63 characters, the limit that applies because Kubernetes copies it into the job-name pod labels; long release names previously produced an unschedulable Job that hung the upgrade + description: Component names now derive their truncation budget from the suffix, so the 63-character limit that applies to a Service name and to a Job name is respected; release names from about 43 characters up previously rendered an admin-console Service the API server rejects, and the migration Job reached 69 characters. Only names that were already too long to exist change - kind: changed description: Setting both server.configuration and server.existingConfigmap now fails the render instead of silently dropping the inline content, which is usually where db.ssl and socketPath live diff --git a/charts/authup/templates/_helpers.tpl b/charts/authup/templates/_helpers.tpl index ad125b4..dd774e3 100644 --- a/charts/authup/templates/_helpers.tpl +++ b/charts/authup/templates/_helpers.tpl @@ -23,27 +23,50 @@ Release-scoped fully qualified name. Every resource name derives from this. {{/* Per-component names. The base is truncated BEFORE suffixing so the component -suffix always survives — otherwise a 63-char fullname would collapse every +suffix always survives: otherwise a 63-char fullname would collapse every component onto one identical name. + +The budget is DERIVED from the suffix, not hardcoded, because the ceiling that +actually bites is 63 and not the 253 a ConfigMap allows: a Service name is a +DNS-1035 label, and a Job name is copied into the job-name pod labels where a +label value stops at 63. A flat `trunc 52` let the 13-character +`-admin-console` suffix render a 66-character Service that the API server +rejects outright, so a long release name could not install at all. + +`min 52` is load-bearing, not decoration. The derived budget is WIDER than 52 +for short suffixes, and widening would RENAME resources on releases whose +fullname lands between 53 and 55 characters. A renamed Secret carrying +`helm.sh/resource-policy: keep` means the old one is orphaned and a new +admin password and system-client secret are generated: a silent credential +rotation on upgrade. The budget may therefore only ever tighten, which by +construction touches only names that are already too long to exist. + +Usage: {{ include "authup.component.fullname" (dict "context" $ "suffix" "server") }} */}} +{{- define "authup.component.fullname" -}} +{{- $suffix := .suffix -}} +{{- $budget := min 52 (sub 63 (add1 (len $suffix))) | int -}} +{{- printf "%s-%s" (include "authup.fullname" .context | trunc $budget | trimSuffix "-") $suffix -}} +{{- end -}} + {{- define "authup.server.fullname" -}} -{{- printf "%s-server" (include "authup.fullname" . | trunc 52 | trimSuffix "-") -}} +{{- include "authup.component.fullname" (dict "context" . "suffix" "server") -}} {{- end -}} {{- define "authup.adminConsole.fullname" -}} -{{- printf "%s-admin-console" (include "authup.fullname" . | trunc 52 | trimSuffix "-") -}} +{{- include "authup.component.fullname" (dict "context" . "suffix" "admin-console") -}} {{- end -}} {{- define "authup.postgresql.fullname" -}} -{{- printf "%s-postgresql" (include "authup.fullname" . | trunc 52 | trimSuffix "-") -}} +{{- include "authup.component.fullname" (dict "context" . "suffix" "postgresql") -}} {{- end -}} {{- define "authup.mysql.fullname" -}} -{{- printf "%s-mysql" (include "authup.fullname" . | trunc 52 | trimSuffix "-") -}} +{{- include "authup.component.fullname" (dict "context" . "suffix" "mysql") -}} {{- end -}} {{- define "authup.valkey.fullname" -}} -{{- printf "%s-valkey" (include "authup.fullname" . | trunc 52 | trimSuffix "-") -}} +{{- include "authup.component.fullname" (dict "context" . "suffix" "valkey") -}} {{- end -}} {{/* diff --git a/charts/authup/templates/server/migration-job.yaml b/charts/authup/templates/server/migration-job.yaml index 80cb6be..5b94308 100644 --- a/charts/authup/templates/server/migration-job.yaml +++ b/charts/authup/templates/server/migration-job.yaml @@ -10,13 +10,15 @@ against the previous release's ConfigMap. apiVersion: batch/v1 kind: Job metadata: - {{- /* Capped at 63, not the 253 a Job name would otherwise allow: with no - hand-written spec.selector the API server copies this name into the - job-name / batch.kubernetes.io/job-name pod labels, and a LABEL VALUE - stops at 63. Rule 9's trunc-52 base plus "-server" plus "-migration" - reaches 69, so a long release name would make the hook unschedulable - and hang the upgrade. */}} - name: {{ printf "%s-migration" (include "authup.server.fullname" .) | trunc 63 | trimSuffix "-" }} + {{- /* Built from the component helper with the WHOLE "server-migration" + suffix rather than by appending to authup.server.fullname, so the + 63-character budget is derived once instead of truncating a name that + was already assembled. It matters here because a Job name is copied + into the job-name / batch.kubernetes.io/job-name pod labels with no + hand-written spec.selector, and a label VALUE stops at 63: appending + to the -server name reaches 69 and the hook becomes unschedulable, + hanging the upgrade. Identical output for any normal release name. */}} + name: {{ include "authup.component.fullname" (dict "context" . "suffix" "server-migration") }} namespace: {{ include "authup.namespace" . | quote }} labels: {{- include "authup.labels" (dict "context" $ "component" "migration") | nindent 4 }} annotations: From cb7e21f4cd5c800fe85c2e868ccba96db440c754 Mon Sep 17 00:00:00 2001 From: tada5hi Date: Mon, 24 Aug 2026 09:23:33 +0200 Subject: [PATCH 5/6] fix(authup): gate the useHelmHooks warning on server.enabled, and correct three records From CodeRabbit's review on #20. The NOTES warning fired whenever server.migration.enabled was set, but the Job it warns about renders only under `server.enabled` too. A UI-only deployment carrying a leftover migration flag was told its migration Job would break on upgrade, with no migration Job in the release. Rule 10 stated its own premise too broadly: "everything the Job references must already exist from the PREVIOUS release" is contradicted two sentences later by the hook-scoped config copy, which is created in the same hook phase at a lower weight. Now scoped to non-hook resources, naming the copy as the exception. testing.md and ci/valkey-values.yaml still described CI as install-only, which the `upgrade: true` added earlier in this same branch had already made false. The layers table, the ct section and the ci comment now describe the two upgrade legs, that helm-extra-args reaches upgrades as well, and that the first leg is skipped once a release bumps the middle digit. Not changed: the `hook-succeeded` delete policy on the hook-scoped ConfigMap. The review flagged it as unsafe before helm 3.16.3, on the grounds that the ConfigMap could be deleted before the weight-0 Job runs. It cannot, in any version this chart supports. In v3.14.0, v3.15.4, v3.16.2, v3.16.3 and v3.18.4 alike, pkg/action/hooks.go runs HookSucceeded deletion in a SECOND loop after the execution loop has finished every hook in the event, under the comment "If all hooks are successful...". helm/helm#13365 (3.16.3) only reverses the iteration order WITHIN that trailing loop, so dependencies are torn down after their dependents; it does not move deletion earlier. The diff 3.16.2 -> 3.16.3 is one line: `for _, h := range executingHooks` becomes a backwards index loop. --- .agents/architecture.md | 7 +++++-- .agents/testing.md | 16 +++++++++++++--- charts/authup/ci/valkey-values.yaml | 10 +++++----- charts/authup/templates/NOTES.txt | 2 +- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/.agents/architecture.md b/.agents/architecture.md index d0aae71..da3b7c6 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -70,8 +70,11 @@ editing templates or values. `useHelmHooks=false` it renders ArgoCD `PreSync` hook annotations instead, which is an ArgoCD-only mode: see rule 19. - Helm applies a pre-upgrade hook BEFORE the release manifest, so everything - the Job references must already exist from the PREVIOUS release. Four + Helm applies a pre-upgrade hook BEFORE the release manifest, so every + NON-HOOK resource the Job references must already exist from the PREVIOUS + release. A hook resource at a lower weight is the one exception: it is + created earlier in the same hook phase, which is exactly what the config + copy below relies on. Four helpers take a `hook` flag (`secretEnv`, the two volume helpers and `configurationConfigMapName`; `configEnv` does not, it is inlined instead) and drop what `migration run` does not read. That flag is the ONE mechanism diff --git a/.agents/testing.md b/.agents/testing.md index ca6054b..9d4cd59 100644 --- a/.agents/testing.md +++ b/.agents/testing.md @@ -12,7 +12,7 @@ matrix. | Render matrix | `make template` | template errors across every `ci/*-values.yaml` | | Values coverage | `make lint-values-coverage` | `.Values.*` paths missing from values.yaml (strict-schema dead features) | | Drift gates (CI) | `make docs` / `make schema` + `git status --porcelain` | uncommitted regenerations of README.md / values.schema.json | -| ct install (CI) | kind cluster, one install per `ci/*-values.yaml` | real boot: DB provisioning, probes, migrations | +| ct install (CI) | kind cluster, per `ci/*-values.yaml`: install, plus two upgrades | real boot: DB provisioning, probes, migrations, and pre-upgrade hooks | `make test` runs lint + template + coverage locally. @@ -175,6 +175,16 @@ The generated `values.schema.json` must keep catching typos (the external-db scenario's throwaway postgres + secrets live there). - The kind job only runs when `ct list-changed` reports chart changes, so docs-only PRs stay fast. -- `--timeout 600s` accounts for first-pull of the authup image plus boot-time - migrations; server-core's startupProbe budget (60 x 5s) covers create-db + +- `upgrade: true` (in `.github/configs/ct.yaml`) is what puts the pre-upgrade + migration Job on a real cluster at all: a plain `helm install` skips + `pre-upgrade` hooks entirely, so without it the Job and its hook-scoped + ConfigMap are render-tested only. Per values file ct then runs the chart on + `master` and upgrades to this revision, then installs this revision and + upgrades it to itself. The first leg is skipped once a release bumps the + middle digit, because ct reads that as a breaking change for a 0.x chart + (`~0.x.y` constraint); the self-upgrade leg always runs. Budget roughly 3x + the install-only runtime. +- `--timeout 600s` is passed to install AND upgrade (ct hands `helm-extra-args` + to both), so it also has to cover hook execution. It accounts for first-pull + of the authup image plus boot-time migrations; server-core's startupProbe budget (60 x 5s) covers create-db + migrate + provision on first boot. diff --git a/charts/authup/ci/valkey-values.yaml b/charts/authup/ci/valkey-values.yaml index 45bb0d5..b46d4e0 100644 --- a/charts/authup/ci/valkey-values.yaml +++ b/charts/authup/ci/valkey-values.yaml @@ -8,11 +8,11 @@ server: replicaCount: 2 migration: enabled: true - # The only ci scenario setting server.configuration, so the release ConfigMap - # and the Deployment's mount get install coverage and the hook-scoped copy gets - # render coverage (ct never runs an upgrade, so no pre-upgrade hook is ever - # created on kind). Deliberately a no-op option: a db block here would fight - # the connection this scenario installs. + # The only ci scenario setting server.configuration, which with ct's + # `upgrade: true` makes the master-to-branch leg the upgrade that FIRST + # introduces the config file while the migration hook is on: the exact case + # the hook-scoped copy exists for. Deliberately a no-op option: a db block + # here would fight the connection this scenario installs. configuration: | logger: true resources: diff --git a/charts/authup/templates/NOTES.txt b/charts/authup/templates/NOTES.txt index a3e8c48..6dc0add 100644 --- a/charts/authup/templates/NOTES.txt +++ b/charts/authup/templates/NOTES.txt @@ -84,7 +84,7 @@ schema migrations are serialized in a pre-upgrade Job instead of racing at pod boot. {{- end }} -{{- if and .Values.server.migration.enabled (not .Values.useHelmHooks) }} +{{- if and .Values.server.enabled .Values.server.migration.enabled (not .Values.useHelmHooks) }} WARNING: useHelmHooks=false renders the migration Job as a plain resource carrying ArgoCD PreSync annotations, which only ArgoCD reads. Under Flux or From 338bc79d55f7c81cf89cb909a52a68c9fcef7c0b Mon Sep 17 00:00:00 2001 From: tada5hi Date: Mon, 24 Aug 2026 09:27:34 +0200 Subject: [PATCH 6/6] docs(authup): record the component-name budget change in BREAKING.md The naming change ships as fix(authup)!, so the ledger has to carry it. Nothing to migrate: every name that moves belongs to a release the API server already refused, because the admin-console Service exceeded 63 characters. --- charts/authup/BREAKING.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/charts/authup/BREAKING.md b/charts/authup/BREAKING.md index 1fb757d..180d316 100644 --- a/charts/authup/BREAKING.md +++ b/charts/authup/BREAKING.md @@ -5,6 +5,16 @@ land on the middle digit. Every entry lists the value migrations required. ## Next release (unreleased) +- Component resource names are truncated on a budget derived from their suffix + (`min 52 (63 - len(suffix) - 1)`) rather than a flat `trunc 52`, so the + 63-character limit that applies to a Service name and to a Job name is + respected. Only names that were already too long to exist change: with a + release name from roughly 43 characters up, the admin-console Service was 66 + characters and the API server rejected it, so the release could not install at + all; the migration Job reached 69. Nothing to migrate, since no cluster can + hold a release in that range. Verified by rendering every release-name length + from 3 to 53 against the previous revision: the name sets differ at no length + where the old chart was installable. - Setting BOTH `server.configuration` and `server.existingConfigmap` now fails the render. It never worked: the existing ConfigMap is the one that gets mounted, so the inline content was silently dropped, and that content is