From 486382fc95746d00a826d5f34c83ca0b2fb451af Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 23 Mar 2021 10:04:26 -0400 Subject: [PATCH 1/2] tests/provider: Bootstrap d.IsNewResource() semgrep rule and fix some A resources to start Reference: https://github.com/hashicorp/terraform-provider-aws/issues/16796 During resource creation, Terraform CLI expects either a properly applied state for the new resource or an error. To signal proper resource existence, the Terraform Plugin SDK uses an underlying resource identifier (set via `d.SetId(/* some value */)`). If for some reason the resource creation is returned without an error, but also without the resource identifier being set, Terraform CLI will return an error such as: ``` Error: Provider produced inconsistent result after apply When applying changes to aws_sns_topic_subscription.sqs, provider "registry.terraform.io/hashicorp/aws" produced an unexpected new value: Root resource was present, but now absent. This is a bug in the provider, which should be reported in the provider's own issue tracker. ``` A typical pattern in resource implementations in the `Create`/`CreateContext` function is to `return` the `Read`/`ReadContext` function at the end to fill in the Terraform State for all attributes. Another typical pattern in resource implementations in the `Read`/`ReadContext` function is to remove the resource from the Terraform State if the remote system returns an error or status that indicates the remote resource no longer exists by explicitly calling `d.SetId("")` and returning no error. If the remote system is not strongly read-after-write consistent (eventually consistent), this means the resource creation can return no error and also return no resource state. To prevent this type of Terraform CLI error, the resource implementation should also check against `d.IsNewResource()` before removing from the Terraform State and returning no error. If that check is `true`, then remote operation error (or one synthesized from the non-existent status) should be returned instead. While adding this check will not fix the resource implementation to handle the eventually consistent nature of the remote system, the error being returned will be less opaque for operators and code maintainers to troubleshoot. Previously: ``` aws/resource_aws_accessanalyzer_analyzer.go severity:warning rule:helper-schema-ResourceData-SetId-empty-without-IsNewResource-check: Calling `d.SetId("")` should ensure `!d.IsNewResource()` is checked first 115: d.SetId("") 116: return nil aws/resource_aws_acm_certificate_validation.go severity:warning rule:helper-schema-ResourceData-SetId-empty-without-IsNewResource-check: Calling `d.SetId("")` should ensure `!d.IsNewResource()` is checked first 181: d.SetId("") 182: return nil aws/resource_aws_acmpca_certificate.go severity:warning rule:helper-schema-ResourceData-SetId-empty-without-IsNewResource-check: Calling `d.SetId("")` should ensure `!d.IsNewResource()` is checked first 175: d.SetId("") 176: return nil aws/resource_aws_acmpca_certificate_authority.go severity:warning rule:helper-schema-ResourceData-SetId-empty-without-IsNewResource-check: Calling `d.SetId("")` should ensure `!d.IsNewResource()` is checked first 329: d.SetId("") 330: return nil -------------------------------------------------------------------------------- 339: d.SetId("") 340: return nil -------------------------------------------------------------------------------- 371: d.SetId("") 372: return nil -------------------------------------------------------------------------------- 398: d.SetId("") 399: return nil aws/resource_aws_ami.go severity:warning rule:helper-schema-ResourceData-SetId-empty-without-IsNewResource-check: Calling `d.SetId("")` should ensure `!d.IsNewResource()` is checked first 344: d.SetId("") 345: return nil -------------------------------------------------------------------------------- 361: d.SetId("") 362: return nil -------------------------------------------------------------------------------- 383: d.SetId("") 384: return nil aws/resource_aws_ami_launch_permission.go severity:warning rule:helper-schema-ResourceData-SetId-empty-without-IsNewResource-check: Calling `d.SetId("")` should ensure `!d.IsNewResource()` is checked first 81: d.SetId("") 82: return nil ``` Output from acceptance testing: ``` --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdnsWildcardAndRoot (4.02s) # alternate testing account issue --- FAIL: TestAccAWSAcmCertificateValidation_validationRecordFqdnsWildcard (4.11s) # alternate testing account issue --- PASS: TestAccAWSAcmCertificateValidation_basic (92.89s) --- PASS: TestAccAWSAcmCertificateValidation_timeout (25.70s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdns (101.67s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsEmail (20.07s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsRoot (92.99s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsRootAndWildcard (63.68s) --- PASS: TestAccAWSAcmCertificateValidation_validationRecordFqdnsSan (88.87s) --- PASS: TestAccAwsAcmpcaCertificate_EndEntityCertificate (35.64s) --- PASS: TestAccAwsAcmpcaCertificate_RootCertificate (28.20s) --- PASS: TestAccAwsAcmpcaCertificate_SubordinateCertificate (38.12s) --- PASS: TestAccAwsAcmpcaCertificate_Validity_Absolute (34.39s) --- PASS: TestAccAwsAcmpcaCertificate_Validity_EndDate (35.95s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_basic (22.75s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_DeleteFromActiveState (24.35s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_disappears (14.37s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_Enabled (53.63s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_RevocationConfiguration_CrlConfiguration_CustomCname (120.00s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_RevocationConfiguration_CrlConfiguration_Enabled (95.90s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_RevocationConfiguration_CrlConfiguration_ExpirationInDays (74.81s) --- PASS: TestAccAwsAcmpcaCertificateAuthority_Tags (62.46s) --- PASS: TestAccAWSAMI_basic (63.47s) --- PASS: TestAccAWSAMI_description (76.70s) --- PASS: TestAccAWSAMI_disappears (60.26s) --- PASS: TestAccAWSAMI_EphemeralBlockDevices (63.85s) --- PASS: TestAccAWSAMI_Gp3BlockDevice (46.78s) --- PASS: TestAccAWSAMI_tags (89.70s) --- PASS: TestAccAWSAMILaunchPermission_basic (337.22s) --- PASS: TestAccAWSAMILaunchPermission_Disappears_AMI (354.06s) --- PASS: TestAccAWSAMILaunchPermission_Disappears_LaunchPermission (336.05s) --- PASS: TestAccAWSAMILaunchPermission_Disappears_LaunchPermission_Public (334.96s) ``` --- .semgrep.yml | 54 +++++++++++++++++++ aws/resource_aws_accessanalyzer_analyzer.go | 3 +- ...resource_aws_acm_certificate_validation.go | 28 +++++++--- aws/resource_aws_acmpca_certificate.go | 18 ++++--- ...source_aws_acmpca_certificate_authority.go | 50 +++++++++-------- aws/resource_aws_ami.go | 10 +++- aws/resource_aws_ami_launch_permission.go | 4 ++ 7 files changed, 130 insertions(+), 37 deletions(-) diff --git a/.semgrep.yml b/.semgrep.yml index 0a8c8c4a7e1b..c373499ea873 100644 --- a/.semgrep.yml +++ b/.semgrep.yml @@ -227,6 +227,60 @@ rules: - pattern: if $VALUE, $OK := d.GetOk($KEY); $OK && len($VALUE.(string)) > 0 { $BODY } severity: WARNING + - id: helper-schema-ResourceData-SetId-empty-without-IsNewResource-check + languages: [go] + message: Calling `d.SetId("")` should ensure `!d.IsNewResource()` is checked first + paths: + exclude: + - aws/resource_aws_api_gateway_*.go + - aws/resource_aws_apigatewayv2_*.go + - aws/resource_aws_app_cookie_stickiness_policy.go + - aws/resource_aws_appautoscaling_*.go + - aws/resource_aws_appmesh_*.go + - aws/resource_aws_appsync_*.go + - aws/resource_aws_athena_*.go + - aws/resource_aws_autoscaling_*.go + - aws/resource_aws_autoscalingplans_scaling_plan.go + - aws/resource_aws_b*.go + - aws/resource_aws_c*.go + - aws/resource_aws_d*.go + - aws/resource_aws_e*.go + - aws/resource_aws_f*.go + - aws/resource_aws_g*.go + - aws/resource_aws_i*.go + - aws/resource_aws_k*.go + - aws/resource_aws_l*.go + - aws/resource_aws_m*.go + - aws/resource_aws_n*.go + - aws/resource_aws_o*.go + - aws/resource_aws_p*.go + - aws/resource_aws_q*.go + - aws/resource_aws_r*.go + - aws/resource_aws_s*.go + - aws/resource_aws_t*.go + - aws/resource_aws_v*.go + - aws/resource_aws_w*.go + - aws/resource_aws_x*.go + include: + - aws/resource*.go + patterns: + - pattern-either: + - pattern: | + d.SetId("") + ... + return nil + - pattern-not-inside: | + if ... { + if <... d.IsNewResource() ...> { ... } + ... + d.SetId("") + ... + return nil + } + - pattern-not-inside: | + if <... d.IsNewResource() ...> { ... } + severity: WARNING + - id: helper-schema-resource-Retry-without-TimeoutError-check languages: [go] message: Check resource.Retry() errors with tfresource.TimedOut() diff --git a/aws/resource_aws_accessanalyzer_analyzer.go b/aws/resource_aws_accessanalyzer_analyzer.go index c07154b97fc5..52fda89c7c69 100644 --- a/aws/resource_aws_accessanalyzer_analyzer.go +++ b/aws/resource_aws_accessanalyzer_analyzer.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/accessanalyzer" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" @@ -110,7 +111,7 @@ func resourceAwsAccessAnalyzerAnalyzerRead(d *schema.ResourceData, meta interfac output, err := conn.GetAnalyzer(input) - if isAWSErr(err, accessanalyzer.ErrCodeResourceNotFoundException, "") { + if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, accessanalyzer.ErrCodeResourceNotFoundException) { log.Printf("[WARN] Access Analyzer Analyzer (%s) not found, removing from state", d.Id()) d.SetId("") return nil diff --git a/aws/resource_aws_acm_certificate_validation.go b/aws/resource_aws_acm_certificate_validation.go index 89ffc3459e6a..e35a802e3afc 100644 --- a/aws/resource_aws_acm_certificate_validation.go +++ b/aws/resource_aws_acm_certificate_validation.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/acm" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" @@ -177,19 +178,32 @@ func resourceAwsAcmCertificateValidationRead(d *schema.ResourceData, meta interf resp, err := acmconn.DescribeCertificate(params) - if err != nil && isAWSErr(err, acm.ErrCodeResourceNotFoundException, "") { + if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, acm.ErrCodeResourceNotFoundException) { + log.Printf("[WARN] ACM Certificate (%s) not found, removing from state", d.Id()) d.SetId("") return nil - } else if err != nil { - return fmt.Errorf("Error describing certificate: %w", err) } - if aws.StringValue(resp.Certificate.Status) != acm.CertificateStatusIssued { - log.Printf("[INFO] Certificate status not issued, was %s, tainting validation", aws.StringValue(resp.Certificate.Status)) + if err != nil { + return fmt.Errorf("error describing ACM Certificate (%s): %w", d.Id(), err) + } + + if resp == nil || resp.Certificate == nil { + return fmt.Errorf("error describing ACM Certificate (%s): empty response", d.Id()) + } + + if status := aws.StringValue(resp.Certificate.Status); status != acm.CertificateStatusIssued { + if d.IsNewResource() { + return fmt.Errorf("ACM Certificate (%s) status not issued: %s", d.Id(), status) + } + + log.Printf("[WARN] ACM Certificate (%s) status not issued (%s), removing from state", d.Id(), status) d.SetId("") - } else { - d.SetId(aws.TimeValue(resp.Certificate.IssuedAt).String()) + return nil } + + d.SetId(aws.TimeValue(resp.Certificate.IssuedAt).String()) + return nil } diff --git a/aws/resource_aws_acmpca_certificate.go b/aws/resource_aws_acmpca_certificate.go index 5a6317d0ec47..afbcb51f8a3c 100644 --- a/aws/resource_aws_acmpca_certificate.go +++ b/aws/resource_aws_acmpca_certificate.go @@ -169,13 +169,19 @@ func resourceAwsAcmpcaCertificateRead(d *schema.ResourceData, meta interface{}) log.Printf("[DEBUG] Reading ACM PCA Certificate: %s", getCertificateInput) certificateOutput, err := conn.GetCertificate(getCertificateInput) + + if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, acmpca.ErrCodeResourceNotFoundException) { + log.Printf("[WARN] ACM PCA Certificate (%s) not found, removing from state", d.Id()) + d.SetId("") + return nil + } + if err != nil { - if isAWSErr(err, acmpca.ErrCodeResourceNotFoundException, "") { - log.Printf("[WARN] ACM PCA Certificate (%s) not found, removing from state", d.Id()) - d.SetId("") - return nil - } - return fmt.Errorf("error reading ACM PCA Certificate: %s", err) + return fmt.Errorf("error reading ACM PCA Certificate (%s): %w", d.Id(), err) + } + + if certificateOutput == nil { + return fmt.Errorf("error reading ACM PCA Certificate (%s): empty response", d.Id()) } d.Set("arn", d.Id()) diff --git a/aws/resource_aws_acmpca_certificate_authority.go b/aws/resource_aws_acmpca_certificate_authority.go index 2455f26e22a1..a27cde4b8825 100644 --- a/aws/resource_aws_acmpca_certificate_authority.go +++ b/aws/resource_aws_acmpca_certificate_authority.go @@ -324,17 +324,21 @@ func resourceAwsAcmpcaCertificateAuthorityRead(d *schema.ResourceData, meta inte certificateAuthority, err := finder.CertificateAuthorityByARN(conn, d.Id()) - if isAWSErr(err, acmpca.ErrCodeResourceNotFoundException, "") { + if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, acmpca.ErrCodeResourceNotFoundException) { log.Printf("[WARN] ACM PCA Certificate Authority (%s) not found, removing from state", d.Id()) d.SetId("") return nil } if err != nil { - return fmt.Errorf("error reading ACM PCA Certificate Authority: %s", err) + return fmt.Errorf("error reading ACM PCA Certificate Authority (%s): %w", d.Id(), err) } if certificateAuthority == nil || aws.StringValue(certificateAuthority.Status) == acmpca.CertificateAuthorityStatusDeleted { + if d.IsNewResource() { + return fmt.Errorf("error reading ACM PCA Certificate Authority (%s): not found or deleted", d.Id()) + } + log.Printf("[WARN] ACM PCA Certificate Authority (%s) not found, removing from state", d.Id()) d.SetId("") return nil @@ -365,17 +369,17 @@ func resourceAwsAcmpcaCertificateAuthorityRead(d *schema.ResourceData, meta inte log.Printf("[DEBUG] Reading ACM PCA Certificate Authority Certificate: %s", getCertificateAuthorityCertificateInput) getCertificateAuthorityCertificateOutput, err := conn.GetCertificateAuthorityCertificate(getCertificateAuthorityCertificateInput) - if err != nil { - if isAWSErr(err, acmpca.ErrCodeResourceNotFoundException, "") { - log.Printf("[WARN] ACM PCA Certificate Authority (%s) not found, removing from state", d.Id()) - d.SetId("") - return nil - } - // Returned when in PENDING_CERTIFICATE status - // InvalidStateException: The certificate authority XXXXX is not in the correct state to have a certificate signing request. - if !isAWSErr(err, acmpca.ErrCodeInvalidStateException, "") { - return fmt.Errorf("error reading ACM PCA Certificate Authority Certificate: %s", err) - } + + if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, acmpca.ErrCodeResourceNotFoundException) { + log.Printf("[WARN] ACM PCA Certificate Authority (%s) not found, removing from state", d.Id()) + d.SetId("") + return nil + } + + // Returned when in PENDING_CERTIFICATE status + // InvalidStateException: The certificate authority XXXXX is not in the correct state to have a certificate signing request. + if err != nil && !tfawserr.ErrCodeEquals(err, acmpca.ErrCodeInvalidStateException) { + return fmt.Errorf("error reading ACM PCA Certificate Authority (%s) Certificate: %w", d.Id(), err) } d.Set("certificate", "") @@ -392,15 +396,17 @@ func resourceAwsAcmpcaCertificateAuthorityRead(d *schema.ResourceData, meta inte log.Printf("[DEBUG] Reading ACM PCA Certificate Authority Certificate Signing Request: %s", getCertificateAuthorityCsrInput) getCertificateAuthorityCsrOutput, err := conn.GetCertificateAuthorityCsr(getCertificateAuthorityCsrInput) - if err != nil { - if isAWSErr(err, acmpca.ErrCodeResourceNotFoundException, "") { - log.Printf("[WARN] ACM PCA Certificate Authority (%s) not found, removing from state", d.Id()) - d.SetId("") - return nil - } - if !isAWSErr(err, acmpca.ErrCodeInvalidStateException, "") { - return fmt.Errorf("error reading ACM PCA Certificate Authority Certificate Signing Request: %s", err) - } + + if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, acmpca.ErrCodeResourceNotFoundException) { + log.Printf("[WARN] ACM PCA Certificate Authority (%s) not found, removing from state", d.Id()) + d.SetId("") + return nil + } + + // Returned when in PENDING_CERTIFICATE status + // InvalidStateException: The certificate authority XXXXX is not in the correct state to have a certificate signing request. + if err != nil && !tfawserr.ErrCodeEquals(err, acmpca.ErrCodeInvalidStateException) { + return fmt.Errorf("error reading ACM PCA Certificate Authority (%s) Certificate Signing Request: %w", d.Id(), err) } d.Set("certificate_signing_request", "") diff --git a/aws/resource_aws_ami.go b/aws/resource_aws_ami.go index 574a9ad7a1fc..5a11ffe13e0e 100644 --- a/aws/resource_aws_ami.go +++ b/aws/resource_aws_ami.go @@ -356,7 +356,11 @@ func resourceAwsAmiRead(d *schema.ResourceData, meta interface{}) error { return fmt.Errorf("Unable to find AMI after retries: %s", err) } - if len(res.Images) != 1 { + if res == nil || len(res.Images) != 1 { + if d.IsNewResource() { + return fmt.Errorf("error reading EC2 AMI (%s): empty response", d.Id()) + } + log.Printf("[WARN] AMI (%s) not found, removing from state", d.Id()) d.SetId("") return nil @@ -379,6 +383,10 @@ func resourceAwsAmiRead(d *schema.ResourceData, meta interface{}) error { } if state == ec2.ImageStateDeregistered { + if d.IsNewResource() { + return fmt.Errorf("error reading EC2 AMI (%s): deregistered", d.Id()) + } + log.Printf("[WARN] AMI (%s) not found, removing from state", d.Id()) d.SetId("") return nil diff --git a/aws/resource_aws_ami_launch_permission.go b/aws/resource_aws_ami_launch_permission.go index 43bcd8f21f82..8d4d190d2812 100644 --- a/aws/resource_aws_ami_launch_permission.go +++ b/aws/resource_aws_ami_launch_permission.go @@ -77,6 +77,10 @@ func resourceAwsAmiLaunchPermissionRead(d *schema.ResourceData, meta interface{} return fmt.Errorf("error reading AMI launch permission (%s): %w", d.Id(), err) } if !exists { + if d.IsNewResource() { + return fmt.Errorf("error reading EC2 AMI Launch Permission (%s): not found", d.Id()) + } + log.Printf("[WARN] AMI launch permission (%s) not found, removing from state", d.Id()) d.SetId("") return nil From 622a3ac83c0e08d227c20a07e27ef2fedeb1424d Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 26 Mar 2021 11:41:11 -0400 Subject: [PATCH 2/2] tests/provider: Simplify semgrep helper-schema-ResourceData-SetId-empty-without-IsNewResource-check exclude paths --- .semgrep.yml | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/.semgrep.yml b/.semgrep.yml index c373499ea873..1e8f05ecde47 100644 --- a/.semgrep.yml +++ b/.semgrep.yml @@ -241,26 +241,10 @@ rules: - aws/resource_aws_athena_*.go - aws/resource_aws_autoscaling_*.go - aws/resource_aws_autoscalingplans_scaling_plan.go - - aws/resource_aws_b*.go - - aws/resource_aws_c*.go - - aws/resource_aws_d*.go - - aws/resource_aws_e*.go - - aws/resource_aws_f*.go - - aws/resource_aws_g*.go + - aws/resource_aws_[b-g]*.go - aws/resource_aws_i*.go - - aws/resource_aws_k*.go - - aws/resource_aws_l*.go - - aws/resource_aws_m*.go - - aws/resource_aws_n*.go - - aws/resource_aws_o*.go - - aws/resource_aws_p*.go - - aws/resource_aws_q*.go - - aws/resource_aws_r*.go - - aws/resource_aws_s*.go - - aws/resource_aws_t*.go - - aws/resource_aws_v*.go - - aws/resource_aws_w*.go - - aws/resource_aws_x*.go + - aws/resource_aws_[k-t]*.go + - aws/resource_aws_[v-x]*.go include: - aws/resource*.go patterns: