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

client: implement support for "unix" resolver scheme #3890

Merged
merged 18 commits into from Oct 16, 2020
Merged

client: implement support for "unix" resolver scheme #3890

merged 18 commits into from Oct 16, 2020

Conversation

GarrettGutierrez1
Copy link
Contributor

@GarrettGutierrez1 GarrettGutierrez1 commented Sep 21, 2020

Implemented unix resolver. Fixes #2806 .

internal/transport/transport.go Outdated Show resolved Hide resolved
internal/resolver/unix/unix.go Outdated Show resolved Hide resolved
internal/resolver/unix/unix.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
internal/resolver/unix/unix.go Outdated Show resolved Hide resolved
internal/resolver/unix/unix.go Outdated Show resolved Hide resolved
internal/resolver/unix/unix.go Outdated Show resolved Hide resolved
internal/resolver/unix/unix.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
test/authority_test.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned GarrettGutierrez1 and unassigned menghanl Oct 1, 2020
internal/grpcutil/target_test.go Show resolved Hide resolved
@@ -268,7 +257,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
cc.authority = creds.Info().ServerName
} else if cc.dopts.insecure && cc.dopts.authority != "" {
cc.authority = cc.dopts.authority
} else if unixScheme {
} else if strings.HasPrefix(cc.target, "unix:") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to come from the resolver (with resolver.Address - is this the same as ServerName? @menghanl?). It can be done in a follow-up change but this is not what we want to be doing, ideally.

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually don't do that.

The :authority header doesn't come from resolver.Address.ServerName:

headerFields = append(headerFields, hpack.HeaderField{Name: ":authority", Value: callHdr.Host})

But the ClientHandshake does:

conn, authInfo, err = transportCreds.ClientHandshake(connectCtx, addr.ServerName, conn)

All the authority thing needs a cleanup..

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is what I thought and what I was worried about. The resolver needs a way to set the authority for addresses. I think ServerName is the right way to do that -- WDYT? Do we want a different handshaker name from authority header value?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they should be the same. For security reason?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed; I think we can consider it a bug that we are not using the address's ServerName for the authority header. But this can be done in a follow-up PR.

dialoptions.go Outdated Show resolved Hide resolved
internal/resolver/unix/unix.go Outdated Show resolved Hide resolved
internal/transport/networktype/networktype.go Outdated Show resolved Hide resolved
internal/transport/networktype/networktype.go Outdated Show resolved Hide resolved
internal/transport/proxy_test.go Outdated Show resolved Hide resolved
internal/transport/proxy.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
test/authority_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM modulo the one minor naming thing.

Please make a follow-up PR that sets resolver.Address.ServerName to "localhost" in the unix resolver and removes the special-casing for "unix" in Dial to set authority.

internal/resolver/unix/unix.go Outdated Show resolved Hide resolved
internal/grpcutil/target_test.go Outdated Show resolved Hide resolved
@@ -268,7 +257,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
cc.authority = creds.Info().ServerName
} else if cc.dopts.insecure && cc.dopts.authority != "" {
cc.authority = cc.dopts.authority
} else if unixScheme {
} else if strings.HasPrefix(cc.target, "unix:") {
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually don't do that.

The :authority header doesn't come from resolver.Address.ServerName:

headerFields = append(headerFields, hpack.HeaderField{Name: ":authority", Value: callHdr.Host})

But the ClientHandshake does:

conn, authInfo, err = transportCreds.ClientHandshake(connectCtx, addr.ServerName, conn)

All the authority thing needs a cleanup..

internal/grpcutil/target.go Outdated Show resolved Hide resolved
internal/transport/proxy.go Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "unix" scheme in new resolver/balancer API style
3 participants