From dfefd80c04f3a5cffdfbfb1eb77b6abfe7c49862 Mon Sep 17 00:00:00 2001 From: tada5hi Date: Tue, 8 Sep 2026 19:53:36 +0200 Subject: [PATCH 1/2] fix(authup): order the migration Job after its own inputs under ArgoCD useHelmHooks=false rendered the migration Job as an ArgoCD PreSync hook, which runs before every Sync-phase resource. On a fresh install with a built-in database, that included the database itself: the Job exhausted backoffLimit against a database that could never appear, PreSync failed, and the Sync phase that would create the database never started. Move the Job (and its hook-scoped ConfigMap and NetworkPolicy) into the Sync phase and order everything with sync-wave instead of by phase. The built-in database, the ServiceAccount, and the auth/external-db Secrets the Job's pod spec can reference all render at wave -10; the hook-scoped ConfigMap and NetworkPolicy at -5; the Job at -1; the server Deployment keeps its implicit wave 0. A ServiceAccount or Secret missing when a Job pod is admitted is a pod-creation failure that never counts toward backoffLimit, so every one of the Job's own inputs has to precede it, not just the database. commonAnnotations setting the same sync-wave key under useHelmHooks=false now fails the render instead of silently colliding: only the resources routed through the annotations helper would honor it, while the Job, ConfigMap and NetworkPolicy keep their own fixed waves, inverting the ordering this fix relies on. useHelmHooks=true (plain Helm, Flux) is unchanged. Fixes #30 --- .agents/architecture.md | 13 +- .agents/testing.md | 13 +- DESIGN.md | 19 ++- charts/authup/Chart.yaml | 2 + charts/authup/README.md | 4 +- charts/authup/templates/NOTES.txt | 6 +- charts/authup/templates/_helpers.tpl | 3 + charts/authup/templates/mysql/secret.yaml | 2 +- charts/authup/templates/mysql/service.yaml | 2 +- .../authup/templates/mysql/statefulset.yaml | 2 +- .../authup/templates/postgresql/secret.yaml | 2 +- .../authup/templates/postgresql/service.yaml | 2 +- .../templates/postgresql/statefulset.yaml | 2 +- charts/authup/templates/secret-db.yaml | 6 +- charts/authup/templates/secret.yaml | 8 +- .../configmap-migration-configuration.yaml | 6 +- .../authup/templates/server/deployment.yaml | 5 +- .../templates/server/migration-job.yaml | 15 +- .../server/migration-networkpolicy.yaml | 2 +- charts/authup/templates/serviceaccount.yaml | 10 +- charts/authup/templates/validations.yaml | 14 ++ charts/authup/values.schema.json | 4 +- charts/authup/values.yaml | 13 +- scripts/check-beta64-contract.py | 128 +++++++++++++++++- 24 files changed, 243 insertions(+), 40 deletions(-) diff --git a/.agents/architecture.md b/.agents/architecture.md index c1f9596..c6bd3d0 100644 --- a/.agents/architecture.md +++ b/.agents/architecture.md @@ -98,10 +98,15 @@ operational invariants that template changes must preserve. inlines non-secret config, narrows secrets to database password and optional encryption key, skips provisioning, and mounts a hook-scoped copy of `authup.yml`. The configuration ConfigMap and migration NetworkPolicy have - weight -5; the Job has weight 0. -22. **`useHelmHooks=false` is ArgoCD-only.** It emits PreSync resources. Flux - and plain Helm need native hooks or they apply an immutable Job as a normal - resource without correct ordering. + Helm hook-weight -5; the Job has weight 0. Under `useHelmHooks=false` this + extends to every object the Job's pod spec can reference: the built-in + database, the ServiceAccount, and the auth/external-db Secrets all render at + sync-wave -10 so the Job (wave -1) never waits on a resource ArgoCD hasn't + created yet, which for a Job (bounded `backoffLimit`, no self-healing retry + like a Deployment) is a deadlock, not a slow start (issue #30). +22. **`useHelmHooks=false` is ArgoCD-only.** It emits Sync-phase resources + ordered by sync-wave, not PreSync. Flux and plain Helm need native hooks or + they apply an immutable Job as a normal resource without correct ordering. 23. **Checksum annotations follow every consumed input.** Deployments roll on chart-managed env, Secret, provisioning, configuration and theme changes. `disableRestartOnChanges` is the explicit escape hatch. diff --git a/.agents/testing.md b/.agents/testing.md index e516832..c889895 100644 --- a/.agents/testing.md +++ b/.agents/testing.md @@ -48,6 +48,9 @@ and cache, plus restrictive NetworkPolicies. - non-empty `server.features.accountConsole`, which moved to `accountConsole.enabled` - invalid theme manifests or dangerous trusted-origin globstars +- `commonAnnotations` setting `argocd.argoproj.io/sync-wave` under + `useHelmHooks=false`, which would collide with the chart's own wave ordering + on only some of the affected resources The beta.64 contract script exercises the moved value, split dependencies, route flags and reserved role env variables directly. @@ -62,12 +65,16 @@ The pre-upgrade migration Job must stay narrower than the server Deployment: - `authup.yml` comes from the hook-scoped configuration ConfigMap - logs mount at `/var/log/authup` - the migration NetworkPolicy selects component `migration`, uses the same hook - family, and runs at weight or wave -5 before the Job at 0 + family, and runs at Helm hook-weight -5 before the Job at 0, or ArgoCD + sync-wave -5 before the Job at -1 +- under `useHelmHooks=false`, the built-in database, the ServiceAccount and the + auth/external-db Secrets all render at sync-wave -10, strictly before the + Job's wave -1 (issue #30: a PreSync Job used to run before all of these) - fresh-install server env has no `MIGRATION_ENABLED`; upgrade server env has `MIGRATION_ENABLED=false` when the Job is enabled and the database persists, but leaves startup migration enabled for non-persistent built-in databases; - with `useHelmHooks=false` every render counts as an upgrade because PreSync - precedes each sync + with `useHelmHooks=false` every render counts as an upgrade because the Job + is a hook on every sync, not just the first Run both Helm and ArgoCD annotation paths: diff --git a/DESIGN.md b/DESIGN.md index 640934c..d72ce75 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -180,11 +180,20 @@ Helm creates hooks before regular release resources. The Job therefore: - mounts a hook-scoped copy of `authup.yml` - mounts `/var/log/authup` -The hook configuration ConfigMap and migration NetworkPolicy run at weight -5; -the Job runs at 0. This ensures configuration and egress policy exist before the -pod. `useHelmHooks=false` emits ArgoCD PreSync annotations. It is not a Flux or -plain-Helm mode because a normal Job has immutable pod templates and no correct -upgrade ordering. +The hook configuration ConfigMap and migration NetworkPolicy run at Helm +hook-weight -5; the Job runs at 0. This ensures configuration and egress policy +exist before the pod. `useHelmHooks=false` emits ArgoCD annotations instead. It +is not a Flux or plain-Helm mode because a normal Job has immutable pod +templates and no correct upgrade ordering. + +Under ArgoCD the Job is a Sync-phase hook (not PreSync), at sync-wave -1: a +PreSync hook runs before every Sync-phase resource, including the built-in +database, which deadlocked a fresh install (issue #30). Everything the Job's +pod spec can reference (the built-in database, the ServiceAccount, and the +auth/external-db Secrets when they carry values the Job needs) renders at wave +-10; the hook-scoped ConfigMap and NetworkPolicy at -5. ArgoCD waits for each +wave to be healthy before starting the next, so the Job always runs after its +own inputs exist, and still before the server Deployment (implicit wave 0). ## 8. Network policy diff --git a/charts/authup/Chart.yaml b/charts/authup/Chart.yaml index 5f22c73..e7a8b56 100644 --- a/charts/authup/Chart.yaml +++ b/charts/authup/Chart.yaml @@ -33,3 +33,5 @@ annotations: artifacthub.io/changes: | - kind: changed description: Track Authup v1.0.0-beta.65; no image entrypoint, CLI or environment-variable contract changes + - kind: fixed + description: Fix ArgoCD first-sync deadlock when server.migration.enabled and a built-in database are both enabled with useHelmHooks=false diff --git a/charts/authup/README.md b/charts/authup/README.md index 982cda0..077cc5e 100644 --- a/charts/authup/README.md +++ b/charts/authup/README.md @@ -609,7 +609,7 @@ Kubernetes: `>=1.25.0-0` | server.mfa.enabled | bool | `false` | Enable multi-factor authentication (MFA_ENABLED) | | server.mfa.required | bool | `false` | Require MFA for every user (MFA_REQUIRED; needs mfa.enabled) | | server.migration.backoffLimit | int | `3` | Job backoff limit | -| server.migration.enabled | bool | `false` | Run `migration run` as a pre-upgrade hook Job. Recommended for multi-replica deployments (serializes DDL before pods roll). Fresh installs and non-persistent built-in databases migrate at boot regardless. Under useHelmHooks=false the Job is a PreSync hook on the first sync too, so with a built-in database enable it only after that sync | +| server.migration.enabled | bool | `false` | Run `migration run` as a pre-upgrade hook Job. Recommended for multi-replica deployments (serializes DDL before pods roll). Fresh installs and non-persistent built-in databases migrate at boot regardless. Under useHelmHooks=false the Job runs as an ArgoCD Sync-phase hook ordered after the built-in database by sync-wave, so it is safe from the first sync | | server.migration.podAnnotations | object | `{}` | Job pod annotations | | server.migration.resources | object | `{}` | Job resources ({} = server resources defaults) | | server.migration.ttlSecondsAfterFinished | int | `300` | Delete the Job this many seconds after it finishes ("" = keep) | @@ -694,7 +694,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 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 | bool | `true` | Render Helm hook annotations on the migration Job. Set false only for ArgoCD, which reads its own Sync-phase, sync-wave annotations instead. ArgoCD also understands Helm hooks, but maps pre-upgrade to an unordered PreSync hook: with server.migration.enabled and a built-in database, set false so the Job waits for the database instead of deadlocking the first sync. 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/templates/NOTES.txt b/charts/authup/templates/NOTES.txt index 63d3a18..d0fce73 100644 --- a/charts/authup/templates/NOTES.txt +++ b/charts/authup/templates/NOTES.txt @@ -85,9 +85,9 @@ migrations are serialized before pods roll. {{- if and .Values.server.enabled .Values.server.migration.enabled (not .Values.useHelmHooks) }} -WARNING: useHelmHooks=false is for ArgoCD PreSync only. Flux and plain Helm -apply the Job as a normal immutable resource, so later upgrades can fail. Use -useHelmHooks=true outside ArgoCD. +WARNING: useHelmHooks=false renders ArgoCD-specific Sync-phase hook +annotations only. Flux and plain Helm apply the Job as a normal immutable +resource, so later upgrades can fail. Use useHelmHooks=true outside ArgoCD. {{- end }} {{- if .Values.server.provisioning.enabled }} diff --git a/charts/authup/templates/_helpers.tpl b/charts/authup/templates/_helpers.tpl index 17d516e..45f4d13 100644 --- a/charts/authup/templates/_helpers.tpl +++ b/charts/authup/templates/_helpers.tpl @@ -168,6 +168,9 @@ Common annotations applied to every object. {{- if .context.Values.commonAnnotations -}} {{ include "authup.tplvalues.render" (dict "value" .context.Values.commonAnnotations "context" .context) }} {{- end -}} +{{- if and .syncWave (not .context.Values.useHelmHooks) }} +argocd.argoproj.io/sync-wave: {{ .syncWave | quote }} +{{- end -}} {{- end -}} {{/* diff --git a/charts/authup/templates/mysql/secret.yaml b/charts/authup/templates/mysql/secret.yaml index bc56e9c..1803dfc 100644 --- a/charts/authup/templates/mysql/secret.yaml +++ b/charts/authup/templates/mysql/secret.yaml @@ -8,7 +8,7 @@ metadata: labels: {{- include "authup.labels" (dict "context" $ "component" "mysql") | nindent 4 }} annotations: helm.sh/resource-policy: keep - {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} type: Opaque data: password: {{ include "authup.secret.rawValue" (dict "secret" $secretName "key" "password" "value" .Values.mysql.auth.password "length" 32 "context" $) | b64enc | quote }} diff --git a/charts/authup/templates/mysql/service.yaml b/charts/authup/templates/mysql/service.yaml index 62e4793..ede60ab 100644 --- a/charts/authup/templates/mysql/service.yaml +++ b/charts/authup/templates/mysql/service.yaml @@ -5,7 +5,7 @@ metadata: name: {{ include "authup.mysql.fullname" . }} namespace: {{ include "authup.namespace" . | quote }} labels: {{- include "authup.labels" (dict "context" $ "component" "mysql") | nindent 4 }} - annotations: {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + annotations: {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} spec: type: ClusterIP ports: diff --git a/charts/authup/templates/mysql/statefulset.yaml b/charts/authup/templates/mysql/statefulset.yaml index 3e8c536..70fea7e 100644 --- a/charts/authup/templates/mysql/statefulset.yaml +++ b/charts/authup/templates/mysql/statefulset.yaml @@ -10,7 +10,7 @@ metadata: name: {{ include "authup.mysql.fullname" . }} namespace: {{ include "authup.namespace" . | quote }} labels: {{- include "authup.labels" (dict "context" $ "component" "mysql") | nindent 4 }} - annotations: {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + annotations: {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} spec: serviceName: {{ include "authup.mysql.fullname" . }} replicas: 1 diff --git a/charts/authup/templates/postgresql/secret.yaml b/charts/authup/templates/postgresql/secret.yaml index 6777dc1..d73965c 100644 --- a/charts/authup/templates/postgresql/secret.yaml +++ b/charts/authup/templates/postgresql/secret.yaml @@ -8,7 +8,7 @@ metadata: labels: {{- include "authup.labels" (dict "context" $ "component" "postgresql") | nindent 4 }} annotations: helm.sh/resource-policy: keep - {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} type: Opaque data: password: {{ include "authup.secret.rawValue" (dict "secret" $secretName "key" "password" "value" .Values.postgresql.auth.password "length" 32 "context" $) | b64enc | quote }} diff --git a/charts/authup/templates/postgresql/service.yaml b/charts/authup/templates/postgresql/service.yaml index 20a1ee9..7d2c1c7 100644 --- a/charts/authup/templates/postgresql/service.yaml +++ b/charts/authup/templates/postgresql/service.yaml @@ -5,7 +5,7 @@ metadata: name: {{ include "authup.postgresql.fullname" . }} namespace: {{ include "authup.namespace" . | quote }} labels: {{- include "authup.labels" (dict "context" $ "component" "postgresql") | nindent 4 }} - annotations: {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + annotations: {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} spec: type: ClusterIP ports: diff --git a/charts/authup/templates/postgresql/statefulset.yaml b/charts/authup/templates/postgresql/statefulset.yaml index fe00b54..42a884a 100644 --- a/charts/authup/templates/postgresql/statefulset.yaml +++ b/charts/authup/templates/postgresql/statefulset.yaml @@ -10,7 +10,7 @@ metadata: name: {{ include "authup.postgresql.fullname" . }} namespace: {{ include "authup.namespace" . | quote }} labels: {{- include "authup.labels" (dict "context" $ "component" "postgresql") | nindent 4 }} - annotations: {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + annotations: {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} spec: serviceName: {{ include "authup.postgresql.fullname" . }} replicas: 1 diff --git a/charts/authup/templates/secret-db.yaml b/charts/authup/templates/secret-db.yaml index c3e4eb6..a38310b 100644 --- a/charts/authup/templates/secret-db.yaml +++ b/charts/authup/templates/secret-db.yaml @@ -4,6 +4,10 @@ inline password still lands in a chart-managed Secret — never in a pod env literal. Deliberately no generation fallback: validations.yaml fails when neither password nor existingSecret is given (the chart will not invent a password for a database it does not manage). + +The migration Job mounts this Secret's password too. Under useHelmHooks=false +it renders at sync-wave -10, same as the built-in database, so it exists +before the Job's wave -1. */}} {{- if and (not .Values.postgresql.enabled) (not .Values.mysql.enabled) .Values.externalDatabase.host .Values.externalDatabase.password (not .Values.externalDatabase.existingSecret) }} {{- $secretName := printf "%s-externaldb" (include "authup.fullname" .) }} @@ -15,7 +19,7 @@ metadata: labels: {{- include "authup.labels" (dict "context" $) | nindent 4 }} annotations: helm.sh/resource-policy: keep - {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} type: Opaque data: password: {{ .Values.externalDatabase.password | b64enc | quote }} diff --git a/charts/authup/templates/secret.yaml b/charts/authup/templates/secret.yaml index b3656ed..d1b1e56 100644 --- a/charts/authup/templates/secret.yaml +++ b/charts/authup/templates/secret.yaml @@ -1,4 +1,10 @@ {{- if include "authup.auth.createSecret" . }} +{{/* +When auth.secretsEncryptionKey is set inline, the migration Job mounts +SECRETS_ENCRYPTION_KEY from this Secret. Under useHelmHooks=false it renders +at sync-wave -10, same as the built-in database, so it exists before the Job's +wave -1. +*/}} {{- $secretName := include "authup.fullname" . }} apiVersion: v1 kind: Secret @@ -8,7 +14,7 @@ metadata: labels: {{- include "authup.labels" (dict "context" $) | nindent 4 }} annotations: helm.sh/resource-policy: keep - {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} type: Opaque data: {{ .Values.auth.secretKeys.adminPasswordKey }}: {{ include "authup.secret.rawValue" (dict "secret" $secretName "key" .Values.auth.secretKeys.adminPasswordKey "value" .Values.auth.adminPassword "length" 32 "context" $) | b64enc | quote }} diff --git a/charts/authup/templates/server/configmap-migration-configuration.yaml b/charts/authup/templates/server/configmap-migration-configuration.yaml index 1cd22dc..c54dd89 100644 --- a/charts/authup/templates/server/configmap-migration-configuration.yaml +++ b/charts/authup/templates/server/configmap-migration-configuration.yaml @@ -14,7 +14,9 @@ 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. +Job); the ArgoCD branch mirrors that with sync-wave -5, as a Sync-phase hook +rather than a PreSync one, so it also runs after the built-in database's wave +-10 and still ahead of the Job at -1. */}} apiVersion: v1 kind: ConfigMap @@ -29,7 +31,7 @@ metadata: 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: Sync argocd.argoproj.io/hook-delete-policy: BeforeHookCreation argocd.argoproj.io/sync-wave: "-5" {{- end }} diff --git a/charts/authup/templates/server/deployment.yaml b/charts/authup/templates/server/deployment.yaml index d886583..e10c821 100644 --- a/charts/authup/templates/server/deployment.yaml +++ b/charts/authup/templates/server/deployment.yaml @@ -129,8 +129,9 @@ spec: value: "false" {{- end }} {{- /* The Job precedes the pods on every helm upgrade and on every ArgoCD - sync (PreSync). Only a database that survives the rollout keeps the - Job's work, so ephemeral built-in stores keep boot migration on. */}} + sync (a lower sync-wave). Only a database that survives the rollout + keeps the Job's work, so ephemeral built-in stores keep boot + migration on. */}} {{- $dbPersists := or (not (or .Values.postgresql.enabled .Values.mysql.enabled)) (and .Values.postgresql.enabled (or .Values.postgresql.persistence.enabled .Values.postgresql.persistence.existingClaim)) (and .Values.mysql.enabled (or .Values.mysql.persistence.enabled .Values.mysql.persistence.existingClaim)) }} {{- if and .Values.server.migration.enabled (or .Release.IsUpgrade (not .Values.useHelmHooks)) $dbPersists }} - name: MIGRATION_ENABLED diff --git a/charts/authup/templates/server/migration-job.yaml b/charts/authup/templates/server/migration-job.yaml index 3e800eb..a3aba65 100644 --- a/charts/authup/templates/server/migration-job.yaml +++ b/charts/authup/templates/server/migration-job.yaml @@ -6,6 +6,18 @@ migrates at boot anyway. On upgrades the Job serializes DDL before new pods roll, which matters for multi-replica deployments (MySQL DDL is not transactional). The env ConfigMap content is INLINED so the hook never runs against the previous release's ConfigMap. + +ArgoCD has no pre-upgrade concept, so useHelmHooks=false hits the same +fresh-install problem a different way: a PreSync hook runs before every +Sync-phase resource, including the built-in database, so the Job used to fail +against a database that could never appear. The fix keeps the Job a hook (for +BeforeHookCreation delete-and-recreate on every sync) but moves it into the +Sync phase at sync-wave -1, ordered by wave instead of by phase. Everything +the Job's pod spec can reference (the built-in database, the ServiceAccount, +and the auth/external-db Secrets when they carry the values it needs) renders +at wave -10; the hook-scoped ConfigMap and NetworkPolicy at -5; so the Job +always runs after all of its own inputs exist and are healthy, and still +before the server Deployment (implicit wave 0). */}} apiVersion: batch/v1 kind: Job @@ -28,8 +40,9 @@ metadata: helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded helm.sh/hook-weight: "0" {{- else }} - argocd.argoproj.io/hook: PreSync + argocd.argoproj.io/hook: Sync argocd.argoproj.io/hook-delete-policy: BeforeHookCreation + argocd.argoproj.io/sync-wave: "-1" {{- end }} spec: backoffLimit: {{ .Values.server.migration.backoffLimit }} diff --git a/charts/authup/templates/server/migration-networkpolicy.yaml b/charts/authup/templates/server/migration-networkpolicy.yaml index 9fb0010..7b28a9e 100644 --- a/charts/authup/templates/server/migration-networkpolicy.yaml +++ b/charts/authup/templates/server/migration-networkpolicy.yaml @@ -14,7 +14,7 @@ metadata: helm.sh/hook-delete-policy: before-hook-creation helm.sh/hook-weight: "-5" {{- else }} - argocd.argoproj.io/hook: PreSync + argocd.argoproj.io/hook: Sync argocd.argoproj.io/hook-delete-policy: BeforeHookCreation argocd.argoproj.io/sync-wave: "-5" {{- end }} diff --git a/charts/authup/templates/serviceaccount.yaml b/charts/authup/templates/serviceaccount.yaml index dc2a5f8..c37bb37 100644 --- a/charts/authup/templates/serviceaccount.yaml +++ b/charts/authup/templates/serviceaccount.yaml @@ -1,4 +1,12 @@ {{- if .Values.serviceAccount.create }} +{{/* +The migration Job (server/migration-job.yaml) always runs as this +ServiceAccount. Under useHelmHooks=false it renders at sync-wave -10, same as +the built-in database, so it exists before the Job's wave -1: a ServiceAccount +missing when a pod is admitted is a pod-creation failure, which never counts +toward the Job's backoffLimit and would hang the sync forever instead of +failing loudly. +*/}} apiVersion: v1 kind: ServiceAccount metadata: @@ -6,7 +14,7 @@ metadata: namespace: {{ include "authup.namespace" . | quote }} labels: {{- include "authup.labels" (dict "context" $) | nindent 4 }} annotations: - {{- include "authup.annotations" (dict "context" $) | nindent 4 }} + {{- include "authup.annotations" (dict "context" $ "syncWave" "-10") | nindent 4 }} {{- if .Values.serviceAccount.annotations }} {{- include "authup.tplvalues.render" (dict "value" .Values.serviceAccount.annotations "context" $) | nindent 4 }} {{- end }} diff --git a/charts/authup/templates/validations.yaml b/charts/authup/templates/validations.yaml index 766b89b..8ae76c9 100644 --- a/charts/authup/templates/validations.yaml +++ b/charts/authup/templates/validations.yaml @@ -160,6 +160,20 @@ serves at "/" and expects the proxy to strip the prefix, hence match AND rewrite {{- fail "authup: auth.secretsEncryptionKeyEnabled requires the key to come from somewhere — set auth.secretsEncryptionKey or reference it via auth.existingSecret." }} {{- end }} +{{/* +Under useHelmHooks=false the chart sets argocd.argoproj.io/sync-wave itself on the +built-in database, the ServiceAccount and the migration Job's Secrets, to keep the +Job ordered after all of them (issue #30). Only those resources route their wave +through the authup.annotations helper; the migration Job, its ConfigMap and its +NetworkPolicy carry their own fixed waves as template literals. A user-supplied +commonAnnotations value for the same key would collide with only the first group, +inverting the ordering instead of overriding it everywhere, so it is rejected +outright rather than silently mishandled. +*/}} +{{- if and (not .Values.useHelmHooks) (hasKey (.Values.commonAnnotations | default dict) "argocd.argoproj.io/sync-wave") }} +{{- fail "authup: commonAnnotations must not set argocd.argoproj.io/sync-wave when useHelmHooks=false. The chart manages that annotation itself to keep the migration Job ordered after the built-in database, the ServiceAccount and its Secrets." }} +{{- end }} + {{/* Deprecation tripwires: when a value moves, add a guard here that names the new location, e.g. diff --git a/charts/authup/values.schema.json b/charts/authup/values.schema.json index 0ea01f6..c44c84c 100644 --- a/charts/authup/values.schema.json +++ b/charts/authup/values.schema.json @@ -4712,7 +4712,7 @@ }, "enabled": { "default": false, - "description": "Run `migration run` as a pre-upgrade hook Job. Recommended\nfor multi-replica deployments (serializes DDL before pods roll). Fresh\ninstalls and non-persistent built-in databases migrate at boot regardless.\nUnder useHelmHooks=false the Job is a PreSync hook on the first sync too,\nso with a built-in database enable it only after that sync", + "description": "Run `migration run` as a pre-upgrade hook Job. Recommended\nfor multi-replica deployments (serializes DDL before pods roll). Fresh\ninstalls and non-persistent built-in databases migrate at boot regardless.\nUnder useHelmHooks=false the Job runs as an ArgoCD Sync-phase hook ordered\nafter the built-in database by sync-wave, so it is safe from the first sync", "required": [], "title": "enabled", "type": "boolean" @@ -5594,7 +5594,7 @@ }, "useHelmHooks": { "default": true, - "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.", + "description": "Render Helm hook annotations on the migration Job. Set false only for\nArgoCD, which reads its own Sync-phase, sync-wave annotations instead.\nArgoCD also understands Helm hooks, but maps pre-upgrade to an unordered\nPreSync hook: with server.migration.enabled and a built-in database, set\nfalse so the Job waits for the database instead of deadlocking the first\nsync. Flux and plain helm need true: a plain Job's pod template is\nimmutable, 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 8234fd2..f4ea9ee 100644 --- a/charts/authup/values.yaml +++ b/charts/authup/values.yaml @@ -35,9 +35,12 @@ commonAnnotations: {} # -- Extra objects to deploy (rendered through tpl; list of manifests or strings) extraDeploy: [] # -- 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. +# ArgoCD, which reads its own Sync-phase, sync-wave annotations instead. +# ArgoCD also understands Helm hooks, but maps pre-upgrade to an unordered +# PreSync hook: with server.migration.enabled and a built-in database, set +# false so the Job waits for the database instead of deadlocking the first +# sync. 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) @@ -529,8 +532,8 @@ server: # -- Run `migration run` as a pre-upgrade hook Job. Recommended # for multi-replica deployments (serializes DDL before pods roll). Fresh # installs and non-persistent built-in databases migrate at boot regardless. - # Under useHelmHooks=false the Job is a PreSync hook on the first sync too, - # so with a built-in database enable it only after that sync + # Under useHelmHooks=false the Job runs as an ArgoCD Sync-phase hook ordered + # after the built-in database by sync-wave, so it is safe from the first sync enabled: false # -- Job backoff limit backoffLimit: 3 diff --git a/scripts/check-beta64-contract.py b/scripts/check-beta64-contract.py index 4a74782..c74606b 100644 --- a/scripts/check-beta64-contract.py +++ b/scripts/check-beta64-contract.py @@ -407,10 +407,129 @@ def check_policy(): ) argocd = one(argocd_documents, "NetworkPolicy", "migration") annotations = argocd["metadata"]["annotations"] - assert annotations["argocd.argoproj.io/hook"] == "PreSync" + assert annotations["argocd.argoproj.io/hook"] == "Sync" assert annotations["argocd.argoproj.io/sync-wave"] == "-5" assert "helm.sh/hook" not in annotations + # Regression guard for #30: under useHelmHooks=false the migration Job must not be + # a PreSync hook (it would then run before the built-in database exists on the first + # sync and deadlock the app). It runs as a Sync-phase hook, ordered after the database + # and the hook-scoped config by sync-wave, and still ahead of the server Deployment's + # implicit wave 0. + wave_documents = render( + chart / "ci" / "valkey-values.yaml", + "--set", + "useHelmHooks=false", + "--set", + "server.networkPolicy.enabled=true", + ) + + # The migration Job's pod spec always references the ServiceAccount (via + # serviceAccountName) and the built-in database. Both must be healthy before + # the Job's wave, or ArgoCD hangs on a pod-admission failure that never + # counts toward the Job's backoffLimit (a silent deadlock, not a fast one). + db_wave = None + for kind, component in ( + ("Secret", "postgresql"), + ("StatefulSet", "postgresql"), + ("Service", "postgresql"), + ("ServiceAccount", None), + ): + resource = one(wave_documents, kind, component) + resource_annotations = resource["metadata"]["annotations"] + assert "argocd.argoproj.io/hook" not in resource_annotations + wave = int(resource_annotations["argocd.argoproj.io/sync-wave"]) + assert db_wave is None or wave == db_wave + db_wave = wave + + configmap = one(wave_documents, "ConfigMap", "migration") + configmap_annotations = configmap["metadata"]["annotations"] + assert configmap_annotations["argocd.argoproj.io/hook"] == "Sync" + cm_wave = int(configmap_annotations["argocd.argoproj.io/sync-wave"]) + + wave_netpol = one(wave_documents, "NetworkPolicy", "migration") + wave_netpol_annotations = wave_netpol["metadata"]["annotations"] + assert wave_netpol_annotations["argocd.argoproj.io/hook"] == "Sync" + assert int(wave_netpol_annotations["argocd.argoproj.io/sync-wave"]) == cm_wave + + job = one(wave_documents, "Job", "migration") + job_annotations = job["metadata"]["annotations"] + assert job_annotations["argocd.argoproj.io/hook"] == "Sync" + assert "helm.sh/hook" not in job_annotations + job_wave = int(job_annotations["argocd.argoproj.io/sync-wave"]) + + assert db_wave < cm_wave < job_wave < 0, ( + f"expected db wave < config wave < job wave < 0, got " + f"{db_wave}, {cm_wave}, {job_wave}" + ) + + wave_server = one(wave_documents, "Deployment", "server") + assert "argocd.argoproj.io/sync-wave" not in ( + wave_server["metadata"].get("annotations") or {} + ) + + # MySQL gets the same wave treatment as the postgresql fixture above. + mysql_documents = render( + chart / "ci" / "mysql-values.yaml", + "--set", + "useHelmHooks=false", + "--set", + "server.migration.enabled=true", + ) + mysql_job_wave = int( + one(mysql_documents, "Job", "migration")["metadata"]["annotations"][ + "argocd.argoproj.io/sync-wave" + ] + ) + for kind in ("Secret", "StatefulSet", "Service"): + resource_annotations = one(mysql_documents, kind, "mysql")["metadata"]["annotations"] + assert "argocd.argoproj.io/hook" not in resource_annotations + assert int(resource_annotations["argocd.argoproj.io/sync-wave"]) < mysql_job_wave + + # Plain Helm (useHelmHooks=true, the default) must render no ArgoCD annotations on + # the built-in database at all. + helm_wave_documents = render(chart / "ci" / "valkey-values.yaml") + helm_db_statefulset = one(helm_wave_documents, "StatefulSet", "postgresql") + assert "argocd.argoproj.io/sync-wave" not in ( + helm_db_statefulset["metadata"].get("annotations") or {} + ) + + # The Job's other two possible Secret inputs (external database with an inline + # password, and the auth Secret when secretsEncryptionKey is set inline) also + # need to precede it, even with no built-in database at all. + externaldb_documents = render( + { + "postgresql": {"enabled": False}, + "externalDatabase": {"host": "db.example.com", "password": "pw"}, + "server": {"migration": {"enabled": True}}, + "useHelmHooks": False, + } + ) + externaldb_secret = one(externaldb_documents, "Secret", None, suffix="-externaldb") + assert int( + externaldb_secret["metadata"]["annotations"]["argocd.argoproj.io/sync-wave"] + ) < int( + one(externaldb_documents, "Job", "migration")["metadata"]["annotations"][ + "argocd.argoproj.io/sync-wave" + ] + ) + + kek_documents = render( + chart / "ci" / "valkey-values.yaml", + "--set", + "useHelmHooks=false", + "--set", + "auth.secretsEncryptionKeyEnabled=true", + "--set", + "auth.secretsEncryptionKey=abcdefgh12345678", + ) + kek_secret = one(kek_documents, "Secret", None) + assert int(kek_secret["metadata"]["annotations"]["argocd.argoproj.io/sync-wave"]) < int( + one(kek_documents, "Job", "migration")["metadata"]["annotations"][ + "argocd.argoproj.io/sync-wave" + ] + ) + def check_validations(): render( @@ -587,6 +706,13 @@ def check_validations(): {"server": {"config": {name: "false"}}}, f"server.config.{name} collides with a first-class chart value", ) + render_fails( + { + "useHelmHooks": False, + "commonAnnotations": {"argocd.argoproj.io/sync-wave": "5"}, + }, + "commonAnnotations must not set argocd.argoproj.io/sync-wave", + ) checks = { From 681a9df37b9fb6ce7df103255afd2ccd8a4c4e54 Mon Sep 17 00:00:00 2001 From: tada5hi Date: Tue, 8 Sep 2026 21:42:52 +0200 Subject: [PATCH 2/2] fix(authup): also reject the reserved sync-wave key in serviceAccount.annotations serviceAccount.annotations is a second collision surface for the reserved argocd.argoproj.io/sync-wave key, distinct from commonAnnotations: a user-supplied value there silently overrides the chart's own "-10" on the ServiceAccount while the migration Job keeps its fixed wave -1, reopening the #30 deadlock. Reject it the same way, gated on serviceAccount.create since the value is otherwise unused. Found by CodeRabbit review on PR #34. --- .agents/testing.md | 5 +++-- charts/authup/templates/validations.yaml | 9 ++++++--- scripts/check-beta64-contract.py | 7 +++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/.agents/testing.md b/.agents/testing.md index c889895..43803da 100644 --- a/.agents/testing.md +++ b/.agents/testing.md @@ -48,8 +48,9 @@ and cache, plus restrictive NetworkPolicies. - non-empty `server.features.accountConsole`, which moved to `accountConsole.enabled` - invalid theme manifests or dangerous trusted-origin globstars -- `commonAnnotations` setting `argocd.argoproj.io/sync-wave` under - `useHelmHooks=false`, which would collide with the chart's own wave ordering +- `commonAnnotations` or `serviceAccount.annotations` setting + `argocd.argoproj.io/sync-wave` under `useHelmHooks=false`, which would + collide with the chart's own wave ordering on only some of the affected resources The beta.64 contract script exercises the moved value, split dependencies, diff --git a/charts/authup/templates/validations.yaml b/charts/authup/templates/validations.yaml index 8ae76c9..a9d3656 100644 --- a/charts/authup/templates/validations.yaml +++ b/charts/authup/templates/validations.yaml @@ -166,13 +166,16 @@ built-in database, the ServiceAccount and the migration Job's Secrets, to keep t Job ordered after all of them (issue #30). Only those resources route their wave through the authup.annotations helper; the migration Job, its ConfigMap and its NetworkPolicy carry their own fixed waves as template literals. A user-supplied -commonAnnotations value for the same key would collide with only the first group, -inverting the ordering instead of overriding it everywhere, so it is rejected -outright rather than silently mishandled. +commonAnnotations or serviceAccount.annotations value for the same key would +collide with only the first group, inverting the ordering instead of overriding +it everywhere, so both are rejected outright rather than silently mishandled. */}} {{- if and (not .Values.useHelmHooks) (hasKey (.Values.commonAnnotations | default dict) "argocd.argoproj.io/sync-wave") }} {{- fail "authup: commonAnnotations must not set argocd.argoproj.io/sync-wave when useHelmHooks=false. The chart manages that annotation itself to keep the migration Job ordered after the built-in database, the ServiceAccount and its Secrets." }} {{- end }} +{{- if and (not .Values.useHelmHooks) .Values.serviceAccount.create (hasKey (.Values.serviceAccount.annotations | default dict) "argocd.argoproj.io/sync-wave") }} +{{- fail "authup: serviceAccount.annotations must not set argocd.argoproj.io/sync-wave when useHelmHooks=false. The chart manages that annotation itself to keep the migration Job ordered after the ServiceAccount." }} +{{- end }} {{/* Deprecation tripwires: when a value moves, add a guard here that names the new diff --git a/scripts/check-beta64-contract.py b/scripts/check-beta64-contract.py index c74606b..880fb36 100644 --- a/scripts/check-beta64-contract.py +++ b/scripts/check-beta64-contract.py @@ -713,6 +713,13 @@ def check_validations(): }, "commonAnnotations must not set argocd.argoproj.io/sync-wave", ) + render_fails( + { + "useHelmHooks": False, + "serviceAccount": {"annotations": {"argocd.argoproj.io/sync-wave": "5"}}, + }, + "serviceAccount.annotations must not set argocd.argoproj.io/sync-wave", + ) checks = {