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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

Implemented unix resolver. Fixes #2806 .

@GarrettGutierrez1 GarrettGutierrez1 added this to the 1.33 Release milestone Sep 21, 2020
menghanl
menghanl suggested changes Sep 21, 2020
View reviewed changes
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
dfawley
dfawley reviewed Oct 1, 2020
View reviewed changes
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
dfawley
dfawley reviewed Oct 9, 2020
View reviewed changes
internal/grpcutil/target_test.go Show resolved Hide resolved
clientconn.go
@@ -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
Member

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:

grpc-go/internal/transport/http2_client.go

Line 439 in c6cfaba

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

But the ClientHandshake does:

grpc-go/internal/transport/http2_client.go

Line 225 in c6cfaba

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

All the authority thing needs a cleanup..

Copy link
Member

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
Member

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
dfawley
dfawley approved these changes Oct 15, 2020
View reviewed changes
Copy link
Member

@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
menghanl
menghanl reviewed Oct 15, 2020
View reviewed changes
internal/grpcutil/target_test.go Outdated Show resolved Hide resolved
clientconn.go
@@ -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:

grpc-go/internal/transport/http2_client.go

Line 439 in c6cfaba

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

But the ClientHandshake does:

grpc-go/internal/transport/http2_client.go

Line 225 in c6cfaba

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
@GarrettGutierrez1 GarrettGutierrez1 merged commit 4be647f into grpc:master Oct 16, 2020
@menghanl menghanl mentioned this pull request Oct 26, 2020
Closed
@dfawley dfawley mentioned this pull request Nov 5, 2020
Closed
@dfawley dfawley changed the title Unix resolver client: implement support for "unix" resolver scheme Dec 2, 2020
@dfawley dfawley added the Type: Feature New features or improvements in behavior label Dec 2, 2020
This was referenced Mar 12, 2021
Closed
Closed
Closed
@mdbooth mdbooth mentioned this pull request May 13, 2021
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

@menghanl menghanl menghanl approved these changes

Assignees

@GarrettGutierrez1 GarrettGutierrez1

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.34 Release
Development

Successfully merging this pull request may close these issues.

Support "unix" scheme in new resolver/balancer API style
3 participants
@GarrettGutierrez1 @menghanl @dfawley