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.

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

test(storage): init retry conformance test scaffolding #4711

Merged
merged 36 commits into from Oct 18, 2021
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
a40676c
test(storage): add retry conformance tests [WIP]
tritone Dec 4, 2020
8c45efb
Merge branch 'master' into conf-test
tritone Mar 8, 2021
f606eeb
[WIP] test(storage): add retry conformance tests
tritone Mar 8, 2021
aa61155
separate idempotent/non-idempotent calls
tritone Mar 11, 2021
400a64b
rework schema
tritone Mar 25, 2021
ff9be59
get per-func errors/naming
tritone Mar 25, 2021
0abbad4
use instruction lists
tritone Mar 29, 2021
87f998e
update schema with fixtures
tritone Apr 2, 2021
57cc43e
update schema with comments
tritone Apr 2, 2021
c6385cd
update test cases
tritone Apr 2, 2021
14c25b6
fix id
tritone Apr 2, 2021
5ddf761
change to new emu format [WIP]
tritone Apr 16, 2021
d792232
Merge branch 'master' into conf-test
tritone Apr 30, 2021
4b1b2b9
use new retry_test resource
tritone May 3, 2021
36ae41c
gofmt
tritone May 3, 2021
d8fbc75
add test check and fix endpoints
tritone May 4, 2021
ec4ea20
change fixture to resource
tritone May 4, 2021
d4fd9c1
Merge branch 'master' into conf-scaffolding
BrennaEpp Aug 31, 2021
e0b4753
Merge branch 'master' into conf-scaffolding
BrennaEpp Aug 31, 2021
bd237cc
fix to changes
BrennaEpp Aug 31, 2021
31ebabd
fix
BrennaEpp Aug 31, 2021
74874e5
Merge branch 'master' into conf-scaffolding
BrennaEpp Sep 29, 2021
1974fa8
add some comments
BrennaEpp Sep 29, 2021
d6c80fb
call error from function
BrennaEpp Sep 29, 2021
4e4c1f8
add comments and remove extra url parsing
BrennaEpp Sep 29, 2021
0c98ce0
Merge branch 'master' into conf-scaffolding
BrennaEpp Sep 29, 2021
c157162
retry json align
BrennaEpp Oct 8, 2021
445f24c
Merge branch 'master' into conf-scaffolding
BrennaEpp Oct 8, 2021
31744de
goimports fix
BrennaEpp Oct 8, 2021
8d1ecfb
remove extra func
BrennaEpp Oct 8, 2021
018c255
Merge branch 'master' into conf-scaffolding
BrennaEpp Oct 9, 2021
afda9b6
Merge branch 'master' into conf-scaffolding
BrennaEpp Oct 12, 2021
8ce513a
Merge branch 'master' into conf-scaffolding
BrennaEpp Oct 18, 2021
ed848c2
separate retry tests
BrennaEpp Oct 18, 2021
907c050
Merge branch 'master' into conf-scaffolding
BrennaEpp Oct 18, 2021
acdd446
fix typos
BrennaEpp Oct 18, 2021
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
316 changes: 316 additions & 0 deletions storage/conformance_test.go
Expand Up @@ -16,21 +16,337 @@ package storage

import (
"bytes"
"context"
"encoding/base64"
"encoding/json"
"fmt"
"io/ioutil"
"net/http"
"net/http/httputil"
"net/url"
"os"
"strconv"
"strings"
"testing"
"time"

"cloud.google.com/go/internal/uid"
storage_v1_tests "cloud.google.com/go/storage/internal/test/conformance"
"github.com/golang/protobuf/jsonpb"
"github.com/google/go-cmp/cmp"
"google.golang.org/api/option"
raw "google.golang.org/api/storage/v1"
htransport "google.golang.org/api/transport/http"
)

var (
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
bucketIDs = uid.NewSpace("bucket", nil)
objectIDs = uid.NewSpace("object", nil)
notificationIDs = uid.NewSpace("notification", nil)
projectID = "my-project-id"
serviceAccountEmail = "my-sevice-account@my-project-id.iam.gserviceaccount.com"
)

