diff --git a/src/core/project/backends/cdk.test.ts b/src/core/project/backends/cdk.test.ts index 20ab3f552..f72ea79ff 100644 --- a/src/core/project/backends/cdk.test.ts +++ b/src/core/project/backends/cdk.test.ts @@ -945,6 +945,78 @@ describe("CdkBackend.deploy", () => { expect(deployed.result).toEqual({ outputs: {} }); }); + test("removes a payment provider recorded under its underscored name once its replacement deployed", async () => { + const input = await project(); + input.spec = ProjectSpecSchema.parse({ + name: "example", + version: 2, + credentials: [ + { authorizerType: "PaymentCredentialProvider", name: "wallet", provider: "CoinbaseCDP" }, + ], + }); + await writeAssembly(input, [TARGET.name]); + const statePath = join(input.rootPath, DEPLOYED_STATE_RELATIVE_PATH); + await mkdir(dirname(statePath), { recursive: true }); + const arn = (name: string) => + `arn:aws:bedrock-agentcore:${TARGET.region}:${TARGET.account}:token-vault/default/paymentcredentialprovider/${name}`; + await writeFile( + statePath, + JSON.stringify({ + targets: { + default: { + stackArn: "arn:stack:default", + resources: { + credentials: { + wallet: { + credentialProviderArn: arn("example_default_wallet"), + authorizerType: "PaymentCredentialProvider", + }, + }, + }, + }, + }, + }), + ); + const provisionCredentials: CredentialProvisioner = async function* () { + yield { type: "step", message: "preparing credential provider 'example-default-wallet'" }; + return { + wallet: { + credentialProviderArn: arn("example-default-wallet"), + authorizerType: "PaymentCredentialProvider", + }, + }; + }; + const handed: CredentialRemovalInput[] = []; + const removeCredentials: CredentialRemover = async function* (_project, removalInput) { + handed.push(removalInput); + yield { type: "step", message: "removing credential provider 'example_default_wallet'" }; + }; + const subject = harness({ provisionCredentials, removeCredentials }); + + const deployed = await collectDeploy(subject.backend.deploy(input, deployInput())); + + // The credential is still declared, but the stack now uses the hyphenated provider, + // and no connector could ever reference the underscored one. + expect(handed).toEqual([ + { + credentials: subject.credentials, + region: TARGET.region, + targetName: TARGET.name, + providers: [ + { + name: "wallet", + authorizerType: "PaymentCredentialProvider", + legacyProvider: "example_default_wallet", + }, + ], + }, + ]); + const messages = stepMessages(deployed.events); + expect( + messages.indexOf("deploying AgentCore-example-default-0 (usually takes a few minutes)"), + ).toBeLessThan(messages.indexOf("removing credential provider 'example_default_wallet'")); + }); + test("says to add a resource when there is no stack to remove either", async () => { const input = await project(); await writeAssembly(input, [TARGET.name], { resources: METADATA_ONLY }); diff --git a/src/core/project/backends/cdk.ts b/src/core/project/backends/cdk.ts index d023e82c1..40bb20f5b 100644 --- a/src/core/project/backends/cdk.ts +++ b/src/core/project/backends/cdk.ts @@ -273,7 +273,7 @@ export class CdkBackend implements ProjectBackend { const recorded = (await readDeployedState(this.json, project.rootPath)).targets[target.name]?.resources ?.credentials ?? {}; - const orphaned = orphanedCredentials(recorded, project.spec.credentials); + const orphaned = orphanedCredentials(project, target.name, recorded); // Credential providers aren't stack resources; the synthesized app reads their // ARNs from deployed-state.json, so they must exist and be recorded before synth. diff --git a/src/core/project/backends/cdk/credentials.test.ts b/src/core/project/backends/cdk/credentials.test.ts index 820b4852e..87e214fd6 100644 --- a/src/core/project/backends/cdk/credentials.test.ts +++ b/src/core/project/backends/cdk/credentials.test.ts @@ -56,10 +56,17 @@ const COINBASE = { /** * The names Identity sees: project, target, then the spec credential name. Spec names * and the `.env.local` variables derived from them are untouched by the scoping. + * Payment names are joined with hyphens, which a payment connector can reference. */ const OPENAI_PROVIDER = "example_dev_openai-key"; const OAUTH_PROVIDER = "example_dev_my-oauth"; -const WALLET_PROVIDER = "example_dev_wallet"; +const WALLET_PROVIDER = "example-dev-wallet"; + +/** The service's pattern for a payment connector's `credentialProviderArn`. */ +const PAYMENT_CONNECTOR_CREDENTIAL_ARN = + /^arn:[^:]+:(acps|bedrock-agentcore):[A-Za-z0-9-]{1,64}:[0-9]{12}:token-vault\/[a-zA-Z0-9-.]+\/paymentcredentialprovider\/[a-zA-Z0-9-.]+$/; +const paymentProviderArn = (name: string) => + `arn:aws:bedrock-agentcore:${REGION}:123456789012:token-vault/default/paymentcredentialprovider/${name}`; const tempDirectories: string[] = []; afterEach(async () => { @@ -716,7 +723,7 @@ describe("createCredentialProvisioner", () => { }); test("updates an existing StripePrivy payment provider", async () => { - const subject = account({ example_dev_pay: { credentialProviderArn: "arn:existing" } }); + const subject = account({ "example-dev-pay": { credentialProviderArn: "arn:existing" } }); const input = await project( [{ authorizerType: "PaymentCredentialProvider", name: "pay", provider: "StripePrivy" }], "AGENTCORE_CREDENTIAL_PAY_APP_ID='app-1'\n" + @@ -727,9 +734,9 @@ describe("createCredentialProvisioner", () => { await run(subject.provision, input); - expect(subject.contents()["example_dev_pay"]?.operation).toBe("updated"); - expect(subject.configFor("example_dev_pay")).toEqual({ - name: "example_dev_pay", + expect(subject.contents()["example-dev-pay"]?.operation).toBe("updated"); + expect(subject.configFor("example-dev-pay")).toEqual({ + name: "example-dev-pay", credentialProviderVendor: "StripePrivy", providerConfigurationInput: { stripePrivyConfiguration: { @@ -742,6 +749,27 @@ describe("createCredentialProvisioner", () => { }); }); + test("names a payment provider so its connector can reference it", async () => { + const subject = account(); + const input = await project( + [{ ...COINBASE, name: "my_wallet" }], + "AGENTCORE_CREDENTIAL_MY_WALLET_API_KEY_ID='key-1'\n" + + "AGENTCORE_CREDENTIAL_MY_WALLET_API_KEY_SECRET='key-secret'\n" + + "AGENTCORE_CREDENTIAL_MY_WALLET_WALLET_SECRET='wallet-secret'\n", + ); + + const { result } = await run(subject.provision, input); + + // The underscores of the scope and of the credential name both become hyphens; + // the spec name, and the variables derived from it, keep theirs. + expect(Object.keys(subject.contents())).toEqual(["example-dev-my-wallet"]); + expect(Object.keys(result)).toEqual(["my_wallet"]); + expect(paymentProviderArn("example-dev-my-wallet")).toMatch(PAYMENT_CONNECTOR_CREDENTIAL_ARN); + expect(paymentProviderArn("example_dev_my_wallet")).not.toMatch( + PAYMENT_CONNECTOR_CREDENTIAL_ARN, + ); + }); + test("names every payment variable that is unset", async () => { const subject = account(); const input = await project([COINBASE], "AGENTCORE_CREDENTIAL_WALLET_API_KEY_ID='key-1'\n"); @@ -934,10 +962,53 @@ describe("orphanedCredentials", () => { }; const input = await project([COINBASE]); - expect(orphanedCredentials(recorded, input.spec.credentials)).toEqual([ + expect(orphanedCredentials(input, INPUT.targetName, recorded)).toEqual([ { name: "openai-key", authorizerType: "ApiKeyCredentialProvider" }, ]); }); + + test("keeps a payment provider recorded under its underscored name, by that name", async () => { + // Created by a CLI that joined payment names with underscores, which no connector + // can reference. The deploy creates the hyphenated one, so the old one goes, + // whether or not its credential is still declared. + const recorded: DeployedCredentials = { + wallet: recordedAs("PaymentCredentialProvider", { + credentialProviderArn: paymentProviderArn("example_dev_wallet"), + }), + dropped: recordedAs("PaymentCredentialProvider", { + credentialProviderArn: paymentProviderArn("example_dev_dropped"), + }), + current: recordedAs("PaymentCredentialProvider", { + credentialProviderArn: paymentProviderArn("example-dev-current"), + }), + }; + const input = await project([COINBASE, { ...COINBASE, name: "current" }]); + + expect(orphanedCredentials(input, INPUT.targetName, recorded)).toEqual([ + { + name: "wallet", + authorizerType: "PaymentCredentialProvider", + legacyProvider: "example_dev_wallet", + }, + { + name: "dropped", + authorizerType: "PaymentCredentialProvider", + legacyProvider: "example_dev_dropped", + }, + ]); + }); + + test("leaves a v0 payment provider's bare name alone", async () => { + const recorded: DeployedCredentials = { + wallet: recordedAs("PaymentCredentialProvider", { + credentialProviderArn: paymentProviderArn("wallet"), + }), + }; + + expect( + orphanedCredentials(await migratedProject([COINBASE]), INPUT.targetName, recorded), + ).toEqual([]); + }); }); describe("createCredentialRemover", () => { @@ -973,6 +1044,21 @@ describe("createCredentialRemover", () => { expect(subject.optionsSeen).toEqual([OPTIONS, OPTIONS, OPTIONS]); }); + test("deletes a payment provider by the name it was recorded under", async () => { + const subject = account({ + example_dev_wallet: { credentialProviderArn: paymentProviderArn("example_dev_wallet") }, + [WALLET_PROVIDER]: { credentialProviderArn: paymentProviderArn(WALLET_PROVIDER) }, + }); + + const { events } = await remove(subject, [ + { ...COINBASE, legacyProvider: "example_dev_wallet" }, + ]); + + expect(subject.deleted).toEqual([{ kind: "payment", name: "example_dev_wallet" }]); + expect(Object.keys(subject.contents())).toEqual([WALLET_PROVIDER]); + expect(stepMessages(events)).toEqual(["removing credential provider 'example_dev_wallet'"]); + }); + test("treats a provider that is already gone as removed", async () => { const notFound = new ResourceNotFoundException({ $metadata: {}, message: "not found" }); const subject = account({}, { deleteFails: notFound }); diff --git a/src/core/project/backends/cdk/credentials.ts b/src/core/project/backends/cdk/credentials.ts index f44b0aa17..d7398f1b4 100644 --- a/src/core/project/backends/cdk/credentials.ts +++ b/src/core/project/backends/cdk/credentials.ts @@ -49,13 +49,46 @@ function keepsV0CredentialNames(project: Project): boolean { * The Identity name a credential's provider is created under. Scoped to the project * and target so two targets in one account and region never share a provider. * + * A payment provider is scoped with hyphens, and the credential name's underscores + * become hyphens too: a payment connector's `credentialProviderArn` only accepts + * letters, digits, `-` and `.` in the provider name, and Identity names do not + * allow `.`, so an underscore anywhere in the name leaves the connector unable to + * reference its provider. Two credentials cannot meet on one name this way: names + * that differ only in `_` and `-` derive the same `.env.local` variables, which the + * spec already refuses. + * * A v0 project, and one migrated from v0, keeps the bare credential name v0 created * its providers under: the agent code it deployed looks them up by that name, and a * scoped name would create a second set of providers beside the ones still in use. */ -function providerName(project: Project, targetName: string, credentialName: string): string { - if (keepsV0CredentialNames(project)) return credentialName; - return `${project.name}_${targetName}_${credentialName}`; +function providerName( + project: Project, + targetName: string, + { name, authorizerType }: CredentialProviderRef, +): string { + if (keepsV0CredentialNames(project)) return name; + if (authorizerType === "PaymentCredentialProvider") { + return `${project.name}-${targetName}-${name.replaceAll("_", "-")}`; + } + return `${project.name}_${targetName}_${name}`; +} + +/** + * The name CLIs before the hyphenated scheme gave a payment provider, when the + * recorded ARN still points at it. Such a provider can never back a connector, so + * the deploy that creates its replacement deletes it like any other orphan. + */ +function legacyPaymentProviderName( + project: Project, + targetName: string, + name: string, + { credentialProviderArn, authorizerType }: DeployedCredential, +): string | undefined { + if (keepsV0CredentialNames(project) || authorizerType !== "PaymentCredentialProvider") { + return undefined; + } + const legacy = `${project.name}_${targetName}_${name}`; + return credentialProviderArn.endsWith(`/${legacy}`) ? legacy : undefined; } const PROVIDER_NAME_MAX_LENGTH = 128; @@ -109,7 +142,10 @@ export type CredentialProvisioner = ( ) => AsyncGenerator; /** A credential's spec name and provider kind, enough to name and delete its provider. */ -export type CredentialProviderRef = Pick; +export type CredentialProviderRef = Pick & { + /** The name a payment provider was recorded under before the hyphenated scheme. */ + legacyProvider?: string; +}; export type CredentialRemovalInput = CredentialProvisionInput & { /** The credentials whose providers to delete. The caller decides the set. */ @@ -129,15 +165,24 @@ export type CredentialRemover = ( * A recorded entry without `authorizerType` was written by a CLI that named providers * by the bare credential name, so nothing exists under the scoped name and the entry * is skipped. + * + * A payment provider recorded under its underscored name is orphaned even while its + * credential is still declared: the deploy creates its hyphenated replacement, and + * the old one is deleted by the name it was recorded under. */ export function orphanedCredentials( + project: Project, + targetName: string, recorded: DeployedCredentials, - declared: readonly Credential[], ): CredentialProviderRef[] { - const names = new Set(declared.map(({ name }) => name)); - return Object.entries(recorded).flatMap(([name, { authorizerType }]) => - authorizerType && !names.has(name) ? [{ name, authorizerType }] : [], - ); + const names = new Set(project.spec.credentials.map(({ name }) => name)); + return Object.entries(recorded).flatMap(([name, deployed]) => { + const { authorizerType } = deployed; + if (!authorizerType) return []; + const legacyProvider = legacyPaymentProviderName(project, targetName, name, deployed); + if (legacyProvider) return [{ name, authorizerType, legacyProvider }]; + return names.has(name) ? [] : [{ name, authorizerType }]; + }); } /** @@ -158,8 +203,9 @@ export function createCredentialRemover(identity: ProviderDeletes): CredentialRe { region, credentials, targetName, providers }, ) { const options: CoreOptions = { region, credentials }; - for (const { name, authorizerType } of providers) { - const provider = providerName(project, targetName, name); + for (const ref of providers) { + const { authorizerType } = ref; + const provider = ref.legacyProvider ?? providerName(project, targetName, ref); if (keepsV0CredentialNames(project) && authorizerType !== "PaymentCredentialProvider") { yield { type: "step", @@ -227,7 +273,7 @@ export function createCredentialProvisioner( // Every name is composed and checked before the first lookup, so one that is // too long fails the deploy before it touches Identity. const named: NamedCredential[] = declared.map((credential) => { - const provider = providerName(project, targetName, credential.name); + const provider = providerName(project, targetName, credential); if (provider.length > PROVIDER_NAME_MAX_LENGTH) { throw new ProjectStateError( `Credential '${credential.name}' would create a credential provider named ` +