From 954b8f0ace2bbd7a612b3541eac507239ad2c50c Mon Sep 17 00:00:00 2001 From: Brenna Epp Date: Thu, 12 Aug 2021 23:20:37 -0500 Subject: [PATCH 1/8] fix(storage): remove unnecessary variable Variable seems to be duplicating information which led to a bug. --- storage/storage.go | 3 --- storage/writer.go | 3 --- 2 files changed, 6 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index 1b0610e380aa..82048923cd5f 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -90,8 +90,6 @@ type Client struct { raw *raw.Service // Scheme describes the scheme under the current host. scheme string - // EnvHost is the host set on the STORAGE_EMULATOR_HOST variable. - envHost string // ReadHost is the default host used on the reader. readHost string } @@ -152,7 +150,6 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error hc: hc, raw: rawService, scheme: scheme, - envHost: host, readHost: readHost, }, nil } diff --git a/storage/writer.go b/storage/writer.go index 345f8bdb4c9b..e468d5d8e879 100644 --- a/storage/writer.go +++ b/storage/writer.go @@ -125,9 +125,6 @@ func (w *Writer) open() error { if w.MD5 != nil { rawObj.Md5Hash = base64.StdEncoding.EncodeToString(w.MD5) } - if w.o.c.envHost != "" { - w.o.c.raw.BasePath = fmt.Sprintf("%s://%s", w.o.c.scheme, w.o.c.envHost) - } call := w.o.c.raw.Objects.Insert(w.o.bucket, rawObj). Media(pr, mediaOpts...). Projection("full"). From 4f6b1dc278793a80eb92f92b49647bdb0254e166 Mon Sep 17 00:00:00 2001 From: Brenna Epp Date: Thu, 12 Aug 2021 23:54:21 -0500 Subject: [PATCH 2/8] fix(storage): preserve supplied endpoint's scheme --- storage/storage.go | 3 ++- storage/storage_test.go | 27 ++++++++++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index 82048923cd5f..6a9426bb1d6e 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -139,12 +139,13 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error if err != nil { return nil, fmt.Errorf("storage client: %v", err) } - // Update readHost with the chosen endpoint. + // Update readHost and scheme with the chosen endpoint. u, err := url.Parse(ep) if err != nil { return nil, fmt.Errorf("supplied endpoint %q is not valid: %v", ep, err) } readHost = u.Host + scheme = u.Scheme return &Client{ hc: hc, diff --git a/storage/storage_test.go b/storage/storage_test.go index 55d00e2252f8..8f99e60ed65f 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -1252,6 +1252,7 @@ func TestAttrToFieldMapCoverage(t *testing.T) { func TestWithEndpoint(t *testing.T) { originalStorageEmulatorHost := os.Getenv("STORAGE_EMULATOR_HOST") testCases := []struct { + desc string CustomEndpoint string StorageEmulatorHost string WantRawBasePath string @@ -1259,6 +1260,7 @@ func TestWithEndpoint(t *testing.T) { WantScheme string }{ { + desc: "No endpoint or emulator host specified", CustomEndpoint: "", StorageEmulatorHost: "", WantRawBasePath: "https://storage.googleapis.com/storage/v1/", @@ -1266,6 +1268,7 @@ func TestWithEndpoint(t *testing.T) { WantScheme: "https", }, { + desc: "With specified https endpoint, no specified emulator host", CustomEndpoint: "https://fake.gcs.com:8080/storage/v1", StorageEmulatorHost: "", WantRawBasePath: "https://fake.gcs.com:8080/storage/v1", @@ -1273,6 +1276,15 @@ func TestWithEndpoint(t *testing.T) { WantScheme: "https", }, { + desc: "With specified http endpoint, no specified emulator host", + CustomEndpoint: "http://fake.gcs.com:8080/storage/v1", + StorageEmulatorHost: "", + WantRawBasePath: "http://fake.gcs.com:8080/storage/v1", + WantReadHost: "fake.gcs.com:8080", + WantScheme: "http", + }, + { + desc: "Emulator host specified, no specified endpoint", CustomEndpoint: "", StorageEmulatorHost: "http://emu.com", WantRawBasePath: "http://emu.com", @@ -1280,10 +1292,19 @@ func TestWithEndpoint(t *testing.T) { WantScheme: "http", }, { + desc: "Endpoint overrides emulator host when both are specified - https", CustomEndpoint: "https://fake.gcs.com:8080/storage/v1", StorageEmulatorHost: "http://emu.com", WantRawBasePath: "https://fake.gcs.com:8080/storage/v1", WantReadHost: "fake.gcs.com:8080", + WantScheme: "https", + }, + { + desc: "Endpoint overrides emulator host when both are specified - http", + CustomEndpoint: "http://fake.gcs.com:8080/storage/v1", + StorageEmulatorHost: "https://emu.com", + WantRawBasePath: "http://fake.gcs.com:8080/storage/v1", + WantReadHost: "fake.gcs.com:8080", WantScheme: "http", }, } @@ -1299,13 +1320,13 @@ func TestWithEndpoint(t *testing.T) { } if c.raw.BasePath != tc.WantRawBasePath { - t.Errorf("raw.BasePath not set correctly: got %v, want %v", c.raw.BasePath, tc.WantRawBasePath) + t.Errorf("%s: raw.BasePath not set correctly\n\tgot %v, want %v", tc.desc, c.raw.BasePath, tc.WantRawBasePath) } if c.readHost != tc.WantReadHost { - t.Errorf("readHost not set correctly: got %v, want %v", c.readHost, tc.WantReadHost) + t.Errorf("%s: readHost not set correctly\n\tgot %v, want %v", tc.desc, c.readHost, tc.WantReadHost) } if c.scheme != tc.WantScheme { - t.Errorf("scheme not set correctly: got %v, want %v", c.scheme, tc.WantScheme) + t.Errorf("%s: scheme not set correctly\n\tgot %v, want %v", tc.desc, c.scheme, tc.WantScheme) } } os.Setenv("STORAGE_EMULATOR_HOST", originalStorageEmulatorHost) From ad65048992f64970ae7f220c635f8beeca465083 Mon Sep 17 00:00:00 2001 From: Brenna Epp Date: Fri, 13 Aug 2021 01:22:44 -0500 Subject: [PATCH 3/8] fix(storage): accept emulator env var without scheme You can now use the client library with an emulator by setting the environment variable to a host, with or without a scheme You also no longer have to supply an endpoint to use the emulator; the endpoint is built for you --- storage/storage.go | 21 +++++++++++++++++---- storage/storage_test.go | 13 +++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index 6a9426bb1d6e..f169bfa816bb 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -120,13 +120,26 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error opts = append(opts, internaloption.WithDefaultEndpoint("https://storage.googleapis.com/storage/v1/")) opts = append(opts, internaloption.WithDefaultMTLSEndpoint("https://storage.mtls.googleapis.com/storage/v1/")) } else { - scheme = "http" - readHost = host + // Add scheme for user if not supplied in STORAGE_EMULATOR_HOST + var hostUrl *url.URL + if strings.Contains(host, "://") { + h, err := url.Parse(host) + if err != nil { + return nil, err + } + hostUrl = h + } else { + hostUrl = &url.URL{Scheme: "http", Host: host} + } + + // Prepend an endpoint for the user in case they don't supply one + hostUrl.Path = "storage/v1" + endpoint := hostUrl.String() opts = append([]option.ClientOption{option.WithoutAuthentication()}, opts...) - opts = append(opts, internaloption.WithDefaultEndpoint(host)) - opts = append(opts, internaloption.WithDefaultMTLSEndpoint(host)) + opts = append(opts, internaloption.WithDefaultEndpoint(endpoint)) + opts = append(opts, internaloption.WithDefaultMTLSEndpoint(endpoint)) } // htransport selects the correct endpoint among WithEndpoint (user override), WithDefaultEndpoint, and WithDefaultMTLSEndpoint. diff --git a/storage/storage_test.go b/storage/storage_test.go index 8f99e60ed65f..e5d9be9d2e8f 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -1287,7 +1287,15 @@ func TestWithEndpoint(t *testing.T) { desc: "Emulator host specified, no specified endpoint", CustomEndpoint: "", StorageEmulatorHost: "http://emu.com", - WantRawBasePath: "http://emu.com", + WantRawBasePath: "http://emu.com/storage/v1", + WantReadHost: "emu.com", + WantScheme: "http", + }, + { + desc: "Emulator host specified without scheme", + CustomEndpoint: "", + StorageEmulatorHost: "emu.com", + WantRawBasePath: "http://emu.com/storage/v1", WantReadHost: "emu.com", WantScheme: "http", }, @@ -1315,9 +1323,6 @@ func TestWithEndpoint(t *testing.T) { if err != nil { t.Fatalf("error creating client: %v", err) } - if err != nil { - t.Fatalf("error creating client: %v", err) - } if c.raw.BasePath != tc.WantRawBasePath { t.Errorf("%s: raw.BasePath not set correctly\n\tgot %v, want %v", tc.desc, c.raw.BasePath, tc.WantRawBasePath) From dd8f1b0ee4e9f3c72709091d27095080248add2e Mon Sep 17 00:00:00 2001 From: Brenna Epp Date: Fri, 13 Aug 2021 12:23:29 -0500 Subject: [PATCH 4/8] Rename var --- storage/storage.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index f169bfa816bb..86748d8519b5 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -121,20 +121,20 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error opts = append(opts, internaloption.WithDefaultMTLSEndpoint("https://storage.mtls.googleapis.com/storage/v1/")) } else { // Add scheme for user if not supplied in STORAGE_EMULATOR_HOST - var hostUrl *url.URL + var hostURL *url.URL if strings.Contains(host, "://") { h, err := url.Parse(host) if err != nil { return nil, err } - hostUrl = h + hostURL = h } else { - hostUrl = &url.URL{Scheme: "http", Host: host} + hostURL = &url.URL{Scheme: "http", Host: host} } // Prepend an endpoint for the user in case they don't supply one - hostUrl.Path = "storage/v1" - endpoint := hostUrl.String() + hostURL.Path = "storage/v1" + endpoint := hostURL.String() opts = append([]option.ClientOption{option.WithoutAuthentication()}, opts...) From 1383d95471d0c86babbbe330ba4f8b0d5b0494a7 Mon Sep 17 00:00:00 2001 From: Brenna Epp Date: Fri, 13 Aug 2021 13:18:25 -0500 Subject: [PATCH 5/8] Fix bug --- storage/storage.go | 2 +- storage/storage_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index 86748d8519b5..433793efb892 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -133,7 +133,7 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error } // Prepend an endpoint for the user in case they don't supply one - hostURL.Path = "storage/v1" + hostURL.Path = "storage/v1/" endpoint := hostURL.String() opts = append([]option.ClientOption{option.WithoutAuthentication()}, opts...) diff --git a/storage/storage_test.go b/storage/storage_test.go index e5d9be9d2e8f..3966ac4b6e2a 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -1287,7 +1287,7 @@ func TestWithEndpoint(t *testing.T) { desc: "Emulator host specified, no specified endpoint", CustomEndpoint: "", StorageEmulatorHost: "http://emu.com", - WantRawBasePath: "http://emu.com/storage/v1", + WantRawBasePath: "http://emu.com/storage/v1/", WantReadHost: "emu.com", WantScheme: "http", }, @@ -1295,7 +1295,7 @@ func TestWithEndpoint(t *testing.T) { desc: "Emulator host specified without scheme", CustomEndpoint: "", StorageEmulatorHost: "emu.com", - WantRawBasePath: "http://emu.com/storage/v1", + WantRawBasePath: "http://emu.com/storage/v1/", WantReadHost: "emu.com", WantScheme: "http", }, From cce5ff4113b0d1d3e4aac504f01e6258fe6e896b Mon Sep 17 00:00:00 2001 From: Brenna Epp Date: Fri, 13 Aug 2021 14:57:43 -0500 Subject: [PATCH 6/8] Small refactorings --- storage/storage.go | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index 055da85558a1..6fe7a362f6a3 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -101,7 +101,6 @@ type Client struct { // Clients should be reused instead of created as needed. The methods of Client // are safe for concurrent use by multiple goroutines. func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error) { - var host, readHost, scheme string // In general, it is recommended to use raw.NewService instead of htransport.NewClient // since raw.NewService configures the correct default endpoints when initializing the @@ -110,18 +109,15 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error // here so it can be re-used by both reader.go and raw.NewService. This means we need to // manually configure the default endpoint options on the http client. Furthermore, we // need to account for STORAGE_EMULATOR_HOST override when setting the default endpoints. - if host = os.Getenv("STORAGE_EMULATOR_HOST"); host == "" { - scheme = "https" - readHost = "storage.googleapis.com" - + if host := os.Getenv("STORAGE_EMULATOR_HOST"); host == "" { // Prepend default options to avoid overriding options passed by the user. opts = append([]option.ClientOption{option.WithScopes(ScopeFullControl), option.WithUserAgent(userAgent)}, opts...) opts = append(opts, internaloption.WithDefaultEndpoint("https://storage.googleapis.com/storage/v1/")) opts = append(opts, internaloption.WithDefaultMTLSEndpoint("https://storage.mtls.googleapis.com/storage/v1/")) } else { - // Add scheme for user if not supplied in STORAGE_EMULATOR_HOST var hostURL *url.URL + if strings.Contains(host, "://") { h, err := url.Parse(host) if err != nil { @@ -129,13 +125,15 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error } hostURL = h } else { + // Add scheme for user if not supplied in STORAGE_EMULATOR_HOST + // URL is only parsed correctly if it has a scheme, so we build it ourselves hostURL = &url.URL{Scheme: "http", Host: host} } - // Prepend an endpoint for the user in case they don't supply one hostURL.Path = "storage/v1/" endpoint := hostURL.String() + // Prepend the emulator host as default endpoint for the user opts = append([]option.ClientOption{option.WithoutAuthentication()}, opts...) opts = append(opts, internaloption.WithDefaultEndpoint(endpoint)) @@ -157,16 +155,12 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error if err != nil { return nil, fmt.Errorf("supplied endpoint %q is not valid: %v", ep, err) } - readHost = u.Host - if u.Scheme != "" { - scheme = u.Scheme - } return &Client{ hc: hc, raw: rawService, - scheme: scheme, - readHost: readHost, + scheme: u.Scheme, + readHost: u.Host, }, nil } From b1aa3771fecb00565751894bc4cecc42cf237fe6 Mon Sep 17 00:00:00 2001 From: Brenna Epp Date: Fri, 13 Aug 2021 15:41:00 -0500 Subject: [PATCH 7/8] Add test case for host:port --- storage/storage_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/storage/storage_test.go b/storage/storage_test.go index 3966ac4b6e2a..4d48f93e5126 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -1299,6 +1299,14 @@ func TestWithEndpoint(t *testing.T) { WantReadHost: "emu.com", WantScheme: "http", }, + { + desc: "Emulator host specified as host:port", + CustomEndpoint: "", + StorageEmulatorHost: "localhost:9000", + WantRawBasePath: "http://localhost:9000/storage/v1/", + WantReadHost: "localhost:9000", + WantScheme: "http", + }, { desc: "Endpoint overrides emulator host when both are specified - https", CustomEndpoint: "https://fake.gcs.com:8080/storage/v1", From 6a54a6d50a8cb4de4d4f84e6f87e2c22d8cffc69 Mon Sep 17 00:00:00 2001 From: Brenna N Epp Date: Fri, 13 Aug 2021 16:17:57 -0500 Subject: [PATCH 8/8] Update comment --- storage/storage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/storage.go b/storage/storage.go index 6fe7a362f6a3..fc4d67be702c 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -133,7 +133,7 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error hostURL.Path = "storage/v1/" endpoint := hostURL.String() - // Prepend the emulator host as default endpoint for the user + // Append the emulator host as default endpoint for the user opts = append([]option.ClientOption{option.WithoutAuthentication()}, opts...) opts = append(opts, internaloption.WithDefaultEndpoint(endpoint))