Merge pull request #24758 from hashicorp/b-servicecatalog-provisioned-product-wait-for-status

r/servicecatalog_provisioned_product: remove waiter from read CRUD op to prevent irreversible state

Author: anGie44

.changelog/24758.txt

@@ -0,0 +1,10 @@
+```release-note:bug
+resource/aws_servicecatalog_provisioned_product: Prevent error when retrieving a provisioned product in a non-available state
+```
+
+```release-note:enhancement
+resource/aws_servicecatalog_provisioned_product: Wait for provisioning to finish
+```
+
+```release-note:enhancement
+resource/aws_servicecatalog_provisioned_product: Wait for update to finish

internal/service/servicecatalog/provisioned_product.go

@@ -347,6 +347,10 @@ func resourceProvisionedProductCreate(d *schema.ResourceData, meta interface{})
 
 	d.SetId(aws.StringValue(output.RecordDetail.ProvisionedProductId))
 
+	if _, err := WaitProvisionedProductReady(conn, d.Get("accept_language").(string), d.Id(), "", d.Timeout(schema.TimeoutCreate)); err != nil {
+		return fmt.Errorf("error waiting for Service Catalog Provisioned Product (%s) create: %w", d.Id(), err)
+	}
+
 	return resourceProvisionedProductRead(d, meta)
 }
 
@@ -368,7 +372,12 @@ func resourceProvisionedProductRead(d *schema.ResourceData, meta interface{}) er
 		acceptLanguage = v.(string)
 	}
 
-	output, err := WaitProvisionedProductReady(conn, acceptLanguage, d.Id(), "", d.Timeout(schema.TimeoutRead))
+	input := &servicecatalog.DescribeProvisionedProductInput{
+		Id:             aws.String(d.Id()),
+		AcceptLanguage: aws.String(acceptLanguage),
+	}
+
+	output, err := conn.DescribeProvisionedProduct(input)
 
 	if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, servicecatalog.ErrCodeResourceNotFoundException) {
 		log.Printf("[WARN] Service Catalog Provisioned Product (%s) not found, removing from state", d.Id())
@@ -514,6 +523,10 @@ func resourceProvisionedProductUpdate(d *schema.ResourceData, meta interface{})
 		return fmt.Errorf("error updating Service Catalog Provisioned Product (%s): %w", d.Id(), err)
 	}
 
+	if _, err := WaitProvisionedProductReady(conn, d.Get("accept_language").(string), d.Id(), "", d.Timeout(schema.TimeoutUpdate)); err != nil {
+		return fmt.Errorf("error waiting for Service Catalog Provisioned Product (%s) update: %w", d.Id(), err)
+	}
+
 	return resourceProvisionedProductRead(d, meta)
 }
 

internal/service/servicecatalog/provisioned_product_test.go

@@ -30,7 +30,72 @@ func TestAccServiceCatalogProvisionedProduct_basic(t *testing.T) {
 		CheckDestroy:      testAccCheckProvisionedProductDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccProvisionedProductConfig_basic(rName, domain, acctest.DefaultEmailAddress),
+				Config: testAccProvisionedProductConfig_basic(rName, domain, acctest.DefaultEmailAddress, "10.1.0.0/16"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckProvisionedProductExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "accept_language", tfservicecatalog.AcceptLanguageEnglish),
+					acctest.MatchResourceAttrRegionalARN(resourceName, "arn", servicecatalog.ServiceName, regexp.MustCompile(fmt.Sprintf(`stack/%s/pp-.*`, rName))),
+					acctest.CheckResourceAttrRFC3339(resourceName, "created_time"),
+					resource.TestCheckResourceAttrSet(resourceName, "last_provisioning_record_id"),
+					resource.TestCheckResourceAttrSet(resourceName, "last_record_id"),
+					resource.TestCheckResourceAttrSet(resourceName, "last_successful_provisioning_record_id"),
+					resource.TestCheckResourceAttr(resourceName, "name", rName),
+					// One output will default to the launched CloudFormation Stack (provisioned outside terraform).
+					// While another output will describe the output parameter configured in the S3 object resource,
+					// which we can check as follows.
+					resource.TestCheckResourceAttr(resourceName, "outputs.#", "2"),
+					resource.TestCheckTypeSetElemNestedAttrs(resourceName, "outputs.*", map[string]string{
+						"description": "VPC ID",
+						"key":         "VpcID",
+					}),
+					resource.TestMatchTypeSetElemNestedAttrs(resourceName, "outputs.*", map[string]*regexp.Regexp{
+						"value": regexp.MustCompile(`vpc-.+`),
+					}),
+					resource.TestCheckResourceAttrPair(resourceName, "path_id", "data.aws_servicecatalog_launch_paths.test", "summaries.0.path_id"),
+					resource.TestCheckResourceAttrPair(resourceName, "product_id", "aws_servicecatalog_product.test", "id"),
+					resource.TestCheckResourceAttrPair(resourceName, "provisioning_artifact_name", "aws_servicecatalog_product.test", "provisioning_artifact_parameters.0.name"),
+					resource.TestCheckResourceAttr(resourceName, "status", servicecatalog.StatusAvailable),
+					resource.TestCheckResourceAttr(resourceName, "type", "CFN_STACK"),
+				),
+			},
+			{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"accept_language",
+					"ignore_errors",
+					"provisioning_artifact_name",
+					"provisioning_parameters",
+					"retain_physical_resources",
+				},
+			},
+		},
+	})
+}
+
+// TestAccServiceCatalogProvisionedProduct_update verifies the resource update
+// of only a change in provisioning_parameters
+func TestAccServiceCatalogProvisionedProduct_update(t *testing.T) {
+	resourceName := "aws_servicecatalog_provisioned_product.test"
+
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+	domain := fmt.Sprintf("http://%s", acctest.RandomDomainName())
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:          func() { acctest.PreCheck(t) },
+		ErrorCheck:        acctest.ErrorCheck(t, servicecatalog.EndpointsID),
+		ProviderFactories: acctest.ProviderFactories,
+		CheckDestroy:      testAccCheckProvisionedProductDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccProvisionedProductConfig_basic(rName, domain, acctest.DefaultEmailAddress, "10.1.0.0/16"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckProvisionedProductExists(resourceName),
+				),
+			},
+			{
+				Config: testAccProvisionedProductConfig_basic(rName, domain, acctest.DefaultEmailAddress, "10.10.0.0/16"),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckProvisionedProductExists(resourceName),
 					resource.TestCheckResourceAttr(resourceName, "accept_language", tfservicecatalog.AcceptLanguageEnglish),
@@ -87,7 +152,7 @@ func TestAccServiceCatalogProvisionedProduct_disappears(t *testing.T) {
 		CheckDestroy:      testAccCheckProvisionedProductDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccProvisionedProductConfig_basic(rName, domain, acctest.DefaultEmailAddress),
+				Config: testAccProvisionedProductConfig_basic(rName, domain, acctest.DefaultEmailAddress, "10.1.0.0/16"),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckProvisionedProductExists(resourceName),
 					acctest.CheckResourceDisappears(acctest.Provider, tfservicecatalog.ResourceProvisionedProduct(), resourceName),
@@ -298,7 +363,7 @@ data "aws_servicecatalog_launch_paths" "test" {
 `, rName, domain, email)
 }
 
