fix(deploy): delete orphaned credential providers on teardown and removal#1675
fix(deploy): delete orphaned credential providers on teardown and removal#1675aidandaly24 wants to merge 3 commits into
Conversation
…haustion The CUSTOM_JWT harness e2e test registers a managed OAuth2 credential provider (`<harness>-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 <harness>-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
|
Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh release download pr-1675-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.21.1.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
LGTM. Clean, well-scoped test-infrastructure fix that mirrors the existing ApiKey cleanup pattern (paginate → filter by prefix + age → delete in parallel). A few things I checked:
- Client lifecycle (
global-setup.ts): The new OAuth2 cleanup is placed before the existingfinally { bedrockCPClient.destroy() }so the client still gets destroyed exactly once. ✓ - Teardown ordering (
harness-custom-jwt.test.ts): The pre-existingafterAlldoesremove all→deploy(which re-registers the OAuth2 provider as a pre-deploy step). The new explicit deletion runs after that second deploy, so the provider is reliably gone by the end. ✓ Also correctly handles the case wherebeforeAllfails before any deploy ever ran —deleteOAuth2CredentialProviderswallows errors with a warn. - Prefix safety:
E2eHrnsJwt<ts>-oauthstarts withE2e, matching the cleanup filter; no risk of reaping non-e2e providers. - No telemetry needed — internal e2e test infrastructure, no user-facing surface area changed.
Optional nit (not blocking): e2e-tests/README.md mentions cleanupStaleCredentialProviders() in the cleanup notes; worth a one-line update there to mention OAuth2 too, but happy to defer.
Coverage Report
|
…oval 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
|
Claude Security Review: no high-confidence findings. (run) |
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).
|
Pushed a hardening commit ( What happened: the e2e regression test ( Hardening:
Verification: the originally-flaky |
|
Claude Security Review: no high-confidence findings. (run) |
Description
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 model providers. Nothing ever deleted them:
cleanupPaymentCredentialProviders) — there was no equivalent for OAuth2 or API key providers, and noDelete*wrapper for them existed in the CLI at all.So both teardown flows leaked:
remove all+ teardown deployremove <credential>+ redeploy (others remain)Orphaned providers accumulate against the account's 50-provider quotas (
L-431051DCOAuth2 /L-B04D9A86API key). Once full, every CUSTOM_JWT deploy fails with:This broke the
E2E Tests (Full Suite)CUSTOM_JWT shard (the account was at exactly 50/50, allE2eHrnsJwt<ts>-oauth), but it leaks the same way for real users — which is why this is fixed in the CLI, not just in the tests.Changes
oauth2-credential-provider.ts/api-key-credential-provider.ts— adddeleteOAuth2Provider/deleteApiKeyProvider. Idempotent:ResourceNotFoundis treated as success. No secret deletion — the managed secret lives in the service-ownedbedrock-agentcore-identity!namespace and is cascade-deleted with the provider (verified empirically).pre-deploy-identity.ts— addreconcileCredentialProviders: delete providers recorded in deployed state (resources.credentials) that the current spec no longer references, routing by ARN segment (oauth2credentialprovider/apikeycredentialprovider). Best-effort: failures are collected and logged, never thrown. The control client usesmaxAttempts: 5so a throttled/transient delete is retried by the SDK rather than silently left as an orphan.deploy/actions.ts(CLI) +useDeployFlow.ts(TUI) — snapshot the prior recorded credentials before state is overwritten, then reconcile:remove+ redeploy): reconcile right after deploy — the removed resource is already detached, so its provider deletes cleanly. Deletes just the diff.remove all): reconcile afterperformStackTeardown, so the harness/agent runtime that referenced the provider is already gone (deleting it while the stack still references it can fail). Deletes all recorded providers.harness-custom-jwt.test.ts— new regression test asserting the managed<harness>-oauthprovider exists while deployed and is gone after teardown. The post-teardown check retries theGetOauth2CredentialProvidercall to tolerate read-after-delete eventual consistency. Reverts the earlier test-side band-aid (globalSetup reaper + manualafterAlldelete).pre-deploy-identity.test.ts— unit tests forreconcileCredentialProviders: OAuth2/ApiKey ARN routing, retained-names skipped, payment ARNs untouched, error collection, teardown-deletes-all.Safety: imported / user-supplied providers are never deleted
reconcileCredentialProvidersoperates only on providers recorded indeployed-state.resources.credentials, which is written only from providers the CLI created this/a prior deploy (setup*Providersresults). Imported OAuth providers are skipped at creation (nodiscoveryUrl→status:'skipped', no ARN recorded), and the import flow never writesresources.credentials. So a pre-existing/shared provider the user supplied an ARN for is never reaped. Payment ARNs recorded in the same map fall through to their existing dedicated cleanup.Related Issue
Closes #1673
Related (CI symptom that surfaced this): #1674
Documentation PR
N/A — bugfix in existing deploy/teardown behavior.
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsResults:
typecheck✅ ·lint✅ (no new warnings) ·prettier✅test:unit✅ 5691 passed (393 files) — includes 6 newreconcileCredentialProvidersteststest:integ✅ 314 passed, 1 skipped (32 files)harness-custom-jwtsuite (which exercises deploy → teardown → assert-provider-gone) passed 4 consecutive runs, including a manual repro where the teardown log confirmedClean up unused credentials → SUCCESSand the provider returnedResourceNotFoundExceptionafterward.Note on the commit history
The first version of this fix ran the reconcile before stack teardown, which flaked the e2e teardown assertion in CI (a throttled delete against a still-referenced provider was best-effort-swallowed, orphaning it). The follow-up commit reorders the teardown reconcile to run after
performStackTeardown, adds SDK retries, and makes the e2e assertion tolerant of eventual consistency. Net diff vsmainis the complete, hardened fix.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.