Repository navigation
fix(migrate): backfill credential secret ARNs v0 didn't record - #2600
Conversation
v0 never records clientSecretArn for API-key credential providers, and v1's CDK app needs it to let a Gateway read the secret, so migrate failed at v1 synth for any v0 project with an authenticated Gateway target. Migrate now looks up each missing secret ARN with the selected target's credentials and records it in the synth workspace and the migrated deployed-state.json. Closes aws#2599
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice fix. The backfill logic correctly scopes the "blocking" vs "warning" distinction to Gateway-referenced credentials, the error paths cover ResourceNotFoundException, no-stored-secret, and generic API failures with actionable messages, and the Windows path separator is normalized in the check message.
A few things I verified while reviewing (no changes requested):
kinds.get(name)filter correctly skipsPaymentCredentialProviderand stale credentials no longer in the spec, and the authorizer strings (ApiKeyCredentialProvider/OAuthCredentialProvider) matchsrc/projectSchemas/v0/schemas/deployed-state.tsand the v1 credential writer insrc/core/project/backends/cdk/credentials.ts.gatewayCredentialNamesonly matchesAPI_KEYandOAUTHoutbound auth types, which matchesOutboundAuthTypeSchemaand the stated invariant that these are the only types requiring a secret ARN at synth time.- When
blocking.length > 0the returneddeployedStatestill contains partially-filled entries, but because the plan reportsready: false, apply never runs and the on-disk file isn't overwritten — so re-runningagentcore migratewill re-fetch cleanly. - The pending-run path in
index.ts(around line 609) parses the already-written v1 spec loosely but only usescredentialsto look upauthorizerType; worst case the backfill is a no-op if the spec is malformed, which is acceptable. - Tests use real temp directories and only fake the Identity client at the SDK boundary — no excessive mocking.
- No new telemetry needed; this is a bugfix within an existing feature that doesn't currently emit telemetry.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2600 +/- ##
============================================
+ Coverage 96.57% 96.58% +0.01%
============================================
Files 699 700 +1
Lines 57427 57630 +203
============================================
+ Hits 55462 55664 +202
- Misses 1965 1966 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jariy17
left a comment
There was a problem hiding this comment.
Multi-target projects with an authenticated Gateway still fail v1 synth. Details inline.
| const statePath = join(input.rootPath, DEPLOYED_STATE_RELATIVE_PATH); | ||
| // Read as written rather than through the schema, so what isn't patched is kept as is. | ||
| const state = JSON.parse(await readFile(statePath, "utf8")) as LooseDeployedState; | ||
| const recorded = state.targets?.[input.target]?.resources?.credentials ?? {}; |
There was a problem hiding this comment.
This only backfills the selected target. The vended CDK app synthesizes a stack for every target, so a multi-target project with an authenticated Gateway still fails v1 synth.
Repro
- v0 project with targets
defaultandprod, both deployed. A Gateway target usesAPI_KEYorOAUTHoutbound auth. agentcore migrate --target defaultfills intargets.defaultonly.synthCommandrunscdk synthwith no stack selector.cdk.tsloops overresolveTargetStacks, which maps every target.- The
prodstack throws:Credential "<name>" has no clientSecretArn in deployed state(AgentCoreGateway.js:556,@aws/agentcore-cdk@1.0.0-rc.3).
deploy --target default and build hit the same error after migrating, until prod gets its own v1 deploy.
Scope: only Gateway outbound auth. Payment connectors and harness skill git auth also read deployed-state credentials, but they only need credentialProviderArn. v0 records that.
Suggested fix
- Backfill every deployed target in the same account and region as the selected one. Provider names are unique there, so the same lookup works.
- For targets in another account or region, fail the check with a clear message.
- Add a two-target test with a synth fake that rejects a Gateway credential missing
clientSecretArn.
I traced this through the code. I didn't run a real two-target synth.
There was a problem hiding this comment.
Good catch, thanks. The single synth covers every target, so backfilling only the selected one wasn't enough. Fixed in 40929b7:
- Migrate now looks up missing secret ARNs for every deployed target in the same account as the active credentials, across regions (one set of credentials reaches any region of its own account), resolving credentials once per region. The written
deployed-state.json(and the pending path's synth workspace) gets the ARNs for all of them. - A target in another account whose Gateway credentials lack an ARN now fails the
credential secretscheck up front, naming the target, its account/region and the active account, with the manualget-*-credential-providersteps, since migrate reads every target with one set of credentials. If nothing is missing there, the plan stays ready; unreferenced credentials still only warn. - Tests: the two-target test you suggested, with a synth fake that throws like the CDK does when any target's Gateway credential lacks
clientSecretArn(same account, two regions → ready, both patched), plus cross-account blocking and nothing-missing cases. I also replaced the earlier test that assertedprodstays unpatched.
nborges-aws
left a comment
There was a problem hiding this comment.
no findings from me; aside from TJs comment which I agree on. Can rereview once PR updated
The vended CDK app synthesizes a stack for every target, so backfilling only the selected target still failed v1 synth (and build/deploy after migrating) for a multi-target project with an authenticated Gateway. Look up missing secret ARNs for every deployed target the active credentials' account owns, resolving credentials once per region, and fail the check with recovery steps for a target in another account whose Gateway credentials lack one.
|
Claude Security Review: no high-confidence findings. (run) |
nborges-aws
left a comment
There was a problem hiding this comment.
LGTM after fixes. Thanks!
Description
v0 never records
clientSecretArnfor API-key credential providers, and doesn't always for OAuth ones, but v1's CDK app needs that ARN to let a Gateway read the secret. Soagentcore migratefailed atv1 synthfor any v0 project with an authenticated Gateway target.Migrate now looks up each missing secret ARN with
GetApiKeyCredentialProvider/GetOauth2CredentialProvider, using the selected target's credentials and region, and writes it into the synth workspace. On a fresh migration it also writes it into the project'sagentcore/.cli/deployed-state.json, so a post-migrationagentcore buildworks before the first deploy.credential secretscheck reports what was found:found the secret ARNs v0 didn't record for X, Y; migrating records them in agentcore/.cli/deployed-state.json.Known limitation:
agentcore/.cliis gitignored, so the git gate can't see a v0 deploy that happens between plan and apply. Apply would then overwritedeployed-state.jsonwith the planned copy.Related Issue
Closes #2599
Documentation PR
Not applicable.
Type of Change
Testing
How have you tested the change?
bun test(4567 pass, 0 fail)bun run test:e2e, or explained why they are not applicable. I didn't run the e2e suite. Instead I ran it end to end against real AWS: a v0 0.31.1 project (LiteLLM Harness, Memory, IAM Gateway with an OpenAPI target on an API-key credential, two API-key credentials) went frommigrate(ready, 6 logical-ID moves, 0 replacements), through the first v1 deploy, a prompt update and redeploy, and adding then removing a runtime. Physical IDs, provider ARNs and memory records matched the v0 baseline throughout.bun run typecheckbun run lint:checkbun run format:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed them (not applicable)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.