-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(elasticloadbalancingv2): application load balancer attributes #29586
Conversation
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
eba9c25
to
9f869dd
Compare
@@ -65,6 +65,66 @@ export interface ApplicationLoadBalancerProps extends BaseLoadBalancerProps { | |||
* @default - Duration.seconds(3600) | |||
*/ | |||
readonly clientKeepAlive?: Duration; | |||
|
|||
/** | |||
* Indicates whether the Application Load Balancer should preserve the Host header in the HTTP request and send it to the target without any change. |
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.
nit: can you shorten these descriptions and put them on newlines?
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
/** | ||
* Application Load Balancer removes the X-Forwarded-For header in the HTTP request before it sends it to targets. | ||
*/ | ||
REMOVE = 'remove', |
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.
nit: can you shorten these docs and move to multiple lines?
* | ||
* @default false | ||
*/ | ||
readonly xAmznTlsVersionAndCipherSuiteHeaders?: boolean; |
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.
can you add @see
values with links to AWS docs?
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.
@badmintoncryer Since the urls are the same, I think it'd make more sense to add the @see
one time under Properties for defining an Application Load Balancer
?
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.
@msambol I was unable to retrieve links for each attribute and ended up entering the same URL...
I made the revisions just as you suggested!
* | ||
* @default false | ||
*/ | ||
readonly preserveXffClientPort?: boolean; |
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.
likewise for this and below, can you add @see
?
@msambol Thank you for your review! I have updated comments. |
denyAllIgwTraffic: false | ||
denyAllIgwTraffic: false, | ||
|
||
// Whether to preserve Host header in the request to the target |
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.
nit: lowercase host
@@ -68,6 +70,74 @@ export interface ApplicationLoadBalancerProps extends BaseLoadBalancerProps { | |||
* @default - Duration.seconds(3600) | |||
*/ | |||
readonly clientKeepAlive?: Duration; | |||
|
|||
/** | |||
* Indicates whether the Application Load Balancer should preserve the Host header in the HTTP request |
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.
nit: host
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.
Two small nits. Looks great 👍🏼
@msambol Thank you very much! |
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.
LGTM, thank you both for reviewing the PR and working on the PR!
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Issue # (if applicable)
Closes #29585.
Reason for this change
ALB supports some attributes that is not configurable from CDK
routing.http.preserve_host_header.enabled
routing.http.x_amzn_tls_version_and_cipher_suite.enabled
routing.http.xff_client_port.enabled
routing.http.xff_header_processing.mode
waf.fail_open.enabled
Description of changes
Added some props to
ApplicationLoadBalancerProps
.preserveHostHeader
xAmznTlsVersionAndCipherSuiteHeaders
preserveXffClientPort
xffHeaderProcessingMode
wafFailOpen
Description of how you validated changes
Added both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license