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

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
a74249c
Add basic aws_acm_certificate resource
flosell Dec 31, 2017
4e49e55
Add basic aws_acm_certificate_validation resource that waits until a …
flosell Dec 31, 2017
d4ef13a
Add documentation for aws_acm_certificate and aws_acm_certificate_val…
flosell Jan 13, 2018
fc359ec
r/aws_acm_certificate_validation: Add sanity checks
flosell Jan 14, 2018
b1bd46b
r/aws_acm_certificate_validation: Test and clean up support for SANs
flosell Jan 14, 2018
fe230cb
r/aws_acm_certificate: Add ability to import certificates
flosell Jan 20, 2018
bf646ab
r/aws_acm_certificate_validation: Ignore trailing . when validating F…
flosell Jan 20, 2018
32a9137
r/aws_acm_certificate: Validate validation_method
flosell Jan 20, 2018
fa7bc0a
r/aws_acm_certificate: Don't set certificate_arn twice
flosell Jan 20, 2018
259ced4
r/aws_acm_certificate: Add support for tagging
flosell Jan 20, 2018
d81ab23
r/aws_acm_certificate: Make domain name used in acceptance tests conf…
flosell Jan 20, 2018
3afd4fb
Adjust code in documentation to the spec
joelhandwell Jan 30, 2018
65c85fe
r/aws_acm_certificate: Improve code style by organising imports, usi…
flosell Feb 1, 2018
f3b4d69
r/aws_acm_certificate: Change certificate_arn attribute to arn
flosell Feb 1, 2018
77fd92d
r/aws_acm_certificate: Remove error checks for setting string attributes
flosell Feb 1, 2018
ee046e8
r/aws_acm_certificate: Remove uselesss describe-call in delete operation
flosell Feb 1, 2018
010da87
r/aws_acm_certificate: Randomize domains used in acceptance test to b…
flosell Feb 2, 2018
263d9b1
r/aws_acm_certificate: Add resource attribute checks to tests to make…
flosell Feb 2, 2018
2f6219b
r/aws_acm_certificate and r/aws_acm_certificate_validation: Clean up …
flosell Feb 2, 2018
0a4b196
r/aws_acm_certificate_validation: Remove attribute reference since it…
flosell Feb 2, 2018
026193a
r/aws_acm_certificate: Be more tolerant of certificates deleted outsi…
flosell Feb 2, 2018
b37bf16
r/aws_acm_certificate: Add support for EMAIL validation
flosell Feb 6, 2018
88a8595
r/aws_acm_certificate: Refactor if-statement for readability
flosell Feb 6, 2018
533badc
r/aws_acm_certificate: Simplify tests, rely on resource state instead…
flosell Feb 6, 2018
d80b669
r/aws_acm_certificate: Remove unused function
flosell Feb 6, 2018
d4c01e1
r/aws_acm_certificate: Simplify tests, parameterize instead of duplicate
flosell Feb 6, 2018
ff443c1
r/aws_acm_certificate and r/aws_acm_certificate_validation: Split tes…
flosell Feb 6, 2018
e42421a
r/aws_acm_certificate_validation: Use built-in timeout functionality …
flosell Feb 6, 2018
75d784c
r/aws_acm_certificate: Allow importing of certificates that aren't AM…
flosell Feb 6, 2018
b2c070c
r/aws_acm_certificate: Add warning about importing imported certifica…
flosell Feb 6, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ func Provider() terraform.ResourceProvider {
},

ResourcesMap: map[string]*schema.Resource{
"aws_acm_certificate": resourceAwsAcmCertificate(),
"aws_acm_certificate_validation": resourceAwsAcmCertificateValidation(),
"aws_ami": resourceAwsAmi(),
"aws_ami_copy": resourceAwsAmiCopy(),
"aws_ami_from_instance": resourceAwsAmiFromInstance(),
Expand Down
237 changes: 237 additions & 0 deletions aws/resource_aws_acm_certificate.go
Original file line number Diff line number Diff line change
@@ -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!

"fmt"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/acm"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
"log"
"time"
)

