Sort ACM cert subject alternative names and domain validation option

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 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 #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)

aws/resource_aws_acm_certificate.go

@@ -258,15 +258,26 @@ func resourceAwsAcmCertificateRead(d *schema.ResourceData, meta interface{}) err
 		d.Set("arn", resp.Certificate.CertificateArn)
 		d.Set("certificate_authority_arn", resp.Certificate.CertificateAuthorityArn)
 
-		if err := d.Set("subject_alternative_names", cleanUpSubjectAlternativeNames(resp.Certificate)); err != nil {
-			return resource.NonRetryableError(err)
+		sans, err := cleanUpSubjectAlternativeNames(resp.Certificate, d)
+		if err != nil {
+			return resource.RetryableError(err)
 		}
 
 		domainValidationOptions, emailValidationOptions, err := convertValidationOptions(resp.Certificate)
 
 		if err != nil {
 			return resource.RetryableError(err)
 		}
+		sortDomainValidationOptions(resp.Certificate.DomainName, sans, domainValidationOptions)
+
+		// subject_alternative_names MUST be set after the above
+		// RetryableError, since sometimes the
+		// subject_alternative_names response from AWS is empty. We
+		// must ensure the domain validation options are good before
+		// actually storing this value, since we read its value up above.
+		if err := d.Set("subject_alternative_names", sans); err != nil {
+			return resource.NonRetryableError(err)
+		}
 
 		if err := d.Set("domain_validation_options", domainValidationOptions); err != nil {
 			return resource.NonRetryableError(err)
@@ -296,6 +307,7 @@ func resourceAwsAcmCertificateRead(d *schema.ResourceData, meta interface{}) err
 		return nil
 	})
 }
+
 func resourceAwsAcmCertificateGuessValidationMethod(domainValidationOptions []map[string]interface{}, emailValidationOptions []string) string {
 	// The DescribeCertificate Response doesn't have information on what validation method was used
 	// so we need to guess from the validation options we see...
@@ -327,16 +339,43 @@ func resourceAwsAcmCertificateUpdate(d *schema.ResourceData, meta interface{}) e
 	return resourceAwsAcmCertificateRead(d, meta)
 }
 
-func cleanUpSubjectAlternativeNames(cert *acm.CertificateDetail) []string {
+func cleanUpSubjectAlternativeNames(cert *acm.CertificateDetail, d *schema.ResourceData) ([]string, error) {
+	if len(cert.SubjectAlternativeNames) == 0 && aws.StringValue(cert.Status) == acm.DomainStatusPendingValidation {
+		log.Printf("[DEBUG] No subject alternative names, need to retry.")
+		return nil, fmt.Errorf("No subject alternative names, need to retry.")
+	}
+
 	sans := cert.SubjectAlternativeNames
 	vs := make([]string, 0)
-	for _, v := range sans {
-		if aws.StringValue(v) != aws.StringValue(cert.DomainName) {
-			vs = append(vs, aws.StringValue(v))
+	dn := aws.StringValue(cert.DomainName)
+
+	// create unique list of remote sans
+	m := make(map[string]bool)
+	for _, r := range sans {
+		v := aws.StringValue(r)
+		if v != dn {
+			m[v] = true
+		}
+	}
+
+	// add sans in existing order, noting sans we saw
+	for _, r := range d.Get("subject_alternative_names").([]interface{}) {
+		v := strings.TrimSuffix(r.(string), ".")
+		_, found := m[v]
+		if found {
+			delete(m, v)
+			vs = append(vs, v)
 		}
 	}
-	return vs
 
+	// add any remote sans not already in subject_alternative_names
+	for v := range m {
+		if v != dn {
+			vs = append(vs, v)
+		}
+	}
+
+	return vs, nil
 }
 
 func convertValidationOptions(certificate *acm.CertificateDetail) ([]map[string]interface{}, []string, error) {
@@ -346,8 +385,8 @@ func convertValidationOptions(certificate *acm.CertificateDetail) ([]map[string]
 	switch aws.StringValue(certificate.Type) {
 	case acm.CertificateTypeAmazonIssued:
 		if len(certificate.DomainValidationOptions) == 0 && aws.StringValue(certificate.Status) == acm.DomainStatusPendingValidation {
-			log.Printf("[DEBUG] No validation options need to retry.")
-			return nil, nil, fmt.Errorf("No validation options need to retry.")
+			log.Printf("[DEBUG] No validation options, need to retry.")
+			return nil, nil, fmt.Errorf("No validation options, need to retry.")
 		}
 		for _, o := range certificate.DomainValidationOptions {
 			if o.ResourceRecord != nil {
@@ -363,8 +402,8 @@ func convertValidationOptions(certificate *acm.CertificateDetail) ([]map[string]
 					emailValidationResult = append(emailValidationResult, *validationEmail)
 				}
 			} else if o.ValidationStatus == nil || aws.StringValue(o.ValidationStatus) == acm.DomainStatusPendingValidation {
-				log.Printf("[DEBUG] No validation options need to retry: %#v", o)
-				return nil, nil, fmt.Errorf("No validation options need to retry: %#v", o)
+				log.Printf("[DEBUG] No validation options, need to retry: %#v", o)
+				return nil, nil, fmt.Errorf("No validation options, need to retry: %#v", o)
 			}
 		}
 	case acm.CertificateTypePrivate:
@@ -378,6 +417,30 @@ func convertValidationOptions(certificate *acm.CertificateDetail) ([]map[string]
 	return domainValidationResult, emailValidationResult, nil
 }
 
+// sortDomainValidationOptions sorts a slice of domain validation
+// options according to the order of domain names in the subject
+// alternative names slice
+func sortDomainValidationOptions(domainName *string, subjectAlternativeNames []string, domainValidationOptions []map[string]interface{}) {
+	sans := append([]string{*domainName}, subjectAlternativeNames...)
+
+	// validation method is email and domainValidationOptions is empty
+	if len(sans) != len(domainValidationOptions) {
+		return
+	}
+
+	// This works because the requesting a certificate is either by email or by dns,
+	// but not mixed and furthermore the method cannot be changed after creation.
+	for i, san := range sans {
+		for j, opt := range domainValidationOptions {
+			if san == opt["domain_name"].(string) {
+				domainValidationOptions[i], domainValidationOptions[j] =
+					domainValidationOptions[j], domainValidationOptions[i]
+				break
+			}
+		}
+	}
+}
+
 func resourceAwsAcmCertificateDelete(d *schema.ResourceData, meta interface{}) error {
 	acmconn := meta.(*AWSClient).acmconn