fix(ecs-patterns): feature flag missing for breaking change passing container port for target group port #21021
Conversation
…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.
|
|
github-actions
bot
added
bug
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
It was hard to understand what modifications this PR made: the PR title and PR body didn't really clarify what and why the changes were in a way that makes it easy for someone without context to come into this.
I think it comes down to this:
- In January, we committed a change that should have had a feature flag attached to it. That feature flag was never there and it had impact on some people, causing replacement of a Load Balancer's Target Group (causing availability impact).
- We are now adding the feature flag that should have been added in the first place, after all.
If true, then that also means that we would be reverting behavior for anyone that init'ed a project in between January and today (half a year), causing availability impact for them.
Given that the new behavior has been out for a good long while, I wonder if reverting to behavior from January wouldn't cause more pain than good (especially since the original bug report hasn't been upvoted once, and has engagement from 2 people).
If we do want to add a feature flag to fix this for people, shouldn't it be an 'off by default' flag that people that want to get rid of the escape hatch in their code can enable?
| * and the `NetworkMultipleTargetGroups<Ec2|Fargate>ServiceProps.targetGroups[X].containerPort` to the generated | ||
| * `ElasticLoadBalancingV2::TargetGroup`'s `Port` property. | ||
| * | ||
| * This is a feature flag because updating `Port` causes a replacement of the target groups, which is a breaking change. |
There was a problem hiding this comment.
Does this mean that if the feature flag is not set, the containerPort property doesn't do anything at all?
If so, might clarify that in this docstring, and also in the docstring of the property object itself.
|
That's a fair point. We can just close this if we think it's not necessary anymore. |
TheRealAmazonKendra
deleted the
TheRealAmazonKendra/feature-flag-redo
branch
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license