Storage bucket: Surface non-retryable object delete error (GoogleClou…

…dPlatform#3336)

* Storage bucket: Surface non-retryable object delete error

* storage: test deleting bucket with non-deletable objects error

third_party/terraform/resources/resource_storage_bucket.go

@@ -616,8 +616,8 @@ func resourceStorageBucketDelete(d *schema.ResourceData, meta interface{}) error
 	// Get the bucket
 	bucket := d.Get("name").(string)
 
-	var listError error
-	for {
+	var listError, deleteObjectError error
+	for deleteObjectError == nil {
 		res, err := config.clientStorage.Objects.List(bucket).Versions(true).Do()
 		if err != nil {
 			log.Printf("Error listing contents of bucket %s: %v", bucket, err)
@@ -672,10 +672,7 @@ func resourceStorageBucketDelete(d *schema.ResourceData, meta interface{}) error
 			wp.Submit(func() {
 				log.Printf("[TRACE] Attempting to delete %s", object.Name)
 				if err := config.clientStorage.Objects.Delete(bucket, object.Name).Generation(object.Generation).Do(); err != nil {
-					// We should really return an error here, but it doesn't really
-					// matter since the following step (bucket deletion) will fail
-					// with an error indicating objects are still present, and this
-					// log line will point to that object.
+					deleteObjectError = err
 					log.Printf("[ERR] Failed to delete storage object %s: %s", object.Name, err)
 				} else {
 					log.Printf("[TRACE] Successfully deleted %s", object.Name)
@@ -701,6 +698,9 @@ func resourceStorageBucketDelete(d *schema.ResourceData, meta interface{}) error
 	if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 409 && strings.Contains(gerr.Message, "not empty") && listError != nil {
 		return fmt.Errorf("could not delete non-empty bucket due to error when listing contents: %v", listError)
 	}
+	if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 409 && strings.Contains(gerr.Message, "not empty") && deleteObjectError != nil {
+		return fmt.Errorf("could not delete non-empty bucket due to error when deleting contents: %v", deleteObjectError)
+	}
 	if err != nil {
 		log.Printf("Error deleting bucket %s: %v", bucket, err)
 		return err

third_party/terraform/tests/resource_storage_bucket_test.go

@@ -590,6 +590,33 @@ func TestAccStorageBucket_forceDestroyWithVersioning(t *testing.T) {
 	})
 }
 
+func TestAccStorageBucket_forceDestroyObjectDeleteError(t *testing.T) {
+	t.Parallel()
+
+	bucketName := fmt.Sprintf("tf-test-acl-bucket-%d", acctest.RandInt())
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccStorageBucketDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccStorageBucket_forceDestroyWithRetentionPolicy(bucketName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckStorageBucketPutItem(bucketName),
+				),
+			},
+			{
+				Config:      testAccStorageBucket_forceDestroyWithRetentionPolicy(bucketName),
+				Destroy:     true,
+				ExpectError: regexp.MustCompile("could not delete non-empty bucket due to error when deleting contents"),
+			},
+			{
+				Config: testAccStorageBucket_forceDestroy(bucketName),
+			},
+		},
+	})
+}
 func TestAccStorageBucket_versioning(t *testing.T) {
 	t.Parallel()
 
@@ -1492,3 +1519,27 @@ resource "google_storage_bucket" "website" {
 }
 `, bucketName)
 }
+
+func testAccStorageBucket_forceDestroy(bucketName string) string {
+	return fmt.Sprintf(`
+resource "google_storage_bucket" "bucket" {
+  name = "%s"
+
+  force_destroy = true
+}
+`, bucketName)
+}
+
+func testAccStorageBucket_forceDestroyWithRetentionPolicy(bucketName string) string {
+	return fmt.Sprintf(`
+resource "google_storage_bucket" "bucket" {
+  name = "%s"
+
+  force_destroy = true
+
+  retention_policy {
+    retention_period = 3600
+  }
+}
+`, bucketName)
+}