// Holds the resources for a particular test case. Only the necessary fields will
// be populated; others will be nil.
type resources struct {
bucket *BucketAttrs
object *ObjectAttrs
notification *Notification
hmacKey *HMACKey
}

func (fs *resources) populate(ctx context.Context, c *Client, resource storage_v1_tests.Resource) error {
switch resource {
case storage_v1_tests.Resource_BUCKET:
bkt := c.Bucket(bucketIDs.New())
if err := bkt.Create(ctx, projectID, &BucketAttrs{}); err != nil {
return fmt.Errorf("creating bucket: %v", err)
}
attrs, err := bkt.Attrs(ctx)
if err != nil {
return fmt.Errorf("getting bucket attrs: %v", err)
}
fs.bucket = attrs
case storage_v1_tests.Resource_OBJECT:
// Assumes bucket has been populated first.
obj := c.Bucket(fs.bucket.Name).Object(objectIDs.New())
w := obj.NewWriter(ctx)
if _, err := w.Write([]byte("abcdef")); err != nil {
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("writing object: %v", err)
}
if err := w.Close(); err != nil {
return fmt.Errorf("closing object: %v", err)
}
attrs, err := obj.Attrs(ctx)
if err != nil {
return fmt.Errorf("getting object attrs: %v", err)
}
fs.object = attrs
case storage_v1_tests.Resource_NOTIFICATION:
// Assumes bucket has been populated first.
n, err := c.Bucket(fs.bucket.Name).AddNotification(ctx, &Notification{
TopicProjectID: projectID,
TopicID: notificationIDs.New(),
PayloadFormat: JSONPayload,
})
if err != nil {
return fmt.Errorf("adding notification: %v", err)
}
fs.notification = n
case storage_v1_tests.Resource_HMAC_KEY:
key, err := c.CreateHMACKey(ctx, projectID, serviceAccountEmail)
if err != nil {
return fmt.Errorf("creating HMAC key: %v", err)
}
fs.hmacKey = key
}
return nil
}

type retryFunc func(ctx context.Context, c *Client, fs *resources, preconditions bool) error

// Methods to retry. This is a map whose keys are a string describing a standard
// API call (e.g. storage.objects.get) and values are a list of functions which
// wrap library methods that implement these calls. There may be multiple values
// because multiple library methods may use the same call (e.g. get could be a
// read or just a metadata get).
var methods = map[string][]retryFunc{
"storage.buckets.get": {
func(ctx context.Context, c *Client, fs *resources, _ bool) error {
_, err := c.Bucket(fs.bucket.Name).Attrs(ctx)
return err
},
},
}

func TestRetryConformance(t *testing.T) {
host := os.Getenv("STORAGE_EMULATOR_HOST")
if host == "" {
t.Skip("This test must use the testbench emulator; set STORAGE_EMULATOR_HOST to run.")
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
}

ctx := context.Background()

// Create non-wrapped client to use for setup steps.
client, err := NewClient(ctx)
if err != nil {
t.Fatalf("storage.NewClient: %v", err)
}

_, _, testFiles := parseFiles(t)

for _, testFile := range testFiles {
for _, retryTest := range testFile.RetryTests {
for _, instructions := range retryTest.Cases {
for _, method := range retryTest.Methods {
if len(methods[method.Name]) == 0 {
t.Logf("No tests for operation %v", method.Name)
}
for i, fn := range methods[method.Name] {
testName := fmt.Sprintf("%v-%v-%v-%v", retryTest.Id, instructions.Instructions, method.Name, i)
t.Run(testName, func(t *testing.T) {

// Create the retry subtest
subtest := &retrySubtest{T: t, name: testName}
Copy link
Contributor

Choose a reason for hiding this comment

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

"subtest" seems like the wrong word here-- this creates/manages a retry test in the emulator, correct? Any thoughts on a better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named it that as each t.Run() calls each function it runs a subtest and this struct is used throughout this functio, but yeah there is probably a better name for it.
Maybe something like emulatorRetryTest?

err := subtest.create(host, map[string][]string{
method.Name: instructions.Instructions,
})
if err != nil {
t.Fatalf("setting up retry test: %v", err)
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
}

// Create necessary test resources in the emulator
fs := &resources{}
for _, resource := range method.Resources {
if err := fs.populate(ctx, client, resource); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should populating the resources be a method on the subtest as well? Not required but it feels slightly cleaner I guess.

t.Fatalf("creating test resources: %v", err)
}
}

// Test
err = fn(ctx, subtest.wrappedClient, fs, retryTest.PreconditionProvided)
if retryTest.ExpectSuccess && err != nil {
t.Errorf("want success, got %v", err)
}
if !retryTest.ExpectSuccess && err == nil {
t.Errorf("want failure, got success")
}

// Verify that all instructions were used up during the test
// (indicates that the client sent the correct requests).
if err := subtest.check(); err != nil {
t.Errorf("checking instructions: %v", err)
}

// Close out test in emulator.
if err := subtest.delete(); err != nil {
t.Errorf("deleting retry test: %v", err)
}
})
}

}
}
}
}

}

