diff --git a/e2e-tests/harness-custom-jwt.test.ts b/e2e-tests/harness-custom-jwt.test.ts index e2ed26e7e..8e4f68edb 100644 --- a/e2e-tests/harness-custom-jwt.test.ts +++ b/e2e-tests/harness-custom-jwt.test.ts @@ -12,8 +12,14 @@ * * Requires: AWS credentials, npm, git. */ -import { hasAwsCredentials, parseJsonOutput, prereqs, runCLI, stripAnsi } from '../src/test-utils/index.js'; +import { computeManagedOAuthCredentialName } from '../src/cli/primitives/credential-utils.js'; +import { hasAwsCredentials, parseJsonOutput, prereqs, retry, runCLI, stripAnsi } from '../src/test-utils/index.js'; import { installCdkTarball, writeAwsTargets } from './e2e-helper.js'; +import { + BedrockAgentCoreControlClient, + GetOauth2CredentialProviderCommand, + ResourceNotFoundException, +} from '@aws-sdk/client-bedrock-agentcore-control'; import { CloudFormationClient, GetTemplateCommand } from '@aws-sdk/client-cloudformation'; import { CognitoIdentityProviderClient, @@ -163,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 }); @@ -328,4 +337,49 @@ 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. 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(); + }, + 600000 + ); }); diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index ae4e146f5..6923f49fd 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,39 @@ export async function handleDeploy(options: ValidatedDeployOptions): 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) 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) { // 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. @@ -564,6 +605,10 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise ({ 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..b2eccaed2 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,74 @@ 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: [] }; + + // maxAttempts > 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; + + 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..42038e1a7 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,32 @@ 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. + // + // 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) 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. // Harnesses are part of the CloudFormation stack, so stack destroy handles them. @@ -819,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();