From 954fe90e54790d2d9d6b528a8749d63942551b90 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Tue, 4 Aug 2026 00:11:33 +0100 Subject: [PATCH] fix(agent): stop hard-coding log-delivery logical ids, closes #703 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stack overrode the logical ids of the six log-delivery resources from a table keyed by stack name, holding values read off one account's live stack. Stack name is not a proxy for deployed state: two accounts running a stack of the same name had diverged, so the table was correct for one and actively caused the rename on the other. That rename is fatal rather than cosmetic. A DeliverySource is unique per (resource ARN, log type) account-wide, and the runtime ARN does not change when the resource is renamed — so CloudFormation's create-before-delete produces a second source for the same runtime, CloudWatch Logs rejects it as already existing, and the whole stack update rolls back. Renaming can never avoid that collision, because the conflict is on the ARN the resources point at rather than on their own names. Since no set of literals can describe every account's deployed state, this holds none. The resources go back to being named by the AgentCore Runtime, which generates them from the construct path — deterministically, and identically in every account, with nothing to keep in sync and no table to re-record when it drifts. The wrong instinct here was to swap the recorded ids for self-chosen stable ones. I tried it: it renames on BOTH accounts, so it breaks the account that is already correct in order to fix the other. Verified by diff before discarding it. Also removes the earlier `-c pinnedLogDeliveryStack` gate along with the table. Gating the safe path behind a flag inverted the default: the failure that teaches an operator the flag exists is a mid-update rollback whose message never mentions it. Effect per account, measured with `cdk diff` against live stacks: - a stack already on the library's naming: zero churn, the delivery resources are not touched at all. This is the account the table was breaking. - a stack the table was holding on legacy ids: those six ids converge once. No operator migration is prescribed, because nothing is being migrated TO — the stack simply stops being held back from what the library already generates. Tests replace the two that asserted the table's existence: one that no captured id, account-unique Name, or override survives anywhere in the source, and one that both log types are still wired so "no pin" cannot silently mean "no delivery". Both were confirmed to fail against a reintroduced hard-coded override AND against the self-chosen-prefix version above, so they reject the wrong fix as well as the original bug. --- cdk/src/stacks/agent.ts | 133 ++-------------------------------- cdk/test/stacks/agent.test.ts | 76 ++++++++++--------- 2 files changed, 43 insertions(+), 166 deletions(-) diff --git a/cdk/src/stacks/agent.ts b/cdk/src/stacks/agent.ts index ea37c3d4..f873a4c3 100644 --- a/cdk/src/stacks/agent.ts +++ b/cdk/src/stacks/agent.ts @@ -403,30 +403,12 @@ export class AgentStack extends Stack { runtimeArnHolder = runtime.agentRuntimeArn; - // --- AgentCore log-delivery: keep the logical ids STABLE across library - // renames, so updating an existing stack never has to be opted into --- - // - // The AgentCore Runtime auto-creates AWS::Logs::DeliverySource + Delivery + - // DeliveryDestination per loggingConfig, naming them from the construct path - // the library happens to use. When that path changes — as it did between - // library versions here — the CFN logical ids change with it, and CFN treats - // renamed resources as new ones: it CREATES before it DELETES. - // - // A DeliverySource is unique per (resource ARN, log type) for the whole - // account, and the runtime ARN does not change across the rename. So the new - // source collides with the live one that is still there, CloudWatch Logs - // rejects it with ``AlreadyExists``, and the whole stack rolls back. Note - // what this means: renaming the resources cannot avoid the collision, because - // the conflict is on the ARN they point at, not on their own names. Only - // keeping the logical id stable avoids it, since that is what makes CFN - // update in place rather than create a second source for the same runtime. - // - // Hence: pinned ALWAYS, for every stack, with no context flag. A flag would - // mean the safe path is the one you have to know to ask for, and the failure - // it prevents is a mid-update rollback that says nothing about the flag's - // existence. A fresh stack is unaffected either way — it has no live sources - // to collide with, and these ids are as valid for it as the library's own. - pinLogDeliveryLogicalIds(runtime); + // Log delivery is left entirely to the AgentCore Runtime, which creates and + // names the DeliverySource / Delivery / DeliveryDestination trio itself. We + // deliberately do not override those logical ids: see the note in the design + // docs, but in short, pinning them to values recorded from one account's live + // stack made the template describe that account rather than the code, and broke + // every other account on update. // --- Session storage (preview) --- // The L2 construct does not yet expose filesystemConfigurations; use the @@ -1441,106 +1423,3 @@ export class AgentStack extends Stack { } } -/** - * A churned log-delivery resource to re-pin: the construct child id under the - * Runtime, the logical id CFN already has deployed, and (for the account-unique - * Source/Destination kinds) the deployed ``Name``. ``liveName`` is omitted for - * Delivery links, which have no Name. - */ -interface PinnedLogResource { - readonly childId: string; - readonly liveLogicalId: string; - readonly liveName?: string; -} - -/** - * Log-delivery logical ids to keep stable, keyed by stack name. Consulted on - * every synth — see {@link pinLogDeliveryLogicalIds} for why there is no flag. - * - * Each entry records what CloudFormation already has for a stack deployed before - * the library renamed these resources. Read from `aws cloudformation - * list-stack-resources` against the live stack, so the ids are observed, not - * constructed — the hash in each one is not reproducible from the construct path - * alone, which is precisely why they have to be written down. - * - * An entry stays until its stack is gone. Removing one while the stack still - * exists re-introduces the rename and the failed update that comes with it. - */ -const PINNED_LOG_DELIVERY_BY_STACK: Record = { - 'backgroundagent-dev': [ - { - childId: 'ApplicationLogsDeliverySource', - liveLogicalId: 'RuntimeCDKSourceAPPLICATIONLOGSbackgroundagentdevRuntimeBC0AE9ED96A02E02', - liveName: 'cdk-applicationlogs-source-backgroundagentdevRuntimeBC0AE9ED', - }, - { - childId: 'UsageLogsDeliverySource', - liveLogicalId: 'RuntimeCDKSourceUSAGELOGSbackgroundagentdevRuntimeBC0AE9ED544FBB22', - liveName: 'cdk-usagelogs-source-backgroundagentdevRuntimeBC0AE9ED', - }, - { - childId: 'ApplicationLogsDest', - liveLogicalId: 'RuntimeCdkLogGroupApplicationLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeApplicationLogGroup454A95E8DestapplicationlogsE09F77DC', - liveName: 'cdk-cwl-Destapplication-logs-dest-backgrounp454A95E829BF8A27', - }, - { - childId: 'UsageLogsDest', - liveLogicalId: 'RuntimeCdkLogGroupUsageLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeUsageLogGroup7FA1FA67Destusagelogs9AB608D0', - liveName: 'cdk-cwl-Destusage-logs-dest-backgroundagroup7FA1FA67A8A16CEE', - }, - // Delivery links: logical-id pin only (no Name — unique per source/dest pair). - { - childId: 'ApplicationLogsDelivery', - liveLogicalId: 'RuntimeCdkLogGroupApplicationLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeApplicationLogGroup454A95E8Delivery92FE492C', - }, - { - childId: 'UsageLogsDelivery', - liveLogicalId: 'RuntimeCdkLogGroupUsageLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeUsageLogGroup7FA1FA67Delivery40F023D7', - }, - ], -}; - -/** - * Pin the auto-created log-delivery resources to stable logical ids, ALWAYS. - * - * These resources are created for us by the AgentCore Runtime and named after - * whatever construct path the library uses internally, so a library-side rename - * silently renames them — and a renamed resource is, to CloudFormation, a new - * one to create before the old is deleted. That is fatal here: a DeliverySource - * is unique per (resource ARN, log type) account-wide, the runtime ARN is - * unchanged by a rename, so the create collides with the live source and the - * update rolls the whole stack back. Owning the ids ourselves decouples us from - * the library's internal naming. - * - * Applied unconditionally rather than behind a flag. Three cases, all safe: - * - * - An existing stack in the account that owns these resources: the ids match - * what CloudFormation already recorded, so it updates them in place. This is - * the case that was broken. - * - A fresh stack or account: nothing owns these names yet, so they create - * normally. The ids are ours rather than the library's, which is the point; - * the values themselves carry no meaning beyond being stable. - * - Any other name: the ids embed the stack name, so each stack gets its own. - * - * The values were read off a stack deployed before the rename. Do not "tidy" - * them — they are a record of what CloudFormation already has, and editing one - * re-breaks exactly the update path this exists to protect. - */ -function pinLogDeliveryLogicalIds(runtime: agentcore.Runtime): void { - const stack = Stack.of(runtime); - const pins = PINNED_LOG_DELIVERY_BY_STACK[stack.stackName]; - // Only the stack these ids were recorded from can use them: they embed that - // stack's name. Any other stack keeps the library's own naming, which is - // correct for it — it has no pre-rename resources to line up with. - if (!pins) return; - - for (const pin of pins) { - const res = runtime.node.tryFindChild(pin.childId) as CfnResource | undefined; - // A future library rename moves the child, so the pin stops matching. Skip - // rather than throw: the stack still deploys, and the next update that hits - // the collision is the signal to re-record the ids from the live stack. - if (!res) continue; - res.overrideLogicalId(pin.liveLogicalId); - if (pin.liveName !== undefined) res.addPropertyOverride('Name', pin.liveName); - } -} diff --git a/cdk/test/stacks/agent.test.ts b/cdk/test/stacks/agent.test.ts index 2fa8d10b..d28e7611 100644 --- a/cdk/test/stacks/agent.test.ts +++ b/cdk/test/stacks/agent.test.ts @@ -529,59 +529,57 @@ describe('AgentStack', () => { expect(objectStatements).toEqual([]); }); - test('log-delivery logical ids are pinned with NO opt-in, so an existing stack updates in place', () => { - // A DeliverySource is unique per (resource ARN, log type) for the whole - // account, and the runtime ARN survives a library-side rename of these - // auto-created resources. So a renamed source is a SECOND source for the same - // runtime: CloudFormation creates before deleting, CloudWatch Logs rejects it - // as already existing, and the update rolls the whole stack back. + test('no hard-coded log-delivery logical ids or names anywhere in the stack', () => { + // The template must not carry values recorded from one account's live stack. // - // The ids must therefore be pinned unconditionally. Behind a flag, the safe - // path is the one an operator has to already know about, and the failure that - // teaches them is a mid-update rollback whose message never mentions it. + // These resources are created and named by the AgentCore Runtime. An earlier + // version overrode their logical ids from a table keyed by STACK NAME, holding + // ids read off one deployed stack — but stack name is not a proxy for deployed + // state. Two accounts running a stack of the same name had diverged, so the + // table was right for one and actively caused the rename on the other, which is + // fatal: a DeliverySource is unique per (resource ARN, log type) account-wide, + // the runtime ARN survives a rename, so CloudFormation's create-before-delete + // collides with the live source and the whole update rolls back. // - // Asserted on the source, not by synthesizing a second stack: constructing - // one under a different construct id trips an unrelated cdk-nag - // suppression-path check first, which masks whatever this is checking. + // There is no set of literals that serves every account, so the fix is to hold + // none. Asserted on the source because the failure mode is a value being + // reintroduced, not a synth-visible shape. const src = fs.readFileSync( path.resolve(__dirname, '../../src/stacks/agent.ts'), 'utf8', ); - const fn = src.slice(src.indexOf('function pinLogDeliveryLogicalIds')); - const body = fn.slice(0, fn.indexOf('\n}')); - // Keyed off the stack's OWN name — no context, no opt-in. - expect(body).toContain('PINNED_LOG_DELIVERY_BY_STACK[stack.stackName]'); - expect(body).not.toContain('tryGetContext'); - // Nothing anywhere may reintroduce a gate. + // No table, no lookup, no per-stack keying, and no flag to gate any of it. + expect(src).not.toContain('PINNED_LOG_DELIVERY_BY_STACK'); expect(src).not.toContain('pinnedLogDeliveryStack'); + expect(src).not.toContain('overrideLogicalId'); - // The ids it pins are the ones CloudFormation already holds for that stack. - // Hard-coded here on purpose: if someone "tidies" a value in the table, this - // fails instead of the next production update rolling back. - expect(src).toContain('RuntimeCDKSourceAPPLICATIONLOGSbackgroundagentdevRuntimeBC0AE9ED96A02E02'); - expect(src).toContain('RuntimeCDKSourceUSAGELOGSbackgroundagentdevRuntimeBC0AE9ED544FBB22'); + // No captured logical id or account-unique Name, in any of their shapes. + expect(src).not.toMatch(/RuntimeCDKSource/); + expect(src).not.toMatch(/cdk-(application|usage)logs-source-/); + expect(src).not.toMatch(/cdk-cwl-Dest/); }); - test('a stack with no recorded ids keeps the library\'s own log-delivery naming', () => { - // The pinned ids embed a stack name, so they are only correct for that stack. - // Another stack has no pre-rename resources to line up with and must not - // inherit them — otherwise two stacks in one account would claim the same - // account-unique DeliverySource Name. The table lookup is what enforces this, - // so assert it returns nothing for an unknown name rather than falling back. - const src = fs.readFileSync( - path.resolve(__dirname, '../../src/stacks/agent.ts'), 'utf8', - ); - const fn = src.slice(src.indexOf('function pinLogDeliveryLogicalIds')); - const body = fn.slice(0, fn.indexOf('\n}')); - expect(body).toMatch(/if \(!pins\) return;/); - - // And this stack — named TestAgentStack, absent from the table — got the - // library's naming, with none of backgroundagent-dev's ids leaking in. - const ids = Object.keys(template.findResources('AWS::Logs::DeliverySource')); + test('log delivery is left to the library, so every account synthesizes the same ids', () => { + // The point of holding no ids: what lands in the template comes from the + // library, so the same code produces the same ids in every account. A stack + // already on the library's naming sees no change at all. + const sources = template.findResources('AWS::Logs::DeliverySource'); + const ids = Object.keys(sources); expect(ids).toHaveLength(2); + for (const id of ids) { + // Library-generated, not ours: no self-chosen prefix and no stack name baked + // in. A stack name in a logical id is the signature of the old table. expect(id).not.toContain('backgroundagentdev'); + expect(id).not.toContain('AgentRuntimeApplication'); + expect(id).toMatch(/^Runtime(Application|Usage)LogsDeliverySource[0-9A-F]{8}$/); } + + // Both log types are wired, so "no pin" cannot mean "no delivery". + const logTypes = Object.values(sources) + .map((r) => (r as { Properties?: { LogType?: string } }).Properties?.LogType) + .sort(); + expect(logTypes).toEqual(['APPLICATION_LOGS', 'USAGE_LOGS']); }); test('the fan-out consumer can reach BOTH surfaces\' credentials registries', () => {