diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 19358a54..ffa51b45 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -189,6 +189,49 @@ jobs: echo "No security-relevant changes detected." >> "$GITHUB_STEP_SUMMARY" fi + # Log-delivery migration state, surfaced BEFORE the approval gate. + # + # `mise //cdk:deploy` runs this preflight automatically, but this pipeline calls + # `npx cdk deploy` directly (see the Deploy job) and so inherits none of that task's + # steps. Without this, a pipeline stack still on the pre-#339 library's delivery + # logical ids would roll back mid-update with `AlreadyExists` and nothing anywhere + # would have said why — and the approver would have had no way to know. + # + # --check-only NEVER deletes; it reports and exits 2 when a migration is needed. + # continue-on-error because this is information for the approver, not a gate: the + # job holds the read-only diff role, and a missing `cloudformation:ListStackResources` + # on it must not block the diff that the approval decision depends on. + - name: Log-delivery migration check (read-only) + continue-on-error: true + env: + COMPUTE_TYPE: ${{ matrix.compute_type }} + run: | + echo "## Log-delivery migration (\`$COMPUTE_TYPE\`)" >> "$GITHUB_STEP_SUMMARY" + echo "" >> "$GITHUB_STEP_SUMMARY" + set +e + node --experimental-strip-types cdk/scripts/preflight-log-delivery.ts \ + --check-only 2>&1 | tee preflight-log-delivery.txt + status=${PIPESTATUS[0]} + set -e + echo '```' >> "$GITHUB_STEP_SUMMARY" + cat preflight-log-delivery.txt >> "$GITHUB_STEP_SUMMARY" + echo '```' >> "$GITHUB_STEP_SUMMARY" + if [ "$status" = "2" ]; then + { + echo "" + echo "> [!WARNING]" + echo "> This stack needs the one-time log-delivery migration. Deploying without" + echo "> it fails mid-update with \`AlreadyExists\` and rolls back. This pipeline" + echo "> does **not** migrate automatically — run the documented step first:" + echo "> \`STACK_NAME= mise //cdk:deploy\`, or the manual sequence in" + echo "> docs/design/OBSERVABILITY.md (\"AgentCore log delivery\")." + } >> "$GITHUB_STEP_SUMMARY" + elif [ "$status" != "0" ]; then + echo "" >> "$GITHUB_STEP_SUMMARY" + echo "_Could not determine migration state (exit $status) — see the step log._" \ + >> "$GITHUB_STEP_SUMMARY" + fi + - name: Upload diff artifacts uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: @@ -196,6 +239,7 @@ jobs: path: | cdk-diff-full.txt cdk-diff-security.txt + preflight-log-delivery.txt deploy: needs: [resolve-targets, diff] diff --git a/cdk/mise.toml b/cdk/mise.toml index 22ceddee..e87bfb7a 100644 --- a/cdk/mise.toml +++ b/cdk/mise.toml @@ -73,12 +73,55 @@ else fi ''' +# One-time migration guard for stacks that predate the log-delivery pin-table +# removal (#703): without it, their first deploy after the change fails +# mid-update (DeliverySource AlreadyExists) and rolls back with no hint of the +# cause. Detects the legacy ids on the live stack and deletes exactly those +# resources so the deploy can recreate them; no-ops for fresh installs and +# already-migrated stacks. +# +# The affected set is a property of DEPLOYED STATE, not of the stack's name: the legacy +# ids are the pre-#339 library's naming, so any stack last deployed before 2026-06-13 has +# them whatever it is called. A custom stack name is not immunity. Select the stack with +# STACK_NAME, --stack-name, or -c stackName=; the deploy task forwards its own arguments +# here so the two cannot disagree. +# +# Escape hatches: --check-only reports without deleting; +# ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1 skips. See docs/design/OBSERVABILITY.md. +[tasks."preflight:log-delivery"] +description = "Migrate pre-#703 pinned log-delivery resources before deploy (no-op otherwise)" +# `node --experimental-strip-types`, matching the `check:*-sync` script tasks and this +# script's own tests. `npx tsx` (as `bootstrap:generate` uses) also works, but tsx is +# in neither package.json, so npx FETCHES it on first use — observed: "The following +# package was not found and will be installed: tsx@4.23.13". Acceptable for an +# occasional codegen task; not for a task on the deploy path, which would take an +# unpinned download on every deploy and fail outright with no network. Type stripping +# needs no dependency at all. +run = "node --experimental-strip-types scripts/preflight-log-delivery.ts" + [tasks.deploy] description = "cdk deploy (pass args after --)" # Reclaim disk first — the agent-image Docker build + CDK asset bundling need # several GB of working space, and uv/Docker caches accumulate across runs. depends = [":clean:disk"] -run = "npx cdk deploy" +# The log-delivery preflight runs as the FIRST command of this task rather than as a +# `depends`, so that the arguments after `--` reach BOTH it and `cdk deploy`. As a +# `depends` it could not see them: mise forwards `--` args only to the invoked task's own +# `run`, so `mise //cdk:deploy -- -c stackName=x` deployed stack `x` while the preflight +# resolved (and would have deleted from) the default stack. Verified, not assumed. This +# is a step that DELETES resources, so the two must not be able to disagree about which +# stack, account, or region they are operating on. +# +# A non-zero exit from the first command aborts the task, which is the intended +# fail-closed behaviour: the deploy must not proceed into a known-bad migration state. +# +# `required=false` is load-bearing — without it a bare `mise //cdk:deploy` (the common +# case, and what the guides tell operators to run) fails with a usage error instead of +# deploying. +run = [ + "node --experimental-strip-types scripts/preflight-log-delivery.ts {{arg(name='cdkargs', var=true, required=false)}}", + "npx cdk deploy {{arg(name='cdkargs', var=true, required=false)}}", +] # Bootstraps with ComputeTypes=agentcore (the template default). To ALSO enable the # ECS compute backend you must set the ComputeTypes CFN *parameter* — `cdk bootstrap` diff --git a/cdk/scripts/README.md b/cdk/scripts/README.md index 548533c3..29591c52 100644 --- a/cdk/scripts/README.md +++ b/cdk/scripts/README.md @@ -7,5 +7,6 @@ Bundling for Lambda assets is handled at synth time; the **`bundle`** task in ** | `generate-bootstrap-artifacts.ts` | Regenerates `cdk/bootstrap/policies/*.json`, `BOOTSTRAP_VERSION`, `BOOTSTRAP_HASH` from the typed policies in `src/bootstrap/policies/` | `mise //cdk:bootstrap:generate` | | `generate-bootstrap-template.ts` | Regenerates `cdk/bootstrap/bootstrap-template.yaml` (least-privilege CDK bootstrap, `ComputeTypes`-gated compute policies) | `mise //cdk:bootstrap:generate` | | `package-microvm-artifact.sh` | Packages `agent/` + `contracts/` + `Dockerfile` into the zip artifact an `AWS::Lambda::MicrovmImage` builds from, and uploads it to the CDK-created artifact bucket (ADR-021) | run directly — see the script header for the full bootstrap sequence | +| `preflight-log-delivery.ts` | One-time migration for stacks whose live `AWS::Logs::Delivery*` logical ids predate the CDK library switch in #339 (a `CDKSource`/`CdkLogGroup` segment): deletes exactly those resources so the deploy can recreate them under library naming, instead of rolling back mid-update with `AlreadyExists`. No-op for fresh installs and already-migrated stacks. | runs as the first step of `mise //cdk:deploy`; standalone as `mise //cdk:preflight:log-delivery` (add `-- --check-only` to report without deleting) | `package-microvm-artifact.sh` exists because CloudFormation cannot produce its own MicroVM `codeArtifact`: the image resource consumes a zip that must already be in S3, and there is no CDK asset type for "zip + Dockerfile a MicroVM image builds from". Everything else on that backend (buckets, roles, network connector, log group, the image resource itself) is CDK-managed in `src/constructs/lambda-microvm-compute.ts`. diff --git a/cdk/scripts/preflight-log-delivery.ts b/cdk/scripts/preflight-log-delivery.ts new file mode 100644 index 00000000..ec311a94 --- /dev/null +++ b/cdk/scripts/preflight-log-delivery.ts @@ -0,0 +1,437 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * Deploy preflight: one-time log-delivery migration for stacks that predate + * the removal of the log-delivery pin table (#703). + * + * WHY THIS RUNS ON EVERY DEPLOY: a stack whose live `AWS::Logs::Delivery*` + * resources still carry the pinned logical ids (`RuntimeCDKSource…` / + * `RuntimeCdkLogGroup…`) hits a guaranteed mid-deploy rollback on the first + * deploy after the pins were removed — CloudFormation renames the resources, + * creates-before-deletes, and the new DeliverySource collides with the live + * one (account-unique per runtime ARN + log type → `AlreadyExists`). That + * rollback message says nothing about the migration, and there is no channel + * to warn every existing deployment. So the deploy path detects the state and + * converges it, instead of letting CloudFormation discover it half an hour in. + * Design rationale: docs/design/OBSERVABILITY.md, "AgentCore log delivery". + * + * WHAT IT DOES: reads the stack's own resource list, and if (and only if) + * legacy-pinned delivery resources are present, deletes exactly those — by the + * physical ids CloudFormation reports, never by listing the account — then + * lets the deploy proceed. The deploy recreates the trio under the library's + * naming; CloudFormation treats the delete of the already-gone old resources + * as a no-op. Agent log delivery is down from the deletion until the deploy + * finishes; no delivered log data is touched. Stacks already on the library's + * ids, and fresh installs (no stack), pass straight through. + * + * Scoped-by-construction: the only delete targets are physical ids read off + * `list-stack-resources` for THIS stack, so delivery configurations belonging + * to anything else in the account are unreachable, even on a shared account. + * + * Uses the AWS CLI rather than SDK clients so the cdk package needs no new + * dependency for a one-time migration path. Note this DOES add the AWS CLI to + * the deploy path's prerequisites — `QUICK_START.mdx` otherwise offers an + * ABCA-CLI route for the post-deploy steps — so a missing `aws` is reported as + * "state could not be determined" rather than as a stack problem (see the + * abort handler at the bottom of this file). + * + * Exit codes: 0 = nothing to do or migration applied; 2 = legacy resources + * found in --check-only mode; 1 = could not determine state or a delete + * failed (deploy must not proceed into a known rollback). + * + * Escape hatch: ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1 skips entirely. + * Cautious mode: --check-only (or ABCA_LOG_DELIVERY_PREFLIGHT=check) reports + * and aborts instead of deleting. + */ + +import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import { pathToFileURL } from 'node:url'; + +const DELIVERY_TYPES = [ + 'AWS::Logs::Delivery', + 'AWS::Logs::DeliverySource', + 'AWS::Logs::DeliveryDestination', +] as const; + +type DeliveryType = (typeof DELIVERY_TYPES)[number]; + +/** + * The two logical-id shapes the retired pin table produced + * (`PINNED_LOG_DELIVERY_BY_STACK`): `RuntimeCDKSource…` for sources and + * `RuntimeCdkLogGroup…` for destinations and delivery links. The library's + * own ids (`RuntimeApplicationLogsDeliverySource` …) match neither. + */ +const LEGACY_ID = /CDKSource|CdkLogGroup/; + +interface StackResource { + LogicalResourceId: string; + PhysicalResourceId?: string; + ResourceType: string; + ResourceStatus?: string; +} + +/** + * Global `aws` options forwarded from the CDK argument list, so this script reads and + * deletes in the SAME account and region as the deploy it gates. + * + * Without this, `mise //cdk:deploy -- --profile prod` deployed to one account while the + * preflight inspected — and then deleted from — whatever the ambient profile pointed at. + * A delete in the wrong account is the worst outcome this script has, so the two + * resolutions must not be able to disagree. + */ +let awsGlobalArgs: readonly string[] = []; + +function aws(args: string[]): string { + return execFileSync('aws', [...awsGlobalArgs, ...args], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); +} + +/** + * The most informative text available for a failed `execFileSync`. + * + * `stderr ?? err` was wrong: an empty-string stderr is not nullish, so a CLI killed by a + * signal (or one that writes nothing) produced a message ending in a bare colon with no + * cause at all. `||` falls through on empty, and the status/signal is appended because + * that is the only signal left in exactly that case. + */ +function errText(err: unknown): string { + const e = err as { stderr?: unknown; status?: unknown; signal?: unknown }; + const base = String(e.stderr || err).trim(); + const how = e.signal ? `signal ${String(e.signal)}` : e.status !== undefined && e.status !== null + ? `exit ${String(e.status)}` : ''; + return base && how ? `${base} (${how})` : base || how || 'no output'; +} + +/** `--profile` / `--region` (space and `=` forms) as they appear in the CDK arg list. */ +function resolveAwsGlobalArgs(argv: readonly string[]): string[] { + const out: string[] = []; + for (let i = 0; i < argv.length; i += 1) { + const a = argv[i]!; + for (const opt of ['--profile', '--region'] as const) { + if (a === opt && argv[i + 1]) out.push(opt, argv[i + 1]!); + else if (a.startsWith(`${opt}=`)) out.push(opt, a.slice(opt.length + 1)); + } + } + return out; +} + +/** + * The account (and region) the deletes would land in, for the resolution log line. + * + * Best-effort by design: this is reporting, not a precondition, so a failure here must + * not fail a deploy. Returns `null` when it cannot be determined and the caller prints + * "target unknown" rather than a guess. + */ +function describeAwsTarget(): string | null { + try { + const account = aws([ + 'sts', 'get-caller-identity', '--query', 'Account', '--output', 'text', + ]).trim(); + let region = ''; + try { + region = aws(['configure', 'get', 'region']).trim(); + } catch { + region = process.env.AWS_REGION ?? process.env.AWS_DEFAULT_REGION ?? ''; + } + return `account ${account}${region ? `, ${region}` : ''}`; + } catch { + // nosemgrep: ts-silent-success-masking -- reporting only; an undeterminable target must not fail a deploy, and the caller prints "target unknown" instead of a guess + return null; + } +} + +/** Default stack name, mirroring `buildApp` in src/main.ts. */ +const DEFAULT_STACK_NAME = 'backgroundagent-dev'; + +/** + * Resolve which stack to inspect, from the same places the CDK app looks plus this + * script's own flag. + * + * The app takes its stack from `stackName` CDK **context** (`src/main.ts`), so a + * resolution that only read `--stack-name`/`STACK_NAME` could disagree with the deploy + * it gates — and this script deletes resources, so disagreeing is not merely untidy. + * `cdk.json` context is therefore read here too, exactly as `cdk deploy` would. + * + * One gap remains and cannot be closed from inside this script: context passed on the + * CDK command line (`mise //cdk:deploy -- -c stackName=x`) is appended to `cdk deploy` + * and never reaches a mise `depends` task, so this script cannot observe it. Verified, + * not assumed. For a non-default stack the operator must therefore set BOTH — the env + * var for this preflight and the context for the deploy: + * + * STACK_NAME=x mise //cdk:deploy -- -c stackName=x + * + * The resolved target is printed on every run so a mismatch is visible in the deploy + * log rather than inferred from what got deleted. `-c stackName=`/`--context stackName=` + * are also accepted here, for a direct `mise //cdk:preflight:log-delivery` invocation. + */ +function resolveStackName(argv: readonly string[]): { stackName: string; source: string } { + const flag = argv.indexOf('--stack-name'); + if (flag >= 0 && argv[flag + 1]) return { stackName: argv[flag + 1]!, source: '--stack-name' }; + + // CDK's own context forms, when handed straight to this script. + for (let i = 0; i < argv.length; i += 1) { + const a = argv[i]!; + if ((a === '-c' || a === '--context') && argv[i + 1]?.startsWith('stackName=')) { + return { stackName: argv[i + 1]!.slice('stackName='.length), source: `${a} stackName=` }; + } + if (a.startsWith('--context=stackName=')) { + return { stackName: a.slice('--context=stackName='.length), source: '--context=stackName=' }; + } + } + + if (process.env.STACK_NAME) return { stackName: process.env.STACK_NAME, source: 'STACK_NAME' }; + + // Persisted context — what `cdk deploy` would read when no flag is given. + try { + // BOTH context files. `cdk.context.json` matters more than it looks: this repo's + // `cdk.json` carries no `context` block at all, so reading only that file made this + // branch dead code — while `.github/workflows/build.yml` writes `stackName` into + // `cdk.context.json` for every pipeline stack (`pr-`). The CI path + // therefore resolved to the default and would have inspected, and deleted from, a + // different stack than the one being deployed. + // Bare filenames, so the display name IS the path — no `file.replace('../', '')`, + // which CodeQL flags as js/incomplete-sanitization (alert #43, HIGH) because a + // single-occurrence replace on a path is the shape of broken traversal sanitization. + const CONTEXT_FILES = ['cdk.context.json', 'cdk.json'] as const; + // Directory the context files are read from — the `cdk/` package root, i.e. one level + // up from `scripts/`. Overridable ONLY for tests: the real `cdk/cdk.context.json` is + // CDK's own lookup cache, shared with every suite jest runs in parallel, so a test + // that wrote or deleted it raced the stack-synth tests. Pointing the tests at a + // scratch directory keeps the resolution chain testable without touching the repo. + const contextDir = process.env.ABCA_PREFLIGHT_CONTEXT_DIR + ? pathToFileURL(`${process.env.ABCA_PREFLIGHT_CONTEXT_DIR}/`) + : new URL('../', import.meta.url); + for (const name of CONTEXT_FILES) { + let cfg: { context?: Record; stackName?: unknown }; + try { + cfg = JSON.parse(readFileSync(new URL(name, contextDir), 'utf8')); + } catch { + // nosemgrep: ts-silent-success-masking -- an absent context file is the normal case (cdk.context.json is generated, and cdk.json has no context block here), not a failure; the next source is tried and 'default' is the documented floor + continue; + } + // `cdk.context.json` is a flat map; `cdk.json` nests under `context`. + const fromContext = cfg.context?.stackName ?? cfg.stackName; + if (typeof fromContext === 'string' && fromContext) { + return { stackName: fromContext, source: name }; + } + } + } catch { + // No readable context file — fall through to the default, which is what the app + // itself does. Not an error worth failing a deploy over. + } + + return { stackName: DEFAULT_STACK_NAME, source: 'default' }; +} + +/** `null` means the stack does not exist (fresh install — nothing to migrate). */ +function listStackResources(stackName: string): StackResource[] | null { + try { + const out = aws([ + 'cloudformation', + 'list-stack-resources', + '--stack-name', + stackName, + '--output', + 'json', + ]); + return (JSON.parse(out) as { StackResourceSummaries: StackResource[] }) + .StackResourceSummaries; + } catch (err) { + const stderr = errText(err); + if (stderr.includes('does not exist')) return null; // nosemgrep: ts-silent-success-masking -- null is not an empty success: it is the documented fresh-install signal, and main() branches on it explicitly + throw new Error(`could not list resources of stack '${stackName}': ${stderr.trim()}`); + } +} + +function deleteResource(type: DeliveryType, physicalId: string): void { + const args: Record = { + 'AWS::Logs::Delivery': ['logs', 'delete-delivery', '--id', physicalId], + 'AWS::Logs::DeliverySource': ['logs', 'delete-delivery-source', '--name', physicalId], + 'AWS::Logs::DeliveryDestination': [ + 'logs', + 'delete-delivery-destination', + '--name', + physicalId, + ], + }; + try { + aws(args[type]); + } catch (err) { + const stderr = errText(err); + // Already gone (e.g. a re-run after an interrupted migration): the goal + // state — old resource absent — is reached, so this is success, not + // failure. Anything else aborts the deploy; see main(). + if (stderr.includes('ResourceNotFoundException')) { + console.log(` already gone: ${type} ${physicalId}`); + return; // nosemgrep: ts-silent-success-masking -- delete target already absent IS the goal state; treating it as success is what makes an interrupted migration safely re-runnable + } + throw new Error(`failed to delete ${type} '${physicalId}': ${stderr.trim()}`); + } + console.log(` deleted: ${type} ${physicalId}`); +} + +function main(): number { + if (process.env.ABCA_SKIP_LOG_DELIVERY_PREFLIGHT === '1') { + console.log('log-delivery preflight: skipped (ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1)'); + return 0; + } + + const argv = process.argv.slice(2); + awsGlobalArgs = resolveAwsGlobalArgs(argv); + const { stackName, source } = resolveStackName(argv); + const checkOnly = + argv.includes('--check-only') || process.env.ABCA_LOG_DELIVERY_PREFLIGHT === 'check'; + + // Printed unconditionally, and it names the ACCOUNT as well as the stack: this script + // deletes, so the log has to be enough to tell after the fact exactly what was in + // scope. A stack name alone is ambiguous across accounts. + const target = describeAwsTarget(); + console.log( + `log-delivery preflight: inspecting stack '${stackName}' (from ${source}) in ` + + `${target ?? 'an undetermined account/region (sts get-caller-identity failed)'}`, + ); + + const resources = listStackResources(stackName); + if (resources === null) { + console.log(`log-delivery preflight: stack '${stackName}' not deployed yet — nothing to do`); + return 0; + } + + const legacy = resources.filter( + (r) => + (DELIVERY_TYPES as readonly string[]).includes(r.ResourceType) && + LEGACY_ID.test(r.LogicalResourceId) && + r.ResourceStatus !== 'DELETE_COMPLETE', + ); + if (legacy.length === 0) { + console.log( + `log-delivery preflight: stack '${stackName}' already on library-managed ids — nothing to do`, + ); + return 0; + } + + console.log( + `log-delivery preflight: stack '${stackName}' still has ${legacy.length} log-delivery ` + + 'resource(s) under the retired pinned naming (#703). The next deploy renames them, ' + + 'which CloudFormation cannot do in place (DeliverySource is account-unique per runtime ' + + 'ARN + log type; create-before-delete collides and rolls the whole update back).', + ); + console.log( + 'One-time migration: delete these exact resources now and let the deploy recreate them. ' + + 'Agent log delivery pauses until the deploy completes; delivered log data is not touched.', + ); + for (const r of legacy) { + console.log(` ${r.ResourceType} ${r.LogicalResourceId} -> ${r.PhysicalResourceId ?? '?'}`); + } + + if (checkOnly) { + console.log( + 'Check-only mode: not deleting. Re-run without --check-only (or unset ' + + 'ABCA_LOG_DELIVERY_PREFLIGHT) to migrate, or follow docs/design/OBSERVABILITY.md.', + ); + return 2; + } + + // FAIL CLOSED on an ambiguous target. Deletions are pending, arguments were supplied, + // and yet the stack name fell all the way through to the built-in default — so the + // operator is customizing this deploy in some way this script did not recognize, and + // the stack it is about to delete from is a guess rather than a derivation. The + // dangerous case is real: an account holding a legacy `backgroundagent-dev` plus a + // second stack, deployed with an argument that names the second, would have had the + // FIRST one's log delivery deleted — and CloudFormation would not recreate it, because + // that stack is not the one being deployed. Its agent logging would simply go dark. + // + // Refuse rather than guess. `--check-only` is unaffected (it deletes nothing), and an + // argument-free deploy is unaffected (default is then the correct derivation, exactly + // as in the CDK app). + if (source === 'default' && argv.length > 0) { + throw new Error( + `refusing to delete from '${stackName}': arguments were supplied (${argv.join(' ')}) but the ` + + 'stack name still resolved from the built-in default, so the target is a guess rather ' + + 'than a derivation of this deploy. Name it explicitly — STACK_NAME=, ' + + '--stack-name , or -c stackName= — or re-run with --check-only to see ' + + 'what would be deleted.', + ); + } + + // Deliveries reference their source and destination, so they go first. + // + // Iterating BY TYPE also re-applies the resource-type restriction, independently of the + // filter above. That redundancy is deliberate and load-bearing: `LEGACY_ID` matches the + // library's stack-scoped `AWS::Logs::ResourcePolicy` + // (`CdkLogGroupLogsDeliveryPolicy…`) too, and deleting that would break log delivery for + // every log type at once — with no rename to make CloudFormation put it back. Flattening + // this to `for (const r of legacy)` would do exactly that; the fixture in + // `test/scripts/preflight-log-delivery.test.ts` includes such a row so the flattening + // fails the suite instead of a deployment. + const order: DeliveryType[] = [ + 'AWS::Logs::Delivery', + 'AWS::Logs::DeliverySource', + 'AWS::Logs::DeliveryDestination', + ]; + for (const type of order) { + for (const r of legacy.filter((l) => l.ResourceType === type)) { + if (!r.PhysicalResourceId) { + throw new Error( + `stack resource ${r.LogicalResourceId} has no physical id; cannot migrate safely`, + ); + } + deleteResource(type, r.PhysicalResourceId); + } + } + console.log( + 'log-delivery preflight: migration applied — the deploy will recreate log delivery ' + + 'under library-managed ids. This was a one-time step; future deploys pass through.', + ); + return 0; +} + +try { + process.exit(main()); +} catch (err) { + const message = err instanceof Error ? err.message : String(err); + console.error(`log-delivery preflight: ${message}`); + // The reason for aborting is NOT the same in every case, and asserting the wrong one + // sends the reader after the wrong problem. With no `aws` on PATH this printed + // "proceeding would fail mid-update with AlreadyExists" — a claim about a resource + // collision, when the actual fault was a missing CLI and the collision may not exist at + // all. Distinguish "could not determine the state" from "the state is known-bad". + const couldNotDetermine = + /ENOENT|not found|could not list resources|AccessDenied|not authorized|ExpiredToken|credential/i + .test(message); + console.error( + couldNotDetermine + ? 'Aborting before deploy: the migration state could not be determined, so whether this ' + + 'deploy would hit the AlreadyExists collision is UNKNOWN — this is not itself evidence ' + + 'of a problem with your stack. Fix the error above (commonly: no AWS CLI on PATH, or ' + + 'credentials lacking cloudformation:ListStackResources) and re-run.' + : 'Aborting before deploy: the stack is on the retired pinned ids, so proceeding would fail ' + + 'mid-update with AlreadyExists and roll back.', + ); + console.error( + 'Manual path: docs/design/OBSERVABILITY.md ("AgentCore log delivery"). To see what would ' + + 'be deleted without deleting: --check-only. To bypass at your own risk: ' + + 'ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1.', + ); + process.exit(1); +} diff --git a/cdk/src/stacks/agent.ts b/cdk/src/stacks/agent.ts index 953ee0f7..d1bae787 100644 --- a/cdk/src/stacks/agent.ts +++ b/cdk/src/stacks/agent.ts @@ -687,30 +687,32 @@ 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 --- + // --- AgentCore log delivery: the library owns these logical ids, not us --- // - // 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. + // The Runtime above creates a DeliverySource / DeliveryDestination / Delivery + // trio per loggingConfig, naming them from its own construct path. We + // deliberately leave those names alone, and that is load-bearing. // - // 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. + // Renaming any of them is fatal on an existing stack. A DeliverySource is + // unique per (resource ARN, log type) account-wide and the runtime ARN does not + // change, so CloudFormation's create-before-delete produces a second source for + // the same runtime, CloudWatch Logs rejects it as already existing, and the + // whole update rolls back. Note what that implies: no choice of name avoids the + // collision, because the conflict is on the ARN they point at rather than on + // their own names. Only leaving a logical id untouched updates in place. // - // 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); + // An earlier version overrode them from a table of ids recorded off a live + // stack, keyed by stack NAME. 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 caused the rename on the other. No set of literals can + // describe every account, so we hold none and let the library generate them — + // deterministically, identically in every account, with nothing to keep in sync. + // + // The one cost: a stack deployed before a library-side rename, or held on older + // ids by that table, converges once and needs a one-time operator step first, + // since the old resources must be gone before the new ones are created. That + // step, and how to tell whether a stack needs it, are in + // docs/design/OBSERVABILITY.md ("AgentCore log delivery"). // --- Session storage (preview) --- // The L2 construct does not yet expose filesystemConfigurations; use the @@ -2123,107 +2125,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/scripts/preflight-log-delivery.test.ts b/cdk/test/scripts/preflight-log-delivery.test.ts new file mode 100644 index 00000000..46e0d049 --- /dev/null +++ b/cdk/test/scripts/preflight-log-delivery.test.ts @@ -0,0 +1,519 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * Tests for `cdk/scripts/preflight-log-delivery.ts` — the one-time + * log-delivery migration guard in the deploy path (#703). + * + * The script is a GATE with a delete side effect, so both failure directions + * matter: a false "nothing to do" sends an unmigrated stack into the + * mid-deploy AlreadyExists rollback the preflight exists to prevent, and an + * over-broad match deletes delivery resources that are not the stack's. It + * is exercised the way it really runs — spawned as a subprocess — with a fake + * `aws` executable on PATH that replays canned CloudFormation/CloudWatch Logs + * responses and records every invocation, so the assertions cover the real + * argument construction, deletion ordering, and exit codes rather than a + * re-implementation. (Same placement rationale as + * `check-constants-sync.test.ts`: `cdk/test/` is the Jest tree that can reach + * the script.) + */ + +import { execFileSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const SCRIPT = path.resolve(__dirname, '../../scripts/preflight-log-delivery.ts'); + +/** Live-state fixtures, shaped like `aws cloudformation list-stack-resources`. */ +const LIBRARY_IDS = { + StackResourceSummaries: [ + { + LogicalResourceId: 'RuntimeApplicationLogsDeliverySource818497BD', + PhysicalResourceId: 'backgroundagent-dev-Runtime-APPLICATION_LOGS', + ResourceType: 'AWS::Logs::DeliverySource', + ResourceStatus: 'UPDATE_COMPLETE', + }, + { + LogicalResourceId: 'SomeUnrelatedFn', + PhysicalResourceId: 'fn-phys', + ResourceType: 'AWS::Lambda::Function', + ResourceStatus: 'CREATE_COMPLETE', + }, + ], +}; + +const PINNED_IDS = { + StackResourceSummaries: [ + { + // NOT a delivery resource, but its logical id DOES match LEGACY_ID — the library + // really creates this at stack scope (`observability.js`, policyId + // "CdkLogGroupLogsDeliveryPolicy"). Only the resource-type filter keeps it out of + // the delete set, and nothing pinned that filter before this row existed. It is a + // stack-wide policy: deleting it would break log delivery for every log type at + // once, and CloudFormation would not recreate it from a rename. + LogicalResourceId: 'CdkLogGroupLogsDeliveryPolicyResourcePolicy4', + PhysicalResourceId: 'backgroundagent-dev-CdkLogGroupLogsDeliveryPolicy', + ResourceType: 'AWS::Logs::ResourcePolicy', + ResourceStatus: 'UPDATE_COMPLETE', + }, + { + LogicalResourceId: + 'RuntimeCDKSourceAPPLICATIONLOGSbackgroundagentdevRuntimeBC0AE9ED96A02E02', + PhysicalResourceId: 'cdk-applicationlogs-source-backgroundagentdevRuntimeBC0AE9ED', + ResourceType: 'AWS::Logs::DeliverySource', + ResourceStatus: 'UPDATE_COMPLETE', + }, + { + LogicalResourceId: + 'RuntimeCdkLogGroupApplicationLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeApplicationLogGroup454A95E8DestapplicationlogsE09F77DC', + PhysicalResourceId: 'cdk-cwl-Destapplication-logs-dest-backgrounp454A95E829BF8A27', + ResourceType: 'AWS::Logs::DeliveryDestination', + ResourceStatus: 'UPDATE_COMPLETE', + }, + { + LogicalResourceId: + 'RuntimeCdkLogGroupApplicationLogsDeliverybackgroundagentdevRuntimeBC0AE9EDbackgroundagentdevRuntimeApplicationLogGroup454A95E8Delivery92FE492C', + PhysicalResourceId: 'AhrN8hFRPWjPQU2Sh', + ResourceType: 'AWS::Logs::Delivery', + ResourceStatus: 'UPDATE_COMPLETE', + }, + // A delivery resource already on the library's naming must NOT be deleted + // even while pinned siblings are being migrated. + { + LogicalResourceId: 'RuntimeUsageLogsDeliverySourceF66198FF', + PhysicalResourceId: 'backgroundagent-dev-Runtime-USAGE_LOGS', + ResourceType: 'AWS::Logs::DeliverySource', + ResourceStatus: 'UPDATE_COMPLETE', + }, + ], +}; + +interface RunResult { + status: number; + stdout: string; + stderr: string; + /** Every `aws ` invocation the script made, one line per call. */ + calls: string[]; +} + +/** + * Run the preflight with a fake `aws` on PATH. + * + * The fake logs each invocation to CALLS_FILE, answers + * `cloudformation list-stack-resources` with the given fixture (or a + * "does not exist" error for the fresh-install case), succeeds on + * `logs delete-*` — except names listed in `failDeletes`, which fail with + * the given stderr once. + */ +function runPreflight(opts: { + listResponse?: object; + /** Written to `cdk/cdk.context.json` for the duration of the run (B3c). */ + cdkContext?: object; + stackMissing?: boolean; + listFails?: boolean; + failDeletes?: Record; + args?: string[]; + env?: Record; +}): RunResult { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'preflight-test-')); + // Context files come from the scratch root via ABCA_PREFLIGHT_CONTEXT_DIR, NOT the real + // `cdk/` directory. Two reasons, both learned the hard way: + // + // - whether a real `cdk.context.json` exists is an ENVIRONMENT fact. Locally it usually + // does not; CI's `build.yml` writes `{"stackName":"pr-"}` into it before + // the test job. Reading it made every test expecting the DEFAULT stack resolve to the + // pipeline's stack — green locally, two failures in CI. + // - the real file is CDK's lookup cache, shared with every suite jest runs in PARALLEL. + // Writing or deleting it from here raced the stack-synth tests: 73 failures, from a + // cached availability-zone lookup disappearing mid-synth. + // + // So the suite owns its own directory and the repo is never touched. + if (opts.cdkContext) { + fs.writeFileSync(path.join(root, 'cdk.context.json'), JSON.stringify(opts.cdkContext)); + } + try { + const callsFile = path.join(root, 'calls.log'); + fs.writeFileSync(callsFile, ''); + fs.writeFileSync(path.join(root, 'list-response.json'), JSON.stringify(opts.listResponse ?? {})); + fs.writeFileSync(path.join(root, 'fail-deletes.json'), JSON.stringify(opts.failDeletes ?? {})); + + const fakeAws = path.join(root, 'aws'); + fs.writeFileSync( + fakeAws, + `#!/usr/bin/env node +const fs = require('node:fs'); +const path = require('node:path'); +const root = ${JSON.stringify(root)}; +const argv = process.argv.slice(2); +fs.appendFileSync(path.join(root, 'calls.log'), argv.join(' ') + '\\n'); +// Skip leading global options the way the real CLI does, so the command match below +// still works when --profile/--region are forwarded ahead of the subcommand. +const args = []; +for (let i = 0; i < argv.length; i += 1) { + if (argv[i] === '--profile' || argv[i] === '--region') { i += 1; continue; } + args.push(argv[i]); +} +if (args[0] === 'cloudformation' && args[1] === 'list-stack-resources') { + if (${JSON.stringify(opts.stackMissing ?? false)}) { + process.stderr.write('An error occurred (ValidationError): Stack with id x does not exist'); + process.exit(254); + } + if (${JSON.stringify(opts.listFails ?? false)}) { + process.stderr.write('An error occurred (AccessDenied): not authorized'); + process.exit(254); + } + process.stdout.write(fs.readFileSync(path.join(root, 'list-response.json'), 'utf8')); + process.exit(0); +} +if (args[0] === 'sts' && args[1] === 'get-caller-identity') { + process.stdout.write('123456789012\\n'); + process.exit(0); +} +if (args[0] === 'configure' && args[1] === 'get' && args[2] === 'region') { + process.stdout.write('us-east-1\\n'); + process.exit(0); +} +if (args[0] === 'logs' && args[1].startsWith('delete-')) { + const target = args[args.length - 1]; + const failures = JSON.parse(fs.readFileSync(path.join(root, 'fail-deletes.json'), 'utf8')); + if (failures[target]) { + process.stderr.write(failures[target]); + process.exit(254); + } + process.exit(0); +} +process.stderr.write('fake aws: unexpected command: ' + args.join(' ')); +process.exit(99); +`, + { mode: 0o755 }, + ); + + const env = { + ...process.env, + PATH: `${root}${path.delimiter}${process.env.PATH}`, + ABCA_SKIP_LOG_DELIVERY_PREFLIGHT: '', + ABCA_LOG_DELIVERY_PREFLIGHT: '', + STACK_NAME: '', + ABCA_PREFLIGHT_CONTEXT_DIR: root, + ...opts.env, + }; + + let status = 0; + let stdout = ''; + let stderr = ''; + try { + stdout = execFileSync( + process.execPath, + ['--experimental-strip-types', SCRIPT, ...(opts.args ?? [])], + { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], env }, + ); + } catch (err) { + const e = err as { status?: number; stdout?: string; stderr?: string }; + status = e.status ?? -1; + stdout = e.stdout ?? ''; + stderr = e.stderr ?? ''; + } + const calls = fs.readFileSync(callsFile, 'utf8').split('\n').filter(Boolean); + return { status, stdout, stderr, calls }; + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +/** + * Strip the forwarded global options (`--profile x`, `--region y`) that now prefix every + * logged call, so a call can be matched on its subcommand regardless of whether the test + * supplied them. Mirrors how the real CLI reads them. + */ +const subcommand = (call: string) => + call.split(' ').reduce((acc, tok, i, all) => { + if (tok === '--profile' || tok === '--region') return acc; + if (i > 0 && (all[i - 1] === '--profile' || all[i - 1] === '--region')) return acc; + return [...acc, tok]; + }, []).join(' '); + +const deleteCalls = (r: RunResult) => + r.calls.map(subcommand).filter((c) => c.startsWith('logs delete-')); +/** + * The `list-stack-resources` call, located by CONTENT rather than by index. + * + * These assertions used `calls[0]`, which broke the moment the script gained an + * `sts get-caller-identity` (it now reports the account it resolved to, so a delete can + * be attributed after the fact). The subject of these tests is *which stack was + * inspected*, not the call ordering, so pin the former and let the latter move. + */ +const listCall = (r: RunResult) => + r.calls.map(subcommand).find((c) => c.startsWith('cloudformation list-stack-resources')) ?? ''; + +describe('preflight-log-delivery', () => { + test('no-ops when the stack does not exist (fresh install)', () => { + const r = runPreflight({ stackMissing: true }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('not deployed yet'); + expect(deleteCalls(r)).toHaveLength(0); + }); + + test('no-ops when delivery resources already use library-managed ids', () => { + const r = runPreflight({ listResponse: LIBRARY_IDS }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('already on library-managed ids'); + expect(deleteCalls(r)).toHaveLength(0); + }); + + test('deletes exactly the pinned resources, deliveries before sources/destinations', () => { + const r = runPreflight({ listResponse: PINNED_IDS }); + expect(r.status).toBe(0); + const deletes = deleteCalls(r); + expect(deletes).toEqual([ + 'logs delete-delivery --id AhrN8hFRPWjPQU2Sh', + 'logs delete-delivery-source --name cdk-applicationlogs-source-backgroundagentdevRuntimeBC0AE9ED', + 'logs delete-delivery-destination --name cdk-cwl-Destapplication-logs-dest-backgrounp454A95E829BF8A27', + ]); + // The sibling already on library naming was in the same stack and same + // resource types — it must survive the migration. + expect(deletes.join('\n')).not.toContain('backgroundagent-dev-Runtime-USAGE_LOGS'); + expect(r.stdout).toContain('migration applied'); + // The stack-scoped AWS::Logs::ResourcePolicy in the fixture matches LEGACY_ID by + // logical id and is excluded ONLY by the resource-type filter. Asserted by name as + // well as by the exact-set equality above, so a regression that widened the filter + // fails with a message naming the resource rather than a diff of three strings. + expect(deletes.join('\n')).not.toContain('CdkLogGroupLogsDeliveryPolicy'); + expect(deletes.join('\n')).not.toContain('ResourcePolicy'); + }); + + test('refuses to delete when arguments were given but the stack fell back to the default', () => { + // The compound failure this guards: an account holding a legacy `backgroundagent-dev` + // plus a second stack, deployed with an argument this script does not understand. + // Without the guard it deleted the FIRST stack's delivery resources — and + // CloudFormation would not recreate them, because that stack is not the one being + // deployed, so its agent logging simply goes dark. + const r = runPreflight({ listResponse: PINNED_IDS, args: ['--profile', 'prod'] }); + expect(r.status).toBe(1); + expect(deleteCalls(r)).toHaveLength(0); + expect(r.stderr).toContain('refusing to delete'); + expect(r.stderr).toContain('--profile prod'); + // And it must offer the way out rather than just refusing. + expect(r.stderr).toMatch(/STACK_NAME|--stack-name|-c stackName=/); + }); + + test('an explicit stack name re-enables deletion even with other arguments present', () => { + // The guard must not block the legitimate case: name the stack and it proceeds. + const r = runPreflight({ + listResponse: PINNED_IDS, + args: ['--profile', 'prod', '--stack-name', 'backgroundagent-dev'], + }); + expect(r.status).toBe(0); + expect(deleteCalls(r)).toHaveLength(3); + }); + + test('forwards --profile and --region to its own AWS calls', () => { + // Otherwise the deploy targets one account while this script reads — and deletes — + // in whatever the ambient profile points at. A delete in the wrong account is the + // worst outcome available to this script. + const r = runPreflight({ + listResponse: PINNED_IDS, + args: ['--profile', 'prod', '--region', 'eu-west-1', '--stack-name', 'backgroundagent-dev'], + }); + expect(r.status).toBe(0); + for (const call of r.calls) { + expect(call).toContain('--profile prod'); + expect(call).toContain('--region eu-west-1'); + } + }); + + test('ABCA_LOG_DELIVERY_PREFLIGHT=check behaves like --check-only', () => { + // Documented as an equivalent knob but only the flag was covered. + const r = runPreflight({ + listResponse: PINNED_IDS, + env: { ABCA_LOG_DELIVERY_PREFLIGHT: 'check' }, + }); + expect(r.status).toBe(2); + expect(deleteCalls(r)).toHaveLength(0); + expect(r.stdout).toContain('Check-only mode'); + }); + + test('skips a resource already reported DELETE_COMPLETE', () => { + // A re-run after a partially applied migration: CloudFormation still lists the + // resource, with a status saying it is gone. Deleting again is a wasted call whose + // failure mode is a raw CLI error on the deploy path. + const r = runPreflight({ + listResponse: { + StackResourceSummaries: [ + { + LogicalResourceId: 'RuntimeCDKSourceAPPLICATIONLOGSbackgroundagentdevRuntimeBC0AE9ED96A02E02', + PhysicalResourceId: 'already-deleted-source', + ResourceType: 'AWS::Logs::DeliverySource', + ResourceStatus: 'DELETE_COMPLETE', + }, + ], + }, + }); + expect(r.status).toBe(0); + expect(deleteCalls(r)).toHaveLength(0); + expect(r.stdout).toContain('already on library-managed ids'); + }); + + test('aborts rather than guessing when a legacy resource has no physical id', () => { + // No physical id means nothing safe to pass to `delete-delivery*`; the only correct + // move is to stop before the deploy rather than delete something else. + const r = runPreflight({ + listResponse: { + StackResourceSummaries: [ + { + LogicalResourceId: 'RuntimeCDKSourceAPPLICATIONLOGSbackgroundagentdevRuntimeBC0AE9ED96A02E02', + ResourceType: 'AWS::Logs::DeliverySource', + ResourceStatus: 'UPDATE_COMPLETE', + }, + ], + }, + }); + expect(r.status).toBe(1); + expect(deleteCalls(r)).toHaveLength(0); + expect(r.stderr).toContain('no physical id'); + }); + + test('resolves the stack from cdk.context.json — the file the CI pipeline writes', () => { + // Mutation-caught gap: removing `cdk.context.json` from the resolution chain left the + // whole suite green. It is the load-bearing source in CI — `build.yml` writes + // `stackName` there for every pipeline stack (`pr-`), and this repo's + // `cdk.json` has NO `context` block at all, so reading only `cdk.json` meant the + // pipeline resolved to the default and would have inspected the wrong stack. + const r = runPreflight({ + listResponse: LIBRARY_IDS, + cdkContext: { stackName: 'pr705-agentcore' }, + }); + expect(r.status).toBe(0); + expect(listCall(r)).toContain('--stack-name pr705-agentcore'); + expect(r.stdout).toContain('(from cdk.context.json)'); + }); + + test('an explicit flag still outranks cdk.context.json', () => { + // Precedence must match `cdk` itself: an argument beats persisted context. + const r = runPreflight({ + listResponse: LIBRARY_IDS, + cdkContext: { stackName: 'from-context' }, + args: ['--stack-name', 'from-flag'], + }); + expect(listCall(r)).toContain('--stack-name from-flag'); + }); + + test('names the account and region it resolved, not just the stack', () => { + // A stack name alone is ambiguous across accounts, and this script deletes — the log + // has to be enough to reconstruct the scope after the fact. + const r = runPreflight({ listResponse: LIBRARY_IDS }); + expect(r.stdout).toMatch(/inspecting stack '[^']+' \(from [^)]+\) in account 123456789012/); + }); + + test('--check-only reports the pinned resources, deletes nothing, exits 2', () => { + const r = runPreflight({ listResponse: PINNED_IDS, args: ['--check-only'] }); + expect(r.status).toBe(2); + expect(deleteCalls(r)).toHaveLength(0); + expect(r.stdout).toContain('Check-only mode'); + }); + + test('treats an already-deleted resource as success (idempotent re-run)', () => { + const r = runPreflight({ + listResponse: PINNED_IDS, + failDeletes: { + AhrN8hFRPWjPQU2Sh: + 'An error occurred (ResourceNotFoundException): Delivery does not exist', + }, + }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('already gone'); + expect(r.stdout).toContain('migration applied'); + }); + + test('aborts (exit 1) when a delete fails for any other reason', () => { + const r = runPreflight({ + listResponse: PINNED_IDS, + failDeletes: { + AhrN8hFRPWjPQU2Sh: 'An error occurred (AccessDeniedException): not authorized', + }, + }); + expect(r.status).toBe(1); + expect(r.stderr).toContain('AccessDeniedException'); + expect(r.stderr).toContain('Aborting before deploy'); + }); + + test('aborts (exit 1) when the stack state cannot be determined', () => { + const r = runPreflight({ listFails: true }); + expect(r.status).toBe(1); + expect(r.stderr).toContain('could not list resources'); + expect(deleteCalls(r)).toHaveLength(0); + }); + + test('honors --stack-name and STACK_NAME for non-default stacks', () => { + const viaFlag = runPreflight({ + listResponse: LIBRARY_IDS, + args: ['--stack-name', 'my-custom-stack'], + }); + expect(listCall(viaFlag)).toContain('--stack-name my-custom-stack'); + + const viaEnv = runPreflight({ listResponse: LIBRARY_IDS, env: { STACK_NAME: 'env-stack' } }); + expect(listCall(viaEnv)).toContain('--stack-name env-stack'); + }); + + test('accepts CDK context form, so a direct invocation cannot target a different stack', () => { + // `cdk deploy` selects its stack from `stackName` CONTEXT, not from --stack-name. + // Reading only the flag/env let this script inspect one stack while the deploy it + // gates built another — and this script deletes, so the two must not diverge. + for (const args of [ + ['-c', 'stackName=ctx-stack'], + ['--context', 'stackName=ctx-stack'], + ['--context=stackName=ctx-stack'], + ]) { + const r = runPreflight({ listResponse: LIBRARY_IDS, args }); + expect(listCall(r)).toContain('--stack-name ctx-stack'); + } + }); + + test('the flag wins over the env var, and both over context', () => { + // Precedence pinned because the three can disagree on a real command line. + const r = runPreflight({ + listResponse: LIBRARY_IDS, + args: ['--stack-name', 'flag-stack', '-c', 'stackName=ctx-stack'], + env: { STACK_NAME: 'env-stack' }, + }); + expect(listCall(r)).toContain('--stack-name flag-stack'); + }); + + test('names the stack AND where the name came from, so a mismatch is visible', () => { + // The one gap that cannot be closed in-script: context passed to `cdk deploy` after + // `--` never reaches a mise `depends` task. Printing the resolved target is what + // turns that from an invisible divergence into something an operator can see in the + // deploy log before anything is deleted. + const r = runPreflight({ listResponse: LIBRARY_IDS, env: { STACK_NAME: 'env-stack' } }); + expect(r.stdout).toContain("inspecting stack 'env-stack' (from STACK_NAME)"); + + const dflt = runPreflight({ listResponse: LIBRARY_IDS }); + expect(dflt.stdout).toContain("inspecting stack 'backgroundagent-dev' (from default)"); + }); + + test('ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1 skips without touching AWS', () => { + const r = runPreflight({ + listResponse: PINNED_IDS, + env: { ABCA_SKIP_LOG_DELIVERY_PREFLIGHT: '1' }, + }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('skipped'); + expect(r.calls).toHaveLength(0); + }); +}); diff --git a/cdk/test/stacks/agent.test.ts b/cdk/test/stacks/agent.test.ts index 9cb1cd28..ab22f600 100644 --- a/cdk/test/stacks/agent.test.ts +++ b/cdk/test/stacks/agent.test.ts @@ -777,59 +777,66 @@ 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'); + // Deliberately the library's CURRENT id shape, so this assertion doubles as a + // canary: the next time the AgentCore library renames these resources, this + // fails here rather than in a deploy. Read the failure as an operational + // warning, not a string mismatch — every already-deployed stack will try to + // rename its DeliverySource on the next update, and a renamed source collides + // with the live one on the unchanged runtime ARN and rolls the stack back. + // Update the regex, and add the new shape to the migration note in + // docs/design/OBSERVABILITY.md ("AgentCore log delivery") so operators can + // tell which side of the rename their stack is on. + 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', () => { diff --git a/docs/design/OBSERVABILITY.md b/docs/design/OBSERVABILITY.md index b6db7a8d..6d1a595c 100644 --- a/docs/design/OBSERVABILITY.md +++ b/docs/design/OBSERVABILITY.md @@ -179,6 +179,91 @@ For post-mortems, eval-harness input, and compliance export, the API exposes a s Fields whose source did not run for a given task are returned `null`/empty (e.g. no `--trace` → `trace_uri: null`), so the schema is stable for consumers. +## AgentCore log delivery + +The agent runtime's application and usage logs reach CloudWatch through a trio of resources per log type — `AWS::Logs::DeliverySource`, `AWS::Logs::DeliveryDestination`, and an `AWS::Logs::Delivery` link joining them. The stack does not declare these. The AgentCore `Runtime` L2 construct creates them from the `loggingConfigs` passed to it, and names them from its own internal construct path. + +**The stack deliberately does not override their logical ids.** That is a load-bearing decision rather than an oversight, because the obvious alternatives are worse in a way that is not obvious until a deployment fails. + +### Why a rename of these resources is fatal + +A `DeliverySource` is unique per `(resource ARN, log type)` for the whole account. The agent runtime's ARN does not change when the delivery resources are renamed, so: + +1. Changing a delivery resource's logical id makes CloudFormation treat it as a new resource, which it **creates before deleting** the old one. +2. The new source points at the same runtime ARN as the live one, which still exists at that moment. +3. CloudWatch Logs rejects it — `AlreadyExists`, "This ResourceId has already been used in another Delivery Source in this account." +4. The whole stack update rolls back. + +The counter-intuitive consequence: **no choice of name avoids this.** The conflict is on the ARN the sources point at, not on their own names, so renaming them to library-generated ids, to hand-picked stable ids, or to anything else collides identically. Only leaving a logical id untouched avoids it, because that is what makes CloudFormation update in place instead of creating a second source for the same runtime. + +### Why the ids are not pinned in the template + +An earlier version of the stack held a table of logical ids to override, keyed by stack name, with values read off a live stack. This does not work, because **stack name is not a proxy for deployed state.** Two accounts running a stack of the same name can sit on different library versions' naming, so one set of literals is correct for one account and actively causes the fatal rename on the other. Re-recording the table inverts which account breaks rather than fixing either. + +Since no set of literals can describe every account's deployed state, the stack holds none. Log delivery is left to the library, which generates the ids from the construct path — deterministically, identically in every account, with nothing to keep in sync. + +The cost of this choice is bounded and one-time: a stack deployed before a library-side rename, or held on older ids by the pin table, must converge once. See the migration below. The cost of the alternative was unbounded — a hand-maintained table that silently breaks a different account every time any library version or any deployment moves. + +### Migrating a stack that predates the current naming + +Fresh deployments need nothing here. A stack whose live delivery resources already match what the library generates needs nothing either, and `cdk diff` will show the six resources untouched. + +A stack still on older ids has to converge, and because the create-before-delete collision above applies, the old resources must be gone before the new ones are created. + +**`mise //cdk:deploy` handles this automatically** (the CI pipeline reports it but does not migrate — see the deployment guide's known-issues entry)**.** The deploy task runs a preflight (`cdk/scripts/preflight-log-delivery.ts`) that reads the stack's own resource list and, only when it finds delivery resources under the retired pinned ids (a `CDKSource` or `CdkLogGroup` segment in the logical id), deletes exactly those before the deploy proceeds. It deletes by the physical ids CloudFormation reports for *this stack*, so other delivery configurations in the account — even on a shared account — are unreachable by construction. Already-migrated stacks and fresh installs pass through untouched, so the preflight is safe to leave on every deploy. + +**Which stacks are affected is a property of deployed state, not of the stack's name.** The legacy ids are the *previous library's* naming, not a product of the pin table: `#339` (`9a58797c`, 2026-06-13) switched the stack from `@aws-cdk/aws-bedrock-agentcore-alpha` to `aws-cdk-lib/aws-bedrockagentcore`, and the pin table only landed seven weeks later in `#695` (`4357c353`, 2026-08-03). The alpha generated the `RuntimeCDKSource…` / `RuntimeCdkLogGroup…` shapes; `aws-cdk-lib` generates `RuntimeApplicationLogsDeliverySource`. + +So the affected predicate is exactly what the preflight itself tests: **the stack's live `AWS::Logs::Delivery*` logical ids contain `CDKSource` or `CdkLogGroup`** — true of any stack last deployed before that library switch, whatever it is called, and whether or not the pin table ever applied to it. A custom-named stack is *not* immune, and neither is a stack created fresh from a commit that carried the table. "It deploys fine today" does not mean "unaffected", because the collision only appears on the deploy that renames. + +Run the check once per stack, regardless of name — it deletes nothing: + +```bash +STACK_NAME= mise //cdk:preflight:log-delivery -- --check-only +``` + +Exit 0 means nothing to migrate; exit 2 lists what would be deleted. The preflight exists because the affected operators cannot be enumerated or notified. + +Knobs, all optional: + +```bash +# Report what would be deleted; exit 2 if migration is needed +mise //cdk:preflight:log-delivery -- --check-only + +# Non-default stack: set BOTH — see the note below +STACK_NAME=my-stack mise //cdk:deploy -- -c stackName=my-stack + +# Skip the preflight entirely (at your own risk) +ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1 mise //cdk:deploy +``` + +**A non-default stack needs the name given twice, and the two are not interchangeable.** `cdk deploy` selects its stack from `stackName` CDK *context*, while the preflight reads `STACK_NAME` (or `--stack-name`, or `stackName` in `cdk.json` context). Arguments after `--` are appended to the `cdk deploy` command and never reach a mise `depends` task, so `-c stackName=x` alone leaves the preflight on its default target — and `STACK_NAME=x` alone leaves the *deploy* on its default target. Setting only one is how you end up migrating one stack while deploying another. + +This is now closed rather than merely documented: the preflight runs as the first command of the `deploy` task, so the arguments after `--` reach it and `cdk deploy` alike, and `--profile` / `--region` are forwarded to its own AWS calls. It also reads `cdk.context.json` (the file the CI pipeline writes `stackName` into), not just `cdk.json`. + +If the stack name still resolves from the built-in default *while arguments were supplied*, the preflight refuses to delete rather than guess — the case that would otherwise migrate one stack's delivery resources while deploying another, leaving the first one's agent logging dark until it is deployed itself. Every run prints the stack, where the name came from, and the account and region it resolved to: + +``` +log-delivery preflight: inspecting stack 'backgroundagent-dev' + (from default) +``` + +To migrate by hand instead (e.g. deploying without mise), list the stack's delivery resources and delete only those, deliveries first — they reference the source and destination: + +```bash +aws cloudformation list-stack-resources --stack-name backgroundagent-dev \ + --query "StackResourceSummaries[?contains(ResourceType,'Logs::Delivery')].\ +[ResourceType,LogicalResourceId,PhysicalResourceId]" --output text +# For each row whose LogicalResourceId carries a CDKSource/CdkLogGroup segment: +# AWS::Logs::Delivery -> aws logs delete-delivery --id +# AWS::Logs::DeliverySource -> aws logs delete-delivery-source --name +# AWS::Logs::DeliveryDestination -> aws logs delete-delivery-destination --name +``` + +Do the deletion immediately before deploying, not in advance: agent logs stop being delivered from the deletion until the deploy completes, and a full deploy (image build included) can take an hour. The deploy then creates the delivery trio under the library's naming and drops the old logical ids, whose underlying resources are already gone — CloudFormation treats a delete of an absent resource as done. Nothing else is affected, and no log data already in CloudWatch is touched. + +If the migration is skipped, the failure is loud but unexplained: the update fails on the new `AWS::Logs::DeliverySource` with `AlreadyExists` ("This ResourceId has already been used in another Delivery Source") and the whole stack rolls back. The stack keeps running on its previous template; run the preflight (or the manual steps) and deploy again. + ## Deployment safety Agent sessions run for up to 8 hours. CDK deployments replace Lambda functions, which can orphan in-flight orchestrator executions. The platform handles this through multiple mechanisms: diff --git a/docs/guides/DEPLOYMENT_GUIDE.md b/docs/guides/DEPLOYMENT_GUIDE.md index d2610486..d0e81057 100644 --- a/docs/guides/DEPLOYMENT_GUIDE.md +++ b/docs/guides/DEPLOYMENT_GUIDE.md @@ -217,6 +217,24 @@ Triggers via `workflow_run` when `build.yml` completes successfully. The pipelin ## Known deployment issues +### Log-delivery rename on upgrade (pin-table removal, #703) + +**Affects:** any stack — **whatever its name** — whose live `AWS::Logs::Delivery*` logical ids contain `CDKSource` or `CdkLogGroup`. In practice that is any stack last deployed before the CDK library switch in #339 (2026-06-13), plus any stack deployed while the pin table held those ids. The legacy names come from the *previous library version*, not from the pin table, so a custom stack name confers no immunity. A currently-working deployment is **not** evidence of being unaffected: the failure only appears on the first deploy after upgrading past the pin removal. + +Check any stack in one command — it deletes nothing: + +```bash +STACK_NAME= mise //cdk:preflight:log-delivery -- --check-only +``` + +**Symptom:** The update fails on a new `AWS::Logs::DeliverySource` with `AlreadyExists` ("This ResourceId has already been used in another Delivery Source in this account") and the whole stack rolls back. The stack keeps running on its previous template. + +**Resolution:** None needed if you deploy with `mise //cdk:deploy`. The preflight is the first command of that task, so it detects the legacy resources on the live stack and applies the one-time migration — deleting exactly those resources, scoped to that stack, in the same account and region as the deploy, because the task forwards its own arguments to it. + +**The GitHub Actions pipeline does not migrate automatically.** Its deploy job calls `cdk deploy` directly and so inherits none of that task's steps. What it does do is *report*: the read-only **diff** job runs the preflight with `--check-only` and writes the result into the job summary, so a stack needing migration is visible in the run summary before the approval gate rather than discovered as a rollback afterwards. Migrate it with `STACK_NAME= mise //cdk:deploy`, or the manual sequence below, then re-run the pipeline. + +A bare `cdk deploy` (no mise, no pipeline) does **not** run the preflight. On that path, or after the rollback above, run the check command shown under *Affects* and then follow the manual steps in [Observability — AgentCore log delivery](../design/OBSERVABILITY.md#agentcore-log-delivery). + ### AgentCore unsupported Availability Zones **Affects:** Fresh deploys in accounts whose default Availability Zones don't line up with the zones AgentCore supports for the region. diff --git a/docs/src/content/docs/architecture/Observability.md b/docs/src/content/docs/architecture/Observability.md index e032fd65..c39700eb 100644 --- a/docs/src/content/docs/architecture/Observability.md +++ b/docs/src/content/docs/architecture/Observability.md @@ -183,6 +183,91 @@ For post-mortems, eval-harness input, and compliance export, the API exposes a s Fields whose source did not run for a given task are returned `null`/empty (e.g. no `--trace` → `trace_uri: null`), so the schema is stable for consumers. +## AgentCore log delivery + +The agent runtime's application and usage logs reach CloudWatch through a trio of resources per log type — `AWS::Logs::DeliverySource`, `AWS::Logs::DeliveryDestination`, and an `AWS::Logs::Delivery` link joining them. The stack does not declare these. The AgentCore `Runtime` L2 construct creates them from the `loggingConfigs` passed to it, and names them from its own internal construct path. + +**The stack deliberately does not override their logical ids.** That is a load-bearing decision rather than an oversight, because the obvious alternatives are worse in a way that is not obvious until a deployment fails. + +### Why a rename of these resources is fatal + +A `DeliverySource` is unique per `(resource ARN, log type)` for the whole account. The agent runtime's ARN does not change when the delivery resources are renamed, so: + +1. Changing a delivery resource's logical id makes CloudFormation treat it as a new resource, which it **creates before deleting** the old one. +2. The new source points at the same runtime ARN as the live one, which still exists at that moment. +3. CloudWatch Logs rejects it — `AlreadyExists`, "This ResourceId has already been used in another Delivery Source in this account." +4. The whole stack update rolls back. + +The counter-intuitive consequence: **no choice of name avoids this.** The conflict is on the ARN the sources point at, not on their own names, so renaming them to library-generated ids, to hand-picked stable ids, or to anything else collides identically. Only leaving a logical id untouched avoids it, because that is what makes CloudFormation update in place instead of creating a second source for the same runtime. + +### Why the ids are not pinned in the template + +An earlier version of the stack held a table of logical ids to override, keyed by stack name, with values read off a live stack. This does not work, because **stack name is not a proxy for deployed state.** Two accounts running a stack of the same name can sit on different library versions' naming, so one set of literals is correct for one account and actively causes the fatal rename on the other. Re-recording the table inverts which account breaks rather than fixing either. + +Since no set of literals can describe every account's deployed state, the stack holds none. Log delivery is left to the library, which generates the ids from the construct path — deterministically, identically in every account, with nothing to keep in sync. + +The cost of this choice is bounded and one-time: a stack deployed before a library-side rename, or held on older ids by the pin table, must converge once. See the migration below. The cost of the alternative was unbounded — a hand-maintained table that silently breaks a different account every time any library version or any deployment moves. + +### Migrating a stack that predates the current naming + +Fresh deployments need nothing here. A stack whose live delivery resources already match what the library generates needs nothing either, and `cdk diff` will show the six resources untouched. + +A stack still on older ids has to converge, and because the create-before-delete collision above applies, the old resources must be gone before the new ones are created. + +**`mise //cdk:deploy` handles this automatically** (the CI pipeline reports it but does not migrate — see the deployment guide's known-issues entry)**.** The deploy task runs a preflight (`cdk/scripts/preflight-log-delivery.ts`) that reads the stack's own resource list and, only when it finds delivery resources under the retired pinned ids (a `CDKSource` or `CdkLogGroup` segment in the logical id), deletes exactly those before the deploy proceeds. It deletes by the physical ids CloudFormation reports for *this stack*, so other delivery configurations in the account — even on a shared account — are unreachable by construction. Already-migrated stacks and fresh installs pass through untouched, so the preflight is safe to leave on every deploy. + +**Which stacks are affected is a property of deployed state, not of the stack's name.** The legacy ids are the *previous library's* naming, not a product of the pin table: `#339` (`9a58797c`, 2026-06-13) switched the stack from `@aws-cdk/aws-bedrock-agentcore-alpha` to `aws-cdk-lib/aws-bedrockagentcore`, and the pin table only landed seven weeks later in `#695` (`4357c353`, 2026-08-03). The alpha generated the `RuntimeCDKSource…` / `RuntimeCdkLogGroup…` shapes; `aws-cdk-lib` generates `RuntimeApplicationLogsDeliverySource`. + +So the affected predicate is exactly what the preflight itself tests: **the stack's live `AWS::Logs::Delivery*` logical ids contain `CDKSource` or `CdkLogGroup`** — true of any stack last deployed before that library switch, whatever it is called, and whether or not the pin table ever applied to it. A custom-named stack is *not* immune, and neither is a stack created fresh from a commit that carried the table. "It deploys fine today" does not mean "unaffected", because the collision only appears on the deploy that renames. + +Run the check once per stack, regardless of name — it deletes nothing: + +```bash +STACK_NAME= mise //cdk:preflight:log-delivery -- --check-only +``` + +Exit 0 means nothing to migrate; exit 2 lists what would be deleted. The preflight exists because the affected operators cannot be enumerated or notified. + +Knobs, all optional: + +```bash +# Report what would be deleted; exit 2 if migration is needed +mise //cdk:preflight:log-delivery -- --check-only + +# Non-default stack: set BOTH — see the note below +STACK_NAME=my-stack mise //cdk:deploy -- -c stackName=my-stack + +# Skip the preflight entirely (at your own risk) +ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1 mise //cdk:deploy +``` + +**A non-default stack needs the name given twice, and the two are not interchangeable.** `cdk deploy` selects its stack from `stackName` CDK *context*, while the preflight reads `STACK_NAME` (or `--stack-name`, or `stackName` in `cdk.json` context). Arguments after `--` are appended to the `cdk deploy` command and never reach a mise `depends` task, so `-c stackName=x` alone leaves the preflight on its default target — and `STACK_NAME=x` alone leaves the *deploy* on its default target. Setting only one is how you end up migrating one stack while deploying another. + +This is now closed rather than merely documented: the preflight runs as the first command of the `deploy` task, so the arguments after `--` reach it and `cdk deploy` alike, and `--profile` / `--region` are forwarded to its own AWS calls. It also reads `cdk.context.json` (the file the CI pipeline writes `stackName` into), not just `cdk.json`. + +If the stack name still resolves from the built-in default *while arguments were supplied*, the preflight refuses to delete rather than guess — the case that would otherwise migrate one stack's delivery resources while deploying another, leaving the first one's agent logging dark until it is deployed itself. Every run prints the stack, where the name came from, and the account and region it resolved to: + +``` +log-delivery preflight: inspecting stack 'backgroundagent-dev' + (from default) +``` + +To migrate by hand instead (e.g. deploying without mise), list the stack's delivery resources and delete only those, deliveries first — they reference the source and destination: + +```bash +aws cloudformation list-stack-resources --stack-name backgroundagent-dev \ + --query "StackResourceSummaries[?contains(ResourceType,'Logs::Delivery')].\ +[ResourceType,LogicalResourceId,PhysicalResourceId]" --output text +# For each row whose LogicalResourceId carries a CDKSource/CdkLogGroup segment: +# AWS::Logs::Delivery -> aws logs delete-delivery --id +# AWS::Logs::DeliverySource -> aws logs delete-delivery-source --name +# AWS::Logs::DeliveryDestination -> aws logs delete-delivery-destination --name +``` + +Do the deletion immediately before deploying, not in advance: agent logs stop being delivered from the deletion until the deploy completes, and a full deploy (image build included) can take an hour. The deploy then creates the delivery trio under the library's naming and drops the old logical ids, whose underlying resources are already gone — CloudFormation treats a delete of an absent resource as done. Nothing else is affected, and no log data already in CloudWatch is touched. + +If the migration is skipped, the failure is loud but unexplained: the update fails on the new `AWS::Logs::DeliverySource` with `AlreadyExists` ("This ResourceId has already been used in another Delivery Source") and the whole stack rolls back. The stack keeps running on its previous template; run the preflight (or the manual steps) and deploy again. + ## Deployment safety Agent sessions run for up to 8 hours. CDK deployments replace Lambda functions, which can orphan in-flight orchestrator executions. The platform handles this through multiple mechanisms: diff --git a/docs/src/content/docs/getting-started/Deployment-guide.md b/docs/src/content/docs/getting-started/Deployment-guide.md index 8d9db0a2..b4822726 100644 --- a/docs/src/content/docs/getting-started/Deployment-guide.md +++ b/docs/src/content/docs/getting-started/Deployment-guide.md @@ -221,6 +221,24 @@ Triggers via `workflow_run` when `build.yml` completes successfully. The pipelin ## Known deployment issues +### Log-delivery rename on upgrade (pin-table removal, #703) + +**Affects:** any stack — **whatever its name** — whose live `AWS::Logs::Delivery*` logical ids contain `CDKSource` or `CdkLogGroup`. In practice that is any stack last deployed before the CDK library switch in #339 (2026-06-13), plus any stack deployed while the pin table held those ids. The legacy names come from the *previous library version*, not from the pin table, so a custom stack name confers no immunity. A currently-working deployment is **not** evidence of being unaffected: the failure only appears on the first deploy after upgrading past the pin removal. + +Check any stack in one command — it deletes nothing: + +```bash +STACK_NAME= mise //cdk:preflight:log-delivery -- --check-only +``` + +**Symptom:** The update fails on a new `AWS::Logs::DeliverySource` with `AlreadyExists` ("This ResourceId has already been used in another Delivery Source in this account") and the whole stack rolls back. The stack keeps running on its previous template. + +**Resolution:** None needed if you deploy with `mise //cdk:deploy`. The preflight is the first command of that task, so it detects the legacy resources on the live stack and applies the one-time migration — deleting exactly those resources, scoped to that stack, in the same account and region as the deploy, because the task forwards its own arguments to it. + +**The GitHub Actions pipeline does not migrate automatically.** Its deploy job calls `cdk deploy` directly and so inherits none of that task's steps. What it does do is *report*: the read-only **diff** job runs the preflight with `--check-only` and writes the result into the job summary, so a stack needing migration is visible in the run summary before the approval gate rather than discovered as a rollback afterwards. Migrate it with `STACK_NAME= mise //cdk:deploy`, or the manual sequence below, then re-run the pipeline. + +A bare `cdk deploy` (no mise, no pipeline) does **not** run the preflight. On that path, or after the rollback above, run the check command shown under *Affects* and then follow the manual steps in [Observability — AgentCore log delivery](/sample-autonomous-cloud-coding-agents/architecture/observability#agentcore-log-delivery). + ### AgentCore unsupported Availability Zones **Affects:** Fresh deploys in accounts whose default Availability Zones don't line up with the zones AgentCore supports for the region.