Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ElasticLoadBalancerV2: migration to addAction() from addTargetGroups() fails because of logicalId change #29496

Closed
ahammond opened this issue Mar 14, 2024 · 3 comments · Fixed by #29746
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@ahammond
Copy link
Contributor

Describe the bug

We need to access functionality not available in the ApplicationListener.addTargetGroups() helper method (specifically, ListenerAction.weightedForward()). To do this, we've changed from

Old code

      this.httpsListener.addTargetGroups(this.serviceRoot, {
        conditions: [ListenerCondition.pathPatterns(this.injectionPaths)],
        priority: this.injectionPriority,
        targetGroups: [this.taskTargetGroup],
      });

New code

      this.httpsListener.addAction(this.serviceRoot, {
        action: ListenerAction.weightedForward(this.weightedTargetGroups),        
        conditions: [ListenerCondition.pathPatterns(this.injectionPaths)],
        priority: this.injectionPriority,
      });

However, when I do a cdk diff of the stack, I get

Resources
[-] AWS::ElasticLoadBalancingV2::ListenerRule UsQaQaUsWest26/ClipsServiceInstance/https/--clips-service-v3 httpsclipsservicev36BE2E78C destroy
[+] AWS::ElasticLoadBalancingV2::ListenerRule UsQaQaUsWest26/ClipsServiceInstance/https/--clips-service-v3RuleRule httpsclipsservicev3RuleRule772FC4D6 

I note that https://github.com/aws/aws-cdk/blob/050a305d6be65df7e5f1e17668cbf512ead34d23/packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/alb/application-listener.ts#L675-L676 is adding the Rule suffix to the logicalId, which is driving a replacement rather than update.

Expected Behavior

We should be able to go from the helper method to the underlying support method without a logicalId change.

Current Behavior

Because of the logicalId change, Cfn wants to replace our ListenerRules. Create before destroy means that Cfn tries to create a new ListenerRule with the same priority as the existing ListnerRule, which fails, blocking upgrade path.

Reproduction Steps

Sigh.

Possible Solution

Add a feature flag for not adding the Rule suffix to the logicalId in the addAction() method.

Additional Information/Context

No response

CDK CLI Version

2.100.0 (build e1b5c77)

Framework Version

No response

Node.js Version

20.5.1

OS

MacOS

Language

TypeScript

Language Version

5.3.3

Other information

No response

@ahammond ahammond added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 14, 2024
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Mar 14, 2024
@pahud
Copy link
Contributor

pahud commented Mar 18, 2024

Yes sounds like we can do that with a feature flag or just override the logicalId.

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 18, 2024
mergify bot pushed a commit that referenced this issue Apr 3, 2024
…roups (under feature flag) (#29513)

### Issue ELBv2 logicalId inconsistency of ApplicationListenerRule logicalIds

Mitigates #29496

### Reason for this change

People using ALBs who need to migrate from the `addTargetGroups()` convenience method to the lower level `addAction()` method should not be blocked due to inconsistent logicalId's. Further, the logicalIds should be consistent going forward.

### Description of changes

There are two feature flags, one which sets a migration compat mode and another which fixed the behaviour to be consistent.

### Description of how you validated changes

Unit testing.

### 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*
@vinayak-kukreja vinayak-kukreja added @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 and removed package/tools Related to AWS CDK Tools or CLI labels Apr 12, 2024
@mergify mergify bot closed this as completed in #29746 Apr 17, 2024
@mergify mergify bot closed this as completed in f4af330 Apr 17, 2024
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
3 participants