type retrySubtest struct {
*testing.T
name string
id string // ID to pass as a header in the test execution
host *url.URL // set the path when using; path is not guaranteed betwen calls
wrappedClient *Client
}

// Create a retry test resource in the emulator
func (rt *retrySubtest) create(host string, instructions map[string][]string) error {
endpoint, err := parseURL(host)
if err != nil {
return err
}
rt.host = endpoint

c := http.DefaultClient
data := struct {
Instructions map[string][]string `json:"instructions"`
}{
Instructions: instructions,
}

buf := new(bytes.Buffer)
if err := json.NewEncoder(buf).Encode(data); err != nil {
return fmt.Errorf("encoding request: %v", err)
}

rt.host.Path = "retry_test"
resp, err := c.Post(rt.host.String(), "application/json", buf)
if err != nil || resp.StatusCode != 200 {
return fmt.Errorf("creating retry test: err: %v, resp: %+v", err, resp)
}
defer func() {
closeErr := resp.Body.Close()
if err == nil {
err = closeErr
}
}()
testRes := struct {
TestID string `json:"id"`
}{}
if err := json.NewDecoder(resp.Body).Decode(&testRes); err != nil {
return fmt.Errorf("decoding test ID: %v", err)
}

rt.id = testRes.TestID

// Create wrapped client which will send emulator instructions
rt.host.Path = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! Curious is the main purpose of creating a wrappedClient in here to populate the retrySubtest struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wrappedClient adds the retry test header to each request, prints the request if the test fails and also adds the DevstorageFullControlScope to the https client transport. Correct, we do create it here to populate the struct

client, err := wrappedClient(rt.T, rt.host.String(), rt.id)
if err != nil {
return fmt.Errorf("creating wrapped client: %v", err)
}
rt.wrappedClient = client

return nil
}

// Verify that all instructions for a given retry testID have been used up.
func (rt *retrySubtest) check() error {
rt.host.Path = strings.Join([]string{"retry_test", rt.id}, "/")
c := http.DefaultClient
resp, err := c.Get(rt.host.String())
if err != nil || resp.StatusCode != 200 {
return fmt.Errorf("getting retry test: err: %v, resp: %+v", err, resp)
}
defer func() {
closeErr := resp.Body.Close()
if err == nil {
err = closeErr
}
}()
testRes := struct {
Instructions map[string][]string
Completed bool
}{}
if err := json.NewDecoder(resp.Body).Decode(&testRes); err != nil {
return fmt.Errorf("decoding response: %v", err)
}
if !testRes.Completed {
return fmt.Errorf("test not completed; unused instructions: %+v", testRes.Instructions)
}
return nil
}

// Delete a retry test resource.
func (rt *retrySubtest) delete() error {
rt.host.Path = strings.Join([]string{"retry_test", rt.id}, "/")
c := http.DefaultClient
req, err := http.NewRequest("DELETE", rt.host.String(), nil)
if err != nil {
return fmt.Errorf("creating request: %v", err)
}
resp, err := c.Do(req)
if err != nil || resp.StatusCode != 200 {
return fmt.Errorf("deleting test: err: %v, resp: %+v", err, resp)
}
return nil
}

type testRoundTripper struct {
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
*testing.T
rt http.RoundTripper
testID string
}

