resource/aws_acm_certificate_validation: Fix root and wildcard validation_record_fqdns handling #3366

bflad · 2018-02-14T07:03:28Z

Fixes #3329 (and heavily augments the ACM testing)

ACM will return the same validation record name across two domain validation objects when creating a certificate for the root domain and wildcard for the same domain. Previously validation_record_fqdns (always a set of 1 in this case) was trying to deeply equal the domain validation record names (a list of 2), which is impossible to workaround. This PR makes the domain validation record names a set for the validation. This also updates the error handling.

Previously the error handling returned (this is the bug that was fixed):

--- FAIL: TestAccAwsAcmCertificateValidation_validationRecordFqdnsRootAndWildcard (128.89s)
	testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:

		* aws_acm_certificate_validation.cert: 1 error(s) occurred:

		* aws_acm_certificate_validation.cert: Certificate needs [_219c781bc2b7b0e36289413e77c82bbc.example.com _219c781bc2b7b0e36289413e77c82bbc.example.com] to be set but only [_219c781bc2b7b0e36289413e77c82bbc.example.com] was passed to validation_record_fqdns

Now returns missing records via multierror along with the associated domain name, which should be very helpful for those with high SAN count:

		* aws_acm_certificate_validation.cert: 2 error(s) occurred:

		* missing tf-acc-8703199145279883318.example.com DNS validation record: _b700c37a7e02189fd014362b37e2ba68.tf-acc-8703199145279883318.example.com
		* missing tf-acc-san-8703199145279883318.example.com DNS validation record: _ee97edac7b32d661f2e43e4cbc99eae6.tf-acc-san-8703199145279883318.example.com

