From 849fb879d1d2d0a0cd0bd426c6fd53806b9f9a24 Mon Sep 17 00:00:00 2001 From: Brenna N Epp Date: Wed, 31 Aug 2022 10:41:09 -0700 Subject: [PATCH] test(storage): fixes bugs introduced in #6510 (#6585) Fixes #6579, #6578, #6577, #6576 --- storage/integration_test.go | 76 +++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 33 deletions(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index afb911dfc3c..02ab34bd860 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -617,13 +617,8 @@ func TestIntegration_BucketPolicyOnly(t *testing.T) { // Metadata updates may be delayed up to 10s. Since we expect an error from // this call, we retry on a nil error until we get the non-retryable error // that we are expecting. - idempotentOrNilRetry := func(err error) bool { - return err == nil || ShouldRetry(err) - } - ctxWithTimeout, cancelCtx := context.WithTimeout(ctx, time.Second*10) - - b := bkt.Retryer(WithErrorFunc(idempotentOrNilRetry)) + b := bkt.Retryer(WithErrorFunc(retryOnNilAndTransientErrs)) _, err = b.ACL().List(ctxWithTimeout) cancelCtx() if err == nil { @@ -632,8 +627,7 @@ func TestIntegration_BucketPolicyOnly(t *testing.T) { // Confirm ObjectAccessControl returns error, for same reason as above. ctxWithTimeout, cancelCtx = context.WithTimeout(ctx, time.Second*10) - - _, err = o.Retryer(WithErrorFunc(idempotentOrNilRetry)).ACL().List(ctxWithTimeout) + _, err = o.Retryer(WithErrorFunc(retryOnNilAndTransientErrs)).ACL().List(ctxWithTimeout) cancelCtx() if err == nil { t.Errorf("ACL.List: expected object ACL list to fail") @@ -647,8 +641,12 @@ func TestIntegration_BucketPolicyOnly(t *testing.T) { } // Check that the object ACLs are the same. + + // Metadata updates may be delayed up to 10s. Before that, we can get a 400 + // indicating that uniform bucket-level access is still enabled. ctxWithTimeout, cancelCtx = context.WithTimeout(ctx, time.Second*10) - acls, err := o.Retryer(WithPolicy(RetryAlways)).ACL().List(ctxWithTimeout) + retryObj := o.Retryer(WithErrorFunc(retryOnTransient400and403)) + acls, err := retryObj.ACL().List(ctxWithTimeout) cancelCtx() if err != nil { t.Errorf("ACL.List: object ACL list failed: %v", err) @@ -659,6 +657,14 @@ func TestIntegration_BucketPolicyOnly(t *testing.T) { } } +func retryOnNilAndTransientErrs(err error) bool { + return err == nil || ShouldRetry(err) +} +func retryOnTransient400and403(err error) bool { + var e *googleapi.Error + return ShouldRetry(err) || errors.As(err, &e) && (e.Code == 400 || e.Code == 403) +} + func TestIntegration_UniformBucketLevelAccess(t *testing.T) { ctx := context.Background() client := testConfig(ctx, t) @@ -697,13 +703,8 @@ func TestIntegration_UniformBucketLevelAccess(t *testing.T) { // Confirm BucketAccessControl returns error. // We retry on nil to account for propagation delay in metadata update. - idempotentOrNilRetry := func(err error) bool { - return err == nil || ShouldRetry(err) - } - ctxWithTimeout, cancelCtx := context.WithTimeout(ctx, time.Second*10) - - b := bkt.Retryer(WithErrorFunc(idempotentOrNilRetry)) + b := bkt.Retryer(WithErrorFunc(retryOnNilAndTransientErrs)) _, err = b.ACL().List(ctxWithTimeout) cancelCtx() if err == nil { @@ -712,8 +713,7 @@ func TestIntegration_UniformBucketLevelAccess(t *testing.T) { // Confirm ObjectAccessControl returns error. ctxWithTimeout, cancelCtx = context.WithTimeout(ctx, time.Second*10) - - _, err = o.Retryer(WithErrorFunc(idempotentOrNilRetry)).ACL().List(ctxWithTimeout) + _, err = o.Retryer(WithErrorFunc(retryOnNilAndTransientErrs)).ACL().List(ctxWithTimeout) cancelCtx() if err == nil { t.Errorf("ACL.List: expected object ACL list to fail") @@ -727,8 +727,10 @@ func TestIntegration_UniformBucketLevelAccess(t *testing.T) { } // Check that the object ACLs are the same. + // We retry on 400 to account for propagation delay in metadata update. ctxWithTimeout, cancelCtx = context.WithTimeout(ctx, time.Second*10) - acls, err := o.Retryer(WithPolicy(RetryAlways)).ACL().List(ctxWithTimeout) + retryObj := o.Retryer(WithErrorFunc(retryOnTransient400and403)) + acls, err := retryObj.ACL().List(ctxWithTimeout) cancelCtx() if err != nil { t.Errorf("ACL.List: object ACL list failed: %v", err) @@ -1457,7 +1459,7 @@ func TestIntegration_Objects(t *testing.T) { testTime = time.Now().UTC() newBucketName := uidSpace.New() h := testHelper{t} - bkt := client.Bucket(newBucketName) + bkt := client.Bucket(newBucketName).Retryer(WithPolicy(RetryAlways)) h.mustCreate(bkt, testutil.ProjID(), nil) defer func() { @@ -2361,16 +2363,18 @@ func TestIntegration_ACL(t *testing.T) { } } - retryAllErrors := func(err error) bool { return err != nil } - - ctxWithTimeout, cancelCtx := context.WithTimeout(ctx, time.Second*10) - acl, err = o.Retryer(WithErrorFunc(retryAllErrors)).ACL().List(ctxWithTimeout) - cancelCtx() + // Retry to account for propagation delay in metadata update. + err = retry(ctx, func() error { + acl, err = o.ACL().List(ctx) + return err + }, func() error { + if !hasRule(acl, rule) { + return fmt.Errorf("hasRule: object ACL missing %+v", rule) + } + return nil + }) if err != nil { - t.Errorf("ACL.List: can't retrieve ACL of %v", name) - } - if !hasRule(acl, rule) { - t.Errorf("hasRule: object ACL missing %+v", rule) + t.Error(err) } if err := o.ACL().Delete(ctx, entity); err != nil { @@ -2393,12 +2397,18 @@ func TestIntegration_ACL(t *testing.T) { ctxWithTimeout, cancel := context.WithTimeout(ctx, time.Second*10) defer cancel() - bACL, err = bkt.Retryer(WithErrorFunc(retryAllErrors)).ACL().List(ctxWithTimeout) + // Retry to account for propagation delay in metadata update. + err = retry(ctx, func() error { + bACL, err = bkt.ACL().List(ctxWithTimeout) + return err + }, func() error { + if !hasRule(bACL, rule2) { + return fmt.Errorf("hasRule: bucket ACL missing %+v", rule2) + } + return nil + }) if err != nil { - t.Errorf("ACL.List: error while getting the ACL of the bucket: %v", err) - } - if !hasRule(bACL, rule2) { - t.Errorf("hasRule: bucket ACL missing %+v", rule2) + t.Error(err) } if err := bkt.ACL().Delete(ctx, entity2); err != nil {