func (wt *testRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
r.Header.Set("x-retry-test-id", wt.testID)

requestDump, err := httputil.DumpRequest(r, false)
if err != nil {
wt.Logf("error creating request dump: %v", err)
}

resp, err := wt.rt.RoundTrip(r)
if err != nil {
wt.Logf("roundtrip error (may be expected): %v\nrequest: %s", err, requestDump)
}
return resp, err
}

// Create custom client that sends instruction
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
func wrappedClient(t *testing.T, host, testID string) (*Client, error) {
ctx := context.Background()
base := http.DefaultTransport
trans, err := htransport.NewTransport(ctx, base, option.WithScopes(raw.DevstorageFullControlScope),
option.WithUserAgent("custom-user-agent"))
if err != nil {
return nil, fmt.Errorf("failed to create http client: %v", err)
}
c := http.Client{Transport: trans}

// Add RoundTripper to the created HTTP client
wrappedTrans := &testRoundTripper{rt: c.Transport, testID: testID, T: t}
c.Transport = wrappedTrans

// Supply this client to storage.NewClient
client, err := NewClient(ctx, option.WithHTTPClient(&c), option.WithEndpoint(host+"/storage/v1/"))
return client, err
}

// A url is only parsed correctly by the url package if it has a scheme,
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
// so we have to check and build it ourselves if not supplied in host
// Assumes http if not provided
func parseURL(host string) (*url.URL, error) {
if strings.Contains(host, "://") {
return url.Parse(host)
} else {
url := &url.URL{Scheme: "http", Host: host}
return url, nil
}
}

func TestPostPolicyV4Conformance(t *testing.T) {
oldUTCNow := utcNow
defer func() {
Expand Down
45 changes: 45 additions & 0 deletions storage/internal/test/conformance/retry_tests.json
@@ -0,0 +1,45 @@
{
"retryTests": [
{
"id": 1,
"description": "always_idempotent",
"cases": [
{
"instructions": ["return-503", "return-503"]
},
{
"instructions": ["return-reset-connection", "return-reset-connection"]
},
{
"instructions": ["return-reset-connection", "return-503"]
}
],
"methods": [
{"name": "storage.bucket_acl.get", "resources": ["BUCKET"]},
{"name": "storage.bucket_acl.list", "resources": ["BUCKET"]},
{"name": "storage.buckets.delete", "resources": ["BUCKET"]},
{"name": "storage.buckets.get", "resources": ["BUCKET"]},
{"name": "storage.buckets.getIamPolicy", "resources": ["BUCKET"]},
{"name": "storage.buckets.insert", "resources": []},
{"name": "storage.buckets.list", "resources": ["BUCKET"]},
{"name": "storage.buckets.lockRetentionPolicy", "resources": ["BUCKET"]},
{"name": "storage.buckets.testIamPermissions", "resources": ["BUCKET"]},
{"name": "storage.default_object_acl.get", "resources": ["BUCKET"]},
{"name": "storage.default_object_acl.list", "resources": ["BUCKET"]},
{"name": "storage.hmacKey.delete", "resources": ["HMAC_KEY"]},
{"name": "storage.hmacKey.get", "resources": ["HMAC_KEY"]},
{"name": "storage.hmacKey.list", "resources": []},
BrennaEpp marked this conversation as resolved.
Show resolved Hide resolved
{"name": "storage.notifications.delete", "resources": ["BUCKET", "NOTIFICATION"]},
{"name": "storage.notifications.get", "resources": ["BUCKET", "NOTIFICATION"]},
{"name": "storage.notifications.list", "resources": ["BUCKET", "NOTIFICATION"]},
{"name": "storage.object_acl.get", "resources": ["BUCKET", "OBJECT"]},
{"name": "storage.object_acl.list", "resources": ["BUCKET", "OBJECT"]},
{"name": "storage.objects.get", "resources": ["BUCKET", "OBJECT"]},
{"name": "storage.objects.list", "resources": ["BUCKET", "OBJECT"]},
{"name": "storage.serviceaccount.get", "resources": []}
],
"preconditionProvided": false,
"expectSuccess": true
}
]
}