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..608edd006 --- /dev/null +++ b/src/core/project/migrate/credentialSecrets.test.ts @@ -0,0 +1,310 @@ +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, + type TargetAccess, +} 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 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; +} + +const DEFAULT = { name: "default", account: "998846730471", region: "us-east-1" }; + +function backfill( + root: string, + identity: CredentialSecretLookups, + gatewayCredentials: string[] = [], + credentials: { name: string; authorizerType: string }[] = F1_SPEC.credentials, + targets: { name: string; account: string; region: string }[] = [DEFAULT], + access: (target: { region: string }) => Promise = async (target) => ({ + options: { region: target.region }, + }), +) { + return backfillCredentialSecrets({ + identity, + rootPath: root, + credentials, + gatewayCredentials: new Set(gatewayCredentials), + targets, + access, + }); +} + +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("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 = { + 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..cc1e5fd71 --- /dev/null +++ b/src/core/project/migrate/credentialSecrets.ts @@ -0,0 +1,203 @@ +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 } }>; +}; + +/** 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 } }[]; + }[]; +}; + +/** + * 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; +} + +/** + * 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. 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< + 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; + /** 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 kinds = new Map(input.credentials.map((c) => [c.name, c.authorizerType])); + 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 failed: string[] = []; + const otherAccounts: string[] = []; + const warnings: string[] = []; + 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}`, + ); + } + 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 ${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: + blocking.join(" ") + + (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..0fbee3ffd 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 { gatewayCredentialNames, 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,21 +86,98 @@ 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")); +/** 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(); @@ -133,6 +216,219 @@ 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 root = await committedProject({ spec: GATEWAY_SPEC }); + 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 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, 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([KEY_SECRET, OAUTH_SECRET]); + + // 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: 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(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(); + + 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.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..bf3eca02a 100644 --- a/src/core/project/migrate/index.ts +++ b/src/core/project/migrate/index.ts @@ -44,6 +44,12 @@ import { type MigrationState, } from "./journal"; import { convertHarness } from "./convertHarness"; +import { + backfillCredentialSecrets, + gatewayCredentialNames, + type CredentialSecretLookups, + type TargetAccess, +} from "./credentialSecrets"; import { checkGitGate } from "./gitGate"; import { AWS_TARGETS_RELATIVE_PATH, @@ -94,6 +100,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 +153,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 +169,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 +259,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, targets.targets, selected, checks); const warnings = [ ...converted.warnings, @@ -257,6 +272,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 +282,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 +403,66 @@ export class FsProjectMigrator implements ProjectMigrator { }; } + /** + * 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[] }> { + if (!this.identity || selected?.template === undefined) return { warnings: [] }; + const { deployedState, check, warnings } = await backfillCredentialSecrets({ + identity: this.identity, + rootPath, + credentials: spec.credentials, + gatewayCredentials: gatewayCredentialNames(spec), + 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, @@ -566,11 +643,30 @@ 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 + // (and the run that wrote them recorded every reachable target's ARNs). + const secrets = await this.backfillSecrets( + rootPath, + { + ...spec, + credentials: Array.isArray(spec.credentials) + ? (spec.credentials as { name: string; authorizerType: string }[]) + : [], + }, + (await this.readTargets(rootPath)).targets, + 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