feat: create credential providers before synthesizing a deploy - #2123
feat: create credential providers before synthesizing a deploy#2123notgitika wants to merge 16 commits into
Conversation
GitHub Actions
Failure
agentcore-devx-automation
Bot
added
the
agentcore-harness-reviewing
![agentcore-devx-automation[bot]](https://avatars.githubusercontent.com/u/2232217?s=60&v=4){kind=small}
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice PR. The design of running credential provisioning before cdk synth and threading the ARNs through deployed-state.json is well-motivated and the comments do a great job of capturing why. The seam boundary (IdentityProviderClient) is drawn at the SDK client rather than at fs/process boundaries, so the tests avoid excessive mocking while still exercising the real spec + .env.local parsing paths. Sharing credentialEnvVarName/CLIENT_SECRET_SUFFIX between add and deploy via envLocal.ts (with a re-export from shared.ts) removes a latent format-drift bug.
A couple of small things that aren't blockers but worth confirming intentional:
-
Stale credential entries when the spec goes to zero credentials. In
src/core/project/backends/cdk.ts(~L115)updateTargetStateis only called whenObject.keys(provisioned).length > 0. If a user deletes their last credential fromagentcore.jsonand re-deploys,provisionedis{}, the state write is skipped, and the previousresources.credentialsmap is left on disk. The doc-comment onupdateTargetStatepromises "A resource map provided in the patch replaces the previous map for that kind wholesale, so a credential dropped from the spec stops being advertised" — that guarantee is only actually delivered when at least one credential remains. Since the synthesized CDK app looks up credentials by name, this is likely inert in practice, but if you want the drop-to-zero case to behave the same as drop-one-of-many, you'd either always callupdateTargetState({ resources: { credentials: provisioned } })or explicitly write{}whendeclaredis non-empty on the spec side but you provisioned nothing. -
parseEnvcast inEnvLocalFile.read(src/core/project/envLocal.tsL90):parseEnv's declared return type isRecord<string, string | undefined>(last-write-wins across duplicate keys), but you cast toRecord<string, string>. All callers happen to useif (!value)so undefined is handled safely today; just be aware the type is a small lie and a future caller doingenv[k].trim()would compile but crash.
Neither of these needs to block the merge.
agentcore-devx-automation
Bot
removed
the
agentcore-harness-reviewing
4b787a8 to
0db4266
Compare
GitHub Actions
Failure
The synthesized CDK app reads credential provider ARNs out of deployed-state.json and fails to synth a project that declares credentials until they exist. Provision them between the account preflight and the build, then record their ARNs via updateTargetState so the assembly is synthesized against a state file that already describes them. Providers are created when absent and reused when present, never updated, so a redeploy neither mints a new secret version nor overwrites one rotated outside the CLI. Payment credentials are rejected up front (agentcore.json can't express the vendor config they need). Secrets come from the same place 'project add credentials' writes them, so the env-var name is now derived from one function in envLocal.ts that both sides share.
0db4266 to
3c09c23
Compare
github-actions
Bot
added
size/l
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #2123 +/- ##
============================================
- Coverage 97.22% 97.10% -0.13%
============================================
Files 507 508 +1
Lines 33809 34269 +460
============================================
+ Hits 32872 33276 +404
- Misses 937 993 +56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
…v type - Add SDK-mocked coverage for createIdentityProviderClient (the real Identity factory the provisioner tests bypass): ~55% -> ~95% on credentials.ts. - Always record the provisioned credential set, so removing the last credential from the spec clears the stale entry instead of leaving it advertised. - EnvLocalFile.read returns Record<string, string | undefined> (parseEnv's real type) rather than casting it away. - Tighten a few verbose comments.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
| ? { name: credential.name, secretRef: credential.secretRef } | ||
| : { | ||
| name: credential.name, | ||
| apiKey: requireEnvSecret(credential.name, env, rootPath, "secretRef"), |
There was a problem hiding this comment.
I think we already have all the code we need to do this in the existing core package. Let's reuse that. We can discuss strategies for making that code reusable if it's not clear.
Deploy's credential provisioning built its own BedrockAgentCoreControlClient and sent Get/Create commands itself, duplicating the IdentityClient in src/core/identity.tsx that already backs the `agentcore identity` commands. It existed only because the credentials had nowhere to travel: control-plane clients were built from a ClientConfig that carried no credentials, while provisioning must run against the deployment target's own. - CoreOptions and ClientConfig now carry optional credentials, forwarded by toClientConfig, mirroring the CredentialedClientConfig the CloudFormation factory already uses. - CoreClient hands its IdentityClient to FsProjectManager, which passes it to CdkBackend; the provisioner takes CredentialProviderCalls, a four-method Pick of CoreIdentityClient, so tests fake four calls instead of ten. - cacheKey no longer keys clients by JSON.stringify alone: credentials are a provider function, which JSON.stringify drops, so two callers with different credentials in one region would have shared a cached client. They are keyed by object identity instead. - IdentityClient's dependency narrows to Pick<AwsClients, "control">, letting a project manager built outside CoreClient construct one from the existing createControlClient factory. The lazily imported SDK is gone with the duplicate client; the claim that it saved startup cost was already untrue, since core/factories.tsx imports the same client statically on every run. credentials.client.test.ts existed only to mock that import and is deleted; its response-mapping and not-found cases move into credentials.test.ts.
The existing collision check compares the .env.local variables the credentials in the spec write today, so it cannot catch a clash with a field the CLI no longer writes but still reads — an OAuth client id, which pre-0.29 projects keep in AGENTCORE_CREDENTIAL_<NAME>_CLIENT_ID — or with one a later credential type adds. An api-key credential named 'svc-client-id' therefore added cleanly and its key was then read as the client id of an OAuth credential named 'svc'. Names whose derived variable ends in a field suffix are now refused at add time, the way main's validateCredentialNameEncryptable refuses them. The check stays in the add flow rather than the schema so that a project already holding such a name keeps loading and can be repaired.
…ider Deploy reused an existing provider untouched, so editing a secret in .env.local and redeploying had no effect on AWS: the provider kept the value it was created with, and nothing said so. main's deploy updates instead — `// Always update to ensure provider has current credentials` — and losing that on the rewrite would be a silent regression for anyone rotating a key. A credential whose secret the CLI can see is now written on every deploy: created when the provider is absent, updated when it is present. A credential with no secret to offer — nothing in .env.local and no external reference — leaves an existing provider exactly as it is, so a project that provisioned once and no longer keeps the secret on disk still deploys; only an absent provider fails, as before.
Provisioning read agentcore/.env.local and nothing else, so a deploy from CI had to write its secrets to disk first. main reads every credential variable from process.env and merges it over the file, which is what makes a non-interactive deploy possible without persisting secrets. Variables carrying the credential prefix now override the file. The filter keeps the deploy from reading anything else out of the environment, and the provisioner takes the environment as an argument so tests do not mutate the process's own.
`project add credentials payment` and `add payment-connector` write a payment credential to the spec, but deploy refused to provision one — it threw and told the user to remove it — so a project could be assembled that added cleanly and then could not deploy at all. main provisions them, and the CDK construct that wires payment connectors already reads their ARNs out of the credentials map in deployed-state.json. - Core's Identity client gains the payment provider operations. The pinned SDK carries them, so unlike main there is no hand-signed HTTP request. - A payment credential is created when absent and updated when present, from the vendor's variables: CoinbaseCDP's api key id, api key secret and wallet secret, or StripePrivy's app id, app secret, authorization private key and authorization id. Every variable that is unset is named in one error rather than one per attempt, and an existing provider is left alone when they are all absent. - Teardown deletes the payment providers the project declares, as main's cleanupPaymentCredentialProviders does, after the stack rather than before, since a resource in it may still be using one. Only payment providers: an api-key or OAuth provider is named account-globally and may be shared with another project. The payment collision test now asserts what actually guards that case: a name ending in a payment field suffix is refused on its own, so a credential can no longer be created that would collide with a payment credential's variables.
Resolving every credential before writing any removes the likeliest cause of a half-provisioned deploy — a missing secret — but not the rest: a create that fails on throttling or permissions after an earlier one succeeded left a provider in AWS that deployed-state.json never recorded. Retrying adopted it by name, but abandoning the deploy or dropping the credential orphaned it. A failure during the write loop now deletes the providers that same run created, newest first, and rethrows the original error. A provider that already existed is not deleted: this deploy only updated its secret, and undoing that would need the value it held before, which the CLI never had. A deletion that fails is reported — naming the provider and saying the next deploy will adopt it — rather than replacing the error that stopped the deploy.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
Conflicts were additive on both sides: - envLocal: this branch added `read`, refactor added `removeKeys`; both kept. - CdkBackend: this branch added the credential provisioner and payment remover, refactor replaced the stack probe with `describeStack`; both kept, and teardown now asks `describeStack` whether the stack is still there.
|
Claude Security Review: no high-confidence findings. (run) |
e2e run — account
|
| # | Scenario | Result |
|---|---|---|
| 1 | First deploy creates the provider before synth | Preparing credential provider 'e2ekey2' precedes Synthesizing CloudFormation templates; provider ARN + secret ARN written to deployed-state.json, then stackArn merged into the same target entry |
| 2 | Rotate the secret in .env.local, redeploy |
Provider updated: new AWSCURRENT secret version at 22:59:05 against a create at 22:57:22, and GetSecretValue returned the rotated value |
| 3 | Variable in the process environment | .env.local held one value, the environment another → the environment's value is what landed in AWS |
| 4 | No secret in the file or the environment | Deploy succeeded, no new secret version, stored value unchanged — the existing provider is left alone rather than failing the deploy |
| 5 | Second credential fails after the first is created | Removing credential provider 'rbkeya' this deploy created; rbkeya gone from AWS, pre-existing e2ekey2 untouched, deployed-state.json unchanged, exit code 1 with the original service error |
| 6 | Payment credential (CoinbaseCDP) | paymentcredentialprovider/e2epay created and its ARN recorded. With two of three variables unset, one error named both: missing the values its payment provider needs: ..._API_KEY_SECRET, ..._WALLET_SECRET |
| 7 | Payment update, then teardown | Rotated apiKeyId reached AWS on redeploy. Teardown removed the stack, deleted e2epay, kept e2ekey2 (account-global, may be shared), and emptied targets |
Two notes for anyone re-running this:
- The service validates vendor key formats. Placeholder payment secrets are rejected (
Invalid apiKeySecret format: Expected base64-encoded Ed25519 private key, thenInvalid walletSecret format: Expected base64-encoded EC P-256 private key). I used structurally valid throwaway keys, so create/update/delete and the request shape are verified — but not against real Coinbase credentials. Those service messages surface as-is rather than CLI-framed. lastUpdatedTimeon an API-key provider does not move on update, soget-api-key-credential-provideralone can't confirm a rotation; Secrets Manager version history can.
Unrelated papercut noticed on the way: project remove harness <name> fails with "too many arguments" — it wants --name <name>.
Hweinstock
left a comment
Hweinstock
left a comment
There was a problem hiding this comment.
I wasn't able to full follow everything done here, so I left some random comments about where I was confused.
I was kind of envisioning this as a single function executed on deploy to reconcile the local env with the deployed environment, but it feels like there's a lot more going on. Is there a way to descope this into something simpler as a start?
| @@ -6,6 +6,7 @@ import type { AddResourceInput } from "../../types"; | |||
| import { | |||
There was a problem hiding this comment.
OOS here, but I think we should rename this file to avoid it becoming a dumping ground.
|
|
||
| // The collision check above only compares the fields credentials write today. A name | ||
| // ending in a field suffix would also shadow a field the CLI no longer writes but | ||
| // still reads (an OAuth client id in a pre-0.29 project) or one a later credential |
There was a problem hiding this comment.
can we link to more context here? I'm not sure what a pre-0.29 project means
| export type Credential = z.infer<typeof CredentialSchema>; | ||
|
|
||
| /** The prefix every variable carrying credential material shares. */ | ||
| export const CREDENTIAL_ENV_PREFIX = "AGENTCORE_CREDENTIAL_"; |
There was a problem hiding this comment.
i'm not super familiar with these schemas, but I'm suprised to find helper functions in projectSchemas. I would have expected it to be strictly zod schemas or their related types/constants.
| @@ -22,11 +29,12 @@ export interface CoreOptions { | |||
| export interface ClientConfig { | |||
There was a problem hiding this comment.
Should these two types be merged? The difference is less clear to me now.
| // AwsCredentials is an explicit credential source for a call: either resolved | ||
| // credentials or a provider that resolves them. Callers that rely on the SDK's own | ||
| // default credential chain leave it unset. | ||
| export type AwsCredentials = NonNullable<CloudFormationClientConfig["credentials"]>; |
There was a problem hiding this comment.
q: is the credentials field in CloudFormationClientConfig valid for all aws sdk clients?
|
|
||
| // credentialsId assigns each credential source a stable id for the lifetime of the | ||
| // object, so the same source reuses its client and a different one gets its own. | ||
| function credentialsId(credentials: NonNullable<ClientConfig["credentials"]>): number { |
There was a problem hiding this comment.
I'm not really following why we need this id?
|
|
||
| export class IdentityClient implements CoreIdentityClient { | ||
| constructor(private readonly clients: AwsClients) {} | ||
| // Only the control plane is used, so the dependency is narrowed to it: CoreClient |
There was a problem hiding this comment.
i feel like the code expresses this.
| CDK: new CdkBackend({ | ||
| logger: config.logger, | ||
| createCloudFormationClient: config.createCloudFormationClient, | ||
| identity: config.identity ?? createIdentityClient(), |
There was a problem hiding this comment.
whats the advantage to making this optional? It looks like we always pass it and I wonder if we it would simplify some of the core changes.
| yield { | ||
| message: | ||
| `Could not remove credential provider '${name}': ${(error as Error).message}. ` + | ||
| `Delete it with 'aws bedrock-agentcore-control delete-payment-credential-provider'.`, |
There was a problem hiding this comment.
isn't there a way to remove these via the resource based commands in our cli? Or did we only do readonly?
| @@ -0,0 +1,711 @@ | |||
| import { afterEach, describe, expect, test } from "bun:test"; | |||
There was a problem hiding this comment.
this testing setup feels extremely complex and coupled to the underlying implementation. Is there a way we can decouple it, and simplify?
6 participants
Creates or updates a project's credential providers before synthesis, so the synthesized CDK app can read their ARNs out of
deployed-state.json. Without this, deploying a project that declares any credential fails insidecdk synth.What it does
CdkBackend.deployprovisions each declared credential provider and records the ARNs viaupdateTargetState. Local prerequisites are checked first, so a setup error never mutates AWS.maindoes). A credential with no secret the CLI can see leaves an existing provider untouched instead of failing..env.local(AGENTCORE_CREDENTIAL_<NAME>) or a Secrets ManagersecretRef; process-environment variables override the file, so CI needs no secrets on disk.add credentials paymentcould build a project that couldn't deploy.core.identityclient instead of a second SDK client; the target's credentials travel viaCoreOptions.Notes
.env.localsilently had no effect.CoreClient's client cache is now keyed by credential identity — credentials are a function, whichJSON.stringifydrops, so different credentials in one region shared a cached client._CLIENT_ID,_APP_SECRET, …) are refused ataddtime; their variable would shadow another credential's field.mainguarded this; the rewrite had lost it.@aws/agentcore-cdk: its API-key Gateway path doesn't grantGetSecretValueon an external secret ARN, so an API-keysecretRefdeploys and then fails at retrieval. The CLI already records the ARN.Tested
_CLIENT_IDfallback, payment vendors, teardown deletion, rollback.add credentials api-key→deploycreated the provider before synth, wrote it todeployed-state.json, then mergedstackArninto the same entry.