Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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 .

@dfawley dfawley assigned GarrettGutierrez1 and unassigned menghanl Oct 1, 2020
@@ -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:

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

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.

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

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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