fix(core): implicit Aspect applications do not override custom Aspect applications (#34132)

Some CDK methods apply mutating Aspects on behalf of users. Since #32333, these Aspects have a priority of `MUTATING` to classify their behavior.

If a user-applied Aspect (priority `DEFAULT`) now configures the same property as an implicitly added Aspect:

* Before that change, the relative execution order depended on the location of the Aspects in the construct tree.
* After that change, the user Aspect always "wins" (executes last) because its priority is higher.

In this change, we roll back to the behavior from pre-2.172.0, and introduce a feature flag which gives the Aspects a priority only if the feature flag is enabled. This introduces the feature flag:

```json
{
  "context": {
    "@aws-cdk/core:aspectPrioritiesMutating": true
  }
}
```

Which sets the priority of Aspects added on your behalf a priority of `MUTATING` (200) (instead of the default `DEFAULT`, 500).

* If you have given your own Aspect a priority of `MUTATING` already to make sure it can get overridden by another Aspect  of priority `MUTATING`, this current change will not affect you (either with or without feature flag).
* If you have come to rely on the new default priority being low already, you can set the above feature flag to re-enable the new behavior.

-----------


Did not touch the following Aspects:

- In `integ-tests-alpha`: overriding logical IDs in assertions stacks does not affect production infrastructure.
- Tags: tags are exclusively manipulated through the official APIs, so there no conflict between custom and implicit Aspects.
- CDK Pipelines: there cannot be a conflict because the customer can't create a default pipeline before the implicit Aspect.

This PR also introduces some slight rendering and documentation changes to the feature flags to improve clarity of the purpose of certain fields and the produced report.

Author: rix0rrr

Diff for: packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-ecr-image-scan-action.js.snapshot/asset.12157b27d30ab71eb24ae65825f672ba5cc2c09dbb1703cd7adfcff3aeaca136.zip

@@ -1,4 +1,5 @@
 import * as cdk from 'aws-cdk-lib/core';
+import * as cxapi from 'aws-cdk-lib/cx-api';
 import { Construct } from 'constructs';
 import { IApplication } from './application';
 import { CheckedStageStackAssociator } from './aspects/stack-associator';
@@ -50,7 +51,9 @@ export class ApplicationAssociator extends Construct {
     this.associateCrossAccountStacks = targetBindResult.associateCrossAccountStacks;
     cdk.Aspects.of(scope).add(new CheckedStageStackAssociator(this, {
       associateCrossAccountStacks: this.associateCrossAccountStacks,
-    }), { priority: cdk.AspectPriority.MUTATING });
+    }), {
+      priority: cdk.FeatureFlags.of(this).isEnabled(cxapi.ASPECT_PRIORITIES_MUTATING) ? cdk.AspectPriority.MUTATING : undefined,
+    });
   }
 
   /**
@@ -61,7 +64,9 @@ export class ApplicationAssociator extends Construct {
     this.associatedStages.add(stage);
     cdk.Aspects.of(stage).add(new CheckedStageStackAssociator(this, {
       associateCrossAccountStacks: this.associateCrossAccountStacks,
-    }), { priority: cdk.AspectPriority.MUTATING });
+    }), {
+      priority: cdk.FeatureFlags.of(this).isEnabled(cxapi.ASPECT_PRIORITIES_MUTATING) ? cdk.AspectPriority.MUTATING : undefined,
+    });
     return stage;
   }
 

Diff for: packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-ecr-image-scan-action.js.snapshot/asset.6a9a0507c3e2bfac9749f257bdd3d9f313c01ee3984ad7bf373597c341863d6f.zip

@@ -17,7 +17,6 @@ import * as iam from '../../aws-iam';
 import * as sns from '../../aws-sns';
 import {
   Annotations,
-  AspectPriority,
   Aspects,
   Aws,
   CfnAutoScalingRollingUpdate, CfnCreationPolicy, CfnUpdatePolicy,
@@ -26,6 +25,7 @@ import {
   Tokenization, UnscopedValidationError, ValidationError, withResolved,
 } from '../../core';
 import { addConstructMetadata, MethodMetadata } from '../../core/lib/metadata-resource';
+import { mutatingAspectPrio32333 } from '../../core/lib/private/aspect-prio';
 import { AUTOSCALING_GENERATE_LAUNCH_TEMPLATE } from '../../cx-api';
 
 /**
@@ -1608,7 +1608,9 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
     this.spotPrice = props.spotPrice;
 
     if (props.requireImdsv2) {
-      Aspects.of(this).add(new AutoScalingGroupRequireImdsv2Aspect(), { priority: AspectPriority.MUTATING });
+      Aspects.of(this).add(new AutoScalingGroupRequireImdsv2Aspect(), {
+        priority: mutatingAspectPrio32333(this),
+      });
     }
 
     this.node.addValidation({ validate: () => this.validateTargetGroup() });

Diff for: packages/@aws-cdk-testing/framework-integ/test/aws-eks/test/integ.eks-cluster-tags.js.snapshot/asset.12157b27d30ab71eb24ae65825f672ba5cc2c09dbb1703cd7adfcff3aeaca136.zip

@@ -4,8 +4,9 @@ import { BackupableResourcesCollector } from './backupable-resources-collector';
 import { IBackupPlan } from './plan';
 import { BackupResource, TagOperation } from './resource';
 import * as iam from '../../aws-iam';
-import { Lazy, Resource, Aspects, AspectPriority } from '../../core';
+import { Lazy, Resource, Aspects } from '../../core';
 import { addConstructMetadata } from '../../core/lib/metadata-resource';
+import { mutatingAspectPrio32333 } from '../../core/lib/private/aspect-prio';
 
 /**
  * Options for a BackupSelection
@@ -143,7 +144,9 @@ export class BackupSelection extends Resource implements iam.IGrantable {
     }
 
     if (resource.construct) {
-      Aspects.of(resource.construct).add(this.backupableResourcesCollector, { priority: AspectPriority.MUTATING });
+      Aspects.of(resource.construct).add(this.backupableResourcesCollector, {
+        priority: mutatingAspectPrio32333(resource.construct),
+      });
       // Cannot push `this.backupableResourcesCollector.resources` to
       // `this.resources` here because it has not been evaluated yet.
       // Will be concatenated to `this.resources` in a `Lazy.list`

Diff for: packages/@aws-cdk-testing/framework-integ/test/aws-eks/test/integ.eks-cluster-tags.js.snapshot/asset.93d96d34e0d3cd20eb082652b91012b131bdc34fcf2bc16eb4170e04772fddb1.zip

@@ -15,9 +15,10 @@ import { UserData } from './user-data';
 import { BlockDevice } from './volume';
 import { IVpc, Subnet, SubnetSelection } from './vpc';
 import * as iam from '../../aws-iam';
-import { Annotations, AspectPriority, Aspects, Duration, FeatureFlags, Fn, IResource, Lazy, Resource, Stack, Tags, Token } from '../../core';
+import { Annotations, Aspects, Duration, FeatureFlags, Fn, IResource, Lazy, Resource, Stack, Tags, Token } from '../../core';
 import { md5hash } from '../../core/lib/helpers-internal';
 import { addConstructMetadata, MethodMetadata } from '../../core/lib/metadata-resource';
+import { mutatingAspectPrio32333 } from '../../core/lib/private/aspect-prio';
 import * as cxapi from '../../cx-api';
 
 /**
@@ -671,7 +672,9 @@ export class Instance extends Resource implements IInstance {
     }));
 
     if (props.requireImdsv2) {
-      Aspects.of(this).add(new InstanceRequireImdsv2Aspect(), { priority: AspectPriority.MUTATING });
+      Aspects.of(this).add(new InstanceRequireImdsv2Aspect(), {
+        priority: mutatingAspectPrio32333(this),
+      });
     }
   }
 

Diff for: packages/@aws-cdk/aws-servicecatalogappregistry-alpha/lib/application-associator.ts

@@ -24,10 +24,10 @@ import {
   IAspect,
   Token,
   Names,
-  AspectPriority,
   FeatureFlags, Annotations,
 } from '../../core';
 import { addConstructMetadata, MethodMetadata } from '../../core/lib/metadata-resource';
+import { mutatingAspectPrio32333 } from '../../core/lib/private/aspect-prio';
 import { Disable_ECS_IMDS_Blocking, Enable_IMDS_Blocking_Deprecated_Feature } from '../../cx-api';
 
 const CLUSTER_SYMBOL = Symbol.for('@aws-cdk/aws-ecs/lib/cluster.Cluster');
@@ -331,7 +331,9 @@ export class Cluster extends Resource implements ICluster {
     // since it's harmless, but we'd prefer not to add unexpected new
     // resources to the stack which could surprise users working with
     // brown-field CDK apps and stacks.
-    Aspects.of(this).add(new MaybeCreateCapacityProviderAssociations(this, id), { priority: AspectPriority.MUTATING });
+    Aspects.of(this).add(new MaybeCreateCapacityProviderAssociations(this, id), {
+      priority: mutatingAspectPrio32333(this),
+    });
   }
 
   /**

Diff for: packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

@@ -1,7 +1,8 @@
 import { IConstruct } from 'constructs';
 import { CfnRole, CfnUser } from './iam.generated';
 import { IManagedPolicy } from './managed-policy';
-import { AspectPriority, Aspects, CfnResource } from '../../core';
+import { Aspects, CfnResource } from '../../core';
+import { mutatingAspectPrio32333 } from '../../core/lib/private/aspect-prio';
 
 /**
  * Modify the Permissions Boundaries of Users and Roles in a construct tree
@@ -40,7 +41,9 @@ export class PermissionsBoundary {
           node.addPropertyOverride('PermissionsBoundary', boundaryPolicy.managedPolicyArn);
         }
       },
-    }, { priority: AspectPriority.MUTATING });
+    }, {
+      priority: mutatingAspectPrio32333(this.scope),
+    });
   }
 
   /**
@@ -56,6 +59,8 @@ export class PermissionsBoundary {
           node.addPropertyDeletionOverride('PermissionsBoundary');
         }
       },
-    }, { priority: AspectPriority.MUTATING });
+    }, {
+      priority: mutatingAspectPrio32333(this.scope),
+    });
   }
 }

Diff for: packages/aws-cdk-lib/aws-backup/lib/selection.ts

@@ -13,9 +13,10 @@ import { ImportedRole } from './private/imported-role';
 import { MutatingPolicyDocumentAdapter } from './private/policydoc-adapter';
 import { PrecreatedRole } from './private/precreated-role';
 import { AttachedPolicies, UniqueStringSet } from './private/util';
-import { ArnFormat, Duration, Resource, Stack, Token, TokenComparison, Aspects, Annotations, RemovalPolicy, AspectPriority } from '../../core';
+import { ArnFormat, Duration, Resource, Stack, Token, TokenComparison, Aspects, Annotations, RemovalPolicy } from '../../core';
 import { getCustomizeRolesConfig, getPrecreatedRoleConfig, CUSTOMIZE_ROLES_CONTEXT_KEY, CustomizeRoleConfig } from '../../core/lib/helpers-internal';
 import { addConstructMetadata, MethodMetadata } from '../../core/lib/metadata-resource';
+import { mutatingAspectPrio32333 } from '../../core/lib/private/aspect-prio';
 
 const MAX_INLINE_SIZE = 10000;
 const MAX_MANAGEDPOL_SIZE = 6000;
@@ -496,7 +497,9 @@ export class Role extends Resource implements IRole {
             this.splitLargePolicy();
           }
         },
-      }, { priority: AspectPriority.MUTATING });
+      }, {
+        priority: mutatingAspectPrio32333(this),
+      });
     }
 
     this.policyFragment = new ArnPrincipal(this.roleArn).policyFragment;

Diff for: packages/aws-cdk-lib/aws-ec2/lib/instance.ts

@@ -1,6 +1,6 @@
 import * as path from 'path';
 import { Match, Template } from '../../assertions';
-import { App, CfnResource, CustomResourceProvider, CustomResourceProviderRuntime, Stack } from '../../core';
+import { App, AspectPriority, Aspects, CfnResource, CustomResourceProvider, CustomResourceProviderRuntime, Stack } from '../../core';
 import * as iam from '../lib';
 
 let app: App;
@@ -166,3 +166,44 @@ test('unapply inherited boundary from a user: order 2', () => {
     PermissionsBoundary: Match.absent(),
   });
 });
+
+test.each([
+  [undefined, false, 'OVERRIDDEN'],
+  [AspectPriority.MUTATING, false, 'OVERRIDDEN'],
+  [AspectPriority.MUTATING, true, 'OVERRIDDEN'],
+  // custom DEFAULT, builtin MUTATING: custom wins and override is not applied
+  [undefined, true, 'BASE'],
+])('overriding works if base PB is applied using Aspect with prio %p (feature flag %p)', (basePrio, featureFlag, winner) => {
+  // When a custom aspect is used to apply a permissions boundary, and the built-in APIs to override it,
+  // the override still works.
+
+  if (featureFlag !== undefined) {
+    app = new App({ context: { '@aws-cdk/core:aspectPrioritiesMutating': featureFlag } });
+    stack = new Stack(app, 'Stack');
+  }
+
+  // GIVEN
+  Aspects.of(stack).add({
+    visit(node) {
+      if (node instanceof CfnResource && node.cfnResourceType === 'AWS::IAM::Role') {
+        node.addPropertyOverride('PermissionsBoundary', 'BASE');
+      }
+    },
+  }, {
+    priority: basePrio,
+  });
+
+  const role = new iam.Role(stack, 'Role', {
+    assumedBy: new iam.AnyPrincipal(),
+  });
+
+  // WHEN
+  iam.PermissionsBoundary.of(role).apply({
+    managedPolicyArn: 'OVERRIDDEN',
+  });
+
+  // THEN
+  Template.fromStack(stack).hasResourceProperties('AWS::IAM::Role', {
+    PermissionsBoundary: winner,
+  });
+});

Diff for: packages/aws-cdk-lib/aws-ecs/lib/cluster.ts

@@ -15,6 +15,7 @@ import { IBucket } from '../../aws-s3';
 import * as sns from '../../aws-sns';
 import * as cdk from '../../core';
 import { addConstructMetadata } from '../../core/lib/metadata-resource';
+import { mutatingAspectPrio32333 } from '../../core/lib/private/aspect-prio';
 
 /**
  * Options for portfolio share.
@@ -369,7 +370,9 @@ export class Portfolio extends PortfolioBase {
           (c as Portfolio).addBucketPermissionsToSharedAccounts();
         }
       },
-    }, { priority: cdk.AspectPriority.MUTATING });
+    }, {
+      priority: mutatingAspectPrio32333(this),
+    });
   }
 
   protected generateUniqueHash(value: string): string {

Diff for: packages/aws-cdk-lib/aws-iam/lib/permissions-boundary.ts

@@ -0,0 +1,15 @@
+import { IConstruct } from 'constructs';
+import * as cxapi from '../../../cx-api';
+import { AspectPriority } from '../aspect';
+import { FeatureFlags } from '../feature-flags';
+
+/**
+ * Return the aspect priority of Aspects changed in https://github.com/aws/aws-cdk/pull/32333
+ *
+ * We retroactively made those controllable using a feature flag.
+ *
+ * Aspects newly added since this change should unconditionally have a priority of `MUTATING`.
+ */
+export function mutatingAspectPrio32333(scope: IConstruct) {
+  return FeatureFlags.of(scope).isEnabled(cxapi.ASPECT_PRIORITIES_MUTATING) ? AspectPriority.MUTATING : undefined;
+}

Diff for: packages/aws-cdk-lib/aws-iam/lib/role.ts

@@ -2,6 +2,7 @@ import { IConstruct } from 'constructs';
 import { Annotations } from './annotations';
 import { Aspects, IAspect, AspectPriority } from './aspect';
 import { CfnResource } from './cfn-resource';
+import { mutatingAspectPrio32333 } from './private/aspect-prio';
 import { RemovalPolicy } from './removal-policy';
 
 /**
@@ -137,7 +138,7 @@ export class RemovalPolicies {
    */
   public apply(policy: RemovalPolicy, props: RemovalPolicyProps = {}) {
     Aspects.of(this.scope).add(new RemovalPolicyAspect(policy, props), {
-      priority: props.priority ?? AspectPriority.MUTATING,
+      priority: props.priority ?? mutatingAspectPrio32333(this.scope),
     });
   }
 

Diff for: packages/aws-cdk-lib/aws-iam/test/permissions-boundary.test.ts

@@ -4,7 +4,7 @@ import { IConstruct, Construct, Node } from 'constructs';
 import { Annotations } from './annotations';
 import { App } from './app';
 import { Arn, ArnComponents, ArnFormat } from './arn';
-import { AspectPriority, Aspects } from './aspect';
+import { Aspects } from './aspect';
 import { DockerImageAssetLocation, DockerImageAssetSource, FileAssetLocation, FileAssetSource } from './assets';
 import { CfnElement } from './cfn-element';
 import { Fn } from './cfn-fn';
