Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PLACEHOLDER] Consistent Resource Not Found Error Handling #15945

Closed
bflad opened this issue Oct 30, 2020 · 6 comments
Closed

[PLACEHOLDER] Consistent Resource Not Found Error Handling #15945

bflad opened this issue Oct 30, 2020 · 6 comments
Labels
proposal Proposes new design or functionality. provider Pertains to the provider itself, rather than any interaction with AWS. stale Old or inactive issues managed by automation, if no further action taken these will get closed. technical-debt Addresses areas of the codebase that need refactoring or redesign.

Comments

@bflad
Copy link
Contributor

bflad commented Oct 30, 2020

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue 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 issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Background

Placeholder Note: This issue contains details for a potential future project, but it has not been agreed upon in principle/design and is currently only here to track various issue/pull request references as they appear along with broad proposals.

The general goal behind this project would be to standardize resource error handling when the resource is not found. Resources today can use the following methodology for handling resource disappearance:

  • Disappears Acceptance Testing: To verify that removing a resource outside of Terraform's lifecycle management is appropriately handled to suggest recreation without returning an error
  • If resource not found error code(s)/message(s) are returned from the API:
    • During resource Read function: Warning log (likely warning diagnostic in future), remove resource from state, return no error
    • During resource Delete function: Return no error (resource is already automatically removed from state)
    • During acceptance testing "CheckExists" function: Return error
    • During acceptance testing CheckDestroy function: Continue resource checking loop (we got what we wanted already)

While there are some efforts to implement disappears testing across the provider codebase, it is not consistent or necessarily enforced during reviews so it is easy to miss. Without disappears testing, there are higher chances of Read handling to be missing. Handling of these errors in the Delete function are often missed because there is not an easy way in the Terraform Plugin SDK acceptance testing framework to trigger a deletion without refresh. CheckDestroy handling is almost always implemented for at least some of the errors as the testing will not pass at all without it.

Another challenge here is that there can be considerable "distance" between where the API/SDK call occurs and the resource code which would be checking for the resource not found error. For example, some resources use finder and waiter packages for delegating read or wait for removal API functionality in a more consistent (and potentially automatically generated in the future) method, while others have manually implemented similar but disparate equivalents in other resource functions. Especially for those custom implementations, some go through efforts to return nil instead of resource not found errors. These types of inconsistencies should be smoothed over through this proposal.

Proposal 1

Create a standard method for converting AWS Go SDK errors with specific error codes and messages to the Terraform Plugin SDK helper/resource.NotFoundError. We could then use static analysis to ensure this type of error is always handled in the Read/Delete/CheckDestroy functions.

Very roughly:

  • Create a standardized function signature type† for consuming the AWS Go SDK error and either returning the error as-is (potentially nil) or wrapping it in resource.NotFoundError††
  • In each resource, implement that function using the existing AWS Go SDK error helpers
  • Around each read/delete API call, add this wrapper before returning errors
  • In each resource Read, Delete, and CheckDestroy function, add a conditional with resource.NotFoundError checking and remove any existing resource not found error checking
  • Implement static analysis that enforces that each read/delete API call uses the resource error wrapper
  • Implement static analysis that enforces that each Read, Delete, and CheckDestroy function contains a conditional with resource.NotFoundError checking

† Or implement a new "Terraform AWS Provider resource" type and this as a new receiver method -- although if this is remotely a path forward, then this should really be thought about in the context of the "service packages" project.
†† This can (should?) be further simplified into "provide a list of error checking functions that return a boolean" whether this should be wrapped. Very related to what would likely be good for #9223 as well.

Proposal 2

Always create wrapper functions for AWS Go SDK API calls and return nil for resource not found errors immediately after they occur.

References

Related Efforts:

Related Issues:

@bflad bflad added proposal Proposes new design or functionality. technical-debt Addresses areas of the codebase that need refactoring or redesign. provider Pertains to the provider itself, rather than any interaction with AWS. labels Oct 30, 2020
bflad added a commit that referenced this issue Jan 8, 2021
…ssing Hosted Zone or VPC association

Reference: #15945
Reference: #17016
Reference: https://docs.aws.amazon.com/Route53/latest/APIReference/API_DisassociateVPCFromHostedZone.html#API_DisassociateVPCFromHostedZone_Errors

This is a best effort fix for the errors returned by the Route 53 `DisassociateVPCFromHostedZone` API, which are unrelated to the potential errors during the `Read` function that uses the `ListHostedZonesByVPC` API. The acceptance testing framework does not lend itself well to testing this situation and this highlights a case where #15945 would need special handling.

Output from acceptance testing in AWS Commercial:

