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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
r/aws_acm_certificate: Add resource attribute checks to tests to make…
… expectations more explicit
flosell committed Feb 2, 2018
commit 263d9b1b98fcb60271eb5ec1bfe3b60dbe5e3f5e
14 changes: 14 additions & 0 deletions aws/resource_aws_acm_certificate_test.go
Original file line number Diff line number Diff line change
@@ -40,8 +40,17 @@ func TestAccAwsAcmResource_certificateIssuingFlow(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckAcmCertificateExists("aws_acm_certificate.cert", &conf, &tags),
testAccCheckAcmCertificateAttributes("aws_acm_certificate.cert", &conf, domain, sanDomain, "PENDING_VALIDATION"),
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 instead just check the Terraform resource state here? Its much simpler and they won't be correct in there unless it the read function did the correct thing. e.g.

resource.TestMatchResourceAttr(resourceName, "arn", regexp.MustCompile(`^arn:[^:]+:acm:[^:]+:[^:]:certificate/.+$`)),
resource.TestCheckResourceAttr(resourceName, "domain_name", domain),
resource.TestCheckResourceAttr(resourceName, "subject_alternative_names.#", "1"),
resource.TestCheckResourceAttr(resourceName, "subject_alternative_names.0", sanDomain),
resource.TestCheckResourceAttr(resourceName, "tags.%", "2"),
resource.TestCheckResourceAttr(resourceName, "tags.Hello", "World"),
resource.TestCheckResourceAttr(resourceName, "tags.Foo", "Bar"),

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 found this pattern in some other tests, e.g. testAccCheckAWSENIAttributes.
I guess it's a bit more paranoid in the sense that it checks the actual state in AWS without relying on the read operation. In theory, a no-op create and read would also result in the correct resource attributes in the state since they are initially populated from the resource declaration in the TF config, right?

Is that worth the extra complexity? I'm not sure...
For now I added the checks you suggested, they definitely make the the expectations more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally speaking we rely on the resource state checks (such as resource.TestCheckResourceAttr) as the acceptance testing is already paranoid about refreshing the resources from AWS. For many AWS services, having the additional checks makes the acceptance testing more susceptible to eventual consistency issues. Some of the older provider testing code may have predated the state helpers or just was never converted over.

I would really consider getting rid of these unless there's some reason to keep them, which generally means an attribute should be added.

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 custom checks. I don't think there's anything missing except the check that the certificate is in state ISSUED but the validation resource checks that.


testAccCheckTagsACM(&tags.Tags, "Hello", "World"),
testAccCheckTagsACM(&tags.Tags, "Foo", "Bar"),

resource.TestMatchResourceAttr("aws_acm_certificate.cert", "arn", regexp.MustCompile(`^arn:aws:acm:[^:]+:[^:]+:certificate/.+$`)),
resource.TestCheckResourceAttr("aws_acm_certificate.cert", "domain_name", domain),
resource.TestCheckResourceAttr("aws_acm_certificate.cert", "subject_alternative_names.#", "1"),
resource.TestCheckResourceAttr("aws_acm_certificate.cert", "subject_alternative_names.0", sanDomain),
resource.TestCheckResourceAttr("aws_acm_certificate.cert", "tags.%", "2"),
resource.TestCheckResourceAttr("aws_acm_certificate.cert", "tags.Hello", "World"),
resource.TestCheckResourceAttr("aws_acm_certificate.cert", "tags.Foo", "Bar"),
),
},
// Test that we can change the tags
@@ -50,8 +59,13 @@ func TestAccAwsAcmResource_certificateIssuingFlow(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckAcmCertificateExists("aws_acm_certificate.cert", &conf, &tags),
testAccCheckAcmCertificateAttributes("aws_acm_certificate.cert", &conf, domain, sanDomain, "PENDING_VALIDATION"),

testAccCheckTagsACM(&tags.Tags, "Environment", "Test"),
testAccCheckTagsACM(&tags.Tags, "Foo", "Baz"),

resource.TestCheckResourceAttr("aws_acm_certificate.cert", "tags.%", "2"),
resource.TestCheckResourceAttr("aws_acm_certificate.cert", "tags.Environment", "Test"),
resource.TestCheckResourceAttr("aws_acm_certificate.cert", "tags.Foo", "Baz"),
),
},
// Test that validation times out if certificate can't be validated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you break this out into a separate acceptance test and move it into resource_aws_certificate_validation_test.go? I would expect a _basic test in resource_aws_certificate_test.go to only check all the relevant create/update/delete/import logic of a certificate, potentially even moving tag testing to a separate test as well since its an optional attribute.

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 agree in principle, there's a catch though: Certificate requests have a limit (twice the number of ACM certificates per region per year so 200 by default but maybe a lot lower) that you hit quickly if you run a lot of tests. It can be increased with approval from the service team but I'm not sure by how much (I got 400 requests per region now).

If we want to run these tests in CI, the HashiCorp account possibly needs a pretty high limit and every test that requests a certificate increases it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please separate any of the validation testing into resource_aws_acm_certificate_validation_test.go

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.