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

New Resources: aws_acm_certificate and aws_acm_certificate_validation #2813

Merged
merged 30 commits into from Feb 7, 2018

Conversation

Projects
None yet
@flosell
Contributor

flosell commented Dec 31, 2017

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 hashicorp/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 flosell changed the title from [WIP] New Resources: aws_acm_certificate and aws_acm_certificate_validation to New Resources: aws_acm_certificate and aws_acm_certificate_validation Jan 20, 2018

@flosell

This comment has been minimized.

Contributor

flosell commented Jan 20, 2018

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

@pawelsocha

This comment has been minimized.

pawelsocha commented Jan 26, 2018

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

@xyloman

This comment has been minimized.

xyloman commented Jan 26, 2018

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

@hashibot hashibot bot added the size/XXL label Jan 30, 2018

@bflad bflad self-assigned this Jan 31, 2018

@bflad

This comment has been minimized.

Contributor

bflad commented Jan 31, 2018

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! 🚀

},
Schema: map[string]*schema.Schema{
"domain_name": &schema.Schema{

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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)

This comment has been minimized.

@flosell

flosell Feb 1, 2018

Contributor

Fixed.

@@ -0,0 +1,237 @@
package aws
import (

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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.

This comment has been minimized.

@flosell

flosell Feb 1, 2018

Contributor

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

Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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"

This comment has been minimized.

@flosell

flosell Feb 1, 2018

Contributor

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.

return
},
},
"certificate_arn": &schema.Schema{

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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

This comment has been minimized.

@flosell

flosell Feb 1, 2018

Contributor
  • 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
Computed: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"domain_name": &schema.Schema{

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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

This comment has been minimized.

@flosell

flosell Feb 1, 2018

Contributor

Fixed.

}
// Verify the error is what we want
acmerr, ok := err.(awserr.Error)

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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

This comment has been minimized.

@flosell

flosell Feb 1, 2018

Contributor

Fixed, thanks for the pointer to isAWSErr!

}
}
data "aws_route53_zone" "zone" {

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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.

This comment has been minimized.

@flosell

flosell Feb 2, 2018

Contributor

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

## Attributes Reference
The following attributes are exported:

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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

This comment has been minimized.

@flosell

flosell Feb 2, 2018

Contributor

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

* `domain_name` - (Required) A domain name for which the certificate should be issued
* `subject_alternative_names` - (Optional) A list of domains that should be SANs in the issued certificate
* `validation_method` - (Required) Which method to use for validation (only `DNS` is supported at the moment)

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

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 👍

This comment has been minimized.

@flosell

flosell Feb 6, 2018

Contributor

Done.

```hcl
resource "aws_acm_certificate" "cert" {
domain_name = "example.com"
validation_method = "DNS"

This comment has been minimized.

@bflad

bflad Jan 31, 2018

Contributor

Nitpick: formatting

This comment has been minimized.

@flosell

flosell Feb 2, 2018

Contributor

tabs and spaces... 😄 Fixed.

r/aws_acm_certificate: Improve code style by organising imports, usi…
…ng constants and utility methods, removing extraneous code

@hashibot hashibot bot added the size/XXL label Feb 1, 2018

@hashibot hashibot bot added the size/XL label Feb 6, 2018

@bflad bflad removed the waiting-response label Feb 6, 2018

@bflad bflad self-requested a review Feb 6, 2018

@hashibot hashibot bot added the size/XL label Feb 6, 2018

@flosell

This comment has been minimized.

Contributor

flosell commented Feb 6, 2018

@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 bflad added this to the v1.9.0 milestone Feb 7, 2018

@bflad

bflad approved these changes Feb 7, 2018

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

@bflad bflad merged commit 07fa4b3 into terraform-providers:master Feb 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cemo

This comment has been minimized.

cemo commented Feb 7, 2018

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

bflad added a commit that referenced this pull request Feb 7, 2018

@benpriestman

This comment has been minimized.

benpriestman commented Feb 8, 2018

This will be a massive help to us.

@bflad

This comment has been minimized.

Contributor

bflad commented Feb 9, 2018

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

This comment has been minimized.

daveadams commented Feb 10, 2018

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.

@laurentr

This comment has been minimized.

laurentr commented Feb 11, 2018

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

This comment has been minimized.

jnatten commented Feb 14, 2018

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

This comment has been minimized.

daveadams commented Feb 14, 2018

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

This comment has been minimized.

jnatten commented Feb 14, 2018

@daveadams Thanks, that works perfectly!

@digitalkaoz

This comment has been minimized.

digitalkaoz commented Feb 21, 2018

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

This comment has been minimized.

daveadams commented Feb 28, 2018

oarmstrong added a commit to oarmstrong/terraform-provider-aws that referenced this pull request Mar 3, 2018

Update example to use new resource
With terraform-providers#2813, a resource was added for aws_acm_certificate. Update the
documentation's example for aws_lb_listener_certificate to utilise this
new resource instead of the data source that it was previously using.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment