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