From 76220c0e0644cfb66f011581d14d0379430c674b Mon Sep 17 00:00:00 2001 From: Aidan Daly Date: Tue, 30 Jun 2026 18:38:58 +0000 Subject: [PATCH 1/3] fix(e2e): reap leaked OAuth2 credential providers to prevent quota exhaustion The CUSTOM_JWT harness e2e test registers a managed OAuth2 credential provider (`-oauth`) that is created imperatively pre-deploy, outside the CloudFormation stack. `remove all` + teardown deploy destroys the stack but leaves the provider behind, and the globalSetup stale-cleanup only reaped ApiKey providers. Every run therefore leaked one provider until the account hit its 50-provider OAuth2 quota (L-431051DC), after which every CUSTOM_JWT deploy failed with "The number of agent identity Oauth2 credential providers in this account has reached its limit". - Add deleteOAuth2CredentialProvider + cleanupStaleOAuth2CredentialProviders to the cleanup util, mirroring the existing ApiKey reaper. - Wire the OAuth2 reaper into the globalSetup hook so every CI run auto-reaps stale E2e-prefixed providers older than 30 minutes. - Delete the managed -oauth provider in harness-custom-jwt afterAll so a single run self-cleans; name resolved via computeManagedOAuthCredentialName to stay in sync with the CLI. Closes #1674 --- e2e-tests/global-setup.ts | 21 +++++++- e2e-tests/harness-custom-jwt.test.ts | 22 ++++++++ .../utils/credential-provider-cleanup.ts | 50 +++++++++++++++++++ 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/e2e-tests/global-setup.ts b/e2e-tests/global-setup.ts index 3e02ba745..3f86feb70 100644 --- a/e2e-tests/global-setup.ts +++ b/e2e-tests/global-setup.ts @@ -1,4 +1,7 @@ -import { cleanupStaleCredentialProviders } from './utils/credential-provider-cleanup'; +import { + cleanupStaleCredentialProviders, + cleanupStaleOAuth2CredentialProviders, +} from './utils/credential-provider-cleanup'; import { getLogger } from './utils/logger'; import { cleanupStaleRecommendations } from './utils/recommendation-cleanup'; import { cleanUpOldStacks } from './utils/stack-cleanup'; @@ -45,6 +48,22 @@ export default async function setup(_project: TestProject): Promise<() => void> } catch (e) { logger.error(String(e)); logger.warn(`failed to clean up all credential providers`); + } + + // OAuth2 credential providers (managed OAuth creds for CUSTOM_JWT harnesses/gateways) + // are created imperatively pre-deploy and live outside the CDK stack, so `remove all` + // never reaps them. The account caps OAuth2 providers at 50 (quota L-431051DC); leaked + // providers accumulate until every CUSTOM_JWT deploy fails with a quota error. Reap them + // here the same way as stale stacks and recommendations. + logger.info(`cleaning up stale OAuth2 credential providers...`); + try { + await cleanupStaleOAuth2CredentialProviders(bedrockCPClient, logger.child('oauth2-credential-provider-cleanup'), { + minAgeMs: 30 * 60 * 1000, + prefix: 'E2e', + }); + } catch (e) { + logger.error(String(e)); + logger.warn(`failed to clean up all OAuth2 credential providers`); } finally { bedrockCPClient.destroy(); } diff --git a/e2e-tests/harness-custom-jwt.test.ts b/e2e-tests/harness-custom-jwt.test.ts index e2ed26e7e..9e8459a35 100644 --- a/e2e-tests/harness-custom-jwt.test.ts +++ b/e2e-tests/harness-custom-jwt.test.ts @@ -12,8 +12,12 @@ * * Requires: AWS credentials, npm, git. */ +import { computeManagedOAuthCredentialName } from '../src/cli/primitives/credential-utils.js'; import { hasAwsCredentials, parseJsonOutput, prereqs, runCLI, stripAnsi } from '../src/test-utils/index.js'; import { installCdkTarball, writeAwsTargets } from './e2e-helper.js'; +import { deleteOAuth2CredentialProvider } from './utils/credential-provider-cleanup.js'; +import { getLogger } from './utils/logger.js'; +import { BedrockAgentCoreControlClient } from '@aws-sdk/client-bedrock-agentcore-control'; import { CloudFormationClient, GetTemplateCommand } from '@aws-sdk/client-cloudformation'; import { CognitoIdentityProviderClient, @@ -172,6 +176,24 @@ describe.sequential('e2e: harness with CUSTOM_JWT auth', () => { } } + // ── Delete the managed OAuth2 credential provider ── + // `add harness --client-id/--client-secret` registers a managed OAuth credential, and + // deploy creates it in AgentCore Identity as a pre-deploy step outside the CDK stack. + // `remove all` tears down the stack but not this provider, so it would leak and count + // against the account's 50-provider OAuth2 quota (L-431051DC). Delete it explicitly. + if (harnessName) { + const controlClient = new BedrockAgentCoreControlClient({ region }); + try { + await deleteOAuth2CredentialProvider( + controlClient, + getLogger('teardown-hrns-jwt'), + computeManagedOAuthCredentialName(harnessName) + ); + } finally { + controlClient.destroy(); + } + } + // ── Delete Cognito resources ── if (userPoolId) { try { diff --git a/e2e-tests/utils/credential-provider-cleanup.ts b/e2e-tests/utils/credential-provider-cleanup.ts index a2f1ef42d..9355fa317 100644 --- a/e2e-tests/utils/credential-provider-cleanup.ts +++ b/e2e-tests/utils/credential-provider-cleanup.ts @@ -2,7 +2,9 @@ import type { Logger } from './logger'; import { BedrockAgentCoreControlClient, DeleteApiKeyCredentialProviderCommand, + DeleteOauth2CredentialProviderCommand, ListApiKeyCredentialProvidersCommand, + ListOauth2CredentialProvidersCommand, } from '@aws-sdk/client-bedrock-agentcore-control'; export async function deleteCredentialProvider( @@ -19,6 +21,20 @@ export async function deleteCredentialProvider( } } +export async function deleteOAuth2CredentialProvider( + client: BedrockAgentCoreControlClient, + logger: Logger, + name: string +): Promise { + try { + await client.send(new DeleteOauth2CredentialProviderCommand({ name })); + logger.info(`Deleted OAuth2 credential provider: ${name}`); + } catch (error) { + const err = error as Error; + logger.warn(`Failed to delete OAuth2 credential provider ${name}: ${err.name}:${err.message}`); + } +} + export async function cleanupStaleCredentialProviders( client: BedrockAgentCoreControlClient, logger: Logger, @@ -40,3 +56,37 @@ export async function cleanupStaleCredentialProviders( nextToken = response.nextToken; } while (nextToken); } + +/** + * Delete e2e OAuth2 credential providers older than `minAgeMs` matching `prefix`. + * + * OAuth2 providers are created imperatively as a pre-deploy step (managed OAuth + * credentials for CUSTOM_JWT harnesses/gateways), so they live outside the CDK + * stack — neither `remove all` nor the per-test stack teardown deletes them. A + * leaked provider therefore persists across runs, and the account is capped at 50 + * OAuth2 credential providers (quota L-431051DC). Once the cap is hit, every + * CreateOauth2CredentialProvider call fails with "The number of agent identity + * Oauth2 credential providers in this account has reached its limit", failing + * every CUSTOM_JWT deploy. Reap leftover e2e providers before starting. + */ +export async function cleanupStaleOAuth2CredentialProviders( + client: BedrockAgentCoreControlClient, + logger: Logger, + options: { + minAgeMs: number; + prefix: string; + } +): Promise { + const cutoff = new Date(Date.now() - options.minAgeMs); + + let nextToken: string | undefined; + do { + const response = await client.send(new ListOauth2CredentialProvidersCommand({ nextToken })); + const providers = response.credentialProviders ?? []; + const stale = providers.filter(p => p.name?.startsWith(options.prefix) && p.createdTime && p.createdTime < cutoff); + + await Promise.all(stale.map(p => deleteOAuth2CredentialProvider(client, logger, p.name!))); + + nextToken = response.nextToken; + } while (nextToken); +} From 633a5197ff58de2bbfdcdb5d7fa48f0b293e7cf1 Mon Sep 17 00:00:00 2001 From: Aidan Daly Date: Tue, 30 Jun 2026 19:45:58 +0000 Subject: [PATCH 2/3] fix(deploy): delete orphaned credential providers on teardown and removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supersedes the test-only mitigation in the previous commit with a fix in the CLI itself. OAuth2 and API key credential providers are created imperatively as a pre-deploy step, outside the CloudFormation stack (managed OAuth creds for CUSTOM_JWT harnesses/gateways, API keys for non-Bedrock providers). Nothing ever deleted them: stack teardown only handles in-stack resources, and the teardown path reaped *payment* providers only. So both `remove all` + teardown deploy and removing a single credential left the provider orphaned, accumulating against the account's 50-provider OAuth2/ApiKey quotas (L-431051DC / L-B04D9A86) until every CUSTOM_JWT deploy failed with "The number of agent identity Oauth2 credential providers in this account has reached its limit". This is what broke the e2e suite, but it leaks the same way for real users. - Add deleteOAuth2Provider / deleteApiKeyProvider (idempotent; treat ResourceNotFound as success — the managed secret is cascade-deleted by the Identity service with the provider). - Add reconcileCredentialProviders: delete providers recorded in deployed state that the current spec no longer references, routing by ARN segment (oauth2credentialprovider / apikeycredentialprovider). Operates only on providers the CLI recorded creating, so imported/pre-existing providers (never recorded) are never touched; payment ARNs fall through to their existing cleanup. Best-effort — failures warn, never abort teardown. - Wire it into both deploy paths (CLI command + TUI) after a successful deploy: an empty spec (teardown) reaps all recorded providers; a partial remove reaps just the diff. - Revert the e2e globalSetup OAuth2 reaper band-aid. Replace the manual afterAll delete in harness-custom-jwt with a regression test that asserts the managed provider is gone after teardown — proving the CLI fix. - Unit tests for reconcileCredentialProviders (ARN routing, retained-skip, payment-skip, error collection, teardown-deletes-all). Closes #1673 --- e2e-tests/global-setup.ts | 21 +--- e2e-tests/harness-custom-jwt.test.ts | 62 ++++++---- .../utils/credential-provider-cleanup.ts | 50 --------- src/cli/commands/deploy/actions.ts | 31 +++++ .../__tests__/pre-deploy-identity.test.ts | 106 ++++++++++++++++++ src/cli/operations/deploy/index.ts | 2 + .../operations/deploy/pre-deploy-identity.ts | 70 +++++++++++- .../identity/api-key-credential-provider.ts | 18 +++ src/cli/operations/identity/index.ts | 2 + .../identity/oauth2-credential-provider.ts | 18 +++ src/cli/tui/screens/deploy/useDeployFlow.ts | 39 +++++++ 11 files changed, 327 insertions(+), 92 deletions(-) diff --git a/e2e-tests/global-setup.ts b/e2e-tests/global-setup.ts index 3f86feb70..3e02ba745 100644 --- a/e2e-tests/global-setup.ts +++ b/e2e-tests/global-setup.ts @@ -1,7 +1,4 @@ -import { - cleanupStaleCredentialProviders, - cleanupStaleOAuth2CredentialProviders, -} from './utils/credential-provider-cleanup'; +import { cleanupStaleCredentialProviders } from './utils/credential-provider-cleanup'; import { getLogger } from './utils/logger'; import { cleanupStaleRecommendations } from './utils/recommendation-cleanup'; import { cleanUpOldStacks } from './utils/stack-cleanup'; @@ -48,22 +45,6 @@ export default async function setup(_project: TestProject): Promise<() => void> } catch (e) { logger.error(String(e)); logger.warn(`failed to clean up all credential providers`); - } - - // OAuth2 credential providers (managed OAuth creds for CUSTOM_JWT harnesses/gateways) - // are created imperatively pre-deploy and live outside the CDK stack, so `remove all` - // never reaps them. The account caps OAuth2 providers at 50 (quota L-431051DC); leaked - // providers accumulate until every CUSTOM_JWT deploy fails with a quota error. Reap them - // here the same way as stale stacks and recommendations. - logger.info(`cleaning up stale OAuth2 credential providers...`); - try { - await cleanupStaleOAuth2CredentialProviders(bedrockCPClient, logger.child('oauth2-credential-provider-cleanup'), { - minAgeMs: 30 * 60 * 1000, - prefix: 'E2e', - }); - } catch (e) { - logger.error(String(e)); - logger.warn(`failed to clean up all OAuth2 credential providers`); } finally { bedrockCPClient.destroy(); } diff --git a/e2e-tests/harness-custom-jwt.test.ts b/e2e-tests/harness-custom-jwt.test.ts index 9e8459a35..5260b0638 100644 --- a/e2e-tests/harness-custom-jwt.test.ts +++ b/e2e-tests/harness-custom-jwt.test.ts @@ -15,9 +15,11 @@ import { computeManagedOAuthCredentialName } from '../src/cli/primitives/credential-utils.js'; import { hasAwsCredentials, parseJsonOutput, prereqs, runCLI, stripAnsi } from '../src/test-utils/index.js'; import { installCdkTarball, writeAwsTargets } from './e2e-helper.js'; -import { deleteOAuth2CredentialProvider } from './utils/credential-provider-cleanup.js'; -import { getLogger } from './utils/logger.js'; -import { BedrockAgentCoreControlClient } from '@aws-sdk/client-bedrock-agentcore-control'; +import { + BedrockAgentCoreControlClient, + GetOauth2CredentialProviderCommand, + ResourceNotFoundException, +} from '@aws-sdk/client-bedrock-agentcore-control'; import { CloudFormationClient, GetTemplateCommand } from '@aws-sdk/client-cloudformation'; import { CognitoIdentityProviderClient, @@ -167,6 +169,9 @@ describe.sequential('e2e: harness with CUSTOM_JWT auth', () => { if (!canRun) return; // ── Tear down deployed stack ── + // The final test ('teardown deletes the managed OAuth2 credential provider') normally tears the + // project down. This is a best-effort safety net for when an earlier test failed before reaching + // it — it re-runs the teardown (idempotent; no-ops if already torn down). if (projectPath) { try { await runCLI(['remove', 'all', '--json'], projectPath, { skipInstall: false }); @@ -176,24 +181,6 @@ describe.sequential('e2e: harness with CUSTOM_JWT auth', () => { } } - // ── Delete the managed OAuth2 credential provider ── - // `add harness --client-id/--client-secret` registers a managed OAuth credential, and - // deploy creates it in AgentCore Identity as a pre-deploy step outside the CDK stack. - // `remove all` tears down the stack but not this provider, so it would leak and count - // against the account's 50-provider OAuth2 quota (L-431051DC). Delete it explicitly. - if (harnessName) { - const controlClient = new BedrockAgentCoreControlClient({ region }); - try { - await deleteOAuth2CredentialProvider( - controlClient, - getLogger('teardown-hrns-jwt'), - computeManagedOAuthCredentialName(harnessName) - ); - } finally { - controlClient.destroy(); - } - } - // ── Delete Cognito resources ── if (userPoolId) { try { @@ -350,4 +337,37 @@ describe.sequential('e2e: harness with CUSTOM_JWT auth', () => { }, 120000 ); + + it.skipIf(!canRun)( + 'teardown deletes the managed OAuth2 credential provider', + async () => { + // Regression test for the orphaned-provider leak: `add harness --client-id/--client-secret` + // registers a managed OAuth2 credential provider that deploy creates imperatively, outside the + // CloudFormation stack. Tearing the project down must delete it — otherwise it leaks and counts + // against the account's 50-provider OAuth2 quota (which previously broke the e2e suite). + const controlClient = new BedrockAgentCoreControlClient({ region }); + const providerName = computeManagedOAuthCredentialName(harnessName); + + // Precondition: the provider exists while the harness is deployed (proves the test is meaningful). + await expect( + controlClient.send(new GetOauth2CredentialProviderCommand({ name: providerName })), + `Managed OAuth2 provider "${providerName}" should exist before teardown` + ).resolves.toBeTruthy(); + + // Tear the project down: empty the spec, then deploy the empty spec (teardown deploy). + const removeResult = await runCLI(['remove', 'all', '--json'], projectPath, { skipInstall: false }); + expect(removeResult.exitCode, `remove all failed: ${removeResult.stderr}`).toBe(0); + const teardownResult = await runCLI(['deploy', '--yes', '--json'], projectPath, { skipInstall: false }); + expect(teardownResult.exitCode, `teardown deploy failed: ${teardownResult.stderr}`).toBe(0); + + // The provider must be gone after teardown. + await expect( + controlClient.send(new GetOauth2CredentialProviderCommand({ name: providerName })), + `Managed OAuth2 provider "${providerName}" should be deleted after teardown` + ).rejects.toBeInstanceOf(ResourceNotFoundException); + + controlClient.destroy(); + }, + 600000 + ); }); diff --git a/e2e-tests/utils/credential-provider-cleanup.ts b/e2e-tests/utils/credential-provider-cleanup.ts index 9355fa317..a2f1ef42d 100644 --- a/e2e-tests/utils/credential-provider-cleanup.ts +++ b/e2e-tests/utils/credential-provider-cleanup.ts @@ -2,9 +2,7 @@ import type { Logger } from './logger'; import { BedrockAgentCoreControlClient, DeleteApiKeyCredentialProviderCommand, - DeleteOauth2CredentialProviderCommand, ListApiKeyCredentialProvidersCommand, - ListOauth2CredentialProvidersCommand, } from '@aws-sdk/client-bedrock-agentcore-control'; export async function deleteCredentialProvider( @@ -21,20 +19,6 @@ export async function deleteCredentialProvider( } } -export async function deleteOAuth2CredentialProvider( - client: BedrockAgentCoreControlClient, - logger: Logger, - name: string -): Promise { - try { - await client.send(new DeleteOauth2CredentialProviderCommand({ name })); - logger.info(`Deleted OAuth2 credential provider: ${name}`); - } catch (error) { - const err = error as Error; - logger.warn(`Failed to delete OAuth2 credential provider ${name}: ${err.name}:${err.message}`); - } -} - export async function cleanupStaleCredentialProviders( client: BedrockAgentCoreControlClient, logger: Logger, @@ -56,37 +40,3 @@ export async function cleanupStaleCredentialProviders( nextToken = response.nextToken; } while (nextToken); } - -/** - * Delete e2e OAuth2 credential providers older than `minAgeMs` matching `prefix`. - * - * OAuth2 providers are created imperatively as a pre-deploy step (managed OAuth - * credentials for CUSTOM_JWT harnesses/gateways), so they live outside the CDK - * stack — neither `remove all` nor the per-test stack teardown deletes them. A - * leaked provider therefore persists across runs, and the account is capped at 50 - * OAuth2 credential providers (quota L-431051DC). Once the cap is hit, every - * CreateOauth2CredentialProvider call fails with "The number of agent identity - * Oauth2 credential providers in this account has reached its limit", failing - * every CUSTOM_JWT deploy. Reap leftover e2e providers before starting. - */ -export async function cleanupStaleOAuth2CredentialProviders( - client: BedrockAgentCoreControlClient, - logger: Logger, - options: { - minAgeMs: number; - prefix: string; - } -): Promise { - const cutoff = new Date(Date.now() - options.minAgeMs); - - let nextToken: string | undefined; - do { - const response = await client.send(new ListOauth2CredentialProvidersCommand({ nextToken })); - const providers = response.credentialProviders ?? []; - const stale = providers.filter(p => p.name?.startsWith(options.prefix) && p.createdTime && p.createdTime < cutoff); - - await Promise.all(stale.map(p => deleteOAuth2CredentialProvider(client, logger, p.name!))); - - nextToken = response.nextToken; - } while (nextToken); -} diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index ae4e146f5..d946520e9 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -51,6 +51,7 @@ import { enableOnlineEvalConfigs } from '../../operations/deploy/post-deploy-onl import { cleanupPaymentCredentialProviders, hasPaymentCredentialProviders, + reconcileCredentialProviders, setupPaymentCredentialProviders, } from '../../operations/deploy/pre-deploy-identity'; import { findOrphanHarnesses } from '../../operations/harness/orphan'; @@ -289,6 +290,13 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise = {}; + // Snapshot the credential providers recorded on the prior deploy BEFORE any state + // mutation below overwrites them. These are CLI-created OAuth2/ApiKey providers; after + // a successful deploy we reconcile this against the current spec to delete the ones no + // longer referenced (they live outside the CFN stack, so teardown wouldn't reap them). + const priorRecordedCredentials = + (await configIO.readDeployedState().catch(() => undefined))?.targets?.[target.name]?.resources?.credentials ?? {}; + if (hasIdentityApiProviders(context.projectSpec)) { startStep('Creating credentials...'); @@ -533,6 +541,29 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 0) { + const retainedCredentialNames = new Set(context.projectSpec.credentials.map(c => c.name)); + const hasOrphans = Object.keys(priorRecordedCredentials).some(name => !retainedCredentialNames.has(name)); + if (hasOrphans) { + startStep('Clean up unused credentials'); + const reconcileResult = await reconcileCredentialProviders({ + region: target.region, + priorCredentials: priorRecordedCredentials, + retainedCredentialNames, + }); + for (const { providerName, error } of reconcileResult.errors) { + logger.log(`Failed to delete unused credential provider '${providerName}': ${error.message}`, 'warn'); + } + endStep(reconcileResult.errors.length > 0 ? 'error' : 'success'); + } + } + if (context.isTeardownDeploy) { // Clean up imperative payment credential providers (CFN stack delete handles manager/connector/roles). // Harnesses are part of the CloudFormation stack, so stack destroy handles them. diff --git a/src/cli/operations/deploy/__tests__/pre-deploy-identity.test.ts b/src/cli/operations/deploy/__tests__/pre-deploy-identity.test.ts index a26d60ecf..bf606759e 100644 --- a/src/cli/operations/deploy/__tests__/pre-deploy-identity.test.ts +++ b/src/cli/operations/deploy/__tests__/pre-deploy-identity.test.ts @@ -1,6 +1,7 @@ import { getAllCredentials, hasIdentityOAuthProviders, + reconcileCredentialProviders, setupApiKeyProviders, setupOAuth2Providers, } from '../pre-deploy-identity.js'; @@ -15,6 +16,8 @@ const { mockOAuth2ProviderExists, mockCreateOAuth2Provider, mockUpdateOAuth2Provider, + mockDeleteOAuth2Provider, + mockDeleteApiKeyProvider, } = vi.hoisted(() => ({ mockKmsSend: vi.fn(), mockControlSend: vi.fn(), @@ -24,6 +27,8 @@ const { mockOAuth2ProviderExists: vi.fn(), mockCreateOAuth2Provider: vi.fn(), mockUpdateOAuth2Provider: vi.fn(), + mockDeleteOAuth2Provider: vi.fn(), + mockDeleteApiKeyProvider: vi.fn(), })); vi.mock('@aws-sdk/client-kms', () => ({ @@ -47,10 +52,12 @@ vi.mock('@aws-sdk/client-bedrock-agentcore-control', () => ({ vi.mock('../../identity/index.js', () => ({ apiKeyProviderExists: vi.fn(), createApiKeyProvider: vi.fn(), + deleteApiKeyProvider: mockDeleteApiKeyProvider, setTokenVaultKmsKey: mockSetTokenVaultKmsKey, updateApiKeyProvider: vi.fn(), oAuth2ProviderExists: mockOAuth2ProviderExists, createOAuth2Provider: mockCreateOAuth2Provider, + deleteOAuth2Provider: mockDeleteOAuth2Provider, updateOAuth2Provider: mockUpdateOAuth2Provider, })); @@ -399,3 +406,102 @@ describe('setupOAuth2Providers', () => { expect(result.results[0]!.error).toBe('Creation failed'); }); }); + +describe('reconcileCredentialProviders', () => { + const ARN_BASE = 'arn:aws:bedrock-agentcore:us-east-1:123456789012:token-vault/default'; + const oauth2Arn = (name: string) => `${ARN_BASE}/oauth2credentialprovider/${name}`; + const apiKeyArn = (name: string) => `${ARN_BASE}/apikeycredentialprovider/${name}`; + const paymentArn = (name: string) => `${ARN_BASE}/paymentcredentialprovider/${name}`; + + beforeEach(() => { + mockGetCredentialProvider.mockReturnValue({}); + mockDeleteOAuth2Provider.mockResolvedValue({ success: true }); + mockDeleteApiKeyProvider.mockResolvedValue({ success: true }); + }); + + afterEach(() => vi.clearAllMocks()); + + it('deletes an OAuth2 provider no longer in the spec, routing by ARN', async () => { + const result = await reconcileCredentialProviders({ + region: 'us-east-1', + priorCredentials: { 'my-harness-oauth': { credentialProviderArn: oauth2Arn('my-harness-oauth') } }, + retainedCredentialNames: new Set(), + }); + + expect(mockDeleteOAuth2Provider).toHaveBeenCalledWith(expect.anything(), 'my-harness-oauth'); + expect(mockDeleteApiKeyProvider).not.toHaveBeenCalled(); + expect(result.deleted).toEqual(['my-harness-oauth']); + expect(result.errors).toEqual([]); + }); + + it('deletes an ApiKey provider no longer in the spec, routing by ARN', async () => { + const result = await reconcileCredentialProviders({ + region: 'us-east-1', + priorCredentials: { 'gemini-key': { credentialProviderArn: apiKeyArn('gemini-key') } }, + retainedCredentialNames: new Set(), + }); + + expect(mockDeleteApiKeyProvider).toHaveBeenCalledWith(expect.anything(), 'gemini-key'); + expect(mockDeleteOAuth2Provider).not.toHaveBeenCalled(); + expect(result.deleted).toEqual(['gemini-key']); + }); + + it('keeps providers still referenced by the spec', async () => { + const result = await reconcileCredentialProviders({ + region: 'us-east-1', + priorCredentials: { + 'kept-oauth': { credentialProviderArn: oauth2Arn('kept-oauth') }, + 'gone-oauth': { credentialProviderArn: oauth2Arn('gone-oauth') }, + }, + retainedCredentialNames: new Set(['kept-oauth']), + }); + + expect(mockDeleteOAuth2Provider).toHaveBeenCalledTimes(1); + expect(mockDeleteOAuth2Provider).toHaveBeenCalledWith(expect.anything(), 'gone-oauth'); + expect(result.deleted).toEqual(['gone-oauth']); + }); + + it('never touches payment provider ARNs (handled separately)', async () => { + const result = await reconcileCredentialProviders({ + region: 'us-east-1', + priorCredentials: { 'pay-cred': { credentialProviderArn: paymentArn('pay-cred') } }, + retainedCredentialNames: new Set(), + }); + + expect(mockDeleteOAuth2Provider).not.toHaveBeenCalled(); + expect(mockDeleteApiKeyProvider).not.toHaveBeenCalled(); + expect(result.deleted).toEqual([]); + expect(result.errors).toEqual([]); + }); + + it('collects delete failures without throwing', async () => { + mockDeleteOAuth2Provider.mockResolvedValue({ success: false, error: new Error('quota throttled') }); + + const result = await reconcileCredentialProviders({ + region: 'us-east-1', + priorCredentials: { 'bad-oauth': { credentialProviderArn: oauth2Arn('bad-oauth') } }, + retainedCredentialNames: new Set(), + }); + + expect(result.deleted).toEqual([]); + expect(result.errors).toHaveLength(1); + expect(result.errors[0]!.providerName).toBe('bad-oauth'); + expect(result.errors[0]!.error.message).toBe('quota throttled'); + }); + + it('deletes all recorded providers on teardown (empty retained set)', async () => { + const result = await reconcileCredentialProviders({ + region: 'us-east-1', + priorCredentials: { + 'h-oauth': { credentialProviderArn: oauth2Arn('h-oauth') }, + 'k-key': { credentialProviderArn: apiKeyArn('k-key') }, + }, + retainedCredentialNames: new Set(), + }); + + expect(mockDeleteOAuth2Provider).toHaveBeenCalledWith(expect.anything(), 'h-oauth'); + expect(mockDeleteApiKeyProvider).toHaveBeenCalledWith(expect.anything(), 'k-key'); + expect(result.deleted).toEqual(expect.arrayContaining(['h-oauth', 'k-key'])); + expect(result.deleted).toHaveLength(2); + }); +}); diff --git a/src/cli/operations/deploy/index.ts b/src/cli/operations/deploy/index.ts index 29f0fd7e7..2706cca3f 100644 --- a/src/cli/operations/deploy/index.ts +++ b/src/cli/operations/deploy/index.ts @@ -47,9 +47,11 @@ export { setupPaymentCredentialProviders, hasPaymentCredentialProviders, cleanupPaymentCredentialProviders, + reconcileCredentialProviders, type SetupPaymentCredentialProvidersOptions, type PaymentCredentialProvidersResult, type PaymentCredentialProviderResult, + type ReconcileCredentialProvidersResult, } from './pre-deploy-identity'; // Post-deploy observability setup diff --git a/src/cli/operations/deploy/pre-deploy-identity.ts b/src/cli/operations/deploy/pre-deploy-identity.ts index 12da31014..dc110efb4 100644 --- a/src/cli/operations/deploy/pre-deploy-identity.ts +++ b/src/cli/operations/deploy/pre-deploy-identity.ts @@ -7,7 +7,7 @@ import { readEnvFile, toError, } from '../../../lib'; -import type { AgentCoreProjectSpec, Credential } from '../../../schema'; +import type { AgentCoreProjectSpec, Credential, CredentialDeployedState } from '../../../schema'; import { getCredentialProvider } from '../../aws'; import { createPaymentCredentialProvider, @@ -26,6 +26,8 @@ import { apiKeyProviderExists, createApiKeyProvider, createOAuth2Provider, + deleteApiKeyProvider, + deleteOAuth2Provider, oAuth2ProviderExists, setTokenVaultKmsKey, updateApiKeyProvider, @@ -596,6 +598,72 @@ export async function cleanupPaymentCredentialProviders(options: { } } +// ───────────────────────────────────────────────────────────────────────────── +// Orphaned Credential Provider Reconciliation +// ───────────────────────────────────────────────────────────────────────────── + +/** ARN resource segment that identifies an OAuth2 credential provider. */ +const OAUTH2_PROVIDER_ARN_SEGMENT = '/oauth2credentialprovider/'; +/** ARN resource segment that identifies an API key credential provider. */ +const API_KEY_PROVIDER_ARN_SEGMENT = '/apikeycredentialprovider/'; + +export interface ReconcileCredentialProvidersResult { + deleted: string[]; + errors: { providerName: string; error: Error }[]; +} + +/** + * Delete OAuth2 and API key credential providers the CLI created on a prior deploy + * but that are no longer referenced by the project spec. + * + * OAuth2 and API key providers are created imperatively pre-deploy, outside the + * CloudFormation stack (see {@link setupOAuth2Providers} / {@link setupApiKeyProviders}), + * so stack teardown never removes them and they accumulate against the account's + * 50-provider quota. This reconciles AWS with the spec on every deploy: tearing the + * project down (empty spec) deletes all recorded providers; removing a single + * credential deletes just that one. + * + * Safety: + * - Operates ONLY on providers recorded in deployed state — i.e. providers the CLI + * created. Imported/pre-existing providers are never recorded (they are skipped at + * creation time for lacking a discoveryUrl), so they are never deleted here. + * - Routes by ARN resource segment, so payment provider ARNs recorded in the same map + * are left untouched (payments have their own {@link cleanupPaymentCredentialProviders}). + * - Best-effort: a delete failure is collected, not thrown, so it never aborts teardown. + */ +export async function reconcileCredentialProviders(options: { + region: string; + /** Providers recorded in deployed state on the prior deploy (CLI-created only). */ + priorCredentials: Record; + /** Credential names still present in the project spec (these are kept). */ + retainedCredentialNames: Set; +}): Promise { + const { region, priorCredentials, retainedCredentialNames } = options; + const result: ReconcileCredentialProvidersResult = { deleted: [], errors: [] }; + + const client = new BedrockAgentCoreControlClient({ region, credentials: getCredentialProvider() }); + + for (const [providerName, state] of Object.entries(priorCredentials)) { + if (retainedCredentialNames.has(providerName)) continue; + + const arn = state.credentialProviderArn; + const deleteResult = arn.includes(OAUTH2_PROVIDER_ARN_SEGMENT) + ? await deleteOAuth2Provider(client, providerName) + : arn.includes(API_KEY_PROVIDER_ARN_SEGMENT) + ? await deleteApiKeyProvider(client, providerName) + : undefined; // payment or unknown ARN — not ours to reap here + + if (deleteResult === undefined) continue; + if (deleteResult.success) { + result.deleted.push(providerName); + } else { + result.errors.push({ providerName, error: deleteResult.error }); + } + } + + return result; +} + // ── Payment Credential Provider Helper ──────────────────────────────────── interface CreateOrUpdatePaymentCredentialProviderOptions { diff --git a/src/cli/operations/identity/api-key-credential-provider.ts b/src/cli/operations/identity/api-key-credential-provider.ts index 0d8040371..fc8c31941 100644 --- a/src/cli/operations/identity/api-key-credential-provider.ts +++ b/src/cli/operations/identity/api-key-credential-provider.ts @@ -10,6 +10,7 @@ import { err, ok } from '@/lib/result'; import { BedrockAgentCoreControlClient, CreateApiKeyCredentialProviderCommand, + DeleteApiKeyCredentialProviderCommand, GetApiKeyCredentialProviderCommand, ResourceNotFoundException, SetTokenVaultCMKCommand, @@ -90,6 +91,23 @@ export async function updateApiKeyProvider( } } +/** + * Delete an API key credential provider. + * + * Treats a missing provider as success so teardown is idempotent — the + * stored key lives in the service-managed token vault and is removed with + * the provider, so there is nothing else to clean up here. + */ +export async function deleteApiKeyProvider(client: BedrockAgentCoreControlClient, name: string): Promise { + try { + await client.send(new DeleteApiKeyCredentialProviderCommand({ name })); + return ok(); + } catch (error) { + if (error instanceof ResourceNotFoundException) return ok(); + return err(toError(error)); + } +} + /** * Configure KMS encryption for the token vault. * This encrypts all API key credential providers stored in the vault. diff --git a/src/cli/operations/identity/index.ts b/src/cli/operations/identity/index.ts index 92a5b40a1..3f41c96c3 100644 --- a/src/cli/operations/identity/index.ts +++ b/src/cli/operations/identity/index.ts @@ -1,11 +1,13 @@ export { apiKeyProviderExists, createApiKeyProvider, + deleteApiKeyProvider, setTokenVaultKmsKey, updateApiKeyProvider, } from './api-key-credential-provider'; export { createOAuth2Provider, + deleteOAuth2Provider, getOAuth2Provider, oAuth2ProviderExists, updateOAuth2Provider, diff --git a/src/cli/operations/identity/oauth2-credential-provider.ts b/src/cli/operations/identity/oauth2-credential-provider.ts index d500460a3..6454adcdf 100644 --- a/src/cli/operations/identity/oauth2-credential-provider.ts +++ b/src/cli/operations/identity/oauth2-credential-provider.ts @@ -11,6 +11,7 @@ import { BedrockAgentCoreControlClient, CreateOauth2CredentialProviderCommand, type CredentialProviderVendorType, + DeleteOauth2CredentialProviderCommand, GetOauth2CredentialProviderCommand, ResourceNotFoundException, UpdateOauth2CredentialProviderCommand, @@ -131,6 +132,23 @@ export async function getOAuth2Provider( } } +/** + * Delete an OAuth2 credential provider. + * + * Treats a missing provider as success so teardown is idempotent — the + * managed secret is owned by the Identity service and is cascade-deleted + * with the provider, so there is nothing else to clean up here. + */ +export async function deleteOAuth2Provider(client: BedrockAgentCoreControlClient, name: string): Promise { + try { + await client.send(new DeleteOauth2CredentialProviderCommand({ name })); + return ok(); + } catch (error) { + if (error instanceof ResourceNotFoundException) return ok(); + return err(toError(error)); + } +} + /** * Update an existing OAuth2 credential provider. */ diff --git a/src/cli/tui/screens/deploy/useDeployFlow.ts b/src/cli/tui/screens/deploy/useDeployFlow.ts index 9b2ebd719..dd74ea873 100644 --- a/src/cli/tui/screens/deploy/useDeployFlow.ts +++ b/src/cli/tui/screens/deploy/useDeployFlow.ts @@ -25,6 +25,7 @@ import { cleanupPaymentCredentialProviders, hasManagedMemoryHarness, performStackTeardown, + reconcileCredentialProviders, setupTransactionSearch, } from '../../../operations/deploy'; import { computeProjectDeployHash } from '../../../operations/deploy/change-detection'; @@ -700,6 +701,21 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState const attrs = context ? computeDeployAttrs(context.projectSpec, 'deploy') : { ...DEFAULT_DEPLOY_ATTRS }; const run = async (): Promise<{ success: true } | { success: false; error: Error }> => { + // Snapshot credential providers recorded on the prior deploy before persistDeployedState + // overwrites them. After deploy we reconcile these CLI-created OAuth2/ApiKey providers + // against the current spec and delete the ones no longer referenced — they live outside + // the CFN stack, so teardown wouldn't reap them. Mirrors the CLI command deploy path. + const reconcileTargetName = context?.awsTargets[0]?.name; + let priorRecordedCredentials: Record = {}; + if (reconcileTargetName) { + try { + const priorState = await new ConfigIO().readDeployedState(); + priorRecordedCredentials = priorState?.targets?.[reconcileTargetName]?.resources?.credentials ?? {}; + } catch { + // No prior deployed state — nothing to reconcile + } + } + // Run diff before deploy to capture pre-deploy differences. // Skip for brand new stacks: CDK changeset-based diff creates a temporary stack // in REVIEW_IN_PROGRESS then deletes it without waiting, racing with the deploy @@ -797,6 +813,29 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState ); setDeployStep(prev => ({ ...prev, status: 'success' })); + // Reap OAuth2/ApiKey credential providers the CLI created on a prior deploy that the + // current spec no longer references (created outside the CFN stack, so teardown won't + // reap them — they'd leak against the 50-provider quota). Only providers recorded in + // deployed state are touched, so imported/pre-existing providers are never deleted. + if (reconcileTargetName && context && Object.keys(priorRecordedCredentials).length > 0) { + const retainedCredentialNames = new Set(context.projectSpec.credentials.map(c => c.name)); + const hasOrphans = Object.keys(priorRecordedCredentials).some(name => !retainedCredentialNames.has(name)); + if (hasOrphans) { + try { + const result = await reconcileCredentialProviders({ + region: context.awsTargets[0]!.region, + priorCredentials: priorRecordedCredentials, + retainedCredentialNames, + }); + for (const { providerName, error } of result.errors) { + logger.log(`Failed to delete unused credential provider '${providerName}': ${error.message}`, 'warn'); + } + } catch (error) { + logger.log(`Credential provider reconciliation failed: ${getErrorMessage(error)}`, 'warn'); + } + } + } + if (context?.isTeardownDeploy) { // After deploying the empty spec, destroy the stack entirely. // Harnesses are part of the CloudFormation stack, so stack destroy handles them. From 536d04def5c29061e335479f44772d10cd636645 Mon Sep 17 00:00:00 2001 From: Aidan Daly Date: Tue, 30 Jun 2026 21:24:38 +0000 Subject: [PATCH 3/3] fix(deploy): reap orphaned credential providers after stack teardown Harden the credential-provider reconciliation so it deletes deterministically: - On a teardown deploy, run the reconcile AFTER performStackTeardown rather than before it. The harness/agent runtime in the CloudFormation stack references the imperative OAuth2/ApiKey provider, so deleting it before the stack is gone could fail; the failure was best-effort-swallowed, leaving the provider orphaned (observed as a flaky e2e teardown assertion). A normal (non-teardown) deploy still reconciles right after deploy, since the removed resource is already detached. - Give the reconcile control client maxAttempts: 5 so a throttled/transient delete is retried by the SDK instead of silently left behind. - Make the e2e teardown assertion tolerant of read-after-delete eventual consistency (retry the GET until it returns ResourceNotFound). Mirrors the reordering in both deploy paths (CLI command + TUI). --- e2e-tests/harness-custom-jwt.test.ts | 24 +++++++++--- src/cli/commands/deploy/actions.ts | 38 +++++++++++++------ .../operations/deploy/pre-deploy-identity.ts | 4 +- src/cli/tui/screens/deploy/useDeployFlow.ts | 35 ++++++++++------- 4 files changed, 68 insertions(+), 33 deletions(-) diff --git a/e2e-tests/harness-custom-jwt.test.ts b/e2e-tests/harness-custom-jwt.test.ts index 5260b0638..8e4f68edb 100644 --- a/e2e-tests/harness-custom-jwt.test.ts +++ b/e2e-tests/harness-custom-jwt.test.ts @@ -13,7 +13,7 @@ * Requires: AWS credentials, npm, git. */ import { computeManagedOAuthCredentialName } from '../src/cli/primitives/credential-utils.js'; -import { hasAwsCredentials, parseJsonOutput, prereqs, runCLI, stripAnsi } from '../src/test-utils/index.js'; +import { hasAwsCredentials, parseJsonOutput, prereqs, retry, runCLI, stripAnsi } from '../src/test-utils/index.js'; import { installCdkTarball, writeAwsTargets } from './e2e-helper.js'; import { BedrockAgentCoreControlClient, @@ -360,11 +360,23 @@ describe.sequential('e2e: harness with CUSTOM_JWT auth', () => { const teardownResult = await runCLI(['deploy', '--yes', '--json'], projectPath, { skipInstall: false }); expect(teardownResult.exitCode, `teardown deploy failed: ${teardownResult.stderr}`).toBe(0); - // The provider must be gone after teardown. - await expect( - controlClient.send(new GetOauth2CredentialProviderCommand({ name: providerName })), - `Managed OAuth2 provider "${providerName}" should be deleted after teardown` - ).rejects.toBeInstanceOf(ResourceNotFoundException); + // The provider must be gone after teardown. Retry to tolerate read-after-delete + // eventual consistency — the delete is issued during teardown, but the GET may briefly + // still see it. Resolves as soon as the GET throws ResourceNotFoundException. + await retry( + async () => { + let stillExists = false; + try { + await controlClient.send(new GetOauth2CredentialProviderCommand({ name: providerName })); + stillExists = true; + } catch (error) { + if (!(error instanceof ResourceNotFoundException)) throw error; + } + expect(stillExists, `Managed OAuth2 provider "${providerName}" should be deleted after teardown`).toBe(false); + }, + 5, + 3000 + ); controlClient.destroy(); }, diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index d946520e9..6923f49fd 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -547,21 +547,31 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 0) { + // + // On a teardown deploy this MUST run after performStackTeardown (below): the harness + // runtime in the stack references the provider, so deleting it before the stack is gone + // can fail. On a normal deploy the removed resource is already detached by the deploy + // above, so reconcile runs here. + const reconcileUnusedCredentials = async (): Promise => { + if (Object.keys(priorRecordedCredentials).length === 0) return; const retainedCredentialNames = new Set(context.projectSpec.credentials.map(c => c.name)); const hasOrphans = Object.keys(priorRecordedCredentials).some(name => !retainedCredentialNames.has(name)); - if (hasOrphans) { - startStep('Clean up unused credentials'); - const reconcileResult = await reconcileCredentialProviders({ - region: target.region, - priorCredentials: priorRecordedCredentials, - retainedCredentialNames, - }); - for (const { providerName, error } of reconcileResult.errors) { - logger.log(`Failed to delete unused credential provider '${providerName}': ${error.message}`, 'warn'); - } - endStep(reconcileResult.errors.length > 0 ? 'error' : 'success'); + if (!hasOrphans) return; + + startStep('Clean up unused credentials'); + const reconcileResult = await reconcileCredentialProviders({ + region: target.region, + priorCredentials: priorRecordedCredentials, + retainedCredentialNames, + }); + for (const { providerName, error } of reconcileResult.errors) { + logger.log(`Failed to delete unused credential provider '${providerName}': ${error.message}`, 'warn'); } + endStep(reconcileResult.errors.length > 0 ? 'error' : 'success'); + }; + + if (!context.isTeardownDeploy) { + await reconcileUnusedCredentials(); } if (context.isTeardownDeploy) { @@ -595,6 +605,10 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 1 so a throttled/transient delete is retried by the SDK rather than + // silently left as an orphan (this runs alongside other e2e shards hammering the same APIs). + const client = new BedrockAgentCoreControlClient({ region, credentials: getCredentialProvider(), maxAttempts: 5 }); for (const [providerName, state] of Object.entries(priorCredentials)) { if (retainedCredentialNames.has(providerName)) continue; diff --git a/src/cli/tui/screens/deploy/useDeployFlow.ts b/src/cli/tui/screens/deploy/useDeployFlow.ts index dd74ea873..42038e1a7 100644 --- a/src/cli/tui/screens/deploy/useDeployFlow.ts +++ b/src/cli/tui/screens/deploy/useDeployFlow.ts @@ -817,24 +817,27 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState // current spec no longer references (created outside the CFN stack, so teardown won't // reap them — they'd leak against the 50-provider quota). Only providers recorded in // deployed state are touched, so imported/pre-existing providers are never deleted. - if (reconcileTargetName && context && Object.keys(priorRecordedCredentials).length > 0) { + // + // On a teardown deploy this MUST run after performStackTeardown: the harness runtime in + // the stack references the provider, so deleting it before the stack is gone can fail. + const reconcileUnusedCredentials = async (): Promise => { + if (!reconcileTargetName || !context || Object.keys(priorRecordedCredentials).length === 0) return; const retainedCredentialNames = new Set(context.projectSpec.credentials.map(c => c.name)); const hasOrphans = Object.keys(priorRecordedCredentials).some(name => !retainedCredentialNames.has(name)); - if (hasOrphans) { - try { - const result = await reconcileCredentialProviders({ - region: context.awsTargets[0]!.region, - priorCredentials: priorRecordedCredentials, - retainedCredentialNames, - }); - for (const { providerName, error } of result.errors) { - logger.log(`Failed to delete unused credential provider '${providerName}': ${error.message}`, 'warn'); - } - } catch (error) { - logger.log(`Credential provider reconciliation failed: ${getErrorMessage(error)}`, 'warn'); + if (!hasOrphans) return; + try { + const result = await reconcileCredentialProviders({ + region: context.awsTargets[0]!.region, + priorCredentials: priorRecordedCredentials, + retainedCredentialNames, + }); + for (const { providerName, error } of result.errors) { + logger.log(`Failed to delete unused credential provider '${providerName}': ${error.message}`, 'warn'); } + } catch (error) { + logger.log(`Credential provider reconciliation failed: ${getErrorMessage(error)}`, 'warn'); } - } + }; if (context?.isTeardownDeploy) { // After deploying the empty spec, destroy the stack entirely. @@ -858,8 +861,12 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState if (!teardown.success) { throw new Error(`Stack teardown failed: ${teardown.error.message}`); } + + // Stack (and the harness referencing them) is gone — now reap the imperative providers. + await reconcileUnusedCredentials(); } } else { + await reconcileUnusedCredentials(); // Deploy succeeded - persist state try { await persistDeployedState();