fix(membership): wizard Job uses migration ServiceAccount#388
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
WalkthroughVersion badges were updated in the root ChangesHelm Chart Version Badge Bumps
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The main fix works for the hook-created migration ServiceAccount path, but it introduces a scheduling failure for configurations that disable creation of the migration ServiceAccount without naming an existing one. That is a regression for the wizard Job.
| securityContext: | ||
| {{- toYaml .Values.podSecurityContext | nindent 8 }} | ||
| serviceAccountName: {{ include "core.serviceAccountName" . }} | ||
| serviceAccountName: {{ include "core.postgres.job.serviceAccountName" . }} |
There was a problem hiding this comment.
🟠 [major] Keep wizard on an existing SA when migration SA is disabled
When config.migration.serviceAccount.create=false and no config.migration.serviceAccount.name is supplied, this helper renders default-migrate, but the chart does not create that ServiceAccount. The wizard Job previously used core.serviceAccountName in this configuration, so it could run with the normal chart/default SA; after this change the wizard pod will fail to schedule unless operators manually create default-migrate or set a name.
There was a problem hiding this comment.
this problem will be resolved in another pr
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The patch fixes the hook ordering case but also changes the rendered ServiceAccount for the default non-hook path, which can break existing configurations that only provision/configure the regular ServiceAccount.
| securityContext: | ||
| {{- toYaml .Values.podSecurityContext | nindent 8 }} | ||
| serviceAccountName: {{ include "core.serviceAccountName" . }} | ||
| serviceAccountName: {{ include "core.postgres.job.serviceAccountName" . }} |
There was a problem hiding this comment.
🟠 [major] Preserve the existing SA when hooks are disabled
When feature.migrationHooks=false, this still switches the wizard Job from the regular core.serviceAccountName to the migration ServiceAccount. In the non-hook path there is no hook-ordering issue, and installs that only configure IAM/RBAC on serviceAccount.annotations (or disable/create a differently named migration SA) will now run the wizard with an unconfigured or missing ServiceAccount even though hooks are off. Consider keeping the previous ServiceAccount unless migration hooks are enabled.
There was a problem hiding this comment.
the helper already does that job ?
The wizard Job picks up `core.job.annotations` (via `merge .Values.config.wizard.job.annotations`), so when `feature.migrationHooks=true` it renders as a `pre-install,pre-upgrade` hook at weight `10`. The Job was still pointing at `core.serviceAccountName` — the regular SA, which is a plain release resource. Hooks run before release resources are installed, so the wizard pod can't schedule: the SA doesn't exist yet. Switching to `core.postgres.job.serviceAccountName` reuses the migration Job's SA, which `core.postgres.job.sa.annotations` already marks as a hook at weight `0`. The SA exists before the wizard pod tries to mount it, and ArgoCD's hook translation lines up the same way. No new value knobs: the wizard needs the same Postgres/IAM access as the migration Job, and operators already configure that on `config.migration.serviceAccount.annotations`. Cascade: bumps membership 3.6.0 → 3.6.1, cloudprem 4.10.0 → 4.10.1, formance 1.14.2 → 1.14.3.
The wizard Job mounts a ConfigMap (`<release>-wizard`) for its `/config/config.yaml`. When `feature.migrationHooks=true` the Job renders as a `pre-install,pre-upgrade` hook, but the ConfigMap was a plain release resource — applied after hooks, so the wizard pod's `volumes` reference points at nothing and the container fails to start with `CreateContainerConfigError`. Annotates the ConfigMap with `core.job.annotations` so it lands in the same `pre-install,pre-upgrade` wave as the Job. Both at weight `10`: Helm/Argo apply weight-10 hooks as a batch, and k8s retries the pod's mount until the ConfigMap exists — soft race, but consistent with the Job's own gating and avoids dragging the ConfigMap into the weight-`0` SA tier.
Dav-14
force-pushed
the
fix/membership-wizard-job-sa
branch
from
ab80b58 to
4610a38
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The hook-mode issue is addressed, but the ServiceAccount change is unconditional and changes/breaks non-hook configurations that previously rendered a schedulable wizard Job.
| securityContext: | ||
| {{- toYaml .Values.podSecurityContext | nindent 8 }} | ||
| serviceAccountName: {{ include "core.serviceAccountName" . }} | ||
| serviceAccountName: {{ include "core.postgres.job.serviceAccountName" . }} |
There was a problem hiding this comment.
🟠 [major] Use the migration ServiceAccount only when hooks need it
When feature.migrationHooks=false this still switches the wizard Job from core.serviceAccountName to core.postgres.job.serviceAccountName, so existing non-hook installs start running the wizard under the migration SA instead of the chart's normal SA. In particular, if operators set config.migration.serviceAccount.create=false without providing a migration SA name, this renders default-migrate even though that ServiceAccount is not created by the chart, whereas the previous template used the regular default/configured service account and could schedule.
There was a problem hiding this comment.
the helper already does that job and know about wether to use the global / job serviceAccount
3 participants
Summary
When
feature.migrationHooks=true, the wizard Job in the membership chart picks upcore.job.annotations(via themerge .Values.config.wizard.job.annotationsblock at the top ofwizard/job.yaml) and renders as apre-install,pre-upgradehook with weight10. The Job was still pointing itsserviceAccountNameatcore.serviceAccountName— the regular ServiceAccount, a plain release resource. Hooks are installed before release resources, so the wizard pod can't schedule:serviceaccount "<name>" not found.Switching to
core.postgres.job.serviceAccountNamereuses the migration Job's SA, whichcore.postgres.job.sa.annotationsalready marks as a hook at weight0(i.e. it's created before the wizard tries to mount it). ArgoCD's hook translation lines up the same way. No new value knobs: the wizard needs the same Postgres/IAM access as the migration Job, and operators already configure that viaconfig.migration.serviceAccount.annotations.This is the same class of fix as #358 (which made the migration Job + its SA stable under Helm and ArgoCD) — extended to the wizard, which had been overlooked because it lives under
wizard/.Cascade: membership 3.6.0 → 3.6.1, cloudprem 4.10.0 → 4.10.1, formance 1.14.0 → 1.14.1.
Test plan
feature.migrationHooks=trueand confirm the wizard Job'sserviceAccountNameresolves to the migration SA.feature.migrationHooks=falseand confirm nothing else changes (the helper falls back to the same name as before in that path).migrationHooks.