From 5e8408f9a13c912d5931e9ed794f4cc111a2a780 Mon Sep 17 00:00:00 2001 From: Angie Pinilla 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 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 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