Passes testing locally:

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcmCertificate_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcmCertificate_ -timeout 120m
=== RUN   TestAccAwsAcmCertificate_emailValidation
--- PASS: TestAccAwsAcmCertificate_emailValidation (17.08s)
=== RUN   TestAccAwsAcmCertificate_dnsValidation
--- PASS: TestAccAwsAcmCertificate_dnsValidation (16.44s)
=== RUN   TestAccAwsAcmCertificate_root
--- PASS: TestAccAwsAcmCertificate_root (15.04s)
=== RUN   TestAccAwsAcmCertificate_rootAndWildcardSubjectAlternativeName
--- PASS: TestAccAwsAcmCertificate_rootAndWildcardSubjectAlternativeName (19.28s)
=== RUN   TestAccAwsAcmCertificate_subjectAlternativeName
--- PASS: TestAccAwsAcmCertificate_subjectAlternativeName (21.36s)
=== RUN   TestAccAwsAcmCertificate_subjectAlternativeNames
--- PASS: TestAccAwsAcmCertificate_subjectAlternativeNames (18.03s)
=== RUN   TestAccAwsAcmCertificate_wildcard
--- PASS: TestAccAwsAcmCertificate_wildcard (14.93s)
=== RUN   TestAccAwsAcmCertificate_wildcardAndRootSubjectAlternativeName
--- PASS: TestAccAwsAcmCertificate_wildcardAndRootSubjectAlternativeName (19.80s)
=== RUN   TestAccAwsAcmCertificate_tags
--- PASS: TestAccAwsAcmCertificate_tags (40.03s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	182.029s

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcmCertificateValidation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcmCertificateValidation_ -timeout 120m
=== RUN   TestAccAwsAcmCertificateValidation_basic
--- PASS: TestAccAwsAcmCertificateValidation_basic (227.62s)
=== RUN   TestAccAwsAcmCertificateValidation_validationRecordFqdns
--- PASS: TestAccAwsAcmCertificateValidation_validationRecordFqdns (140.92s)
=== RUN   TestAccAwsAcmCertificateValidation_validationRecordFqdnsRoot
--- PASS: TestAccAwsAcmCertificateValidation_validationRecordFqdnsRoot (196.60s)
=== RUN   TestAccAwsAcmCertificateValidation_validationRecordFqdnsRootAndWildcard
--- PASS: TestAccAwsAcmCertificateValidation_validationRecordFqdnsRootAndWildcard (146.92s)
=== RUN   TestAccAwsAcmCertificateValidation_validationRecordFqdnsSan
--- PASS: TestAccAwsAcmCertificateValidation_validationRecordFqdnsSan (212.38s)
=== RUN   TestAccAwsAcmCertificateValidation_validationRecordFqdnsWildcard
--- PASS: TestAccAwsAcmCertificateValidation_validationRecordFqdnsWildcard (171.43s)
=== RUN   TestAccAwsAcmCertificateValidation_validationRecordFqdnsWildcardAndRoot
--- PASS: TestAccAwsAcmCertificateValidation_validationRecordFqdnsWildcardAndRoot (147.40s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1243.333s

…tion_record_fqdns handling and augment ACM testing

radeksimko

Just a few nitpicks, overall LGTM.

I couldn't run acceptance tests to actually verify this works yet though. I'm in the process of getting us a dedicated domain we could use for this testing.

radeksimko · 2018-02-16T11:31:02Z

aws/resource_aws_acm_certificate_test.go

 					resource.TestCheckResourceAttr("aws_acm_certificate.cert", "subject_alternative_names.#", "0"),
+					resource.TestCheckResourceAttr("aws_acm_certificate.cert", "validation_emails.#", "0"),


radeksimko · 2018-02-16T11:33:43Z

aws/resource_aws_acm_certificate_test.go

+	})
+}
+
+func TestAccAwsAcmCertificate_subjectAlternativeName(t *testing.T) {


Nitpick: I'd perhaps pick different names for TestAccAwsAcmCertificate_subjectAlternativeName and TestAccAwsAcmCertificate_subjectAlternativeNames below so we can run the first one in isolation if we want to.

e.g. TestAccAwsAcmCertificate_subjectAlternativeName_single and TestAccAwsAcmCertificate_subjectAlternativeName_multiple or we can shorten subjectAlternativeName to SAN as it's very common to do so.

I'll do both 😄

radeksimko · 2018-02-16T11:36:03Z

aws/resource_aws_acm_certificate_validation_test.go

@@ -31,64 +25,225 @@ func TestAccAwsAcmCertificateValidation_basic(t *testing.T) {
 		Steps: []resource.TestStep{
 			// Test that validation times out if certificate can't be validated
 			resource.TestStep{
-				Config:      testAccAcmCertificateValidation_basic(domain),
+				Config:      testAccAcmCertificateValidation_timeout(domain),


Nitpick: I'd expect _basic test to be actually basic and this timeout logic to be exercised perhaps in a separate test.

Valid point, I'll separate

radeksimko · 2018-02-16T11:36:51Z

aws/resource_aws_acm_certificate_validation_test.go

 	return fmt.Sprintf(`
 %s

 resource "aws_acm_certificate_validation" "cert" {
  certificate_arn = "${aws_acm_certificate.cert.arn}"
  timeouts {
-    create = "20s"
+    create = "5s"


radeksimko · 2018-02-16T12:02:10Z

aws/resource_aws_acm_certificate_test.go

 	if os.Getenv("ACM_CERTIFICATE_ROOT_DOMAIN") == "" {
 		t.Skip("Environment variable ACM_CERTIFICATE_ROOT_DOMAIN is not set")
 	}
+	return os.Getenv("ACM_CERTIFICATE_ROOT_DOMAIN")


Can you add any brief instructions in a comment on what is this domain in case it's not clear from the ENV variable name?

e.g.

// ACM_CERTIFICATE_ROOT_DOMAIN is a domain that was previously registered // and successfully verified via ACM either via email or DNS // See https://us-west-2.console.aws.amazon.com/acm/home

Also I assume the verification only needs to be done for the TLD (root) domain (e.g. terraformtesting.com), not for SAN or anything else?

Unfortunately, its more complicated then that. Its per domain in each certificate request done via email validation. So if I request foo.example.com and bar.example.com with email validation, I will get two approval emails for that single request. If I go to renew them, I have to click the emails again. Hence why everyone was clamoring for DNS validation 😁

But I'll definitely add information about the environment variable, its non-descriptive at the moment.

bflad · 2018-02-27T13:08:04Z

This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-07T17:50:07Z

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!

resource/aws_acm_certificate_validation: Fix root and wildcard valida…

d201aae

…tion_record_fqdns handling and augment ACM testing

bflad added bug Addresses a defect in current functionality. service/acm Issues and PRs that pertain to the acm service. labels Feb 14, 2018

bflad added this to the v1.10.0 milestone Feb 14, 2018

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 14, 2018

This was referenced Feb 14, 2018

aws_acm_certificate_validation.validation_record_fqdns is not checked correctly #3329

Closed

Bug in 1.9.0 when creating route53 entries for ACM approvals #3362

Closed

radeksimko approved these changes Feb 16, 2018

View reviewed changes

resource/aws_acm_certificate_validation: #3366 PR review comments

ec6f2eb

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Feb 16, 2018

bflad merged commit 1bedfb2 into master Feb 16, 2018

bflad deleted the t-aws_acm_certificate-augment-testing branch February 16, 2018 15:24

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

Update CHANGELOG for #3366

fea2dcb

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_acm_certificate_validation: Fix root and wildcard validation_record_fqdns handling #3366

resource/aws_acm_certificate_validation: Fix root and wildcard validation_record_fqdns handling #3366

bflad commented Feb 14, 2018

radeksimko left a comment

radeksimko Feb 16, 2018

radeksimko Feb 16, 2018

bflad Feb 16, 2018

radeksimko Feb 16, 2018

bflad Feb 16, 2018

radeksimko Feb 16, 2018

radeksimko Feb 16, 2018

bflad Feb 16, 2018

bflad Feb 16, 2018

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

		resource.TestCheckResourceAttr("aws_acm_certificate.cert", "subject_alternative_names.#", "0"),
		resource.TestCheckResourceAttr("aws_acm_certificate.cert", "validation_emails.#", "0"),

resource/aws_acm_certificate_validation: Fix root and wildcard validation_record_fqdns handling #3366

resource/aws_acm_certificate_validation: Fix root and wildcard validation_record_fqdns handling #3366

Conversation

bflad commented Feb 14, 2018

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020