Skip to content

fix(iac): one bucket policy per bucket, fallback LF ARNs name the account, generate iac calls no AWS API - #684

Merged
fas89 merged 3 commits into
mainfrom
fix/iac-offline-generate-lf-arns-bucket-policy
Sep 28, 2026
Merged

fas89 merged 3 commits into
mainfrom
fix/iac-offline-generate-lf-arns-bucket-policy

Conversation

@fas89

@fas89 fas89 commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Three defects on main (736494c), found on 28 Sept 2026 while fixing the examples for #674. This PR is kept to fluid_build/iac/providers/aws.py, fluid_build/cli/generate_iac.py, one error-catalog entry and tests, so it merges beside #674: a trial git merge of #674 onto this branch is conflict-free, and on the merged tree 421 tests across the examples e2e, LF, column-restriction, cross-account, packaging and apply-engine files pass (41 skipped). No example, workflow or CHANGELOG change.

1. Lake Formation ARNs on the fallback bucket carried an unresolved account

Evidence (main, tofu plan against moto). With location.bucket: "{{ env.DATA_LAKE_BUCKET }}" unset, the bucket falls back to <account>-fluid-data. The Lake Formation location and bucket-policy ARNs were plain strings, so the renderer escaped the account token to $${...}, a literal:

aws_lakeformation_resource.probe_lf_fallback_lf_loc_... ['create']
  {'arn': 'arn:aws:s3:::${data.aws_caller_identity.fluid_lf_caller.account_id}-fluid-data/curated/orders/'}

That is the planned value, i.e. registerLocation would register the ARN as written. With an arn: grantee the plan did not get that far, because the bucket policy referenced an aws_s3_bucket keyed off the token that the module never declares:

Error: Reference to undeclared resource
  "bucket": "${aws_s3_bucket.lf_plan___data_aws_caller_identity_fluid_lf_caller_account_id__fluid_data.id}"
There is no managed resource "aws_s3_bucket" "lf_plan___data_aws_caller_identity_..." definition in the root module.

Fix. _lf_s3_arn builds the LF location ARN and the policy's bucket and object ARNs. On the fallback it returns a TofuExpr with the contract-derived suffix escaped first, the same way _emit_glue already builds the table's location. The fallback is decided on the raw input (bucket_uses_fallback), so a contract that writes the token itself still gets a literal (tested). On the fallback the policy's bucket is the fallback bucket's name, interpolated, not a reference to an undeclared resource. In all-grantees mode the static document becomes a TofuExpr there, with principals and prefixes escaped before encoding. Any other bucket emits exactly what it did before.

I did not fail closed here. tests/iac/test_iac_packaging_env_bucket.py pins that an owned or LEGACY bucket falls back to <account>-fluid-data ("the fallback is the point there"). Only a pool bucket fails closed (shared-bucket-unresolved), and that stays as it is.

Proof (tofu plan vs moto, account 123456789012). Planned aws_lakeformation_resource.arn = arn:aws:s3:::123456789012-fluid-data/orders/. The planned aws_s3_bucket_policy has bucket = 123456789012-fluid-data and its statements' resources are arn:aws:s3:::123456789012-fluid-data and .../orders/*. Both cross-account and all-grantees are covered (TestTheFallbackBucketAtPlanTime).

2. Two exposes on one bucket: the second bucket policy replaced the first

Evidence. Every expose emitted the bucket's aws_s3_bucket_policy, its aws_iam_policy_document and its aws_arn grantees under the same keys, so the last expose on a bucket won. I planned #674's examples/aws-medallion-lake against moto, with the grantees' account (AWS_ACCOUNT_ID=222222222222) different from moto's so they count as cross-account. GetObject statements in the planned policy:

On main, the new synthetic plan test shows the same loss: only FluidLfBucketGet0 on lf-plan-lake/curated/* is planned.

Fix. _lf_bucket_policies merges each bucket's exposes, in contract order, into one policy, and emit() writes one aws_s3_bucket_policy per bucket. S3 allows one policy per bucket, and the hashicorp/aws docs warn that a second aws_s3_bucket_policy on the same bucket silently overrides the first. Grantees are numbered across the whole policy, so Sids and data.aws_arn keys stay unique. The provider's aws_iam_policy_document rejects a duplicate Sid. In cross-account mode each expose gets its own pair of dynamic statements, scoped to its own prefix and filtered over its own grantees (tostring(i + N)). The resource's count, and the product KMS key's reader statement, use a merge() of those maps. A bucket with one expose gets the same bytes as before: the existing byte pins in test_iac_lakeformation_bucket_policy.py and test_iac_packaging_default_pin.py pass unchanged. If two exposes ask one bucket for different bucketPolicy modes (cross-account and all-grantees), the emit refuses (lakeformation-bucket-policy). An expose with none adds no statements and does not remove the others'.

Proof (tofu plan vs moto). In TestOneBucketTwoExposesAtPlanTime, one planned instance, aws_s3_bucket_policy.lf_plan_lf_bucket_policy_lf_plan_lake[0], carries FluidLfBucketList1/Get1 for raw/* and FluidLfBucketList2/Get2 for curated/*, with the same-account grantee filtered out. all-grantees plans all four statements. Two same-account zones plan no policy.

3. fluid generate iac called sts:GetCallerIdentity

Evidence. I ran fluid generate iac examples/aws-medallion-lake/contract.fluid.yaml with AWS_ACCOUNT_ID unset and a spy on client construction. It built one sts client:

generate_iac.run -> native_actions -> _common.build_provider -> AwsProvider.__init__
  -> util/config.py resolve_account_and_region -> boto3.client("sts") ... get_caller_identity()

The account it finds, from whatever credentials the machine has, then goes into the ARNs of the planned orchestration resources. The examples' READMEs say generate iac needs no AWS account or credentials.

Fix. native_actions(contract, logger, *, offline=False). generate iac (plain and --shadow) passes offline=True. With AWS as the provider and no AWS_ACCOUNT_ID, the planner then gets the placeholder account fluid-aws-account-id-unset, so no boto3 client is built. The planner still runs, including the sovereignty check that only it enforces for AWS. The emitter's own account references were already data.aws_caller_identity lookups made at plan time. Only planner-built ARNs (Lambda, EventBridge, Step Functions) name the account. If one of those would reach the module, run() refuses before writing anything: generate_iac_aws_account_required, whose message and catalog suggestions name AWS_ACCOUNT_ID. fluid apply calls native_actions(contract, logger) and resolves the account exactly as before (pinned by a test).

One behaviour changes. Take a contract whose orchestration plans Lambda or EventBridge resources, run with AWS_ACCOUNT_ID unset. Before, a machine with credentials got those credentials' account written into the module. A machine without credentials got a module silently missing those resources: the planner failed its own check on the bucket name None-fluid-staging and the failure was logged at DEBUG. Now both are refused with the message above. With AWS_ACCOUNT_ID set, the output is unchanged. The shipped examples plan no such resources, so they emit as before.

Proof. tests/iac/test_iac_generate_offline.py replaces botocore's Session.create_client, which every boto3 client and resource goes through, with a recorder. It runs fluid generate iac on all eight examples/aws-* contracts, plain and --shadow, and on a Lake Formation contract on the fallback bucket, and asserts that no client is built and the placeholder never reaches a module. A scheduled contract is refused with nothing written. With AWS_ACCOUNT_ID set, the same contract emits role = arn:aws:iam::123456789012:role/fluid-workflow-execution.

Tests that fail without the fix (revert checks)

  • generate_iac.py reverted to main: 19 failed, 3 passed in test_iac_generate_offline.py. The failures are assert ['sts'] == [], or ['sts', 'sts'] for --shadow. The three that pass hold on main too: the examples-exist check, the AWS_ACCOUNT_ID-set case, and the apply-path pin. Seeding only the removal of the placeholder assignment gives the same 19 failures.
  • aws.py reverted to main: 18 failed, 29 passed across the LF unit and plan files. The plan failures are the undeclared-resource error above, and a two-zone policy left with only the curated/* statements. The new tests that pass on main are guards that hold either way: spoofed token, none expose, two buckets, the candidate-set equality, two same-account zones.
  • Seeding only the overwrite (bug 2): 8 failed, including both two-zone plan tests.
  • Seeding only a plain-string _lf_s3_arn (bug 1): 7 failed, including both fallback plan tests.

tests/iac/test_iac_packaging_default_pin.py: its three native_actions stubs now accept keyword arguments (lambda contract, logger, **_: []), because run() passes offline=True. Those lines are outside #674's hunk in that file.

Gates (local)

  • ruff check fluid_build/ tests/: pass. The dead-code gate (--select F401,F841,F811,B007): pass.
  • black --check fluid_build/ tests/ with black 24.10.0: 1767 files unchanged.
  • python scripts/mypy_strict.py: no issues in 7 source files.
  • lint-imports: 6 contracts kept, 0 broken.
  • python scripts/check_license_headers.py: pass.
  • detect-secrets-hook 1.5.0 with .secrets.baseline on the 8 changed files: exit 0.
  • Product Guardrail: 0 failing findings (13 warnings, none in the changed files).
  • pytest tests/iac tests/cli tests/test_aws_examples_e2e.py tests/providers/aws -p randomly --randomly-seed=1 -n 8, with tofu 1.12.0, moto 5.2.3, AWS_CONFIG_FILE=/dev/null and AWS_SHARED_CREDENTIALS_FILE=/dev/null: 3109 passed, 201 skipped, 1 xpassed, 0 failed.

The new plan tests are in test_iac_lakeformation_bucket_policy_plan.py, which iac-tests.yml Stage 1 already runs against moto and asserts did not only skip. The offline test is unit-marked, and .[dev,local] pulls boto3 in, so the main matrix runs it.

Prior art (borrow-before-build receipts)

  • hashicorp/aws aws_s3_bucket_policy docs (website/docs/r/s3_bucket_policy.html.markdown): define one aws_s3_bucket_policy per bucket, because the policy applied last silently overrides the previous one. Hence one merged policy per bucket, and no second resource.
  • hashicorp/aws aws_iam_policy_document source (internal/service/iam/policy_document_data_source.go): principals.identifiers is a TypeSet, and a repeated Sid is rejected ("duplicate Sid"). Hence grantees numbered across the policy, not per expose.
  • The Terraform/OpenTofu strings docs: $${ produces a literal ${. That escaping is what turned the fallback ARN into a literal. The fix follows the repo's own _emit_glue pattern: escape the contract-derived pieces, then wrap the emitter-built value in TofuExpr.
  • hashicorp/aws aws_caller_identity data source docs: account_id is the account the provider connection runs as, which is why the emitter already defers the account to plan time rather than asking STS at generate time.
  • boto3 boto3/session.py: Session.client returns self._session.create_client(...), and Session.resource calls client. Hence the guard on botocore's Session.create_client, which catches every boto3 client and resource.

Found, not fixed (separate change)

On the fallback, _emit_s3 still creates an aws_s3_bucket named literally {{ env.DATA_LAKE_BUCKET }}, from the raw contract value. That plans, and S3 would refuse it at apply. Meanwhile the Glue table, the LF location and now the bucket policy all target <account>-fluid-data. discover_imports also imports that bucket by the raw name.

…ount, generate iac calls no AWS API

Three defects found on main while fixing the examples for #674.

1. On the {account}-fluid-data fallback bucket (an unresolved
   {{ env.DATA_LAKE_BUCKET }}), the Lake Formation location and the
   bucket-policy ARNs were plain strings, so the renderer escaped the
   account token to $${...}, a literal: tofu plan registered
   arn:aws:s3:::${data.aws_caller_identity...}-fluid-data/... as written.
   The bucket policy also referenced an aws_s3_bucket the module never
   declares, so a plan with an ARN grantee failed "Reference to undeclared
   resource". The fallback ARNs are now TofuExpr with the contract-derived
   suffix escaped (the way _emit_glue builds the table location), decided
   on the raw input, and the policy addresses the fallback bucket by name.

2. Every expose emitted the bucket's aws_s3_bucket_policy under the same
   key, so a second expose on the bucket replaced the first: a grantee in
   another account lost s3:GetObject on the first prefix. The exposes on a
   bucket are now merged into one policy, grantees numbered across it so
   Sids and data.aws_arn keys stay unique. A one-expose bucket gets the
   same policy as before, byte for byte. Two exposes asking one bucket for
   different bucketPolicy modes are refused.

3. fluid generate iac built the native AWS provider, which resolves a
   missing account with sts:GetCallerIdentity over whatever credentials
   the machine has. generate iac now gives the planner a placeholder
   account when AWS_ACCOUNT_ID is unset, so no boto3 client is built and
   the sovereignty check still runs, and refuses a module that would carry
   the placeholder (generate_iac_aws_account_required, naming
   AWS_ACCOUNT_ID). fluid apply keeps resolving the account as before.

Proved by tofu plan against moto (tests/iac/test_iac_lakeformation_bucket_policy_plan.py)
and by a guard on botocore's Session.create_client
(tests/iac/test_iac_generate_offline.py).
@github-actions github-actions Bot added cli Changes to the CLI surface or implementation tests Test coverage or test infrastructure changes needs-docs Pull request needs a linked docs update or justification labels Sep 28, 2026
@github-actions

Copy link
Copy Markdown

📄 Documentation Reminder

This PR appears to be missing a documentation reference. Our docs live in a separate repo.

Please update the PR description with one of:

  • Link a docs PR — check the "Docs PR linked" box and paste the URL
  • Mark as no docs needed — check "No docs needed" with a justification
  • Acknowledge docs TODO — check "Docs TODO" and create the docs PR before merge

See the Contributing Guide for details.

Comment thread fluid_build/_error_catalog.py Fixed
@fas89
fas89 merged commit 22e1289 into main Sep 28, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Changes to the CLI surface or implementation needs-docs Pull request needs a linked docs update or justification tests Test coverage or test infrastructure changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant