From 965a217f190f37afdb87c00dcafb566d346d818c Mon Sep 17 00:00:00 2001 From: BrennaEpp Date: Fri, 3 Dec 2021 11:23:06 -0800 Subject: [PATCH 1/2] feat(storage): add retry config to ACL handle methods --- storage/acl.go | 13 +++++----- storage/bucket.go | 6 ++++- storage/storage.go | 1 + storage/storage_test.go | 54 ++++++++++++++++++++++++++++++++--------- 4 files changed, 55 insertions(+), 19 deletions(-) diff --git a/storage/acl.go b/storage/acl.go index 79b398c2217..4b77cf5e0a7 100644 --- a/storage/acl.go +++ b/storage/acl.go @@ -73,6 +73,7 @@ type ACLHandle struct { object string isDefault bool userProject string // for requester-pays buckets + retry *retryConfig } // Delete permanently deletes the ACL entry for the given entity. @@ -120,12 +121,12 @@ func (a *ACLHandle) List(ctx context.Context) (rules []ACLRule, err error) { func (a *ACLHandle) bucketDefaultList(ctx context.Context) ([]ACLRule, error) { var acls *raw.ObjectAccessControls var err error - err = runWithRetry(ctx, func() error { + err = run(ctx, func() error { req := a.c.raw.DefaultObjectAccessControls.List(a.bucket) a.configureCall(ctx, req) acls, err = req.Do() return err - }) + }, a.retry, true) if err != nil { return nil, err } @@ -141,12 +142,12 @@ func (a *ACLHandle) bucketDefaultDelete(ctx context.Context, entity ACLEntity) e func (a *ACLHandle) bucketList(ctx context.Context) ([]ACLRule, error) { var acls *raw.BucketAccessControls var err error - err = runWithRetry(ctx, func() error { + err = run(ctx, func() error { req := a.c.raw.BucketAccessControls.List(a.bucket) a.configureCall(ctx, req) acls, err = req.Do() return err - }) + }, a.retry, true) if err != nil { return nil, err } @@ -174,12 +175,12 @@ func (a *ACLHandle) bucketDelete(ctx context.Context, entity ACLEntity) error { func (a *ACLHandle) objectList(ctx context.Context) ([]ACLRule, error) { var acls *raw.ObjectAccessControls var err error - err = runWithRetry(ctx, func() error { + err = run(ctx, func() error { req := a.c.raw.ObjectAccessControls.List(a.bucket, a.object) a.configureCall(ctx, req) acls, err = req.Do() return err - }) + }, a.retry, true) if err != nil { return nil, err } diff --git a/storage/bucket.go b/storage/bucket.go index 42888cc49c5..93ef7575216 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -146,6 +146,7 @@ func (b *BucketHandle) DefaultObjectACL() *ACLHandle { // for valid object names can be found at: // https://cloud.google.com/storage/docs/naming-objects func (b *BucketHandle) Object(name string) *ObjectHandle { + retry := b.retry.clone() return &ObjectHandle{ c: b.c, bucket: b.name, @@ -155,10 +156,11 @@ func (b *BucketHandle) Object(name string) *ObjectHandle { bucket: b.name, object: name, userProject: b.userProject, + retry: retry, }, gen: -1, userProject: b.userProject, - retry: b.retry.clone(), + retry: retry, } } @@ -1436,6 +1438,8 @@ func (b *BucketHandle) Retryer(opts ...RetryOption) *BucketHandle { opt.apply(retry) } b2.retry = retry + b2.acl.retry = retry + b2.defaultObjectACL.retry = retry return &b2 } diff --git a/storage/storage.go b/storage/storage.go index 07ba64cc9e9..f3953735429 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1806,6 +1806,7 @@ func (o *ObjectHandle) Retryer(opts ...RetryOption) *ObjectHandle { opt.apply(retry) } o2.retry = retry + o2.acl.retry = retry return &o2 } diff --git a/storage/storage_test.go b/storage/storage_test.go index b4f7a4ef429..6cbbc5aeede 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -876,7 +876,7 @@ func TestObjectRetryer(t *testing.T) { } // Test the interactions between ObjectHandle and BucketHandle Retryers and that -// they correctly configure the retry configuration +// they correctly configure the retry configuration for objects and ACLs func TestRetryer(t *testing.T) { testCases := []struct { name string @@ -1007,17 +1007,47 @@ func TestRetryer(t *testing.T) { o = o.Retryer(tc.objectOptions...) } - if diff := cmp.Diff( - o.retry, - tc.want, - cmp.AllowUnexported(retryConfig{}, gax.Backoff{}), - // ErrorFunc cannot be compared directly, but we check if both are - // either nil or non-nil. - cmp.Comparer(func(a, b func(err error) bool) bool { - return (a == nil && b == nil) || (a != nil && b != nil) - }), - ); diff != "" { - s.Fatalf("retry not configured correctly: %v", diff) + configHandleCases := []struct { + r *retryConfig + name string + want *retryConfig + }{ + { + name: "object.retry", + r: o.retry, + want: tc.want, + }, + { + name: "object.ACL()", + r: o.ACL().retry, + want: tc.want, + }, + { + name: "bucket.ACL()", + r: b.ACL().retry, + want: b.retry, + }, + { + name: "bucket.DefaultObjectACL()", + r: b.DefaultObjectACL().retry, + want: b.retry, + }, + } + for _, ac := range configHandleCases { + s.Run(ac.name, func(ss *testing.T) { + if diff := cmp.Diff( + ac.want, + ac.r, + cmp.AllowUnexported(retryConfig{}, gax.Backoff{}), + // ErrorFunc cannot be compared directly, but we check if both are + // either nil or non-nil. + cmp.Comparer(func(a, b func(err error) bool) bool { + return (a == nil && b == nil) || (a != nil && b != nil) + }), + ); diff != "" { + ss.Fatalf("retry not configured correctly: %v", diff) + } + }) } }) } From f64dfc84d9064ed01048c2626c947e6d93000c1a Mon Sep 17 00:00:00 2001 From: BrennaEpp Date: Tue, 7 Dec 2021 20:24:42 -0600 Subject: [PATCH 2/2] fill acl methods --- storage/acl.go | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/storage/acl.go b/storage/acl.go index 4b77cf5e0a7..9bdf96e3ed3 100644 --- a/storage/acl.go +++ b/storage/acl.go @@ -136,7 +136,10 @@ func (a *ACLHandle) bucketDefaultList(ctx context.Context) ([]ACLRule, error) { func (a *ACLHandle) bucketDefaultDelete(ctx context.Context, entity ACLEntity) error { req := a.c.raw.DefaultObjectAccessControls.Delete(a.bucket, string(entity)) a.configureCall(ctx, req) - return req.Do() + + return run(ctx, func() error { + return req.Do() + }, a.retry, false) } func (a *ACLHandle) bucketList(ctx context.Context) ([]ACLRule, error) { @@ -162,14 +165,18 @@ func (a *ACLHandle) bucketSet(ctx context.Context, entity ACLEntity, role ACLRol } req := a.c.raw.BucketAccessControls.Update(a.bucket, string(entity), acl) a.configureCall(ctx, req) - _, err := req.Do() - return err + return run(ctx, func() error { + _, err := req.Do() + return err + }, a.retry, false) } func (a *ACLHandle) bucketDelete(ctx context.Context, entity ACLEntity) error { req := a.c.raw.BucketAccessControls.Delete(a.bucket, string(entity)) a.configureCall(ctx, req) - return req.Do() + return run(ctx, func() error { + return req.Do() + }, a.retry, false) } func (a *ACLHandle) objectList(ctx context.Context) ([]ACLRule, error) { @@ -205,14 +212,18 @@ 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) - _, err := req.Do() - return err + return run(ctx, func() error { + _, err := req.Do() + return err + }, a.retry, false) } func (a *ACLHandle) objectDelete(ctx context.Context, entity ACLEntity) error { req := a.c.raw.ObjectAccessControls.Delete(a.bucket, a.object, string(entity)) a.configureCall(ctx, req) - return req.Do() + return run(ctx, func() error { + return req.Do() + }, a.retry, false) } func (a *ACLHandle) configureCall(ctx context.Context, call interface{ Header() http.Header }) {