[PLACEHOLDER] Refactor StateChangeConf and StateRefreshFunc Logic into Internal Packages

[bflad]

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

Description

This issue is a placeholder for future provider design considerations.

The Terraform AWS Provider, from its inception within the shared Terraform codebase, has existed as a monolithic Go package (aws). All resource, acceptance testing, and shared logic has co-mingled in this single Go package mostly in an effort to simplify community development for those unfamiliar with Go language and Terraform provider framework programming. Fast forward a few years and a few hundred more Terraform resources, this single Go package approach introduces a few burdens:

• Coding patterns and good practices are hard to discover
• Code generation based off those coding patterns is harder to determine
• Editors and their integrations (e.g. go-pls) spend many additional computational cycles to recompile this large package, slowing some gains that those provide

Luckily for operators using the released binary, these development tradeoffs are invisible, however we would like to improve the development experience as well to improve time to feature release. Since the Terraform AWS Provider codebase is not to be considered an upstream library (in implementation consideration or versioning), we can freely adjust the codebase without those concerns. The main caveat to any refactoring work does however need to differentiate between acceptance testing and unit testing, whereas the Terraform Plugin SDK and associated tooling/handling would need notable development experience improvements before moving acceptance tests from the monolithic Go package.

In #12765, a proof of concept design for migrating some of the monolithic package code into its own per-AWS-service internal Go package was implemented. This internal package, which will likely be implemented with a few other common design patterns on a per-AWS-service level, has the following:

aws/internal/service/servicediscovery/waiter
├── status.go
└── waiter.go

The status.go file holding resource.StateRefreshFunc, e.g.

package waiter

import (
	"fmt"

	"github.com/aws/aws-sdk-go/aws"
	"github.com/aws/aws-sdk-go/service/servicediscovery"
	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
)

// OperationStatus fetches the Operation and its Status
func OperationStatus(conn *servicediscovery.ServiceDiscovery, operationID string) resource.StateRefreshFunc {
	return func() (interface{}, string, error) {
		input := &servicediscovery.GetOperationInput{
			OperationId: aws.String(operationID),
		}

		output, err := conn.GetOperation(input)

		if err != nil {
			return nil, servicediscovery.OperationStatusFail, err
		}

		// Error messages can also be contained in the response with FAIL status
		//   "ErrorCode":"CANNOT_CREATE_HOSTED_ZONE",
		//   "ErrorMessage":"The VPC that you chose, vpc-xxx in region xxx, is already associated with another private hosted zone that has an overlapping name space, xxx.. (Service: AmazonRoute53; Status Code: 400; Error Code: ConflictingDomainExists; Request ID: xxx)"
		//   "Status":"FAIL",

		if aws.StringValue(output.Operation.Status) == servicediscovery.OperationStatusFail {
			return output, servicediscovery.OperationStatusFail, fmt.Errorf("%s: %s", aws.StringValue(output.Operation.ErrorCode), aws.StringValue(output.Operation.ErrorMessage))
		}

		return output, aws.StringValue(output.Operation.Status), nil
	}
}

The waiter.go file holding resource.StateChangeConf and timeout constants (for those not needing customizable timeouts), e.g.

package waiter

import (
	"time"

	"github.com/aws/aws-sdk-go/service/servicediscovery"
	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
)

const (
	// Maximum amount of time to wait for an Operation to return Success
	OperationSuccessTimeout = 5 * time.Minute
)

// OperationSuccess waits for an Operation to return Success
func OperationSuccess(conn *servicediscovery.ServiceDiscovery, operationID string) (*servicediscovery.GetOperationOutput, error) {
	stateConf := &resource.StateChangeConf{
		Pending: []string{servicediscovery.OperationStatusSubmitted, servicediscovery.OperationStatusPending},
		Target:  []string{servicediscovery.OperationStatusSuccess},
		Refresh: OperationStatus(conn, operationID),
		Timeout: OperationSuccessTimeout,
	}

	outputRaw, err := stateConf.WaitForState()

	if output, ok := outputRaw.(*servicediscovery.GetOperationOutput); ok {
		return output, err
	}

	return nil, err
}

This type of design can help with the following:

• Allowing developers to more easily see various implementations of similar functionality
• Allowing maintainers to more easily document how to accomplish retry logic
• In the future, offering an easier design point for code generation (or if nothing else, great reference material for said generation)
• Reducing the computational burden of changes within the monolithic Go package

For resources implementing this logic, it becomes:

• Adding the Go import, e.g.:

"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/servicediscovery/waiter"

• Adding the logic, e.g.

if _, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId)); err != nil {
	return fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) deletion: %w", d.Id(), err)
}

In cases where multiple service waiters or timeout constants are required (utilizing the IAM propagation timeout is very common), the preference is to have the current service Go import without an alias (effectively staying waiter), while prefixing the service name to other Go imports (generically SERVICEwaiter), e.g.

iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter"

Naming conventions:

• Functions returning resource.StateRefreshFunc: {Thing}{StateField}, e.g. OperationStatus
• Functions using StateChangeConf: {Thing}{Target}, e.g. OperationSuccess
• StateChangeConf Timeout constants: {Thing}{Target}Timeout e.g. OperationSuccessTimeout
• Other useful timeouts may be also added {Description}Timeout, e.g. PropagationTimeout for IAM eventual consistency

As another consideration, this effort could move us positively towards enabling go-mnd linting, if timeouts must be split into constants. This will reduce errors when attempting to update a timeout with multiple implementations.

This refactoring would apply to all existing and future usage of StateChangeConf/StateRefreshFunc until another design proposal supersedes this one.

Definition of Done

• Design pattern is agreed by maintainers
• Coding style is documented in the Contributing Guide
• Usage of StateChangeConf/StateRefreshFunc in the monolithic aws package is statically analyzed and able to be manually reported
• Existing usage of StateChangeConf is migrated
• Existing usage of StateRefreshFunc is migrated
• Usage of StateChangeConf/StateRefreshFunc in the monolithic aws package is reported in CI for enforcement
• (Preferably) Hardcoded timeouts (note: which are often preferred in many cases) are separated into constants

Affected Resource(s)

When the design work is completed and approved a list will be generated here.

[bflad]

Updated to include information about multiple Go imports based off #12863, e.g.

iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter"

[YakDriver]

Interesting.

[YakDriver]

I'm concerned about creating a whole separate service package structure when the resources should also be in service packages. Also, I'm concerned about the proliferation of directories, finder, lister, waiter, etc. This may be making the overall problem worse.

For an alternate proposal, see #19347. (Easier to view in tree view: https://github.com/hashicorp/terraform-provider-aws/tree/td-repackage)

[github-actions]

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