From 90c518e5eaef9fd361191cf8412596833ad7e9b5 Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Thu, 14 Mar 2024 16:43:22 -0700 Subject: [PATCH 01/12] fix(ElasticLoadBalancerV2): consistent logicalIds (under feature flag) --- CONTRIBUTING.md | 10 +-- .../lib/alb/application-listener.ts | 16 ++++- .../test/alb/listener.test.ts | 67 +++++++++++++++++++ packages/aws-cdk-lib/cx-api/README.md | 26 +++++++ packages/aws-cdk-lib/cx-api/lib/features.ts | 37 ++++++++++ 5 files changed, 148 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7644c072eb899..2b2025947dd1c 100644 --- a/CONTRIBUTING.md +++ b/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 diff --git a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts index bbb7d02f34214..0f03747e1b65a 100644 --- a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts +++ b/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, diff --git a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts index 907cf8a20a683..178134fe11f37 100644 --- a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts +++ b/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(); diff --git a/packages/aws-cdk-lib/cx-api/README.md b/packages/aws-cdk-lib/cx-api/README.md index cdbd86f3ae08e..598076627a17a 100644 --- a/packages/aws-cdk-lib/cx-api/README.md +++ b/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. diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index 2d76033fb0a5f..bab896490a3e2 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/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 = { ////////////////////////////////////////////////////////////////////// @@ -1034,6 +1036,41 @@ export const FLAGS: Record = { 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'; From 086d2f463082fbc20234158eb2c3f529cc270cd1 Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Thu, 28 Mar 2024 15:57:52 -0700 Subject: [PATCH 02/12] fix comment to match feature flag names --- .../lib/alb/application-listener.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts index 0f03747e1b65a..710cb6b498e9f 100644 --- a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts +++ b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts @@ -622,7 +622,7 @@ abstract class ExternalApplicationListener extends Resource implements IApplicat * 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. + * ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_TO_ADDACTION_MIGRATION feature flag. */ public addTargetGroups(id: string, props: AddApplicationTargetGroupsProps): void { checkAddRuleProps(props); @@ -672,7 +672,7 @@ abstract class ExternalApplicationListener extends Resource implements IApplicat * 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. + * ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID feature flag. */ public addAction(id: string, props: AddApplicationActionProps): void { checkAddRuleProps(props); From 4ad1095133f89e24d00786fb5a7510f343b323c4 Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 07:46:40 -0700 Subject: [PATCH 03/12] Update packages/aws-cdk-lib/cx-api/README.md Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com> --- packages/aws-cdk-lib/cx-api/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/cx-api/README.md b/packages/aws-cdk-lib/cx-api/README.md index 598076627a17a..c3ca349b65558 100644 --- a/packages/aws-cdk-lib/cx-api/README.md +++ b/packages/aws-cdk-lib/cx-api/README.md @@ -315,7 +315,7 @@ _cdk.json_ 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. +This will prevent `Rule` from being added as a suffix to the logicalId so that the logicalId will remain the same. Do not enable this if you have already deployed `ListenerRule` resources using the `addAction()` method. From 562b4a6794a9b182071e9dffa8994da03a598e3e Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 08:51:35 -0700 Subject: [PATCH 04/12] Update packages/aws-cdk-lib/cx-api/lib/features.ts Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com> --- packages/aws-cdk-lib/cx-api/lib/features.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index bab896490a3e2..8f32108462f5c 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/packages/aws-cdk-lib/cx-api/lib/features.ts @@ -1038,7 +1038,7 @@ export const FLAGS: Record = { }, ////////////////////////////////////////////////////////////////////// - [ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_TO_ADDACTION_MIGRATION]: { + [ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION]: { 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: ` From 06bdfe598f99e10da7ec071bfe19e636b5f707ff Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 08:51:53 -0700 Subject: [PATCH 05/12] Update packages/aws-cdk-lib/cx-api/lib/features.ts Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com> --- packages/aws-cdk-lib/cx-api/lib/features.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index 8f32108462f5c..22fd191bfee8d 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/packages/aws-cdk-lib/cx-api/lib/features.ts @@ -1040,7 +1040,7 @@ export const FLAGS: Record = { ////////////////////////////////////////////////////////////////////// [ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION]: { 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.', + summary: 'When enabled, you can switch 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. From c83bac6870d7dd8d8b819ce454bef87547ccc390 Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 08:52:00 -0700 Subject: [PATCH 06/12] Update packages/aws-cdk-lib/cx-api/lib/features.ts Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com> --- packages/aws-cdk-lib/cx-api/lib/features.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index 22fd191bfee8d..baa7b1d504f58 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/packages/aws-cdk-lib/cx-api/lib/features.ts @@ -1042,7 +1042,7 @@ export const FLAGS: Record = { type: FlagType.VisibleContext, summary: 'When enabled, you can switch 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, + When switching 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\`, From 7a8ee82987336b7df758d67d8cb3c833ba6f04a3 Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 08:52:37 -0700 Subject: [PATCH 07/12] Update packages/aws-cdk-lib/cx-api/lib/features.ts Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com> --- packages/aws-cdk-lib/cx-api/lib/features.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index baa7b1d504f58..6af443f04cc28 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/packages/aws-cdk-lib/cx-api/lib/features.ts @@ -1045,7 +1045,7 @@ export const FLAGS: Record = { When switching 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\`, + When this feature is not enabled, switching over to \`addAction()\` from using \`addTargetGroups()\` will 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. From c6d1de3c620237bf3516074389b48afe21866452 Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 08:52:49 -0700 Subject: [PATCH 08/12] Update packages/aws-cdk-lib/cx-api/lib/features.ts Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com> --- packages/aws-cdk-lib/cx-api/lib/features.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index 6af443f04cc28..64d9026a6bb50 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/packages/aws-cdk-lib/cx-api/lib/features.ts @@ -1051,7 +1051,7 @@ export const FLAGS: Record = { 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. + allowing you to switch from the \`addTargetGroups()\` method without having CloudFormation generate a new resource. For new resources, see instead the ${ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID} flag. `, From 440cb4ee7c4a2d6a5d2acf23bcec56c2e541486f Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 09:10:00 -0700 Subject: [PATCH 09/12] address comments --- .../lib/alb/application-listener.ts | 11 +++----- packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md | 17 +++++++++++ packages/aws-cdk-lib/cx-api/lib/features.ts | 28 ++++--------------- 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts index 710cb6b498e9f..78be70a1c6a5f 100644 --- a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts +++ b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts @@ -620,17 +620,13 @@ 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_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_TO_ADDACTION_MIGRATION 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 + idSuffix, { + new ApplicationListenerRule(this, id + 'Rule', { listener: this, priority: props.priority, ...props, @@ -672,13 +668,14 @@ abstract class ExternalApplicationListener extends Resource implements IApplicat * 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_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID feature flag. + * `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction` + * 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'; + const idSuffix = FeatureFlags.of(this).isEnabled(cxapi.ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION) ? '' : 'Rule'; // New rule // // TargetGroup.registerListener is called inside ApplicationListenerRule. diff --git a/packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md b/packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md index 0c39405d31b7e..95a327e7e786b 100644 --- a/packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md +++ b/packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md @@ -67,6 +67,8 @@ Flags come in three types: | [@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse](#aws-cdkaws-codepipelinecrossaccountkeysdefaultvaluetofalse) | Enables Pipeline to set the default value for crossAccountKeys to false. | 2.127.0 | (default) | | [@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2](#aws-cdkaws-codepipelinedefaultpipelinetypetov2) | Enables Pipeline to set the default pipeline type to V2. | 2.133.0 | (default) | | [@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope](#aws-cdkaws-kmsreducecrossaccountregionpolicyscope) | When enabled, IAM Policy created from KMS key grant will reduce the resource scope to this key only. | V2NEXT | (fix) | +| [@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction](#aws-cdkaws-elasticloadbalancingv2externalapplicationlistener-norulesuffixforaddaction) | When enabled, you can switch from the `addTargetGroups()` method of declaring a `ListenerRule` to the `addAction()` method, +without changing the logicalId and replacing your resource. | V2NEXT | (fix) | @@ -1265,4 +1267,19 @@ When this feature flag is enabled and calling KMS key grant method, the created | V2NEXT | `false` | `true` | +### @aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction + +*When enabled, you can switch from the `addTargetGroups()` method of declaring a `ListenerRule` to the `addAction()` method, +without changing the logicalId and replacing your resource.* (fix) + +Setting this feature flag will cause the `addAction()` method to not add the `Rule` suffix on the logicalId. +This allows you to switch from the `addTargetGroups()` method without having CloudFormation deadlock while attempting to replace the resource. + + +| Since | Default | Recommended | +| ----- | ----- | ----- | +| (not in v1) | | | +| V2NEXT | `false` | `false` | + + diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index 64d9026a6bb50..7a97a515dc557 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/packages/aws-cdk-lib/cx-api/lib/features.ts @@ -101,8 +101,7 @@ 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 ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION = '@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction'; export const FLAGS: Record = { ////////////////////////////////////////////////////////////////////// @@ -1045,32 +1044,17 @@ export const FLAGS: Record = { When switching 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. - When this feature is not enabled, switching over to \`addAction()\` from using \`addTargetGroups()\` will add a \`Rule\` suffix to the logicalId of your \`ListnerRule\`, + Before this feature is enabled, switching over to \`addAction()\` from using \`addTargetGroups()\` will add a \`Rule\` suffix to the logicalId of your \`ListenerRule\`, causing CloudFormation to attempt to replace the resource. - Since ListenerRules have a unique priority, - CloudFormation will always fail when generating the new ListenerRule. + Since \`ListenerRule\`s 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, - allowing you to switch from the \`addTargetGroups()\` method without having CloudFormation generate a new resource. - - For new resources, see instead the ${ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_CONSISTENT_LOGICALID} flag. + Setting this feature flag will cause the \`addAction()\` method to not add the \`Rule\` suffix on the logicalId. + This allows you to switch from the \`addTargetGroups()\` method without having CloudFormation deadlock while attempting to replace the resource. `, 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'; From d6417c1bd447a68769ec04e92db3e1688b0997c3 Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 09:38:16 -0700 Subject: [PATCH 10/12] fix tests --- .../test/alb/listener.test.ts | 34 +------------------ 1 file changed, 1 insertion(+), 33 deletions(-) diff --git a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts index 178134fe11f37..0f1f949a2b82b 100644 --- a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts +++ b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/listener.test.ts @@ -1686,7 +1686,7 @@ describe('tests', () => { test('compatibility mode for addAction', () => { // GIVEN - const context = { [cxapi.ENABLE_ALBV2_EXTERNALAPPLICATIONLISTENER_ADDTARGETGROUP_TO_ADDACTION_MIGRATION]: true }; + const context = { [cxapi.ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION]: true }; const app = new cdk.App({ context }); const stack = new cdk.Stack(app, 'stack', { env: { @@ -1714,38 +1714,6 @@ describe('tests', () => { 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', () => { From c737e4b9ac1ea8a235aeba8e7e84cf2314d4a2fa Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Tue, 2 Apr 2024 13:12:31 -0700 Subject: [PATCH 11/12] correctly revert the change for addTargetGroups --- .../aws-elasticloadbalancingv2/lib/alb/application-listener.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts index 78be70a1c6a5f..6d6c269f6de48 100644 --- a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts +++ b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts @@ -626,7 +626,7 @@ abstract class ExternalApplicationListener extends Resource implements IApplicat if (props.priority !== undefined) { // New rule - new ApplicationListenerRule(this, id + 'Rule', { + new ApplicationListenerRule(this, id, { listener: this, priority: props.priority, ...props, From b743db653ba05bd1d5fed0be9b62d40fb7d187f6 Mon Sep 17 00:00:00 2001 From: paulhcsun <47882901+paulhcsun@users.noreply.github.com> Date: Wed, 3 Apr 2024 10:09:08 -0700 Subject: [PATCH 12/12] Update packages/aws-cdk-lib/cx-api/lib/features.ts Add linebreak --- packages/aws-cdk-lib/cx-api/lib/features.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/cx-api/lib/features.ts b/packages/aws-cdk-lib/cx-api/lib/features.ts index d9794b32adeab..f3b7b142bd034 100644 --- a/packages/aws-cdk-lib/cx-api/lib/features.ts +++ b/packages/aws-cdk-lib/cx-api/lib/features.ts @@ -1044,7 +1044,8 @@ export const FLAGS: Record = { When switching 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. - Before this feature is enabled, switching over to \`addAction()\` from using \`addTargetGroups()\` will add a \`Rule\` suffix to the logicalId of your \`ListenerRule\`, + Before this feature is enabled, switching over to \`addAction()\` from using \`addTargetGroups()\` + will add a \`Rule\` suffix to the logicalId of your \`ListenerRule\`, causing CloudFormation to attempt to replace the resource. Since \`ListenerRule\`s have a unique priority, CloudFormation will always fail when generating the new \`ListenerRule\`.