Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[elasticloadbalancing] Clarify expected value of sslCertificateId property of LoadBalancerListener is an ARN #9303

Closed
dillon-odonovan opened this issue Jul 28, 2020 · 2 comments · Fixed by #13766
Closed

[elasticloadbalancing] Clarify expected value of sslCertificateId property of LoadBalancerListener is an ARN #9303

dillon-odonovan opened this issue Jul 28, 2020 · 2 comments · Fixed by #13766
Assignees
@njlynch
Labels
@aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing documentation This is a problem with documentation. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@dillon-odonovan
Copy link
Contributor

https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancing.LoadBalancerListener.html#sslcertificateid

I was attempting to replicate some existing AWS resources in a CDK app and one of the resources was a Classic Load Balancer which had an HTTPS listener and was using an ACM certificate. The documentation describes the sslCertificateId property of LoadBalancerListener as the "ID of the SSL Certificate", and the data type of the field is a string, so I supplied the ACM identifier. When I tried cdk deploy, I eventually got an HTTP 400 error "Server Certificate not found for the key". I verified that I had no typos and was left a little confused as to why the sslCertificateId was not found, so I went to the CLI documentation looking for additional information on the API call being used and any restrictions on the input. I landed on the AWS CLI documentation for set-load-balancer-listener-ssl-certificate, where the definition of --ssl-certificate-id states that this is the ARN of the SSL Certificate.

I would like the CDK documentation updated to reflect this, and explicitly state that sslCertificateId refers to the ARN of the certificate and not just the identifier, as simply referring to it as an ID or identifier is a bit ambiguous for those not already aware that the expected value is an ARN.

This is a 📕 documentation issue
@dillon-odonovan dillon-odonovan added documentation This is a problem with documentation. feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 28, 2020
@dillon-odonovan dillon-odonovan changed the title [elasticloadbalancing] Clarify expected value of sslCertificateId property of LoadBalancerListener [elasticloadbalancing] Clarify expected value of sslCertificateId property of LoadBalancerListener is an ARN Jul 28, 2020
@github-actions github-actions bot added the @aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing label Jul 28, 2020
@ericzbeard ericzbeard assigned rix0rrr and unassigned ericzbeard Jul 28, 2020
@ericzbeard ericzbeard removed the needs-triage This issue or PR still needs to be triaged. label Jul 28, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 4, 2020 •
edited
Loading

Fair. Even better probably is to rename to sslCertificateArn and deprecate the old property.

@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2 labels Aug 4, 2020
@SomayaB SomayaB assigned njlynch and unassigned rix0rrr Aug 20, 2020
OksanaH added a commit to OksanaH/aws-cdk that referenced this issue Mar 20, 2021
@OksanaH
chore(elasticloadbalancing): Clarify expected value of sslCertificate…
2989c85 
…Id property of LoadBalancerListener is an ARN aws#9303
OksanaH added a commit to OksanaH/aws-cdk that referenced this issue Mar 24, 2021
@OksanaH
feat(elasticloadbalancing): deprecate sslCertificateId property of th…
14cfc79 
…e LB listener, use sslCertificateArn property instead aws#9303
@OksanaH OksanaH mentioned this issue Mar 24, 2021
Merged
@mergify mergify bot closed this as completed in #13766 Apr 12, 2021
mergify bot pushed a commit that referenced this issue Apr 12, 2021
@OksanaH
feat(elasticloadbalancing): rename 'sslCertificateId' property of LB …
1a30272 
…listener to 'sslCertificateArn'; deprecate sslCertificateId property (#13766)

The property `sslCertificateId` of the LoadBalancer listener actually means sslCertificateArn. So as suggested in #9303, I have deprecated `sslCertificateId` and replaced it by `sslCertificateArn` to better reflect its actual meaning.

fixes #9303
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
@OksanaH @hollanddd
feat(elasticloadbalancing): rename 'sslCertificateId' property of LB …
e5cbef5 
…listener to 'sslCertificateArn'; deprecate sslCertificateId property (aws#13766)

The property `sslCertificateId` of the LoadBalancer listener actually means sslCertificateArn. So as suggested in aws#9303, I have deprecated `sslCertificateId` and replaced it by `sslCertificateArn` to better reflect its actual meaning.

fixes aws#9303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@njlynch njlynch

Labels
@aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing documentation This is a problem with documentation. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
Milestone
No milestone
4 participants
@rix0rrr @ericzbeard @njlynch @dillon-odonovan