Dataproc autogen bucket changes (#1171)

* add extra wait for storage bucket object deletion

* make timeout for object deletion 5 minutes, make it succeed 3 times

* delete the cluster before deleting the bucket

* deprecate delete_autogen_bucket

* improve deprecation message

google/resource_dataproc_cluster.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/helper/validation"
@@ -95,6 +96,10 @@ func resourceDataprocCluster() *schema.Resource {
 							Type:     schema.TypeBool,
 							Optional: true,
 							Default:  false,
+							Deprecated: "autogenerated buckets are shared by all clusters in the same region, " +
+								"so deleting this bucket could adversely harm other dataproc clusters. " +
+								"If you need a bucket that can be deleted, please create a new one and set the " +
+								"`staging_bucket` field",
 						},
 
 						"staging_bucket": {
@@ -832,15 +837,8 @@ func resourceDataprocClusterDelete(d *schema.ResourceData, meta interface{}) err
 
 	region := d.Get("region").(string)
 	clusterName := d.Get("name").(string)
-	deleteAutoGenBucket := d.Get("cluster_config.0.delete_autogen_bucket").(bool)
 	timeoutInMinutes := int(d.Timeout(schema.TimeoutDelete).Minutes())
 
-	if deleteAutoGenBucket {
-		if err := deleteAutogenBucketIfExists(d, meta); err != nil {
-			return err
-		}
-	}
-
 	log.Printf("[DEBUG] Deleting Dataproc cluster %s", clusterName)
 	op, err := config.clientDataproc.Projects.Regions.Clusters.Delete(
 		project, region, clusterName).Do()
@@ -855,6 +853,13 @@ func resourceDataprocClusterDelete(d *schema.ResourceData, meta interface{}) err
 	}
 
 	log.Printf("[INFO] Dataproc cluster %s has been deleted", d.Id())
+
+	if d.Get("cluster_config.0.delete_autogen_bucket").(bool) {
+		if err := deleteAutogenBucketIfExists(d, meta); err != nil {
+			return err
+		}
+	}
+
 	d.SetId("")
 	return nil
 }
@@ -942,6 +947,26 @@ func deleteStorageBucketContents(config *Config, bucket string) error {
 			}
 			log.Printf("[DEBUG] Attempting to delete autogenerated bucket (for dataproc cluster): Object deleted: %s \n\n", object.Name)
 		}
+
+		// Wait until they're actually deleted
+		refreshFunc := func() (interface{}, string, error) {
+			res, err := config.clientStorage.Objects.List(bucket).Do()
+			if err != nil {
+				return nil, "error", err
+			}
+			return res.Items, strconv.FormatBool(len(res.Items) == 0), nil
+		}
+		conf := &resource.StateChangeConf{
+			Pending:                   []string{"false"},
+			Target:                    []string{"true"},
+			Refresh:                   refreshFunc,
+			Timeout:                   5 * time.Minute,
+			ContinuousTargetOccurence: 3,
+		}
+		_, err := conf.WaitForState()
+		if err != nil {
+			return errwrap.Wrapf(fmt.Sprintf("Error waiting for all items to be deleted from bucket %q: {{err}}", bucket), err)
+		}
 	}
 
 	return nil

google/resource_dataproc_cluster_test.go

