Sort ACM cert subject alternative names and domain validation opts #8708

jtsaito · 2019-05-19T18:31:45Z

This PR is an alternative implementation to #8657 addressing #8657 (comment). It ads a fix for #8531.

The advantage of this implementation in comparison to #8657 is that the subject alternative names (SANs) are also ordered and they will be ordered consistently with the DVOs.

We sort the SANs as specified in the Terraform file and subsequently sort the domain validation options (DVOs) such that the option for the certificate's domain name comes first and the remainder is sorted according to the order of the SANs.

I have included unit tests but I'm not sure if they belong here at all. (Obviously, the are passing.)

I would be grateful if someone could run the acceptance tests as I don't want to handle setting up the email validation.

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 repeatedly non-empty plans after apply.

This release imposes an ordering on both attribute lists. As a consequence, there will be a one-time re-ordering of the elements in the two attribute lists. If you apply this change in a region not affected before, this is the first and only time you will notice this change. As a consequence, you may be asked to recreate resources depending on SANs or DVOs. In regions affected before already, the problem should go away after applying one more time.

Output from acceptance testing:

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

…phabetically

jtsaito · 2019-05-26T22:41:52Z

The SANs are no longer sorted alphabetically. Instead the order given by the user is preserved if possible. My apologies for the inconvenience caused by the earlier sorting.

