From d2703ac24143d364635a0121395723d8e927a190 Mon Sep 17 00:00:00 2001 From: Andrew Hammond <445764+ahammond@users.noreply.github.com> Date: Mon, 8 Apr 2024 11:39:09 -0700 Subject: [PATCH] retain legacy behavior for addTargetGroups --- .../aws-elasticloadbalancingv2/README.md | 12 ++++----- .../lib/alb/application-listener.ts | 22 +++++++++++++--- .../test/alb/listener.test.ts | 26 ++++++++++++------- 3 files changed, 40 insertions(+), 20 deletions(-) diff --git a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md index 36f6ddf7d1be3..b79f330a17acb 100644 --- a/packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md +++ b/packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md @@ -768,13 +768,11 @@ const targetGroupMetrics: elbv2.IApplicationTargetGroupMetrics = targetGroup.met ## logicalIds on ExternalApplicationListener.addTargetGroups() and .addAction() -Legacy behavior for the `addTargetGroups()` method did not follow the standard of adding -a `Rule` suffix to the logicalId. -If you have `ListenerRule`s deployed using the legacy behavior and are upgrading, -you will need to enable the `removeRuleSuffixFromLogicalId: true` property -so that CloudFormation will not attempt to replace your existing `ListenerRule`s. - -Similarly, if you have `ListenerRule`s deployed using the legacy behavior of `addTargetGroups()`, +By default, the `addTargetGroups()` method does not follow the standard behavior +of adding a `Rule` suffix to the logicalId of the `ListenerRule` it creates. +If you are deploying new `ListenerRule`s using `addTargetGroups()` the recommendation +is to set the `removeRuleSuffixFromLogicalId: false` property. +If you have `ListenerRule`s deployed using the legacy behavior of `addTargetGroups()`, which you need to switch over to being managed by the `addAction()` method, then you will need to enable the `removeRuleSuffixFromLogicalId: true` property in the `addAction()` method. 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 d91955300e6b3..f0b7739264f66 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,12 +620,23 @@ 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. + * + * NOTE: if you are deploying new infrastructure using this method, + * it is recommended to set the `removeRuleSuffixFromLogicalId: false` + * property so that the logicalId for the generated `ListenerRule` + * is consistent with other methods for managing `ListenerRule`s. + * + * If you have already deployed a `ListenerRule` using this method + * and need to migrate to using the more feature rich `addAction()` + * method, then you will need to set the `removeRuleSuffixFromLogicalId: true` + * property there to avoid having CloudFormation attempt to replace your resource. */ public addTargetGroups(id: string, props: AddApplicationTargetGroupsProps): void { checkAddRuleProps(props); if (props.priority !== undefined) { - const ruleId = props.removeRuleSuffixFromLogicalId ? id : id + 'Rule'; + const removeRuleSuffixFromLogicalId = props.removeRuleSuffixFromLogicalId ?? true; + const ruleId = removeRuleSuffixFromLogicalId ? id : id + 'Rule'; // New rule new ApplicationListenerRule(this, ruleId, { listener: this, @@ -665,6 +676,11 @@ 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. + * + * If you previously deployed a `ListenerRule` using the `addTargetGroups()` method + * and need to migrate to using the more feature rich `addAction()` + * method, then you will need to set the `removeRuleSuffixFromLogicalId: true` + * property here to avoid having CloudFormation attempt to replace your resource. */ public addAction(id: string, props: AddApplicationActionProps): void { checkAddRuleProps(props); @@ -804,9 +820,9 @@ export interface AddApplicationTargetGroupsProps extends AddRuleProps { * * Legacy behavior did not include the `Rule` suffix on the logicalId of the generated `ListenerRule` * when generated by the `addTargetGroups()` method. - * This exists to allow backwards compatible behavior with `ListenerRule`s deployed by older versions of aws-cdk. + * This exists to allow new `ListenerRule`s to have consistent logicalIds. * - * @default - do not remove the logicalId `Rule` suffix + * @default true remove the logicalId `Rule` suffix */ readonly removeRuleSuffixFromLogicalId?: boolean; } 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 1602665e63142..85b934f0692da 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 @@ -1699,16 +1699,21 @@ describe('tests', () => { }); describe('Rule suffix for logicalId', () => { + const identifierToken = 'SuperMagicToken'; interface TestCase { - readonly removeRuleSuffixFromLogicalId: boolean; + readonly removeRuleSuffixFromLogicalId?: boolean; + readonly expectedLogicalId: string; }; - const testCases: TestCase[] = [ - { removeRuleSuffixFromLogicalId: true }, - { removeRuleSuffixFromLogicalId: false }, + const nonDefaultTestCases: TestCase[] = [ + { removeRuleSuffixFromLogicalId: true, expectedLogicalId: identifierToken }, + { removeRuleSuffixFromLogicalId: false, expectedLogicalId: identifierToken + 'Rule' }, ]; - test.each(testCases)('addTargetGroups %s', ({ removeRuleSuffixFromLogicalId }) => { + test.each([ + // Default is to not have the `Rule` suffix, per legacy behavior. + { removeRuleSuffixFromLogicalId: undefined, expectedLogicalId: identifierToken }, + ...nonDefaultTestCases, + ])('addTargetGroups %s', ({ removeRuleSuffixFromLogicalId, expectedLogicalId }) => { // GIVEN - const identifierToken = 'SuperMagicToken'; const app = new cdk.App(); const stack = new cdk.Stack(app, 'TestStack', { env: { account: '123456789012', region: 'us-east-1' } }); const vpc = new ec2.Vpc(stack, 'Stack'); @@ -1728,15 +1733,17 @@ describe('tests', () => { }); // THEN - const expectedLogicalId = removeRuleSuffixFromLogicalId ? identifierToken : identifierToken + 'Rule'; const applicationListenerRule = listener.node.children.find((v)=> v.hasOwnProperty('conditions')); expect(applicationListenerRule).toBeDefined(); expect(applicationListenerRule!.node.id).toBe(expectedLogicalId); }); - test.each(testCases)('addAction %s', ({ removeRuleSuffixFromLogicalId }) => { + test.each([ + // Default is consistent, which means it has the `Rule` suffix. This means no change from legacy behavior + { removeRuleSuffixFromLogicalId: undefined, expectedLogicalId: identifierToken + 'Rule' }, + ...nonDefaultTestCases, + ])('addAction %s', ({ removeRuleSuffixFromLogicalId, expectedLogicalId }) => { // GIVEN - const identifierToken = 'SuperMagicToken'; const app = new cdk.App(); const stack = new cdk.Stack(app, 'TestStack', { env: { account: '123456789012', region: 'us-east-1' } }); const vpc = new ec2.Vpc(stack, 'Stack'); @@ -1756,7 +1763,6 @@ describe('tests', () => { }); // THEN - const expectedLogicalId = removeRuleSuffixFromLogicalId ? identifierToken : identifierToken + 'Rule'; const applicationListenerRule = listener.node.children.find((v)=> v.hasOwnProperty('conditions')); expect(applicationListenerRule).toBeDefined(); expect(applicationListenerRule!.node.id).toBe(expectedLogicalId);