From 5a89f3c6b06ac3cbda4fe94275e2a899cfbd7839 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 11 Apr 2023 16:38:32 -0700 Subject: [PATCH 1/8] Returns an error on failure --- .../provisioned_product_test.go | 41 ++++++++++++++----- internal/service/servicecatalog/wait.go | 18 ++++---- 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/internal/service/servicecatalog/provisioned_product_test.go b/internal/service/servicecatalog/provisioned_product_test.go index b4b98bdd8f71..dbf66aeb1019 100644 --- a/internal/service/servicecatalog/provisioned_product_test.go +++ b/internal/service/servicecatalog/provisioned_product_test.go @@ -247,8 +247,27 @@ func TestAccServiceCatalogProvisionedProduct_tags(t *testing.T) { }) } -// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/24574 -func TestAccServiceCatalogProvisionedProduct_tainted(t *testing.T) { +func TestAccServiceCatalogProvisionedProduct_taintedCreate(t *testing.T) { + ctx := acctest.Context(t) + + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + domain := fmt.Sprintf("http://%s", acctest.RandomDomainName()) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, servicecatalog.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckProvisionedProductDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccProvisionedProductConfig_error(rName, domain, acctest.DefaultEmailAddress, "10.1.0.0/16"), + ExpectError: regexp.MustCompile(`AmazonCloudFormationException Unresolved resource dependencies \[MyVPC\] in the Outputs block of the template`), + }, + }, + }) +} + +func TestAccServiceCatalogProvisionedProduct_taintedUpdate(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_servicecatalog_provisioned_product.test" @@ -268,17 +287,16 @@ func TestAccServiceCatalogProvisionedProduct_tainted(t *testing.T) { ), }, { - Config: testAccProvisionedProductConfig_updateTainted(rName, domain, acctest.DefaultEmailAddress, "10.1.0.0/16"), + Config: testAccProvisionedProductConfig_error(rName, domain, acctest.DefaultEmailAddress, "10.1.0.0/16"), + ExpectError: regexp.MustCompile(`AmazonCloudFormationException Unresolved resource dependencies \[MyVPC\] in the Outputs block of the template`), + }, + { + // Check we can still run a complete apply after the previous update error + Config: testAccProvisionedProductConfig_basic(rName, domain, acctest.DefaultEmailAddress, "10.1.0.0/16"), Check: resource.ComposeTestCheckFunc( testAccCheckProvisionedProductExists(ctx, resourceName), - resource.TestCheckResourceAttr(resourceName, "status", servicecatalog.ProvisionedProductStatusTainted), ), }, - { - // Check we can still run a complete plan after the previous update error - Config: testAccProvisionedProductConfig_updateTainted(rName, domain, acctest.DefaultEmailAddress, "10.1.0.0/16"), - PlanOnly: true, - }, }, }) } @@ -584,7 +602,10 @@ resource "aws_servicecatalog_provisioned_product" "test" { `, rName, vpcCidr)) } -func testAccProvisionedProductConfig_updateTainted(rName, domain, email, vpcCidr string) string { +// Because the `provisioning_parameter` "LeaveMeEmpty" is not empty, this configuration results in an error. +// The `status_message` will be: +// AmazonCloudFormationException Unresolved resource dependencies [MyVPC] in the Outputs block of the template +func testAccProvisionedProductConfig_error(rName, domain, email, vpcCidr string) string { return acctest.ConfigCompose(testAccProvisionedProductTemplateURLBaseConfig(rName, domain, email), fmt.Sprintf(` resource "aws_servicecatalog_provisioned_product" "test" { diff --git a/internal/service/servicecatalog/wait.go b/internal/service/servicecatalog/wait.go index f31c0a2a57a7..c2b6b64d5105 100644 --- a/internal/service/servicecatalog/wait.go +++ b/internal/service/servicecatalog/wait.go @@ -9,7 +9,6 @@ import ( "github.com/aws/aws-sdk-go/service/servicecatalog" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" - "github.com/hashicorp/terraform-provider-aws/internal/tfresource" ) const ( @@ -496,7 +495,7 @@ func WaitLaunchPathsReady(ctx context.Context, conn *servicecatalog.ServiceCatal func WaitProvisionedProductReady(ctx context.Context, conn *servicecatalog.ServiceCatalog, acceptLanguage, id, name string, timeout time.Duration) (*servicecatalog.DescribeProvisionedProductOutput, error) { stateConf := &retry.StateChangeConf{ Pending: []string{StatusNotFound, StatusUnavailable, servicecatalog.ProvisionedProductStatusUnderChange, servicecatalog.ProvisionedProductStatusPlanInProgress}, - Target: []string{servicecatalog.StatusAvailable, servicecatalog.ProvisionedProductStatusTainted}, // Note: "TAINTED" is a stable state, ready for any operation, but is not exactly what was requested (new version failed and rolled back to current version) + Target: []string{servicecatalog.StatusAvailable}, Refresh: StatusProvisionedProduct(ctx, conn, acceptLanguage, id, name), Timeout: timeout, ContinuousTargetOccurence: ContinuousTargetOccurrence, @@ -508,11 +507,14 @@ func WaitProvisionedProductReady(ctx context.Context, conn *servicecatalog.Servi if output, ok := outputRaw.(*servicecatalog.DescribeProvisionedProductOutput); ok { if detail := output.ProvisionedProductDetail; detail != nil { - status := aws.StringValue(detail.Status) - - if status == servicecatalog.ProvisionedProductStatusError || status == servicecatalog.ProvisionedProductStatusTainted { - // for TAINTED, this may be ineffectual since err is likely nil - tfresource.SetLastError(err, errors.New(aws.StringValue(detail.StatusMessage))) + var foo *retry.UnexpectedStateError + if errors.As(err, &foo) { + // The statuses `ERROR` and `TAINTED` are equivalent: the application of the requested change has failed. + // The difference is that, in the case of `TAINTED`, there is a previous version to roll back to. + status := aws.StringValue(detail.Status) + if status == servicecatalog.ProvisionedProductStatusError || status == servicecatalog.ProvisionedProductStatusTainted { + return output, errors.New(aws.StringValue(detail.StatusMessage)) + } } } return output, err @@ -523,7 +525,7 @@ func WaitProvisionedProductReady(ctx context.Context, conn *servicecatalog.Servi func WaitProvisionedProductTerminated(ctx context.Context, conn *servicecatalog.ServiceCatalog, acceptLanguage, id, name string, timeout time.Duration) error { stateConf := &retry.StateChangeConf{ - Pending: []string{servicecatalog.StatusAvailable, servicecatalog.ProvisionedProductStatusUnderChange, servicecatalog.ProvisionedProductStatusTainted}, + Pending: []string{servicecatalog.StatusAvailable, servicecatalog.ProvisionedProductStatusUnderChange}, Target: []string{StatusNotFound, StatusUnavailable}, Refresh: StatusProvisionedProduct(ctx, conn, acceptLanguage, id, name), Timeout: timeout, From 3cb345294be08e8765b63c9ec7e827f21e7292d8 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 11 Apr 2023 16:38:42 -0700 Subject: [PATCH 2/8] Updates documentation --- .../r/servicecatalog_provisioned_product.html.markdown | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/website/docs/r/servicecatalog_provisioned_product.html.markdown b/website/docs/r/servicecatalog_provisioned_product.html.markdown index ce50cff383ea..21d438960d51 100644 --- a/website/docs/r/servicecatalog_provisioned_product.html.markdown +++ b/website/docs/r/servicecatalog_provisioned_product.html.markdown @@ -14,8 +14,6 @@ A provisioned product is a resourced instance of a product. For example, provisi Like this resource, the `aws_servicecatalog_record` data source also provides information about a provisioned product. Although a Service Catalog record provides some overlapping information with this resource, a record is tied to a provisioned product event, such as provisioning, termination, and updating. -~> **NOTE:** This resource will continue to function normally, _not_ returning an error unless AWS does, if a stack has a `status` of `TAINTED`. "`TAINTED`" means that Service Catalog, from its perspective, completed an update but the stack is not exactly what you requested. For example, if you request to update a stack to a new version but the request fails, AWS will roll back the stack to the current version and give a `TAINTED` status. (`status_message` also provides more information.) This is a little different than Terraform's typical declarative way. However, the approach aligns with AWS's. When `TAINTED`, the stack is in a "stable state" and "ready to perform any operation." - -> **Tip:** If you include conflicted keys as tags, AWS will report an error, "Parameter validation failed: Missing required parameter in Tags[N]:Value". -> **Tip:** A "provisioning artifact" is also referred to as a "version." A "distributor" is also referred to as a "vendor." @@ -107,11 +105,8 @@ In addition to all arguments above, the following attributes are exported: ### `status` Meanings -~> **NOTE:** [Enable logging](https://www.terraform.io/plugin/log/managing) to `WARN` verbosity to further investigate error messages associated with a provisioned product in the `ERROR` or `TAINTED` state which can occur during resource creation or update. - * `AVAILABLE` - Stable state, ready to perform any operation. The most recent operation succeeded and completed. -* `UNDER_CHANGE` - Transitive state. Operations performed might not have -valid results. Wait for an `AVAILABLE` status before performing operations. +* `UNDER_CHANGE` - Transitive state. Operations performed might not have valid results. Wait for an `AVAILABLE` status before performing operations. * `TAINTED` - Stable state, ready to perform any operation. The stack has completed the requested operation but is not exactly what was requested. For example, a request to update to a new version failed and the stack rolled back to the current version. * `ERROR` - An unexpected error occurred. The provisioned product exists but the stack is not running. For example, CloudFormation received a parameter value that was not valid and could not launch the stack. * `PLAN_IN_PROGRESS` - Transitive state. The plan operations were performed to provision a new product, but resources have not yet been created. After reviewing the list of resources to be created, execute the plan. Wait for an `AVAILABLE` status before performing operations. From 7c88d0d7ff642076da8e5fa75017c7a746dd4b30 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 11 Apr 2023 16:55:14 -0700 Subject: [PATCH 3/8] Removes superseded change from CHANGELOG --- CHANGELOG.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db35b0f72bbe..50ca3adad2d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,5 @@ ## 4.63.0 (Unreleased) -NOTES: - -* resource/aws_servicecatalog_provisioned_product: Previously, this resource would error or hang until timeout and then error, if a stack had a `status` of `TAINTED`. Now, the resource will continue functioning normally if the `status` is `TAINTED`. This is because the stack is in a "stable state" and "ready to perform any operation." However, although "the stack has completed the requested operation," it "is not exactly what was requested." The `status` will reflect `TAINTED` and `status_message` will provide clues as to what happened. See the `aws_servicecatalog_provisioned_product` documentation for more information and ways to use logging with this. ([#30522](https://github.com/hashicorp/terraform-provider-aws/issues/30522)) - FEATURES: * **New Data Source:** `aws_dms_certificate` ([#30498](https://github.com/hashicorp/terraform-provider-aws/issues/30498)) @@ -31,7 +27,6 @@ BUG FIXES: * resource/aws_launch_template: Fix crash when `instance_market_options.spot_options` is empty ([#30539](https://github.com/hashicorp/terraform-provider-aws/issues/30539)) * resource/aws_msk_serverless_cluster: Change `vpc_config.security_group_ids` to Computed ([#30535](https://github.com/hashicorp/terraform-provider-aws/issues/30535)) * resource/aws_rds_cluster_parameter_group: Fixes differences being reported on every apply when setting system-source parameters ([#30536](https://github.com/hashicorp/terraform-provider-aws/issues/30536)) -* resource/aws_servicecatalog_provisioned_product: Allow the waiting that comes with updating and deleting to not error due to a `status` of `TAINTED` ([#30522](https://github.com/hashicorp/terraform-provider-aws/issues/30522)) ## 4.62.0 (April 6, 2023) From 3cfd761dc13744f3134c933baf17aba8f484012e Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 11 Apr 2023 17:06:50 -0700 Subject: [PATCH 4/8] Adds CHANGELOG entry --- .changelog/30588.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.changelog/30588.txt b/.changelog/30588.txt index 8e12b685fd8a..fa15734f5559 100644 --- a/.changelog/30588.txt +++ b/.changelog/30588.txt @@ -1,3 +1,3 @@ -```release-note:bug -resource/aws_api_gateway_stage: `cache_cluster_size` is not updated if `cache_cluster_size` is `false` -``` \ No newline at end of file +```release-note:enhancement +resource/aws_servicecatalog_provisioned_product: Surfaces more clear error message when resource fails to apply +``` From c06a42f072cffafc2c46d82e879982b0ae41d11f Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 11 Apr 2023 17:07:10 -0700 Subject: [PATCH 5/8] Updates test names --- internal/service/servicecatalog/provisioned_product_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/servicecatalog/provisioned_product_test.go b/internal/service/servicecatalog/provisioned_product_test.go index dbf66aeb1019..8a4982a366f0 100644 --- a/internal/service/servicecatalog/provisioned_product_test.go +++ b/internal/service/servicecatalog/provisioned_product_test.go @@ -247,7 +247,7 @@ func TestAccServiceCatalogProvisionedProduct_tags(t *testing.T) { }) } -func TestAccServiceCatalogProvisionedProduct_taintedCreate(t *testing.T) { +func TestAccServiceCatalogProvisionedProduct_errorOnCreate(t *testing.T) { ctx := acctest.Context(t) rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -267,7 +267,7 @@ func TestAccServiceCatalogProvisionedProduct_taintedCreate(t *testing.T) { }) } -func TestAccServiceCatalogProvisionedProduct_taintedUpdate(t *testing.T) { +func TestAccServiceCatalogProvisionedProduct_errorOnUpdate(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_servicecatalog_provisioned_product.test" From 966fbf60743ce968c4b4502b9082195a98d403e0 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 12 Apr 2023 10:32:20 -0700 Subject: [PATCH 6/8] Corrects CHANGELOG entry PR number --- .changelog/{30588.txt => 30663.txt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{30588.txt => 30663.txt} (100%) diff --git a/.changelog/30588.txt b/.changelog/30663.txt similarity index 100% rename from .changelog/30588.txt rename to .changelog/30663.txt From 3257db04de301391c2b0b205dbe4e5f612723755 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 12 Apr 2023 10:38:18 -0700 Subject: [PATCH 7/8] Returns correct values from `StatusProvisionedProduct` --- .../servicecatalog/provisioned_product_test.go | 13 ++++++++++--- internal/service/servicecatalog/status.go | 6 +++--- internal/service/servicecatalog/wait.go | 4 ++-- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/internal/service/servicecatalog/provisioned_product_test.go b/internal/service/servicecatalog/provisioned_product_test.go index 8a4982a366f0..31745feff003 100644 --- a/internal/service/servicecatalog/provisioned_product_test.go +++ b/internal/service/servicecatalog/provisioned_product_test.go @@ -6,6 +6,7 @@ import ( "regexp" "testing" + "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" sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" @@ -310,15 +311,21 @@ func testAccCheckProvisionedProductDestroy(ctx context.Context) resource.TestChe continue } - err := tfservicecatalog.WaitProvisionedProductTerminated(ctx, conn, tfservicecatalog.AcceptLanguageEnglish, rs.Primary.ID, "", tfservicecatalog.ProvisionedProductDeleteTimeout) + input := &servicecatalog.DescribeProvisionedProductInput{ + Id: aws.String(rs.Primary.ID), + AcceptLanguage: aws.String(rs.Primary.Attributes["accept_language"]), + } + _, err := conn.DescribeProvisionedProductWithContext(ctx, input) if tfawserr.ErrCodeEquals(err, servicecatalog.ErrCodeResourceNotFoundException) { continue } if err != nil { - return fmt.Errorf("error getting Service Catalog Provisioned Product (%s): %w", rs.Primary.ID, err) + return err } + + return fmt.Errorf("Service Catalog Provisioned Product (%s) still exists", rs.Primary.ID) } return nil @@ -338,7 +345,7 @@ func testAccCheckProvisionedProductExists(ctx context.Context, resourceName stri _, err := tfservicecatalog.WaitProvisionedProductReady(ctx, conn, tfservicecatalog.AcceptLanguageEnglish, rs.Primary.ID, "", tfservicecatalog.ProvisionedProductReadyTimeout) if err != nil { - return fmt.Errorf("error describing Service Catalog Provisioned Product (%s): %w", rs.Primary.ID, err) + return fmt.Errorf("describing Service Catalog Provisioned Product (%s): %w", rs.Primary.ID, err) } return nil diff --git a/internal/service/servicecatalog/status.go b/internal/service/servicecatalog/status.go index 437897d98201..8f72160aad17 100644 --- a/internal/service/servicecatalog/status.go +++ b/internal/service/servicecatalog/status.go @@ -368,15 +368,15 @@ func StatusProvisionedProduct(ctx context.Context, conn *servicecatalog.ServiceC output, err := conn.DescribeProvisionedProductWithContext(ctx, input) if tfawserr.ErrCodeEquals(err, servicecatalog.ErrCodeResourceNotFoundException) { - return nil, StatusNotFound, err + return nil, "", nil } if err != nil { - return nil, servicecatalog.StatusFailed, err + return nil, "", err } if output == nil || output.ProvisionedProductDetail == nil { - return nil, StatusNotFound, err + return nil, "", nil } return output, aws.StringValue(output.ProvisionedProductDetail.Status), err diff --git a/internal/service/servicecatalog/wait.go b/internal/service/servicecatalog/wait.go index c2b6b64d5105..bafdcdda7169 100644 --- a/internal/service/servicecatalog/wait.go +++ b/internal/service/servicecatalog/wait.go @@ -494,7 +494,7 @@ func WaitLaunchPathsReady(ctx context.Context, conn *servicecatalog.ServiceCatal func WaitProvisionedProductReady(ctx context.Context, conn *servicecatalog.ServiceCatalog, acceptLanguage, id, name string, timeout time.Duration) (*servicecatalog.DescribeProvisionedProductOutput, error) { stateConf := &retry.StateChangeConf{ - Pending: []string{StatusNotFound, StatusUnavailable, servicecatalog.ProvisionedProductStatusUnderChange, servicecatalog.ProvisionedProductStatusPlanInProgress}, + Pending: []string{servicecatalog.ProvisionedProductStatusUnderChange, servicecatalog.ProvisionedProductStatusPlanInProgress}, Target: []string{servicecatalog.StatusAvailable}, Refresh: StatusProvisionedProduct(ctx, conn, acceptLanguage, id, name), Timeout: timeout, @@ -526,7 +526,7 @@ func WaitProvisionedProductReady(ctx context.Context, conn *servicecatalog.Servi func WaitProvisionedProductTerminated(ctx context.Context, conn *servicecatalog.ServiceCatalog, acceptLanguage, id, name string, timeout time.Duration) error { stateConf := &retry.StateChangeConf{ Pending: []string{servicecatalog.StatusAvailable, servicecatalog.ProvisionedProductStatusUnderChange}, - Target: []string{StatusNotFound, StatusUnavailable}, + Target: []string{}, Refresh: StatusProvisionedProduct(ctx, conn, acceptLanguage, id, name), Timeout: timeout, } From a1294a29876106ea35862c56b43c63096f691cac Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 13 Apr 2023 09:37:43 -0700 Subject: [PATCH 8/8] Deletes correct CHANGELOG entry file --- .changelog/30522.txt | 7 ------- .changelog/30588.txt | 3 +++ 2 files changed, 3 insertions(+), 7 deletions(-) delete mode 100644 .changelog/30522.txt create mode 100644 .changelog/30588.txt diff --git a/.changelog/30522.txt b/.changelog/30522.txt deleted file mode 100644 index 652e078b1c14..000000000000 --- a/.changelog/30522.txt +++ /dev/null @@ -1,7 +0,0 @@ -```release-note:bug -resource/aws_servicecatalog_provisioned_product: Allow the waiting that comes with updating and deleting to not error due to a `status` of `TAINTED` -``` - -```release-note:note -resource/aws_servicecatalog_provisioned_product: Previously, this resource would error or hang until timeout and then error, if a stack had a `status` of `TAINTED`. Now, the resource will continue functioning normally if the `status` is `TAINTED`. This is because the stack is in a "stable state" and "ready to perform any operation." However, although "the stack has completed the requested operation," it "is not exactly what was requested." The `status` will reflect `TAINTED` and `status_message` will provide clues as to what happened. See the `aws_servicecatalog_provisioned_product` documentation for more information and ways to use logging with this. -``` \ No newline at end of file diff --git a/.changelog/30588.txt b/.changelog/30588.txt new file mode 100644 index 000000000000..9ffd5af94a46 --- /dev/null +++ b/.changelog/30588.txt @@ -0,0 +1,3 @@ +``release-note:bug +resource/aws_api_gateway_stage: `cache_cluster_size` is not updated if `cache_cluster_size` is `false` +```