Anyway, it seems AWS is going to address the issue! (#8531 (comment))

tdmalone · 2019-06-02T05:34:41Z

It looks like the SANs are now possibly randomly sorted when the certificate is first created, but that order is then preserved afterwards. ref #8531 (comment)

jtsaito · 2019-06-03T06:40:14Z

@tdmalone Thanks for investigating this matter in such detail. Based on your observation, I think this fix is still helpful because it would make the sorting unnecessary.

However, I would suggest to wait for one or two weeks more to see if the problem still persists before asking for this PR to be merged.

morancj · 2019-06-12T10:42:32Z

@jtsaito Great work, thanks! Is this expected to handle the case where SAN entries are in different zones?

tdmalone · 2019-06-25T00:51:52Z

Update: in ap-southeast-2 at least, it appears that the initial addressing of the issue for domain_validation_options has regressed again :( It'd be really great to get this fix merged :)

wvidana · 2019-08-02T19:07:35Z

Anything else needed to see this PR merged? People are still facing the issue with subject_alternative_names

zacblazic · 2019-08-19T12:43:51Z

@bflad When can we expect this to be merged?

Even though this is a breaking change, the implicit changes made by AWS to cause this issue have already broken many users' terraform plans. We've been patiently waiting for a fix for a long while now.

kraduk · 2019-09-04T14:05:27Z

this is still a major headache, when is it getting merged?

aeschright · 2019-09-04T20:16:42Z

Hi everyone, there's some internal discussion we need to have about the underlying issue and preferred fixes. In general, anything labeled breaking change would not be merged until the next major release. This is on our radar, though, and we'll post an update as soon as we can. Thanks for digging into the issue! If you want to discuss workarounds, the bug report is #8531

dshkyra · 2019-09-20T22:21:01Z

aws/resource_aws_acm_certificate.go

 if err != nil {
 return resource.RetryableError(err)
 }
+ sortDomainValidationOptions(d.Get("domain_name").(*string), sans, domainValidationOptions)


I tried to use this patch with terraform-provider-aws v2.23.0, but unfortunately it failed with an error.
I'm not an expert in golang, however it looks to me that d.Get("domain_name") returns just string, not *string.
So changing the type of domainName from *string to string worked for me.

dshkyra · 2019-09-24T18:02:32Z

aws/resource_aws_acm_certificate.go

+ sans := make([]string, 0)
+
+ // intersection of all SANs, user defined (ud) and remote. ordered as user defined (ud) in state.
+ for _, ud := range d.Get("subject_alternative_names").([]interface{}) {


I like this idea to have the certificates ordered as defined by user.
Just want to highlight that when I tried to use this solution (with terraform 0.11.4 and based on terraform aws provider 2.23) SANs were not ordered as defined by user because d.Get("subject_alternative_names") returned an empty list during the refresh.

I tried to use this PR as well, and had issues with it. I have created a new pull request that should address the problems: #10791

kcburge · 2019-11-07T18:51:54Z

Just wanted to add to this discussion that I addressed the issues with this pull request, here: #10791

This pull request is similar to, and was based on, hashicorp#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. hashicorp#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 succesfully from creation (as I understand it), and about the only way to assure the object is created successfully 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 hashicorp#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: hashicorp#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)

Bharathkumarraju · 2020-02-15T11:39:53Z

is this been released already? I am facing this issue ..i.e. if i add additional SAN names it is trying to recreate ACM validation again and tried to delete the old ACM one which has been already used in ALB https listener?

This pull request is similar to, and was based on, hashicorp#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. hashicorp#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 succesfully from creation (as I understand it), and about the only way to assure the object is created successfully 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 hashicorp#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: hashicorp#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)

jtsaito · 2020-05-20T12:40:08Z

This is ancient, so I'm closing this.

ghost · 2020-06-19T17:12:16Z

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!

Sort ACM cert subject alternative names and domain validation opts al…

a40ac13

…phabetically

ghost added size/M Managed by automation to categorize the size of a PR. service/acm Issues and PRs that pertain to the acm service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels May 19, 2019

jtsaito mentioned this pull request May 19, 2019

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

Closed

jtsaito changed the title ~~[WIP] Sort ACM cert subject alternative names and domain validation opts al…~~ Sort ACM cert subject alternative names and domain validation opts al… May 20, 2019

edulop91 mentioned this pull request May 23, 2019

aws_acm_certificate resource domain_validation_options output not stable #8748

Closed

Switch from alphabetical sorting of SANs to user defined.

7e26bf9

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels May 26, 2019

jtsaito changed the title ~~Sort ACM cert subject alternative names and domain validation opts al…~~ Sort ACM cert subject alternative names and domain validation opts May 26, 2019

bflad added breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. upstream Addresses functionality related to the cloud provider. labels Jun 7, 2019

tdmalone mentioned this pull request Jun 25, 2019

aws_acm_certificate subject_alternative_names & domain_validation_options get returned in a different order each time #8531

Closed

aeschright requested a review from a team June 28, 2019 18:16

dshkyra reviewed Sep 20, 2019

View reviewed changes

dshkyra reviewed Sep 24, 2019

View reviewed changes

kcburge mentioned this pull request Nov 8, 2019

Sort ACM cert subject alternative names and domain validation option #10791

Closed

n3ph mentioned this pull request Dec 15, 2019

aws_acm_certificate: fix subject_alternative_names forces recreation #11300

Merged

bflad mentioned this pull request May 18, 2020

Research: aws_acm_certificate redesign #13053

Closed

jtsaito closed this May 20, 2020

jtsaito deleted the fix-acm-options-sans-sort branch May 20, 2020 12:40

ghost locked and limited conversation to collaborators Jun 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort ACM cert subject alternative names and domain validation opts #8708

Sort ACM cert subject alternative names and domain validation opts #8708

jtsaito commented May 19, 2019 •

edited

Loading

jtsaito commented May 26, 2019 •

edited

Loading

tdmalone commented Jun 2, 2019

jtsaito commented Jun 3, 2019

morancj commented Jun 12, 2019

tdmalone commented Jun 25, 2019

wvidana commented Aug 2, 2019

zacblazic commented Aug 19, 2019

kraduk commented Sep 4, 2019

aeschright commented Sep 4, 2019

dshkyra Sep 20, 2019

dshkyra Sep 24, 2019

kcburge Nov 7, 2019

kcburge commented Nov 7, 2019

Bharathkumarraju commented Feb 15, 2020

jtsaito commented May 20, 2020

ghost commented Jun 19, 2020

Sort ACM cert subject alternative names and domain validation opts #8708

Sort ACM cert subject alternative names and domain validation opts #8708

Conversation

jtsaito commented May 19, 2019 • edited Loading

Community Note

jtsaito commented May 26, 2019 • edited Loading

tdmalone commented Jun 2, 2019

jtsaito commented Jun 3, 2019

morancj commented Jun 12, 2019

tdmalone commented Jun 25, 2019

wvidana commented Aug 2, 2019

zacblazic commented Aug 19, 2019

kraduk commented Sep 4, 2019

aeschright commented Sep 4, 2019

dshkyra Sep 20, 2019

Choose a reason for hiding this comment

dshkyra Sep 24, 2019

Choose a reason for hiding this comment

kcburge Nov 7, 2019

Choose a reason for hiding this comment

kcburge commented Nov 7, 2019

Bharathkumarraju commented Feb 15, 2020

jtsaito commented May 20, 2020

ghost commented Jun 19, 2020

jtsaito commented May 19, 2019 •

edited

Loading

jtsaito commented May 26, 2019 •

edited

Loading