Skip to content

fix(scripts): ship the Azure role module in generate-federation-iac bundles - #1467

Merged
IAmJSD merged 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1442
Oct 26, 2026
Merged

IAmJSD merged 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1442

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Closes #1442

Root cause

scripts/generate-federation-iac.go --format bundle has its own bundle builder. It zipped only the target's Terraform directory (iac/federation/azure-target/terraform), whose main.tf calls ../../../../terraform/modules/iam/azure/cudly-reservation-role. #1440 fixed this for the API download by shipping the module inside the zip, but that helper was unexported in internal/api, so the script never got the fix. terraform init on a script-built Azure bundle still failed with "Unreadable module directory".

Fix

  • New small package internal/iacbundle holds the bundle Terraform writer, the module list (ExtraModules), the source rewrite, and the UnresolvedLocalModules check. It reads any fs.FS and skips hidden files and directories, so the script can pass its on-disk modules directory without picking up a local .terraform/ or lock file.
  • The API now builds its bundles through it. Behaviour is unchanged: the embedded FS has no hidden files, and the module set and rewrite are the same. The now-unused addDirToZip is removed.
  • The script's addTerraformToBundle calls iacbundle.AddTerraformDir(os.DirFS(modulesDir), ...), so the Azure bundle ships the role module at terraform/modules/cudly-reservation-role and the call points at it. Like the API's bundle, the script's Azure bundle now also carries terraform/tests/identity_guards.tftest.hcl. Before, the script copied only top-level files.

Three commits, each under 400 lines: add the package, switch the API to it, then switch the script and add its regression test.

Verification

  • New scripts/generate_federation_iac_bundle_test.go builds every bundle the script can make (all 5 target/source combos), runs it through iacbundle.UnresolvedLocalModules, and checks that no hidden file is shipped. On main the azure-target case fails with terraform/main.tf: ../../../../terraform/modules/iam/azure/cudly-reservation-role. It passes with the fix.
  • New internal/iacbundle unit tests: module shipped and rewritten, hidden files skipped, an uncalled module is an error, and the guard's self-test (moved from internal/api).
  • Manual run: built an Azure bundle with the script, unzipped it to an empty directory, and ran terraform get there. It resolved cudly_reservation_role in modules/cudly-reservation-role. No init or plan was run against any backend.
  • go build ./..., go vet (plain, integration, e2e), golangci-lint v2.10.1 with and without --build-tags integration (0 issues), gocyclo -over 10 (clean), go test -race -short on iacbundle/api/scripts/terraform, go test -race -tags integration ./internal/api/ (ok), the secrets scan (clean), and scripts/test-git-secrets-allowlist.sh (183 PASS).

🤖 Generated with Claude Code

IAmJSD and others added 3 commits October 26, 2026 10:21
…d script

