-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore(elasticloadbalancingv2): add warning for missing ApplicationTargetGroup protocol #35301
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
base: main
Are you sure you want to change the base?
Changes from 16 commits
3fd3149
5e9b248
00386c1
5969f87
c74c543
75c668a
f9873a1
20709d8
991a07d
380db07
93b259b
2b5a0ad
aaad69b
2b8ed69
dd68af8
115f18f
3aac685
f3b5672
ccd7c83
b9d9121
1de9500
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -404,6 +404,14 @@ export class ApplicationTargetGroup extends TargetGroupBase implements IApplicat | |
| this.setAttribute('load_balancing.algorithm.anomaly_mitigation', props.enableAnomalyMitigation ? 'on' : 'off'); | ||
| } | ||
| } | ||
|
|
||
| // Add a warning if protocol wasn't explicitly specified for non-Lambda targets | ||
| if (props.protocol === undefined && this.targetType !== TargetType.LAMBDA) { | ||
| Annotations.of(this).addWarningV2( | ||
| '@aws-cdk/aws-elbv2:target-group-protocol-default', | ||
| 'ApplicationTargetGroup protocol not specified. Please specify protocol explicitly (e.g., ApplicationProtocol.HTTP or ApplicationProtocol.HTTPS). Note: Missing protocol will cause deployment failures.', | ||
| ); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This validation should remain in the validateTargetGroup method to maintain consistency with existing validation patterns. This change weakens validation by converting an error to a warning and moves logic outside the dedicated validation method. I noticed also that the original validation logic was incorrect (used || instead of &&). We can fix this since, changing it to && logic actually loosens the validation, so it's not a breaking change. We can do:
|
||
|
|
||
| public get metrics(): IApplicationTargetGroupMetrics { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also sufficient to provide port number for stack to be deployable. We can check if both of those are undefined, otherwise ask it to provide either one of them. In the scenario where port number is provided, I can still get this validation error, even though I do not have any issue.
Reference to documentation: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_elasticloadbalancingv2.ApplicationTargetGroup.html#port
This change would require an update in the unit tests as well