fix(storage): fix endpoint selection logic #3172
Conversation
Sync from upstream repo
google-cla
bot
added
the
cla: yes
product-auto-label
bot
added
the
api: storage
|
To help understand the non-trivial endpoint selection logic, refer to the doc at: https://docs.google.com/document/d/1FmjwOW0g0y3QPUY5PQlQ4H2A0_ShPm3YjmdgOAj40zM/edit |
There was a problem hiding this comment.
Generally this looks good. Spoke with @andyrzhao offline and he will add tests. I think this should fix #2476 .
| if err != nil { | ||
| return nil, fmt.Errorf("storage client: %v", err) | ||
| } | ||
| if ep == "" { |
There was a problem hiding this comment.
So if I'm understanding this correctly, we no longer need the ep == "" case since we can always expect a valid endpoint to be returned given that we are passing in WithEndpoint to NewService. Is that correct?
There was a problem hiding this comment.
the endpoint is computed by htransport, which will now always return a non-empty endpoint after this change, since we are setting the defaults. Passing "WithEndpoint" to NewService is just a short-cut since the endpoint has already been computed (the alternative would be to pass the full set of user-specified ClientOptions to NewService, which would end up recomputing the endpoint.)
storage/storage.go
Outdated
| @@ -105,41 +106,48 @@ type Client struct { | |||
| func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error) { | |||
| var host, readHost, scheme string | |||
|
|
|||
| // In general, it is recommended to use NewService instead of NewClient, since NewService | |||
There was a problem hiding this comment.
I guess the recommended path is to call NewService without WithHTTPClient, which avoids the need for NewClient?
There was a problem hiding this comment.
Yes, that's right. Maybe we can consider exporting the internal http client from NewService in the future so it can support the direct-access, client reuse scenario found in reader.go.
tbpg
changed the title
tbpg
changed the title
| opts = append(opts, internaloption.WithDefaultEndpoint("https://storage.googleapis.com/storage/v1/")) | ||
| opts = append(opts, internaloption.WithDefaultMTLSEndpoint("https://storage.mtls.googleapis.com/storage/v1/")) |
There was a problem hiding this comment.
internaloption API question: why do we have WithDefault... rather than prepending the defaults like we do above (see "Prepend default options" comment)? Should have asked this when updating internaloption, but I didn't think of it.
There was a problem hiding this comment.
Ah yea, the reason why "WithDefaultEndpoint" was introduced is that we needed a way to distinguish between user specified override and auto-generated default endpoint. In other words, with the "Prepend" technique, downstream code has no way to tell whether someone wanted to override the endpoint explicitly, since it's all coming from a "WithEndpoint". This distinction is needed to support DCA, where we automatically upgrade the default endpoint to defaultMTLSendpoint, unless explicitly overridden by the user. Therefore, base clients should almost always use WithDefaultEndpoint. (Note that this PR sets WithEndpoint for NewService because the endpoint has already been computed - the alternative would be to pass in the full set of ClientOptions.)
There was a problem hiding this comment.
It seems like we should do that directly, rather than indirectly through "default" vs not. So, we could add another setting that's something like attemptDCA with a default of true. If the user specifies an option that means we shouldn't attempt DCA, set attemptDCA=false.
Right now, the logic you laid out (which makes sense) is all implicit, making this more difficult to understand. I could totally be missing something, though.
There was a problem hiding this comment.
So DCA aside, I'd argue that having the client options "WithDefaultEndpoint" and "WithDefaultMTLSEndpoint" actually makes the client intentions more explicit and less error prone, and we no longer have to rely on endpoint options being propagated in the right order, which is made even more troublesome by hand-written clients. Having the defaults as separate options also allows us to support other use-cases, such as "endpoint merging". For example, cbro added the following logic in https://github.com/googleapis/google-api-go-client/blob/master/transport/internal/dca/dca.go a while back:
// If the endpoint override is an address (host:port) rather than full base
// URL (ex. https://...), then the user-provided address will be merged into
// the default endpoint. For example, WithEndpoint("myhost:8000") and
// WithDefaultEndpoint("https://foo.com/bar/baz") will return "https://myhost:8080/bar/baz"
And circling back to your original comment about DCA, I agree the logic feels a bit like a black box today, but it is by design, as we eventually want to make it the default behavior that happens without any additional user configuration. You can read more about the DCA spec at https://google.aip.dev/auth/4114, including the env variables we already support today to control the behavior. Thanks!
🤖 I have created a release \*beep\* \*boop\* --- ## [1.13.0](https://www.github.com/googleapis/google-cloud-go/compare/storage/v1.12.0...v1.13.0) (2021-02-03) ### Features * **storage:** add missing StorageClass in BucketAttrsToUpdate ([#3038](https://www.github.com/googleapis/google-cloud-go/issues/3038)) ([2fa1b72](https://www.github.com/googleapis/google-cloud-go/commit/2fa1b727f8a7b20aa62fe0990530744f6c109be0)) * **storage:** add projection parameter for BucketHandle.Objects() ([#3549](https://www.github.com/googleapis/google-cloud-go/issues/3549)) ([9b9c3dc](https://www.github.com/googleapis/google-cloud-go/commit/9b9c3dce3ee10af5b6c4d070821bf47a861efd5b)) ### Bug Fixes * **storage:** fix endpoint selection logic ([#3172](https://www.github.com/googleapis/google-cloud-go/issues/3172)) ([99edf0d](https://www.github.com/googleapis/google-cloud-go/commit/99edf0d211a9e617f2586fbc83b6f9630da3c537)) This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
The hand-written storage.go client need to be updated to work with DCA (https://google.aip.dev/auth/4114).
In particular, we need to update endpoint selection logic taking into account of the custom http client used by reader.go, the rawService, as well as the STORAGE_EMULATOR_HOST override.