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

transport: new stream with actual server name #5748

Merged
merged 10 commits into from Nov 18, 2022
28 changes: 22 additions & 6 deletions internal/transport/http2_client.go
Expand Up @@ -59,11 +59,15 @@ var clientConnectionCounter uint64

// http2Client implements the ClientTransport interface with HTTP2.
type http2Client struct {
lastRead int64 // Keep this field 64-bit aligned. Accessed atomically.
ctx context.Context
cancel context.CancelFunc
ctxDone <-chan struct{} // Cache the ctx.Done() chan.
userAgent string
lastRead int64 // Keep this field 64-bit aligned. Accessed atomically.
ctx context.Context
cancel context.CancelFunc
ctxDone <-chan struct{} // Cache the ctx.Done() chan.
userAgent string
// address contains the resolver returned address for this transport.
// If the `ServerName` field is set, it takes precedence over `CallHdr.Host`
// passed to `NewStream`, when determining the :authority header.
address resolver.Address
md metadata.MD
conn net.Conn // underlying communication channel
loopy *loopyWriter
Expand Down Expand Up @@ -314,6 +318,7 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts
cancel: cancel,
userAgent: opts.UserAgent,
registeredCompressors: grpcutil.RegisteredCompressors(),
address: addr,
conn: conn,
remoteAddr: conn.RemoteAddr(),
localAddr: conn.LocalAddr(),
Expand Down Expand Up @@ -702,7 +707,18 @@ func (e NewStreamError) Error() string {
// streams. All non-nil errors returned will be *NewStreamError.
func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (*Stream, error) {
ctx = peer.NewContext(ctx, t.getPeer())
headerFields, err := t.createHeaderFields(ctx, callHdr)

dupCallHdr := *callHdr
// ServerName field of the resolver returned address takes precedence over
// Host field of CallHdr to determine the :authority header. This is because,
// the ServerName field takes precedence for server authentication during
// TLS handshake, and the :authority header should match the value used
// for server authentication.
if t.address.ServerName != "" {
dupCallHdr.Host = t.address.ServerName
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@dfawley : I asked @holdno to make this copy here, because I wasn't completely sure of the downstream effects of changing the CallHdr.Host field. Could you please take a look. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's the right call; otherwise if you reuse callHdr across multiple attempts for one RPC, and one transport has an authority set and another doesn't, then you could end up with the wrong behavior.

Can we avoid making this copy, though, if there is no change?

if t.address.ServerName != "" {
	newCallHdr := *callHdr
	newCallHdr.Host = t.address.ServerName
	callHdr = &newCallHdr
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your advice, it looks better.


headerFields, err := t.createHeaderFields(ctx, &dupCallHdr)
if err != nil {
return nil, &NewStreamError{Err: err, AllowTransparentRetry: false}
}
Expand Down
34 changes: 34 additions & 0 deletions test/authority_test.go
Expand Up @@ -36,6 +36,8 @@ import (
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/resolver/manual"
"google.golang.org/grpc/status"
testpb "google.golang.org/grpc/test/grpc_testing"
)
Expand Down Expand Up @@ -205,3 +207,35 @@ func (s) TestColonPortAuthority(t *testing.T) {
t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err)
}
}

// TestAuthorityReplacedWithResolverAddress This test makes sure that the http2 client
// replaces the authority to the resolver address server name when it is set.
easwars marked this conversation as resolved.
Show resolved Hide resolved
func (s) TestAuthorityReplacedWithResolverAddress(t *testing.T) {
const expectedAuthority = "test.server.name"

ss := &stubserver.StubServer{
EmptyCallF: func(ctx context.Context, _ *testpb.Empty) (*testpb.Empty, error) {
return authorityChecker(ctx, expectedAuthority)
},
Network: "tcp",
easwars marked this conversation as resolved.
Show resolved Hide resolved
}
if err := ss.Start(nil); err != nil {
t.Fatalf("Error starting endpoint server: %v", err)
}
defer ss.Stop()

r := manual.NewBuilderWithScheme("whatever")
r.InitialState(resolver.State{Addresses: []resolver.Address{{Addr: ss.Address, ServerName: expectedAuthority}}})
cc, err := grpc.Dial(r.Scheme()+":///whatever", grpc.WithInsecure(), grpc.WithResolvers(r))
if err != nil {
t.Fatalf("grpc.Dial(%q) = %v", ss.Address, err)
}
defer cc.Close()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
_, err = testpb.NewTestServiceClient(cc).EmptyCall(ctx, &testpb.Empty{})
if err != nil {
easwars marked this conversation as resolved.
Show resolved Hide resolved
t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err)
easwars marked this conversation as resolved.
Show resolved Hide resolved
}
}