From 5e8408f9a13c912d5931e9ed794f4cc111a2a780 Mon Sep 17 00:00:00 2001
From: Angie Pinilla <angie@hashicorp.com>
Date: Mon, 16 May 2022 22:39:02 -0400
Subject: [PATCH 1/3] r/servicecatalog_provisioned_product: remove wait
 condition when calling DescribeRecord

---
 .../servicecatalog/provisioned_product.go     | 25 ++++++++++++--
 internal/service/servicecatalog/status.go     | 28 ----------------
 internal/service/servicecatalog/wait.go       | 33 -------------------
 3 files changed, 23 insertions(+), 63 deletions(-)

diff --git a/internal/service/servicecatalog/provisioned_product.go b/internal/service/servicecatalog/provisioned_product.go
index 4383c15d9534..a875959f1360 100644
--- a/internal/service/servicecatalog/provisioned_product.go
+++ b/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 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. Thus, waiters are now present in the CREATE and UPDATE methods of this resource instead.
+	// TODO: determine if waiter is needed in the IMPORT case
+	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,25 @@ 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.
+	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
diff --git a/internal/service/servicecatalog/status.go b/internal/service/servicecatalog/status.go
index 00120bd0c589..71d563d2e8ed 100644
--- a/internal/service/servicecatalog/status.go
+++ b/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{
diff --git a/internal/service/servicecatalog/wait.go b/internal/service/servicecatalog/wait.go
index b160389a1e69..b8a0ecbc5a6b 100644
--- a/internal/service/servicecatalog/wait.go
+++ b/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
@@ -537,36 +534,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},

From 10e4636009c3ed3cf947e9fb70a4ab189283725d Mon Sep 17 00:00:00 2001
From: Angie Pinilla <angie@hashicorp.com>
Date: Mon, 23 May 2022 15:06:09 -0400
Subject: [PATCH 2/3] r/servicecatalog_provisioned_product: update valid target
 states to include TAINTED

---
 internal/service/servicecatalog/provisioned_product.go | 10 ++++++----
 internal/service/servicecatalog/wait.go                |  4 +++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/internal/service/servicecatalog/provisioned_product.go b/internal/service/servicecatalog/provisioned_product.go
index a875959f1360..6f45b7714f0c 100644
--- a/internal/service/servicecatalog/provisioned_product.go
+++ b/internal/service/servicecatalog/provisioned_product.go
@@ -416,10 +416,10 @@ func resourceProvisionedProductRead(d *schema.ResourceData, meta interface{}) er
 	d.Set("status_message", detail.StatusMessage)
 	d.Set("type", detail.Type)
 
-	// Previously, we waited for the record to return a target state of 'SUCCEEDED' or 'AVAILABLE'
+	// 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. Thus, waiters are now present in the CREATE and UPDATE methods of this resource instead.
-	// TODO: determine if waiter is needed in the IMPORT case
+	// 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),
@@ -441,7 +441,9 @@ func resourceProvisionedProductRead(d *schema.ResourceData, meta interface{}) er
 	}
 
 	// To enable debugging of potential errors, log as a warning
-	// instead of exiting prematurely with an error.
+	// 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
 
diff --git a/internal/service/servicecatalog/wait.go b/internal/service/servicecatalog/wait.go
index b8a0ecbc5a6b..112c12724526 100644
--- a/internal/service/servicecatalog/wait.go
+++ b/internal/service/servicecatalog/wait.go
@@ -502,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,

From 26509ed39afbb6389be0f7b50f166e151ef5a0e2 Mon Sep 17 00:00:00 2001
From: Angie Pinilla <angie@hashicorp.com>
Date: Thu, 26 May 2022 09:45:37 -0400
Subject: [PATCH 3/3] Update CHANGELOG for #24804

---
 .changelog/24804.txt | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 .changelog/24804.txt

diff --git a/.changelog/24804.txt b/.changelog/24804.txt
new file mode 100644
index 000000000000..aa458f8eebd0
--- /dev/null
+++ b/.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
+```
\ No newline at end of file