feat(ec2): add subnetNameTagStrategy to tag subnets after their AZ - #2076
Conversation
Review Verdict: Comment OnlyAdds a new public schema surface (subnetNaming enum + Vpc input) generated into five language SDKs — a one-way door on the public contract, so a maintainer should own it. Implementation is clean: schema regeneration is byte-identical, Legacy remains the default with zero behavior change, and aliases correctly make opt-in a rename rather than a replace. Only two non-blocking nits found. Automated low-risk assessment, not a human review. View session · Was this review helpful? Yes · No |
Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
There was a problem hiding this comment.
Reviewed the subnetNaming addition across correctness, tests, AGENTS.md/REVIEW.md compliance, and AWS service facts.
The implementation is solid and the direction is safe: Legacy stays the default, the guard/allocator wiring threads the strategy through all four allocators, and opting in is a rename rather than a delete-and-recreate because every renamed resource (subnet, route table, association, route) carries an alias back to its index-based name. I confirmed the alias approach is sound given Pulumi URN semantics (URNs embed the parent type chain but not parent names, so each child's name-only alias resolves to the correct old URN even though its parent subnet is also renamed). The CIDR allocation key is deliberately pinned to the index-based name, so address ranges don't move when opting in, and extractSubnetSpecInputFromLegacyLayout reads legacySubnetName to keep the derived layout identical under either naming.
Verification I ran: regenerated schema.json from provider/pkg/schemagen/ec2.go — byte-identical to the committed file, so the schema and all five SDK diffs are clean regenerations, purely additive (new optional plain enum arg), not a breaking change. tsc is clean and the ec2 test suites pass (388 tests locally). The AZ-suffix regex was validated against real AZ/Local Zone/Wavelength/GovCloud/China/ISO shapes across partitions; within-region uniqueness holds by construction, and the fallback path is guarded by validateAzSuffixes.
No correctness or security defects found. Two non-blocking nits are inline. Withholding approval only because this adds a new public schema surface (subnetNaming enum + Vpc input) — a one-way door for external consumers across five language SDKs — so a maintainer should own that contract. Required CI checks were still in progress at review time (none failing); this is an automated assessment, not a human review.
| const assignGeneratedIpv6CidrBlock = args.assignGeneratedIpv6CidrBlock ?? false; | ||
|
|
||
| if ((args.subnetNaming ?? "Legacy") === "AvailabilityZone") { | ||
| validateAzSuffixes(availabilityZones); |
There was a problem hiding this comment.
Nit — tests
The component-level wiring of this guard is untested. validateAzSuffixes has unit tests for its throw path (subnetNaming.test.ts), but no vpc.test.ts case proves that Vpc actually calls it when subnetNaming="AvailabilityZone" (and skips it under the default). A regression that dropped this call, or ran it under Legacy, would not be caught.
A single component test — e.g. new Vpc(..., { subnetNaming: "AvailabilityZone", availabilityZoneNames: ["us-east-1a", "1a"] }) expecting a throw — would close the gap.
e4857ae to
565f4ea
Compare
| @@ -217,7 +227,7 @@ export class Vpc extends schema.Vpc<VpcData> { | |||
There was a problem hiding this comment.
@jkodroff is your primary use case for this change visibility in the AWS console? If so then a simpler approach would be to just change the Name tag. This eliminates aliases, one-way migration behavior, state adoption risk, and downstream changes based on URNs. The change becomes reversible and AWS updates the tags in place.
There was a problem hiding this comment.
Good catch. (and that's me talking, not Claude)
Not intentional to change any resource names - just the Name tag. Will fix.
Generated subnets are named after the 1-based index of their availability zone: vpc-public-1, vpc-public-2, vpc-public-3. Naming them after the AZ itself - vpc-public-1a, vpc-public-1b, vpc-public-1c - is more useful when reading the AWS console. Add a `subnetNaming` strategy to VpcArgs, mirroring the existing `subnetStrategy`, defaulting to `Legacy` so nothing changes for anyone who does not opt in: Legacy (default) Name tag = vpc-public-1 AvailabilityZone Name tag = vpc-public-1a `subnetNaming` only changes the AWS `Name` tag on the subnet and its route table. The Pulumi resource names - of the subnet, its route table, route table association and routes - stay index-based (legacySubnetName), because a resource name is its identity and renaming a subnet would delete/recreate it, which fails while ENIs are attached. Keeping the resource name stable means opting in is a pure tag change: no aliases, no replacement. Every allocator honours it - Legacy, Auto, AutoMerge and Exact - since subnetStrategy still defaults to Legacy and most users are on that path. Every spec carries both names: `subnetName` (the strategy-dependent Name tag) and `legacySubnetName` (the index-based resource name). The latter also backs extractSubnetSpecInputFromLegacyLayout, which recovers a spec name by stripping the trailing index so the derived subnetLayout is identical under either naming. The CIDR allocation table is keyed off a name, so it is deliberately pinned to the index-based form: the address ranges a user gets must not depend on what their subnets are called. Also gitignore .claude/settings.local.json and capture subnet-naming session learnings in the AI harness docs. Fixes #913 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
565f4ea to
6eca153
Compare
corymhall
left a comment
There was a problem hiding this comment.
Looks good! I just have a couple of small naming updates I think we should make now that we are only updating the name tag and not the resource name.
| }, | ||
| }, | ||
| "subnetNaming": { | ||
| Description: "The strategy to use when naming the VPC's subnets and their associated route " + |
There was a problem hiding this comment.
I think we should update this to something like:
Controls the AWS Name tags applied to generated subnets and their associated route tables. Pulumi logical resource names and URNs are unchanged. Optional; defaults to Legacy.
There was a problem hiding this comment.
Done, used your wording nearly verbatim.
| // The AWS "Name" tag applied to the subnet and the resources named after it. Under | ||
| // subnetNaming="AvailabilityZone" this is the AZ-suffixed form, e.g. "vpc-public-1a". | ||
| subnetName: string; | ||
| // The index-based name (e.g. "vpc-public-1"), always the subnetNaming="Legacy" form regardless of | ||
| // the naming strategy in effect. This is the Pulumi resource name for the subnet and its route | ||
| // table/association/routes, so subnetNaming never changes a child's identity - only the Name tag. | ||
| // extractSubnetSpecInputFromLegacyLayout also depends on it to recover a spec name by stripping | ||
| // the index. | ||
| legacySubnetName: string; |
There was a problem hiding this comment.
I think we should update the names of these properties now. Maybe resourceName and nameTag?
There was a problem hiding this comment.
Done: subnetName → nameTag and legacySubnetName → resourceName, threaded through both distributors, vpc.ts and the tests.
One knock-on worth flagging: OverlappingSubnet.subnetName also became resourceName, since specs get passed to it structurally. The index-based name is the right identifier for those overlap/gap error messages anyway, and it keeps them identical under either strategy.
| Plain: true, | ||
| }, | ||
| }, | ||
| "subnetNaming": { |
There was a problem hiding this comment.
I think we should update this to be something like subnetNameTagStrategy to make it clear that this is only changing the name tag.
There was a problem hiding this comment.
Done — renamed to subnetNameTagStrategy, and renamed the enum type to SubnetNameTagStrategy to match. Also reworded the two enum value descriptions, which still said "suffix each subnet with…" rather than "suffix each Name tag with…".
Review feedback: the option only changes the AWS Name tag, so the name should say so. Renames the Vpc input and its enum type, and reworks the descriptions that still implied subnets themselves were being renamed. Also renames the internal SubnetSpec fields to match what they are: subnetName -> nameTag, legacySubnetName -> resourceName. OverlappingSubnet follows, since specs are passed to it structurally; the index-based name is the right identifier for those error messages and keeps them unchanged under either strategy. Adds a component test that validateAzSuffixes is wired up, and only under the opt-in strategy. It spies on the guard rather than asserting the throw: a throw inside initialize() surfaces as an unhandled rejection out of the ComponentResource constructor, which no caller can catch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This PR has been shipped in release v3.8.0. |
Fixes #913.
Subnets are currently tagged after the 1-based index of their AZ —
vpc-public-1,vpc-public-2,vpc-public-3. Naming them after the AZ itself is more useful when you're reading the console. This adds an opt-in way to do that.subnetNameTagStrategyNametagsLegacy(default)vpc-public-1,vpc-public-2,vpc-public-3AvailabilityZonevpc-public-1a,vpc-public-1b,vpc-public-1cWhy this shape
This only changes the AWS
Nametag. Pulumi logical resource names and URNs are untouched.That's the whole design constraint. A child's Pulumi resource name is its identity: the generated name is the logical name for the subnet and for the route table, route table association and routes hanging off it. Changing it would change four URNs per subnet, so Pulumi would delete and recreate every subnet in every existing VPC — which in practice doesn't even complete, because the delete fails while ENIs are attached. Tagging is an in-place update instead, and it's reversible.
Concretely, every spec carries two names:
resourceName— the index-based name (vpc-public-1). Always used as the Pulumi resource name for the subnet and its route table, association and routes, under either strategy.nameTag— the strategy-dependent AWSNametag. The only thingsubnetNameTagStrategyaffects.Because no resource is ever renamed, no aliases are needed anywhere, and opting in (or backing out) is a tag update rather than a state migration.
It's an opt-in strategy enum defaulting to
Legacy, mirroring the existingsubnetStrategy, and orthogonal to it: all four allocators (Legacy,Auto,AutoMerge,Exact) honor it. That last part matters —subnetStrategystill defaults toLegacy, so a feature that only worked underAutowould be invisible to most of the install base.Two smaller things worth calling out:
The CIDR allocation table is keyed off a name. It's deliberately pinned to the index-based form (
subnetAllocationID), so the address ranges a user gets can't depend on what their subnets are tagged. There's a test asserting the allocated blocks are byte-identical under either strategy.extractSubnetSpecInputFromLegacyLayoutreadsresourceName, notnameTag. It recovers a spec name by stripping a trailing index, and-1adoesn't match\d+$. Reading the resource name keeps the derivedsubnetLayoutoutput identical under either strategy, with no regex change.Anything doing tag-based subnet lookup will of course notice the new tag values — that's the point of the feature, but it's the one user-visible effect to be aware of when opting in.
AZ suffix
us-east-1a→1a, by stripping the region prefix (/^[a-z]{2}(?:-[a-z]+)+-/) rather than pattern-matching the tail. That handles every AZ shape AWS hands out — GovCloud (us-gov-west-1a→1a), China, the ISO partitions (us-isob-east-1b→1b), Local Zones (us-west-2-lax-1a→2-lax-1a) and Wavelength Zones, which end in a digit rather than a letter (us-east-1-wl1-bos-wlz-1→1-wl1-bos-wlz-1).Because every AZ in a region shares that prefix exactly, stripping it preserves uniqueness — two AZs in one region can't reduce to the same suffix. A tail-matching rule would have collapsed
us-west-2-lax-1aandus-west-2-den-1aboth to1a. There's avalidateAzSuffixesguard for the fallback path anyway.Tests
awsx/ec2/subnetNaming.test.ts(new) covers the suffix rule against a corpus of every AZ shape across ~25 regions and partitions, asserting name validity and per-region uniqueness for all of them, plus a fast-check property over generated AZ names.awsx/ec2/vpc.test.tscovers the allocators (all four honor the strategy; default is unchanged; CIDR blocks andsubnetLayoutidentical under either strategy) and the component itself under mocks: that the subnets, route tables, route table associations and routes all keep their index-based resource names under both strategies, that only theNametags on the subnets and route tables change, that no resource gains an alias, and that NAT gateways and EIPs are untouched.The component tests use
us-west-2, so the expected suffixes are2a/2b/2c— a bug that hardcoded1or dropped the region number would still have passed inus-east-1.The
validateAzSuffixesguard is covered at the component level by spying on it (called with the AZ list underAvailabilityZone, not called under the default) rather than by asserting the throw. A throw insideinitialize()surfaces as an unhandled rejection out of theComponentResourceconstructor that no caller can catch — it crashes the Jest worker rather than failing a test. The guard's own throw path is unit-tested insubnetNaming.test.ts.443 tests pass;
tscand prettier clean. (yarn --cwd awsx lintfails on a pre-existingno-unused-expressioninvpc.ts, unchanged frommaster.)🤖 Generated with Claude Code