Skip to content

Place resources correctly when subnet ids are only known at apply time - #230

Open
patrickchugh wants to merge 1 commit into
mainfrom
fix/subnet-instance-placement
Open

patrickchugh wants to merge 1 commit into
mainfrom
fix/subnet-instance-placement

Conversation

@patrickchugh

Copy link
Copy Markdown
Owner

Type of Change

  • Bug Fix
  • New Feature
  • Refactor
  • Documentation

What and why

TerraVision plans Terraform against empty local state, so no subnet has an id at plan time. Three places mishandled that, and together they made any environment whose subnets come from count or for_each draw wrongly: an autoscaling group was copied into every subnet of the VPC, and a database behind a DB subnet group was drawn outside the VPC.

  1. Autoscaling groups expanded into every subnet. expand_autoscaling_groups_to_subnets in modules/resource_handlers_aws.py matched subnets by id and treated an unknown id as a wildcard. It now expands into the subnets the vpc_zone_identifier expression names (aws_subnet.private[*].id, [for s in aws_subnet.private : s.id], or an explicit list), and falls back to the old id matching only when no subnet resource is named, for example a data source.
  2. A for expression over a whole resource was dropped as ambiguous. _disambiguate_instances in modules/graphmaker.py already kept every instance for a splat, but [for s in aws_subnet.this : s.id], the only way to list for_each instances, was rejected with "ambiguous for_each reference, connection omitted". Both are one-to-many and are now treated alike. A loop over one instance (aws_subnet.this[each.key]) is still not treated as one-to-many.
  3. DB subnet groups were unlinked from the VPC. move_to_vpc_parent in modules/resource_transformers.py runs before availability zone boxes are inserted, so it looked for an AZ between subnet and VPC, found none, and had already cut the subnet link. It now accepts a VPC that is the subnet's direct parent. This also affects Lambda functions in a VPC, which use the same transformation.

The expected-secretsmanager-rds.json snapshot changes in one line: its DB subnet group is now a child of the VPC rather than floating outside it.

Verified on two minimal repros (count with splat, for_each with for expression) replayed from plan files, and on a larger dev/prod example: the autoscaling group now sits only in the private subnets, the ElastiCache cluster and DB subnet group are inside the VPC, and the warnings are gone.

Known remaining issue, not addressed here: an RDS instance whose security group is the only thing linking it to the VPC can still end up outside it. redirect_to_security_group correctly swaps the DB subnet group for the security group inside the VPC, but aws_handle_sg, which runs afterwards, deletes that security group without creating a per-member copy for the RDS instance. The old snapshot already showed this. It needs a separate change to the security group handler.

Tests:

  • tests/test_subnet_placement.py (new): autoscaling expansion for splat, for expression and explicit list; fallback when no subnet is named; module-prefixed nodes; move_to_vpc_parent with and without an AZ node.
  • tests/test_foreach_matching.py: for expression cases added to the one-to-many parametrisation, including a loop over one instance that must stay ambiguous.
  • Full suite (pytest tests, slow tests included): 2,294 passed, 1 failed. The failure is test_live_source for the wordpress_fargate example, which looks up an existing Route 53 zone with a data source that does not exist in the account the test ran against. It fails identically without this change.

Checklist

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written Documentation/Tests?
  • Have you done your own code-review?
  • Have you disclosed any use of AI tools and models with their version?

AI Assistance Declaration

  • Tools used: Claude Code
  • Model: Claude Fable 5.1
  • Scope: Diagnosed the three defects by tracing the handler chain on minimal repros, wrote the fixes and the tests, refreshed the one changed snapshot, and ran the full suite. Reviewed by the author.

Checklist for Changes to Core Features:

  • Have you discussed any major revamp with a reviewer/maintainer first? (It's okay to just raise a PR directly for minor bugfixes)
  • Have you ensured your PR is focused on one major improvement and is not trying to do too many changes at once?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable, and made sure the new tests PASS?
  • Have you successfully run all previous system wide tests with your changes locally?

🤖 Generated with Claude Code

TerraVision plans against empty local state, so no subnet has an id yet.
Three places mishandled that:

* expand_autoscaling_groups_to_subnets matched subnets by id and treated
  an unknown id as a wildcard, so an autoscaling group set to
  aws_subnet.private[*].id was copied into the public and data subnets
  too. It now expands into the subnets the expression names, and falls
  back to id matching only when no subnet resource is named.
* _disambiguate_instances kept every instance for a splat but dropped a
  for expression over the whole resource ([for s in aws_subnet.this :
  s.id], the only way to list for_each instances) as ambiguous. Both are
  one-to-many and are now treated alike.
* move_to_vpc_parent runs before availability zone boxes are inserted,
  so it looked for an AZ between subnet and VPC, found none, and left DB
  subnet groups and Lambda functions unlinked from both. It now accepts
  a VPC that is the subnet's direct parent. The secretsmanager-rds
  snapshot changes accordingly: its DB subnet group is now inside the
  VPC.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant