fix(ElasticLoadBalancerV2): consistent logicalIds (under feature flag)

CONTRIBUTING.md

@@ -1196,18 +1196,18 @@ Adding a new flag looks as follows:
    with the name of the context key that enables this new feature (for
    example, `ENABLE_STACK_NAME_DUPLICATES`). The context key should be in the
    form `module.Type:feature` (e.g. `@aws-cdk/core:enableStackNameDuplicates`).
+2. Add your feature flag to the `FLAGS` map in
+   [cx-api/lib/features.ts](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/cx-api/lib/features.ts).
     - Set `introducedIn.v2` to the literal string `'V2NEXT'`.
     - Double negatives should be avoided. If you want to add a flag that disables something that was previously
       enabled, set `default.v2` to `true` and the `recommendedValue` to `false`. You will need to update
       a test in `features.test.ts` -- this is okay if you have a good reason.
-2. Use `FeatureFlags.of(construct).isEnabled(cxapi.ENABLE_XXX)` to check if this feature is enabled
-   in your code. If it is not defined, revert to the legacy behavior.
-3. Add your feature flag to the `FLAGS` map in
-   [cx-api/lib/features.ts](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/cx-api/lib/features.ts). In
-   your description, be sure to cover the following:
+   In your description, be sure to cover the following:
    - Consciously pick the type of feature flag. Can the flag be removed in a future major version, or not?
    - Motivate why the feature flag exists. What is the change to existing infrastructure and why is it not safe?
    - In case of a "default change flag", describe what the user needs to do to restore the old behavior.
