- 
                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?
Conversation
…plicationTargetGroup protocol property Closes aws#35295 - Add protocol validation during synthesis instead of waiting for CloudFormation deployment failures - Validate protocol requirement for non-Lambda targets regardless of targetType status - Maintain Lambda target exemption from protocol requirement - Update README examples to include required protocol property - Add comprehensive test coverage for all validation scenarios This change provides immediate developer feedback during CDK synthesis rather than runtime CloudFormation errors, improving the development experience without breaking existing valid configurations.
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| } | ||
|  | ||
| // Add a warning if protocol wasn't explicitly specified for non-Lambda targets | ||
| if (props.protocol === undefined && this.targetType !== TargetType.LAMBDA) { | 
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
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.
Left a comment
…and port validation - Update warning message to check for both protocol and port - Modify validation logic to use warning instead of hard error - Add test cases for new protocol/port validation behavior - Ensure backward compatibility with existing target group configurations - Provide clearer guidance for users about protocol and port requirements
…ApplicationTargetGroup - Add test for warning when protocol and port are not specified - Verify warning is not shown when port is provided - Ensure no validation error when protocol is specified - Test Lambda target group protocol handling - Add test for protocol/port validation when targets are added later - Improve test coverage for ApplicationTargetGroup configuration scenarios
| 'ApplicationTargetGroup protocol and port not specified. Please specify either protocol (e.g., ApplicationProtocol.HTTP or ApplicationProtocol.HTTPS) or port number. Note: Missing both will cause deployment failures.', | ||
| ); | ||
| } | ||
| } | 
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.
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:
- Fix the existing error validation in validateTargetGroup (change || to && logic)
- Add the warning annotation as supplementary feedback within validateTargetGroup when targetType is undefined
Issue # (if applicable)
Closes #35295.
Reason for this change
The AWS CDK ApplicationTargetGroup construct currently allows users to create target groups without specifying a protocol, which causes deployments to fail at runtime with AWS service errors ("A protocol must be specified (Service: ElasticLoadBalancingV2, Status Code: 400)") instead of providing early feedback during CDK synthesis.
As reported in #35295, developers can create ApplicationTargetGroups without specifying a protocol, and the error is only discovered during CloudFormation deployment rather than getting immediate guidance during development.
Description of changes
This change adds a warning-only enhancement to provide better developer experience without breaking existing code. When users create ApplicationTargetGroups without explicitly specifying a protocol (for non-Lambda targets), they now receive a clear warning message during synthesis.
Why warnings instead of validation errors?
We chose a warning-only approach for several important reasons:
Backward compatibility: Many existing CDK applications may have ApplicationTargetGroups without explicit protocol specification. Making this a hard validation error would break existing deployments and CI/CD pipelines.
Gradual migration: Warnings allow teams to identify and fix these issues at their own pace without blocking current development workflows.
Existing functionality: The current behavior technically works - it just fails later during CloudFormation deployment. A warning preserves this behavior while guiding users toward best practices.
Non-disruptive guidance: Warnings provide the educational benefit without forcing immediate code changes, allowing developers to prioritize fixes appropriately.
CDK philosophy: The CDK generally favors warnings for guidance over hard validation errors that could break existing code, especially for issues that don't prevent synthesis but may cause deployment problems.
Key changes
Technical implementation
Before this change
After this change
Benefits
Description of how you validated changes
Unit Tests
Integration Tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license