From bdb5e3a19d10a42abd0586d7b8275a8dcc2460c5 Mon Sep 17 00:00:00 2001 From: gitikavj Date: Thu, 15 Oct 2026 17:46:02 +0000 Subject: [PATCH 1/2] fix(migrate): backfill credential secret ARNs v0 didn't record v0 never records clientSecretArn for API-key credential providers, and v1's CDK app needs it to let a Gateway read the secret, so migrate failed at v1 synth for any v0 project with an authenticated Gateway target. Migrate now looks up each missing secret ARN with the selected target's credentials and records it in the synth workspace and the migrated deployed-state.json. Closes #2599 --- src/core/index.tsx | 1 + .../project/migrate/credentialSecrets.test.ts | 216 ++++++++++++++++++ src/core/project/migrate/credentialSecrets.ts | 141 ++++++++++++ src/core/project/migrate/index.apply.test.ts | 197 +++++++++++++++- src/core/project/migrate/index.test.ts | 3 +- src/core/project/migrate/index.ts | 59 ++++- src/core/project/migrate/workspace.test.ts | 27 ++- src/core/project/migrate/workspace.ts | 9 + 8 files changed, 644 insertions(+), 9 deletions(-) create mode 100644 src/core/project/migrate/credentialSecrets.test.ts create mode 100644 src/core/project/migrate/credentialSecrets.ts diff --git a/src/core/index.tsx b/src/core/index.tsx index 2c171dce8..f2999d0df 100644 --- a/src/core/index.tsx +++ b/src/core/index.tsx @@ -155,6 +155,7 @@ export class CoreClient implements AwsClients { new FsProjectMigrator({ logger: this.logger.child({ module: "projectMigrator" }), createCloudFormationClient: config.createCloudFormationClient, + identity: this.identity, }); } diff --git a/src/core/project/migrate/credentialSecrets.test.ts b/src/core/project/migrate/credentialSecrets.test.ts new file mode 100644 index 000000000..249607db0 --- /dev/null +++ b/src/core/project/migrate/credentialSecrets.test.ts @@ -0,0 +1,216 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { readFile } from "node:fs/promises"; +import { join } from "node:path"; +import { ResourceNotFoundException } from "@aws-sdk/client-bedrock-agentcore-control"; +import { + backfillCredentialSecrets, + gatewayCredentialNames, + type CredentialSecretLookups, +} from "./credentialSecrets"; +import { F1_DEPLOYED_STATE, F1_SPEC, writeV0Project } from "./migrate-test-support"; + +const cleanups: Array<() => Promise> = []; +afterEach(() => Promise.all(cleanups.splice(0).map((cleanup) => cleanup()))); + +const ARN = "arn:aws:bedrock-agentcore:us-east-1:998846730471"; +const KEY_SECRET = "arn:aws:secretsmanager:us-east-1:998846730471:secret:apikey/compatA-key-AbCdEf"; +const OAUTH_SECRET = + "arn:aws:secretsmanager:us-east-1:998846730471:secret:oauth2/compatA-oauth-GhIjKl"; +const OPTIONS = { region: "us-east-1" }; +const DENIED = "User is not authorized to perform this action"; + +/** Identity reads answered from a map of provider name to secret ARN (or the error to throw). */ +function fakeIdentity(secrets: Record) { + const calls: string[] = []; + const answer = (kind: string, name: string) => { + calls.push(`${kind}:${name}`); + const secret = secrets[name]; + if (secret instanceof Error) throw secret; + return secret; + }; + const identity: CredentialSecretLookups = { + getApiKeyCredentialProvider: async (name) => { + const secretArn = answer("apiKey", name); + return { ...(secretArn && { apiKeySecretArn: { secretArn } }) } as never; + }, + getOauth2CredentialProvider: async (name) => { + const secretArn = answer("oauth2", name); + return { ...(secretArn && { clientSecretArn: { secretArn } }) } as never; + }, + }; + return Object.assign(identity, { calls }); +} + +async function project(deployedState: unknown = F1_DEPLOYED_STATE): Promise { + const { root, cleanup } = await writeV0Project({ deployedState }); + cleanups.push(cleanup); + return root; +} + +function backfill( + root: string, + identity: CredentialSecretLookups, + gatewayCredentials: string[] = [], + credentials: { name: string; authorizerType: string }[] = F1_SPEC.credentials, +) { + return backfillCredentialSecrets({ + identity, + rootPath: root, + credentials, + gatewayCredentials: new Set(gatewayCredentials), + target: "default", + options: OPTIONS, + }); +} + +const recorded = (deployedState: string | undefined) => + JSON.parse(deployedState!).targets.default.resources.credentials; + +const notFound = () => new ResourceNotFoundException({ message: "not found", $metadata: {} }); + +describe("backfillCredentialSecrets", () => { + test("fills in the API key and OAuth secret ARNs v0 didn't record", async () => { + const root = await project(); + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + + const result = await backfill(root, identity); + + expect(identity.calls).toEqual(["apiKey:compatA-key", "oauth2:compatA-oauth"]); + expect(recorded(result.deployedState)).toEqual({ + "compatA-key": { + credentialProviderArn: `${ARN}:token-vault/default/apikeycredentialprovider/compatA-key`, + clientSecretArn: KEY_SECRET, + }, + "compatA-oauth": { + credentialProviderArn: `${ARN}:token-vault/default/oauth2credentialprovider/compatA-oauth`, + clientSecretArn: OAUTH_SECRET, + }, + }); + // Everything else in the file is kept as it was. + expect(JSON.parse(result.deployedState!).targets.default.resources.memories).toEqual( + F1_DEPLOYED_STATE.targets.default.resources.memories, + ); + expect(result.check).toEqual({ + name: "credential secrets", + ok: true, + message: + "found the secret ARNs v0 didn't record for compatA-key, compatA-oauth; migrating " + + "records them in agentcore/.cli/deployed-state.json", + }); + expect(result.warnings).toEqual([]); + // The project itself isn't written. + expect( + JSON.parse(await readFile(join(root, "agentcore", ".cli", "deployed-state.json"), "utf8")), + ).toEqual(F1_DEPLOYED_STATE); + }); + + test("makes no lookups when every secret ARN is recorded, and skips payment providers", async () => { + const credentials = { + "compatA-key": { + credentialProviderArn: `${ARN}:token-vault/default/apikeycredentialprovider/compatA-key`, + clientSecretArn: KEY_SECRET, + }, + "compatA-pay": { + credentialProviderArn: `${ARN}:token-vault/default/paymentcredentialprovider/compatA-pay`, + }, + }; + const root = await project({ + targets: { + default: { resources: { ...F1_DEPLOYED_STATE.targets.default.resources, credentials } }, + }, + }); + const identity = fakeIdentity({}); + + const result = await backfill( + root, + identity, + [], + [ + { name: "compatA-key", authorizerType: "ApiKeyCredentialProvider" }, + { name: "compatA-pay", authorizerType: "PaymentCredentialProvider" }, + ], + ); + + expect(identity.calls).toEqual([]); + expect(result).toEqual({ warnings: [] }); + }); + + test("fails the check for a Gateway's credential whose secret ARN can't be read", async () => { + const root = await project(); + const identity = fakeIdentity({ + "compatA-key": new Error(DENIED), + "compatA-oauth": OAUTH_SECRET, + }); + + const result = await backfill(root, identity, ["compatA-key"]); + + expect(result.check?.ok).toBe(false); + expect(result.check?.message).toContain( + `compatA-key: GetApiKeyCredentialProvider failed (${DENIED}); check that these ` + + "credentials can call bedrock-agentcore:GetApiKeyCredentialProvider in us-east-1", + ); + // Only the API actually called is named. + expect(result.check?.message).not.toContain("GetOauth2CredentialProvider"); + expect(result.check?.message).toContain("run `agentcore migrate` again"); + expect(result.check?.message).toContain("The secret ARNs of compatA-oauth were found."); + expect(result.warnings).toEqual([]); + expect(recorded(result.deployedState)["compatA-key"].clientSecretArn).toBeUndefined(); + expect(recorded(result.deployedState)["compatA-oauth"].clientSecretArn).toBe(OAUTH_SECRET); + }); + + test("only warns for credentials no Gateway target uses, with advice for each cause", async () => { + const root = await project(); + const identity = fakeIdentity({ "compatA-key": notFound() }); + + const result = await backfill(root, identity); + + expect(result.check).toBeUndefined(); + expect(result.deployedState).toBeUndefined(); + expect(result.warnings).toEqual([ + "v0 didn't record the secret ARN of credential compatA-key, and it couldn't be found: its " + + "provider 'compatA-key' no longer exists in AgentCore Identity; recreate it, or remove " + + "the credential from the project. No Gateway target uses it, so migrating doesn't need it.", + "v0 didn't record the secret ARN of credential compatA-oauth, and it couldn't be found: its " + + "provider 'compatA-oauth' has no stored secret. No Gateway target uses it, so migrating " + + "doesn't need it.", + ]); + }); + + test("names the OAuth API when an OAuth provider's lookup fails", async () => { + const root = await project(); + const identity = fakeIdentity({ + "compatA-key": KEY_SECRET, + "compatA-oauth": new Error(DENIED), + }); + + const result = await backfill(root, identity, ["compatA-oauth"]); + + expect(result.check?.ok).toBe(false); + expect(result.check?.message).toContain( + `compatA-oauth: GetOauth2CredentialProvider failed (${DENIED})`, + ); + expect(result.check?.message).not.toContain("GetApiKeyCredentialProvider"); + }); +}); + +describe("gatewayCredentialNames", () => { + test("names the credentials Gateway targets authenticate with by API key or OAuth", () => { + const spec = { + agentCoreGateways: [ + { + targets: [ + { outboundAuth: { type: "API_KEY", credentialName: "orders" } }, + { outboundAuth: { type: "OAUTH", credentialName: "crm" } }, + { outboundAuth: { type: "NONE" } }, + { outboundAuth: { type: "GATEWAY_IAM_ROLE", credentialName: "ignored" } }, + {}, + ], + }, + {}, + ], + }; + + expect([...gatewayCredentialNames(spec)]).toEqual(["orders", "crm"]); + expect(gatewayCredentialNames({}).size).toBe(0); + }); +}); diff --git a/src/core/project/migrate/credentialSecrets.ts b/src/core/project/migrate/credentialSecrets.ts new file mode 100644 index 000000000..b3489e94e --- /dev/null +++ b/src/core/project/migrate/credentialSecrets.ts @@ -0,0 +1,141 @@ +import { readFile } from "node:fs/promises"; +import { join } from "node:path"; +import { ResourceNotFoundException } from "@aws-sdk/client-bedrock-agentcore-control"; +import type { CoreIdentityClient } from "../../../handlers/identity/types"; +import type { MigrationCheck } from "../../../handlers/project/migrate/types"; +import type { CoreOptions } from "../../types"; +import { DEPLOYED_STATE_RELATIVE_PATH } from "../backends/cdk/deployedState"; + +/** The Identity reads that report a credential provider's secret ARN. */ +export type CredentialSecretLookups = Pick< + CoreIdentityClient, + "getApiKeyCredentialProvider" | "getOauth2CredentialProvider" +>; + +type RecordedCredential = { credentialProviderArn?: string; clientSecretArn?: string }; + +type LooseDeployedState = { + targets?: Record } }>; +}; + +type LooseSpec = { + agentCoreGateways?: { + targets?: { outboundAuth?: { type?: string; credentialName?: string } }[]; + }[]; +}; + +/** + * The credentials a Gateway target authenticates with (API key or OAuth outbound + * auth): the only ones v1's CDK app can't synthesize without a secret ARN. Reads a + * v0 or v1 spec alike, since both name them the same way. + */ +export function gatewayCredentialNames(spec: unknown): Set { + const names = new Set(); + for (const gateway of (spec as LooseSpec).agentCoreGateways ?? []) { + for (const target of gateway.targets ?? []) { + const auth = target.outboundAuth; + if ((auth?.type === "API_KEY" || auth?.type === "OAUTH") && auth.credentialName) { + names.add(auth.credentialName); + } + } + } + return names; +} + +/** + * v1's CDK app grants a Gateway read access to a credential's secret by the ARN + * deployed-state.json records, but v0 never records it for API key providers and + * doesn't always for OAuth ones. Looks up each of the target's API key and OAuth + * credentials recorded without one and returns the deployed state with the ARNs + * filled in (the file's new contents, only when something was filled in). A + * credential a Gateway target uses and whose ARN can't be found fails the check; + * any other is only a warning, since nothing synthesizes from it. No lookups are + * made when nothing is missing. + */ +export async function backfillCredentialSecrets(input: { + identity: CredentialSecretLookups; + rootPath: string; + /** The spec's credentials, for which kind of provider each one is. */ + credentials: { name: string; authorizerType: string }[]; + /** The credentials a Gateway target uses; see {@link gatewayCredentialNames}. */ + gatewayCredentials: Set; + target: string; + options: CoreOptions; +}): Promise<{ deployedState?: string; check?: MigrationCheck; warnings: string[] }> { + const statePath = join(input.rootPath, DEPLOYED_STATE_RELATIVE_PATH); + // Read as written rather than through the schema, so what isn't patched is kept as is. + const state = JSON.parse(await readFile(statePath, "utf8")) as LooseDeployedState; + const recorded = state.targets?.[input.target]?.resources?.credentials ?? {}; + const kinds = new Map(input.credentials.map((c) => [c.name, c.authorizerType])); + const missing = Object.entries(recorded).filter( + ([name, entry]) => + entry.credentialProviderArn !== undefined && + entry.clientSecretArn === undefined && + (kinds.get(name) === "ApiKeyCredentialProvider" || + kinds.get(name) === "OAuthCredentialProvider"), + ); + if (missing.length === 0) return { warnings: [] }; + + const found: string[] = []; + const blocking: string[] = []; + const warnings: string[] = []; + for (const [name, entry] of missing) { + // The provider's name is the ARN's last segment, whatever naming the v0 release used. + const provider = entry.credentialProviderArn!.split("/").at(-1)!; + const apiKey = kinds.get(name) === "ApiKeyCredentialProvider"; + const api = apiKey ? "GetApiKeyCredentialProvider" : "GetOauth2CredentialProvider"; + let reason: string; + try { + const secretArn = apiKey + ? (await input.identity.getApiKeyCredentialProvider(provider, input.options)) + .apiKeySecretArn?.secretArn + : (await input.identity.getOauth2CredentialProvider(provider, input.options)) + .clientSecretArn?.secretArn; + if (secretArn !== undefined) { + entry.clientSecretArn = secretArn; + found.push(name); + continue; + } + reason = `its provider '${provider}' has no stored secret`; + } catch (error) { + reason = + error instanceof ResourceNotFoundException + ? `its provider '${provider}' no longer exists in AgentCore Identity; recreate it, or ` + + "remove the credential from the project" + : `${api} failed (${(error as Error).message}); check that these credentials can call ` + + `bedrock-agentcore:${api} in ${input.options.region}`; + } + if (input.gatewayCredentials.has(name)) { + blocking.push(`${name}: ${reason}`); + } else { + warnings.push( + `v0 didn't record the secret ARN of credential ${name}, and it couldn't be found: ` + + `${reason}. No Gateway target uses it, so migrating doesn't need it.`, + ); + } + } + + const name = "credential secrets"; + const recordedMessage = + `found the secret ARNs v0 didn't record for ${found.join(", ")}; migrating records them ` + + `in ${DEPLOYED_STATE_RELATIVE_PATH.split("\\").join("/")}`; + let check: MigrationCheck | undefined; + if (blocking.length > 0) { + check = { + name, + ok: false, + message: + "v0 didn't record the secret ARN v1's CDK app needs to let a Gateway read " + + `${blocking.length === 1 ? "this credential" : "these credentials"}, and it couldn't ` + + `be found. ${blocking.join(". ")}. Then run \`agentcore migrate\` again.` + + (found.length > 0 ? ` The secret ARNs of ${found.join(", ")} were found.` : ""), + }; + } else if (found.length > 0) { + check = { name, ok: true, message: recordedMessage }; + } + return { + ...(found.length > 0 && { deployedState: `${JSON.stringify(state, null, 2)}\n` }), + ...(check && { check }), + warnings, + }; +} diff --git a/src/core/project/migrate/index.apply.test.ts b/src/core/project/migrate/index.apply.test.ts index 9330f2099..e31dc4c04 100644 --- a/src/core/project/migrate/index.apply.test.ts +++ b/src/core/project/migrate/index.apply.test.ts @@ -12,10 +12,12 @@ import { STATE_RELATIVE_PATH, writeMigrationState, } from "./journal"; +import type { CredentialSecretLookups } from "./credentialSecrets"; import { CDK_LIBRARY_OVERRIDE_ENV, FsProjectMigrator } from "./index"; import { commitAll, F1_DEPLOYED_STATE, + F1_SPEC, FAKE_AWS_IDENTITY, fakeCloudFormation, fakeSynth, @@ -25,6 +27,7 @@ import { type FakeCloudFormationOptions, type V0ProjectFiles, } from "./migrate-test-support"; +import type { WorkspaceSynthesizer } from "./synth"; const cleanups: Array<() => Promise> = []; afterEach(() => Promise.all(cleanups.splice(0).map((cleanup) => cleanup()))); @@ -61,6 +64,9 @@ function migrator( vendedVersion?: string; env?: NodeJS.ProcessEnv; account?: string; + identity?: CredentialSecretLookups; + /** Called with each workspace the v1 app is synthesized in, before it synthesizes. */ + onSynth?: (workspaceDirectory: string) => Promise; } = {}, ) { const stacks = options.stacks ?? [{ name: STACK, target: "default" }]; @@ -80,19 +86,63 @@ function migrator( createCloudFormationClient: cloudFormation.create, ...FAKE_AWS_IDENTITY, ...(options.account && { resolveAccount: async () => options.account! }), - synthesize: fakeSynth( - stacks.map((stack) => ({ - target: stack.target, - stackName: stack.name, - template: templateFixture("pinned.v1"), - })), + synthesize: synthesizer( + fakeSynth( + stacks.map((stack) => ({ + target: stack.target, + stackName: stack.name, + template: templateFixture("pinned.v1"), + })), + ), + options.onSynth, ), + ...(options.identity && { identity: options.identity }), env: options.env ?? {}, poll: { intervalMs: 0, sleep: async () => {} }, }); return { subject, cloudFormation, run }; } +function synthesizer( + synthesize: WorkspaceSynthesizer, + onSynth?: (workspaceDirectory: string) => Promise, +): WorkspaceSynthesizer { + if (!onSynth) return synthesize; + return async (workspaceDirectory, options) => { + await onSynth(workspaceDirectory); + return synthesize(workspaceDirectory, options); + }; +} + +/** Answers each provider's secret ARN from `secrets` (an Error is thrown), recording the calls. */ +function fakeIdentity(secrets: Record) { + const calls: string[] = []; + const secretArn = (name: string) => { + calls.push(name); + const secret = secrets[name]; + if (secret instanceof Error) throw secret; + return secret === undefined ? undefined : { secretArn: secret }; + }; + const identity: CredentialSecretLookups = { + getApiKeyCredentialProvider: async (name) => ({ apiKeySecretArn: secretArn(name) }) as never, + getOauth2CredentialProvider: async (name) => ({ clientSecretArn: secretArn(name) }) as never, + }; + return Object.assign(identity, { calls }); +} + +const KEY_SECRET = "arn:aws:secretsmanager:us-east-1:998846730471:secret:apikey/compatA-key-AbCdEf"; +const OAUTH_SECRET = + "arn:aws:secretsmanager:us-east-1:998846730471:secret:oauth2/compatA-oauth-GhIjKl"; +const STATE_PATH = join("agentcore", ".cli", "deployed-state.json"); +const DENIED = "User is not authorized to perform this action"; + +/** The secret ARN recorded for each of a target's credentials, in the file's order. */ +const secretArns = (target: unknown) => + Object.values( + (target as { resources: { credentials: Record } }) + .resources.credentials, + ).map((credential) => credential.clientSecretArn); + const readJson = async (path: string) => JSON.parse(await readFile(path, "utf8")); describe("FsProjectMigrator: applying a migration", () => { @@ -133,6 +183,141 @@ describe("FsProjectMigrator: applying a migration", () => { expect(executed).toHaveLength(1); }); + test("records the secret ARNs v0 didn't, for the synth and in the migrated project", async () => { + const root = await committedProject(); + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + const synthesized: unknown[] = []; + const { subject } = migrator({ + identity, + onSynth: async (workspace) => { + synthesized.push((await readJson(join(workspace, STATE_PATH))).targets.default); + }, + }); + + const prepared = await subject.prepare(root); + expect(prepared.plan.ready).toBe(true); + expect(prepared.plan.checks).toContainEqual({ + name: "credential secrets", + ok: true, + message: + "found the secret ARNs v0 didn't record for compatA-key, compatA-oauth; migrating " + + "records them in agentcore/.cli/deployed-state.json", + }); + expect(prepared.plan.changes).toContainEqual({ + path: "agentcore/.cli/deployed-state.json", + kind: "modified", + }); + expect(synthesized.map(secretArns)).toEqual([[KEY_SECRET, OAUTH_SECRET]]); + // Planning leaves the project's deployed state alone. + expect(await readJson(join(root, STATE_PATH))).toEqual(F1_DEPLOYED_STATE); + + const result = await prepared.apply(); + await prepared.dispose(); + + expect(result.migrated).toBe(true); + const state = await readJson(join(root, STATE_PATH)); + expect(secretArns(state.targets.default)).toEqual([KEY_SECRET, OAUTH_SECRET]); + expect(state.targets.default.resources.memories).toEqual( + F1_DEPLOYED_STATE.targets.default.resources.memories, + ); + expect(identity.calls).toEqual(["compatA-key", "compatA-oauth"]); + }); + + test("only warns when a credential no Gateway target uses has no readable secret ARN", async () => { + const root = await committedProject(); + const identity = fakeIdentity({ + "compatA-key": new Error(DENIED), + "compatA-oauth": OAUTH_SECRET, + }); + const { subject } = migrator({ identity }); + + const plan = await subject.plan(root); + + // F1's only Gateway target authenticates with nothing, so neither credential blocks. + expect(plan.ready).toBe(true); + expect(plan.checks.find((entry) => entry.name === "credential secrets")?.ok).toBe(true); + expect(plan.warnings).toContainEqual( + expect.stringContaining( + `credential compatA-key, and it couldn't be found: GetApiKeyCredentialProvider failed (${DENIED})`, + ), + ); + }); + + test("blocks with the credential named when a Gateway target's secret ARN can't be read", async () => { + const [gateway] = F1_SPEC.agentCoreGateways; + const root = await committedProject({ + spec: { + ...F1_SPEC, + agentCoreGateways: [ + { + ...gateway!, + targets: gateway!.targets.map((target) => ({ + ...target, + outboundAuth: { type: "OAUTH", credentialName: "compatA-oauth", scopes: ["openid"] }, + })), + }, + ], + }, + }); + const identity = fakeIdentity({ + "compatA-key": KEY_SECRET, + "compatA-oauth": new Error(DENIED), + }); + const { subject } = migrator({ identity }); + + const plan = await subject.plan(root); + + expect(plan.problems).toEqual([]); + expect(plan.ready).toBe(false); + const check = plan.checks.find((entry) => entry.name === "credential secrets"); + expect(check?.ok).toBe(false); + expect(check?.message).toContain( + `compatA-oauth: GetOauth2CredentialProvider failed (${DENIED})`, + ); + expect(await readJson(join(root, STATE_PATH))).toEqual(F1_DEPLOYED_STATE); + }); + + test("records only the selected target's secret ARNs, and a pending target's only for its synth", async () => { + const root = await committedProject(twoTargets()); + const stacks = [ + { name: STACK, target: "default" }, + { name: PROD_STACK, target: "prod" }, + ]; + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + const first = migrator({ stacks, identity }); + const prepared = await first.subject.prepare(root, { target: "default" }); + expect(prepared.plan.ready).toBe(true); + await prepared.apply(); + await prepared.dispose(); + + const written = await readJson(join(root, STATE_PATH)); + expect(secretArns(written.targets.default)).toEqual([KEY_SECRET, OAUTH_SECRET]); + expect(secretArns(written.targets.prod)).toEqual([undefined, undefined]); + + const synthesized: unknown[] = []; + const rerun = migrator({ + stacks, + sharedCloudFormation: first.cloudFormation, + identity, + onSynth: async (workspace) => { + synthesized.push((await readJson(join(workspace, STATE_PATH))).targets.prod); + }, + }); + const finishing = await rerun.subject.prepare(root, { target: "prod" }); + expect(finishing.plan.finishPending).toBe(true); + expect(finishing.plan.ready).toBe(true); + expect(finishing.plan.changes).toEqual([]); + expect(synthesized.map(secretArns)).toEqual([[KEY_SECRET, OAUTH_SECRET]]); + await finishing.apply(); + await finishing.dispose(); + + // The pending run's files were already written, so the next deploy records prod's. + expect(secretArns((await readJson(join(root, STATE_PATH))).targets.prod)).toEqual([ + undefined, + undefined, + ]); + }); + test("refuses to apply when the tree changed after planning", async () => { const root = await committedProject(); const { subject, cloudFormation } = migrator(); diff --git a/src/core/project/migrate/index.test.ts b/src/core/project/migrate/index.test.ts index 030244445..8fd4a7f50 100644 --- a/src/core/project/migrate/index.test.ts +++ b/src/core/project/migrate/index.test.ts @@ -94,7 +94,8 @@ describe("FsProjectMigrator.plan", () => { "Dropped fields v1 doesn't use: credentials[compatA-oauth].usage", expect.stringContaining("This project has credentials (compatA-key, compatA-oauth)."), ]); - // The project isn't written; deployed state isn't part of the migration. + // The project isn't written, and deployed state isn't changed unless planning filled in + // secret ARNs v0 didn't record (this migrator has no Identity client to look them up). expect(plan.changes).not.toContainEqual( expect.objectContaining({ path: "agentcore/.cli/deployed-state.json" }), ); diff --git a/src/core/project/migrate/index.ts b/src/core/project/migrate/index.ts index 82180d969..f6ea528b6 100644 --- a/src/core/project/migrate/index.ts +++ b/src/core/project/migrate/index.ts @@ -44,6 +44,11 @@ import { type MigrationState, } from "./journal"; import { convertHarness } from "./convertHarness"; +import { + backfillCredentialSecrets, + gatewayCredentialNames, + type CredentialSecretLookups, +} from "./credentialSecrets"; import { checkGitGate } from "./gitGate"; import { AWS_TARGETS_RELATIVE_PATH, @@ -94,6 +99,8 @@ export type FsProjectMigratorConfig = { poll?: PollOptions; /** Builds the store large templates are uploaded to; defaults to the CDK bootstrap bucket. */ templateStore?: (target: AwsDeploymentTarget) => TemplateStore; + /** Reads the secret ARNs v0 didn't record; without it, deployed state is used as v0 left it. */ + identity?: CredentialSecretLookups; }; /** A deployment target with a deployed stack, and what planning has read about it. */ @@ -145,6 +152,7 @@ export class FsProjectMigrator implements ProjectMigrator { private readonly env: NodeJS.ProcessEnv; private readonly poll?: PollOptions; private readonly templateStore?: (target: AwsDeploymentTarget) => TemplateStore; + private readonly identity?: CredentialSecretLookups; constructor(config: FsProjectMigratorConfig) { this.runner = config.runner ?? runProcess; @@ -160,6 +168,7 @@ export class FsProjectMigrator implements ProjectMigrator { config.synthesize ?? createWorkspaceSynthesizer({ runner: this.runner, env: this.env }); if (config.poll) this.poll = config.poll; if (config.templateStore) this.templateStore = config.templateStore; + if (config.identity) this.identity = config.identity; } public async plan(rootPath: string, options: MigrationPlanOptions = {}): Promise { @@ -249,6 +258,11 @@ export class FsProjectMigrator implements ProjectMigrator { if (selection.check) checks.push(selection.check); const { selected } = selection; if (selected) await this.readSelectedStack(selected, checks, skippedStacks, options); + // A project that doesn't convert isn't synthesized, so its secrets aren't needed. + const secrets = + problems.length > 0 + ? { warnings: [] } + : await this.backfillSecrets(rootPath, v0Spec, selected, checks); const warnings = [ ...converted.warnings, @@ -257,6 +271,7 @@ export class FsProjectMigrator implements ProjectMigrator { ...credentialNameWarnings(v0Spec.credentials.map((credential) => credential.name)), ...library.warnings, ...overrideWarnings(cdkLibraryOverride), + ...secrets.warnings, ]; const workspace = await buildMigrationWorkspace({ @@ -266,6 +281,7 @@ export class FsProjectMigrator implements ProjectMigrator { yaml === undefined ? [] : [{ directory, yaml }], ), assetSource: this.assetSource, + ...(secrets.deployedState !== undefined && { deployedState: secrets.deployedState }), }); try { // The selected stack is planned whatever else failed, so one run shows @@ -386,6 +402,30 @@ export class FsProjectMigrator implements ProjectMigrator { }; } + /** + * Fills in the secret ARNs v0 didn't record for the selected target's credentials, + * read with the credentials its stack was read with; returns deployed-state.json's + * new contents when any were. + */ + private async backfillSecrets( + rootPath: string, + spec: { credentials: { name: string; authorizerType: string }[] }, + selected: DeployedTarget | undefined, + checks: MigrationCheck[], + ): Promise<{ deployedState?: string; warnings: string[] }> { + if (!this.identity || selected?.template === undefined) return { warnings: [] }; + const { deployedState, check, warnings } = await backfillCredentialSecrets({ + identity: this.identity, + rootPath, + credentials: spec.credentials, + gatewayCredentials: gatewayCredentialNames(spec), + target: selected.target.name, + options: { region: selected.target.region, credentials: selected.credentials }, + }); + if (check) checks.push(check); + return { ...(deployedState !== undefined && { deployedState }), warnings }; + } + /** Reads the selected target's stack, recording why it was skipped when it can't be. */ private async readSelectedStack( entry: DeployedTarget, @@ -566,11 +606,28 @@ export class FsProjectMigrator implements ProjectMigrator { } try { if (selected?.template !== undefined) { + const spec = JSON.parse(await readFile(join(rootPath, SPEC_RELATIVE_PATH), "utf8")) as { + credentials?: unknown; + }; + // Only the synth reads it here: a pending migration's files are already written. + const secrets = await this.backfillSecrets( + rootPath, + { + ...spec, + credentials: Array.isArray(spec.credentials) + ? (spec.credentials as { name: string; authorizerType: string }[]) + : [], + }, + selected, + checks, + ); + warnings.push(...secrets.warnings); workspace = await buildMigrationWorkspace({ rootPath, - spec: JSON.parse(await readFile(join(rootPath, SPEC_RELATIVE_PATH), "utf8")), + spec, harnesses: [], assetSource: this.assetSource, + ...(secrets.deployedState !== undefined && { deployedState: secrets.deployedState }), }); const result = await this.planStacks(workspace.directory, [selected], checks, [], options); planned = result.planned[0]; diff --git a/src/core/project/migrate/workspace.test.ts b/src/core/project/migrate/workspace.test.ts index a3de53bb4..887b017f3 100644 --- a/src/core/project/migrate/workspace.test.ts +++ b/src/core/project/migrate/workspace.test.ts @@ -63,6 +63,30 @@ describe("buildMigrationWorkspace", () => { expect(byPath["app/CompatAAgent/main.py"]).toBeUndefined(); }); + test("writes the deployed state planning filled in, as a modified file", async () => { + const { root, cleanup } = await writeV0Project(); + cleanups.push(cleanup); + const statePath = join("agentcore", ".cli", "deployed-state.json"); + const deployedState = `${JSON.stringify({ targets: { default: { resources: {} } } }, null, 2)}\n`; + + const workspace = await buildMigrationWorkspace({ + rootPath: root, + spec: { name: "CompatA", version: 2, migratedFrom: "v1" }, + harnesses: [], + assetSource: new FsAssetSource(), + deployedState, + }); + cleanups.push(workspace.dispose); + + expect(await readFile(join(workspace.directory, statePath), "utf8")).toBe(deployedState); + expect(workspace.changes).toContainEqual({ + path: "agentcore/.cli/deployed-state.json", + kind: "modified", + }); + // The project's own copy isn't written. + expect(await readFile(join(root, statePath), "utf8")).not.toBe(deployedState); + }); + test("copies the deployed state and the vended app as they are", async () => { const { root, cleanup } = await writeV0Project(); cleanups.push(cleanup); @@ -77,7 +101,8 @@ describe("buildMigrationWorkspace", () => { }); cleanups.push(workspace.dispose); - // migratedFrom in agentcore.json keeps the names; deployed state isn't touched. + // migratedFrom in agentcore.json keeps the names; deployed state isn't touched unless + // planning filled in secret ARNs v0 didn't record. expect(await readFile(join(workspace.directory, statePath), "utf8")).toBe(originalState); expect(workspace.changes).not.toContainEqual( expect.objectContaining({ path: "agentcore/.cli/deployed-state.json" }), diff --git a/src/core/project/migrate/workspace.ts b/src/core/project/migrate/workspace.ts index b71736760..0efba8be6 100644 --- a/src/core/project/migrate/workspace.ts +++ b/src/core/project/migrate/workspace.ts @@ -14,6 +14,7 @@ import { constants, tmpdir } from "node:os"; import { basename, dirname, isAbsolute, join, relative, sep } from "node:path"; import { ProjectStateError } from "../../../errors"; import type { FileChange } from "../../../handlers/project/migrate/types"; +import { DEPLOYED_STATE_RELATIVE_PATH } from "../backends/cdk/deployedState"; import type { AssetSource } from "../source"; import { PROJECT_SPEC_RELATIVE_PATH } from "../fsUtils"; import { FsTreeNode } from "../templates/fsTree"; @@ -54,6 +55,8 @@ export type WorkspaceInput = { harnesses: { directory: string; yaml: string }[]; /** Where v1's vended CDK app (`cdk/`) is read from. */ assetSource: AssetSource; + /** Replaces the copied deployed-state.json, when planning filled in what v0 didn't record. */ + deployedState?: string; }; export type MigrationWorkspace = { @@ -106,6 +109,12 @@ export async function buildMigrationWorkspace(input: WorkspaceInput): Promise Date: Thu, 15 Oct 2026 19:48:31 +0000 Subject: [PATCH 2/2] fix(migrate): backfill secret ARNs for every deployed target The vended CDK app synthesizes a stack for every target, so backfilling only the selected target still failed v1 synth (and build/deploy after migrating) for a multi-target project with an authenticated Gateway. Look up missing secret ARNs for every deployed target the active credentials' account owns, resolving credentials once per region, and fail the check with recovery steps for a target in another account whose Gateway credentials lack one. --- .../project/migrate/credentialSecrets.test.ts | 100 ++++++++++- src/core/project/migrate/credentialSecrets.ts | 170 ++++++++++++------ src/core/project/migrate/index.apply.test.ts | 163 ++++++++++++++--- src/core/project/migrate/index.ts | 53 +++++- 4 files changed, 396 insertions(+), 90 deletions(-) diff --git a/src/core/project/migrate/credentialSecrets.test.ts b/src/core/project/migrate/credentialSecrets.test.ts index 249607db0..608edd006 100644 --- a/src/core/project/migrate/credentialSecrets.test.ts +++ b/src/core/project/migrate/credentialSecrets.test.ts @@ -6,6 +6,7 @@ import { backfillCredentialSecrets, gatewayCredentialNames, type CredentialSecretLookups, + type TargetAccess, } from "./credentialSecrets"; import { F1_DEPLOYED_STATE, F1_SPEC, writeV0Project } from "./migrate-test-support"; @@ -16,7 +17,6 @@ const ARN = "arn:aws:bedrock-agentcore:us-east-1:998846730471"; const KEY_SECRET = "arn:aws:secretsmanager:us-east-1:998846730471:secret:apikey/compatA-key-AbCdEf"; const OAUTH_SECRET = "arn:aws:secretsmanager:us-east-1:998846730471:secret:oauth2/compatA-oauth-GhIjKl"; -const OPTIONS = { region: "us-east-1" }; const DENIED = "User is not authorized to perform this action"; /** Identity reads answered from a map of provider name to secret ARN (or the error to throw). */ @@ -47,19 +47,25 @@ async function project(deployedState: unknown = F1_DEPLOYED_STATE): Promise Promise = async (target) => ({ + options: { region: target.region }, + }), ) { return backfillCredentialSecrets({ identity, rootPath: root, credentials, gatewayCredentials: new Set(gatewayCredentials), - target: "default", - options: OPTIONS, + targets, + access, }); } @@ -193,6 +199,94 @@ describe("backfillCredentialSecrets", () => { }); }); +describe("backfillCredentialSecrets across targets", () => { + const PROD = { name: "prod", account: "111111111111", region: "eu-west-1" }; + const twoTargets = { + targets: { + default: F1_DEPLOYED_STATE.targets.default, + prod: F1_DEPLOYED_STATE.targets.default, + }, + }; + const otherAccount = async (target: { region: string }): Promise => + target.region === "eu-west-1" ? { account: "998846730471" } : { options: target }; + + test("fails the check for another account's Gateway credential, saying how to record it", async () => { + const root = await project(twoTargets); + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + + const result = await backfill( + root, + identity, + ["compatA-key"], + F1_SPEC.credentials, + [DEFAULT, PROD], + otherAccount, + ); + + // Only the reachable target is looked up. + expect(identity.calls).toEqual(["apiKey:compatA-key", "oauth2:compatA-oauth"]); + expect(result.check?.ok).toBe(false); + expect(result.check?.message).toBe( + "Target prod is in account 111111111111 (eu-west-1), but the active credentials are for " + + "account 998846730471. v1's CDK app synthesizes every target's stack, so it needs the " + + "secret ARNs of compatA-key there too, and migrate reads every target with the one set " + + "of credentials it runs with. Record them by hand: with credentials for that account, " + + "run `aws bedrock-agentcore-control get-api-key-credential-provider` (or " + + "get-oauth2-credential-provider) --name --region eu-west-1, add the secret " + + "ARN as clientSecretArn under targets.prod.resources.credentials. in " + + "agentcore/.cli/deployed-state.json, commit it, then run `agentcore migrate` again. " + + "The secret ARNs of compatA-key (target default), compatA-oauth (target default) were found.", + ); + expect(result.warnings).toEqual([ + "v0 didn't record the secret ARN of credential compatA-oauth (target prod), and it " + + "couldn't be found: target prod is in account 111111111111, not the active account " + + "998846730471. No Gateway target uses it, so migrating doesn't need it.", + ]); + const state = JSON.parse(result.deployedState!); + expect(state.targets.default.resources.credentials["compatA-key"].clientSecretArn).toBe( + KEY_SECRET, + ); + expect(state.targets.prod.resources.credentials["compatA-key"].clientSecretArn).toBeUndefined(); + }); + + test("only warns for another account's credentials no Gateway target uses", async () => { + const root = await project(twoTargets); + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + + const result = await backfill( + root, + identity, + [], + F1_SPEC.credentials, + [DEFAULT, PROD], + otherAccount, + ); + + expect(result.check?.ok).toBe(true); + expect(result.warnings).toHaveLength(2); + }); + + test("reports a region whose credentials can't be resolved", async () => { + const root = await project(twoTargets); + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + + const result = await backfill( + root, + identity, + ["compatA-key"], + F1_SPEC.credentials, + [DEFAULT, { ...PROD, account: DEFAULT.account }], + async (target) => + target.region === "eu-west-1" ? { error: "no profile" } : { options: target }, + ); + + expect(result.check?.ok).toBe(false); + expect(result.check?.message).toContain( + "compatA-key (target prod): no credentials could be resolved for eu-west-1 (no profile)", + ); + }); +}); + describe("gatewayCredentialNames", () => { test("names the credentials Gateway targets authenticate with by API key or OAuth", () => { const spec = { diff --git a/src/core/project/migrate/credentialSecrets.ts b/src/core/project/migrate/credentialSecrets.ts index b3489e94e..cc1e5fd71 100644 --- a/src/core/project/migrate/credentialSecrets.ts +++ b/src/core/project/migrate/credentialSecrets.ts @@ -18,6 +18,9 @@ type LooseDeployedState = { targets?: Record } }>; }; +/** deployed-state.json's path as messages show it, with `/` on every platform. */ +const STATE_PATH = DEPLOYED_STATE_RELATIVE_PATH.split("\\").join("/"); + type LooseSpec = { agentCoreGateways?: { targets?: { outboundAuth?: { type?: string; credentialName?: string } }[]; @@ -42,92 +45,151 @@ export function gatewayCredentialNames(spec: unknown): Set { return names; } +/** + * How a target's providers can be read: with options for its account and region, + * not at all because the active credentials are for another account, or not at + * all because no credentials could be resolved for its region. + */ +export type TargetAccess = { options: CoreOptions } | { account: string } | { error: string }; + /** * v1's CDK app grants a Gateway read access to a credential's secret by the ARN * deployed-state.json records, but v0 never records it for API key providers and - * doesn't always for OAuth ones. Looks up each of the target's API key and OAuth - * credentials recorded without one and returns the deployed state with the ARNs - * filled in (the file's new contents, only when something was filled in). A - * credential a Gateway target uses and whose ARN can't be found fails the check; - * any other is only a warning, since nothing synthesizes from it. No lookups are - * made when nothing is missing. + * doesn't always for OAuth ones. The app synthesizes every target's stack, so + * every deployed target needs them: for each target recorded with API key or + * OAuth credentials missing one, looks them up with the target's access and + * returns the deployed state with the ARNs filled in (the file's new contents, + * only when something was filled in). A credential a Gateway target uses and + * whose ARN can't be found, or can't be looked up because the target is in + * another account, fails the check; any other is only a warning, since nothing + * synthesizes from it. Nothing is resolved or looked up when nothing is missing. */ -export async function backfillCredentialSecrets(input: { +export async function backfillCredentialSecrets< + Target extends { name: string; account: string; region: string }, +>(input: { identity: CredentialSecretLookups; rootPath: string; /** The spec's credentials, for which kind of provider each one is. */ credentials: { name: string; authorizerType: string }[]; /** The credentials a Gateway target uses; see {@link gatewayCredentialNames}. */ gatewayCredentials: Set; - target: string; - options: CoreOptions; + /** The project's deployment targets; those without recorded credentials are skipped. */ + targets: Target[]; + access: (target: Target) => Promise; }): Promise<{ deployedState?: string; check?: MigrationCheck; warnings: string[] }> { const statePath = join(input.rootPath, DEPLOYED_STATE_RELATIVE_PATH); // Read as written rather than through the schema, so what isn't patched is kept as is. const state = JSON.parse(await readFile(statePath, "utf8")) as LooseDeployedState; - const recorded = state.targets?.[input.target]?.resources?.credentials ?? {}; const kinds = new Map(input.credentials.map((c) => [c.name, c.authorizerType])); - const missing = Object.entries(recorded).filter( - ([name, entry]) => - entry.credentialProviderArn !== undefined && - entry.clientSecretArn === undefined && - (kinds.get(name) === "ApiKeyCredentialProvider" || - kinds.get(name) === "OAuthCredentialProvider"), - ); - if (missing.length === 0) return { warnings: [] }; + const candidates = input.targets.flatMap((target) => { + const recorded = state.targets?.[target.name]?.resources?.credentials ?? {}; + const missing = Object.entries(recorded).filter( + ([name, entry]) => + entry.credentialProviderArn !== undefined && + entry.clientSecretArn === undefined && + (kinds.get(name) === "ApiKeyCredentialProvider" || + kinds.get(name) === "OAuthCredentialProvider"), + ); + return missing.length > 0 ? [{ target, missing }] : []; + }); + if (candidates.length === 0) return { warnings: [] }; + // Names are qualified by target only when more than one target is involved. + const label = (name: string, target: string) => + candidates.length > 1 ? `${name} (target ${target})` : name; const found: string[] = []; - const blocking: string[] = []; + const failed: string[] = []; + const otherAccounts: string[] = []; const warnings: string[] = []; - for (const [name, entry] of missing) { - // The provider's name is the ARN's last segment, whatever naming the v0 release used. - const provider = entry.credentialProviderArn!.split("/").at(-1)!; - const apiKey = kinds.get(name) === "ApiKeyCredentialProvider"; - const api = apiKey ? "GetApiKeyCredentialProvider" : "GetOauth2CredentialProvider"; - let reason: string; - try { - const secretArn = apiKey - ? (await input.identity.getApiKeyCredentialProvider(provider, input.options)) - .apiKeySecretArn?.secretArn - : (await input.identity.getOauth2CredentialProvider(provider, input.options)) - .clientSecretArn?.secretArn; - if (secretArn !== undefined) { - entry.clientSecretArn = secretArn; - found.push(name); - continue; + const unneeded = (name: string, reason: string) => + warnings.push( + `v0 didn't record the secret ARN of credential ${name}, and it couldn't be found: ` + + `${reason}. No Gateway target uses it, so migrating doesn't need it.`, + ); + for (const { target, missing } of candidates) { + const access = await input.access(target); + if ("account" in access) { + const needed = missing.map(([name]) => name).filter((n) => input.gatewayCredentials.has(n)); + for (const [name] of missing) { + if (needed.includes(name)) continue; + unneeded( + label(name, target.name), + `target ${target.name} is in account ${target.account}, not the active account ${access.account}`, + ); } - reason = `its provider '${provider}' has no stored secret`; - } catch (error) { - reason = - error instanceof ResourceNotFoundException - ? `its provider '${provider}' no longer exists in AgentCore Identity; recreate it, or ` + - "remove the credential from the project" - : `${api} failed (${(error as Error).message}); check that these credentials can call ` + - `bedrock-agentcore:${api} in ${input.options.region}`; - } - if (input.gatewayCredentials.has(name)) { - blocking.push(`${name}: ${reason}`); - } else { - warnings.push( - `v0 didn't record the secret ARN of credential ${name}, and it couldn't be found: ` + - `${reason}. No Gateway target uses it, so migrating doesn't need it.`, + if (needed.length === 0) continue; + otherAccounts.push( + `Target ${target.name} is in account ${target.account} (${target.region}), but the ` + + `active credentials are for account ${access.account}. v1's CDK app synthesizes ` + + `every target's stack, so it needs the secret ARNs of ${needed.join(", ")} there ` + + "too, and migrate reads every target with the one set of credentials it runs with. " + + "Record them by hand: with credentials for that account, run `aws " + + "bedrock-agentcore-control get-api-key-credential-provider` (or " + + `get-oauth2-credential-provider) --name --region ${target.region}, add the ` + + `secret ARN as clientSecretArn under targets.${target.name}.resources.credentials.` + + ` in ${STATE_PATH}, commit it, then run \`agentcore migrate\` again.`, ); + continue; + } + for (const [name, entry] of missing) { + // The provider's name is the ARN's last segment, whatever naming the v0 release used. + const provider = entry.credentialProviderArn!.split("/").at(-1)!; + const apiKey = kinds.get(name) === "ApiKeyCredentialProvider"; + const api = apiKey ? "GetApiKeyCredentialProvider" : "GetOauth2CredentialProvider"; + let reason: string; + if ("error" in access) { + reason = `no credentials could be resolved for ${target.region} (${access.error})`; + } else { + try { + const secretArn = apiKey + ? (await input.identity.getApiKeyCredentialProvider(provider, access.options)) + .apiKeySecretArn?.secretArn + : (await input.identity.getOauth2CredentialProvider(provider, access.options)) + .clientSecretArn?.secretArn; + if (secretArn !== undefined) { + entry.clientSecretArn = secretArn; + found.push(label(name, target.name)); + continue; + } + reason = `its provider '${provider}' has no stored secret`; + } catch (error) { + reason = + error instanceof ResourceNotFoundException + ? `its provider '${provider}' no longer exists in AgentCore Identity; recreate it, ` + + "or remove the credential from the project" + : `${api} failed (${(error as Error).message}); check that these credentials can ` + + `call bedrock-agentcore:${api} in ${target.region}`; + } + } + if (input.gatewayCredentials.has(name)) { + failed.push(`${label(name, target.name)}: ${reason}`); + } else { + unneeded(label(name, target.name), reason); + } } } const name = "credential secrets"; const recordedMessage = `found the secret ARNs v0 didn't record for ${found.join(", ")}; migrating records them ` + - `in ${DEPLOYED_STATE_RELATIVE_PATH.split("\\").join("/")}`; + `in ${STATE_PATH}`; + const blocking = [ + ...(failed.length > 0 + ? [ + "v0 didn't record the secret ARN v1's CDK app needs to let a Gateway read " + + `${failed.length === 1 ? "this credential" : "these credentials"}, and it ` + + `couldn't be found. ${failed.join(". ")}. Then run \`agentcore migrate\` again.`, + ] + : []), + ...otherAccounts, + ]; let check: MigrationCheck | undefined; if (blocking.length > 0) { check = { name, ok: false, message: - "v0 didn't record the secret ARN v1's CDK app needs to let a Gateway read " + - `${blocking.length === 1 ? "this credential" : "these credentials"}, and it couldn't ` + - `be found. ${blocking.join(". ")}. Then run \`agentcore migrate\` again.` + + blocking.join(" ") + (found.length > 0 ? ` The secret ARNs of ${found.join(", ")} were found.` : ""), }; } else if (found.length > 0) { diff --git a/src/core/project/migrate/index.apply.test.ts b/src/core/project/migrate/index.apply.test.ts index e31dc4c04..0fbee3ffd 100644 --- a/src/core/project/migrate/index.apply.test.ts +++ b/src/core/project/migrate/index.apply.test.ts @@ -12,7 +12,7 @@ import { STATE_RELATIVE_PATH, writeMigrationState, } from "./journal"; -import type { CredentialSecretLookups } from "./credentialSecrets"; +import { gatewayCredentialNames, type CredentialSecretLookups } from "./credentialSecrets"; import { CDK_LIBRARY_OVERRIDE_ENV, FsProjectMigrator } from "./index"; import { commitAll, @@ -145,6 +145,39 @@ const secretArns = (target: unknown) => const readJson = async (path: string) => JSON.parse(await readFile(path, "utf8")); +/** F1 with its Gateway target authenticating by OAuth, so the v1 app needs compatA-oauth's secret ARN. */ +const GATEWAY_SPEC = { + ...F1_SPEC, + agentCoreGateways: F1_SPEC.agentCoreGateways.map((gateway) => ({ + ...gateway, + targets: gateway.targets.map((target) => ({ + ...target, + outboundAuth: { type: "OAUTH", credentialName: "compatA-oauth", scopes: ["openid"] }, + })), + })), +}; + +/** + * Fails the synth as v1's CDK app does: it synthesizes every target's stack, and a + * Gateway credential recorded without a secret ARN throws. + */ +async function requireGatewaySecrets(workspace: string): Promise { + const spec = await readJson(join(workspace, "agentcore", "agentcore.json")); + const state = await readJson(join(workspace, STATE_PATH)); + for (const target of Object.values(state.targets) as { + resources?: { credentials?: Record }; + }[]) { + for (const name of gatewayCredentialNames(spec)) { + const credential = target.resources?.credentials?.[name]; + if (credential && credential.clientSecretArn === undefined) { + throw new Error( + `Credential "${name}" has no clientSecretArn in deployed state; the gateway role needs it to read the secret.`, + ); + } + } + } +} + describe("FsProjectMigrator: applying a migration", () => { test("converts the files in place and refactors the stack, keeping physical IDs", async () => { const root = await committedProject(); @@ -244,21 +277,7 @@ describe("FsProjectMigrator: applying a migration", () => { }); test("blocks with the credential named when a Gateway target's secret ARN can't be read", async () => { - const [gateway] = F1_SPEC.agentCoreGateways; - const root = await committedProject({ - spec: { - ...F1_SPEC, - agentCoreGateways: [ - { - ...gateway!, - targets: gateway!.targets.map((target) => ({ - ...target, - outboundAuth: { type: "OAUTH", credentialName: "compatA-oauth", scopes: ["openid"] }, - })), - }, - ], - }, - }); + const root = await committedProject({ spec: GATEWAY_SPEC }); const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": new Error(DENIED), @@ -277,47 +296,139 @@ describe("FsProjectMigrator: applying a migration", () => { expect(await readJson(join(root, STATE_PATH))).toEqual(F1_DEPLOYED_STATE); }); - test("records only the selected target's secret ARNs, and a pending target's only for its synth", async () => { - const root = await committedProject(twoTargets()); + test("records every same-account target's secret ARNs, so the v1 app synthesizes all of them", async () => { + // Both targets in one account, in different regions, with a Gateway authenticating by OAuth. + const root = await committedProject({ ...twoTargets(), spec: GATEWAY_SPEC }); const stacks = [ { name: STACK, target: "default" }, { name: PROD_STACK, target: "prod" }, ]; const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); - const first = migrator({ stacks, identity }); + const first = migrator({ stacks, identity, onSynth: requireGatewaySecrets }); const prepared = await first.subject.prepare(root, { target: "default" }); expect(prepared.plan.ready).toBe(true); + expect(prepared.plan.checks).toContainEqual( + expect.objectContaining({ + name: "credential secrets", + ok: true, + message: expect.stringContaining("compatA-oauth (target prod)"), + }), + ); await prepared.apply(); await prepared.dispose(); const written = await readJson(join(root, STATE_PATH)); expect(secretArns(written.targets.default)).toEqual([KEY_SECRET, OAUTH_SECRET]); - expect(secretArns(written.targets.prod)).toEqual([undefined, undefined]); + expect(secretArns(written.targets.prod)).toEqual([KEY_SECRET, OAUTH_SECRET]); - const synthesized: unknown[] = []; + // The pending run finds every ARN recorded, so it synthesizes without looking any up. + const calls = identity.calls.length; const rerun = migrator({ stacks, sharedCloudFormation: first.cloudFormation, identity, - onSynth: async (workspace) => { - synthesized.push((await readJson(join(workspace, STATE_PATH))).targets.prod); - }, + onSynth: requireGatewaySecrets, }); const finishing = await rerun.subject.prepare(root, { target: "prod" }); expect(finishing.plan.finishPending).toBe(true); expect(finishing.plan.ready).toBe(true); expect(finishing.plan.changes).toEqual([]); - expect(synthesized.map(secretArns)).toEqual([[KEY_SECRET, OAUTH_SECRET]]); + expect(identity.calls).toHaveLength(calls); await finishing.apply(); await finishing.dispose(); + }); + + test("the pending run records the secret ARNs for its synth, without writing the project", async () => { + const root = await committedProject({ ...twoTargets(), spec: GATEWAY_SPEC }); + const stacks = [ + { name: STACK, target: "default" }, + { name: PROD_STACK, target: "prod" }, + ]; + // The first run has no Identity client, as a CLI from before the fix would, so it records none. + const first = migrator({ stacks }); + const prepared = await first.subject.prepare(root, { target: "default" }); + await prepared.apply(); + await prepared.dispose(); - // The pending run's files were already written, so the next deploy records prod's. + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + const rerun = migrator({ + stacks, + sharedCloudFormation: first.cloudFormation, + identity, + onSynth: requireGatewaySecrets, + }); + const finishing = await rerun.subject.prepare(root, { target: "prod" }); + expect(finishing.plan.ready).toBe(true); + expect(finishing.plan.changes).toEqual([]); + await finishing.dispose(); expect(secretArns((await readJson(join(root, STATE_PATH))).targets.prod)).toEqual([ undefined, undefined, ]); }); + test("blocks when a target in another account has a Gateway credential without its secret ARN", async () => { + const files = twoTargets(); + const root = await committedProject({ + ...files, + spec: GATEWAY_SPEC, + targets: [ + { name: "default", account: "998846730471", region: "us-east-1" }, + { name: "prod", account: "111111111111", region: "us-west-2" }, + ], + }); + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + const { subject } = migrator({ identity, onSynth: requireGatewaySecrets }); + + const plan = await subject.plan(root, { target: "default" }); + + expect(plan.ready).toBe(false); + const check = plan.checks.find((entry) => entry.name === "credential secrets"); + expect(check?.ok).toBe(false); + expect(check?.message).toContain( + "Target prod is in account 111111111111 (us-west-2), but the active credentials are for " + + "account 998846730471. v1's CDK app synthesizes every target's stack, so it needs the " + + "secret ARNs of compatA-oauth there too", + ); + }); + + test("a target in another account with every secret ARN recorded doesn't block", async () => { + const files = twoTargets(); + const state = files.deployedState as typeof F1_DEPLOYED_STATE & { + targets: { prod: typeof F1_DEPLOYED_STATE.targets.default }; + }; + const credentials = state.targets.prod.resources.credentials; + const root = await committedProject({ + ...files, + spec: GATEWAY_SPEC, + targets: [ + { name: "default", account: "998846730471", region: "us-east-1" }, + { name: "prod", account: "111111111111", region: "us-west-2" }, + ], + deployedState: { + targets: { + default: state.targets.default, + prod: { + resources: { + ...state.targets.prod.resources, + credentials: { + "compatA-key": { ...credentials["compatA-key"], clientSecretArn: KEY_SECRET }, + "compatA-oauth": { ...credentials["compatA-oauth"], clientSecretArn: OAUTH_SECRET }, + }, + }, + }, + }, + }, + }); + const identity = fakeIdentity({ "compatA-key": KEY_SECRET, "compatA-oauth": OAUTH_SECRET }); + const { subject } = migrator({ identity, onSynth: requireGatewaySecrets }); + + const plan = await subject.plan(root, { target: "default" }); + + expect(plan.ready).toBe(true); + expect(plan.checks.every((check) => check.ok)).toBe(true); + }); + test("refuses to apply when the tree changed after planning", async () => { const root = await committedProject(); const { subject, cloudFormation } = migrator(); diff --git a/src/core/project/migrate/index.ts b/src/core/project/migrate/index.ts index f6ea528b6..bf3eca02a 100644 --- a/src/core/project/migrate/index.ts +++ b/src/core/project/migrate/index.ts @@ -48,6 +48,7 @@ import { backfillCredentialSecrets, gatewayCredentialNames, type CredentialSecretLookups, + type TargetAccess, } from "./credentialSecrets"; import { checkGitGate } from "./gitGate"; import { @@ -262,7 +263,7 @@ export class FsProjectMigrator implements ProjectMigrator { const secrets = problems.length > 0 ? { warnings: [] } - : await this.backfillSecrets(rootPath, v0Spec, selected, checks); + : await this.backfillSecrets(rootPath, v0Spec, targets.targets, selected, checks); const warnings = [ ...converted.warnings, @@ -403,13 +404,15 @@ export class FsProjectMigrator implements ProjectMigrator { } /** - * Fills in the secret ARNs v0 didn't record for the selected target's credentials, - * read with the credentials its stack was read with; returns deployed-state.json's - * new contents when any were. + * Fills in the secret ARNs v0 didn't record for every deployed target's + * credentials, since the v1 app synthesizes every target's stack; returns + * deployed-state.json's new contents when any were. Only once the selected + * stack was read, so the active credentials are known to work. */ private async backfillSecrets( rootPath: string, spec: { credentials: { name: string; authorizerType: string }[] }, + targets: AwsDeploymentTarget[], selected: DeployedTarget | undefined, checks: MigrationCheck[], ): Promise<{ deployedState?: string; warnings: string[] }> { @@ -419,13 +422,47 @@ export class FsProjectMigrator implements ProjectMigrator { rootPath, credentials: spec.credentials, gatewayCredentials: gatewayCredentialNames(spec), - target: selected.target.name, - options: { region: selected.target.region, credentials: selected.credentials }, + targets, + access: this.targetAccess(selected), }); if (check) checks.push(check); return { ...(deployedState !== undefined && { deployedState }), warnings }; } + /** + * Reads a target's providers with the credentials migrate resolves for its + * region, which reach any region of their own account. Resolved once per + * region, starting from the ones the selected stack was read with. + */ + private targetAccess( + selected: DeployedTarget, + ): (target: AwsDeploymentTarget) => Promise { + type Resolved = { credentials: CdkCredentialProvider; account: string }; + const byRegion = new Map>([ + [ + selected.target.region, + Promise.resolve({ credentials: selected.credentials!, account: selected.target.account }), + ], + ]); + return async (target) => { + let resolved = byRegion.get(target.region); + if (!resolved) { + resolved = (async () => { + const credentials = await this.resolveCredentials(target.region); + return { credentials, account: await this.resolveAccount(target.region, credentials) }; + })(); + byRegion.set(target.region, resolved); + } + try { + const { credentials, account } = await resolved; + if (account !== target.account) return { account }; + return { options: { region: target.region, credentials } }; + } catch (error) { + return { error: (error as Error).message }; + } + }; + } + /** Reads the selected target's stack, recording why it was skipped when it can't be. */ private async readSelectedStack( entry: DeployedTarget, @@ -609,7 +646,8 @@ export class FsProjectMigrator implements ProjectMigrator { const spec = JSON.parse(await readFile(join(rootPath, SPEC_RELATIVE_PATH), "utf8")) as { credentials?: unknown; }; - // Only the synth reads it here: a pending migration's files are already written. + // Only the synth reads it here: a pending migration's files are already written + // (and the run that wrote them recorded every reachable target's ARNs). const secrets = await this.backfillSecrets( rootPath, { @@ -618,6 +656,7 @@ export class FsProjectMigrator implements ProjectMigrator { ? (spec.credentials as { name: string; authorizerType: string }[]) : [], }, + (await this.readTargets(rootPath)).targets, selected, checks, );