From 73663bfd5b43ed603e720b84e1ca1cd01f1ae27d Mon Sep 17 00:00:00 2001 From: Wilken Rivera Date: Fri, 20 Sep 2019 09:24:43 -0700 Subject: [PATCH] Revert "resource/aws_acm_certificate: Retry logic refactor" (#10184) * Revert "Merge pull request #9863 from terraform-providers/rfd-retry-acm" This reverts commit 4879c29ec772f5393f7653020e43585a7af59f84, reversing changes made to 38fc3b97370dcfccd27761e0654245cafaa5ff98. Before Change ``` --- FAIL: TestAccAWSAcmCertificate_tags (76.71s) testing.go:569: Step 0 error: errors during apply: Error: error getting validation options for ACM certificate: no validation options need to retry. on /tmp/tf-test030858259/main.tf line 2: (source code not available) --- FAIL: TestAccAWSAcmCertificate_san_TrailingPeriod (76.93s) --- FAIL: TestAccAWSAcmCertificate_disableCTLogging (77.52s) --- FAIL: TestAccAWSAcmCertificate_san_single (78.13s) --- FAIL: TestAccAWSAcmCertificate_root_TrailingPeriod (81.75s) --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdnsEmail (74.93s) testing.go:562: Step 0, expected error: errors during apply: error getting validation options for ACM certificate: no validation options need to retry. To match: validation_record_fqdns is only valid for DNS validation --- FAIL: TestAccAWSAcmCertificateValidation_timeout (75.22s) --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdns (77.35s) --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdnsRoot (79.41s) --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdnsWildcardAndRoot (79.65s) --- FAIL: TestAccAWSAcmCertificateValidation_basic (79.71s) --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdnsRootAndWildcard (80.34s) --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdnsSan (80.93s) --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdnsWildcard (82.78s) ``` After Revert ``` --- PASS: TestAccAWSAcmCertificate_imported_IpAddress (24.53s) --- PASS: TestAccAWSAcmCertificate_emailValidation (27.48s) --- PASS: TestAccAWSAcmCertificate_root (27.82s) --- PASS: TestAccAWSAcmCertificate_root_TrailingPeriod (29.32s) --- PASS: TestAccAWSAcmCertificate_dnsValidation (29.96s) --- PASS: TestAccAWSAcmCertificate_disableCTLogging (30.17s) --- PASS: TestAccAWSAcmCertificate_san_TrailingPeriod (30.38s) --- PASS: TestAccAWSAcmCertificate_wildcardAndRootSan (30.80s) --- PASS: TestAccAWSAcmCertificate_wildcard (30.85s) --- PASS: TestAccAWSAcmCertificate_rootAndWildcardSan (32.26s) --- PASS: TestAccAWSAcmCertificate_privateCert (34.07s) --- PASS: TestAccAWSAcmCertificate_san_single (36.02s) --- PASS: TestAccAWSAcmCertificate_imported_DomainName (37.99s) --- PASS: TestAccAWSAcmCertificate_tags (66.36s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsEmail (17.38s) --- PASS: TestAccAWSAcmCertificateValidation_timeout (24.80s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsWildcard (668.03s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsRootAndWildcard (672.25s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsWildcardAndRoot (672.72s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdns (698.98s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsSan (700.61s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsRoot (702.39s) --- PASS: TestAccAWSAcmCertificateValidation_basic (708.19s) ``` * CHANGELOG: remove entry for #9863 --- CHANGELOG.md | 9 ++- aws/resource_aws_acm_certificate.go | 108 ++++++++++++---------------- 2 files changed, 51 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22dc09e1cfb..9f4c7d73070 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,7 @@ ENHANCEMENTS: * resource/aws_waf_rate_based_rule: Update rate based rule limit for WAF ([#9946](https://github.com/terraform-providers/terraform-provider-aws/pull/9946)) * resource/aws_wafregional_rate_based_rule: Update rate based rule limit for WAF ([#9946](https://github.com/terraform-providers/terraform-provider-aws/pull/9946)) -BUG FIXES: -* resource/aws_acm_certificate: Final retries after timeouts reading and deleting certificates [GH-9863] +BUG FIXES: * ecs_task_definition_equivalency: Fix a crash if environment name is missing [GH-10074] ## 2.28.1 (September 12, 2019) @@ -348,7 +347,7 @@ ENHANCEMENTS: BUG FIXES: -* resource/aws_appautoscaling_policy: Properly support importing of dynamodb policies [[#8397](https://github.com/terraform-providers/terraform-provider-aws/issues/8397)] +* resource/aws_appautoscaling_policy: Properly support importing of dynamodb policies [[#8397](https://github.com/terraform-providers/terraform-provider-aws/issues/8397)] * resource/aws_cloudwatch_event_permissions: Clean up error handling when reading event permissions ([#9065](https://github.com/terraform-providers/terraform-provider-aws/issues/9065)) * resource/aws_cloudwatch_event_rule: Retry error handling when creating and updating event rules ([#9065](https://github.com/terraform-providers/terraform-provider-aws/issues/9065)) * resource/aws_cloudwatch_log_destination: Clean up error handling when putting log destination ([#9065](https://github.com/terraform-providers/terraform-provider-aws/issues/9065)) @@ -478,7 +477,7 @@ ENHANCEMENTS: * resource/aws_backup_vault: Support resource import ([#9041](https://github.com/terraform-providers/terraform-provider-aws/issues/9041)) * resource/aws_codepipeline: Add `tags` argument ([#8993](https://github.com/terraform-providers/terraform-provider-aws/issues/8993)) * resource/aws_codepipeline_webhook: Add `tags` argument ([#8993](https://github.com/terraform-providers/terraform-provider-aws/issues/8993)) -* resource/aws_ecs_task_definition: Add `proxy_configuration` configuration block (support AppMesh proxying) [[#8780](https://github.com/terraform-providers/terraform-provider-aws/issues/8780)] +* resource/aws_ecs_task_definition: Add `proxy_configuration` configuration block (support AppMesh proxying) [[#8780](https://github.com/terraform-providers/terraform-provider-aws/issues/8780)] * resource/aws_instance: Prevent panic when `credit_specification` configuration block is missing arguments ([#9003](https://github.com/terraform-providers/terraform-provider-aws/issues/9003)) * resource/aws_organizations_organization: Add `non_master_accounts` attribute ([#8926](https://github.com/terraform-providers/terraform-provider-aws/issues/8926)) * resource/aws_secretsmanager_secret: Tag on create (support tag limiting IAM policies) ([#9023](https://github.com/terraform-providers/terraform-provider-aws/issues/9023)) @@ -639,7 +638,7 @@ ENHANCEMENTS: * resource/aws_elastic_beanstalk_application: Add `tags` argument and `arn` attribute ([#8614](https://github.com/terraform-providers/terraform-provider-aws/issues/8614)) * resource/aws_elastic_beanstalk_application_version: Add `tags` argument and `arn` attribute ([#8614](https://github.com/terraform-providers/terraform-provider-aws/issues/8614)) * resource/aws_emr_cluster: Add `master_instance_group` and `core_instance_group` configuration blocks (deprecates other instance group configuration methods) ([#8459](https://github.com/terraform-providers/terraform-provider-aws/issues/8459)) -* resource/aws_emr_instance_group: Add support for `autoscaling_policy`, `bid_price`, and resource import [[#8078](https://github.com/terraform-providers/terraform-provider-aws/issues/8078)] +* resource/aws_emr_instance_group: Add support for `autoscaling_policy`, `bid_price`, and resource import [[#8078](https://github.com/terraform-providers/terraform-provider-aws/issues/8078)] * resource/aws_kinesis_analytics_application: Add `tags` argument ([#8643](https://github.com/terraform-providers/terraform-provider-aws/issues/8643)) * resource/aws_lambda_function: Support `nodejs10.x` in `runtime` validation ([#8622](https://github.com/terraform-providers/terraform-provider-aws/issues/8622)) * resource/aws_organizations_account: Add parent_id argument (support moving accounts) ([#8583](https://github.com/terraform-providers/terraform-provider-aws/issues/8583)) diff --git a/aws/resource_aws_acm_certificate.go b/aws/resource_aws_acm_certificate.go index 748f1dcbb52..53da3da8343 100644 --- a/aws/resource_aws_acm_certificate.go +++ b/aws/resource_aws_acm_certificate.go @@ -170,7 +170,7 @@ func resourceAwsAcmCertificateCreateImported(d *schema.ResourceData, meta interf acmconn := meta.(*AWSClient).acmconn resp, err := resourceAwsAcmCertificateImport(acmconn, d, false) if err != nil { - return fmt.Errorf("error importing certificate: %s", err) + return fmt.Errorf("Error importing certificate: %s", err) } d.SetId(*resp.CertificateArn) @@ -182,7 +182,7 @@ func resourceAwsAcmCertificateCreateImported(d *schema.ResourceData, meta interf _, err := acmconn.AddTagsToCertificate(params) if err != nil { - return fmt.Errorf("error requesting certificate: %s", err) + return fmt.Errorf("Error requesting certificate: %s", err) } } @@ -217,7 +217,7 @@ func resourceAwsAcmCertificateCreateRequested(d *schema.ResourceData, meta inter resp, err := acmconn.RequestCertificate(params) if err != nil { - return fmt.Errorf("error requesting certificate: %s", err) + return fmt.Errorf("Error requesting certificate: %s", err) } d.SetId(*resp.CertificateArn) @@ -229,7 +229,7 @@ func resourceAwsAcmCertificateCreateRequested(d *schema.ResourceData, meta inter _, err := acmconn.AddTagsToCertificate(params) if err != nil { - return fmt.Errorf("error requesting certificate: %s", err) + return fmt.Errorf("Error requesting certificate: %s", err) } } @@ -243,70 +243,58 @@ func resourceAwsAcmCertificateRead(d *schema.ResourceData, meta interface{}) err CertificateArn: aws.String(d.Id()), } - resp, err := acmconn.DescribeCertificate(params) - if err != nil { - if !isAWSErr(err, acm.ErrCodeResourceNotFoundException, "") { - d.SetId("") - return nil + return resource.Retry(time.Duration(1)*time.Minute, func() *resource.RetryError { + resp, err := acmconn.DescribeCertificate(params) + + if err != nil { + if isAWSErr(err, acm.ErrCodeResourceNotFoundException, "") { + d.SetId("") + return nil + } + return resource.NonRetryableError(fmt.Errorf("Error describing certificate: %s", err)) } - return fmt.Errorf("error describing certificate: %s", err) - } - if resp == nil { - return fmt.Errorf("empty response received when describing ACM certificate") - } - d.Set("domain_name", resp.Certificate.DomainName) - d.Set("arn", resp.Certificate.CertificateArn) - d.Set("certificate_authority_arn", resp.Certificate.CertificateAuthorityArn) + d.Set("domain_name", resp.Certificate.DomainName) + 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 fmt.Errorf("error setting subject alternative nates: %s", err) - } + if err := d.Set("subject_alternative_names", cleanUpSubjectAlternativeNames(resp.Certificate)); err != nil { + return resource.NonRetryableError(err) + } - var domainValidationOptions []map[string]interface{} - var emailValidationOptions []string - err = resource.Retry(time.Duration(1)*time.Minute, func() *resource.RetryError { - var err error - domainValidationOptions, emailValidationOptions, err = convertValidationOptions(resp.Certificate) + domainValidationOptions, emailValidationOptions, err := convertValidationOptions(resp.Certificate) if err != nil { return resource.RetryableError(err) } - return nil - }) - if isResourceTimeoutError(err) { - domainValidationOptions, emailValidationOptions, err = convertValidationOptions(resp.Certificate) - } - if err != nil { - return fmt.Errorf("error getting validation options for ACM certificate: %s", err) - } - if err := d.Set("domain_validation_options", domainValidationOptions); err != nil { - return fmt.Errorf("error setting domain validation options: %s", err) - } - if err := d.Set("validation_emails", emailValidationOptions); err != nil { - return fmt.Errorf("error setting email validation options: %s", err) - } + if err := d.Set("domain_validation_options", domainValidationOptions); err != nil { + return resource.NonRetryableError(err) + } + if err := d.Set("validation_emails", emailValidationOptions); err != nil { + return resource.NonRetryableError(err) + } - d.Set("validation_method", resourceAwsAcmCertificateGuessValidationMethod(domainValidationOptions, emailValidationOptions)) + d.Set("validation_method", resourceAwsAcmCertificateGuessValidationMethod(domainValidationOptions, emailValidationOptions)) - if err := d.Set("options", flattenAcmCertificateOptions(resp.Certificate.Options)); err != nil { - return fmt.Errorf("error setting certificate options: %s", err) - } + if err := d.Set("options", flattenAcmCertificateOptions(resp.Certificate.Options)); err != nil { + return resource.NonRetryableError(fmt.Errorf("error setting certificate options: %s", err)) + } - input := &acm.ListTagsForCertificateInput{ - CertificateArn: aws.String(d.Id()), - } + params := &acm.ListTagsForCertificateInput{ + CertificateArn: aws.String(d.Id()), + } - tagResp, err := acmconn.ListTagsForCertificate(input) - if err != nil { - return fmt.Errorf("error listing tags for certificate (%s): %s", d.Id(), err) - } - if err := d.Set("tags", tagsToMapACM(tagResp.Tags)); err != nil { - return fmt.Errorf("error setting tags for certificate: %s", err) - } + tagResp, err := acmconn.ListTagsForCertificate(params) + if err != nil { + return resource.NonRetryableError(fmt.Errorf("error listing tags for certificate (%s): %s", d.Id(), err)) + } + if err := d.Set("tags", tagsToMapACM(tagResp.Tags)); err != nil { + return resource.NonRetryableError(err) + } - return nil + return nil + }) } func resourceAwsAcmCertificateGuessValidationMethod(domainValidationOptions []map[string]interface{}, emailValidationOptions []string) string { // The DescribeCertificate Response doesn't have information on what validation method was used @@ -326,7 +314,7 @@ func resourceAwsAcmCertificateUpdate(d *schema.ResourceData, meta interface{}) e if d.HasChange("private_key") || d.HasChange("certificate_body") || d.HasChange("certificate_chain") { _, err := resourceAwsAcmCertificateImport(acmconn, d, true) if err != nil { - return fmt.Errorf("error updating certificate: %s", err) + return fmt.Errorf("Error updating certificate: %s", err) } } @@ -359,7 +347,7 @@ func convertValidationOptions(certificate *acm.CertificateDetail) ([]map[string] 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.") + return nil, nil, fmt.Errorf("No validation options need to retry.") } for _, o := range certificate.DomainValidationOptions { if o.ResourceRecord != nil { @@ -376,7 +364,7 @@ func convertValidationOptions(certificate *acm.CertificateDetail) ([]map[string] } } 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) + return nil, nil, fmt.Errorf("No validation options need to retry: %#v", o) } } case acm.CertificateTypePrivate: @@ -410,11 +398,9 @@ func resourceAwsAcmCertificateDelete(d *schema.ResourceData, meta interface{}) e } return nil }) - if isResourceTimeoutError(err) { - _, err = acmconn.DeleteCertificate(params) - } + if err != nil && !isAWSErr(err, acm.ErrCodeResourceNotFoundException, "") { - return fmt.Errorf("error deleting certificate: %s", err) + return fmt.Errorf("Error deleting certificate: %s", err) } return nil