The API's federation bundle ships the out-of-tree Azure role module (LeanerCloud#1431),
but the helper that does it is unexported in internal/api, so
scripts/generate-federation-iac.go cannot use it and still builds an Azure
bundle that calls a module outside the zip (LeanerCloud#1442). Move the writer, the
module list and the "local module sources resolve inside the zip" check into
a small package both builders can import. It reads any fs.FS so the script
can pass its on-disk modules directory, and skips hidden files and
directories so a developer's .terraform/ or lock file never lands in a zip.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Use the shared writer for the API's federation bundles so the API and the
developer script cannot drift apart again. Behaviour is unchanged: the
embedded FS has no hidden files, and the module set and rewrite are the
same. The bundle test keeps running every combo through the shared
unresolved-module check; the guard's own self-test moved with it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…undles

--format bundle --target azure zipped only iac/federation/azure-target/
terraform, whose main.tf calls ../../../../terraform/modules/iam/azure/
cudly-reservation-role, so `terraform init` on the unzipped bundle failed
with "Unreadable module directory". Build the Terraform part through
internal/iacbundle, as the API does since LeanerCloud#1440: the module ships at
terraform/modules/cudly-reservation-role and the call points at it. Like the
API's bundle it now also carries the target's terraform/tests/ directory.

The new test runs every bundle the script builds through the same
resolves-inside-the-zip check as the API's bundles; the Azure case fails
on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p3 Polish / idea / may never ship type/bug Defect labels Oct 26, 2026
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 8 billable files and costs up to $2.00.

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 01fb35a and c0a30ad.

Files selected for processing: 8
  • internal/api/federation_bundle_modules.go
  • internal/api/federation_bundle_modules_test.go
  • internal/api/handler_federation.go
  • internal/iacbundle/bundle.go
  • internal/iacbundle/bundle_test.go
  • scripts/generate-federation-iac.go
  • scripts/generate_federation_iac_bundle_test.go
  • terraform/embed.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Opened as a draft at c0a30ad: the script's bundle now goes through the shared internal/iacbundle writer, so its Azure bundle ships the role module. A regression test covers all 5 script bundles. Checks are listed in the PR body.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE c0a30ad179b8bd7c1402d227cc7c44279e5bb9b4

Blocking:

  • none

Non-blocking:

  • internal/iacbundle/bundle.go:82-90: walkVisible skips dot-prefixed names only. go:embed also leaves out _-prefixed names, so the API's copy and the script's os.DirFS copy would differ if a _foo.tf were ever added under iac/federation/*/terraform. None exist today, so this is theoretical.

Attacked:

  • API path equivalence: the old addBundleTerraformDir + addDirToZip and the new AddTerraformDir walk the same embedded FS in the same WalkDir order, with the same rewrite and the same module set. The embed has no dot files, so the API zip contents don't change. The module still comes from the *.tf-only embed in both builders.
  • Script path: the main directory is read from disk through os.DirFS(modulesDir) with hidden entries skipped, so the .terraform.lock.hcl present in every iac/federation/*/terraform dir stays out, as before. The only subdirectory on disk is azure-target/terraform/tests, which now ships, matching the API bundle. The PR body says so.
  • Fail-before: put main's scripts/generate-federation-iac.go back in the scratch tree and ran TestGenerator_BundleLocalModulesResolveInsideZip. The azure-target case fails with terraform/main.tf: ../../../../terraform/modules/iam/azure/cudly-reservation-role. The other 4 combos pass. Restored, all pass.
  • Ran: go build ./...; go vet on iacbundle/api/scripts/terraform, with and without -tags integration; go test -race -short -count=1 on iacbundle, scripts and terraform, plus the federation/bundle tests in internal/api (all ok); go test -race -tags integration -run 'Federation|Bundle|IaC' ./internal/api/ (ok); golangci-lint v2.10.1, with and without the integration tag (0 issues); gocyclo -over 10 on the changed non-test files (clean). Closes fix(scripts): generate-federation-iac.go's Azure bundle still references a module outside the zip #1442 fits: both acceptance points (shared helper, script output run through the resolve check) are met.

Body-stale:

  • none

Reviewer: rE2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE c0a30ad179b8bd7c1402d227cc7c44279e5bb9b4

Blocking:

  • none
    Non-blocking:
  • scripts/generate-federation-iac.go:333 / internal/iacbundle/bundle.go:79: the script now walks the on-disk module directory recursively through os.DirFS, and it skips only dot-prefixed names. A developer's local, non-hidden artefacts in that directory (terraform.tfstate, terraform.tfstate.backup, a *.tfvars, a saved tfplan) would ship in the zip. That was already true for top-level files before this PR, and it is a dev-only tool, so this isn't new exposure. A tracked-files-only filter, or an allowlist of *.tf, *.tftest.hcl and README*, would close it for good. The API path is unaffected: go:embed only ever sees tracked files.
  • Minor: go:embed also drops _-prefixed names, and walkVisible doesn't. No such files exist today.
    Attacked: I diffed the new iacbundle against the deleted internal/api helpers. The module set, rewrite, uncalled-module error and zip layout are identical, and the API keeps using the embedded FS (which has no hidden files), so the API bundle is unchanged. I checked that the extra module comes from the compile-time embed (*.tf only), so a go run of the script ships the current tree's module, and that UnresolvedLocalModules resolves ./modules/cudly-reservation-role relative to terraform/main.tf. I also checked how the //go:build ignore script is exercised by runGenerator in the scripts tests. Ran: go build ./..., go vet with and without -tags integration on iacbundle/api/scripts/terraform, go test -race -short -count=1 on iacbundle, api, scripts, terraform and terraform/... (all ok), go test -race -tags integration -run 'Federation|Bundle' ./internal/api/ (ok), and golangci-lint run ./internal/iacbundle/ ./scripts/ (0 issues). Fail-before: with scripts/generate-federation-iac.go reset to the merge-base, TestGenerator_BundleLocalModulesResolveInsideZip/azure-target fails with terraform/main.tf: ../../../../terraform/modules/iam/azure/cudly-reservation-role. It passes at the head. GitHub reports the PR MERGEABLE.
    Body-stale: none

Reviewer: rE3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at c0a30ad179b8bd7c1402d227cc7c44279e5bb9b4 merged onto main (990f4936), together with #1462, #1467, #1468 in that order (combined tree 7a76ef40, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 26, 2026 12:26
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 26, 2026 12:26
@IAmJSD
IAmJSD merged commit 7753ae2 into LeanerCloud:main Oct 26, 2026
41 checks passed
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on c0a30ad1 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly c0a30ad179b8bd7c1402d227cc7c44279e5bb9b4.

IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 26, 2026
Picks up LeanerCloud#1467, which also edits scripts/generate-federation-iac.go, so
the generator tests are re-run on the merged tree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit that referenced this pull request Oct 26, 2026
…rst, read the dev Function URL from state (#1312)

* chore(iac): derive the CFN issuer host, create the GCP signing key first, read the dev Function URL from state

Three low findings from the 2026-09-02 audit.

A13b-014: the aws-target CloudFormation template took the trust policy's
condition-key host as a second parameter, OIDCIssuerHost, that the operator
had to keep equal to OIDCIssuerURL minus https://. The host is now derived
with !Select [1, !Split ["https://", !Ref OIDCIssuerURL]], the same value as
the Terraform module's local.oidc_condition_host, and the server no longer
passes the parameter in the bundle's params JSON or deploy script (CFN would
reject an undeclared parameter).

A13c-024: the Cloud Run module's OIDC signing key now has
create_before_destroy, matching its AWS twin, so a change that forces a new
key cannot destroy the live one first. destroy_scheduled_duration becomes an
input whose default keeps every deployed key unchanged (the provider marks it
ForceNew). prevent_destroy cannot be an input (Terraform requires a literal)
and staging cleanup runs terraform destroy, so the redundant
`prevent_destroy = false` line is replaced by a comment saying so.

A13c-017: github-dev.tfvars no longer pins the dev Lambda Function URL host.
The deploy and rollback workflows read the previous apply's
lambda_function_url output (scripts/read-lambda-function-url.sh) and pass it
as TF_VAR_lambda_deployed_function_url; compute.tf appends its origin to the
CORS allowlist. A post-apply step warns when the apply changed the URL.

Refs #324

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(iac): allowlist the mocked caller-identity account ID in the CORS tftest

The combined-tree git-secrets run flagged
terraform/environments/aws/tests/lambda_cors_origins.tftest.hcl:10, a bare
account_id = "123456789012" in the mocked aws_caller_identity. Add an
anchored whole-line .gitallowed entry for that one line (bracketed digits so
the entry does not match itself), placed before the other terraform/ entries
so it does not collide with #1255's entry for its own tftest.

Refs #324

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Merge origin/main into fix/issue-324

Bring the branch up to date with main.

Conflicted files:
- .github/workflows/ci.yml: this branch appended a "Run Function URL
  reader self-tests" step to the aws-tfstate-platform-key job; main
  (#1112) added a new rollback-ecr-registry job right after that job.
  Kept both: the step stays at the end of aws-tfstate-platform-key and
  main's job follows it unchanged.

rollback.yml merged cleanly: main's rewritten "Verify image exists" steps
and this branch's TF_VAR_lambda_deployed_function_url export touch
different steps.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Merge origin/main into fix/issue-324

Two textual conflicts, both resolved by keeping both sides:
- ci.yml: main added an init-backend.sh self-test step to the same job
  this branch appends the Function URL reader self-tests to, plus a new
  tfstate-concurrency-group job after it. Both steps stay in the job,
  and main's job follows.
- cloud-run/.terraform.lock.hcl: main's "~> 5.45" constraint with the
  union of both sides' h1: hashes for google 5.45.2.

#1256 (merged on main) added OIDCIssuerHost to the standalone
generator; the next commit drops it again to match this branch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(scripts): drop OIDCIssuerHost from the standalone IaC generator

#1256 added OIDCIssuerHost to iacData because aws-cfn-deploy.sh.tmpl
passed it at the time. This branch removes the parameter from the
CloudFormation template and the deploy script (the host is derived from
OIDCIssuerURL), so the field is unused again and the drift test that
compares iacData with federationIaCData fails until it goes.

The bundle test now checks that the rendered deploy script passes
OIDCIssuerURL and never OIDCIssuerHost, since CloudFormation rejects an
override for an undeclared parameter. The field walker's control uses
OIDCIssuerURL instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs(iac): note the key ring order and the URL reader's failure mode

Review follow-ups on #1312. create_before_destroy on the signing key
also applies to the key ring it depends on, and the Function URL reader
treats any terraform output failure as "no URL". Both are intended, but
neither was written down where the code is.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(api): pass t to federationHandler in the CFN bundle test

#1223 on main made federationHandler take *testing.T so it can register
AssertExpectations on its mock. This branch's CFN bundle parameter test
still called it with no arguments, so internal/api stopped compiling
after the merge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Merge origin/main into fix/issue-324

Picks up #1467, which also edits scripts/generate-federation-iac.go, so
the generator tests are re-run on the merged tree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority/p3 Polish / idea / may never ship type/bug Defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(scripts): generate-federation-iac.go's Azure bundle still references a module outside the zip

1 participant