From 8ccdff4ee083d66f73259223ba75ba0b8a0752a0 Mon Sep 17 00:00:00 2001 From: Sumu Pitchayan <35242245+sumupitchayan@users.noreply.github.com> Date: Fri, 29 Nov 2024 12:33:14 -0500 Subject: [PATCH] feat(aspects): priority-ordered aspect invocation (#32097) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #21341 This feature was designed in [RFC648](https://github.com/aws/aws-cdk-rfcs/pull/651) ### Reason for this change The current algorithm for invoking aspects (see invokeAspects in [synthesis.ts](https://github.com/aws/aws-cdk/blob/8b495f9ec157c0b00674715f62b1bbcabf2096ac/packages/aws-cdk-lib/core/lib/private/synthesis.ts#L217)) does not handle all use cases — specifically, when an Aspect adds a new node to the Construct tree and when Aspects are applied out of order. ### Description of changes This PR introduces a priority-based ordering system for aspects in the CDK to allow users to control the order in which aspects are applied on the construct tree. This PR also adds a stabilization loop for invoking aspects that can be enabled via the feature flag `@aws-cdk/core:aspectStabilization` - the stabilization loop ensures that newly added Aspects to the construct tree are visited and nested Aspects are invoked. ### Description of how you validated changes Plenty of unit tests - see `aspects.test.ts`. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk-lib/core/README.md | 142 ++++ packages/aws-cdk-lib/core/lib/aspect.ts | 108 ++- .../aws-cdk-lib/core/lib/private/synthesis.ts | 118 ++- packages/aws-cdk-lib/core/lib/stage.ts | 13 + .../aws-cdk-lib/core/test/aspect.prop.test.ts | 733 ++++++++++++++++++ packages/aws-cdk-lib/core/test/aspect.test.ts | 250 +++++- packages/aws-cdk-lib/cx-api/lib/features.ts | 16 + .../aws-cdk-lib/cx-api/test/features.test.ts | 1 + 8 files changed, 1359 insertions(+), 22 deletions(-) create mode 100644 packages/aws-cdk-lib/core/test/aspect.prop.test.ts diff --git a/packages/aws-cdk-lib/core/README.md b/packages/aws-cdk-lib/core/README.md index ecec3322f5351..17e65307db2ce 100644 --- a/packages/aws-cdk-lib/core/README.md +++ b/packages/aws-cdk-lib/core/README.md @@ -1629,6 +1629,148 @@ scenarios are (non-exhaustive list): valid) - Warn if the user is using a deprecated API +## Aspects + +[Aspects](https://docs.aws.amazon.com/cdk/v2/guide/aspects.html) is a feature in CDK that allows you to apply operations or transformations across all +constructs in a construct tree. Common use cases include tagging resources, enforcing encryption on S3 Buckets, or applying specific security or +compliance rules to all resources in a stack. + +Conceptually, there are two types of Aspects: + +- **Read-only aspects** scan the construct tree but do not make changes to the tree. Common use cases of read-only aspects include performing validations +(for example, enforcing that all S3 Buckets have versioning enabled) and logging (for example, collecting information about all deployed resources for +audits or compliance). +- **Mutating aspects** either (1.) add new nodes or (2.) mutate existing nodes of the tree in-place. One commonly used mutating Aspect is adding Tags to +resources. An example of an Aspect that adds a node is one that automatically adds a security group to every EC2 instance in the construct tree if +no default is specified. + +Here is a simple example of creating and applying an Aspect on a Stack to enable versioning on all S3 Buckets: + +```ts +import { IAspect, IConstruct, Tags, Stack } from 'aws-cdk-lib'; + +class EnableBucketVersioning implements IAspect { + visit(node: IConstruct) { + if (node instanceof CfnBucket) { + node.versioningConfiguration = { + status: 'Enabled' + }; + } + } +} + +const app = new App(); +const stack = new MyStack(app, 'MyStack'); + +// Apply the aspect to enable versioning on all S3 Buckets +Aspects.of(stack).add(new EnableBucketVersioning()); +``` + +### Aspect Stabilization + +The modern behavior is that Aspects automatically run on newly added nodes to the construct tree. This is controlled by the +flag `@aws-cdk/core:aspectStabilization`, which is default for new projects (since version 2.172.0). + +The old behavior of Aspects (without stabilization) was that Aspect invocation runs once on the entire construct +tree. This meant that nested Aspects (Aspects that create new Aspects) are not invoked and nodes created by Aspects at a higher level of the construct tree are not visited. + +To enable the stabilization behavior for older versions, use this feature by putting the following into your `cdk.context.json`: + +```json +{ + "@aws-cdk/core:aspectStabilization": true +} +``` + +### Aspect Priorities + +Users can specify the order in which Aspects are applied on a construct by using the optional priority parameter when applying an Aspect. Priority +values must be non-negative integers, where a higher number means the Aspect will be applied later, and a lower number means it will be applied sooner. + +By default, newly created nodes always inherit aspects. Priorities are mainly for ordering between mutating aspects on the construct tree. + +CDK provides standard priority values for mutating and readonly aspects to help ensure consistency across different construct libraries: + +```ts +/** + * Default Priority values for Aspects. + */ +export class AspectPriority { + /** + * Suggested priority for Aspects that mutate the construct tree. + */ + static readonly MUTATING: number = 200; + + /** + * Suggested priority for Aspects that only read the construct tree. + */ + static readonly READONLY: number = 1000; + + /** + * Default priority for Aspects that are applied without a priority. + */ + static readonly DEFAULT: number = 500; +} +``` + +If no priority is provided, the default value will be 500. This ensures that aspects without a specified priority run after mutating aspects but before +any readonly aspects. + +Correctly applying Aspects with priority values ensures that mutating aspects (such as adding tags or resources) run before validation aspects. This allows users to avoid misconfigurations and ensure that the final +construct tree is fully validated before being synthesized. + +### Applying Aspects with Priority + +```ts +import { Aspects, Stack, IAspect, Tags } from 'aws-cdk-lib'; +import { Bucket } from 'aws-cdk-lib/aws-s3'; + +class MyAspect implements IAspect { + visit(node: IConstruct) { + // Modifies a resource in some way + } +} + +class ValidationAspect implements IAspect { + visit(node: IConstruct) { + // Perform some readonly validation on the cosntruct tree + } +} + +const stack = new Stack(); + +Aspects.of(stack).add(new MyAspect(), { priority: AspectPriority.MUTATING } ); // Run first (mutating aspects) +Aspects.of(stack).add(new ValidationAspect(), { priority: AspectPriority.READONLY } ); // Run later (readonly aspects) +``` + +### Inspecting applied aspects and changing priorities + +We also give customers the ability to view all of their applied aspects and override the priority on these aspects. +The `AspectApplication` class represents an Aspect that is applied to a node of the construct tree with a priority. + +Users can access AspectApplications on a node by calling `applied` from the Aspects class as follows: + +```ts +const app = new App(); +const stack = new MyStack(app, 'MyStack'); + +Aspects.of(stack).add(new MyAspect()); + +let aspectApplications: AspectApplication[] = Aspects.of(root).applied; + +for (const aspectApplication of aspectApplications) { + // The aspect we are applying + console.log(aspectApplication.aspect); + // The construct we are applying the aspect to + console.log(aspectApplication.construct); + // The priority it was applied with + console.log(aspectApplication.priority); + + // Change the priority + aspectApplication.priority = 700; +} +``` + ### Acknowledging Warnings If you would like to run with `--strict` mode enabled (warnings will throw diff --git a/packages/aws-cdk-lib/core/lib/aspect.ts b/packages/aws-cdk-lib/core/lib/aspect.ts index 21610fcb99669..3ee6e97958a93 100644 --- a/packages/aws-cdk-lib/core/lib/aspect.ts +++ b/packages/aws-cdk-lib/core/lib/aspect.ts @@ -12,6 +12,39 @@ export interface IAspect { visit(node: IConstruct): void; } +/** + * Default Priority values for Aspects. + */ +export class AspectPriority { + /** + * Suggested priority for Aspects that mutate the construct tree. + */ + static readonly MUTATING: number = 200; + + /** + * Suggested priority for Aspects that only read the construct tree. + */ + static readonly READONLY: number = 1000; + + /** + * Default priority for Aspects that are applied without a priority. + */ + static readonly DEFAULT: number = 500; +} + +/** + * Options when Applying an Aspect. + */ +export interface AspectOptions { + /** + * The priority value to apply on an Aspect. + * Priority must be a non-negative integer. + * + * @default - AspectPriority.DEFAULT + */ + readonly priority?: number; +} + /** * Aspects can be applied to CDK tree scopes and can operate on the tree before * synthesis. @@ -24,7 +57,7 @@ export class Aspects { public static of(scope: IConstruct): Aspects { let aspects = (scope as any)[ASPECTS_SYMBOL]; if (!aspects) { - aspects = new Aspects(); + aspects = new Aspects(scope); Object.defineProperty(scope, ASPECTS_SYMBOL, { value: aspects, @@ -35,24 +68,83 @@ export class Aspects { return aspects; } - private readonly _aspects: IAspect[]; + private readonly _scope: IConstruct; + private readonly _appliedAspects: AspectApplication[]; - private constructor() { - this._aspects = []; + private constructor(scope: IConstruct) { + this._appliedAspects = []; + this._scope = scope; } /** * Adds an aspect to apply this scope before synthesis. * @param aspect The aspect to add. + * @param options Options to apply on this aspect. */ - public add(aspect: IAspect) { - this._aspects.push(aspect); + public add(aspect: IAspect, options?: AspectOptions) { + this._appliedAspects.push(new AspectApplication(this._scope, aspect, options?.priority ?? AspectPriority.DEFAULT)); } /** * The list of aspects which were directly applied on this scope. */ public get all(): IAspect[] { - return [...this._aspects]; + return this._appliedAspects.map(application => application.aspect); + } + + /** + * The list of aspects with priority which were directly applied on this scope. + * + * Also returns inherited Aspects of this node. + */ + public get applied(): AspectApplication[] { + return [...this._appliedAspects]; + } +} + +/** + * Object respresenting an Aspect application. Stores the Aspect, the pointer to the construct it was applied + * to, and the priority value of that Aspect. + */ +export class AspectApplication { + /** + * The construct that the Aspect was applied to. + */ + public readonly construct: IConstruct; + + /** + * The Aspect that was applied. + */ + public readonly aspect: IAspect; + + /** + * The priority value of this Aspect. Must be non-negative integer. + */ + private _priority: number; + + /** + * Initializes AspectApplication object + */ + public constructor(construct: IConstruct, aspect: IAspect, priority: number) { + this.construct = construct; + this.aspect = aspect; + this._priority = priority; + } + + /** + * Gets the priority value. + */ + public get priority(): number { + return this._priority; } -} \ No newline at end of file + + /** + * Sets the priority value. + */ + public set priority(priority: number) { + if (priority < 0) { + throw new Error('Priority must be a non-negative number'); + } + this._priority = priority; + } +} diff --git a/packages/aws-cdk-lib/core/lib/private/synthesis.ts b/packages/aws-cdk-lib/core/lib/private/synthesis.ts index 3526991880735..320f3cadf8eaf 100644 --- a/packages/aws-cdk-lib/core/lib/private/synthesis.ts +++ b/packages/aws-cdk-lib/core/lib/private/synthesis.ts @@ -8,7 +8,7 @@ import { CloudAssembly } from '../../../cx-api'; import * as cxapi from '../../../cx-api'; import { Annotations } from '../annotations'; import { App } from '../app'; -import { Aspects, IAspect } from '../aspect'; +import { AspectApplication, Aspects } from '../aspect'; import { FileSystem } from '../fs'; import { Stack } from '../stack'; import { ISynthesisSession } from '../stack-synthesizers/types'; @@ -37,7 +37,11 @@ export function synthesize(root: IConstruct, options: SynthesisOptions = { }): c // we start by calling "synth" on all nested assemblies (which will take care of all their children) synthNestedAssemblies(root, options); - invokeAspects(root); + if (options.aspectStabilization == true) { + invokeAspectsV2(root); + } else { + invokeAspects(root); + } injectMetadataResources(root); @@ -215,25 +219,30 @@ function synthNestedAssemblies(root: IConstruct, options: StageSynthesisOptions) * twice for the same construct. */ function invokeAspects(root: IConstruct) { - const invokedByPath: { [nodePath: string]: IAspect[] } = { }; + const invokedByPath: { [nodePath: string]: AspectApplication[] } = { }; let nestedAspectWarning = false; recurse(root, []); - function recurse(construct: IConstruct, inheritedAspects: IAspect[]) { + function recurse(construct: IConstruct, inheritedAspects: AspectApplication[]) { const node = construct.node; const aspects = Aspects.of(construct); - const allAspectsHere = [...inheritedAspects ?? [], ...aspects.all]; + + let localAspects = aspects.applied; + const allAspectsHere = sortAspectsByPriority(inheritedAspects, localAspects); + const nodeAspectsCount = aspects.all.length; - for (const aspect of allAspectsHere) { + for (const aspectApplication of allAspectsHere) { let invoked = invokedByPath[node.path]; if (!invoked) { invoked = invokedByPath[node.path] = []; } - if (invoked.includes(aspect)) { continue; } + if (invoked.some(invokedApp => invokedApp.aspect === aspectApplication.aspect)) { + continue; + } - aspect.visit(construct); + aspectApplication.aspect.visit(construct); // if an aspect was added to the node while invoking another aspect it will not be invoked, emit a warning // the `nestedAspectWarning` flag is used to prevent the warning from being emitted for every child @@ -243,7 +252,7 @@ function invokeAspects(root: IConstruct) { } // mark as invoked for this node - invoked.push(aspect); + invoked.push(aspectApplication); } for (const child of construct.node.children) { @@ -254,6 +263,97 @@ function invokeAspects(root: IConstruct) { } } +/** + * Invoke aspects V2 runs a stabilization loop and allows Aspects to invoke other Aspects. + * Runs if the feature flag '@aws-cdk/core:aspectStabilization' is enabled. + * + * Unlike the original function, this function does not emit a warning for ignored aspects, since this + * function invokes Aspects that are created by other Aspects. + * + * Throws an error if the function attempts to invoke an Aspect on a node that has a lower priority value + * than the most recently invoked Aspect on that node. + */ +function invokeAspectsV2(root: IConstruct) { + const invokedByPath: { [nodePath: string]: AspectApplication[] } = { }; + + recurse(root, []); + + for (let i = 0; i <= 100; i++) { + const didAnythingToTree = recurse(root, []); + + if (!didAnythingToTree) { + return; + } + } + + throw new Error('We have detected a possible infinite loop while invoking Aspects. Please check your Aspects and verify there is no configuration that would cause infinite Aspect or Node creation.'); + + function recurse(construct: IConstruct, inheritedAspects: AspectApplication[]): boolean { + const node = construct.node; + const aspects = Aspects.of(construct); + + let didSomething = false; + + let localAspects = aspects.applied; + const allAspectsHere = sortAspectsByPriority(inheritedAspects, localAspects); + + for (const aspectApplication of allAspectsHere) { + + let invoked = invokedByPath[node.path]; + if (!invoked) { + invoked = invokedByPath[node.path] = []; + } + + if (invoked.some(invokedApp => invokedApp.aspect === aspectApplication.aspect)) { + continue; + } + + // If the last invoked Aspect has a higher priority than the current one, throw an error: + const lastInvokedAspect = invoked[invoked.length - 1]; + if (lastInvokedAspect && lastInvokedAspect.priority > aspectApplication.priority) { + throw new Error( + `Cannot invoke Aspect ${aspectApplication.aspect.constructor.name} with priority ${aspectApplication.priority} on node ${node.path}: an Aspect ${lastInvokedAspect.aspect.constructor.name} with a lower priority (${lastInvokedAspect.priority}) was already invoked on this node.`, + ); + }; + + aspectApplication.aspect.visit(construct); + + didSomething = true; + + // mark as invoked for this node + invoked.push(aspectApplication); + } + + let childDidSomething = false; + for (const child of construct.node.children) { + if (!Stage.isStage(child)) { + childDidSomething = recurse(child, allAspectsHere) || childDidSomething; + } + } + return didSomething || childDidSomething; + } +} + +/** + * Given two lists of AspectApplications (inherited and locally defined), this function returns a list of + * AspectApplications ordered by priority. For Aspects of the same priority, inherited Aspects take precedence. + */ +function sortAspectsByPriority(inheritedAspects: AspectApplication[], localAspects: AspectApplication[]): AspectApplication[] { + const allAspects = [...inheritedAspects, ...localAspects].sort((a, b) => { + // Compare by priority first + if (a.priority !== b.priority) { + return a.priority - b.priority; // Ascending order by priority + } + // If priorities are equal, give preference to inheritedAspects + const isAInherited = inheritedAspects.includes(a); + const isBInherited = inheritedAspects.includes(b); + if (isAInherited && !isBInherited) return -1; // a comes first + if (!isAInherited && isBInherited) return 1; // b comes first + return 0; // Otherwise, maintain original order + }); + return allAspects; +} + /** * Find all stacks and add Metadata Resources to all of them * diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index c3f9acf4a0563..ff50cbb895944 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -1,5 +1,6 @@ import { IConstruct, Construct, Node } from 'constructs'; import { Environment } from './environment'; +import { FeatureFlags } from './feature-flags'; import { PermissionsBoundary } from './permissions-boundary'; import { synthesize } from './private/synthesis'; import { IPolicyValidationPluginBeta1 } from './validation'; @@ -224,6 +225,7 @@ export class Stage extends Construct { this.assembly = synthesize(this, { skipValidation: options.skipValidation, validateOnSynthesis: options.validateOnSynthesis, + aspectStabilization: options.aspectStabilization ?? FeatureFlags.of(this).isEnabled(cxapi.ASPECT_STABILIZATION) ?? false, }); newConstructPaths = this.listAllConstructPaths(this); this.constructPathsCache = newConstructPaths; @@ -318,4 +320,15 @@ export interface StageSynthesisOptions { * @default true */ readonly errorOnDuplicateSynth?: boolean; + + /** + * Whether or not run the stabilization loop while invoking Aspects. + * + * The stabilization loop runs multiple passes of the construct tree when invoking + * Aspects. Without the stabilization loop, Aspects that are created by other Aspects + * are not run and new nodes that are created at higher points on the construct tree by + * an Aspect will not inherit their parent aspects. + * @default false + */ + readonly aspectStabilization?: boolean; } diff --git a/packages/aws-cdk-lib/core/test/aspect.prop.test.ts b/packages/aws-cdk-lib/core/test/aspect.prop.test.ts new file mode 100644 index 0000000000000..25484c0a7e447 --- /dev/null +++ b/packages/aws-cdk-lib/core/test/aspect.prop.test.ts @@ -0,0 +1,733 @@ +import { Construct, IConstruct } from 'constructs'; +import * as fc from 'fast-check'; +import * as fs from 'fs-extra'; +import { App, AppProps, AspectApplication, Aspects, IAspect } from '../lib'; + +////////////////////////////////////////////////////////////////////// +// Tests + +describe('every aspect gets invoked exactly once', () => { + test('every aspect gets executed at most once on every construct', () => + fc.assert( + fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => { + afterSynth((testApp) => { + forEveryVisitPair(testApp.visitLog, (a, b) => { + if (sameConstruct(a, b) && sameAspect(a, b)) { + throw new Error(`Duplicate visit: ${a.index} and ${b.index}`); + } + }); + }, stabilizeAspects)(app); + }), + ), + ); + + test('all aspects that exist at the start of synthesis get invoked on all nodes in its scope at the start of synthesis', () => + fc.assert( + fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => { + const originalConstructsOnApp = app.cdkApp.node.findAll(); + const originalAspectApplications = getAllAspectApplications(originalConstructsOnApp); + afterSynth((testApp) => { + const visitsMap = getVisitsMap(testApp.visitLog); + + for (const aspectApplication of originalAspectApplications) { + // Check that each original AspectApplication also visited all child nodes of its original scope: + for (const construct of originalConstructsOnApp) { + if (ancestorOf(aspectApplication.construct, construct)) { + if (!visitsMap.get(construct)!.includes(aspectApplication.aspect)) { + throw new Error(`Aspect ${aspectApplication.aspect.constructor.name} applied on ${aspectApplication.construct.node.path} did not visit construct ${construct.node.path} in its original scope.`); + } + } + } + } + }, stabilizeAspects)(app); + }), + ), + ); + + test('with stabilization, every aspect applied on the tree eventually executes on all of its nodes in scope', () => + fc.assert( + fc.property(appWithAspects(), (app) => { + afterSynth((testApp) => { + + const allConstructsOnApp = testApp.cdkApp.node.findAll(); + const allAspectApplications = getAllAspectApplications(allConstructsOnApp); + const visitsMap = getVisitsMap(testApp.visitLog); + + for (const aspectApplication of allAspectApplications) { + // Check that each AspectApplication also visited all child nodes of its scope: + for (const construct of allConstructsOnApp) { + if (ancestorOf(aspectApplication.construct, construct)) { + if (!visitsMap.get(construct)!.includes(aspectApplication.aspect)) { + throw new Error(`Aspect ${aspectApplication.aspect.constructor.name} applied on ${aspectApplication.construct.node.path} did not visit construct ${construct.node.path} in its scope.`); + } + } + } + } + }, true)(app); + }), + ), + ); +}); + +test('inherited aspects get invoked before locally defined aspects, if both have the same priority', () => + fc.assert( + fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => { + afterSynth((testApp) => { + forEveryVisitPair(testApp.visitLog, (a, b) => { + if (!sameConstruct(a, b)) return; + + const aPrio = lowestAspectPrioFor(a.aspect, a.construct); + const bPrio = lowestAspectPrioFor(b.aspect, b.construct); + + if (!(aPrio == bPrio)) return; + + const aInherited = allAncestorAspects(a.construct).includes(a.aspect); + const bInherited = allAncestorAspects(b.construct).includes(b.aspect); + + if (!(aInherited == true && bInherited == false)) return; + + if (!(a.index < b.index)) { + throw new Error( + `Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`, + ); + } + }); + }, stabilizeAspects)(app); + }), + ), +); + +test('for every construct, lower priorities go before higher priorities', () => + fc.assert( + fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => { + afterSynth((testApp) => { + forEveryVisitPair(testApp.visitLog, (a, b) => { + if (!sameConstruct(a, b)) return; + + const aPrio = lowestAspectPrioFor(a.aspect, a.construct); + const bPrio = lowestAspectPrioFor(b.aspect, b.construct); + + if (!implies(aPrio < bPrio, a.index < b.index)) { + throw new Error( + `Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`, + ); + } + }); + }, stabilizeAspects)(app); + }), + ), +); + +test('for every construct, if a invokes before b that must mean it is of equal or lower priority', () => + fc.assert( + fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => { + afterSynth((testApp) => { + forEveryVisitPair(testApp.visitLog, (a, b) => { + if (!sameConstruct(a, b)) return; + + const aPrio = lowestAspectPrioFor(a.aspect, a.construct); + const bPrio = lowestAspectPrioFor(b.aspect, b.construct); + + if (!implies(a.index < b.index, aPrio <= bPrio)) { + throw new Error( + `Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`, + ); + } + }); + }, stabilizeAspects)(app); + }), + ), +); + +test('visitLog is nonempty', () => + fc.assert( + fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => { + afterSynth((testApp) => { + expect(testApp.visitLog).not.toEqual([]); + }, stabilizeAspects)(app); + }), + ), +); + +////////////////////////////////////////////////////////////////////// +// Test Helpers + +function afterSynth(block: (x: PrettyApp) => void, stabilizeAspects: boolean) { + return (app) => { + let asm; + try { + asm = app.cdkApp.synth({ aspectStabilization: stabilizeAspects }); + } catch (error) { + if (error.message.includes('Cannot invoke Aspect')) { + return; + } + throw error; + } + try { + block(app); + } finally { + fs.rmSync(asm.directory, { recursive: true, force: true }); + } + }; +} + +/** + * Implication operator, for readability + */ +function implies(a: boolean, b: boolean) { + return !a || b; +} + +interface AspectVisitWithIndex extends AspectVisit { + index: number; +} + +/** + * Check a property for every pair of visits in the log + * + * This is humongously inefficient at large scale, so we might need more clever + * algorithms to keep this tractable. + */ +function forEveryVisitPair(log: AspectVisitLog, block: (a: AspectVisitWithIndex, b: AspectVisitWithIndex) => void) { + for (let i = 0; i < log.length; i++) { + for (let j = 0; j < log.length; j++) { + if (i === j) { continue; } + + block({ ...log[i], index: i }, { ...log[j], index: j }); + } + } +} + +function arraysEqual(arr1: T[], arr2: T[]): boolean { + if (arr1.length !== arr2.length) { + return false; + } + + return arr1.every((value, index) => value === arr2[index]); +} + +function arraysEqualUnordered(arr1: T[], arr2: T[]): boolean { + return arr1.length === arr2.length && new Set(arr1).size === new Set(arr2).size; +} + +/** + * Given an AspectVisitLog, returns a map of Constructs with a list of all Aspects that + * visited the construct. + */ +function getVisitsMap(log: AspectVisitLog): Map { + const visitsMap = new Map(); + for (let i = 0; i < log.length; i++) { + const visit = log[i]; + if (!visitsMap.has(visit.construct)) { + visitsMap.set(visit.construct, []); + } + visitsMap.get(visit.construct)!.push(visit.aspect); + } + return visitsMap; +} + +/** + * Returns a list of all AspectApplications from a list of Constructs. + */ +function getAllAspectApplications(constructs: IConstruct[]): AspectApplication[] { + const aspectApplications: AspectApplication[] = []; + + constructs.forEach((construct) => { + aspectApplications.push(...Aspects.of(construct).applied); + }); + + return aspectApplications; +} + +function sameConstruct(a: AspectVisit, b: AspectVisit) { + return a.construct === b.construct; +} + +function sameAspect(a: AspectVisit, b: AspectVisit) { + return a.aspect === b.aspect; +} + +/** + * Returns whether `a` is an ancestor of `b` (or if they are the same construct) + */ +function ancestorOf(a: Construct, b: Construct) { + return b.node.path === a.node.path || b.node.path.startsWith(a.node.path + '/'); +} + +/** + * Returns the ancestors of `a`, including `a` itself + * + * The first element is `a` itself, and the last element is its root. + */ +function ancestors(a: Construct): IConstruct[] { + return a.node.scopes.reverse(); +} + +/** + * Returns all aspects of the given construct's ancestors (excluding its own locally defined aspects) + */ +function allAncestorAspects(a: IConstruct): IAspect[] { + const ancestorConstructs = ancestors(a); + + // Filter out the current node and get aspects of the ancestors + return ancestorConstructs + .slice(1) // Exclude the node itself + .flatMap((ancestor) => Aspects.of(ancestor).applied.map((aspectApplication) => aspectApplication.aspect)); +} + +/** + * Returns all aspect applications in scope for the given construct + */ +function allAspectApplicationsInScope(a: Construct): AspectApplication[] { + return ancestors(a).flatMap((c) => Aspects.of(c).applied); +} + +/** + * Return the lowest priority of Aspect `a` inside the given list of applications + */ +function lowestPriority(a: IAspect, as: AspectApplication[]): number | undefined { + const filtered = as.filter((x) => x.aspect === a); + filtered.sort((x, y) => x.priority - y.priority); + return filtered[0]?.priority; +} + +function lowestAspectPrioFor(a: IAspect, c: IConstruct) { + const ret = lowestPriority(a, allAspectApplicationsInScope(c)); + if (ret === undefined) { + throw new Error(`Got an invocation of ${a} on ${c} with no priority`); + } + return ret; +} + +////////////////////////////////////////////////////////////////////// +// Arbitraries + +function appWithAspects() { + return arbCdkAppFactory().chain(arbAddAspects).map(([a, l]) => { + return new PrettyApp(a, l); + }); +} + +/** + * Generate an arbitrary CDK app + * + * First builds a tree of factories and then applies the factories + * + * Returns a wrapper class for `App` with a `toString()` method, so + * that if we find a counterexample `fast-check` will pretty-print it nicely. + */ +function arbCdkAppFactory(props?: AppProps): fc.Arbitrary { + return arbConstructTreeFactory().map((fac) => () => { + const app = new App(props); + fac(app, 'First'); + return app; + }); +} + +/** + * Produce an arbitrary construct tree + */ +function arbConstructTreeFactory(): fc.Arbitrary { + const { tree } = fc.letrec((rec) => { + return { + tree: fc.oneof({ depthSize: 'small', withCrossShrink: true }, rec('leaf'), rec('node')) as fc.Arbitrary, + leaf: fc.constant(constructFactory({})), + node: fc.tuple( + fc.array(fc.tuple(identifierString(), rec('tree') as fc.Arbitrary), { maxLength: 5 }), + ).map(([children]) => { + const c = Object.fromEntries(children); + return constructFactory(c); + }), + }; + }); + return tree; +} + +/** + * A class to pretty print a CDK app if a property test fails, so it becomes readable. + * + * Also holds the aspect visit log because why not. + */ +class PrettyApp { + private readonly initialTree: Set; + + constructor(public readonly cdkApp: App, public readonly executionState: ExecutionState) { + this.initialTree = new Set(cdkApp.node.findAll().map(c => c.node.path)); + } + + /** + * Return the log of all aspect visits + */ + public get visitLog() { + return this.executionState.visitLog; + } + + /** + * Return a list of all constructs added by aspects + */ + public get addedConstructs() { + return this.executionState.addedConstructs; + } + + /** + * Return a list of all aspects added by other aspects + */ + public get addedAspects() { + return this.executionState.addedAspects; + } + + public toString() { + const self = this; + + const lines: string[] = []; + const prefixes: string[] = []; + + // Add some empty lines to render better in the fast-check error message + lines.push('', ''); + + prefixes.push(' '); + lines.push('TREE'); + recurse(this.cdkApp); + lines.push('VISITS'); + this.visitLog.forEach((visit, i) => { + lines.push(` ${i}. ${visit.construct} <-- ${visit.aspect}`); + }); + + lines.push('', ''); + + return lines.join('\n'); + + function line(x: string) { + lines.push(`${prefixes.join('')}${x}`); + } + + function recurse(construct: Construct) { + const aspects = Aspects.of(construct).applied.map(a => `${a.aspect}@${a.priority}`); + line([ + '+-', + ...self.initialTree.has(construct.node.path) ? [] : ['(added)'], + construct.node.id || '(root)', + ...aspects.length > 0 ? [` <-- ${aspects.join(', ')}`] : [], + ].join(' ')); + + prefixes.push(' '); + construct.node.children.forEach((child, i) => { + recurse(child); + }); + prefixes.pop(); + } + } +} + +interface AspectVisit { + construct: IConstruct; + aspect: TracingAspect; +} + +type AspectVisitLog = AspectVisit[]; + +/** + * Add arbitrary aspects to the given tree + */ +function arbAddAspects(appFac: AppFactory): fc.Arbitrary<[App, ExecutionState]> { + // Synthesize the tree, but just to get the construct paths to apply aspects to. + // We won't hold on to the instances, because we will clone the tree later (or + // regenerate it, which is easier), and attach the aspects in the clone. + const baseTree = appFac(); + const constructs = baseTree.node.findAll(); + + const applications = fc.array(arbAspectApplication(constructs), { + size: 'small', + minLength: 1, + maxLength: 5, + }); + + return applications.map((appls) => { + // A fresh tree copy for every tree with aspects. `fast-check` may re-use old values + // when generating variants, so if we mutate the tree in place different runs will + // interfere with each other. Also a different aspect invocation log for every tree. + const tree = appFac(); + const state: ExecutionState = { + visitLog: [], + addedConstructs: [], + addedAspects: [], + }; + tree[EXECUTIONSTATE_SYM] = state; // Stick this somewhere the aspects can find it + + for (const app of appls) { + const ctrs = app.constructPaths.map((p) => findConstructDeep(tree, p)); + for (const ctr of ctrs) { + Aspects.of(ctr).add(app.aspect, { priority: app.priority }); + } + } + + return [tree, state]; + }); +} + +function arbAspectApplication(constructs: Construct[]): fc.Arbitrary { + return fc.record({ + constructPaths: fc.shuffledSubarray(constructs, { minLength: 1, maxLength: Math.min(3, constructs.length) }) + .map((cs) => cs.map((c) => c.node.path)), + aspect: arbAspect(constructs), + priority: fc.nat({ max: 1000 }), + }); +} + +function arbAspect(constructs: Construct[]): fc.Arbitrary { + return (fc.oneof( + { + depthIdentifier: 'aspects', + }, + // Simple: inspecting aspect + fc.constant(() => fc.constant(new InspectingAspect())), + // Simple: mutating aspect + fc.constant(() => fc.constant(new MutatingAspect())), + // More complex: adds a new construct, optionally immediately adds an aspect to it + fc.constant(() => fc.record({ + constructLoc: arbConstructLoc(constructs), + newAspects: fc.array(arbAspectApplication(constructs), { size: '-1', maxLength: 2 }), + }).map(({ constructLoc, newAspects }) => { + return new NodeAddingAspect(constructLoc, newAspects); + })), + // More complex: adds a new aspect to an existing construct. + // NOTE: this will never add an aspect to a construct that didn't exist in the initial tree. + fc.constant(() => arbAspectApplication(constructs).map(((aspectApp) => + new AspectAddingAspect(aspectApp) + ))), + ) satisfies fc.Arbitrary<() => fc.Arbitrary>).chain((fact) => fact()); +} + +function arbConstructLoc(constructs: Construct[]): fc.Arbitrary { + return fc.record({ + scope: fc.constantFrom(...constructs.map(c => c.node.path)), + id: identifierString(), + }); +} + +/** + * A location for a construct (scope and id) + */ +interface ConstructLoc { + scope: string; + id: string; +} + +/** + * fast-check is fully value-based, but Constructs must be added to their parent at construction time + * + * Instead of fast-check returning constructs, we will have it return construct FACTORIES, + * which are functions that are given a parent and add the construct to it. + * + * We also have the parent control the `id`, so that we can avoid accidentally generating constructs + * with conflicting names. + */ +type ConstructFactory = (scope: Construct, id: string) => Construct; + +type AppFactory = () => App; + +interface ExecutionState { + /** + * Visit log of all aspects + */ + visitLog: AspectVisitLog; + + /** + * Constructs that were added by aspects + */ + addedConstructs: Construct[]; + + /** + * Record where we added an aspect + */ + addedAspects: RecordedAspectApplication[]; +} + +/** + * A unique symbol for the root construct to hold dynamic information + */ +const EXECUTIONSTATE_SYM = Symbol(); + +function findConstructDeep(root: IConstruct, path: string) { + if (path === '') { + return root; + } + const parts = path.split('/'); + let ctr: IConstruct = root; + for (const part of parts) { + ctr = ctr.node.findChild(part); + } + return ctr; +} + +function identifierString() { + return fc.string({ minLength: 1, maxLength: 3, unit: fc.constantFrom('Da', 'Fu', 'Glo', 'Ba', 'Ro', 'ni', 'za', 'go', 'moo', 'flub', 'bu', 'vin', 'e', 'be') }); +} + +/** + * Create a construct factory + */ +function constructFactory(childGenerators: Record): ConstructFactory { + return (scope, id) => { + const construct = new ArbConstruct(scope, id); + for (const [childId, generator] of Object.entries(childGenerators)) { + generator(construct, childId); + } + return construct; + }; +} + +/** + * A construct class specifically for this test to distinguish it from other constructs + */ +class ArbConstruct extends Construct { + /** + * The state of a construct. + * + * This is an arbitrary number that can be modified by aspects. + */ + public state: number = 0; + + public toString() { + return this.node.path; + } +} + +////////////////////////////////////////////////////////////////////// +// Aspects + +let UUID = 1000; + +/** + * Implementor of Aspect that logs its actions + * + * All subclasses should call `super.visit()`. + */ +abstract class TracingAspect implements IAspect { + public readonly id: number; + + constructor() { + this.id = UUID++; + } + + protected executionState(node: IConstruct): ExecutionState { + return node.node.root[EXECUTIONSTATE_SYM]; + } + + visit(node: IConstruct): void { + this.executionState(node).visitLog.push({ + aspect: this, + construct: node, + }); + } +} + +/** + * An inspecting aspect doesn't really do anything + */ +class InspectingAspect extends TracingAspect { + public toString() { + return `Inspect_${this.id}`; + } +} + +/** + * An aspect that increases the 'state' number of a construct + */ +class MutatingAspect extends TracingAspect { + visit(node: IConstruct): void { + super.visit(node); + if (node instanceof ArbConstruct) { + node.state++; + } + } + + public toString() { + return `Mutate_${this.id}`; + } +} + +/** + * Partial Aspect application + * + * Contains just the aspect and priority + */ +interface PartialTestAspectApplication { + aspect: IAspect; + priority?: number; +} + +interface TestAspectApplication extends PartialTestAspectApplication { + /** + * Need to go by path because the constructs themselves are mutable and these paths remain valid in multiple trees + */ + constructPaths: string[]; +} + +/** + * An aspect application added by another aspect, during execution + */ +interface RecordedAspectApplication extends PartialTestAspectApplication { + construct: Construct; +} + +/** + * An aspect that adds a new node, if one doesn't exist yet + */ +class NodeAddingAspect extends TracingAspect { + constructor(private readonly loc: ConstructLoc, private readonly newAspects: PartialTestAspectApplication[]) { + super(); + } + + visit(node: IConstruct): void { + super.visit(node); + const scope = findConstructDeep(node.node.root, this.loc.scope); + + if (scope.node.tryFindChild(this.loc.id)) { + return; + } + + const newConstruct = new ArbConstruct(scope, this.loc.id); + for (const { aspect, priority } of this.newAspects) { + Aspects.of(newConstruct).add(aspect, { priority }); + this.executionState(node).addedAspects.push({ + construct: newConstruct, + aspect, + priority, + }); + } + // Log that we added this construct + this.executionState(node).addedConstructs.push(newConstruct); + } + + public toString() { + const childId = `${this.loc.scope}/${this.loc.id}`; + const newAspects = this.newAspects.map((a) => `${a.aspect}@${a.priority}`); + + return `AddConstruct_${this.id}(${childId}, [${newAspects.join('\n')}])`; + } +} + +class AspectAddingAspect extends TracingAspect { + constructor(private readonly newAspect: TestAspectApplication) { + super(); + } + + visit(node: IConstruct): void { + super.visit(node); + + const constructs = this.newAspect.constructPaths.map((p) => findConstructDeep(node.node.root, p)); + for (const construct of constructs) { + Aspects.of(construct).add(this.newAspect.aspect, { priority: this.newAspect.priority }); + this.executionState(node).addedAspects.push({ + construct, + aspect: this.newAspect.aspect, + priority: this.newAspect.priority, + }); + } + } + + public toString() { + return `AddAspect_${this.id}([${this.newAspect.constructPaths.join(',')}], ${this.newAspect.aspect}@${this.newAspect.priority})`; + } +} \ No newline at end of file diff --git a/packages/aws-cdk-lib/core/test/aspect.test.ts b/packages/aws-cdk-lib/core/test/aspect.test.ts index c050f4be0ccaa..f27837a9f7c62 100644 --- a/packages/aws-cdk-lib/core/test/aspect.test.ts +++ b/packages/aws-cdk-lib/core/test/aspect.test.ts @@ -1,8 +1,9 @@ import { Construct, IConstruct } from 'constructs'; +import { Template } from '../../assertions'; +import { Bucket, CfnBucket } from '../../aws-s3'; import * as cxschema from '../../cloud-assembly-schema'; -import { App } from '../lib'; -import { IAspect, Aspects } from '../lib/aspect'; - +import { App, CfnResource, Stack, Tag, Tags } from '../lib'; +import { IAspect, Aspects, AspectPriority } from '../lib/aspect'; class MyConstruct extends Construct { public static IsMyConstruct(x: any): x is MyConstruct { return x.visitCounter !== undefined; @@ -24,6 +25,44 @@ class MyAspect implements IAspect { } } +class EnableBucketVersioningAspect implements IAspect { + public visit(node: IConstruct): void { + if (node instanceof CfnBucket) { + node.versioningConfiguration = { + status: 'Enabled', + }; + } + } +} + +class AddLoggingBucketAspect implements IAspect { + private processed = false; + public visit(node: IConstruct): void { + // Check if the node is an S3 Bucket + if (node instanceof CfnBucket && !this.processed) { + // Add a new logging bucket Bucket to the stack this bucket belongs to. + const stack = Stack.of(node); + new Bucket(stack, 'my-new-logging-bucket-from-aspect', { + bucketName: 'my-new-logging-bucket-from-aspect', + }); + this.processed = true; + } + } +} + +class AddSingletonBucketAspect implements IAspect { + private processed = false; + public visit(node: IConstruct): void { + if (Stack.isStack(node) && !this.processed) { + // Add a new logging bucket Bucket to the stack this bucket belongs to. + new Bucket(node, 'my-new-logging-bucket-from-aspect', { + bucketName: 'my-new-logging-bucket-from-aspect', + }); + this.processed = true; + } + } +} + describe('aspect', () => { test('Aspects are invoked only once', () => { const app = new App(); @@ -35,8 +74,8 @@ describe('aspect', () => { expect(root.visitCounter).toEqual(1); }); - test('Warn if an Aspect is added via another Aspect', () => { - const app = new App(); + test('if stabilization is disabled, warn if an Aspect is added via another Aspect', () => { + const app = new App({ context: { '@aws-cdk/core:aspectStabilization': false } }); const root = new MyConstruct(app, 'MyConstruct'); const child = new MyConstruct(root, 'ChildConstruct'); Aspects.of(root).add({ @@ -69,4 +108,205 @@ describe('aspect', () => { expect(root.node.metadata.length).toEqual(1); expect(child.node.metadata.length).toEqual(1); }); + + test('Aspects applied without priority get the default priority value', () => { + const app = new App(); + const root = new MyConstruct(app, 'Construct'); + const child = new MyConstruct(root, 'ChildConstruct'); + + // WHEN - adding an Aspect without priority specified + Aspects.of(root).add(new MyAspect()); + + // THEN - the priority is set to default + let aspectApplication = Aspects.of(root).applied[0]; + expect(aspectApplication.priority).toEqual(AspectPriority.DEFAULT); + }); + + test('Can override Aspect priority', () => { + const app = new App(); + const root = new MyConstruct(app, 'Construct'); + const child = new MyConstruct(root, 'ChildConstruct'); + + // WHEN - adding an Aspect without priority specified and resetting it. + Aspects.of(root).add(new MyAspect()); + let aspectApplication = Aspects.of(root).applied[0]; + + // THEN - we can reset the priority of an Aspect + aspectApplication.priority = 0; + expect(Aspects.of(root).applied[0].priority).toEqual(0); + }); + + test('In-place mutating Aspect gets applied', () => { + const app = new App({ context: { '@aws-cdk/core:aspectStabilization': true } }); + const stack = new Stack(app, 'My-Stack'); + + // GIVEN - Bucket with versioning disabled + const bucket = new Bucket(stack, 'my-bucket', { + versioned: false, + }); + + // WHEN - adding the Aspect to enable bucket versioning gets applied: + Aspects.of(stack).add(new EnableBucketVersioningAspect()); + + // THEN - Aspect is successfully applied + Template.fromStack(stack).hasResourceProperties('AWS::S3::Bucket', { + VersioningConfiguration: { + Status: 'Enabled', + }, + }); + }); + + test('mutating Aspect that creates a node gets applied', () => { + const app = new App({ context: { '@aws-cdk/core:aspectStabilization': true } }); + const stack = new Stack(app, 'My-Stack'); + + // GIVEN - Bucket with versioning disabled + const bucket = new Bucket(stack, 'my-bucket', { + bucketName: 'my-original-bucket', + versioned: false, + }); + + // WHEN - adding the Aspect to add a logging bucket: + Aspects.of(stack).add(new AddLoggingBucketAspect()); + + // THEN - Aspect is successfully applied, new logging bucket is added + Template.fromStack(stack).hasResourceProperties('AWS::S3::Bucket', { + BucketName: 'my-new-logging-bucket-from-aspect', + }); + }); + + test('can set mutating Aspects in specified orcder and visit newly created node', () => { + const app = new App({ context: { '@aws-cdk/core:aspectStabilization': true } }); + const stack = new Stack(app, 'My-Stack'); + + // GIVEN - Bucket with versioning disabled + const bucket = new Bucket(stack, 'my-bucket', { + bucketName: 'my-original-bucket', + versioned: false, + }); + + // WHEN - adding both Aspects but making LoggingBucket Aspect run first + Aspects.of(stack).add(new AddLoggingBucketAspect(), { priority: 90 }); + Aspects.of(stack).add(new EnableBucketVersioningAspect(), { priority: 100 }); + + // THEN - both Aspects are successfully applied, new logging bucket is added with versioning enabled + Template.fromStack(stack).hasResourceProperties('AWS::S3::Bucket', { + BucketName: 'my-new-logging-bucket-from-aspect', + VersioningConfiguration: { + Status: 'Enabled', + }, + }); + }); + + test('Tags are applied to newly created node from the LoggingBucket Aspect', () => { + const app = new App({ context: { '@aws-cdk/core:aspectStabilization': true } }); + const stack = new Stack(app, 'My-Stack'); + + // GIVEN - Bucket with versioning disabled + const bucket = new Bucket(stack, 'my-bucket', { + bucketName: 'my-original-bucket', + versioned: false, + }); + + // WHEN - adding both Aspects but making LoggingBucket Aspect run first + Aspects.of(stack).add(new AddLoggingBucketAspect(), { priority: 0 }); + Tags.of(stack).add('TestKey', 'TestValue'); + + // THEN - check that Tags Aspect is applied to stack with default priority + let aspectApplications = Aspects.of(stack).applied; + expect(aspectApplications.length).toEqual(2); + expect(aspectApplications[1].priority).toEqual(AspectPriority.DEFAULT); + + // THEN - both Aspects are successfully applied, new logging bucket is added with versioning enabled + Template.fromStack(stack).hasResourceProperties('AWS::S3::Bucket', { + BucketName: 'my-new-logging-bucket-from-aspect', + Tags: [{ + Key: 'TestKey', + Value: 'TestValue', + }], + }); + }); + + test('Newly created node inherits Aspect that was already invoked on its parent node.', () => { + const app = new App({ context: { '@aws-cdk/core:aspectStabilization': true } }); + const root = new Stack(app, 'My-Stack'); + + // GIVEN - 3 Aspects with increasing priority + Aspects.of(root).add(new Tag('AspectA', 'Visited'), { priority: 10 }); + // This Aspect is applied after Aspect A, but creates a child of the root which should still + // inherit Aspect A. + Aspects.of(root).add(new AddSingletonBucketAspect(), { priority: 20 }); + Aspects.of(root).add(new Tag('AspectC', 'Visited'), { priority: 30 }); + + // THEN - both Aspects A and C are invoked on the new node created by Aspect B as a child of the root. + Template.fromStack(root).hasResourceProperties('AWS::S3::Bucket', { + BucketName: 'my-new-logging-bucket-from-aspect', + Tags: [{ + Key: 'AspectA', + Value: 'Visited', + }, + { + Key: 'AspectC', + Value: 'Visited', + }], + }); + }); + + class WeirdAspect implements IAspect { + private processed = false; + public visit(node: IConstruct): void { + // Check if the node is an S3 Bucket + if (node instanceof CfnBucket && !this.processed) { + const stack = Stack.of(node); + + // Adds a new Tag to the parent of this bucket + Aspects.of(stack).add(new Tag('AspectB', 'Visited'), { priority: 0 }); + + this.processed = true; + } + } + } + + test('Error is thrown if Aspect adds an Aspect to a node with a lower priority than the last invoked Aspect of a node.', () => { + const app = new App({ context: { '@aws-cdk/core:aspectStabilization': true } }); + const root = new Stack(app, 'My-Stack'); + const child = new Bucket(root, 'my-bucket', { + bucketName: 'my-bucket', + }); + + // GIVEN - 2 Aspects where the second one applied an Aspect with a lower priority to the root: + Aspects.of(root).add(new Tag('AspectA', 'Visited'), { priority: 10 }); + Aspects.of(child).add(new WeirdAspect(), { priority: 20 }); + + expect(() => { + app.synth(); + }).toThrow('Cannot invoke Aspect Tag with priority 0 on node My-Stack: an Aspect Tag with a lower priority (10) was already invoked on this node.'); + }); + + test('Infinitely recursing Aspect is caught with error', () => { + const app = new App({ context: { '@aws-cdk/core:aspectStabilization': true } }); + const root = new Stack(app, 'My-Stack'); + const child = new MyConstruct(root, 'MyConstruct'); + + Aspects.of(root).add(new InfiniteAspect()); + + expect(() => { + app.synth(); + }).toThrow('We have detected a possible infinite loop while invoking Aspects.'); + }); + + class InfiniteAspect implements IAspect { + private static counter = 0; + + visit(node: IConstruct): void { + // Adds a new resource as a child of the stack. + if (!(node instanceof Stack)) { + const stack = Stack.of(node); + const uniqueId = `InfiniteResource-${InfiniteAspect.counter++}`; + new CfnResource(stack, uniqueId, { + type: 'AWS::CDK::Broken', + }); + } + } + } }); diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index 9bc3d0d5a8977..b80d31117ec7c 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/packages/aws-cdk-lib/cx-api/lib/features.ts @@ -115,6 +115,7 @@ export const USE_CORRECT_VALUE_FOR_INSTANCE_RESOURCE_ID_PROPERTY = '@aws-cdk/aws export const CFN_INCLUDE_REJECT_COMPLEX_RESOURCE_UPDATE_CREATE_POLICY_INTRINSICS = '@aws-cdk/core:cfnIncludeRejectComplexResourceUpdateCreatePolicyIntrinsics'; export const LAMBDA_NODEJS_SDK_V3_EXCLUDE_SMITHY_PACKAGES = '@aws-cdk/aws-lambda-nodejs:sdkV3ExcludeSmithyPackages'; export const STEPFUNCTIONS_TASKS_FIX_RUN_ECS_TASK_POLICY = '@aws-cdk/aws-stepfunctions-tasks:fixRunEcsTaskPolicy'; +export const ASPECT_STABILIZATION = '@aws-cdk/core:aspectStabilization'; export const FLAGS: Record = { ////////////////////////////////////////////////////////////////////// @@ -1250,6 +1251,21 @@ export const FLAGS: Record = { introducedIn: { v2: '2.163.0' }, recommendedValue: true, }, + + ////////////////////////////////////////////////////////////////////// + [ASPECT_STABILIZATION]: { + type: FlagType.VisibleContext, + summary: 'When enabled, a stabilization loop will be run when invoking Aspects during synthesis.', + detailsMd: ` + Currently, when Aspects are invoked in one single pass of the construct tree. + This means that the Aspects that create other Aspects are not run and Aspects that create new nodes of the tree sometimes do not inherit their parent Aspects. + + When this feature flag is enabled, a stabilization loop is run to recurse the construct tree multiple times when invoking Aspects. + `, + defaults: { v2: true }, + introducedIn: { v2: 'V2NEXT' }, + recommendedValue: true, + }, }; const CURRENT_MV = 'v2'; diff --git a/packages/aws-cdk-lib/cx-api/test/features.test.ts b/packages/aws-cdk-lib/cx-api/test/features.test.ts index bfd64873122ec..67d3a8ee66183 100644 --- a/packages/aws-cdk-lib/cx-api/test/features.test.ts +++ b/packages/aws-cdk-lib/cx-api/test/features.test.ts @@ -40,6 +40,7 @@ test('feature flag defaults may not be changed anymore', () => { [feats.PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE]: true, [feats.USE_NEW_S3URI_PARAMETERS_FOR_BEDROCK_INVOKE_MODEL_TASK]: true, // Add new disabling feature flags below this line + [feats.ASPECT_STABILIZATION]: true, }); });