-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
(ecs-patterns): unnecessary breaking target group port change introduced after upgrading cdk #19411
Comments
Thank you for raising this issue and providing a workaround @aaaeeeo! The purpose of the change in #18157 is to pass through the This is the behavior we want for new applications, since it works for all network modes. However, it has unfortunately introduced a breaking change for applications that have already been deployed with the previous behavior. While reviewing this fix, I did not realize that updating the |
For those looking to work around the issue via the L1 construct, it looks something like this: const service = new patterns.NetworkLoadBalancedFargateService(this, "FargateService", {
// props
});
// Work around https://github.com/aws/aws-cdk/issues/19411
(service.targetGroup.node.defaultChild as elbv2.CfnTargetGroup).port = 80; |
…ontainer port for target group port PR #18157 results in a new TargetGroup being created from NetworkLoadBalancedEc2Service, NetworkLoadBalancedFargateService, NetworkMultipleTargetGroupsEc2Service, and NetworkMultipleTargerGroupsFargateService even when no change is made because we are now passing through the containerPort props to TargetGroups's Port. For existing servies, this is a breaking change so a feature flag is needed. This PR adds that feature flag. Closes #19411.
…ontainer port for target group port PR #18157 results in a new TargetGroup being created from NetworkLoadBalancedEc2Service, NetworkLoadBalancedFargateService, NetworkMultipleTargetGroupsEc2Service, and NetworkMultipleTargerGroupsFargateService even when no change is made because we are now passing through the containerPort props to TargetGroups's Port. For existing servies, this is a breaking change so a feature flag is needed. This PR adds that feature flag. Closes #19411.
…ontainer port for target group port (#20284) PR #18157 results in a new TargetGroup being created from NetworkLoadBalancedEc2Service, NetworkLoadBalancedFargateService, NetworkMultipleTargetGroupsEc2Service, and NetworkMultipleTargerGroupsFargateService even when no change is made because we are now passing through the containerPort props to TargetGroups's Port. For existing services, this is a breaking change so a feature flag is needed. This PR adds that feature flag. Closes #19411. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
…ontainer port for target group port (aws#20284) PR aws#18157 results in a new TargetGroup being created from NetworkLoadBalancedEc2Service, NetworkLoadBalancedFargateService, NetworkMultipleTargetGroupsEc2Service, and NetworkMultipleTargerGroupsFargateService even when no change is made because we are now passing through the containerPort props to TargetGroups's Port. For existing services, this is a breaking change so a feature flag is needed. This PR adds that feature flag. Closes aws#19411. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ontainer port for target group port This re-implements #20284, which was reverted in #20430 due to the feature flag tests. PR #18157 results in a new TargetGroup being created from NetworkLoadBalancedEc2Service, NetworkLoadBalancedFargateService, NetworkMultipleTargetGroupsEc2Service, and NetworkMultipleTargerGroupsFargateService even when no change is made because we are now passing through the containerPort props to TargetGroups's Port. For existing services, this is a breaking change so a feature flag is needed. This PR adds that feature flag. Closes #19411.
What is the problem?
We recently upgraded the cdk version from 1.13x to 1.147 and noticed a breaking change prevent us from deploying.
The change is introduced by this commit: #18157 from #18073
In ecs-patterns, target group was generated with hard-coded port 80, and now changed to containerPort that in our case is 8080.
This triggered a replacement of the target group, and in production, we certainly cannot replace the target group or LB.
The service is currently working fine, and my understanding is when used with ECS and dynamic port mapping, the port in target group is meaningless as ECS host ports are dynamically opened:
https://stackoverflow.com/questions/42715647/whats-the-target-group-port-for-when-using-application-load-balancer-ec2-con
https://aws.amazon.com/premiumsupport/knowledge-center/dynamic-port-mapping-ecs/
Looks like even ECS is hard coding target group port to 80 when create service in console.
So what is the exact issue of #18073, what is it trying to fix? #18157 says "Fix Network Load Balancer Port assignments" but actaully changed the target group port instead? Looks like the change is meaningless and introduced breaking replacement to people who are already using the construct.
Currently, we have to patch the L1 target group to get this around. Could we revert #18157?
Reproduction Steps
Upgrade the cdk version from 1.130 to 1.147 and compare the generated TargetGroup from NetworkLoadBalancedEc2Service without any code change
What did you expect to happen?
Generated template should remain unchanged without any code change:
What actually happened?
Target group port changed to container port triggered a replacement of the target group for our service that is working fine.
CDK CLI Version
1.147
Framework Version
No response
Node.js Version
14
OS
Mac
Language
Typescript
Language Version
No response
Other information
No response
The text was updated successfully, but these errors were encountered: