From 40ec7887be2c8f5a5dae0c991b0eb432398214b3 Mon Sep 17 00:00:00 2001 From: holdno Date: Fri, 28 Oct 2022 18:32:25 +0800 Subject: [PATCH 01/10] transport: new stream with actual server name --- internal/transport/http2_client.go | 29 ++++++++++++++++++----------- internal/transport/transport.go | 3 +++ stream.go | 7 +++++++ 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index d518b07e16f7..40147ce892c1 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -59,17 +59,18 @@ 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 - md metadata.MD - conn net.Conn // underlying communication channel - loopy *loopyWriter - remoteAddr net.Addr - localAddr net.Addr - authInfo credentials.AuthInfo // auth info about the connection + 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 + usedAddress resolver.Address // Record the used resolver address of client, and replace :authority to resolver address if serverName is not empty. + md metadata.MD + conn net.Conn // underlying communication channel + loopy *loopyWriter + remoteAddr net.Addr + localAddr net.Addr + authInfo credentials.AuthInfo // auth info about the connection readerDone chan struct{} // sync point to enable testing. writerDone chan struct{} // sync point to enable testing. @@ -314,6 +315,7 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts cancel: cancel, userAgent: opts.UserAgent, registeredCompressors: grpcutil.RegisteredCompressors(), + usedAddress: addr, // resolver address conn: conn, remoteAddr: conn.RemoteAddr(), localAddr: conn.LocalAddr(), @@ -454,6 +456,11 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts return t, nil } +// GetUsedResolverAddress return the transport used resolver address meta info +func (t *http2Client) GetUsedResolverAddress() resolver.Address { + return t.usedAddress +} + func (t *http2Client) newStream(ctx context.Context, callHdr *CallHdr) *Stream { // TODO(zhaoq): Handle uint32 overflow of Stream.id. s := &Stream{ diff --git a/internal/transport/transport.go b/internal/transport/transport.go index e21587b5321c..bac7c1fd85a0 100644 --- a/internal/transport/transport.go +++ b/internal/transport/transport.go @@ -657,6 +657,9 @@ type ClientTransport interface { // with a human readable string with debug info. GetGoAwayReason() (GoAwayReason, string) + // GetUsedResolverAddress return the transport used resolver address meta info + GetUsedResolverAddress() resolver.Address + // RemoteAddr returns the remote network address. RemoteAddr() net.Addr diff --git a/stream.go b/stream.go index b10ab1ab6324..72e727156f77 100644 --- a/stream.go +++ b/stream.go @@ -455,6 +455,13 @@ func (a *csAttempt) getTransport() error { func (a *csAttempt) newStream() error { cs := a.cs cs.callHdr.PreviousAttempts = cs.numRetries + + // Replace with the actual serverName, if it exist + addr := a.t.GetUsedResolverAddress() + if addr.ServerName != "" { + cs.callHdr.Host = addr.ServerName + } + s, err := a.t.NewStream(a.ctx, cs.callHdr) if err != nil { nse, ok := err.(*transport.NewStreamError) From 2b12d75fd644d58b3872387cc4207dbb5a33b7b3 Mon Sep 17 00:00:00 2001 From: holdno Date: Fri, 4 Nov 2022 16:34:17 +0800 Subject: [PATCH 02/10] renamed and add test for: transport: ensure value of :authority header matches server name used in TLS handshake when the latter is overridden by the name resolver --- internal/transport/http2_client.go | 38 +++++++++------- internal/transport/transport.go | 4 +- internal/transport/transport_test.go | 67 +++++++++++++++++++++++++++- stream.go | 6 --- 4 files changed, 90 insertions(+), 25 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 40147ce892c1..e15024c6acef 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -59,18 +59,18 @@ 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 - usedAddress resolver.Address // Record the used resolver address of client, and replace :authority to resolver address if serverName is not empty. - md metadata.MD - conn net.Conn // underlying communication channel - loopy *loopyWriter - remoteAddr net.Addr - localAddr net.Addr - authInfo credentials.AuthInfo // auth info about the connection + 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 resolver.Address // Record the used resolver address of client, and replace :authority to resolver address if serverName is not empty. + md metadata.MD + conn net.Conn // underlying communication channel + loopy *loopyWriter + remoteAddr net.Addr + localAddr net.Addr + authInfo credentials.AuthInfo // auth info about the connection readerDone chan struct{} // sync point to enable testing. writerDone chan struct{} // sync point to enable testing. @@ -315,7 +315,7 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts cancel: cancel, userAgent: opts.UserAgent, registeredCompressors: grpcutil.RegisteredCompressors(), - usedAddress: addr, // resolver address + address: addr, // resolver address conn: conn, remoteAddr: conn.RemoteAddr(), localAddr: conn.LocalAddr(), @@ -456,9 +456,9 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts return t, nil } -// GetUsedResolverAddress return the transport used resolver address meta info -func (t *http2Client) GetUsedResolverAddress() resolver.Address { - return t.usedAddress +// Address return the resolver address meta info +func (t *http2Client) Address() resolver.Address { + return t.address } func (t *http2Client) newStream(ctx context.Context, callHdr *CallHdr) *Stream { @@ -709,6 +709,12 @@ 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()) + + // replace host with the actual server name, if it is exist and unmatch + if t.address.ServerName != "" && t.address.ServerName != callHdr.Host { + callHdr.Host = t.address.ServerName + } + headerFields, err := t.createHeaderFields(ctx, callHdr) if err != nil { return nil, &NewStreamError{Err: err, AllowTransparentRetry: false} diff --git a/internal/transport/transport.go b/internal/transport/transport.go index bac7c1fd85a0..3cc27bf8217c 100644 --- a/internal/transport/transport.go +++ b/internal/transport/transport.go @@ -657,8 +657,8 @@ type ClientTransport interface { // with a human readable string with debug info. GetGoAwayReason() (GoAwayReason, string) - // GetUsedResolverAddress return the transport used resolver address meta info - GetUsedResolverAddress() resolver.Address + // Address return the transport used resolver address meta info + Address() resolver.Address // RemoteAddr returns the remote network address. RemoteAddr() net.Addr diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index 16bbf8c8ac3f..f10e2723b510 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -35,6 +35,7 @@ import ( "testing" "time" + "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" "github.com/google/go-cmp/cmp" @@ -91,6 +92,7 @@ const ( invalidHeaderField delayRead pingpong + returnHeaderAuthority ) func (h *testStreamHandler) handleStreamAndNotify(s *Stream) { @@ -203,6 +205,31 @@ func (h *testStreamHandler) handleStreamInvalidHeaderField(s *Stream) { }) } +func (h *testStreamHandler) handleStreamReturnValueOfAuthority(t *testing.T, s *Stream) { + var ( + md, exist = metadata.FromIncomingContext(s.ctx) + resp string + ) + + if !exist || len(md.Get(":authority")) == 0 { + h.handleStreamInvalidHeaderField(s) + return + } + + resp = md.Get(":authority")[0] + + req := expectedRequest + p := make([]byte, len(req)) + _, err := s.Read(p) + if err != nil { + return + } + // send a response back to the client. + h.t.Write(s, nil, []byte(resp), &Options{}) + // send the trailer to end the stream. + h.t.WriteStatus(s, status.New(codes.OK, "")) +} + // handleStreamDelayRead delays reads so that the other side has to halt on // stream-level flow control. // This handler assumes dynamic flow control is turned off and assumes window @@ -379,6 +406,12 @@ func (s *server) start(t *testing.T, port int, serverConfig *ServerConfig, ht hT }, func(ctx context.Context, method string) context.Context { return ctx }) + case returnHeaderAuthority: + go transport.HandleStreams(func(s *Stream) { + go h.handleStreamReturnValueOfAuthority(t, s) + }, func(ctx context.Context, method string) context.Context { + return ctx + }) case delayRead: h.notify = make(chan struct{}) h.getNotified = make(chan struct{}) @@ -448,7 +481,7 @@ func setUp(t *testing.T, port int, maxStreams uint32, ht hType) (*server, *http2 func setUpWithOptions(t *testing.T, port int, sc *ServerConfig, ht hType, copts ConnectOptions) (*server, *http2Client, func()) { server := setUpServerOnly(t, port, sc, ht) - addr := resolver.Address{Addr: "localhost:" + server.port} + addr := resolver.Address{Addr: "localhost:" + server.port, ServerName: server.addr()} copts.ChannelzParentID = channelz.NewIdentifierForTesting(channelz.RefSubChannel, time.Now().Unix(), nil) connectCtx, cancel := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second)) @@ -1431,6 +1464,38 @@ func (s) TestEncodingRequiredStatus(t *testing.T) { server.stop() } +func (s) TestHeaderHostReplacedWithResolverAddress(t *testing.T) { + server, ct, cancel := setUp(t, 0, math.MaxUint32, returnHeaderAuthority) + defer cancel() + callHdr := &CallHdr{ + Host: "scheme://testSrv.com/testPath", + Method: "foo", + } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + s, err := ct.NewStream(ctx, callHdr) + if err != nil { + return + } + + opts := Options{Last: true} + if err := ct.Write(s, nil, expectedRequest, &opts); err != nil && err != errStreamDone { + t.Fatalf("Failed to write the request: %v", err) + } + respOfAuthority := make([]byte, http2MaxFrameLen) + len, recvErr := s.Read(respOfAuthority) + if err, ok := status.FromError(recvErr); ok { + t.Fatalf("Read got error %v, headers are unexpected", err) + } + + if string(respOfAuthority[:len]) != server.addr() { + t.Fatalf("Read got a unexpected :authority value %v, want %v", string(respOfAuthority), server.addr()) + } + + ct.Close(fmt.Errorf("closed manually by test")) + server.stop() +} + func (s) TestInvalidHeaderField(t *testing.T) { server, ct, cancel := setUp(t, 0, math.MaxUint32, invalidHeaderField) defer cancel() diff --git a/stream.go b/stream.go index 72e727156f77..fd573f5c5416 100644 --- a/stream.go +++ b/stream.go @@ -456,12 +456,6 @@ func (a *csAttempt) newStream() error { cs := a.cs cs.callHdr.PreviousAttempts = cs.numRetries - // Replace with the actual serverName, if it exist - addr := a.t.GetUsedResolverAddress() - if addr.ServerName != "" { - cs.callHdr.Host = addr.ServerName - } - s, err := a.t.NewStream(a.ctx, cs.callHdr) if err != nil { nse, ok := err.(*transport.NewStreamError) From 95a427b0a89c8f3f8ebb849caacdd913912b8e4a Mon Sep 17 00:00:00 2001 From: holdno Date: Fri, 4 Nov 2022 17:04:34 +0800 Subject: [PATCH 03/10] fix: copy callHdr to resolve race --- internal/transport/http2_client.go | 7 ++++--- stream.go | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index e15024c6acef..9e7e60c73def 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -710,12 +710,13 @@ func (e NewStreamError) Error() string { func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (*Stream, error) { ctx = peer.NewContext(ctx, t.getPeer()) + dupCallHdr := *callHdr // replace host with the actual server name, if it is exist and unmatch - if t.address.ServerName != "" && t.address.ServerName != callHdr.Host { - callHdr.Host = t.address.ServerName + if t.address.ServerName != "" && t.address.ServerName != dupCallHdr.Host { + dupCallHdr.Host = t.address.ServerName } - headerFields, err := t.createHeaderFields(ctx, callHdr) + headerFields, err := t.createHeaderFields(ctx, &dupCallHdr) if err != nil { return nil, &NewStreamError{Err: err, AllowTransparentRetry: false} } diff --git a/stream.go b/stream.go index fd573f5c5416..b10ab1ab6324 100644 --- a/stream.go +++ b/stream.go @@ -455,7 +455,6 @@ func (a *csAttempt) getTransport() error { func (a *csAttempt) newStream() error { cs := a.cs cs.callHdr.PreviousAttempts = cs.numRetries - s, err := a.t.NewStream(a.ctx, cs.callHdr) if err != nil { nse, ok := err.(*transport.NewStreamError) From 4cbfed1aca80289f07d26edcf58c0b86505ba4e1 Mon Sep 17 00:00:00 2001 From: holdno Date: Fri, 11 Nov 2022 15:04:11 +0800 Subject: [PATCH 04/10] remove and modify valueless comments --- internal/transport/http2_client.go | 30 ++++++++++++++++-------------- internal/transport/transport.go | 3 --- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 9e7e60c73def..847ac9a521d1 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -59,12 +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 - address resolver.Address // Record the used resolver address of client, and replace :authority to resolver address if serverName is not empty. + 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 @@ -315,7 +318,7 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts cancel: cancel, userAgent: opts.UserAgent, registeredCompressors: grpcutil.RegisteredCompressors(), - address: addr, // resolver address + address: addr, conn: conn, remoteAddr: conn.RemoteAddr(), localAddr: conn.LocalAddr(), @@ -456,11 +459,6 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts return t, nil } -// Address return the resolver address meta info -func (t *http2Client) Address() resolver.Address { - return t.address -} - func (t *http2Client) newStream(ctx context.Context, callHdr *CallHdr) *Stream { // TODO(zhaoq): Handle uint32 overflow of Stream.id. s := &Stream{ @@ -711,8 +709,12 @@ func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (*Stream, ctx = peer.NewContext(ctx, t.getPeer()) dupCallHdr := *callHdr - // replace host with the actual server name, if it is exist and unmatch - if t.address.ServerName != "" && t.address.ServerName != dupCallHdr.Host { + // 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 } diff --git a/internal/transport/transport.go b/internal/transport/transport.go index 3cc27bf8217c..e21587b5321c 100644 --- a/internal/transport/transport.go +++ b/internal/transport/transport.go @@ -657,9 +657,6 @@ type ClientTransport interface { // with a human readable string with debug info. GetGoAwayReason() (GoAwayReason, string) - // Address return the transport used resolver address meta info - Address() resolver.Address - // RemoteAddr returns the remote network address. RemoteAddr() net.Addr From 6f29d4eae387d9c270cd428ac4d923cfa59bd134 Mon Sep 17 00:00:00 2001 From: holdno Date: Fri, 11 Nov 2022 16:09:11 +0800 Subject: [PATCH 05/10] move resolver authority test to e2e test --- internal/transport/transport_test.go | 65 ---------------------------- test/authority_test.go | 33 ++++++++++++++ 2 files changed, 33 insertions(+), 65 deletions(-) diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index f10e2723b510..eca6c5c31847 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -35,7 +35,6 @@ import ( "testing" "time" - "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" "github.com/google/go-cmp/cmp" @@ -92,7 +91,6 @@ const ( invalidHeaderField delayRead pingpong - returnHeaderAuthority ) func (h *testStreamHandler) handleStreamAndNotify(s *Stream) { @@ -205,31 +203,6 @@ func (h *testStreamHandler) handleStreamInvalidHeaderField(s *Stream) { }) } -func (h *testStreamHandler) handleStreamReturnValueOfAuthority(t *testing.T, s *Stream) { - var ( - md, exist = metadata.FromIncomingContext(s.ctx) - resp string - ) - - if !exist || len(md.Get(":authority")) == 0 { - h.handleStreamInvalidHeaderField(s) - return - } - - resp = md.Get(":authority")[0] - - req := expectedRequest - p := make([]byte, len(req)) - _, err := s.Read(p) - if err != nil { - return - } - // send a response back to the client. - h.t.Write(s, nil, []byte(resp), &Options{}) - // send the trailer to end the stream. - h.t.WriteStatus(s, status.New(codes.OK, "")) -} - // handleStreamDelayRead delays reads so that the other side has to halt on // stream-level flow control. // This handler assumes dynamic flow control is turned off and assumes window @@ -406,12 +379,6 @@ func (s *server) start(t *testing.T, port int, serverConfig *ServerConfig, ht hT }, func(ctx context.Context, method string) context.Context { return ctx }) - case returnHeaderAuthority: - go transport.HandleStreams(func(s *Stream) { - go h.handleStreamReturnValueOfAuthority(t, s) - }, func(ctx context.Context, method string) context.Context { - return ctx - }) case delayRead: h.notify = make(chan struct{}) h.getNotified = make(chan struct{}) @@ -1464,38 +1431,6 @@ func (s) TestEncodingRequiredStatus(t *testing.T) { server.stop() } -func (s) TestHeaderHostReplacedWithResolverAddress(t *testing.T) { - server, ct, cancel := setUp(t, 0, math.MaxUint32, returnHeaderAuthority) - defer cancel() - callHdr := &CallHdr{ - Host: "scheme://testSrv.com/testPath", - Method: "foo", - } - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - s, err := ct.NewStream(ctx, callHdr) - if err != nil { - return - } - - opts := Options{Last: true} - if err := ct.Write(s, nil, expectedRequest, &opts); err != nil && err != errStreamDone { - t.Fatalf("Failed to write the request: %v", err) - } - respOfAuthority := make([]byte, http2MaxFrameLen) - len, recvErr := s.Read(respOfAuthority) - if err, ok := status.FromError(recvErr); ok { - t.Fatalf("Read got error %v, headers are unexpected", err) - } - - if string(respOfAuthority[:len]) != server.addr() { - t.Fatalf("Read got a unexpected :authority value %v, want %v", string(respOfAuthority), server.addr()) - } - - ct.Close(fmt.Errorf("closed manually by test")) - server.stop() -} - func (s) TestInvalidHeaderField(t *testing.T) { server, ct, cancel := setUp(t, 0, math.MaxUint32, invalidHeaderField) defer cancel() diff --git a/test/authority_test.go b/test/authority_test.go index c841c64736fb..ef14b6532551 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -36,6 +36,7 @@ import ( "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/resolver/manual" "google.golang.org/grpc/status" testpb "google.golang.org/grpc/test/grpc_testing" ) @@ -205,3 +206,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 replace the authority +// to the resolver address server name when it is set. +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", + } + 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 { + t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err) + } +} From acdbf62140d023ebad2af6067df9fa53bf5613fa Mon Sep 17 00:00:00 2001 From: holdno Date: Fri, 11 Nov 2022 16:13:30 +0800 Subject: [PATCH 06/10] restore transport/transport_test.go --- internal/transport/transport_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index eca6c5c31847..16bbf8c8ac3f 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -448,7 +448,7 @@ func setUp(t *testing.T, port int, maxStreams uint32, ht hType) (*server, *http2 func setUpWithOptions(t *testing.T, port int, sc *ServerConfig, ht hType, copts ConnectOptions) (*server, *http2Client, func()) { server := setUpServerOnly(t, port, sc, ht) - addr := resolver.Address{Addr: "localhost:" + server.port, ServerName: server.addr()} + addr := resolver.Address{Addr: "localhost:" + server.port} copts.ChannelzParentID = channelz.NewIdentifierForTesting(channelz.RefSubChannel, time.Now().Unix(), nil) connectCtx, cancel := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second)) From 56e5899faf8b2ebe721689285bf30c0791c3a532 Mon Sep 17 00:00:00 2001 From: holdno Date: Fri, 11 Nov 2022 16:18:14 +0800 Subject: [PATCH 07/10] modify test notes --- test/authority_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/authority_test.go b/test/authority_test.go index ef14b6532551..1e7d99af0fb7 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -207,8 +207,8 @@ func (s) TestColonPortAuthority(t *testing.T) { } } -// TestAuthorityReplacedWithResolverAddress This test makes sure that the http2 client replace the authority -// to the resolver address server name when it is set. +// TestAuthorityReplacedWithResolverAddress This test makes sure that the http2 client +// replaces the authority to the resolver address server name when it is set. func (s) TestAuthorityReplacedWithResolverAddress(t *testing.T) { const expectedAuthority = "test.server.name" From 3d99b172ace3e39e24e419a698e32dbdd9dec9d4 Mon Sep 17 00:00:00 2001 From: holdno Date: Mon, 14 Nov 2022 12:19:18 +0800 Subject: [PATCH 08/10] fix: undefined resolver --- test/authority_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/authority_test.go b/test/authority_test.go index 1e7d99af0fb7..ad21c169ae68 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -36,6 +36,7 @@ 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" @@ -207,7 +208,7 @@ func (s) TestColonPortAuthority(t *testing.T) { } } -// TestAuthorityReplacedWithResolverAddress This test makes sure that the http2 client +// TestAuthorityReplacedWithResolverAddress This test makes sure that the http2 client // replaces the authority to the resolver address server name when it is set. func (s) TestAuthorityReplacedWithResolverAddress(t *testing.T) { const expectedAuthority = "test.server.name" From 86b897fc50b724396565c76f968542c7dc108f42 Mon Sep 17 00:00:00 2001 From: holdno Date: Thu, 17 Nov 2022 11:15:30 +0800 Subject: [PATCH 09/10] modify test notes --- test/authority_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/authority_test.go b/test/authority_test.go index ad21c169ae68..ccc99b18dba0 100644 --- a/test/authority_test.go +++ b/test/authority_test.go @@ -208,8 +208,10 @@ func (s) TestColonPortAuthority(t *testing.T) { } } -// TestAuthorityReplacedWithResolverAddress This test makes sure that the http2 client -// replaces the authority to the resolver address server name when it is set. +// TestAuthorityReplacedWithResolverAddress tests the scenario where the resolver +// returned address contains a ServerName override. The test verifies that the the +// :authority header value sent to the server as part of the http/2 HEADERS frame +// is set to the value specified in the resolver returned address. func (s) TestAuthorityReplacedWithResolverAddress(t *testing.T) { const expectedAuthority = "test.server.name" @@ -217,7 +219,6 @@ func (s) TestAuthorityReplacedWithResolverAddress(t *testing.T) { EmptyCallF: func(ctx context.Context, _ *testpb.Empty) (*testpb.Empty, error) { return authorityChecker(ctx, expectedAuthority) }, - Network: "tcp", } if err := ss.Start(nil); err != nil { t.Fatalf("Error starting endpoint server: %v", err) @@ -234,8 +235,7 @@ func (s) TestAuthorityReplacedWithResolverAddress(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() - _, err = testpb.NewTestServiceClient(cc).EmptyCall(ctx, &testpb.Empty{}) - if err != nil { - t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err) + if _, err = testpb.NewTestServiceClient(cc).EmptyCall(ctx, &testpb.Empty{}); err != nil { + t.Fatalf("EmptyCall() rpc failed: %v", err) } } From 223845ff41805b82db6dec11851e128a1480359d Mon Sep 17 00:00:00 2001 From: holdno Date: Fri, 18 Nov 2022 14:21:43 +0800 Subject: [PATCH 10/10] avoid redundant copying --- internal/transport/http2_client.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go index 847ac9a521d1..23d6ec6bc497 100644 --- a/internal/transport/http2_client.go +++ b/internal/transport/http2_client.go @@ -708,17 +708,18 @@ func (e NewStreamError) Error() string { func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (*Stream, error) { ctx = peer.NewContext(ctx, t.getPeer()) - 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 + newCallHdr := *callHdr + newCallHdr.Host = t.address.ServerName + callHdr = &newCallHdr } - headerFields, err := t.createHeaderFields(ctx, &dupCallHdr) + headerFields, err := t.createHeaderFields(ctx, callHdr) if err != nil { return nil, &NewStreamError{Err: err, AllowTransparentRetry: false} }