New Resources: aws_acm_certificate and aws_acm_certificate_validation

[flosell]

This is a second draft trying to implement automated ACM certificate issuing based on my suggestions in #2418. It's similar to PR #2801 but uses a resource instead of a data source to implement polling for certificate issuing as a data source implementation seems to have a few pitfalls for the users.

Here is what it does:

• Add a aws_acm_certificate resource that requests a new certificate and returns once validationOptions (the information what needs to be done to validate ownership of the domain) are available (for some reason, those aren't immediately available). Only supports DNS validation at this point
• Add a aws_acm_certificate_validation resource that does some basic sanity checks and then waits for the given certificate to be issued
• Add an acceptance test that runs the whole certificate issuing flow (using route53 for DNS validation)

Current Limitations:

• Only supports DNS validation: E-Mail validation is hard to completely automate and test (and already discussed at length in Add support for AWS ACM resources terraform#4782) so I did not invest time here. It shouldn't be a problem to add this later if there is demand for it
• Only supports certificates that are AMAZON_ISSUED: Importing existing certificate data could be added later if there is demand for it

Test considerations:

Acceptance Tests need a publicly resolvable Route53 zone to make certificate validation work. To run tests, set the ACM_CERTIFICATE_ROOT_DOMAIN environment variable to the domain name of the Route53 zone. The test will use data-providers to look up the zone-id and add CNAMES to allow ACM to validate ownership of the domain:

ACM_CERTIFICATE_ROOT_DOMAIN="sandbox.sellmayr.net" make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcm'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcm -timeout 120m
=== RUN   TestAccAwsAcmCertificateDataSource_noMatchReturnsError
--- PASS: TestAccAwsAcmCertificateDataSource_noMatchReturnsError (20.45s)
=== RUN   TestAccAwsAcmResource_certificateIssuingFlow
--- PASS: TestAccAwsAcmResource_certificateIssuingFlow (324.23s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	344.721s

Here are a few things I'm unsure about:

• Is it a good idea to model waiting for certificate issuing in a resource? I feel it's harder for the user to shoot himself in the foot this way but it's also a bit unintuitive to have a resource that doesn't correspond to any resource in AWS, especially when destroying it: Does destroying a validation destroy the certificate? It doesn't but is that obvious to the user?
• AWS does not expect subject_alternative_names to contain values when requesting a certificate but DescribeCertificate always includes the domain_name in subject_alternative_names. I'm currently filtering this so terraform doesn't detect this as a change. Is there a better way to do this?
• Should we have the validation_method property? It currently only allows DNS as a value so we could do without. However, it probably makes adding new validation methods easier

[flosell]

Removed the [WIP] flag, from my point of view this PR is ready for review.

[pawelsocha]

@bflad what you think? it's possible to merge this PR?

[xyloman]

Agreed we have a new terraform that would benefit from this.

[bflad]

You caught me. 😄 I'm in the ACM neighborhood right now with #1837 and was actually planning on looking at this right after so I didn't have too much context switching. While that one is waiting for some feedback let me at least provide an initial review for this one. More shortly.

[bflad]

Wow this is AWESOME! Thank you so much for all the effort here! This is really off to a great start (I promise, even with all the comments below 😄 ).

I'm leaving this as a first round of feedback of since it is indeed a Size/XXL PR and has two resources (we generally prefer one at a time, but I totally get why this is like this). Admittedly, I have not run the acceptance testing yet or given the validation resource a hardcore line-by-line review like the first since I'd like to shore up the core aws_acm_certificate first.

Please don't hesitate to ask questions or pushback about anything here. Your feedback is just as valuable.

Let me know when this is ready for a round two review. I will keep this high on my priority queue. I am (assuredly among many others) excited to see this ship soon! 🚀

[bflad]

Nitpick: the &schema.Schema for all the attributes here are extraneous since Go 1.7, but since we have a lot of older code you probably saw these other places 😄 (important note: the Elem: &schmema.Schema ones are still necessary)

[flosell]

Fixed.

[bflad]

Nitpick: We try to keep the standard packages up top and third party below in the imports. Most of the maintainers run goimports to automatically organize (and add/remove!) the imports. Its pretty awesome if you haven't tried it, especially as a post-save hook in your editor.

[flosell]

Done, thanks for the pointers to goimports, that helps a lot!

[bflad]

Two things:

• I do not think there's a reason to force only DNS as the only valid method here. If someone wants to create a EMAIL validated certificate with this resource (e.g. for later applying the arn attribute of this resource to other resources), I don't think we should necessarily stop them.
• Nitpick: constant is available acm.ValidationMethodDns instead of "DNS"

[flosell]

Using the constant now.

Supporting E-Mail validation will probably add a bit more work and complexity. It's also hard to test.
At a minimum, we'd need to consider cases where domain validation information isn't present to make sure the provider doesn't break if someone tries to use it with EMAIL.

Since this PR is already pretty big, my initial idea was to add support for E-Mail validation in a separate PR later if there is demand for it. If DNS validation works smoothly, I'd guess most people (unless they can't for some reason) will use it and not bother with E-Mail.

Does that make sense? If you prefer adding E-Mail support right now, I'd also be willing to invest a bit of time to get it in.

[bflad]

Two things about this attribute:

• Can we change it to just arn to match many of our other resources?
• ForceNew is extraneous here with Computed

[flosell]

• Renamed to arn in acm_certificate. I left certificate_arn in acm_certificate_validation for now as I feel it's not quite as obvious which arn is meant there. No strong opinion though, happy to change it if you feel otherwise
• Removed ForceNew

[bflad]

Nitpick: the &schema.Schema in this map are also extraneous 👍

[flosell]

Fixed.

[bflad]

We can simplify this with just: if isAWSErr(err, acm.ErrCodeResourceNotFoundException, "") 👍

[flosell]

Fixed, thanks for the pointer to isAWSErr!

[bflad]

Nitpick: I think the configuration example should include pieces relevant only to this resource itself. The Route 53 bits here don't fit into everyone's situation, would require documentation updates twice potentially, and we already (very nicely!) mention the validation resource above.

[flosell]

Removed. If I understand you correctly we leave the more complete example in the docs for validation? I think otherwise it will be hard to follow how these resources work together (e.g. that you should use the certificate_arn from the aws_acm_certificate_validation resource and not aws_acm_certificate if you want to make sure you depend on an issued certificate).

[bflad]

The following additional attributes are exported: (we've had Github issues about the wording previously 😄 ).
We should also add a mention of:

* `id` - The ARN of the certificate

[flosell]

Fixed. I also removed the attributes reference section from the acm_certificate_validation docs since that resource doesn't export any additional attributes.

[bflad]

If the validation is updated, this should have both DNS and EMAIL and probably a shoutout that automated validation is only possible with DNS at this time 👍

[flosell]

Done.

[bflad]

Nitpick: formatting

[flosell]

tabs and spaces... 😄 Fixed.

[flosell]

@bflad unless I missed something this should be ready for round two:

• Now Supporting E-Mail Validation and and importing certificates that aren't AMAZON_ISSUED
• Cleaned up code and tests

[bflad]

I am going to give this a lovely ✅ ! This is a huge addition to the provider and its at the point where we can triage any little things with it merged in. I have a few testing nitpicks but I'll leave that as a post-merge exercise for myself.

@flosell thank you so much! You just made a bunch of people very happy! 😄

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcmResource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcmResource -timeout 120m
=== RUN   TestAccAwsAcmResource_emailValidation
--- SKIP: TestAccAwsAcmResource_emailValidation (0.00s)
	resource_aws_acm_certificate_test.go:21: Environment variable ACM_CERTIFICATE_ROOT_DOMAIN is not set
=== RUN   TestAccAwsAcmResource_dnsValidationAndTagging
--- SKIP: TestAccAwsAcmResource_dnsValidationAndTagging (0.00s)
	resource_aws_acm_certificate_test.go:57: Environment variable ACM_CERTIFICATE_ROOT_DOMAIN is not set
=== RUN   TestAccAwsAcmResource_certificateIssuingAndValidationFlow
--- SKIP: TestAccAwsAcmResource_certificateIssuingAndValidationFlow (0.00s)
	resource_aws_acm_certificate_validation_test.go:16: Environment variable ACM_CERTIFICATE_ROOT_DOMAIN is not set
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.040s

export ACM_CERTIFICATE_ROOT_DOMAIN=valid-public-route53-domain.tld

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcmResource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcmResource -timeout 120m
=== RUN   TestAccAwsAcmResource_emailValidation
--- PASS: TestAccAwsAcmResource_emailValidation (18.68s)
=== RUN   TestAccAwsAcmResource_dnsValidationAndTagging
--- PASS: TestAccAwsAcmResource_dnsValidationAndTagging (58.55s)
=== RUN   TestAccAwsAcmResource_certificateIssuingAndValidationFlow
--- PASS: TestAccAwsAcmResource_certificateIssuingAndValidationFlow (216.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	293.343s

[cemo]

@bflad and @flosell this is really a very big step. Looking forward to try. Thanks for everything.

[benpriestman]

This will be a massive help to us.

[bflad]

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[daveadams]

Hello, this is great! The example code works fine. But I have one problem. How do I handle creating multiple DNS records for an ACM cert with multiple subject alternative names? All of the facts I need are embedded in the resource's domain_validation_options property, but the values in the maps inside that list are not directly accessible as far as I can tell. Example code:

resource "aws_acm_certificate" "cert" {
  domain_name = "myname.example.com"
  subject_alternative_names = [
    "my-alt-name.example.com",
    "my-other-name.example.com",
  ]
  validation_method = "DNS"
}

output "validation-options" {
  value = "${aws_acm_certificate.cert.domain_validation_options}"
}

This results in this output:

validation-options = [
    {
        domain_name = myname.example.com,
        resource_record_name = _8b1eece1bd9eb24c465d20ee5e902e34.myname.example.com.,
        resource_record_type = CNAME,
        resource_record_value = _7fcc0e9fce650cfc93cf84e9b3afa0a8.acm-validations.aws.
    },
    {
        domain_name = my-alt-name.example.com,
        resource_record_name = _117f0bed1655685f817791a0d446fbaf.my-alt-name.example.com.,
        resource_record_type = CNAME,
        resource_record_value = _eae8c229110499e4c535fde67c6cd8c0.acm-validations.aws.
    },
    {
        domain_name = my-other-name.example.com,
        resource_record_name = _456f43b3eacb2496165b136bda0f47db.my-other-name.example.com.,
        resource_record_type = CNAME,
        resource_record_value = _61efde25808a9a12bbd24ac29ca0308c.acm-validations.aws.
    }
]

Fair enough. But then I am stuck at how to create those three records. My first instinct is to treat this property like a multi-instance resource, and do something like:

resource "aws_route53_record" "cert_validation" {
  count = "${length(aws_acm_certificate.cert.domain_validation_options)}"
  name    = "${element(aws_acm_certificate.cert.domain_validation_options.*.resource_record_name, count.index)}"
  type    = "${element(aws_acm_certificate.cert.domain_validation_options.*.resource_record_type, count.index)}"
  records = ["${element(aws_acm_certificate.cert.domain_validation_options.*.resource_record_value, count.index)}"]
  zone_id = "${data.aws_route53_zone.zone.id}"
  ttl     = 300
}

But that gives me an error: "value of 'count' cannot be computed"

If I tweak things around or even hardcode "count" to the correct value, then I get errors like: "Resource 'aws_acm_certificate.cert' does not have attribute 'domain_validation_options.*.resource_record_type'".

I've tried using locals as interstitial calculators, but I can't figure out how to get, say, an array of just the resource_record_name values, given the cert_validation structure.

I'm also not able to get this to work using just straight local variables instead of the resource attribute. There just don't seem to be sufficient tools in the interpolation language to manipulate this particular data structure. Not sure if it's easier to fix the resource or the interpolation language.

[lucidaeon]

This exactly describes the challenge I was experiencing yesterday. I know you can attach multiple ACM certs to an ALB, but it's nice if I can minimize the number of certificate lifecycles to manage.

[jnatten]

Has anyone been able to create multiple aws_route53_record's for certificates that has multiple domains?

I am unable to dynamically lookup the domain_validation_options
If i try to access aws_acm_certificate_cert.domain_validation_options.0.resource_record_name i get the expected value.

But if i try to access it dynamically in a resource block with count like this:

resource "aws_route53_record" "cert_validation" {
  count = "${length(local.certificate_additional_domains) + 1}" // One record for each additional domain + one for the main domain
  name = "${element(aws_acm_certificate.cert.domain_validation_options.*.resource_record_name, count.index)}"
  [ ... ]
}

terraform plan tells me that:

* module.mymodule.aws_route53_record.cert_validation: At column 3, line 1: element: argument 1 should be type list, got type string in:

${element(aws_acm_certificate.cert.domain_validation_options.*.resource_record_type, count.index)})

If i try to output just aws_acm_certificate.cert.domain_validation_options.*.resource_record_name somewhere the result is 1

[daveadams]

I've found that it works if you use:

name = "${lookup(aws_acm_certificate.cert.domain_validation_options[count.index], "resource_record_name")}"

And similar for the other fields.

[jnatten]

@daveadams Thanks, that works perfectly!

[digitalkaoz]

it doesnt work i you use counts:

variable "apps" {
  type    = "list"
  default = ["foo", "bar"]
}

# dns validation record
resource "aws_route53_record" "live_cert_validation" {
  count   = "${length(var.apps)}"
  name    = "${lookup(element(aws_acm_certificate.live.*.domain_validation_options[count.index], 0), "resource_record_name")}"
  type    = "${lookup(element(aws_acm_certificate.live.*.domain_validation_options[count.index], 0), "resource_record_type")}"
  zone_id = "${data.aws_route53_zone.live.*.id[count.index]}"
  records = ["${lookup(element(aws_acm_certificate.live.*.domain_validation_options[count.index], 0), "resource_record_value")}"]
  ttl     = 60
}

more weird: it works if i create the certificate in one stack, and the certificate dns records and the validation in the next stack. but all in one stack gives me the following error:

* aws_route53_record.live_cert_validation: At column 3, line 1: lookup: argument 1 should be type map, got type string in:

${lookup(element(aws_acm_certificate.live.*.domain_validation_options[count.index], 0), "resource_record_name")}

it tried it in a lot of obscure ways, but nothing seems to work, i guess the interpolation comes in play to early here...

@flosell @bflad any ideas?

[daveadams]

Looks like you have a second "type =" in your route53 definition. You should just have the one pointing to the domain_validation_options record type attribute. I'm guessing it's getting created as the wrong type.

…

On Wed, Feb 28, 2018 at 11:29 AM, Brian Auron ***@***.***> wrote: Hello, I'm trying to get this to work for me in (what I think is) a pretty simple manner: I've created a domain, a certificate request, and a validation request. I can see that the CNAME exists as expected, but the certificate request just sits in Pending Validation status for 72 hours and times out: resource "aws_acm_certificate" "nexus_certificate" { domain_name = "${var.module_name}.${var.route53_zone_name}" validation_method = "DNS" tags { Environment = "${var.module_name}-${var.route53_zone_name}" } } resource "aws_acm_certificate_validation" "nexus_certificate_validation"{ certificate_arn = "${aws_acm_certificate.nexus_certificate.arn}" validation_record_fqdns = ["${aws_route53_record.nexus_cert_validation_record.fqdn}"] } resource "aws_route53_record" "nexus_cert_validation_record" { name = "${aws_acm_certificate.nexus_certificate.domain_validation_options.0.resource_record_name}" type = "${aws_acm_certificate.nexus_certificate.domain_validation_options.0.resource_record_type}" zone_id = "${var.route53_zone_id}" ttl = 60 type = "CNAME" records = ["${aws_acm_certificate.nexus_certificate.domain_validation_options.0.resource_record_value}"] } If anybody has any tips or advice, I'd very much appreciate it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2813 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAETogsexXn940uyq45yJVl-47XmzbqAks5tZYzJgaJpZM4RP30U> .

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!