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

fix(storage): fix endpoint selection logic #3172

Merged
merged 13 commits into from Nov 16, 2020
Merged

Conversation

@andyrzhao
Copy link
Contributor

@andyrzhao andyrzhao commented Nov 9, 2020

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.

@andyrzhao
Copy link
Contributor Author

@andyrzhao andyrzhao commented Nov 9, 2020

To help understand the non-trivial endpoint selection logic, refer to the doc at: https://docs.google.com/document/d/1FmjwOW0g0y3QPUY5PQlQ4H2A0_ShPm3YjmdgOAj40zM/edit

Loading

@tritone tritone requested review from tbpg, broady and codyoss Nov 11, 2020
Copy link
Collaborator

@tritone tritone left a comment

Generally this looks good. Spoke with @andyrzhao offline and he will add tests. I think this should fix #2476 .

Loading

if err != nil {
return nil, fmt.Errorf("storage client: %v", err)
}
if ep == "" {
Copy link
Collaborator

@tritone tritone Nov 11, 2020

Choose a reason for hiding this comment

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

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?

Loading

Copy link
Contributor Author

@andyrzhao andyrzhao Nov 11, 2020

Choose a reason for hiding this comment

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

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.)

Loading

@@ -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
Copy link
Collaborator

@tritone tritone Nov 11, 2020

Choose a reason for hiding this comment

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

I guess the recommended path is to call NewService without WithHTTPClient, which avoids the need for NewClient?

Loading

Copy link
Contributor Author

@andyrzhao andyrzhao Nov 11, 2020

Choose a reason for hiding this comment

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

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.

Loading

@tbpg tbpg changed the title fix(storage): Fix endpoint selection logic in storage.go fix(storage): fix endpoint selection logic in storage.go Nov 11, 2020
@tbpg tbpg changed the title fix(storage): fix endpoint selection logic in storage.go fix(storage): fix endpoint selection logic Nov 11, 2020
storage/storage.go Outdated Show resolved Hide resolved
Loading
storage/storage.go Outdated Show resolved Hide resolved
Loading
opts = append(opts, internaloption.WithDefaultEndpoint("https://storage.googleapis.com/storage/v1/"))
opts = append(opts, internaloption.WithDefaultMTLSEndpoint("https://storage.mtls.googleapis.com/storage/v1/"))
Copy link
Collaborator

@tbpg tbpg Nov 11, 2020

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@andyrzhao andyrzhao Nov 11, 2020

Choose a reason for hiding this comment

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

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.)

Loading

Copy link
Collaborator

@tbpg tbpg Nov 12, 2020

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@andyrzhao andyrzhao Nov 12, 2020

Choose a reason for hiding this comment

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

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!

Loading

storage/storage.go Outdated Show resolved Hide resolved
Loading
storage/storage.go Outdated Show resolved Hide resolved
Loading
@andyrzhao andyrzhao requested a review from as a code owner Nov 12, 2020
@andyrzhao andyrzhao requested review from tbpg and tritone Nov 12, 2020
tbpg
tbpg approved these changes Nov 16, 2020
@tritone tritone merged commit 99edf0d into googleapis:master Nov 16, 2020
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants