Skip to content

fix(agent): stop hard-coding log-delivery logical ids #705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
isadeks wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(agent): stop hard-coding log-delivery logical ids #705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 6 additions & 127 deletions cdk/src/stacks/agent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<string, readonly PinnedLogResource[]> = {
'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);
}
}
76 changes: 37 additions & 39 deletions cdk/test/stacks/agent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
Loading