Merge pull request #30663 from hashicorp/f-servicecatalog-tainted

resource/aws_servicecatalog_provisioned_product: Return clearer error message on failure

.changelog/30588.txt

@@ -1,3 +1,3 @@
-```release-note:bug
+``release-note:bug
 resource/aws_api_gateway_stage: `cache_cluster_size` is not updated if `cache_cluster_size` is `false`
-```
+```

.changelog/30663.txt

@@ -0,0 +1,3 @@
+```release-note:enhancement
+resource/aws_servicecatalog_provisioned_product: Surfaces more clear error message when resource fails to apply
+```

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))
@@ -47,7 +43,6 @@ BUG FIXES:
 * 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: Prevent `db_instance_parameter_group_name` from causing errors on minor upgrades ([#30679](https://github.com/hashicorp/terraform-provider-aws/issues/30679))
 * 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)
 

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"
@@ -247,8 +248,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_errorOnCreate(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_errorOnUpdate(t *testing.T) {
 	ctx := acctest.Context(t)
 	resourceName := "aws_servicecatalog_provisioned_product.test"
 
@@ -268,17 +288,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,
-			},
 		},
 	})
 }
@@ -292,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
@@ -320,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
@@ -584,7 +609,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" {

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

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 (
@@ -495,8 +494,8 @@ 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)
+		Pending:                   []string{servicecatalog.ProvisionedProductStatusUnderChange, servicecatalog.ProvisionedProductStatusPlanInProgress},
+		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,8 +525,8 @@ 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},
-		Target:  []string{StatusNotFound, StatusUnavailable},
+		Pending: []string{servicecatalog.StatusAvailable, servicecatalog.ProvisionedProductStatusUnderChange},
+		Target:  []string{},
 		Refresh: StatusProvisionedProduct(ctx, conn, acceptLanguage, id, name),
 		Timeout: timeout,
 	}

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.