Add Semgrep rule to warn on calling 'd.SetId()' in a resource create or delete function

[ewbankkit]

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

Relates #12796.
Relates #18382.
Relates #18390.

Warns if d.SetId("") is called in a resource's Create function or d.SetId(...) is called in a resource's Delete function.

% make testacc TESTS=TestAccCloudFormationStack_basic PKG=cloudformation
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/cloudformation/... -v -count 1 -parallel 20 -run='TestAccCloudFormationStack_basic'  -timeout 180m
=== RUN   TestAccCloudFormationStack_basic
=== PAUSE TestAccCloudFormationStack_basic
=== CONT  TestAccCloudFormationStack_basic
--- PASS: TestAccCloudFormationStack_basic (58.15s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/cloudformation	62.549s
% make testacc TESTS=TestAccEC2Fleet_basic PKG=ec2                      
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 20 -run='TestAccEC2Fleet_basic'  -timeout 180m
=== RUN   TestAccEC2Fleet_basic
=== PAUSE TestAccEC2Fleet_basic
=== CONT  TestAccEC2Fleet_basic
--- PASS: TestAccEC2Fleet_basic (58.07s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ec2	65.831s
% make testacc TESTS=TestAccEC2SpotFleetRequest_basic PKG=ec2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 20 -run='TestAccEC2SpotFleetRequest_basic'  -timeout 180m
=== RUN   TestAccEC2SpotFleetRequest_basic
=== PAUSE TestAccEC2SpotFleetRequest_basic
=== CONT  TestAccEC2SpotFleetRequest_basic
--- PASS: TestAccEC2SpotFleetRequest_basic (142.54s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ec2	145.979s
% make testacc TESTS=TestAccNeptuneEventSubscription_basic PKG=neptune
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/neptune/... -v -count 1 -parallel 20 -run='TestAccNeptuneEventSubscription_basic'  -timeout 180m
=== RUN   TestAccNeptuneEventSubscription_basic
=== PAUSE TestAccNeptuneEventSubscription_basic
=== CONT  TestAccNeptuneEventSubscription_basic
--- PASS: TestAccNeptuneEventSubscription_basic (130.70s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/neptune	135.190s
% make testacc TESTS=TestAccRedshiftSnapshotScheduleAssociation_basic PKG=redshift
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/redshift/... -v -count 1 -parallel 20 -run='TestAccRedshiftSnapshotScheduleAssociation_basic'  -timeout 180m
=== RUN   TestAccRedshiftSnapshotScheduleAssociation_basic
=== PAUSE TestAccRedshiftSnapshotScheduleAssociation_basic
=== CONT  TestAccRedshiftSnapshotScheduleAssociation_basic
--- PASS: TestAccRedshiftSnapshotScheduleAssociation_basic (260.14s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/redshift	265.418s
% make testacc TESTS=TestAccSNSTopicSubscription_basic PKG=sns            
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/sns/... -v -count 1 -parallel 20 -run='TestAccSNSTopicSubscription_basic'  -timeout 180m
=== RUN   TestAccSNSTopicSubscription_basic
=== PAUSE TestAccSNSTopicSubscription_basic
=== CONT  TestAccSNSTopicSubscription_basic
--- PASS: TestAccSNSTopicSubscription_basic (101.15s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/sns	104.894s
% make testacc TESTS=TestAccWAFRegionalRegexMatchSet_serial/basic PKG=wafregional
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/wafregional/... -v -count 1 -parallel 20 -run='TestAccWAFRegionalRegexMatchSet_serial/basic'  -timeout 180m
=== RUN   TestAccWAFRegionalRegexMatchSet_serial
=== RUN   TestAccWAFRegionalRegexMatchSet_serial/basic
--- PASS: TestAccWAFRegionalRegexMatchSet_serial (25.20s)
    --- PASS: TestAccWAFRegionalRegexMatchSet_serial/basic (25.19s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/wafregional	28.857s

[github-actions]

Thank you for your contribution! 🚀

Please note that the CHANGELOG.md file contents are handled by the maintainers during merge. This is to prevent pull request merge conflicts, especially for contributions which may not be merged immediately. Please see the Contributing Guide for additional pull request review items.

Remove any changes to the CHANGELOG.md file and commit them in this pull request to prevent delays with reviewing and potentially merging this pull request.

[ewbankkit]

Consider an additional rule for calling d.Set(...) in an update or delete function:

  - id: calling-Set-in-resource-update-or-delete
    languages: [go]
    message: Do not call `d.Set(...)` inside a resource update or delete function
    paths:
      include:
        - aws/
    patterns:
      - pattern: |
          func $FUNC(...) {
            ...
            d.Set(...)
          }
      - metavariable-regex:
          metavariable: "$FUNC"
          regex: "^resourceAws\\w*(Update|Delete|Disable)$"
    severity: WARNING

[ewbankkit]

aws/resource_aws_cloudformation_stack.go
severity:warning rule:calling-SetId-with-empty-string-in-resource-create: Do not call `d.SetId("")` inside a resource create function
123:func resourceAwsCloudFormationStackCreate(d *schema.ResourceData, meta interface{}) error {
124:	conn := meta.(*AWSClient).cfconn
125:	defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig
126:	tags := defaultTagsConfig.MergeTags(keyvaluetags.New(d.Get("tags").(map[string]interface{})))
127:
128:	requestToken := resource.UniqueId()
129:	input := cloudformation.CreateStackInput{
130:		StackName:          aws.String(d.Get("name").(string)),
131:		ClientRequestToken: aws.String(requestToken),
132:	}
-------- [hid 70 additional lines, adjust with --max-lines-per-finding] --------

aws/resource_aws_ec2_fleet.go
severity:warning rule:calling-SetId-with-empty-string-in-resource-create: Do not call `d.SetId("")` inside a resource create function
341:func resourceAwsEc2FleetCreate(d *schema.ResourceData, meta interface{}) error {
342:	conn := meta.(*AWSClient).ec2conn
343:	defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig
344:	tags := defaultTagsConfig.MergeTags(keyvaluetags.New(d.Get("tags").(map[string]interface{})))
345:
346:	input := &ec2.CreateFleetInput{
347:		ExcessCapacityTerminationPolicy:  aws.String(d.Get("excess_capacity_termination_policy").(string)),
348:		LaunchTemplateConfigs:            expandEc2FleetLaunchTemplateConfigRequests(d.Get("launch_template_config").([]interface{})),
349:		OnDemandOptions:                  expandEc2OnDemandOptionsRequest(d.Get("on_demand_options").([]interface{})),
350:		ReplaceUnhealthyInstances:        aws.Bool(d.Get("replace_unhealthy_instances").(bool)),
-------- [hid 50 additional lines, adjust with --max-lines-per-finding] --------

aws/resource_aws_spot_fleet_request.go
severity:warning rule:calling-SetId-with-empty-string-in-resource-create: Do not call `d.SetId("")` inside a resource create function
924:func resourceAwsSpotFleetRequestCreate(d *schema.ResourceData, meta interface{}) error {
925:	// http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_RequestSpotFleet.html
926:	conn := meta.(*AWSClient).ec2conn
927:	defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig
928:	tags := defaultTagsConfig.MergeTags(keyvaluetags.New(d.Get("tags").(map[string]interface{})))
929:
930:	_, launchSpecificationOk := d.GetOk("launch_specification")
931:	_, launchTemplateConfigsOk := d.GetOk("launch_template_config")
932:
933:	// http://docs.aws.amazon.com/sdk-for-go/api/service/ec2.html#type-SpotFleetRequestConfigData
------- [hid 187 additional lines, adjust with --max-lines-per-finding] --------

The aws_cloudformation_stack finding looks like it could be related to #107 and #6800.

[zhelding]

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

[ewbankkit]

To keep this PR's scope smaller open a separate PR for the case of calling d.SetId("") in resource Update.
Often these are due to calling resource Update immediately after Create rather than resource Read.

[github-actions]

This functionality has been released in v3.73.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. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.