[WIP] ACM Certificate: sort domain validation options by domain name

[jtsaito]

This PR ads a fix for #8531.

Domain validation options (DVOs) are sorted to guarantee the same order even when the AWS API returns them jumbled up. To order imposed is as follows: (1) the certificate's domain name comes first, (2) the remaining options are sorted by their respective domain_name attribute alphabetically.

This PR does not address the issue of unsorted subject alternative names. However, these may be ignored by lifecycle ignore_changes.

I have included unit tests but I'm not sure if they belong here at all.

I would be grateful if someone could run the acceptance tests.

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

Fixes: #8531

Release note for CHANGELOG:

For several regions [it has been reported](https://github.com/terraform-providers/terraform-provider-aws/issues/8531) that the ACM API returns randomly sorted *subject alternative names* (SANs) and *domain validation options* (DVOs) elements. This resulted in non-empty plans after apply, indefinitely.

The release imposes an ordering on only the DVOs attribute lists. As a consequence, there will be a one-time re-ordering of the elements.

If you apply this change in a region not affected before, this is the first and only time you will notice the change. Dependent resources maybe require re-creating. In regions affected by the API change already, the problem should go away after applying one more time.

You may chose to ignore the changing ordering on the SANs.

Output from acceptance testing:

I did not run the acc tests on this because I did not want to run the email validation. However, I tested the implementation by compiling and using it.

[denisdefreyne]

Question: Would it make sense for the domain validations to be ordered by the matching order in subject_alternative_names? IMO, that would be more predictable from an end-user point of view.

[rifelpet]

It might be worth mentioning in the release notes that users may see a change in ordering the first time they plan and apply after upgrading the provider, especially since the original issue is only impacting certain regions.

[tdmalone]

^ Yes, this definitely shouldn’t be NONE for the release notes. It’s a bug fix that users need to know about.

Thank you for doing this though!!

[jtsaito]

@rifelpet @tdmalone Thanks for your useful feedback. I have added a comment for the release note, please check the updated description above. I have also incorporated it in the alternative implementation #8708.

[jtsaito]

@ddfreyne Thanks for your constructive feedback. I have opened a PR that includes your suggestion to sort domain validation options (DVOs) according to the ordering of the subject alternative names (SANs) (#8708).

The current PR suggests to ignore the problem of unsorted SANs and only sort the domain validation names (the SANs could be ignored with ignore_change). #8708 now has to solve sorting the SANs first and then the DVOs accordingly otherwise both keep changing from apply to apply.

I have a slight preference for #8708 because it keeps both attribute sets consistent and won't require ignore_changes. (However, the problem of unordered SANs seems to be less of a problem the way I read the comments in #8531. Someone will correct me if I'm wrong :) And the first PR has a smaller footprint.)

Please let me know what you think.

[jamescarr]

Thanks, we can really benefit from this fix... we have many people running applies many times a day and each time they see the ACM change they hit the breaks and start asking around. Sinks a bit of time throughout the week! :-)

[jonseymour]

@jtsaito, @tdmalone I'd like to comment that I think that a SANs fix is needed as well.

The ignore_changes workaround for that problem is only acceptable if you never change the list of SANs. It is particularly a problem if you need to edit an existing name. If you do, then there is a chance that required changes won't be applied because of the ignore_changes declaration.

[tdmalone]

@jonseymour I believe the alternative PR mentioned above - #8708 - covers that

[jonseymour]

@tdmalone Understood. My point is that i think #8708 should not be rejected in favour of this PR, because I don't think this PR alone is sufficient to address all the issues raised by the recent change in AWS behaviour - the other issues are real and need to be addressed, otherwise manual, out of band, workarounds will be required in a variety of scenarios which would be counter to the purpose of using terraform in the first place.

[jtsaito]

I'm closing this PR. According to https://github.com/mlafeldt a fix for the issue should be rolled out by AWS until about 1. June 2019. Also, #8708 would now definitely a better solution.

[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!