func resourceAwsAcmCertificate() *schema.Resource {
return &schema.Resource{
Create: resourceAwsAcmCertificateCreate,
Read: resourceAwsAcmCertificateRead,
Update: resourceAwsAcmCertificateUpdate,
Delete: resourceAwsAcmCertificateDelete,
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},

Schema: map[string]*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.

Can we add status to the resource's schema as a computed string attribute please? This will make acceptance testing a little easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it out, looks like this leads to some interesting interactions: When using the fully automatic validation flow (e.g. with route53 and the acm_certificate_validation resource), the acm_certificate resource completes first (with a PENDING_VALIDATION status), then the route53 record gets added and acm_certificate_validation waits until ACM issues the certificate. Since the acm_certificate resource isn't refreshed at that point, it stays in status PENDING_VALIDATION. After the next terraform run, this state switches to ISSUED.

I haven't seen an immediate impact but I'd guess this can lead to some interesting surprises down the road :)

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

Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"subject_alternative_names": &schema.Schema{
Type: schema.TypeList,
Optional: true,
ForceNew: true,
Elem: &schema.Schema{Type: schema.TypeString},
},
"validation_method": &schema.Schema{
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.

if v.(string) != "DNS" {
errors = append(errors, fmt.Errorf("only validation_method DNS is supported at the moment"))
}
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

Type: schema.TypeString,
Computed: true,
ForceNew: true,
},
"domain_validation_options": &schema.Schema{
Type: schema.TypeList,
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.

Type: schema.TypeString,
Computed: true,
},
"resource_record_name": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"resource_record_type": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"resource_record_value": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
},
},
},
"tags": tagsSchema(),
},
}
}

func resourceAwsAcmCertificateCreate(d *schema.ResourceData, meta interface{}) error {
acmconn := meta.(*AWSClient).acmconn
params := &acm.RequestCertificateInput{
DomainName: aws.String(d.Get("domain_name").(string)),
ValidationMethod: aws.String("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: 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.

Fixed.

}

sans, ok := d.GetOk("subject_alternative_names")
if ok {
sanStrings := sans.([]interface{})
params.SubjectAlternativeNames = expandStringList(sanStrings)
}

log.Printf("[DEBUG] ACM Certificate Request: %#v", params)
resp, err := acmconn.RequestCertificate(params)

if err != nil {
return fmt.Errorf("Error requesting certificate: %s", err)
}

d.SetId(*resp.CertificateArn)
if v, ok := d.GetOk("tags"); ok {
params := &acm.AddTagsToCertificateInput{
CertificateArn: resp.CertificateArn,
Tags: tagsFromMapACM(v.(map[string]interface{})),
}
_, err := acmconn.AddTagsToCertificate(params)

if err != nil {
return fmt.Errorf("Error requesting certificate: %s", err)
}
}

return resourceAwsAcmCertificateRead(d, meta)
}

func resourceAwsAcmCertificateRead(d *schema.ResourceData, meta interface{}) error {
acmconn := meta.(*AWSClient).acmconn

params := &acm.DescribeCertificateInput{
CertificateArn: aws.String(d.Id()),
}

return resource.Retry(time.Duration(1)*time.Minute, func() *resource.RetryError {
resp, err := acmconn.DescribeCertificate(params)

if err != nil {
return resource.NonRetryableError(fmt.Errorf("Error describing certificate: %s", err))
}

if *resp.Certificate.Type != "AMAZON_ISSUED" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a technical reason for limiting this here? Is there some reason I wouldn't be able to import an IMPORTED certificate type?

Otherwise, nitpick: constant available acm.CertificateTypeAmazonIssued to replace "AMAZON_ISSUED"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly because I haven't tested it yet. I'm guessing DomainValidationOptions would be missing so similar to support for E-Mail validation we'd need to be more flexible with what data we expect from the API.

For now, I fixed the the nitpick. For the rest, just like E-Mail validation, we have to make the decision to split work on this into a separate PR or work on it in this one.

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, removed the limitation on AMAZON_ISSUED certificates, IMPORTED certificates.

Also added a warning to the docs that this should be used carefully as it might lead to fragile terraform projects: IMPORTED certificates can be imported and managed by terraform but not re-created after being destroyed.

return resource.NonRetryableError(fmt.Errorf("Certificate has type %s, only AMAZON_ISSUED is supported at the moment", *resp.Certificate.Type))
}

if err := d.Set("domain_name", resp.Certificate.DomainName); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: for anything that's a simple d.Set() from a *string, you do not need to do the full err checking (we already handle nil by setting it in the state to "") 👍

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 guess that was me getting paranoid after I missed some errors on other d.Set() calls 😄

return resource.NonRetryableError(err)
}
if err := d.Set("certificate_arn", resp.Certificate.CertificateArn); err != nil {
return resource.NonRetryableError(err)
}
if err := d.Set("validation_method", resp.Certificate.DomainValidationOptions[0].ValidationMethod); err != nil {
return resource.NonRetryableError(err)
}
if err := d.Set("subject_alternative_names", cleanUpSubjectAlternativeNames(resp.Certificate)); err != nil {
return resource.NonRetryableError(err)
}

domainValidationOptions, err := convertDomainValidationOptions(resp.Certificate.DomainValidationOptions)

if err != nil {
return resource.RetryableError(err)
}

if err := d.Set("domain_validation_options", domainValidationOptions); err != nil {
return resource.NonRetryableError(err)
}

params := &acm.ListTagsForCertificateInput{
CertificateArn: aws.String(d.Id()),
}

tagResp, err := acmconn.ListTagsForCertificate(params)
if err := d.Set("tags", tagsToMapACM(tagResp.Tags)); err != nil {
return resource.NonRetryableError(err)
}

return nil
})
}

