Skip to content

Commit

Permalink
feat(elasticloadbalancingv2): add removeSuffix param for ExternalAppl…
Browse files Browse the repository at this point in the history
…icationListener.addAction() (#29746)

### Issue # (if applicable)

Closes #29496

### Reason for this change

See #29513 (props based solution instead of feature-flag)

### Description of changes

Adds a `removeSuffix` property to addAction method to address problems due to logicalId inconsistency.

### Description of how you validated changes

UTs. Per IT document, integration tests are not necessary.

### 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*
  • Loading branch information
ahammond authored Apr 17, 2024
1 parent 0da25e5 commit f4af330
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 1 deletion.
13 changes: 13 additions & 0 deletions packages/aws-cdk-lib/aws-elasticloadbalancingv2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -765,3 +765,16 @@ const targetGroup = elbv2.ApplicationTargetGroup.fromTargetGroupAttributes(this,

const targetGroupMetrics: elbv2.IApplicationTargetGroupMetrics = targetGroup.metrics; // throws an Error()
```

## logicalIds on ExternalApplicationListener.addTargetGroups() and .addAction()

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.

`ListenerRule`s have a unique `priority` for a given `Listener`.
Because the `priority` must be unique, CloudFormation will always fail when creating a new `ListenerRule` to replace the existing one, unless you change the `priority` as well as the logicalId.
Original file line number Diff line number Diff line change
Expand Up @@ -664,15 +664,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.
*
* 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);

if (props.priority !== undefined) {
const ruleId = props.removeSuffix ? id : id + 'Rule';
// New rule
//
// TargetGroup.registerListener is called inside ApplicationListenerRule.
new ApplicationListenerRule(this, id + 'Rule', {
new ApplicationListenerRule(this, ruleId, {
listener: this,
priority: props.priority,
...props,
Expand Down Expand Up @@ -807,6 +813,19 @@ export interface AddApplicationActionProps extends AddRuleProps {
* Action to perform
*/
readonly action: ListenerAction;
/**
* `ListenerRule`s have a `Rule` suffix on their logicalId by default. This allows you to remove that suffix.
*
* Legacy behavior of the `addTargetGroups()` convenience method did not include the `Rule` suffix on the logicalId of the generated `ListenerRule`.
* At some point, increasing complexity of requirements can require users to switch from the `addTargetGroups()` method
* to the `addAction()` method.
* When migrating `ListenerRule`s deployed by a legacy version of `addTargetGroups()`,
* you will need to enable this flag to avoid changing the logicalId of your resource.
* Otherwise Cfn will attempt to replace the `ListenerRule` and fail.
*
* @default - use standard logicalId with the `Rule` suffix
*/
readonly removeSuffix?: boolean;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1698,6 +1698,47 @@ describe('tests', () => {
}).toThrow(/Specify at most one/);
});

describe('Rule suffix for logicalId', () => {
const identifierToken = 'SuperMagicToken';
interface TestCase {
readonly removeSuffix?: boolean;
readonly expectedLogicalId: string;
};
const nonDefaultTestCases: TestCase[] = [
{ removeSuffix: true, expectedLogicalId: identifierToken },
{ removeSuffix: false, expectedLogicalId: identifierToken + 'Rule' },
];
test.each<TestCase>([
// Default is consistent, which means it has the `Rule` suffix. This means no change from legacy behavior
{ removeSuffix: undefined, expectedLogicalId: identifierToken + 'Rule' },
...nonDefaultTestCases,
])('addAction %s', ({ removeSuffix, expectedLogicalId }) => {
// GIVEN
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');
const targetGroup = new elbv2.ApplicationTargetGroup(stack, 'TargetGroup', { vpc, port: 80 });
const listener = elbv2.ApplicationListener.fromLookup(stack, 'a', {
loadBalancerTags: {
some: 'tag',
},
});

// WHEN
listener.addAction(identifierToken, {
action: elbv2.ListenerAction.weightedForward([{ targetGroup, weight: 1 }]),
conditions: [elbv2.ListenerCondition.pathPatterns(['/fake'])],
priority: 42,
removeSuffix,
});

// THEN
const applicationListenerRule = listener.node.children.find((v)=> v.hasOwnProperty('conditions'));
expect(applicationListenerRule).toBeDefined();
expect(applicationListenerRule!.node.id).toBe(expectedLogicalId);
});
});

describe('lookup', () => {
test('Can look up an ApplicationListener', () => {
// GIVEN
Expand Down

0 comments on commit f4af330

Please sign in to comment.