Merge pull request #24804 from hashicorp/b-servicecatalog-provisioned-product-record-wait-for-status

r/servicecatalog_provisioned_product: add `TAINTED` to target states in waiters

Author: anGie44

.changelog/24804.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_servicecatalog_provisioned_product: Add possible `TAINTED` target state for resource update and remove one of the internal waiters during read
+```

internal/service/servicecatalog/provisioned_product.go

@@ -8,6 +8,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/servicecatalog"
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
+	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
@@ -415,9 +416,16 @@ func resourceProvisionedProductRead(d *schema.ResourceData, meta interface{}) er
 	d.Set("status_message", detail.StatusMessage)
 	d.Set("type", detail.Type)
 
-	// tags are only available from the record tied to the provisioned product
+	// Previously, we waited for the record to only return a target state of 'SUCCEEDED' or 'AVAILABLE'
+	// but this can interfere complete reads of this resource when an error occurs after initial creation
+	// or after an invalid update that returns a 'FAILED' record state. Thus, waiters are now present in the CREATE and UPDATE methods of this resource instead.
+	// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/24574#issuecomment-1126339193
+	recordInput := &servicecatalog.DescribeRecordInput{
+		Id:             detail.LastProvisioningRecordId,
+		AcceptLanguage: aws.String(acceptLanguage),
+	}
 
-	recordOutput, err := WaitRecordReady(conn, acceptLanguage, aws.StringValue(detail.LastProvisioningRecordId), RecordReadyTimeout)
+	recordOutput, err := conn.DescribeRecord(recordInput)
 
 	if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, servicecatalog.ErrCodeResourceNotFoundException) {
 		log.Printf("[WARN] Service Catalog Provisioned Product (%s) Record (%s) not found, unable to set tags", d.Id(), aws.StringValue(detail.LastProvisioningRecordId))
@@ -432,12 +440,27 @@ func resourceProvisionedProductRead(d *schema.ResourceData, meta interface{}) er
 		return fmt.Errorf("error getting Service Catalog Provisioned Product (%s) Record (%s): empty response", d.Id(), aws.StringValue(detail.LastProvisioningRecordId))
 	}
 
+	// To enable debugging of potential errors, log as a warning
+	// instead of exiting prematurely with an error, e.g.
+	// errors can be present after update to a new version failed and the stack
+	// rolled back to the current version.
+	if errors := recordOutput.RecordDetail.RecordErrors; len(errors) > 0 {
+		var errs *multierror.Error
+
+		for _, err := range errors {
+			errs = multierror.Append(errs, fmt.Errorf("%s: %s", aws.StringValue(err.Code), aws.StringValue(err.Description)))
+		}
+
+		log.Printf("[WARN] Errors found when describing Service Catalog Provisioned Product (%s) Record (%s): %s", d.Id(), aws.StringValue(detail.LastProvisioningRecordId), errs.ErrorOrNil())
+	}
+
 	if err := d.Set("outputs", flattenRecordOutputs(recordOutput.RecordOutputs)); err != nil {
 		return fmt.Errorf("error setting outputs: %w", err)
 	}
 
 	d.Set("path_id", recordOutput.RecordDetail.PathId)
 
+	// tags are only available from the record tied to the provisioned product
 	tags := recordKeyValueTags(recordOutput.RecordDetail.RecordTags).IgnoreAWS().IgnoreConfig(ignoreTagsConfig)
 
 	//lintignore:AWSR002

internal/service/servicecatalog/status.go

@@ -388,34 +388,6 @@ func StatusProvisionedProduct(conn *servicecatalog.ServiceCatalog, acceptLanguag
 	}
 }
 
-func StatusRecord(conn *servicecatalog.ServiceCatalog, acceptLanguage, id string) resource.StateRefreshFunc {
-	return func() (interface{}, string, error) {
-		input := &servicecatalog.DescribeRecordInput{
-			Id: aws.String(id),
-		}
-
-		if acceptLanguage != "" {
-			input.AcceptLanguage = aws.String(acceptLanguage)
-		}
-
-		output, err := conn.DescribeRecord(input)
-
-		if tfawserr.ErrCodeEquals(err, servicecatalog.ErrCodeResourceNotFoundException) {
-			return nil, StatusNotFound, err
-		}
-
-		if err != nil {
-			return nil, servicecatalog.StatusFailed, err
-		}
-
-		if output == nil || output.RecordDetail == nil {
-			return nil, StatusNotFound, err
-		}
-
-		return output, aws.StringValue(output.RecordDetail.Status), err
-	}
-}
-
 func StatusPortfolioConstraints(conn *servicecatalog.ServiceCatalog, acceptLanguage, portfolioID, productID string) resource.StateRefreshFunc {
 	return func() (interface{}, string, error) {
 		input := &servicecatalog.ListConstraintsForPortfolioInput{

internal/service/servicecatalog/wait.go

@@ -2,13 +2,11 @@ package servicecatalog
 
 import (
 	"errors"
-	"fmt"
 	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/servicecatalog"
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
-	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-provider-aws/internal/tfresource"
 )
@@ -50,7 +48,6 @@ const (
 	ProvisioningArtifactReadTimeout            = 10 * time.Minute
 	ProvisioningArtifactReadyTimeout           = 3 * time.Minute
 	ProvisioningArtifactUpdateTimeout          = 3 * time.Minute
-	RecordReadyTimeout                         = 30 * time.Minute
 	ServiceActionDeleteTimeout                 = 3 * time.Minute
 	ServiceActionReadTimeout                   = 10 * time.Minute
 	ServiceActionReadyTimeout                  = 3 * time.Minute
@@ -505,8 +502,10 @@ func WaitLaunchPathsReady(conn *servicecatalog.ServiceCatalog, acceptLanguage, p
 
 func WaitProvisionedProductReady(conn *servicecatalog.ServiceCatalog, acceptLanguage, id, name string, timeout time.Duration) (*servicecatalog.DescribeProvisionedProductOutput, error) {
 	stateConf := &resource.StateChangeConf{
+		// "TAINTED" is a valid target state as its described as a stable state per API docs, though can result from a failed update
+		// such that the stack rolls back to a previous version.
 		Pending:                   []string{StatusNotFound, StatusUnavailable, servicecatalog.ProvisionedProductStatusUnderChange, servicecatalog.ProvisionedProductStatusPlanInProgress},
-		Target:                    []string{servicecatalog.StatusAvailable},
+		Target:                    []string{servicecatalog.StatusAvailable, servicecatalog.ProvisionedProductStatusTainted},
 		Refresh:                   StatusProvisionedProduct(conn, acceptLanguage, id, name),
 		Timeout:                   timeout,
 		ContinuousTargetOccurence: ContinuousTargetOccurrence,
@@ -537,36 +536,6 @@ func WaitProvisionedProductTerminated(conn *servicecatalog.ServiceCatalog, accep
 	return err
 }
 
-func WaitRecordReady(conn *servicecatalog.ServiceCatalog, acceptLanguage, id string, timeout time.Duration) (*servicecatalog.DescribeRecordOutput, error) {
-	stateConf := &resource.StateChangeConf{
-		Pending:                   []string{StatusNotFound, StatusUnavailable, servicecatalog.ProvisionedProductStatusUnderChange, servicecatalog.ProvisionedProductStatusPlanInProgress},
-		Target:                    []string{servicecatalog.RecordStatusSucceeded, servicecatalog.StatusAvailable},
-		Refresh:                   StatusRecord(conn, acceptLanguage, id),
-		Timeout:                   timeout,
-		ContinuousTargetOccurence: ContinuousTargetOccurrence,
-		NotFoundChecks:            NotFoundChecks,
-		MinTimeout:                MinTimeout,
-	}
-
-	outputRaw, err := stateConf.WaitForState()
-
-	if output, ok := outputRaw.(*servicecatalog.DescribeRecordOutput); ok {
-		if errors := output.RecordDetail.RecordErrors; len(errors) > 0 {
-			var errs *multierror.Error
-
-			for _, err := range output.RecordDetail.RecordErrors {
-				errs = multierror.Append(errs, fmt.Errorf("%s: %s", aws.StringValue(err.Code), aws.StringValue(err.Description)))
-			}
-
-			tfresource.SetLastError(err, errs.ErrorOrNil())
-		}
-
-		return output, err
-	}
-
-	return nil, err
-}
-
 func WaitPortfolioConstraintsReady(conn *servicecatalog.ServiceCatalog, acceptLanguage, portfolioID, productID string, timeout time.Duration) ([]*servicecatalog.ConstraintDetail, error) {
 	stateConf := &resource.StateChangeConf{
 		Pending: []string{StatusNotFound},