tests/provider: Bootstrap d.IsNewResource() semgrep rule and fix some A resources to start

[bflad]

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

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

[ewbankkit]

There are cases where a d.SetId("") call is being made in resource Create (#18376) or Update functions:

terraform-provider-aws/aws/resource_aws_api_gateway_vpc_link.go

Lines 165 to 173 in 3d8e0fa

	_, err := conn.UpdateVpcLink(input)
	if err != nil {
	if isAWSErr(err, apigateway.ErrCodeNotFoundException, "") {
	log.Printf("[WARN] VPC Link %s not found, removing from state", d.Id())
	d.SetId("")
	return nil
	}
	return err
	}

[bflad]

@ewbankkit yeah, any d.SetId("") in Create/Update/Delete should be removed, but they are unrelated to this particular check.

[anGie44]

are these explicit lines in place of usage like: aws/resource_aws_[b-g]*.go?

[anGie44]

i tried using the range usage locally and the report didn't send back any errors from those files 🤔 so maybe it works?

[bflad]

It does seem to work! 👍 Will adjust by rebasing this with that change.

[anGie44]

Sweet LGTM! (just a small qq re: path matching)

[ghost]

This has been released in version 3.35.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

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