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.

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

Conversation

flosell
Copy link
Contributor

@flosell 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 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

@radeksimko radeksimko added the new-resource Introduces a new resource. label Jan 2, 2018
@bflad bflad added the service/acm Issues and PRs that pertain to the acm service. label Jan 11, 2018
@flosell flosell changed the title [WIP] New Resources: aws_acm_certificate and aws_acm_certificate_validation New Resources: aws_acm_certificate and aws_acm_certificate_validation Jan 20, 2018
@flosell
Copy link
Contributor Author

flosell commented Jan 20, 2018

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

@pawelsocha
Copy link

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

@xyloman
Copy link

xyloman commented Jan 26, 2018

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

@ghost ghost added the size/XXL Managed by automation to categorize the size of a PR. label Jan 30, 2018
@bflad bflad self-assigned this Jan 31, 2018
@bflad
Copy link
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.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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

import (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks for the pointer to isAWSErr!

}
}

data "aws_route53_zone" "zone" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabs and spaces... 😄 Fixed.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 1, 2018
…ng constants and utility methods, removing extraneous code
@ghost ghost added the size/XXL Managed by automation to categorize the size of a PR. label Feb 1, 2018
@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 6, 2018
@bflad bflad self-requested a review February 6, 2018 22:07
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 6, 2018
@flosell
Copy link
Contributor Author

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
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hashicorp:master Feb 7, 2018
@cemo
Copy link

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
Copy link

This will be a massive help to us.

@bflad
Copy link
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
Copy link
Contributor

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
Copy link

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
Copy link

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
Copy link
Contributor

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
Copy link

jnatten commented Feb 14, 2018

@daveadams Thanks, that works perfectly!

@digitalkaoz
Copy link

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
Copy link
Contributor

daveadams commented Feb 28, 2018 via email

@ghost
Copy link

ghost commented Apr 7, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-resource Introduces a new resource. service/acm Issues and PRs that pertain to the acm service. size/XL Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.