Repository navigation
fix(ci/azure): name the missing bootstrap stack when the role lookup fails - #1795
Conversation
…fails The Azure deploy has failed on every main run since 2026-07-19 (0 successes in 100 runs) because a custom role definition is absent from the subscription. The provider error, "loading Role Definition List: could not find role ...", names neither the prerequisite nor the stack that creates it, so three weeks of identical failures produced no action. The role is created by terraform/environments/azure/ci-cd-permissions and looked up by the deploy stack via data.azurerm_role_definition. That split is deliberate: creating a role definition needs roleDefinitions/write, which the deploy service principal intentionally lacks. The remediation was documented, but only in a source comment nobody reading a failed CI log would see. A Terraform precondition cannot help here. The provider fails the data read itself when the role is absent, so Terraform aborts before any postcondition evaluates; such a check would never fire for the failure it exists to explain. The remediation is emitted from the workflow's failure path instead, gated on the provider's error text. Terraform Apply now tees to a log so that step can inspect it. set -o pipefail is required: the default shell is bash -e, where a pipeline takes tee's exit status, and a failed apply would otherwise be reported as success. Verified both directions. Also hoists the reconstructed role name into a named local. The bootstrap module's role_definition_name output cannot be referenced across stack boundaries without terraform_remote_state, which this repo does not use, so the format string stays duplicated by necessity; the local at least makes it one place per module and records why. This does not fix the deploy. The bootstrap stack still has to be applied. It makes the next occurrence self-explaining rather than silent. Refs #1794
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Terraform module centralizes reconstruction of the Azure custom-role name. The deployment workflow logs Terraform plan and apply output, preserves failure status, and reports guidance when the custom role is missing. ChangesAzure role diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Independent adversarial review — head
|
| log contents | annotation printed | exit |
|---|---|---|
| real #1794 error text | yes | 0 |
Role Definition "foo" was not found |
yes | 0 |
unrelated failure (409 Conflict, RoleAssignmentExists) |
no | 0 |
| file absent (failure before Apply ran) | no | 0 |
| file empty | no | 0 |
Always exits 0, so it never adds a second failure or masks the real one.
if: failure() placement cannot interfere with the lock release
Sequential steps, independent conditions. failure() is a job-state predicate, so the explain step's own exit status does not change whether Release state lock on failure runs; and it exits 0 in all five cases above regardless. On cancellation the explain step is skipped (failure() alone) while the release step still runs (failure() || cancelled()). The grep is on a local file with no network call, so it cannot stall the release either.
Untrusted input
Both changed run: blocks contain zero ${{ }} interpolation — the only expansions are the shell variables ${RUNNER_TEMP} and $GITHUB_OUTPUT. Clean.
The local hoist is byte-identical
Compared programmatically against eca603a36:
old inline : 'CUDly Reservation Purchaser (custom) - ${data.azurerm_subscription.current.subscription_id}'
new local : 'CUDly Reservation Purchaser (custom) - ${data.azurerm_subscription.current.subscription_id}'
identical : True
and it still matches modules/iam/azure/cudly-reservation-role's local.role_definition_name modulo the suffix variable.
The remote-state claim in the PR body: correct, nothing missed
git grep terraform_remote_state -- '*.tf' returns exactly one hit — the new comment. The bootstrap's role_definition_name output genuinely cannot be consumed by the deploy stack: separate state, and module outputs do not cross stack boundaries. The only mechanism that would actually remove the duplicated format string is extracting a naming-only module instantiated in both stacks, which is more machinery than one string is worth. Duplicating the format with cross-references in both places is the right call.
Tooling
terraform fmt -check -recursive terraform/— clean.terraform validateonmodules/compute/azure/container-apps—Success! The configuration is valid.actionlint— 23 shellcheck findings on base, 23 on head. No new lint debt, and none point at the new step (they are pre-existingSC2086/SC2129on unquoted$GITHUB_OUTPUTand repeated>>).- CI 20/20 green.
Findings
F2 — medium-low, completeness: the diagnostic only covers a failure in Terraform Apply
The error currently does surface in that step — verified in run 31310520753: the Terraform Apply group opens at log line 1476, the could not find role error is at line 2092, and the next step group opens at 2103. So the tee captures it and the grep fires. Today the feature works.
But that is contingent, not structural. The plan for the same run says:
# module.compute_container_apps[0].data.azurerm_role_definition.cudly_reservation_purchaser will be read during apply
# (config refers to values not yet known)
The read is deferred, which traces to the module-level depends_on = [module.networking, module.database, module.secrets, module.build] at terraform/environments/azure/compute.tf:75. If that dependency ever stops forcing deferral, the read moves to plan time, Terraform Plan fails with the same message, tf-apply.log is never written, and the explain step silently exit 0s. That is the identical "grep a log nothing wrote" defect already caught once in this PR, surviving in a second location — and it fails silently, so nobody would notice the diagnostic had stopped working.
Cheap fix, same shape as the apply step:
- name: Terraform Plan
run: |
set -o pipefail
cd terraform/environments/azure
terraform plan ... -out=tfplan 2>&1 | tee "${RUNNER_TEMP}/tf-plan.log"and grep both logs in the explain step (grep -qiE ... "${RUNNER_TEMP}/tf-plan.log" "${RUNNER_TEMP}/tf-apply.log" — with 2>/dev/null it already tolerates either file being absent).
Note the plan step would need its own set -o pipefail for the same reason the apply step does; without it a failing plan piped through tee reports success, which is the F1 hazard reintroduced.
F3 — low, scope: single-use local plus heavy comment density
local.cudly_reservation_purchaser_role_name has exactly one consumer and changes no behaviour. The hunk is +17 lines, 16 of them comment, stacked above a pre-existing 15-line BOOTSTRAP-vs-RUNTIME SPLIT comment on the same 4-line data block — roughly 35 lines of prose for 4 lines of config, against the repo's "comment sparingly, 1-2 lines, only where the why isn't deducible" rule. The load-bearing part of this PR is entirely in the workflow. Reasonable alternative: keep the postcondition-rationale comment (that one earns its place — it stops the next person re-adding the check), drop the local and its 8-line comment, and leave the format string inline where it already carried a cross-reference.
Not blocking; your call.
F4 — low: one grep alternative matches nothing the provider emits
Role Definition .* was not found does not correspond to any azurerm message. The actual text is loading Role Definition List: could not find role '<name>' (first alternative, which matches). The dead alternative traces to the pre-existing comment at container-apps/main.tf:277, which asserts the data source "fails loudly with Role Definition ... was not found" — that quote is wrong, and this PR edited that exact block for exactly this reason and left it standing. Suggest correcting the comment to the real string and dropping the second alternative, or keeping it with a note that it is defensive against a future provider message.
F5 — informational: guard against a future artifact upload of tf-apply.log
No leak today (see above), but the file holds an unmasked subscription ID and possibly a non-sensitive admin_email. One line next to the tee saying the log must not be uploaded or cat'd would stop a later change from turning a diagnostic into a secret exfil path. Optional.
Verdict: mergeable. The riskiest change is correct and I could not break it. F2 is the one I would fix first — it is two lines, and without it the feature has a silent-no-op mode that is exactly the class of bug this PR exists to eliminate.
…ately Review finding F2: the data source is read at plan time as well as apply, so a missing role can surface in either step. The diagnostic only inspected the apply log, so a plan-time failure would have produced the same silence this change exists to end. Terraform Plan now tees to its own log. It needs its own `set -o pipefail` for the same reason the apply step does: under `bash -e` a pipeline takes tee's exit status, so a failed plan would be reported as success. The two logs are grepped separately rather than as two operands to one grep. With multiple operands where one does not exist, grep's exit status is implementation-defined: GNU returns 0 when -q matched an earlier file, BSD returns 2. Either log may legitimately be absent when an earlier step failed first, so the combined form goes silent on some hosts. Caught by testing the one-file-missing case, which is the case that actually occurs. Verified in all four states: plan-only match with apply absent fires, apply-only match with plan absent fires, both present with neither matching stays silent, both absent stays silent. Refs #1794
Delta review —
|
| plan.log | apply.log | annotation | exit |
|---|---|---|---|
| matches | absent | fires | 0 |
| absent | matches | fires | 0 |
| present, no match | present, no match | silent | 0 |
| absent | absent | silent | 0 |
| present, no match | matches | fires | 0 |
| matches | present, no match | fires | 0 |
| matches | matches | fires (once) | 0 |
| present but empty | matches | fires | 0 |
Always exits 0, so it still cannot add a second failure or displace the real one, and the lock release is unaffected. The break prevents a double annotation when both logs match. Under bash -e the loop is safe: grep -q sits in an if condition, and a final continue or a false if both leave the loop's status at 0.
Finding — the comment's grep-exit-status claim is false on both implementations
With multiple operands where one does not exist, grep's exit status is implementation-defined: GNU returns 0 when
-qmatched an earlier file, BSD returns 2.
Measured directly, same pattern, same operand orders:
--- grep (BSD grep, GNU compatible) 2.6.0-FreeBSD --- --- ggrep (GNU grep) 3.12 ---
absent, match -> 0 absent, match -> 0
match, absent -> 0 match, absent -> 0
absent, nomatch -> 2 absent, nomatch -> 2
nomatch, absent -> 2 nomatch, absent -> 2
both absent -> 2 both absent -> 2
nomatch, match -> 0 nomatch, match -> 0
BSD returns 0, not 2, in exactly the case the comment names. The two implementations are identical across all six combinations. It is also not implementation-defined: POSIX specifies that -q exits zero if an input line is selected, and both man pages carry the same carve-out ("if any error occurs and -q is not specified, the exit status is 2").
Follow the consequence through: the only rows returning 2 are the ones with no match anywhere, where the wanted behaviour is silence — and if ! grep ... on exit 2 is silence. So the original combined form
grep -qiE '...' "$TMP/tf-plan.log" "$TMP/tf-apply.log" 2>/dev/nullwould have been correct in all four states, on both greps. There was no portability bug to fix.
To be clear about what this does and does not mean: the loop you shipped is correct, more explicit, and I would not ask you to revert it. But the 5-line paragraph justifying it asserts a portability hazard that does not exist, and that is the kind of comment a future reader cites as settled fact rather than re-measuring. Suggest cutting it to something honest about the real reason — one file may legitimately be absent, and grepping each behind [ -f ] makes that explicit — or dropping the paragraph entirely.
Second comment claim, also worth correcting
the data source is read at plan time as well as apply, so a missing role can surface in either step depending on state freshness
This overstates my finding and does not match the observed run. In run 31310520753 the plan explicitly deferred the read:
# module.compute_container_apps[0].data.azurerm_role_definition.cudly_reservation_purchaser will be read during apply
# (config refers to values not yet known)
The read happens at apply today, not at both. What I flagged was that this is contingent rather than structural: it traces to the module-level depends_on = [module.networking, module.database, module.secrets, module.build] at terraform/environments/azure/compute.tf:75, and if that stops forcing deferral the read moves to plan. "Depending on state freshness" names a mechanism that was not measured. Teeing both logs is the right response either way — the fix is correct, only the stated reason is. Suggest replacing with the depends_on reference so the next reader can check it.
Secret-bearing logs: property still holds with the second file
- Both logs live only in
${RUNNER_TEMP}for the job's lifetime, and neither is uploaded. Re-verified at the new head: the onlyactions/upload-artifact(line 324) haspath: deployment-info.json, and its step carries noif:, so it is skipped on the failure path where these logs exist. The explain step greps and nevercats either file. tf-plan.logis the more exposure-prone of the two, since plan output renders full attribute diffs rather than apply's progress lines.admin_passwordissensitive = true(variables.tf:361) so it renders as(sensitive value);admin_email(variables.tf:356) is not, so it can appear in the plan diff and lands unmasked on disk. No leak today, but F5 applies more strongly to this file than to the apply log.
Tooling
actionlint: 23 shellcheck findings at e11d47b6b — identical to base eca603a36 and to bd07659b4. The new for loop and [ -f ] guard introduce nothing.
Verdict on the delta: clean, but do not merge yet
The fix is correct and I could not break it in eight states. F1/F4 need nothing further — F4 stands as previously described (the dead Role Definition .* was not found alternative and the wrong error quote at container-apps/main.tf:277), still optional. F3 I withdraw: your reasoning is sound, the comment records something a reader would otherwise re-derive from two stacks, and it is the author's call.
Two blockers on the mechanical side, neither about the diff:
- CI is still running at
e11d47b6b—Build Docker Image,Integration Tests,Security ScanningandUnit Testswere pending when I checked; PR state isBLOCKED. Green those before merging. - The two comment corrections above are worth folding in first, since both are load-bearing prose in a change whose entire purpose is that a future reader understands why the deploy failed.
Review findings on the prose, both in a change whose entire purpose is that a future reader understands why the deploy failed. The grep-exit-status claim was wrong. The comment asserted that multi-operand behaviour is implementation-defined, GNU returning 0 and BSD 2. Measured on both real binaries, they are identical and match POSIX: -q exits 0 if a line is selected even when another operand errored, and the only combinations returning 2 are those with no match anywhere, where silence is the wanted behaviour. The combined form would have worked. The contradicting measurement came from ugrep: `grep` in the authoring shell is a function that execs ugrep 7.5.0, so the experiment measured neither the system grep nor what CI runs. The per-file loop is kept because it makes the expected "this log does not exist" case explicit at the point of reading, not because the alternative is broken. The plan-time claim overstated what was observed. The role lookup is deferred to apply today, reported by the plan as "will be read during apply (config refers to values not yet known)". That is contingent on the module-level depends_on at terraform/environments/azure/compute.tf:75 rather than structural, so teeing both logs is still correct; the comment now names the depends_on so the next reader can check whether it still holds. Refs #1794
Delta review —
|
|
Merging. Refs #1794. This does not fix the deploy — the bootstrap stack still has to be applied. It makes the next occurrence self-explaining instead of silent for three weeks. Independently reviewed; I wrote it, so I did not clear it myself. What went wrong, and why nobody saw itThe Azure deploy has failed on every It stayed invisible because Three unfireable guards, caught in a change whose purpose is that a guard firesWorth recording plainly, since this PR exists to end a silent failure.
Two false claims in my own comments, correctedBoth found in review, both load-bearing prose in a change about legibility. The grep-exit-status claim was wrong. I asserted multi-operand behaviour was implementation-defined, GNU returning 0 and BSD 2. Measured on both real binaries they are identical and match POSIX: My contradicting measurement came from The plan-time claim overstated the evidence. The lookup is deferred to apply today — the plan reports "will be read during apply (config refers to values not yet known)" — contingent on the module-level What the step printsThe stack to apply, an explicit warning not to grant Name duplication: reduced, not removedHoisted to a named local with the reason recorded. The bootstrap module exports The dependency itself is structural: the role needs a privileged stack to create it, while the identity it is assigned to does not exist until the deploy stack runs. Verification20/20 checks. Still open on #1794: whether |
1 participant
Refs #1794. This does not fix the deploy — the bootstrap stack still has to be applied. It makes the next occurrence self-explaining instead of silent.
Why three weeks of identical failures produced no action
The Azure deploy has failed on every
mainrun since 2026-07-19 (0 successes in 100 runs), at Terraform Apply:That error names neither the missing prerequisite nor the stack that creates it. The role is created by
terraform/environments/azure/ci-cd-permissionsand consumed by the deploy stack throughdata.azurerm_role_definition. The split is deliberate: creating a role definition needsMicrosoft.Authorization/roleDefinitions/write, which the deploy service principal intentionally lacks.The remediation was documented — at
container-apps/main.tf:275-280, in a source comment. That is the right place for someone reading the module and the wrong place for someone reading a failed CI log. Nobody was reading the module.It also went unnoticed because
CI - Build & Testis green and has been throughout. Every gate anyone looks at is green; the deploy is a separate workflow that nothing surfaces. Same shape as #1751, where./...covered one workspace module of six.A Terraform precondition cannot solve this, and I tried it first
My first version added a
lifecycle { postcondition }to the data source. It would never have fired. The provider fails the data read when the role is absent, so Terraform aborts before any postcondition evaluates — a guard that cannot fire for the failure it exists to explain.Caught before pushing, and worth recording since it is the exact defect class this repo has spent #1740/#1750/#1758 removing.
So the remediation is emitted from the workflows failure path, gated on the providers error text.
The second unfireable guard, also caught
The new step greps
${RUNNER_TEMP}/tf-apply.log— and nothing was writing that file, so the grep would always miss and the message would never print.Terraform Applynow tees to it.set -o pipefailis required: the default shell isbash -e, where a pipeline takestees exit status, so a failed apply would have been reported as success. Verified both directions:And the grep verified in both directions too — it matches the real error text, and stays silent on an unrelated Terraform failure.
What the step prints
The stack to apply, plus an explicit warning not to grant
roleDefinitions/writeto the deploy SP as a workaround, and three ordered checks if the bootstrap has already been applied. The first matters most: a deploy SP lackingMicrosoft.Authorization/roleDefinitions/readsees an empty list, not an authorization error — indistinguishable from the role being absent.The name duplication, reduced but not removed
The lookup name is hoisted into a named local with the reason recorded. The bootstrap module exports
role_definition_namefor exactly this, but its output cannot be referenced across a stack boundary withoutterraform_remote_state, which this repo does not use anywhere. Introducing it (or a parameter store) would relocate the dependency rather than remove it, and add a second mechanism where the repo has 16datablocks and one pattern.The dependency itself is structural and cannot be removed: the role needs a privileged stack to create it, while the identity it is assigned to does not exist until the deploy stack runs.
Verification
terraform validate: Success,terraform fmtcleanTerraform ApplyandRelease state lock on failure, withif: failure()github.event.*interpolation)Still open on #1794: whether
azurerm_container_app.mainapplies before the failure, which decides if three weeks of merged Azure changes are live or not. The cheap way to settle it is comparing the running image tag againstmain.Summary by CodeRabbit
Bug Fixes
Documentation