Repository navigation
Conversation
arm/CUDly-CrossSubscription/template.json declared roleAssignmentGuidPrefix with a [newGuid()] default and described it as the base GUID for deterministic role assignment names, but nothing in the template read it: the custom role name and all three assignment names are guid() seeds over the principal, a literal and subscription().subscriptionId. Delete it rather than wire it in. Wiring the newGuid() default into the seeds would rename every assignment on each redeploy, and Azure refuses a second assignment of the same role to the same principal at the same scope (RoleAssignmentExists), so existing customers could no longer redeploy. No shipped command passes the parameter (setup.sh and the known-issues.md redeploy commands pass only servicePrincipalObjectId), so removal breaks none of them. The new iac tests fail if a declared parameter goes unread, if a parameter defaults to newGuid(), or if a shipped deploy command for the template passes a parameter the template no longer declares. Closes LeanerCloud#143 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
arm/CUDly-CrossSubscription/template.jsondeclared aroleAssignmentGuidPrefixparameter with a[newGuid()]default. Its description said it was the "Base GUID used to derive deterministic role assignment names", but nothing in the template read it. The custom role definition name and all three role assignment names come fromguid(...)seeds over the principal, a literal andsubscription().subscriptionId, so the parameter did nothing.Fix: remove it rather than wire it in
The parameter is deleted from the template. Wiring it in was rejected for three reasons:
newGuid()default in theguid(...)seeds, every deployment would produce new assignment names. Azure refuses a second assignment of the same role to the same principal at the same scope (RoleAssignmentExists), so a customer re-running the template (whichknown-issues.mdtells existing tenants to do) would fail. Any fixed default would also rename the assignments every existing tenant already has, with the same result.arm/CUDly-CrossSubscription/setup.shand both redeploy commands inknown-issues.mdpass onlyservicePrincipalObjectId. The federation bundles served byinternal/api/internal/iacfiles(azure-wif-deploy.sh,azure-wif-bicep-deploy.sh,azure-wif-bicep-params.json) deployiac/federation/azure-target/bicep/azure-wif.{bicep,arm.json}, not this template, and none of them mention the parameter. A repo-wide grep (frontend included) finds it only in the template, the 2026-09-02 audit report (A13-020, which recommends deleting it) and thescripts/testdata/role-parityfixtures.The only callers removal could break are customers who wrote their own command passing
roleAssignmentGuidPrefix=...by hand. The parameter was documented as "Leave at default", so that should be rare. Such a command will now fail with ARM's "parameter not present in the original template" error, and the fix is to drop the argument.Regression tests
New file
iac/arm_cross_subscription_template_test.go, in the existingiactest package that already testsarm/CUDly-CrossSubscription/setup-gcp-wif.sh:TestCrossSubscriptionTemplateReadsEveryParameterfails if any declared parameter is never referenced asparameters('<name>')outside theparameterssection (matched case-insensitively, like ARM).TestCrossSubscriptionTemplateHasNoNewGuidDefaultfails if a parameter defaults tonewGuid(). It catches the "wire it up" variant, which the reference check alone would pass.TestCrossSubscriptionDeployCommandsPassOnlyDeclaredParametersparses eachaz deployment sub createcommand for this template insetup.shandknown-issues.md(continuation lines joined) and fails if a command passes a--parameters key=the template does not declare. It also fails if it finds no command in a source file, so it cannot pass vacuously.Closes #143
Verification
Unit and fixture evidence only. Nothing was deployed, no
azcommand was run and no Azure API was called.Fail before the template change (test file added, template untouched):
Pass after:
Mutation probe for the caller guard: I temporarily added
--parameters roleAssignmentGuidPrefix="x"to thesetup.shdeploy command.TestCrossSubscriptionDeployCommandsPassOnlyDeclaredParametersthen failed withsetup.sh: deploy command passes undeclared parameter "roleAssignmentGuidPrefix", which ARM rejects. I reverted the probe afterwards.Other checks run on this branch:
Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@b56afb2b= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-324andfix/issue-204both edit.github/workflows/ci.ymlnext to theazure-kv-access-policyjob. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran oned0c7174, which differs fromb56afb2bonly by one.gitallowedline on fix/issue-324:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout):internal/api,internal/purchase,internal/schedulerandinternal/serverall pass..gitallowedline. Re-run atb56afb2b: git-secrets, using the repo'sscripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
scripts/testdata/role-parity/*fixtures were copied from this template and still declareroleAssignmentGuidPrefix. The parity checker ignores it, and they are standalone checker inputs that still pass, so I left them alone to keep the diff small. They can be cleaned up in a follow-up if wanted.docs/audits/codebase-audit-2026-09-02.mdstill reads "issue: (pending cross-reference)". It is a dated audit snapshot, so I did not edit it.setup.shor the documented redeploy.Maintainer: please mirror the issue's triage labels on this PR:
type/chore,impact/internal,priority/p3,severity/low,urgency/eventually,effort/xs,triaged.🤖 Generated with Claude Code