-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Add validation_domain parameter to aws_acm_certificate #3853
Conversation
Works like a charm, thanks! |
Ok, yesterday it looked good, today it doesn't. |
@telepath nice observation! I never set the domain validation for the subject alternative names because the certificate is issued as long as the domain name is validated. I have tested it with several subject alternative names, and it issues fine for me, even though the alternative names are wrong like you said. I looked at your issue again and it looks like you want to set the validation domain to be the same for all the subject alternative names, is that correct? There are two ways to fix this problem: 1) change the input to match the cloudformation/cli so that you need to specify a list of DomainName and ValidationDomain objects in a domain_validation_options field (which name conflicts with an existing, computed field... but this computed field doesn't do much of anything anyways so we can get rid of it) or 2) keep the inputs like they are and if a validation_domain is set, loop through each of the subject alternative names and set the validationdomain to what was provided. I think option two would probably be best, what do you think? |
Option 2 would be good. Option one would be more complete, but difficult to assemble in terraform (im using a simple list of domains as input, that would not work to assemble a key-value string). Maybe both? I suspect the most cases for this could be handled with option 2, but if more complexity is needed, the option string for option 1 could still be assembled. I'm not sure why the behaviour differs here, maybe because some subdomains are delegated zones. |
But if I remember correctly, weren't the domain_validation_options used for automatic dns validation somewhere? |
@telepath I fixed it. Below is an example how to specify the domain_validaton_options. The output has changed as well, because of the name collision i talked about earlier. I changed the output that used to be called domain_validation_options to "certificate_details" which is a list of objects with the following variables; domain_name, resource_record_name, resource_record_type, resource_record_value, validation_domain, validation_emails, validation_method. So some documentation will have to be changed but it works fine
|
Ok, thanks! |
if you know the length of the list is always going to be the same, then you can hard code it: list(map("domain_name","${var.subject_alternative_names[0],"validation_domain","${var.validation_domain}",map("domain_name",${var.subject_alternative_names[1],"validation_domain","${var.validation_domain}"]) ... not the prettiest solution, but straightforward enough. A more general solution working with arbitrary list lengths would be a good stackoverflow question because I can't think how to do it on the top of my head. maybe something to do with null_resources or templates and doing some trickery with the count parameter? You can't pass a list of maps as an input variable to modules, so there aren't any examples I could find of people doing this. |
Yes, this is a normal limitation of terraform. |
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.
Thanks for this contribution, @scottwinkler. I left some initial review comments on why these changes would prevent us from moving forward with this PR as it breaks backwards compatibility. Please let us know if you have any questions.
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.
We still need to set the old attributes if we are deprecating them. Have you run the acceptance tests?
all acceptance tests are now passing
|
I cannot get DNS validation to work with these changes:
|
@telepath I was not able to replicate the crash. It looks like it is failing on the following line 260 of resource_aws_acm_certificate.go Below is the configuration I used to test the resource and I did not encounter any errors on creation or destruction.
|
Is there still something missing for this to be merged? |
I changed the name of the conflicting attribute from domain_validation_options to validation_options. The original domain_validation_options is left as is for backwards compatibility, but is marked as deprecated. Certificate details is now the data struct that holds all the computed information. It passes all tests and works fine. I also merged all the latest changes with my old code so it is completely up to date. |
@bflad Any update on this being ship-able? This would be super helpful to our team as well! |
Bump -- we could also use this |
Hello, sorry to bump an old thread but is this working now? |
@intxEmeka its been so long that i would have to fix some merge conflicts to get it in working shape again, but if I could get some assurance that this will actually get merged if i make the changes then I will do it. |
After #14199, I believe the resource changes should reduce down to the following (removing all the deprecations,
"validation_domain": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
},
The new |
Is this being actively worked on? |
I would also like to express interest in this ! |
Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding. Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author. For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000. For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide. |
ce0368d
to
e47e878
Compare
@scottwinkler Thanks for the contribution 🎉 👏. |
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 🚀.
% ACM_CERTIFICATE_ROOT_DOMAIN=xxxxxxxx make testacc TESTS=TestAccACMCertificate_emailValidation PKG=acm
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/acm/... -v -count 1 -parallel 20 -run='TestAccACMCertificate_emailValidation' -timeout 180m
=== RUN TestAccACMCertificate_emailValidation
=== PAUSE TestAccACMCertificate_emailValidation
=== CONT TestAccACMCertificate_emailValidation
--- PASS: TestAccACMCertificate_emailValidation (23.21s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/acm 27.081s
% ACM_CERTIFICATE_ROOT_DOMAIN=xxxxxxxx make testacc TESTS=TestAccACMCertificate_dnsValidation PKG=acm
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/acm/... -v -count 1 -parallel 20 -run='TestAccACMCertificate_dnsValidation' -timeout 180m
=== RUN TestAccACMCertificate_dnsValidation
=== PAUSE TestAccACMCertificate_dnsValidation
=== CONT TestAccACMCertificate_dnsValidation
--- PASS: TestAccACMCertificate_dnsValidation (23.85s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/acm 27.797s
% ACM_CERTIFICATE_ROOT_DOMAIN=xxxxxxxx make testacc TESTS=TestAccACMCertificate_root PKG=acm
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/acm/... -v -count 1 -parallel 20 -run='TestAccACMCertificate_root' -timeout 180m
=== RUN TestAccACMCertificate_root
=== PAUSE TestAccACMCertificate_root
=== RUN TestAccACMCertificate_rootAndWildcardSan
=== PAUSE TestAccACMCertificate_rootAndWildcardSan
=== CONT TestAccACMCertificate_root
=== CONT TestAccACMCertificate_rootAndWildcardSan
--- PASS: TestAccACMCertificate_rootAndWildcardSan (24.17s)
--- PASS: TestAccACMCertificate_root (24.23s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/acm 28.255s
% ACM_CERTIFICATE_ROOT_DOMAIN=xxxxxxxx make testacc TESTS=TestAccACMCertificate_validationOptions PKG=acm
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/acm/... -v -count 1 -parallel 20 -run='TestAccACMCertificate_validationOptions' -timeout 180m
=== RUN TestAccACMCertificate_validationOptions
=== PAUSE TestAccACMCertificate_validationOptions
=== CONT TestAccACMCertificate_validationOptions
--- PASS: TestAccACMCertificate_validationOptions (22.18s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/acm 25.957s
@ewbankkit thanks for merging !! And thanks @scottwinkler 👍🏼 |
This functionality has been released in v4.12.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
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. |
Adding an optional input parameter that is present in the sdk but missing from the terraform resource. This allows a user to specify an alternate validation_domain for EMAIL approval.
Closes #3851.