Repository navigation
test(api/federation): guard provider bounds across bundles and list them in the README - #1295
Conversation
…hem in the README
The five customer federation bundles under iac/federation each declare
their own required_providers. Nothing stopped one bundle from drifting to
a different floor or ceiling than another for the same provider, and the
bundle README never said which provider versions the module expects or
how a customer should move to a newer line.
- Parse required_providers from each bundle's embedded main.tf and append
a "Provider versions" section to the bundle's README.txt, with the
ranges read from the file itself so the README cannot drift from them.
It also says the bundle ships without a lock file and how to upgrade.
- Add a test that renders every bundle through getFederationIaC and fails
when two bundles declare a different floor or ceiling for the same
provider ("~> 4.0" and ">= 4.0, < 5.0" count as equal).
The constraint lines themselves are left to LeanerCloud#1203, which adds the
ceilings; this commit does not edit any main.tf.
Refs LeanerCloud#55
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
type/chore
|
Warning Review limit reached
This review includes 3 billable files and costs up to $0.75. Or wait 59 minutes for your next included review. View limit details
Comment |
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: REQUEST_CHANGES Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB19. 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. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LeanerCloud#1223 (on main) gave federationHandler a *testing.T so it can register AssertExpectations. This branch's new renderBundleFiles still called it with no argument, so the internal/api test build broke once main was merged in. Same handler, same assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the review at |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB22. 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. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB23. 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. |
|
Local verification at |
|
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 |
Root cause
The five customer federation bundles (
iac/federation/*/terraform/main.tf, served as zips bybuildFederationBundle) each declare their ownrequired_providers. Nothing checks that they stay consistent with each other, and the bundle'sREADME.txtnever said which provider versions the module expects or how a customer moves to a newer line. The issue's acceptance list asks for both.Current main, re-checked against cristim's enumeration:
>= 5.0>= 3.4>= 3.0>= 5.0>= 3.4>= 5.0>= 3.4>= 5.0>= 3.4>= 3.4~> 3.8~> 4.0One correction to the comment's table:
aws-cross-accountline 10 ishashicorp/random >= 3.0, notazurerm >= 3.0. No bundle declares azurerm except azure-target.Two things have changed since the issue was filed:
~> 5.0floors no longer fit. Each bundle directory now has a committed.terraform.lock.hclat aws 6.40.0 and google 7.27.0.go:embedskips dot files, so customers never receive those locks, and a freshinittoday resolves aws 6.70.0 and google 8.8.0 (shown below).~> 5.0would exclude the repo's own locks and every current customer.main.tffiles. It keeps the existing floors and adds a ceiling below the next major: aws>= 5.0, < 7.0, google>= 5.0, < 9.0, http>= 3.4, < 4.0, random>= 3.0, < 4.0. That covers the "unbounded" half of this issue. This PR changes nomain.tfline, so the two PRs don't conflict.The floors are already the same across bundles for every provider that more than one bundle uses: aws 5.0 in both AWS bundles, google 5.0 in both GCP bundles, and http 3.4 in all five. What was missing is a guard that keeps them that way, plus the README note.
Fix
internal/api/federation_bundle_providers.go(new): a strict line parser for therequired_providersblock of a bundle's embeddedmain.tf. Any line it doesn't recognize is an error, so a layout change fails loudly instead of dropping a provider.bundleProviderReadmeSectionadds a "Provider versions" section to the bundleREADME.txt. It lists each provider's range read from the shippedmain.tf, so the README can't drift from the constraints. It also says the bundle ships without a lock file, and that to move to a newer provider line a customer copies the new.tffiles over the old ones, keeps their state, backend configuration and lock file, then runsterraform init -upgradeand reviewsplan.internal/api/handler_federation.go:buildFederationBundleappends that section toREADME.txt. That's a 5-line change.internal/api/federation_bundle_providers_test.go(new):TestFederationBundles_ProviderBoundsAgreeAcrossBundlesrenders all five bundles throughgetFederationIaC(the real API path), parses the renderedterraform/main.tfand fails if two bundles declare a different floor or ceiling for the same provider source. Equivalent forms are normalized:~> 4.0equals>= 4.0, < 5.0, and5equals5.0.0. Because it compares ceilings too, it also catches a partial rollout of fix(iac): bound every Terraform provider constraint below the next major #1203's ceilings.TestProviderBoundsDisagreements_DetectsDivergencechecks that the guard reports a mismatched floor and a ceiling present in only one bundle, and doesn't report aligned bundles, equivalent spellings, or a provider used by only one bundle.TestConstraintBoundsandTestParseRequiredProviderscover the normalizer (including~> 5having no ceiling, as in hashicorp/go-version) and the parser's errors (inline entries, missing version, unknown attribute, unterminated block, no block).TestFederationBundleReadme_ListsProviderConstraintschecks that every rendered README lists each provider range from its renderedmain.tfand gives the upgrade steps.Refs #55
On its own, this PR is only part of the fix for #55: the bundles stay unbounded until #1203 merges. Once both are merged, the floors are aligned and enforced, the ceilings are in place and the README note ships, so the issue can be shut then. #1203's body currently says it leaves #55 open. Whichever of the two merges second can shut the issue.
Verification
All of this is unit-level and fixture-level evidence on Linux. No cloud API was called, nothing was planned or applied, and
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=truewas set throughout. Go 1.26.9, Terraform 1.10.5.Fail before / pass after:
git archive origin/maintree without thehandler_federation.gochange.TestFederationBundleReadme_ListsProviderConstraintsthen FAILS for all five bundles (... does not contain "Provider versions"). On this branch it passes.TestFederationBundles_ProviderBoundsAgreeAcrossBundlespasses on current main too, because the floors already agree there. It's a guard, not a regression test. These mutation probes show it can fail (each applied alone to a copy of this branch):awschanged to>= 6.0: FAILhashicorp/aws has 2 different bounds across bundles: floor=">= 5.0.0" ...: aws-cross-account (>= 5.0); floor=">= 6.0.0" ...: aws-target (>= 6.0)httpchanged to>= 3.4, < 4.0, the other four left as they are (a partial fix(iac): bound every Terraform provider constraint below the next major #1203): FAILhashicorp/http has 2 different bounds across bundles: ... ceiling="< 4.0.0": azure-target (>= 3.4, < 4.0)Combined with #1203 and our unopened
fix/issue-153:git merge-treeof this branch with fix(iac): bound every Terraform provider constraint below the next major #1203's headdcdd2fa0and withfix/issue-153(87290742) is clean in both cases. The changed files don't overlap: this PR touchesinternal/api/only, fix(iac): bound every Terraform provider constraint below the next major #1203 touchesterraform/andiac/federation/*/main.tf, and fix/issue-153 touchesscripts/only.go test ./internal/api/ -run 'TestFederationBundle|TestProviderBounds|TestConstraintBounds|TestParseRequiredProviders|TestGetFederationIaC',go test ./scripts/andgo test ./terraform/(fix(iac): bound every Terraform provider constraint below the next major #1203's guard) all pass. Under fix(iac): bound every Terraform provider constraint below the next major #1203 the README shows, for example,hashicorp/google >= 5.0, < 9.0andhashicorp/http >= 3.4, < 4.0.Rendered bundles, Terraform: I dumped all five bundles from
getFederationIaCon this branch and on the #1203-combined tree. With a throwawayTF_DATA_DIRand plugin cache, I ranterraform fmt -check,terraform init -backend=falseandterraform validatein each renderedterraform/directory:Success! The configuration is valid.on both trees. A fresh init with no lock resolved aws 6.70.0, google 8.8.0, http 3.6.3 and random 3.9.1, which confirms that no lock ships in the zip.initfails on both trees and on main (pre-existing, see below). Withterraform/modules/iam/azure/cudly-reservation-rolestaged at the relative path the bundle expects, validate succeeds (azuread 3.10.0, azurerm 4.81.0, http 3.6.3).fmt -check: every.tffile is clean. The one finding is pre-existing, in the renderedgcp-target*.auto.tfvars(column alignment ininternal/iacfiles/templates/gcp-wif.tfvars.tmpl). It's cosmetic and not touched here.Go and repo checks (this branch):
go build ./...,go vet ./...andgo vet -tags integration ./...: cleango test ./internal/api/ -count=1: okgo test ./internal/mocks/ -count=1: okrun --allow-parallel-runners ./internal/api/, with and without--build-tags integration: 0 issuesgocyclo -over 10 -ignore "_test\.go"onfederation_bundle_providers.goandhandler_federation.go: silentCombined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; 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:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/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 / left for an owner decision
>= 6.0, google>= 7.0) would give the "deliberate floor" the issue describes, but it edits the same lines as fix(iac): bound every Terraform provider constraint below the next major #1203. It also forces any customer still locked on aws 5.x or google 5.x/6.x to upgrade on their nextinit. If you want that, it belongs in fix(iac): bound every Terraform provider constraint below the next major #1203 or a follow-up after it, and the guard here keeps the bundles in step either way.initas downloaded.main.tfcalls../../../../terraform/modules/iam/azure/cudly-reservation-role, butaddBundleTerraformzips onlyfederation/azure-target/terraform, so the module path doesn't resolve in the extracted zip (The directory could not be read for module "cudly_reservation_role" at main.tf:69). This is unrelated to version pinning and is also noted in fix(iac): bound every Terraform provider constraint below the next major #1203. It deserves its own issue.gcp-targettfvars isn'tterraform fmt-clean (see above).scripts/generate-federation-iac.gobuilds a bundle with no README at all, so the provider note is API-only. That script is being changed on ourfix/issue-153branch, and I left it alone to avoid conflicts.terraform/environments/aws(~> 5) versus itsci-cd-permissions(~> 6).Frontend follow-up for the UI owner
None. This PR has no UI impact.
Maintainers: please mirror the issue's triage labels onto this PR:
type/chore,triaged,priority/p3,severity/low,urgency/eventually,impact/few,effort/s.🤖 Generated with Claude Code