+3. Use `FeatureFlags.of(construct).isEnabled(cxapi.ENABLE_XXX)` to check if this feature is enabled
+   in your code. If it is not defined, revert to the legacy behavior.
 4. Add an entry for your feature flag in the [README](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/cx-api/README.md) file.
 5. In your tests, ensure that you test your feature with and without the feature flag enabled. You can do this by passing the feature flag to the `context` property when instantiating an `App`.
    ```ts

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

@@ -7,7 +7,7 @@ import { ApplicationTargetGroup, IApplicationLoadBalancerTarget, IApplicationTar
 import { ListenerCondition } from './conditions';
 import * as ec2 from '../../../aws-ec2';
 import * as cxschema from '../../../cloud-assembly-schema';
-import { Duration, Lazy, Resource, Token } from '../../../core';
+import { Duration, FeatureFlags, Lazy, Resource, Token } from '../../../core';
 import * as cxapi from '../../../cx-api';
 import { BaseListener, BaseListenerLookupOptions, IListener } from '../shared/base-listener';
 import { HealthCheck } from '../shared/base-target-group';
@@ -620,13 +620,17 @@ abstract class ExternalApplicationListener extends Resource implements IApplicat
    *
    * It's possible to add conditions to the TargetGroups added in this way.
    * At least one TargetGroup must be added without conditions.
+   *
+   * Warning, when creating new resources, we strongly recommend setting the
+   * ENABLE_ALBV2_ADDTARGETGROUP_CONSISTENT_LOGICALID feature flag.
    */
   public addTargetGroups(id: string, props: AddApplicationTargetGroupsProps): void {
     checkAddRuleProps(props);
 
     if (props.priority !== undefined) {
+      const idSuffix = FeatureFlags.of(this).isEnabled(cxapi.ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID) ? 'Rule' : '';
       // New rule
-      new ApplicationListenerRule(this, id, {
+      new ApplicationListenerRule(this, id + idSuffix, {
         listener: this,
         priority: props.priority,
         ...props,
@@ -664,15 +668,21 @@ abstract class ExternalApplicationListener extends Resource implements IApplicat
    * It is not possible to add a default action to an imported IApplicationListener.
    * In order to add actions to an imported IApplicationListener a `priority`
    * must be provided.
+   *
+   * Warning, if you are attempting to migrate an existing `ListenerAction`
+   * which was declared by the {@link addTargetGroups} method, you will
+   * need to enable the
+   * ENABLE_ALBV2_ADDTARGETGROUP_TO_ADDACTION_MIGRATION feature flag.
    */
   public addAction(id: string, props: AddApplicationActionProps): void {
     checkAddRuleProps(props);
 
     if (props.priority !== undefined) {
+      const idSuffix = FeatureFlags.of(this).isEnabled(cxapi.ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_TO_ADDACTION_MIGRATION) ? '' : 'Rule';
       // New rule
       //
       // TargetGroup.registerListener is called inside ApplicationListenerRule.
-      new ApplicationListenerRule(this, id + 'Rule', {
+      new ApplicationListenerRule(this, id + idSuffix, {
         listener: this,
         priority: props.priority,
         ...props,

packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts

@@ -6,6 +6,7 @@ import { Metric } from '../../../aws-cloudwatch';
 import * as ec2 from '../../../aws-ec2';
 import * as cdk from '../../../core';
 import { SecretValue } from '../../../core';
+import * as cxapi from '../../../cx-api';
 import * as elbv2 from '../../lib';
 import { FakeSelfRegisteringTarget } from '../helpers';
 
@@ -1681,6 +1682,72 @@ describe('tests', () => {
     }).toThrow(/specify only one/);
   });
 
+  describe('ExternalApplicationListener logicalId support', () => {
+
+    test('compatibility mode for addAction', () => {
+      // GIVEN
+      const context = { [cxapi.ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_TO_ADDACTION_MIGRATION]: true };
+      const app = new cdk.App({ context });
+      const stack = new cdk.Stack(app, 'stack', {
+        env: {
+          account: '123456789012',
+          region: 'us-west-2',
+        },
+      });
+      const vpc = new ec2.Vpc(stack, 'Stack');
+      const targetGroup = new elbv2.ApplicationTargetGroup(stack, 'TargetGroup', { vpc, port: 80 });
+      const listener = elbv2.ApplicationListener.fromLookup(stack, 'a', {
+        loadBalancerTags: {
+          some: 'tag',
+        },
+      });
+      // WHEN
+      const identifierToken = 'SuperMagicToken';
+      listener.addAction(identifierToken, {
+        action: elbv2.ListenerAction.weightedForward([{ targetGroup, weight: 1 }]),
+        conditions: [elbv2.ListenerCondition.pathPatterns(['/fake'])],
+        priority: 42,
+      });
+
+      // THEN
+      const applicationListenerRule = listener.node.children.find((v)=> v.hasOwnProperty('conditions'));
+      expect(applicationListenerRule).toBeDefined();
+      expect(applicationListenerRule!.node.id).toBe(identifierToken); // Should not have `Rule` suffix
+    });
+
+    test('consistent', () => {
+      // GIVEN
+      const context = { [cxapi.ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID]: true };
+      const app = new cdk.App({ context });
+      const stack = new cdk.Stack(app, 'stack', {
+        env: {
+          account: '123456789012',
+          region: 'us-west-2',
+        },
+      });
+      const vpc = new ec2.Vpc(stack, 'Stack');
+      const targetGroup = new elbv2.ApplicationTargetGroup(stack, 'TargetGroup', { vpc, port: 80 });
+      const listener = elbv2.ApplicationListener.fromLookup(stack, 'a', {
+        loadBalancerTags: {
+          some: 'tag',
+        },
+      });
+
+      // WHEN
+      const identifierToken = 'SuperMagicToken';
+      listener.addTargetGroups(identifierToken, {
+        conditions: [elbv2.ListenerCondition.pathPatterns(['/fake'])],
+        priority: 42,
+        targetGroups: [targetGroup],
+      });
+
+      // THEN
+      const applicationListenerRule = listener.node.children.find((v)=> v.hasOwnProperty('conditions'));
+      expect(applicationListenerRule).toBeDefined();
+      expect(applicationListenerRule!.node.id).toBe(identifierToken + 'Rule'); // Should have `Rule` suffix
+    });
+  });
+
   test('not allowed to specify defaultTargetGroups and defaultAction together', () => {
     // GIVEN
     const stack = new cdk.Stack();

packages/aws-cdk-lib/cx-api/README.md

@@ -309,3 +309,29 @@ _cdk.json_
   }
 }
 ```
+
+* `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction`
+
+Enable this feature flag if you have deployed a `ListenerRule` using the `addTargetGroups()`
+convenience method against an `ExternalApplicationListener` and you need to migrate to
+using the `addAction()` method for more complex rule configurations.
+This will prevent the logicalId from changing.
+
+Do not enable this if you have already deployed `ListenerRule` resources using the
+`addAction()` method.
+Instead consider the [cdk-logical-id-mapper](https://github.com/mbonig/cdk-logical-id-mapper),
+possibly in conjunction with `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-addTargetGroupsConsistentLogicalId` (see below).
+
+* `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-addTargetGroupsConsistentLogicalId`
+
+Enable this feature flag to ensure that the logicalIds of `ListenerRule`s created
+on a `ExternalApplicationListener` by the `addTargetGroups()` method are consistent
+with logicalIds for `ListenerRules` generated by other methods.
+This will allow you to migrate between the different methods
+without causing a replacement of the `ListenerRule` resource.
+
+You should enable this on new apps, before creating any resources.
+If you have already created resources with the previous behavior,
+you may still enable this flag, but will need to use something like the
+[cdk-logical-id-mapper](https://github.com/mbonig/cdk-logical-id-mapper).
+Alternatively, do not enable this feature flag and instead consider the `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction` as necessary.

packages/aws-cdk-lib/cx-api/lib/features.ts

@@ -101,6 +101,8 @@ export const LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION = '@aws-cdk/aws-clou
 export const CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE = '@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse';
 export const CODEPIPELINE_DEFAULT_PIPELINE_TYPE_TO_V2 = '@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2';
 export const KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE = '@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope';
+export const ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_TO_ADDACTION_MIGRATION = '@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction';
+export const ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID = '@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-addTargetGroupsConsistentLogicalId';
 
 export const FLAGS: Record<string, FlagInfo> = {
   //////////////////////////////////////////////////////////////////////
@@ -1034,6 +1036,41 @@ export const FLAGS: Record<string, FlagInfo> = {
     introducedIn: { v2: 'V2NEXT' },
     recommendedValue: true,
   },
+
+  //////////////////////////////////////////////////////////////////////
+  [ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_TO_ADDACTION_MIGRATION]: {
+    type: FlagType.VisibleContext,
+    summary: 'When enabled, you can migrate from the \`addTargetGroups()\` method of declaring a \`ListenerRule\` to the \`addAction()\` method, without changing the logicalId and replacing your resource.',
+    detailsMd: `
+      When migrating from a less complex to a more complex use of ALB,
+      you will eventually need features not available in the \`addTargetGroups()\` convenience method.
+      In this case you will want to use the \`addAction()\` method.
+      Without this feature being activated, the change will also add a \`Rule\` suffix to the logicalId of your \`ListnerRule\`,
+      causing CloudFormation to attempt to replace the resource.
+      Since ListenerRules have a unique priority,
+      CloudFormation will always fail when generating the new ListenerRule.
+
+      Setting this feature flag will cause the \`addAction()\` method to not add the \`Rule\` suffix on the logicalId,
+      enabling a migration from the \`addTargetGroups()\` method.
+
+      For new resources, see instead the ${ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID} flag.
+    `,
+    introducedIn: { v2: 'V2NEXT' },
+    recommendedValue: false,
+  },
+
+  //////////////////////////////////////////////////////////////////////
+  [ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID]: {
+    type: FlagType.BugFix,
+    summary: 'When enabled, the \`addTargetGroups()\` method will generate logicalIds which are consistent with other methods',
+    detailsMd: `
+      By default, the \`addTargetGroups()\` method omits the \`Rule\` suffix on the logicalId for the generated \`ListenerRule\`.
+      All other methods add this suffix when generating a \`ListenerRule\`.
+      Enabling this flag will cause the \`addTargetGroups()\` method to follow this naming pattern.
+    `,
+    introducedIn: { v2: 'V2NEXT' },
+    recommendedValue: true,
+  },
 };
 
 const CURRENT_MV = 'v2';