@@ -97,7 +97,7 @@ func TestAccDataprocCluster_basic(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_basic(rnd),
@@ -144,7 +144,7 @@ func TestAccDataprocCluster_basicWithInternalIpOnlyTrue(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_basicWithInternalIpOnlyTrue(rnd),
@@ -167,7 +167,7 @@ func TestAccDataprocCluster_basicWithMetadata(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_basicWithMetadata(rnd),
@@ -182,32 +182,6 @@ func TestAccDataprocCluster_basicWithMetadata(t *testing.T) {
 	})
 }
 
-func TestAccDataprocCluster_basicWithAutogenDeleteTrue(t *testing.T) {
-	t.Parallel()
-
-	var cluster dataproc.Cluster
-	rnd := acctest.RandString(10)
-	resource.Test(t, resource.TestCase{
-		PreCheck:     func() { testAccPreCheck(t) },
-		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(true),
-		Steps: []resource.TestStep{
-			{
-				Config: testAccDataprocCluster_basicWithAutogenDeleteTrue(rnd),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckDataprocClusterExists("google_dataproc_cluster.basic", &cluster),
-					resource.TestCheckResourceAttrSet("google_dataproc_cluster.basic", "cluster_config.0.bucket"),
-				),
-			},
-			{
-				// Force an explicit destroy
-				Config: emptyTFDefinition,
-				Check:  testAccCheckDataprocAutogenBucketDeleted(&cluster),
-			},
-		},
-	})
-}
-
 func TestAccDataprocCluster_singleNodeCluster(t *testing.T) {
 	t.Parallel()
 
@@ -216,7 +190,7 @@ func TestAccDataprocCluster_singleNodeCluster(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_singleNodeCluster(rnd),
@@ -243,7 +217,7 @@ func TestAccDataprocCluster_updatable(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_updatable(rnd, 2, 1),
@@ -275,7 +249,7 @@ func TestAccDataprocCluster_withStagingBucket(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_withStagingBucketAndCluster(clusterName, bucketName),
@@ -306,7 +280,7 @@ func TestAccDataprocCluster_withInitAction(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_withInitAction(rnd, bucketName, objectName),
@@ -329,7 +303,7 @@ func TestAccDataprocCluster_withConfigOverrides(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_withConfigOverrides(rnd),
@@ -354,7 +328,7 @@ func TestAccDataprocCluster_withServiceAcc(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_withServiceAcc(sa, rnd),
@@ -382,7 +356,7 @@ func TestAccDataprocCluster_withImageVersion(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_withImageVersion(rnd),
@@ -403,7 +377,7 @@ func TestAccDataprocCluster_withLabels(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_withLabels(rnd),
@@ -434,7 +408,7 @@ func TestAccDataprocCluster_withNetworkRefs(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
-		CheckDestroy: testAccCheckDataprocClusterDestroy(false),
+		CheckDestroy: testAccCheckDataprocClusterDestroy(),
 		Steps: []resource.TestStep{
 			{
 				Config: testAccDataprocCluster_withNetworkRefs(rnd, netName),
@@ -448,7 +422,7 @@ func TestAccDataprocCluster_withNetworkRefs(t *testing.T) {
 	})
 }
 
-func testAccCheckDataprocClusterDestroy(expectedBucketDestroy bool) resource.TestCheckFunc {
+func testAccCheckDataprocClusterDestroy() resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		config := testAccProvider.Meta().(*Config)
 
@@ -462,28 +436,23 @@ func testAccCheckDataprocClusterDestroy(expectedBucketDestroy bool) resource.Tes
 			}
 
 			attributes := rs.Primary.Attributes
-			computedBucket := attributes["cluster_config.0.bucket"]
 			project, err := getTestProject(rs.Primary, config)
 			if err != nil {
 				return err
 			}
 
-			// 1. Verify actual cluster deleted
-			if err := validateClusterDeleted(project, attributes["region"], rs.Primary.ID, config); err != nil {
-				return err
-			}
+			_, err = config.clientDataproc.Projects.Regions.Clusters.Get(
+				project, attributes["region"], rs.Primary.ID).Do()
 
-			// 2. Depending on delete_autogen_bucket setting, check if
-			//    autogen bucket is deleted
-			if expectedBucketDestroy {
-				return validateBucketDoesNotExist(computedBucket, config)
-			}
-
-			// 3. Many of the tests use the default delete_autogen_bucket setting (false)
-			//    Clean up to avoid dangling resources after test.
-			if err := emptyAndDeleteStorageBucket(config, computedBucket); err != nil {
-				return fmt.Errorf("Error occured trying to clean up autogenerate bucket after test %v", err)
+			if err != nil {
+				if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == http.StatusNotFound {
+					return nil
+				} else if ok {
+					return fmt.Errorf("Error validating cluster deleted. Code: %d. Message: %s", gerr.Code, gerr.Message)
+				}
+				return fmt.Errorf("Error validating cluster deleted. %s", err.Error())
 			}
+			return fmt.Errorf("Dataproc cluster still exists")
 		}
 
 		return nil
@@ -501,35 +470,6 @@ func testAccCheckDataprocClusterHasServiceScopes(t *testing.T, cluster *dataproc
 	}
 }
 
-func validateClusterDeleted(project, region, clusterName string, config *Config) error {
-	_, err := config.clientDataproc.Projects.Regions.Clusters.Get(
-		project, region, clusterName).Do()
-
-	if err != nil {
-		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == http.StatusNotFound {
-			return nil
-		} else if ok {
-			return fmt.Errorf("Error validating cluster deleted. Code: %d. Message: %s", gerr.Code, gerr.Message)
-		}
-		return fmt.Errorf("Error validating cluster deleted. %s", err.Error())
-	}
-	return fmt.Errorf("Dataproc cluster still exists")
-}
-
-func validateBucketDoesNotExist(bucket string, config *Config) error {
-	_, err := config.clientStorage.Buckets.Get(bucket).Do()
-
-	if err != nil {
-		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == http.StatusNotFound {
-			return nil
-		} else if ok {
-			return fmt.Errorf("Error validating bucket does not exist: http code error : %d, http message error: %s", gerr.Code, gerr.Message)
-		}
-		return fmt.Errorf("Error validating bucket does not exist: %s", err.Error())
-	}
-	return fmt.Errorf("Storage bucket %s still exists", bucket)
-}
-
 func validateBucketExists(bucket string, config *Config) (bool, error) {
 	_, err := config.clientStorage.Buckets.Get(bucket).Do()
 
@@ -561,13 +501,6 @@ func testAccCheckDataprocStagingBucketExists(bucketName string) resource.TestChe
 
 }
 
-func testAccCheckDataprocAutogenBucketDeleted(cluster *dataproc.Cluster) resource.TestCheckFunc {
-	return func(s *terraform.State) error {
-		config := testAccProvider.Meta().(*Config)
-		return validateBucketDoesNotExist(cluster.Config.ConfigBucket, config)
-	}
-}
-
 func testAccCheckDataprocClusterInitActionSucceeded(bucket, object string) resource.TestCheckFunc {
 
 	// The init script will have created an object in the specified bucket.
@@ -781,19 +714,6 @@ resource "google_dataproc_cluster" "basic" {
 `, rnd)
 }
 
-func testAccDataprocCluster_basicWithAutogenDeleteTrue(rnd string) string {
-	return fmt.Sprintf(`
-resource "google_dataproc_cluster" "basic" {
-	name                  = "dproc-cluster-test-%s"
-	region                = "us-central1"
-
-	cluster_config {
-		delete_autogen_bucket = true
-	}
-}
-`, rnd)
-}
-
 func testAccDataprocCluster_singleNodeCluster(rnd string) string {
 	return fmt.Sprintf(`
 resource "google_dataproc_cluster" "single_node_cluster" {

google/resource_dataproc_job_test.go

@@ -457,8 +457,6 @@ resource "google_dataproc_cluster" "basic" {
 	region                = "us-central1"
 
 	cluster_config {
-		delete_autogen_bucket = true
-
 		# Keep the costs down with smallest config we can get away with
 		software_config {
 			override_properties = {

website/docs/r/dataproc_cluster.html.markdown

@@ -32,7 +32,6 @@ resource "google_dataproc_cluster" "mycluster" {
     }
 
     cluster_config {
-        delete_autogen_bucket = true
         staging_bucket        = "dataproc-staging-bucket"
 
         master_config {
@@ -105,8 +104,6 @@ The **cluster_config** block supports:
 
 ```hcl
     cluster_config {
-        delete_autogen_bucket = true
-
         gce_cluster_config        { ... }
         master_config             { ... }
         worker_config             { ... }
@@ -126,10 +123,11 @@ The **cluster_config** block supports:
    with other clusters in the same region/zone also choosing to use the auto generation
    option.
 
-* `delete_autogen_bucket` (Optional) If this is set to true, upon destroying the cluster,
+* `delete_autogen_bucket` (Optional, Deprecated) If this is set to true, upon destroying the cluster,
    if no explicit `staging_bucket` was specified (i.e. an auto generated bucket was relied
    upon) then this auto generated bucket will also be deleted as part of the cluster destroy.
-   By default this is set to false.
+   By default this is set to false. This value is deprecated: autogenerated buckets are shared by
+   all clusters in the same region, so deleting the bucket could adversely harm other dataproc clusters.
 
 * `gce_cluster_config` (Optional) Common config settings for resources of Google Compute Engine cluster
    instances, applicable to all instances in the cluster. Structure defined below.