```
--- PASS: TestAccAWSRoute53ZoneAssociation_disappears_Zone (146.44s)
--- PASS: TestAccAWSRoute53ZoneAssociation_disappears (147.22s)
--- PASS: TestAccAWSRoute53ZoneAssociation_disappears_VPC (147.31s)
--- PASS: TestAccAWSRoute53ZoneAssociation_CrossRegion (149.84s)
--- PASS: TestAccAWSRoute53ZoneAssociation_basic (150.01s)
--- PASS: TestAccAWSRoute53ZoneAssociation_CrossAccount (507.91s)
```
bflad added a commit that referenced this issue Jan 11, 2021
…ssing Hosted Zone or VPC association (#17023)

Reference: #15945
Reference: #17016
Reference: https://docs.aws.amazon.com/Route53/latest/APIReference/API_DisassociateVPCFromHostedZone.html#API_DisassociateVPCFromHostedZone_Errors

This is a best effort fix for the errors returned by the Route 53 `DisassociateVPCFromHostedZone` API, which are unrelated to the potential errors during the `Read` function that uses the `ListHostedZonesByVPC` API. The acceptance testing framework does not lend itself well to testing this situation and this highlights a case where #15945 would need special handling.

Output from acceptance testing in AWS Commercial:

```
--- PASS: TestAccAWSRoute53ZoneAssociation_disappears_Zone (146.44s)
--- PASS: TestAccAWSRoute53ZoneAssociation_disappears (147.22s)
--- PASS: TestAccAWSRoute53ZoneAssociation_disappears_VPC (147.31s)
--- PASS: TestAccAWSRoute53ZoneAssociation_CrossRegion (149.84s)
--- PASS: TestAccAWSRoute53ZoneAssociation_basic (150.01s)
--- PASS: TestAccAWSRoute53ZoneAssociation_CrossAccount (507.91s)
```
ewbankkit added a commit to ewbankkit/terraform-provider-aws that referenced this issue Jan 22, 2021
…und (hashicorp#15945).

r/aws_route: Implement 'd.IsNewResource()' checksin 'resourceAwsRouteRead' (hashicorp#16796).

Acceptance test output:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRoute_basic\|TestAccAWSRoute_disappears' ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 2 -run=TestAccAWSRoute_basic\|TestAccAWSRoute_disappears -timeout 120m
=== RUN   TestAccAWSRoute_basic
=== PAUSE TestAccAWSRoute_basic
=== RUN   TestAccAWSRoute_disappears
=== PAUSE TestAccAWSRoute_disappears
=== RUN   TestAccAWSRoute_disappears_RouteTable
=== PAUSE TestAccAWSRoute_disappears_RouteTable
=== CONT  TestAccAWSRoute_basic
=== CONT  TestAccAWSRoute_disappears_RouteTable
--- PASS: TestAccAWSRoute_disappears_RouteTable (34.67s)
=== CONT  TestAccAWSRoute_disappears
--- PASS: TestAccAWSRoute_basic (36.39s)
--- PASS: TestAccAWSRoute_disappears (31.85s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	66.620s
@ewbankkit

This comment has been minimized.

@ewbankkit
Copy link
Contributor

ewbankkit commented Feb 12, 2021

Some initial thoughts on what this could look like.

Returning NotFoundError when a resource is not found.

In the per-service finder/finder.go:

// ThingByID returns the thing corresponding to the specified ID.
// Returns NotFoundError if no thing is found.
func ThingByID(conn *example.Example, thingID string) (*example.Thing, error) {
	input := &example.GetThingInput{
		ThingId: aws.String(thingID),
	}

	return Thing(conn, input)
}

// Thing returns the thing corresponding to the specified input.
// Returns NotFoundError if no thing is found.
func Thing(conn *example.Example, input *example.GetThingInput) (*example.Thing, error) {
	output, err := conn.GetThing(input)

	// If the AWS API signals that the thing doesn't exist, return NotFoundError.
	if tfawserr.ErrCodeEquals(err, example.ErrCodeResourceNotFoundException) {
		return nil, &resource.NotFoundError{
			LastError:   err,
			LastRequest: input,
		}
	}

	if err != nil {
		return nil, err
	}

	// Handle any empty result.
	if output == nil || output.Thing == nil {
		return nil, &resource.NotFoundError{
			Message:     "Empty result",
			LastRequest: input,
		}
	}

	return output.Thing, nil
}

If the resource has an associated status for which one or more values indicate that nothing more can be done with the resource, finder/finder.go can be enhanced:

// ThingByID returns the thing corresponding to the specified ID.
// Returns NotFoundError if no thing is found.
func ThingByID(conn *example.Example, thingID string) (*example.Thing, error) {
	input := &example.GetThingInput{
		ThingId: aws.String(thingID),
	}

	return Thing(conn, input)
}

// Thing returns the thing corresponding to the specified input.
// Returns NotFoundError if no thing is found.
func Thing(conn *example.Example, input *example.GetThingInput) (*example.Thing, error) {
	output, err := conn.GetThing(input)

	// If the AWS API signals that the thing doesn't exist, return NotFoundError.
	if tfawserr.ErrCodeEquals(err, example.ErrCodeResourceNotFoundException) {
		return nil, &resource.NotFoundError{
			LastError:   err,
			LastRequest: input,
		}
	}

	if err != nil {
		return nil, err
	}

	// Handle any empty result.
	if output == nil || output.Thing == nil {
		return nil, &resource.NotFoundError{
			Message:     "Empty result",
			LastRequest: input,
		}
	}

	// If Thing has status(es) indicating that nothing more can be done with the thing
	// and that the thing will eventually be garbage collected by AWS, return NotFoundError.
	if status := aws.StringValue(output.Thing.Status); status == example.ThingStatusDeleted {
		return nil, &resource.NotFoundError{
			Message:     status,
			LastRequest: input,
		}
	}

	return output.Thing, nil
}

and waiter/status.go and waiter/waiter.go can be added (#12840) if the deletion occurs asynchronously:

func ThingStatus(conn *example.Example, thingID string) resource.StateRefreshFunc {
	return func() (interface{}, string, error) {
		thing, err := finder.ThingByID(conn, thingID)

		if tfresource.NotFound(err) {
			return nil, "", nil
		}

		if err != nil {
			return nil, "", err
		}

		return thing, aws.StringValue(thing.Status), nil
	}
}
const (
	// Maximum amount of time to wait for a Thing to be deleted.
	ThingDeletedTimeout = 5 * time.Minute
)

// ThingDeleted waits for a Thing to be deleted.
func ThingDeleted(conn *example.Example, thingID string) (*example.Thing, error) {
	stateConf := &resource.StateChangeConf{
		Pending: []string{example.ThingStatusDeleting},
		Target:  []string{},
		Refresh: ThingStatus(conn, thingID),
		Timeout: ThingDeletedTimeout,
	}

	outputRaw, err := stateConf.WaitForState()

	if v, ok := outputRaw.(*example.Thing); ok {
		return v, err
	}

	return nil, err
}

Similarly for resource deletion, in deleter/deleter.go (package name should be debated, I'm just mirroring finder for now):

// ThingByID deletes the thing corresponding to the specified ID.
// Returns NotFoundError if no thing is found.
func ThingByID(conn *example.Example, thingID string) error {
	input := &example.DeleteThingInput{
		ThingId: aws.String(thingID),
	}

	_, err := conn.DeleteThing(input)

	if tfawserr.ErrCodeEquals(err, example.ErrCodeResourceNotFoundException) {
		return &resource.NotFoundError{
			LastError:   err,
			LastRequest: input,
		}
	}

	return err
}

The resource implementation would then look like

func resourceAwsExampleThingCreate(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	// Create a thing.
	output, err := conn.CreateThing(&example.CreateThingInput{
		...
	})

	if err != nil {
		return fmt.Errorf("error creating Example Thing: %w", err)
	}

	d.SetId(aws.StringValue(output.ThingId))

	return resourceAwsExampleThingRead(d, meta)
}

func resourceAwsExampleThingRead(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	thing, err := finder.ThingByID(conn, d.Id())

	if !d.IsNewResource() && tfresource.NotFound(err) {
		log.Printf("[WARN] Example Thing (%s) not found, removing from state", d.Id())
		d.SetId("")
		return nil
	}

	if err != nil {
		return fmt.Errorf("error reading Example Thing (%s): %w", d.Id(), err)
	}

	// Set all the attributes.

	return nil
}

func resourceAwsExampleThingUpdate(d *schema.ResourceData, meta interface{}) error {
	// Update the thing.

	return resourceAwsExampleThingRead(d, meta)
}

func resourceAwsExampleThingDelete(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	err := deleter.ThingByID(conn, d.Id())

	if tfresource.NotFound(err) {
		return nil
	}

	if err != nil {
		return fmt.Errorf("error deleting Example Thing (%s): %w", d.Id(), err)
	}

	// If the deletion occurs asynchronously, wait.
	_, err = waiter.ThingDeleted(conn, d.Id())

	if err != nil {
		return fmt.Errorf("error waiting for Example Thing (%s) to delete: %w", d.Id(), err)
	}

	return nil
}

The acceptance test Exists and Destroy functions would look like

func testAccCheckAwsExampleThingExists(name string) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		conn := testAccProvider.Meta().(*AWSClient).exampleconn

		rs, ok := s.RootModule().Resources[name]
		if !ok {
			return fmt.Errorf("Not found: %s", name)
		}

		if rs.Primary.ID == "" {
			return fmt.Errorf("No ID is set")
		}

		_, err := finder.ThingByID(conn, rs.Primary.ID)

		if err != nil {
			return err
		}

		return nil
	}
}

func testAccCheckAwsExampleThingDestroy(s *terraform.State) error {
	conn := testAccProvider.Meta().(*AWSClient).globalacceleratorconn

	for _, rs := range s.RootModule().Resources {
		if rs.Type != "aws_example_thing" {
			continue
		}

		_, err := finder.ThingByID(conn, rs.Primary.ID)

		if tfresource.NotFound(err) {
			continue
		}

		if err != nil {
			return err
		}

		return fmt.Errorf("Example Thing %s still exists", rs.Primary.ID)
	}
	return nil
}

Services with GetThings() method

Some AWS services (for example AutoScalingPlans and EC2) have no GetThing() method, preferring a GetThings() lister method.
For these services prefer the paginated variant GetThingsPages() (see #13516 for how to generate a paginated lister function if the service does not provide such a method).

In the per-service finder/finder.go:

// ThingByID returns the thing corresponding to the specified ID.
// Returns NotFoundError if no thing is found.
func ThingByID(conn *example.Example, thingID string) (*example.Thing, error) {
	input := &example.GetThingsInput{
		ThingIds: aws.StringSlice([]string{thingID}),
	}

	things, err := Things(conn, input)

	if err != nil {
		return nil, err
	}

	// Handle any empty result.
	if len(things) == 0 {
		return nil, &resource.NotFoundError{
			Message:     "Empty result",
			LastRequest: input,
		}
	}

	return things[0], nil
}

// Things returns the things corresponding to the specified input.
// Returns an empty slice if no things are found.
func Things(conn *example.Example, input *example.GetThingsInput) ([]*example.Thing, error) {
	var things []*example.Thing

	err := conn.GetThingsPages(input, func(page *example.GetThingsOutput, isLast bool) bool {
		if page == nil {
			return !isLast
		}

		for _, thing := range page.Things {
			if thing == nil {
				continue
			}

			// If Thing has status(es) indicating that nothing more can be done with the thing
			// and that the thing will eventually be garbage collected by AWS, ignore the thing.
			if aws.StringValue(thing.Status) == example.ThingStatusDeleted {
				continue
			}

			things = append(things, thing)
		}

		return !isLast
	})

	// If the AWS API signals that the thing doesn't exist, return an empty slice.
	if tfawserr.ErrCodeEquals(err, example.ErrCodeResourceNotFoundException) {
		return things, nil
	}

	if err != nil {
		return nil, err
	}

	return things, nil
}

The waiter package and the aws_example_thing resource do not change.

aws_example_thing and aws_example_things data sources

The finder.Things code above can be used in any aws_example_thing and aws_example_things data sources, especially if the service supports generic attribute filtering (#13304).
Using the namevaluesfilters package from #13313, add to finder/finder.go

// ThingsByNameValuesFilters returns the things corresponding to the specified filter.
// Returns an empty slice if no thing is found.
func ThingsByNameValuesFilters(conn *example.Example, filters namevaluesfilters.NameValuesFilters) ([]*example.Thing, error) {
	input := &example.GetThingsInput{
		Filters: filters.ExampleFilters(),
	}

	things, err := Things(conn, input)

	if err != nil {
		return nil, err
	}

	return things, nil
}

The aws_example_thing data source implementation would then look like

func dataSourceAwsExampleThingRead(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	filters := namevaluesfilters.New(map[string]string{
		"name-1": "value1",
		"name-2": "value2",
	})

	things, err := finder.ThingsByNameValuesFilters(conn, filters)

	if len(things) == 0 {
		return fmt.Errorf("no Example Things matched; change the search criteria and try again")
	}

	if err != nil {
		return fmt.Errorf("error reading Example Things: %w", err)
	}

	if n := len(things); n > 0 {
		return fmt.Errorf("%d Example Things matched; use additional constraints to reduce matches to a single Thing", n)
	}

	// Set all the attributes.

	return nil
}

and aws_example_things data source implementation would then look like

func dataSourceAwsExampleThingsRead(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	filters := namevaluesfilters.New(map[string]string{
		"name-1": "value1",
		"name-2": "value2",
	})

	things, err := finder.ThingsByNameValuesFilters(conn, filters)

	if err != nil {
		return fmt.Errorf("error reading Example Things: %w", err)
	}

	for _, thing := range things {
		// Set all the attributes.
	}

	return nil
}

Returning nil when a resource is not found.

In the per-service finder/finder.go:

// ThingByID returns the thing corresponding to the specified ID.
// Returns nil if no thing is found.
func ThingByID(conn *example.Example, thingID string) (*example.Thing, error) {
	input := &example.GetThingInput{
		ThingId: aws.String(thingID),
	}

	return Thing(conn, input)
}

// Thing returns the thing corresponding to the specified input.
// Returns nil if no thing is found.
func Thing(conn *example.Example, input *example.GetThingInput) (*example.Thing, error) {
	output, err := conn.GetThing(input)

	// If the AWS API signals that the thing doesn't exist, return nil.
	if tfawserr.ErrCodeEquals(err, example.ErrCodeResourceNotFoundException) {
		return nil, nil
	}

	if err != nil {
		return nil, err
	}

	// Handle any empty result.
	if output == nil || output.Thing == nil {
		return nil, nil
	}

	return output.Thing, nil
}

If the resource has an associated status for which one or more values indicate that nothing more can be done with the resource, finder/finder.go can be enhanced:

// ThingByID returns the thing corresponding to the specified ID.
// Returns nil if no thing is found.
func ThingByID(conn *example.Example, thingID string) (*example.Thing, error) {
	input := &example.GetThingInput{
		ThingId: aws.String(thingID),
	}

	return Thing(conn, input)
}

// Thing returns the thing corresponding to the specified input.
// Returns nil if no thing is found.
func Thing(conn *example.Example, input *example.GetThingInput) (*example.Thing, error) {
	output, err := conn.GetThing(input)

	// If the AWS API signals that the thing doesn't exist, return nil.
	if tfawserr.ErrCodeEquals(err, example.ErrCodeResourceNotFoundException) {
		return nil, nil
	}

	if err != nil {
		return nil, err
	}

	// Handle any empty result.
	if output == nil || output.Thing == nil {
		return nil, nil
	}

	// If Thing has status(es) indicating that nothing more can be done with the thing
	// and that the thing will eventually be garbage collected by AWS, return nil.
	if status := aws.StringValue(output.Thing.Status); status == example.ThingStatusDeleted {
		return nil, nil
	}

	return output.Thing, nil
}

and waiter/status.go and waiter/waiter.go can be added (#12840) if the deletion occurs asynchronously:

func ThingStatus(conn *example.Example, thingID string) resource.StateRefreshFunc {
	return func() (interface{}, string, error) {
		thing, err := finder.ThingByID(conn, thingID)

		if err != nil {
			return nil, "", err
		}

		if thing == nil {
			return nil, "", nil
		}

		return thing, aws.StringValue(thing.Status), nil
	}
}
const (
	// Maximum amount of time to wait for a Thing to be deleted.
	ThingDeletedTimeout = 5 * time.Minute
)

// ThingDeleted waits for a Thing to be deleted.
func ThingDeleted(conn *example.Example, thingID string) (*example.Thing, error) {
	stateConf := &resource.StateChangeConf{
		Pending: []string{example.ThingStatusReady, example.ThingStatusDeleting},
		Target:  []string{},
		Refresh: ThingStatus(conn, thingID),
		Timeout: ThingDeletedTimeout,
	}

	outputRaw, err := stateConf.WaitForState()

	if v, ok := outputRaw.(*example.Thing); ok {
		return v, err
	}

	return nil, err
}

Similarly for resource deletion, in deleter/deleter.go (package name should be debated, I'm just mirroring finder for now):

// ThingByID deletes the thing corresponding to the specified ID.
// Returns nil if no thing is found.
func ThingByID(conn *example.Example, thingID string) error {
	input := &example.DeleteThingInput{
		ThingId: aws.String(thingID),
	}

	_, err := conn.DeleteThing(input)

	if tfawserr.ErrCodeEquals(err, example.ErrCodeResourceNotFoundException) {
		return nil
	}

	return err
}

The resource implementation would then look like

func resourceAwsExampleThingCreate(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	// Create a thing.
	output, err := conn.CreateThing(&example.CreateThingInput{
		...
	})

	if err != nil {
		return fmt.Errorf("error creating Example Thing: %w", err)
	}

	d.SetId(aws.StringValue(output.ThingId))

	return resourceAwsExampleThingRead(d, meta)
}

func resourceAwsExampleThingRead(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	thing, err := finder.ThingByID(conn, d.Id())

	if err != nil {
		return fmt.Errorf("error reading Example Thing (%s): %w", d.Id(), err)
	}

	if !d.IsNewResource() && thing == nil {
		log.Printf("[WARN] Example Thing (%s) not found, removing from state", d.Id())
		d.SetId("")
		return nil
	}

	if thing == nil {
		return fmt.Errorf("error reading Example Thing (%s): Not found", d.Id())
	}

	// Set all the attributes.

	return nil
}

func resourceAwsExampleThingUpdate(d *schema.ResourceData, meta interface{}) error {
    // Update the thing.

	return resourceAwsExampleThingRead(d, meta)
}

func resourceAwsExampleThingDelete(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	err := deleter.ThingByID(conn, d.Id())

	if err != nil {
		return fmt.Errorf("error deleting Example Thing (%s): %w", d.Id(), err)
	}

	// If the deletion occurs asynchronously, wait.
	_, err = waiter.ThingDeleted(conn, d.Id())

	if err != nil {
		return fmt.Errorf("error waiting for Example Thing (%s) to delete: %w", d.Id(), err)
	}

	return nil
}

The acceptance test Exists and Destroy functions would look like

func testAccCheckAwsExampleThingExists(name string) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		conn := testAccProvider.Meta().(*AWSClient).exampleconn

		rs, ok := s.RootModule().Resources[name]
		if !ok {
			return fmt.Errorf("Not found: %s", name)
		}

		if rs.Primary.ID == "" {
			return fmt.Errorf("No ID is set")
		}

		thing, err := finder.ThingByID(conn, rs.Primary.ID)

		if err != nil {
			return err
		}

		if thing == nil {
			return fmt.Errorf("Example Thing %s not found", rs.Primary.ID)
		}

		return nil
	}
}

func testAccCheckAwsExampleThingDestroy(s *terraform.State) error {
	conn := testAccProvider.Meta().(*AWSClient).globalacceleratorconn

	for _, rs := range s.RootModule().Resources {
		if rs.Type != "aws_example_thing" {
			continue
		}

		thing, err := finder.ThingByID(conn, rs.Primary.ID)

		if thing == nil {
			continue
		}

		if err != nil {
			return err
		}

		return fmt.Errorf("Example Thing %s still exists", rs.Primary.ID)
	}
	return nil
}

Services with GetThings() method

Some AWS services (for example AutoScalingPlans and EC2) have no GetThing() method, preferring a GetThings() lister method.
For these services prefer the paginated variant GetThingsPages() (see #13516 for how to generate a paginated lister function if the service does not provide such a method).

In the per-service finder/finder.go:

// ThingByID returns the thing corresponding to the specified ID.
// Returns nil if no thing is found.
func ThingByID(conn *example.Example, thingID string) (*example.Thing, error) {
	input := &example.GetThingsInput{
		ThingIds: aws.StringSlice([]string{thingID}),
	}

	things, err := Things(conn, input)

	if err != nil {
		return nil, err
	}

	if things == nil {
		return nil, nil
	}

	return things[0], nil
}

// Things returns the things corresponding to the specified input.
// Returns an empty slice if no things are found.
func Things(conn *example.Example, input *example.GetThingsInput) ([]*example.Thing, error) {
	var things []*example.Thing

	err := conn.GetThingsPages(input, func(page *example.GetThingsOutput, isLast bool) bool {
		if page == nil {
			return !isLast
		}

		for _, thing := range page.Things {
			if thing == nil {
				continue
			}

			// If Thing has status(es) indicating that nothing more can be done with the thing
			// and that the thing will eventually be garbage collected by AWS, ignore the thing.
			if aws.StringValue(thing.Status) == example.ThingStatusDeleted {
				continue
			}

			things = append(things, thing)
		}

		return !isLast
	})

	// If the AWS API signals that the thing doesn't exist, return an empty slice.
	if tfawserr.ErrCodeEquals(err, example.ErrCodeResourceNotFoundException) {
		return things, nil
	}

	if err != nil {
		return nil, err
	}

	return things, nil
}

The waiter package and the aws_example_thing resource do not change.

aws_example_thing and aws_example_things data sources

The finder.Things code above can be used in any aws_example_thing and aws_example_things data sources, especially if the service supports generic attribute filtering (#13304).
Using the namevaluesfilters package from #13313, add to finder/finder.go

// ThingsByNameValuesFilters returns the things corresponding to the specified filter.
// Returns an empty slice if no thing is found.
func ThingsByNameValuesFilters(conn *example.Example, filters namevaluesfilters.NameValuesFilters) ([]*example.Thing, error) {
	input := &example.GetThingsInput{
		Filters: filters.ExampleFilters(),
	}

	things, err := Things(conn, input)

	if err != nil {
		return nil, err
	}

	return things, nil
}

The aws_example_thing data source implementation would then look like

func dataSourceAwsExampleThingRead(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	filters := namevaluesfilters.New(map[string]string{
		"name-1": "value1",
		"name-2": "value2",
	})

	things, err := finder.ThingsByNameValuesFilters(conn, filters)

	if err != nil {
		return fmt.Errorf("error reading Example Things: %w", err)
	}

	if len(things) == 0 {
		return fmt.Errorf("no Example Things matched; change the search criteria and try again")
	}

	if n := len(things); n > 0 {
		return fmt.Errorf("%d Example Things matched; use additional constraints to reduce matches to a single Thing", n)
	}

	// Set all the attributes.

	return nil
}

and aws_example_things data source implementation would then look like

func dataSourceAwsExampleThingsRead(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).exampleconn

	filters := namevaluesfilters.New(map[string]string{
		"name-1": "value1",
		"name-2": "value2",
	})

	things, err := finder.ThingsByNameValuesFilters(conn, filters)

	if err != nil {
		return fmt.Errorf("error reading Example Things: %w", err)
	}

	for _, thing := range things {
		// Set all the attributes.
	}

	return nil
}

@ewbankkit
Copy link
Contributor

Some conclusions and considerations from the initial thoughts above:

  • Returning NotFoundError simplifies the code (error propagation) and logic (no nil checks required with associated crashes if they are forgotten)
  • Moving relevant AWS error checking and status code checking into the finder seems more robust (see Deleted transit gateway attachments causing resource refresh to fail #10393 for example of where not checking for DELETED status code causes issues)
  • Do we want to move the resource deletion API call into a separate package (if so, what should the name be)? If acceptance test sweepers are changed to use the
			r := resourceAwsExampleThing()
			d := r.Data(nil)
			d.SetId(...)
			err = r.Delete(d, client)

form then there is usually only a single place that the resource deletion API method is called

ewbankkit added a commit to ewbankkit/terraform-provider-aws that referenced this issue Feb 23, 2021
…f 'nil' when no application found (hashicorp#15945).

r/aws_kinesis_analytics_application: Implement 'd.IsNewResource()' checks in 'resourceAwsKinesisAnalyticsApplicationRead' (hashicorp#16796).

Acceptance test output:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisAnalyticsApplication_basic\|TestAccAWSKinesisAnalyticsApplication_disappears'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSKinesisAnalyticsApplication_basic\|TestAccAWSKinesisAnalyticsApplication_disappears -timeout 120m
=== RUN   TestAccAWSKinesisAnalyticsApplication_basic
=== PAUSE TestAccAWSKinesisAnalyticsApplication_basic
=== RUN   TestAccAWSKinesisAnalyticsApplication_disappears
=== PAUSE TestAccAWSKinesisAnalyticsApplication_disappears
=== CONT  TestAccAWSKinesisAnalyticsApplication_basic
=== CONT  TestAccAWSKinesisAnalyticsApplication_disappears
--- PASS: TestAccAWSKinesisAnalyticsApplication_basic (15.29s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_disappears (18.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	18.344s
@ewbankkit
Copy link
Contributor

Testing the Resource Not Found error on resource deletion can be done by adding a second call to testAccCheckResourceDisappears in the resource's _disappears acceptance test.
For example:

func TestAccAWSVpc_disappears(t *testing.T) {
	var vpc ec2.Vpc
	resourceName := "aws_vpc.test"

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckVpcDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccVpcConfig,
				Check: resource.ComposeTestCheckFunc(
					testAccCheckVpcExists(resourceName, &vpc),
					testAccCheckResourceDisappears(testAccProvider, resourceAwsVpc(), resourceName),
					// Deletion of deleted resource.
					testAccCheckResourceDisappears(testAccProvider, resourceAwsVpc(), resourceName),
				),
				ExpectNonEmptyPlan: true,
			},
		},
	})
}

ewbankkit added a commit to ewbankkit/terraform-provider-aws that referenced this issue Mar 25, 2021
…und (hashicorp#15945).

r/aws_route: Implement 'd.IsNewResource()' checksin 'resourceAwsRouteRead' (hashicorp#16796).

Acceptance test output:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRoute_basic\|TestAccAWSRoute_disappears' ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 2 -run=TestAccAWSRoute_basic\|TestAccAWSRoute_disappears -timeout 120m
=== RUN   TestAccAWSRoute_basic
=== PAUSE TestAccAWSRoute_basic
=== RUN   TestAccAWSRoute_disappears
=== PAUSE TestAccAWSRoute_disappears
=== RUN   TestAccAWSRoute_disappears_RouteTable
=== PAUSE TestAccAWSRoute_disappears_RouteTable
=== CONT  TestAccAWSRoute_basic
=== CONT  TestAccAWSRoute_disappears_RouteTable
--- PASS: TestAccAWSRoute_disappears_RouteTable (34.67s)
=== CONT  TestAccAWSRoute_disappears
--- PASS: TestAccAWSRoute_basic (36.39s)
--- PASS: TestAccAWSRoute_disappears (31.85s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	66.620s
ewbankkit added a commit to ewbankkit/terraform-provider-aws that referenced this issue Mar 26, 2021
…of 'nil' when no application found (hashicorp#15945).

Acceptance test output:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisAnalyticsV2Application_' ACCTEST_PARALLELISM=3
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 3 -run=TestAccAWSKinesisAnalyticsV2Application_ -timeout 120m
=== RUN   TestAccAWSKinesisAnalyticsV2Application_basicFlinkApplication
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_basicFlinkApplication
=== RUN   TestAccAWSKinesisAnalyticsV2Application_basicSQLApplication
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_basicSQLApplication
=== RUN   TestAccAWSKinesisAnalyticsV2Application_disappears
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_disappears
=== RUN   TestAccAWSKinesisAnalyticsV2Application_Tags
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_Tags
=== RUN   TestAccAWSKinesisAnalyticsV2Application_ApplicationCodeConfiguration_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_ApplicationCodeConfiguration_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Add
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Add
=== RUN   TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Delete
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Delete
=== RUN   TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_EnvironmentProperties_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_EnvironmentProperties_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_FlinkApplicationConfiguration_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_FlinkApplicationConfiguration_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_FlinkApplicationConfiguration_EnvironmentProperties_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_FlinkApplicationConfiguration_EnvironmentProperties_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_ServiceExecutionRole_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_ServiceExecutionRole_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Input_Add
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Input_Add
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Input_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Input_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Add
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Add
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Delete
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Delete
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Multiple_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Multiple_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Output_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Output_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Add
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Add
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Delete
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Delete
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Update
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Add
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Add
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Delete
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Delete
=== RUN   TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Update
=== PAUSE TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Update
=== CONT  TestAccAWSKinesisAnalyticsV2Application_basicFlinkApplication
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Input_Update
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_basicFlinkApplication (54.58s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Delete
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Update (74.66s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Add
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Input_Update (117.92s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Delete (67.58s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Delete
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_VPCConfiguration_Add (68.71s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Add
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Add (103.54s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Output_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Delete (129.15s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Multiple_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_ReferenceDataSource_Update (135.56s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Multiple_Update (108.99s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Delete
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Update (113.14s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Add
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Output_Update (126.17s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Update (41.51s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Input_Add
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Add (97.33s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_ServiceExecutionRole_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_InputProcessingConfiguration_Delete (106.22s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_FlinkApplicationConfiguration_EnvironmentProperties_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_ServiceExecutionRole_Update (42.77s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_FlinkApplicationConfiguration_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_FlinkApplicationConfiguration_EnvironmentProperties_Update (66.21s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_EnvironmentProperties_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_SQLApplicationConfiguration_Input_Add (118.86s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_ApplicationCodeConfiguration_Update
--- PASS: TestAccAWSKinesisAnalyticsV2Application_ApplicationCodeConfiguration_Update (52.50s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Delete
--- PASS: TestAccAWSKinesisAnalyticsV2Application_FlinkApplicationConfiguration_Update (80.04s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Add
--- PASS: TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Delete (42.49s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_disappears
--- PASS: TestAccAWSKinesisAnalyticsV2Application_EnvironmentProperties_Update (103.32s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_Tags
--- PASS: TestAccAWSKinesisAnalyticsV2Application_CloudWatchLoggingOptions_Add (52.77s)
=== CONT  TestAccAWSKinesisAnalyticsV2Application_basicSQLApplication
--- PASS: TestAccAWSKinesisAnalyticsV2Application_disappears (35.36s)
--- PASS: TestAccAWSKinesisAnalyticsV2Application_basicSQLApplication (36.17s)
--- PASS: TestAccAWSKinesisAnalyticsV2Application_Tags (59.22s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	695.388s
ewbankkit added a commit to ashishmohite/terraform-provider-aws that referenced this issue May 21, 2021
Acceptance test output:

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSAmplifyApp_disappears'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSAmplifyApp_disappears -timeout 180m
=== RUN   TestAccAWSAmplifyApp_disappears
=== PAUSE TestAccAWSAmplifyApp_disappears
=== CONT  TestAccAWSAmplifyApp_disappears
--- PASS: TestAccAWSAmplifyApp_disappears (10.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	14.084s
@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

@github-actions github-actions bot added the stale Old or inactive issues managed by automation, if no further action taken these will get closed. label Mar 8, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 8, 2023
@github-actions
Copy link

github-actions bot commented May 9, 2023

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
proposal Proposes new design or functionality. provider Pertains to the provider itself, rather than any interaction with AWS. stale Old or inactive issues managed by automation, if no further action taken these will get closed. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Projects
None yet
Development

No branches or pull requests

2 participants