Skip to content

fix(elasticloadbalancingv2): add validation on application listeners for certificates on HTTP protocol #34233

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

Merged

Conversation

wmattei
Copy link
Contributor

@wmattei wmattei commented Apr 23, 2025

Issue # (if applicable)

Reason for this change

ElasticLoadBalancerV2 throw a 400 error if you try to append a certificate to a listener on port 80 (or protocol HTTP).
This PR brings this same validation to CDK

Description of changes

Added a new check for the application protocol and the length of certificates, and if there is any certificate, throw a validation error.
Also, added a test for this case.

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the p2 label Apr 23, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team April 23, 2025 19:06
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Apr 23, 2025
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 23, 2025
@badmintoncryer
Copy link
Contributor

@wmattei Could you please merge the newest main branch? It results that needs-maintainer-review lavel will be attached.

@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Apr 26, 2025
@shikha372 shikha372 self-assigned this May 5, 2025
@@ -263,6 +263,10 @@ export class ApplicationListener extends BaseListener implements IApplicationLis
throw new ValidationError('At least one of \'port\' or \'protocol\' is required', scope);
}

if (protocol === ApplicationProtocol.HTTP && props.certificates?.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @wmattei , does this cause any failure if these values are passed to CFN ? if yes then okay to add it in CDK, if not then this can lead to a breaking change for the users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shikha372 it causes an error during deployment.
If I pass certificates to a listener on protocol HTTP I get the following error during deployment:

2:49:19 PM | UPDATE_FAILED        | AWS::ElasticLoadBalancingV2::Listener     | backendbackendlbba...listen
er805306FF55
Resource handler returned message: "A certificate cannot be specified for HTTP listeners (Service: ElasticL
oadBalancingV2, Status Code: 400, Request ID: 90e1f770-456b-4639-bd83-39ef554c8d0f) (SDK Attempt Count: 1)"
(RequestToken: f6460b90-6dc1-63d3-cad3-259db82be7a1, HandlerErrorCode: InvalidRequest)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, what do you think about throwing a warning here instead of an error, do not want to cause an unintended breaking change for the existing customers.

Copy link
Contributor Author

@wmattei wmattei May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that, but I am not sure about the experience of seeing a warning during synth only to the see the deploy fail if the warning is ignore.

Do we have things like this happening on other resources/constructs?

Copy link
Contributor

@shikha372 shikha372 May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we've warnings in other constructs/resources as well, if the check is already being handled by CFN, i think we should be good to just let them know before deployment with the warning which they can either bypass or choose to act upon, it doesn't become a blocker for any existing stack that are using such configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Ok, updated the PR, let me know what you think.
Not sure if the warning id needs to be defined somewhere else?

@shikha372 shikha372 changed the title chore(elasticloadbalancingv2): add validation on application listeners for certificates on HTTP protocol fix(elasticloadbalancingv2): add validation on application listeners for certificates on HTTP protocol May 8, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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)

@wmattei
Copy link
Contributor Author

wmattei commented May 14, 2025

Exemption Request:

This PR does not contain any change to synthesized stacks, it just adds a new warning.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label May 14, 2025
@shikha372 shikha372 added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels May 15, 2025
shikha372
shikha372 previously approved these changes May 15, 2025
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 15, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review May 15, 2025 17:37

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Copy link
Contributor

mergify bot commented May 15, 2025

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).

Copy link
Contributor

mergify bot commented May 15, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot dismissed shikha372’s stale review May 15, 2025 17:41

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 15, 2025
@wmattei
Copy link
Contributor Author

wmattei commented May 15, 2025

@shikha372 I think I will need your review again (:

shikha372
shikha372 previously approved these changes May 21, 2025
@mergify mergify bot dismissed shikha372’s stale review May 22, 2025 00:08

Pull request has been modified.

@wmattei
Copy link
Contributor Author

wmattei commented May 23, 2025

@shikha372 I think I am bit lost on what needs to be done on this PR.

The approval was dismissed.

Is there anything I must do? Thanks

shikha372
shikha372 previously approved these changes May 23, 2025
@mergify mergify bot dismissed shikha372’s stale review May 23, 2025 23:15

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 7aca52e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@wmattei
Copy link
Contributor Author

wmattei commented May 23, 2025

@shikha372 mergify seems to be dismissing your approval 🤔

Copy link
Contributor

mergify bot commented May 24, 2025

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).

@mergify mergify bot merged commit ca065bb into aws:main May 24, 2025
18 checks passed
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2025
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants