Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test(storage): add gRPC error code to retry to fix flaky PAP test #6974

Merged
merged 7 commits into from
Nov 1, 2022
Merged

test(storage): add gRPC error code to retry to fix flaky PAP test #6974

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
27329d8
test(storage): add gRPC error code to retry to fix flaky PAP test
BrennaEpp Nov 1, 2022
da21632
revert debug change
BrennaEpp Nov 1, 2022
6ae5016
typo
BrennaEpp Nov 1, 2022
e63e1d9
debug revert
BrennaEpp Nov 1, 2022
c08030a
remove magic nubmer
BrennaEpp Nov 1, 2022
a2ff233
rev
BrennaEpp Nov 1, 2022
8b9a17a
Merge branch 'main' into fix-pap
BrennaEpp Nov 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 21 additions & 4 deletions storage/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,13 +815,16 @@ func TestIntegration_PublicAccessPrevention(t *testing.T) {

// Now, making object public or making bucket public should succeed. Run with
// retry because ACL settings may take time to propagate.
idempotentOrNilRetry := func(err error) bool {
return err == nil || ShouldRetry(err)
retrier := func(err error) bool {
// Once ACL settings propagate, PAP should no longer be enforced and the call will suceed.
// In the meantime, while PAP is enforced, trying to set ACL results in:
// - FailedPrecondition for gRPC
// - condition not met (412) for HTTP
return ShouldRetry(err) || status.Code(err) == codes.FailedPrecondition || extractErrCode(err) == 412
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
}

ctxWithTimeout, cancelCtx := context.WithTimeout(ctx, time.Second*10)

a = o.Retryer(WithErrorFunc(idempotentOrNilRetry)).ACL()
a = o.Retryer(WithErrorFunc(retrier), WithPolicy(RetryAlways)).ACL()
err = a.Set(ctxWithTimeout, AllUsers, RoleReader)
cancelCtx()
if err != nil {
Expand Down Expand Up @@ -4818,6 +4821,7 @@ func retryOnNilAndTransientErrs(err error) bool {
return err == nil || ShouldRetry(err)
}
func retryOnTransient400and403(err error) bool {
fmt.Println("retrying")
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
var e *googleapi.Error
var ae *apierror.APIError
return ShouldRetry(err) ||
Expand All @@ -4832,3 +4836,16 @@ func skipGRPC(reason string) context.Context {
func skipHTTP(reason string) context.Context {
return context.WithValue(context.Background(), skipTransportTestKey("http"), reason)
}

// Extract the error code if it's a googleapi.Error
func extractErrCode(err error) int {
if err == nil {
return 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: perhaps http.StatusOK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was extracted from a method (see my requester pays PR). I could change it, but I'm not sure it's more accurate, as we can't check if the error would be a googleapi.Error if it existed. For example, in the RequesterPays test I pass in an error that may be a gRPC error - it might be confusing in that case for this method to return a 200?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair! I'm OK with that as well. I trust your judgement.

}
var e *googleapi.Error
if errors.As(err, &e) {
return e.Code
}

return -1
}