-func testAccProvisionedProductConfig_basic(rName, domain, email string) string {
+func testAccProvisionedProductConfig_basic(rName, domain, email, vpcCidr string) string {
 	return acctest.ConfigCompose(testAccProvisionedProductTemplateURLBaseConfig(rName, domain, email),
 		fmt.Sprintf(`
 resource "aws_servicecatalog_provisioned_product" "test" {
@@ -309,15 +374,15 @@ resource "aws_servicecatalog_provisioned_product" "test" {
 
   provisioning_parameters {
     key   = "VPCPrimaryCIDR"
-    value = "10.1.0.0/16"
+    value = %[2]q
   }
 
   provisioning_parameters {
     key   = "LeaveMeEmpty"
     value = ""
   }
 }
-`, rName))
+`, rName, vpcCidr))
 }
 
 func testAccProvisionedProductConfig_tags(rName, tagKey, tagValue, domain, email string) string {

internal/service/servicecatalog/wait.go

@@ -1,11 +1,14 @@
 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"
 )
@@ -514,6 +517,7 @@ func WaitProvisionedProductReady(conn *servicecatalog.ServiceCatalog, acceptLang
 	outputRaw, err := stateConf.WaitForState()
 
 	if output, ok := outputRaw.(*servicecatalog.DescribeProvisionedProductOutput); ok {
+		tfresource.SetLastError(err, errors.New(aws.StringValue(output.ProvisionedProductDetail.StatusMessage)))
 		return output, err
 	}
 
@@ -547,6 +551,16 @@ func WaitRecordReady(conn *servicecatalog.ServiceCatalog, acceptLanguage, id str
 	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
 	}