@@ -584,7 +584,9 @@ export class Stack extends Construct implements ITaggable {
             node.addPropertyOverride('PermissionsBoundary', permissionsBoundaryArn);
           }
         },
-      }, { priority: AspectPriority.MUTATING });
+      }, {
+        priority: mutatingAspectPrio32333(this),
+      });
     }
   }
 
@@ -1838,4 +1840,5 @@ import { deployTimeLookup } from './private/region-lookup';
 import { makeUniqueResourceName } from './private/unique-resource-name';
 import { PRIVATE_CONTEXT_DEFAULT_STACK_SYNTHESIZER } from './private/private-context';
 import { Intrinsic } from './private/intrinsic';
+import { mutatingAspectPrio32333 } from './private/aspect-prio';
 /* eslint-enable import/order */

Diff for: packages/aws-cdk-lib/aws-servicecatalog/lib/portfolio.ts

@@ -68,6 +68,20 @@ export interface StageProps {
    * Options for applying a permissions boundary to all IAM Roles
    * and Users created within this Stage
    *
+   * Be aware that this feature uses Aspects, and the Aspects are applied at the
+   * Stack level with a priority of `MUTATING` (if the feature flag
+   * `@aws-cdk/core:aspectPrioritiesMutating` is set) or `DEFAULT` (if the flag
+   * is not set). This is relevant if you are both using your own Aspects to
+   * assign Permissions Boundaries, as well as specifying this property.  The
+   * Aspect added by this property will overwrite the Permissions Boundary
+   * assigned by your own Aspect if both: (a) your Aspect has a lower or equal
+   * priority to the automatic Aspect, and (b) your Aspect is applied *above*
+   * the Stack level.  If either of those conditions are not true, your own
+   * Aspect will win.
+   *
+   * We recommend assigning Permissions Boundaries only using the provided APIs,
+   * and not using custom Aspects.
+   *
    * @default - no permissions boundary is applied
    */
   readonly permissionsBoundary?: PermissionsBoundary;

Diff for: packages/aws-cdk-lib/core/lib/private/aspect-prio.ts

@@ -1,6 +1,7 @@
 import { Construct, IConstruct } from 'constructs';
 import { Annotations } from './annotations';
-import { IAspect, Aspects, AspectPriority, AspectOptions } from './aspect';
+import { IAspect, Aspects, AspectOptions } from './aspect';
+import { mutatingAspectPrio32333 } from './private/aspect-prio';
 import { ITaggable, ITaggableV2, TagManager } from './tag-manager';
 
 /**
@@ -160,7 +161,7 @@ export class Tags {
    */
   public add(key: string, value: string, props: TagProps = {}) {
     const tag = new Tag(key, value, props);
-    const options: AspectOptions = { priority: AspectPriority.MUTATING };
+    const options: AspectOptions = { priority: mutatingAspectPrio32333(this.scope) };
     Aspects.of(this.scope).add(tag, options);
   }
 
@@ -169,7 +170,7 @@ export class Tags {
    */
   public remove(key: string, props: TagProps = {}) {
     const removeTag = new RemoveTag(key, props);
-    const options: AspectOptions = { priority: AspectPriority.MUTATING };
+    const options: AspectOptions = { priority: mutatingAspectPrio32333(this.scope) };
     Aspects.of(this.scope).add(removeTag, options);
   }
 }

Diff for: packages/aws-cdk-lib/core/lib/removal-policies.ts

@@ -232,7 +232,6 @@ describe('aspect', () => {
     // THEN - check that Tags Aspect is applied to stack with mutating priority
     let aspectApplications = Aspects.of(stack).applied;
     expect(aspectApplications.length).toEqual(2);
-    expect(aspectApplications[1].priority).toEqual(AspectPriority.MUTATING);
 
     // THEN - both Aspects are successfully applied, new logging bucket is added with versioning enabled
     Template.fromStack(stack).hasResourceProperties('AWS::S3::Bucket', {

Diff for: packages/aws-cdk-lib/core/lib/stack.ts

@@ -144,8 +144,9 @@ describe('tag aspect', () => {
     expect(res2.tags.renderTags()).toEqual([{ key: 'first', value: 'there is only 1' }]);
   });
 
-  test('Tags applied without priority get mutating priority value', () => {
+  test.each([false, true])('Tags applied without priority get priority value that depends on feature flag %p', (flag) => {
     const root = new Stack();
+    root.node.setContext('@aws-cdk/core:aspectPrioritiesMutating', flag);
     const res = new TaggableResource(root, 'FakeResource', {
       type: 'AWS::Fake::Thing',
     });
@@ -154,11 +155,13 @@ describe('tag aspect', () => {
     Tags.of(res).add('first', 'there is only 1');
     Tags.of(res).remove('root');
 
+    const expected = flag ? AspectPriority.MUTATING : AspectPriority.DEFAULT;
+
     const rootAspectApplications = Aspects.of(root).applied;
-    expect(rootAspectApplications[0].priority).toEqual(AspectPriority.MUTATING);
+    expect(rootAspectApplications[0].priority).toEqual(expected);
     const resAspectApplications = Aspects.of(res).applied;
-    expect(resAspectApplications[0].priority).toEqual(AspectPriority.MUTATING);
-    expect(resAspectApplications[1].priority).toEqual(AspectPriority.MUTATING);
+    expect(resAspectApplications[0].priority).toEqual(expected);
+    expect(resAspectApplications[1].priority).toEqual(expected);
   });
 
   test('add will add a tag and remove will remove a tag if it exists', () => {