feat: Allow multiple domains in one cert #137
Conversation
amontalban
changed the title
amontalban
changed the title
|
@antonbabenko Do you think this is good to be merged? I have tested with my use case but I don't have access to the zones/domains that are described in the test cases so cannot fully validate it is a breaking change or not. Thank you! |
There was a problem hiding this comment.
It looks pretty good, but small updates are necessary. Please make examples 100% working and verify them yourself (for e.g., by providing your own domain names via terraform.tfvars). Also, set required validation_method argument.
Also, update main README.md and include code snippet for multi-domain ACM in the same fashion as with other snippets.
| @@ -32,8 +35,7 @@ module "acm" { | |||
| source = "../../" | |||
|
|
|||
| providers = { | |||
| aws.acm = aws, | |||
| aws.dns = aws | |||
| aws = aws.acm | |||
There was a problem hiding this comment.
While your change is valid, please revert to show that there can be two distinct providers passed to the module.
| @@ -60,11 +62,13 @@ module "acm" { | |||
| ################################################################ | |||
|
|
|||
| provider "aws" { | |||
| alias = "route53" | |||
| alias = "route53" | |||
| region = local.region | |||
There was a problem hiding this comment.
Please revert all changes not necessary for your PR.
| ########################################################## | ||
|
|
||
| provider "aws" { | ||
| region = local.region |
There was a problem hiding this comment.
Please remove region as it is not important for this example.
|
|
||
| # Removing trailing dot from domain - just to be sure :) | ||
| domain_name = trimsuffix(local.domain, ".") | ||
|
|
||
| region = "us-east-1" |
There was a problem hiding this comment.
Please remove this region, too.
| } | ||
|
|
||
| variable "extra_domain" { | ||
| description = "Extr adomain to be used for the tests" |
There was a problem hiding this comment.
| description = "Extr adomain to be used for the tests" | |
| description = "Extra domain to be used in acm_multi_domain module" |
|
Any update on this merge? |
|
changes have been requested but not implemented as shown above, so it won't be merged until resolved |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
This PR was automatically closed because of stale in 10 days |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
This PR allows creating one ACM certificate for multiple domains, which, is useful when using the certificate for CloudFront that only allows one certificate per distribution.
Motivation and Context
CloudFront does not support multiple ACM certificates, like ALB. Therefore, if you need to support multiple domains in a single CloudFront distribution you would have to create the certificate manually because this module does not support it.
Breaking Changes
This change should be backward compatible as I added a
zonesvar containing a map with domains and their Route53 zone ID so the validation records are created in the correct Route53 zone.Additionally, I have updated the tests in
examples/complete-dns-validationto allow variables so it was easier for me (and others) to test with my test domains.How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull requestFixes #136