From ffa903eeec61aa3869e5220e2f09371127b5c393 Mon Sep 17 00:00:00 2001
From: Chris Cotter <cjcotter@google.com>
Date: Thu, 2 Sep 2021 12:26:46 -0700
Subject: [PATCH] fix(storage): align retry idempotency (part 1) (#4715)

Certain non-idempotent operations were being retried by the
library, and certain idempotent operations were not. This
fixes the following operations:

Add retry: notifications.delete, service_account.get
Remove retry: bucket_acl.delete, bucket_acl.update,
  default_object_acl.delete, default_object_acl.update,
  object_acl.delete, object_acl.update, hmacKey.create

Additional PR on conditionally idempotent operations to come.
---
 storage/acl.go           | 44 ++++++++++++++--------------------------
 storage/hmac.go          |  7 +------
 storage/notifications.go |  4 +++-
 storage/storage.go       |  7 ++++++-
 4 files changed, 25 insertions(+), 37 deletions(-)

diff --git a/storage/acl.go b/storage/acl.go
index a28b89b10e8..79b398c2217 100644
--- a/storage/acl.go
+++ b/storage/acl.go
@@ -133,11 +133,9 @@ func (a *ACLHandle) bucketDefaultList(ctx context.Context) ([]ACLRule, error) {
 }
 
 func (a *ACLHandle) bucketDefaultDelete(ctx context.Context, entity ACLEntity) error {
-	return runWithRetry(ctx, func() error {
-		req := a.c.raw.DefaultObjectAccessControls.Delete(a.bucket, string(entity))
-		a.configureCall(ctx, req)
-		return req.Do()
-	})
+	req := a.c.raw.DefaultObjectAccessControls.Delete(a.bucket, string(entity))
+	a.configureCall(ctx, req)
+	return req.Do()
 }
 
 func (a *ACLHandle) bucketList(ctx context.Context) ([]ACLRule, error) {
@@ -161,24 +159,16 @@ func (a *ACLHandle) bucketSet(ctx context.Context, entity ACLEntity, role ACLRol
 		Entity: string(entity),
 		Role:   string(role),
 	}
-	err := runWithRetry(ctx, func() error {
-		req := a.c.raw.BucketAccessControls.Update(a.bucket, string(entity), acl)
-		a.configureCall(ctx, req)
-		_, err := req.Do()
-		return err
-	})
-	if err != nil {
-		return err
-	}
-	return nil
+	req := a.c.raw.BucketAccessControls.Update(a.bucket, string(entity), acl)
+	a.configureCall(ctx, req)
+	_, err := req.Do()
+	return err
 }
 
 func (a *ACLHandle) bucketDelete(ctx context.Context, entity ACLEntity) error {
-	return runWithRetry(ctx, func() error {
-		req := a.c.raw.BucketAccessControls.Delete(a.bucket, string(entity))
-		a.configureCall(ctx, req)
-		return req.Do()
-	})
+	req := a.c.raw.BucketAccessControls.Delete(a.bucket, string(entity))
+	a.configureCall(ctx, req)
+	return req.Do()
 }
 
 func (a *ACLHandle) objectList(ctx context.Context) ([]ACLRule, error) {
@@ -214,18 +204,14 @@ func (a *ACLHandle) objectSet(ctx context.Context, entity ACLEntity, role ACLRol
 		req = a.c.raw.ObjectAccessControls.Update(a.bucket, a.object, string(entity), acl)
 	}
 	a.configureCall(ctx, req)
-	return runWithRetry(ctx, func() error {
-		_, err := req.Do()
-		return err
-	})
+	_, err := req.Do()
+	return err
 }
 
 func (a *ACLHandle) objectDelete(ctx context.Context, entity ACLEntity) error {
-	return runWithRetry(ctx, func() error {
-		req := a.c.raw.ObjectAccessControls.Delete(a.bucket, a.object, string(entity))
-		a.configureCall(ctx, req)
-		return req.Do()
-	})
+	req := a.c.raw.ObjectAccessControls.Delete(a.bucket, a.object, string(entity))
+	a.configureCall(ctx, req)
+	return req.Do()
 }
 
 func (a *ACLHandle) configureCall(ctx context.Context, call interface{ Header() http.Header }) {
diff --git a/storage/hmac.go b/storage/hmac.go
index 7d8185f37b8..34b6d1fb579 100644
--- a/storage/hmac.go
+++ b/storage/hmac.go
@@ -214,12 +214,7 @@ func (c *Client) CreateHMACKey(ctx context.Context, projectID, serviceAccountEma
 
 	setClientHeader(call.Header())
 
-	var hkPb *raw.HmacKey
-	var err error
-	err = runWithRetry(ctx, func() error {
-		hkPb, err = call.Context(ctx).Do()
-		return err
-	})
+	hkPb, err := call.Context(ctx).Do()
 	if err != nil {
 		return nil, err
 	}
diff --git a/storage/notifications.go b/storage/notifications.go
index 84619b6d58c..08b3ec3327b 100644
--- a/storage/notifications.go
+++ b/storage/notifications.go
@@ -184,5 +184,7 @@ func (b *BucketHandle) DeleteNotification(ctx context.Context, id string) (err e
 	if b.userProject != "" {
 		call.UserProject(b.userProject)
 	}
-	return call.Context(ctx).Do()
+	return runWithRetry(ctx, func() error {
+		return call.Context(ctx).Do()
+	})
 }
diff --git a/storage/storage.go b/storage/storage.go
index fbf437f68d3..5b3230fe98d 100644
--- a/storage/storage.go
+++ b/storage/storage.go
@@ -1769,7 +1769,12 @@ func setEncryptionHeaders(headers http.Header, key []byte, copySource bool) erro
 // ServiceAccount fetches the email address of the given project's Google Cloud Storage service account.
 func (c *Client) ServiceAccount(ctx context.Context, projectID string) (string, error) {
 	r := c.raw.Projects.ServiceAccount.Get(projectID)
-	res, err := r.Context(ctx).Do()
+	var res *raw.ServiceAccount
+	var err error
+	err = runWithRetry(ctx, func() error {
+		res, err = r.Context(ctx).Do()
+		return err
+	})
 	if err != nil {
 		return "", err
 	}