Proposal: Add more fine-grained errors for finder functions

[gdavison]

Adds EmptyResultError and TooManyResultsError as additional error cases for finder functions such as SecurityGroupByID() to replace testing the error message for string equality.

These two new errors can still be handled by isResourceNotFoundError() or an errors.As() check. In addition, defines ErrEmptyResult and ErrTooManyResults which can be used in errors.Is() checks.

Examples:

sg, err := finder.SecurityGroup(conn, req)
if errors.Is(err, tfresource.ErrEmptyResult) {
	return fmt.Errorf("no matching SecurityGroup found")
}
if errors.Is(err, tfresource.ErrTooManyResults) {
	return fmt.Errorf("multiple Security Groups matched; use additional constraints to reduce matches to a single Security Group")
}
if err != nil {
	return err
}

replaces

resp, err := conn.DescribeSecurityGroups(req)
if err != nil {
	return err
}
if resp == nil || len(resp.SecurityGroups) == 0 {
	return fmt.Errorf("no matching SecurityGroup found")
}
if len(resp.SecurityGroups) > 1 {
	return fmt.Errorf("multiple Security Groups matched; use additional constraints to reduce matches to a single Security Group")
}

sg := resp.SecurityGroups[0]

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSELB_\|TestAccAWSDefaultSecurityGroup_\|TestAccDataSourceAwsSecurityGroup_'

--- PASS: TestAccDataSourceAwsSecurityGroup_basic (169.35s)
--- PASS: TestAccAWSELB_Timeout (196.42s)
--- PASS: TestAccAWSELB_HealthCheck (197.28s)
--- PASS: TestAccAWSELB_ConnectionDraining (197.33s)
--- PASS: TestAccAWSDefaultSecurityGroup_Vpc_empty (223.01s)
--- PASS: TestAccAWSDefaultSecurityGroup_Classic_basic (286.84s)
--- PASS: TestAccAWSELB_disappears (288.85s)
--- PASS: TestAccAWSELB_generatesNameForZeroValue (289.64s)
--- PASS: TestAccAWSELB_Listener_SSLCertificateID_IAMServerCertificate (293.94s)
--- PASS: TestAccAWSELB_namePrefix (322.23s)
--- PASS: TestAccAWSELB_generatedName (326.91s)
--- PASS: TestAccAWSELB_fullCharacterRange (329.27s)
--- PASS: TestAccAWSELB_availabilityZones (354.05s)
--- PASS: TestAccAWSELB_SecurityGroups (371.73s)
--- PASS: TestAccAWSELB_basic (203.03s)
--- PASS: TestAccAWSDefaultSecurityGroup_Vpc_basic (235.37s)
--- PASS: TestAccAWSELB_swap_subnets (440.54s)
--- PASS: TestAccAWSELB_AccessLogs_disabled (445.80s)
--- PASS: TestAccAWSELB_AccessLogs_enabled (463.81s)
--- PASS: TestAccAWSELB_tags (482.03s)
--- PASS: TestAccAWSELB_InstanceAttaching (485.73s)
--- PASS: TestAccAWSELB_listener (557.68s)

[bflad]

My recommendation here would be to first document the existing finder package with its intended use cases and implementation patterns in the Contributing Guide, then introduce the documentation updates in this pull request as well so it is clear for everyone about the coding practice changes. 👍

Another consideration here is that we likely have quite a few error handling scenarios we want to ensure we are consistently encoding across all packages, not just finder, each with its own semantics. The existing resource.NotFoundError and new error types probably need to be defined better in the Error Handling section of the Contribution Guide.

Resource Not Found

Applicable to:

• Managed Resources
• Singular Data Sources
• Plural Data Sources (where the API requires a parent resource for listing)

Explicitly converting AWS service error codes or missing results into the common resource.NotFoundError type.

Expected uses:

• Triggering resource recreation by removing the resource from state and returning no error
• Delete error that can be skipped
• CheckDestroy error that can be skipped
• Missing object in multiple object response

Empty API Response

Applicable to:

• Managed Resources
• Singular Data Sources
• Plural Data Sources

If the API response includes missing output types (e.g. empty Go pointers).

Expected uses:

• Panic prevention for API or Terraform logic implementation issues

Multiple API Response

Applicable to:

• Managed Resources
• Singular Data Sources

If the API response includes multiple objects, but we expect only one object.

Expected uses:

• Where the API only returns a set of objects and returns multiple results, when we expect one

[bflad]

Related to the above, I have created two tracking issues:

• Add Contribution Guide Documentation for Service finder Packages #18417
• Implement Consistent Error Types and Handling in Service finder Packages #18418

[ewbankkit]

