Sort ACM cert subject alternative names and domain validation option

[kcburge]

This pull request is similar to, and was based on, #8708. However, it resolves a few issues I discovered with that patch.

The certificate creation process is clearly asynchronous, and, given that the provider is attempting to read properties of an asynchronously created object, it must poll, retrying, until all critical information is available. #8530, however, expects that this object creation succeeds BEFORE validation is complete, so, we cannot wait until the certificate is status succeeded, OR, wait until the domain validation is complete; however, Terraform requires the state to be intact before returning successfully from creation (as I understand it), and about the only way to assure the object is created correctly is to retry, which is what this resource does.

My updates:

• I added a retry in case the subject alternate names was empty.

• I wait to Set the subject alternate names until after we've received all of the domain validation options (if any), so as to prevent side-effects from retrying.

• Like Sort ACM cert subject alternative names and domain validation opts #8708, this patch sorts the SANs and DVOs according to the order in the original request / terraform state file, so that the order is predictable.

This should address issue: #8531.

If this patch is applied, users will be required to either recreate their certificates, OR, manually edit the terraform state files to ensure that the order in the state file reflects the order in their terraform code.

If found three places that must be edited:

• Reorder domain_validation_options

"domain_validation_options.0.resource_record_name": "domain.com",
"domain_validation_options.0.resource_record_type": "CNAME",
"domain_validation_options.0.resource_record_value": "...",

Replace ".N." in the name with the zero-based index of each domain_validation_options.

• Reorder subject_alternative_names

"subject_alternative_names.0": "*.domain.com"

Replace ".N" in the name with the zero-based index of each subject_alternative_name.

• Reorder aws_route53_record validation resources:

"aws_route53_record.validation.1": {

Replace ".N" with the zero-based index of each route 53 record's domain.

Kevin Burge
Nice, Inc. (https://nice.com)

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Release note for CHANGELOG:

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

[jpSimkins]

Does this also solve the issue with duplicate SSL verfications?

I am referring to the duplicate SSL DNS verification entries for a domain.com and wildcard for same domain *.domain.com.

I am expecting the solution for the ordering to solve this bug as well.

[kcburge]

I have not tested "duplicate SSL DNS verification entries". Is there an open issue that has example terraform? Without that, I don't know if this addresses that issue as well.

[jpSimkins]

I have not tested "duplicate SSL DNS verification entries". Is there an open issue that has example terraform? Without that, I don't know if this addresses that issue as well.

This was reported on this ticket as this really goes belongs to the ordering issue as well: #8531 (comment)

The solution was to use -1 on the count to prevent the issue with duplicate SSL verification entries due to domain.com and *.domain.com having the same values for their verification, creating 2 entries to be validated. The -1 was a way to remove the duplicate but due to random ordering, this proved to be problematic.

It seems most efficient to provide this solution in this patch due to the nature of the issue. Ideally, remove the duplicates IMHO.

[kcburge]

I can confirm that this should resolve the issue you reported, since the ordering is now "guaranteed". I agree that the duplicate domain validation option should not be included in the response, but, this would be a significant breaking change to not include it, so, using the -1 workaround seems to work, as in below:

locals {
  domains = [
    "domain.com",
    "*.cdn.domain.com",
    "*.assets.cdn.domain.com",
    "*.api.domain.com",
    "*.domain.com",
  ]
}

resource "aws_acm_certificate" "this" {
  domain_name               = "${local.domains[0]}"
  subject_alternative_names = "${slice(local.domains, 1, length(local.domains))}"
  validation_method         = "DNS"
}

resource "aws_route53_record" "validation" {
  # exclude *.domain.com from validation since it is a duplicate
  count = "${length(local.domains)-1}"

  name    = "${lookup(aws_acm_certificate.this.domain_validation_options[count.index], "resource_record_name")}"
  type    = "${lookup(aws_acm_certificate.this.domain_validation_options[count.index], "resource_record_type")}"
  zone_id = "${data.terraform_remote_state.account_base.zoneid}"
  records = ["${lookup(aws_acm_certificate.this.domain_validation_options[count.index], "resource_record_value")}"]
  ttl     = 60
}

[CumpsD]

Looking forward to seeing this merged, certificates are a hell in Terraform right now with

[lorengordon]

It seems really weird that even if I sort subject_alternative_names myself, terraform state doesn't keep them in the sorted order. I mean, that's a property of a list, order is important.

resource "aws_acm_certificate" "this" {
  domain_name               = local.domains[0]
  subject_alternative_names = sort(slice(local.domains, 1, length(local.domains)))  # <-- not stored in sorted order
  validation_method         = "DNS"
}

Hopefully this gets merged. It's super annoying that the provider wants to recreate the cert as it is.

[kcburge]

It seems really weird that even if I sort subject_alternative_names myself, terraform state doesn't keep them in the sorted order. I mean, that's a property of a list, order is important.

resource "aws_acm_certificate" "this" {
  domain_name               = local.domains[0]
  subject_alternative_names = sort(slice(local.domains, 1, length(local.domains)))  # <-- not stored in sorted order
  validation_method         = "DNS"
}

Hopefully this gets merged. It's super annoying that the provider wants to recreate the cert as it is.

It is not a matter of the provider not keeping them in sorted order - the provider is not preserving any order. It is taking what it receives from the AWS API and storing that in the state. The AWS API provides no guarantee as to the order, so, sometimes it is the same as was requested, sometimes it isn't. This change tries to provide some assurance that you get out what you put in, so that your associated validation aws_route53_records don't keep getting re-ordered.

[CumpsD]

What is required to get this merged?

[philtrep]

@fcuello-fudo this is blocking our team as well, is there something we can do to help move this along?

[fcuello-fudo]

@fcuello-fudo this is blocking our team as well, is there something we can do to help move this along?

I have no saying on this, just made a review comment. I'd like to see this solved also.

[nathanielram]

I'm going to have to bump this as well. Hopefully can be included in a release soon.

[CumpsD]

Spent 3 hours yesterday re-ordering my certs in TF files because of this again, because AWS decided to return them differently and TF wanted to re-create them.

[kcburge]

Spent 3 hours yesterday re-ordering my certs in TF files because of this again, because AWS decided to return them differently and TF wanted to re-create them.

Did this patch not work for you?

[CumpsD]

Did this patch not work for you?

I didn't build TF myself if that's what you mean? Using the latest stable

[kcburge]

Did this patch not work for you?

I didn't build TF myself if that's what you mean? Using the latest stable

I see. You don't need to rebuild terraform to use this patch. You only have to patch and build the terraform aws provider, and then ensure that your terraform code uses your built version rather than pulling the latest (utilizing the plugin cache and provider "version" setting). This is a bit of a pain, to be sure. But, it is worth it for us to have our plans run cleanly instead of always reporting false changes.

[aaronmell]

@kcburge Can you rerequest the review?

@bflad Can you assist here?

[bflad]

Hi folks 👋 As part of a spiked effort against the aws_acm_certificate resource (#13053), this pull request was holistically reviewed against all open reports and pull requests for the resource. The issues reported as part of #8531 are certainly a major pain and we want to address them with minimal operator changes where possible.

To this end, our typical implementation with schema attributes where the API returns them unordered is to simply switch them from TypeList to TypeSet and any Go type assertion updates from .([]interface{}) to .(*schema.Set).List(). This tells the Terraform Plugin SDK to ignore ordering differences of elements by hashing each of their values. We have found this to be the easiest to maintain in the resource code with the minimal changes noted above and requires no configuration updates for operators except if the attribute is referenced elsewhere (e.g. adding wrapping function calls to maintain sorted list behavior
sort(tolist(aws_acm_certificate.example.subject_alternative_names))).

It looks like #11300 is on the desired implementation path already, so to reduce confusion, we are going to close this pull request to focus efforts there. We want to thank @kcburge for the effort put into this contribution and apologies that this situation has been confusing and frustrating.

Please note that we will likely wait until the version 3.0.0 release in the coming weeks to merge this type of change, however, so this change that affects a large portion of configurations can be well documented and easily guarded against in Terraform modules. The newer resource documentation and version 3 upgrade guide will include details about how to switch to the newer (unordered) configuration, using Terraform 0.12's features.

[ghost]

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!