func resourceAwsAcmCertificateUpdate(d *schema.ResourceData, meta interface{}) error {
if d.HasChange("tags") {
acmconn := meta.(*AWSClient).acmconn
err := setTagsACM(acmconn, d)
if err != nil {
return err
}
}
return nil
}

func cleanUpSubjectAlternativeNames(cert *acm.CertificateDetail) []string {
sans := cert.SubjectAlternativeNames
vs := make([]string, 0, len(sans)-1)
for _, v := range sans {
if *v != *cert.DomainName {
vs = append(vs, *v)
}
}
return vs

}

func convertDomainValidationOptions(validations []*acm.DomainValidation) ([]map[string]interface{}, error) {
var result []map[string]interface{}

for _, o := range validations {
if o.ResourceRecord != nil {
validationOption := map[string]interface{}{
"domain_name": *o.DomainName,
"resource_record_name": *o.ResourceRecord.Name,
"resource_record_type": *o.ResourceRecord.Type,
"resource_record_value": *o.ResourceRecord.Value,
}
result = append(result, validationOption)
} else {
log.Printf("[DEBUG] No resource record found in validation options, need to retry: %#v", o)
return nil, fmt.Errorf("No resource record found in DNS DomainValidationOptions: %v", o)
}
}

return result, nil
}

func resourceAwsAcmCertificateDelete(d *schema.ResourceData, meta interface{}) error {
acmconn := meta.(*AWSClient).acmconn

if err := resourceAwsAcmCertificateRead(d, meta); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a little heavier handed than it needs to be. Can we get away with just catching something like isAWSErr(err, acm.ErrCodeResourceNotFoundException, "") from calling DeleteCertificate? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I probably stole that from the eip resource... I guess it's not really necessary and I fixed it in the way you outlined, which gave me another thought:
The point of this is to tolerate cases where certificates were already deleted outside of terraform, correct? Wouldn't it make more sense to catch ErrCodeResourceNotFoundException in the read-operation instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually do that in a bunch of resources as well. Probably good to add into this one for good measure too 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and by that I mean in both read and delete. Some folks when they get stuck in a really bad configuration for terraform destroy will run the (not advised) -refresh=false, that's why we try to add it to both read and delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, makes sense. Added the check to read in both resources as well.

return err
}
if d.Id() == "" {
// This might happen from the read
return nil
}

params := &acm.DeleteCertificateInput{
CertificateArn: aws.String(d.Id()),
}

_, err := acmconn.DeleteCertificate(params)

if err != nil {
return fmt.Errorf("Error deleting certificate: %s", err)
}

d.SetId("")
return nil
}
Loading