LGTM 🚀.

Commercial

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSDefaultSecurityGroup_\|TestAccDataSourceAwsSecurityGroup_\|TestAccAWSELB_SecurityGroups'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSDefaultSecurityGroup_\|TestAccDataSourceAwsSecurityGroup_\|TestAccAWSELB_SecurityGroups -timeout 120m
=== RUN   TestAccDataSourceAwsSecurityGroup_basic
=== PAUSE TestAccDataSourceAwsSecurityGroup_basic
=== RUN   TestAccAWSDefaultSecurityGroup_Vpc_basic
=== PAUSE TestAccAWSDefaultSecurityGroup_Vpc_basic
=== RUN   TestAccAWSDefaultSecurityGroup_Vpc_empty
=== PAUSE TestAccAWSDefaultSecurityGroup_Vpc_empty
=== RUN   TestAccAWSDefaultSecurityGroup_Classic_basic
=== PAUSE TestAccAWSDefaultSecurityGroup_Classic_basic
=== RUN   TestAccAWSDefaultSecurityGroup_Classic_empty
    provider_test.go:56: This resource does not currently clear tags when adopting the resource
--- SKIP: TestAccAWSDefaultSecurityGroup_Classic_empty (0.00s)
=== RUN   TestAccAWSELB_SecurityGroups
=== PAUSE TestAccAWSELB_SecurityGroups
=== CONT  TestAccDataSourceAwsSecurityGroup_basic
=== CONT  TestAccAWSDefaultSecurityGroup_Classic_basic
=== CONT  TestAccAWSELB_SecurityGroups
=== CONT  TestAccAWSDefaultSecurityGroup_Vpc_empty
=== CONT  TestAccAWSDefaultSecurityGroup_Vpc_basic
--- PASS: TestAccAWSDefaultSecurityGroup_Classic_basic (13.54s)
--- PASS: TestAccDataSourceAwsSecurityGroup_basic (24.25s)
--- PASS: TestAccAWSDefaultSecurityGroup_Vpc_empty (28.20s)
--- PASS: TestAccAWSELB_SecurityGroups (41.01s)
--- PASS: TestAccAWSDefaultSecurityGroup_Vpc_basic (46.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	48.880s

GovCloud

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSDefaultSecurityGroup_\|TestAccDataSourceAwsSecurityGroup_\|TestAccAWSELB_SecurityGroups'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSDefaultSecurityGroup_\|TestAccDataSourceAwsSecurityGroup_\|TestAccAWSELB_SecurityGroups -timeout 120m
=== RUN   TestAccDataSourceAwsSecurityGroup_basic
=== PAUSE TestAccDataSourceAwsSecurityGroup_basic
=== RUN   TestAccAWSDefaultSecurityGroup_Vpc_basic
=== PAUSE TestAccAWSDefaultSecurityGroup_Vpc_basic
=== RUN   TestAccAWSDefaultSecurityGroup_Vpc_empty
=== PAUSE TestAccAWSDefaultSecurityGroup_Vpc_empty
=== RUN   TestAccAWSDefaultSecurityGroup_Classic_basic
=== PAUSE TestAccAWSDefaultSecurityGroup_Classic_basic
=== RUN   TestAccAWSDefaultSecurityGroup_Classic_empty
    provider_test.go:56: This resource does not currently clear tags when adopting the resource
--- SKIP: TestAccAWSDefaultSecurityGroup_Classic_empty (0.00s)
=== RUN   TestAccAWSELB_SecurityGroups
=== PAUSE TestAccAWSELB_SecurityGroups
=== CONT  TestAccDataSourceAwsSecurityGroup_basic
=== CONT  TestAccAWSDefaultSecurityGroup_Classic_basic
=== CONT  TestAccAWSDefaultSecurityGroup_Vpc_empty
=== CONT  TestAccAWSELB_SecurityGroups
=== CONT  TestAccAWSDefaultSecurityGroup_Vpc_basic
=== CONT  TestAccAWSDefaultSecurityGroup_Classic_basic
    ec2_classic_test.go:54: this test can only run in EC2-Classic, platforms available in us-gov-west-1: ["VPC"]
--- SKIP: TestAccAWSDefaultSecurityGroup_Classic_basic (2.52s)
--- PASS: TestAccDataSourceAwsSecurityGroup_basic (27.38s)
--- PASS: TestAccAWSDefaultSecurityGroup_Vpc_empty (31.82s)
--- PASS: TestAccAWSELB_SecurityGroups (49.64s)
--- PASS: TestAccAWSDefaultSecurityGroup_Vpc_basic (52.89s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	55.663s

[github-actions]

This functionality has been released in v3.55.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.