From 110d7c8df30b1428b3ec848046b43cb22818e8a2 Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Mon, 7 Sep 2026 02:15:09 -0700 Subject: [PATCH] fix(cd): repair the deploy pipeline, which had never run green CD had zero successful runs since it was added. Five independent defects: 1. OIDC login failed on every run with AADSTS700213. The Entra federated identity credential carries the legacy subject `repo:Badri-Rajendran/Trueup:environment:production`, but GitHub now presents the immutable owner-ID/repo-ID form (`repo:Badri-Rajendran@79736071/Trueup@1357531178:...`), confirmed via `gh api .../actions/oidc/customization/sub`. A matching credential was added in Entra; the legacy one is kept as a fallback. 2. The `production` environment's required_reviewers rule left runs queued for up to 16h. Removed (by request). `environment: production` stays on every job deliberately -- it is what scopes the OIDC subject claim, so dropping it would break Azure login. 3. The worker readiness check could never pass. It compared `runningState` to "Running", but an app pinned to min=max=1 replicas settles on "RunningAtMaxScale" -- verified live against backend, frontend and worker. It also indexed `revision list [0]`, which has no ordering guarantee and can return a deprovisioning revision. Now resolves the revision by `latestRevisionName` and accepts both running spellings. 4. mcp-server would have crash-looped instantly. `python app/mcp/entrypoint.py` puts the script's own directory on sys.path rather than /app, so the first import raises `ModuleNotFoundError: No module named 'app'` -- reproduced directly against the built image. Now `python -m app.mcp.entrypoint`, which is what docker-compose.yml already used. 5. The post-deploy health check could report a false green. It curled the ingress straight after `containerapp update`, which returns once the new revision is created, not once it serves -- so the previous revision could answer. It now waits for the new revision to be provisioned, active and running before curling. Also restructures the worker/mcp-server steps from "exists -> update image only" to "bootstrap once, reconcile image + secrets + env vars every run". The old shortcut meant a half-bootstrapped app stayed broken forever, which is what had happened to worker: hand-created without secrets or env vars, it has been crash-looping on missing database_url/redis_url with only FLASK_APP set, and no CD run would ever have repaired it. Role assignment stays bootstrap-only and now fails with an actionable operator message, since CD's service principal holds Contributor, which cannot create role assignments -- granting it that right would let anyone who can merge to main hand out Key Vault roles. Adds a cd-production concurrency group so two merges cannot race and leave the older commit deployed. --- .github/workflows/cd.yml | 231 +++++++++++++++++++++++++++++---------- CHANGELOG.md | 28 +++++ 2 files changed, 199 insertions(+), 60 deletions(-) diff --git a/.github/workflows/cd.yml b/.github/workflows/cd.yml index cb770a2..1e3e454 100644 --- a/.github/workflows/cd.yml +++ b/.github/workflows/cd.yml @@ -1,10 +1,21 @@ name: CD # Root CLAUDE.md: push images to Azure Container Registry, run Alembic migrations, deploy to -# Azure. Gated on green CI (the `ci` workflow already required on `main`) plus a manual approval -# via the `production` GitHub Environment -- rollback is re-running the previous successful -# workflow run, since every image is tagged with its commit SHA, never `:latest` (Container Apps -# does not reliably re-pull a moved tag, learned the hard way during the MVP deploy). +# Azure. Gated on green CI (the `ci` workflow already required on `main`) -- rollback is re-running +# the previous successful workflow run, since every image is tagged with its commit SHA, never +# `:latest` (Container Apps does not reliably re-pull a moved tag, learned the hard way during the +# MVP deploy). +# +# The `production` GitHub Environment is deliberately kept on every job even though its +# `required_reviewers` rule was removed (2026-09-07, by request): the environment is what scopes +# the OIDC subject claim to `...:environment:production`, which is what the Entra federated +# identity credential is registered against. Dropping `environment:` here would change the subject +# to `...:ref:refs/heads/main` and break Azure login. It also keeps GitHub's deployment history. +# Re-gating is a one-click change in Settings -> Environments, with no workflow edit. +# +# NOTE: root CLAUDE.md still says "Gate production deploys on green CI plus manual approval". That +# line was deliberately left in place when the gate was removed; the approval half no longer +# reflects this workflow. on: workflow_run: workflows: ["CI"] @@ -15,6 +26,14 @@ permissions: id-token: write contents: read +# Production deploys must never overlap. Without this, two merges landing close together race +# through build/migrate/deploy independently and the *older* SHA can be the one that lands last on +# Container Apps. `cancel-in-progress: false` queues instead of cancelling -- cancelling a run +# mid-`migrate` would leave the database ahead of the deployed image. +concurrency: + group: cd-production + cancel-in-progress: false + env: RESOURCE_GROUP: trueup-mvp-rg REGISTRY: trueupmvpacr.azurecr.io @@ -31,7 +50,7 @@ jobs: runs-on: ubuntu-latest environment: production steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: ref: ${{ github.event.workflow_run.head_sha }} - name: Azure login (OIDC) @@ -124,10 +143,48 @@ jobs: --name frontend \ --resource-group ${{ env.RESOURCE_GROUP }} \ --image ${{ env.REGISTRY }}/trueup-frontend:${{ env.IMAGE_TAG }} + # `az containerapp update` returns once the new revision is *created*, not once it is + # serving. Curling the ingress straight afterwards can therefore be answered by the PREVIOUS + # revision and report a false green while the new image is still starting -- or already + # crash-looping. Waiting for the new revision to actually be the active, running one is what + # makes the health check below mean anything. + - name: Wait for the new backend + frontend revisions to become active + run: | + # The status is joined in JMESPath, not the shell: `--query "[a,b,c]" -o tsv` emits the + # three values on separate LINES (not tab-separated), so building the string with `tr` + # silently yields a multi-line value that no `case` pattern can ever match. + for APP in backend frontend; do + REV=$(az containerapp show --name "$APP" --resource-group ${{ env.RESOURCE_GROUP }} \ + --query properties.latestRevisionName -o tsv) + echo "$APP: waiting for revision $REV" + READY="" + for i in $(seq 1 60); do + STATUS=$(az containerapp revision show \ + --name "$APP" --resource-group ${{ env.RESOURCE_GROUP }} --revision "$REV" \ + --query "join('/', [properties.provisioningState, properties.runningState, to_string(properties.active)])" \ + -o tsv | tr '[:upper:]' '[:lower:]') + echo " $REV -> $STATUS" + case "$STATUS" in + # A single-replica app (min=max=1 for all three of these) settles on + # "RunningAtMaxScale" and never reports a bare "Running", so both are accepted. + provisioned/running/true|provisioned/runningatmaxscale/true) + READY=yes; break ;; + failed/*|*/failed/*|*/degraded/*) + echo "::error::$APP revision $REV failed to start ($STATUS)"; exit 1 ;; + esac + sleep 5 + done + [ -n "$READY" ] || { echo "::error::$APP revision $REV never became active ($STATUS)"; exit 1; } + echo "$APP: $REV active" + done + - name: Verify backend + frontend health run: | FQDN=$(az containerapp show --name frontend --resource-group ${{ env.RESOURCE_GROUP }} --query properties.configuration.ingress.fqdn -o tsv) for i in $(seq 1 10); do + # /health/ready is the backend's readiness probe, proxied through the frontend's nginx. + # A 200 here therefore proves BOTH containers plus the backend's Postgres and Redis + # connectivity in one call -- the response body is {"checks":{"database":..,"redis":..}}. CODE=$(curl -s -o /dev/null -w "%{http_code}" "https://$FQDN/health/ready" || echo "000") [ "$CODE" = "200" ] && echo "healthy" && exit 0 sleep 5 @@ -150,39 +207,60 @@ jobs: # MCP agent surface) reuse the backend image with a different command -- the same "different # process, same database" shape docker-compose.yml already establishes locally. Neither # existed as a Container App before this workflow; both are created idempotently here (a - # first run creates, every run after just updates the image) rather than provisioned by - # hand outside the pipeline, so this workflow is the one place their config is defined. + # first run creates, every run after reconciles image + secrets + env vars) rather than + # provisioned by hand outside the pipeline, so this workflow is the one place their config + # is defined. - name: Create or update the worker Container App run: | - if az containerapp show --name worker --resource-group ${{ env.RESOURCE_GROUP }} &>/dev/null; then - echo "worker exists -- updating image only" - az containerapp update \ + # Bootstrap -- `containerapp create` plus the managed-identity role grants -- runs only + # when the app is absent. Everything after it (image, secrets, env vars) is reapplied on + # EVERY run, and that is deliberate. The previous "exists -> update image only" shortcut + # meant a half-bootstrapped app stayed broken forever, which is not hypothetical: worker + # was created by hand without its secrets or env vars, sat crash-looping with only + # FLASK_APP set, and no CD run would ever have repaired it. `secret set` and + # `--set-env-vars` are idempotent and need only Contributor, so reconciling them every + # time costs nothing and makes this workflow the real source of truth for the config. + if ! az containerapp show --name worker --resource-group ${{ env.RESOURCE_GROUP }} &>/dev/null; then + echo "worker does not exist -- bootstrapping" + az containerapp create \ --name worker \ --resource-group ${{ env.RESOURCE_GROUP }} \ - --image ${{ env.REGISTRY }}/trueup-backend:${{ env.IMAGE_TAG }} - exit 0 + --environment ${{ env.CONTAINER_ENV }} \ + --image ${{ env.REGISTRY }}/trueup-backend:${{ env.IMAGE_TAG }} \ + --command flask \ + --args "jobs" "outbox-worker" \ + --min-replicas 1 --max-replicas 1 \ + --system-assigned \ + --registry-server ${{ env.REGISTRY }} + + + PRINCIPAL_ID=$(az containerapp show --name worker --resource-group ${{ env.RESOURCE_GROUP }} --query identity.principalId -o tsv) + # CD's service principal holds Contributor + AcrPush, and Contributor deliberately + # does NOT include Microsoft.Authorization/roleAssignments/write. Granting the pipeline + # that right would let anyone able to merge to main hand out Key Vault roles, which is + # exactly the escalation root CLAUDE.md's least-privilege rule exists to prevent -- so + # this bootstrap branch is only runnable by an operator, and says so instead of dying + # on a raw AuthorizationFailed. + for ROLE in "Key Vault Secrets User" "Key Vault Crypto User"; do + az role assignment create --assignee "$PRINCIPAL_ID" --role "$ROLE" --scope ${{ env.KEY_VAULT_ID }} && continue + echo "::error::Could not grant '$ROLE' to worker's managed identity." + echo "::error::CD's identity holds Contributor, which cannot create role assignments." + echo "::error::Bootstrap 'worker' out of band as an Owner/User Access Administrator;" + echo "::error::every later run then skips this block and just reconciles config." + exit 1 + done + + # RBAC assignments are not instantaneous -- the secret-set step below resolves each + # keyvaultref immediately, so a fixed wait here is the simplest correct fix (confirmed + # during the manual OIDC setup earlier this session: ~15s was sufficient in practice). + sleep 30 fi - echo "worker does not exist -- bootstrapping" - az containerapp create \ + echo "worker: applying image, secrets and environment" + az containerapp update \ --name worker \ --resource-group ${{ env.RESOURCE_GROUP }} \ - --environment ${{ env.CONTAINER_ENV }} \ - --image ${{ env.REGISTRY }}/trueup-backend:${{ env.IMAGE_TAG }} \ - --command flask \ - --args "jobs" "outbox-worker" \ - --min-replicas 1 --max-replicas 1 \ - --system-assigned \ - --registry-server ${{ env.REGISTRY }} - - PRINCIPAL_ID=$(az containerapp show --name worker --resource-group ${{ env.RESOURCE_GROUP }} --query identity.principalId -o tsv) - az role assignment create --assignee "$PRINCIPAL_ID" --role "Key Vault Secrets User" --scope ${{ env.KEY_VAULT_ID }} - az role assignment create --assignee "$PRINCIPAL_ID" --role "Key Vault Crypto User" --scope ${{ env.KEY_VAULT_ID }} - - # RBAC assignments are not instantaneous -- the secret-set step below resolves each - # keyvaultref immediately, so a fixed wait here is the simplest correct fix (confirmed - # during the manual OIDC setup earlier this session: ~15s was sufficient in practice). - sleep 30 + --image ${{ env.REGISTRY }}/trueup-backend:${{ env.IMAGE_TAG }} az containerapp secret set \ --name worker \ @@ -219,38 +297,53 @@ jobs: FEE_RATE_PCT=0.0 - name: Create or update the mcp-server Container App run: | - if az containerapp show --name mcp-server --resource-group ${{ env.RESOURCE_GROUP }} &>/dev/null; then - echo "mcp-server exists -- updating image only" - az containerapp update \ + # Same bootstrap-once / reconcile-every-run shape as the worker step above. + if ! az containerapp show --name mcp-server --resource-group ${{ env.RESOURCE_GROUP }} &>/dev/null; then + echo "mcp-server does not exist -- bootstrapping" + # External ingress: an MCP client connects from outside Azure (ADR 24 "Transport and + # deployment"). Auth is the app's own staff_api_token bearer check on every tool call + # (app/mcp/auth.py), not network-level restriction -- matching backend's own external + # ingress, which relies on its session/CSRF layer the same way. + # + # `python -m app.mcp.entrypoint`, NOT `python app/mcp/entrypoint.py`. Running the file + # as a path makes sys.path[0] the script's own directory (/app/app/mcp) and does NOT + # add the working directory, so the module's first import dies with + # `ModuleNotFoundError: No module named 'app'` -- verified directly against the built + # image. `-m` puts /app on sys.path and resolves correctly, and docker-compose.yml + # already uses the `-m` form for this same process. + az containerapp create \ --name mcp-server \ --resource-group ${{ env.RESOURCE_GROUP }} \ - --image ${{ env.REGISTRY }}/trueup-backend:${{ env.IMAGE_TAG }} - exit 0 + --environment ${{ env.CONTAINER_ENV }} \ + --image ${{ env.REGISTRY }}/trueup-backend:${{ env.IMAGE_TAG }} \ + --command python \ + --args "-m" "app.mcp.entrypoint" \ + --min-replicas 1 --max-replicas 1 \ + --system-assigned \ + --ingress external \ + --target-port 8001 \ + --registry-server ${{ env.REGISTRY }} + + PRINCIPAL_ID=$(az containerapp show --name mcp-server --resource-group ${{ env.RESOURCE_GROUP }} --query identity.principalId -o tsv) + # See the identical note in the worker step above: Contributor cannot create role + # assignments, and CD is deliberately not granted that right. + for ROLE in "Key Vault Secrets User" "Key Vault Crypto User"; do + az role assignment create --assignee "$PRINCIPAL_ID" --role "$ROLE" --scope ${{ env.KEY_VAULT_ID }} && continue + echo "::error::Could not grant '$ROLE' to mcp-server's managed identity." + echo "::error::CD's identity holds Contributor, which cannot create role assignments." + echo "::error::Bootstrap 'mcp-server' out of band as an Owner/User Access Administrator;" + echo "::error::every later run then skips this block and just reconciles config." + exit 1 + done + + sleep 30 fi - echo "mcp-server does not exist -- bootstrapping" - # External ingress: an MCP client connects from outside Azure (ADR 24 "Transport and - # deployment"). Auth is the app's own staff_api_token bearer check on every tool call - # (app/mcp/auth.py), not network-level restriction -- matching backend's own external - # ingress, which relies on its session/CSRF layer the same way. - az containerapp create \ + echo "mcp-server: applying image, secrets and environment" + az containerapp update \ --name mcp-server \ --resource-group ${{ env.RESOURCE_GROUP }} \ - --environment ${{ env.CONTAINER_ENV }} \ - --image ${{ env.REGISTRY }}/trueup-backend:${{ env.IMAGE_TAG }} \ - --command python \ - --args "app/mcp/entrypoint.py" \ - --min-replicas 1 --max-replicas 1 \ - --system-assigned \ - --ingress external \ - --target-port 8001 \ - --registry-server ${{ env.REGISTRY }} - - PRINCIPAL_ID=$(az containerapp show --name mcp-server --resource-group ${{ env.RESOURCE_GROUP }} --query identity.principalId -o tsv) - az role assignment create --assignee "$PRINCIPAL_ID" --role "Key Vault Secrets User" --scope ${{ env.KEY_VAULT_ID }} - az role assignment create --assignee "$PRINCIPAL_ID" --role "Key Vault Crypto User" --scope ${{ env.KEY_VAULT_ID }} - - sleep 30 + --image ${{ env.REGISTRY }}/trueup-backend:${{ env.IMAGE_TAG }} az containerapp secret set \ --name mcp-server \ @@ -281,12 +374,30 @@ jobs: MCP_SERVER_PORT=8001 - name: Verify worker is running (no HTTP endpoint of its own) run: | - for i in $(seq 1 10); do - STATE=$(az containerapp revision list --name worker --resource-group ${{ env.RESOURCE_GROUP }} --query "[0].properties.runningState" -o tsv) - [ "$STATE" = "Running" ] && echo "worker running" && exit 0 + # Two bugs lived here and both would fail this job against a perfectly healthy worker: + # 1. `revision list` has no ordering guarantee, so `[0]` could be the OLD revision that + # is mid-Deprovisioning. Resolve the revision by name via `latestRevisionName`. + # 2. worker is min-replicas=1 / max-replicas=1, so a healthy replica settles on + # "RunningAtMaxScale" -- a bare "Running" is never reported and the old + # `[ "$STATE" = "Running" ]` comparison could never be satisfied. + REV=$(az containerapp show --name worker --resource-group ${{ env.RESOURCE_GROUP }} \ + --query properties.latestRevisionName -o tsv) + echo "worker: waiting for revision $REV" + for i in $(seq 1 60); do + STATUS=$(az containerapp revision show \ + --name worker --resource-group ${{ env.RESOURCE_GROUP }} --revision "$REV" \ + --query "join('/', [properties.provisioningState, properties.runningState, to_string(properties.active)])" \ + -o tsv | tr '[:upper:]' '[:lower:]') + echo " $REV -> $STATUS" + case "$STATUS" in + provisioned/running/true|provisioned/runningatmaxscale/true) + echo "worker running ($REV)"; exit 0 ;; + failed/*|*/failed/*|*/degraded/*) + echo "::error::worker revision $REV failed to start ($STATUS)"; exit 1 ;; + esac sleep 5 done - echo "::error::worker never reached Running state ($STATE)" && exit 1 + echo "::error::worker revision $REV never reached a running state ($STATUS)" && exit 1 - name: Verify mcp-server health run: | FQDN=$(az containerapp show --name mcp-server --resource-group ${{ env.RESOURCE_GROUP }} --query properties.configuration.ingress.fqdn -o tsv) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0826ea..cf1fd6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,34 @@ ## Unreleased +- Fix the CD pipeline, which had never completed a successful run. Five defects, each independently + fatal: (1) the Entra federated identity credential was registered against GitHub's legacy OIDC + subject `repo:Badri-Rajendran/Trueup:environment:production`, but GitHub now presents the + immutable owner-ID/repo-ID form, so every run died at Azure login with `AADSTS700213`; (2) the + `production` environment's `required_reviewers` rule left runs queued for up to 16 hours; (3) the + worker readiness check compared `runningState` to `Running`, which a `min=max=1` app never + reports — it settles on `RunningAtMaxScale` — so the job could only ever fail, and it read + `revision list [0]`, which has no ordering guarantee and can return a deprovisioning revision; + (4) `mcp-server` was launched as `python app/mcp/entrypoint.py`, which puts the script's own + directory on `sys.path` instead of `/app` and dies with `ModuleNotFoundError: No module named + 'app'` — now `python -m app.mcp.entrypoint`, matching docker-compose.yml; (5) the post-deploy + health check curled the ingress immediately after `containerapp update`, so it could be answered + by the previous revision and report a false green — it now waits for the new revision to be + provisioned, active and running first. + +- Restructure the worker/mcp-server CD steps from "exists -> update image only" to "bootstrap once, + reconcile image + secrets + env vars every run". The old shortcut meant a half-bootstrapped app + stayed broken permanently, which is exactly what had happened: `worker` was hand-created without + its secrets or environment variables and had been crash-looping on missing `database_url` / + `redis_url` config with only `FLASK_APP` set, and no CD run would ever have repaired it. Role + assignment remains bootstrap-only and now fails with an actionable operator message rather than a + raw `AuthorizationFailed`, because CD's service principal holds Contributor, which deliberately + cannot create role assignments. + +- Add a `cd-production` concurrency group (`cancel-in-progress: false`) so two merges landing close + together cannot race and leave the older commit deployed, and so a run is never cancelled + mid-migration with the database ahead of the deployed image. + - Redesign the Fees page: real Stripe Elements for card entry (`useStripeCardElement.js`, `stripeLoader.js`), a `HighWaterMarkCard` and `BillingPeriodProgress` making the accrual mechanics legible instead of a bare number, and an honest `DunningBanner` for a past-due charge