From e54a726f0acc519d46ccdb61e78fa0d7622c0503 Mon Sep 17 00:00:00 2001 From: Michal Witkowski Date: Mon, 16 May 2016 18:10:00 +0100 Subject: [PATCH 1/4] make :authority propagate to MD --- transport/http_util.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/transport/http_util.go b/transport/http_util.go index 7a3594ac..309eacfd 100644 --- a/transport/http_util.go +++ b/transport/http_util.go @@ -106,6 +106,17 @@ type decodeState struct { mdata map[string][]string } +// isWhitelistedHttp2Header checks whether hdr belongs to HTTP2 headers +// that should be propagated into metadata visible to users. +func isWhitelistedHttp2Header(hdr string) bool { + switch hdr { + case ":authority": + return true + default: + return false + } +} + // isReservedHeader checks whether hdr belongs to HTTP2 headers // reserved by gRPC protocol. Any other headers are classified as the // user-specified metadata. @@ -162,7 +173,7 @@ func (d *decodeState) processHeaderField(f hpack.HeaderField) { case ":path": d.method = f.Value default: - if !isReservedHeader(f.Name) { + if !isReservedHeader(f.Name) || isWhitelistedHttp2Header(f.Name) { if f.Name == "user-agent" { i := strings.LastIndex(f.Value, " ") if i == -1 { From 1ef2c5293ffcc66fab98414b100fcf9f21f18996 Mon Sep 17 00:00:00 2001 From: Michal Witkowski Date: Tue, 17 May 2016 14:29:48 +0100 Subject: [PATCH 2/4] fix `TestCompressOK` and client reserved HTTP header handling --- server.go | 7 ++++--- test/end2end_test.go | 14 +++++++++++--- transport/handler_server.go | 14 ++++++++++++-- transport/http2_server.go | 8 ++++++++ transport/http_util.go | 22 +++++++++++----------- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/server.go b/server.go index d3a8073d..6e8e09a1 100644 --- a/server.go +++ b/server.go @@ -460,6 +460,10 @@ func (s *Server) processUnaryRPC(t transport.ServerTransport, stream *transport. } }() } + if s.opts.cp != nil { + // NOTE: this needs to be ahead of all handling, https://github.com/grpc/grpc-go/issues/686. + stream.SetSendCompress(s.opts.cp.Type()) + } p := &parser{r: stream} for { pf, req, err := p.recvMsg() @@ -545,9 +549,6 @@ func (s *Server) processUnaryRPC(t transport.ServerTransport, stream *transport. Last: true, Delay: false, } - if s.opts.cp != nil { - stream.SetSendCompress(s.opts.cp.Type()) - } if err := s.sendResponse(t, stream, reply, s.opts.cp, opts); err != nil { switch err := err.(type) { case transport.ConnectionError: diff --git a/test/end2end_test.go b/test/end2end_test.go index edddc7d2..4bf38792 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -130,6 +130,9 @@ func newPayload(t testpb.PayloadType, size int32) (*testpb.Payload, error) { func (s *testServer) UnaryCall(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { md, ok := metadata.FromContext(ctx) if ok { + if _, exists := md[":authority"]; !exists { + return nil, grpc.Errorf(codes.DataLoss, "expected an :authority metadata: %v", md) + } if err := grpc.SendHeader(ctx, md); err != nil { return nil, fmt.Errorf("grpc.SendHeader(%v, %v) = %v, want %v", ctx, md, err, nil) } @@ -167,6 +170,7 @@ func (s *testServer) UnaryCall(ctx context.Context, in *testpb.SimpleRequest) (* if err != nil { return nil, err } + return &testpb.SimpleResponse{ Payload: payload, }, nil @@ -174,8 +178,11 @@ func (s *testServer) UnaryCall(ctx context.Context, in *testpb.SimpleRequest) (* func (s *testServer) StreamingOutputCall(args *testpb.StreamingOutputCallRequest, stream testpb.TestService_StreamingOutputCallServer) error { if md, ok := metadata.FromContext(stream.Context()); ok { - // For testing purpose, returns an error if there is attached metadata. - if len(md) > 0 { + if _, exists := md[":authority"]; !exists { + return grpc.Errorf(codes.DataLoss, "expected an :authority metadata: %v", md) + } + // For testing purpose, returns an error if there is attached metadata except for authority. + if len(md) > 1 { return grpc.Errorf(codes.DataLoss, "got extra metadata") } } @@ -1678,7 +1685,8 @@ func testCompressOK(t *testing.T, e env) { ResponseSize: proto.Int32(respSize), Payload: payload, } - if _, err := tc.UnaryCall(context.Background(), req); err != nil { + ctx := metadata.NewContext(context.Background(), metadata.Pairs("something", "something")) + if _, err := tc.UnaryCall(ctx, req); err != nil { t.Fatalf("TestService/UnaryCall(_, _) = _, %v, want _, ", err) } // Streaming RPC diff --git a/transport/handler_server.go b/transport/handler_server.go index fef541db..b194a7e6 100644 --- a/transport/handler_server.go +++ b/transport/handler_server.go @@ -92,9 +92,12 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request) (ServerTr } var metakv []string + if r.Host != "" { + metakv = append(metakv, ":authority", r.Host) + } for k, vv := range r.Header { k = strings.ToLower(k) - if isReservedHeader(k) { + if isReservedHeader(k) && !isWhitelistedHttp2Header(k){ continue } for _, v := range vv { @@ -108,7 +111,6 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request) (ServerTr } } metakv = append(metakv, k, v) - } } st.headerMD = metadata.Pairs(metakv...) @@ -196,6 +198,10 @@ func (ht *serverHandlerTransport) WriteStatus(s *Stream, statusCode codes.Code, } if md := s.Trailer(); len(md) > 0 { for k, vv := range md { + // Clients don't tolerate reading restricted headers after some non restricted ones were sent. + if isReservedHeader(k) { + continue + } for _, v := range vv { // http2 ResponseWriter mechanism to // send undeclared Trailers after the @@ -249,6 +255,10 @@ func (ht *serverHandlerTransport) WriteHeader(s *Stream, md metadata.MD) error { ht.writeCommonHeaders(s) h := ht.rw.Header() for k, vv := range md { + // Clients don't tolerate reading restricted headers after some non restricted ones were sent. + if isReservedHeader(k) { + continue + } for _, v := range vv { h.Add(k, v) } diff --git a/transport/http2_server.go b/transport/http2_server.go index 21b63116..1c4d5852 100644 --- a/transport/http2_server.go +++ b/transport/http2_server.go @@ -460,6 +460,10 @@ func (t *http2Server) WriteHeader(s *Stream, md metadata.MD) error { t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-encoding", Value: s.sendCompress}) } for k, v := range md { + if isReservedHeader(k) { + // Clients don't tolerate reading restricted headers after some non restricted ones were sent. + continue + } for _, entry := range v { t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry}) } @@ -502,6 +506,10 @@ func (t *http2Server) WriteStatus(s *Stream, statusCode codes.Code, statusDesc s t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-message", Value: statusDesc}) // Attach the trailer metadata. for k, v := range s.trailer { + // Clients don't tolerate reading restricted headers after some non restricted ones were sent. + if isReservedHeader(k) { + continue + } for _, entry := range v { t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry}) } diff --git a/transport/http_util.go b/transport/http_util.go index 309eacfd..d168acce 100644 --- a/transport/http_util.go +++ b/transport/http_util.go @@ -106,17 +106,6 @@ type decodeState struct { mdata map[string][]string } -// isWhitelistedHttp2Header checks whether hdr belongs to HTTP2 headers -// that should be propagated into metadata visible to users. -func isWhitelistedHttp2Header(hdr string) bool { - switch hdr { - case ":authority": - return true - default: - return false - } -} - // isReservedHeader checks whether hdr belongs to HTTP2 headers // reserved by gRPC protocol. Any other headers are classified as the // user-specified metadata. @@ -138,6 +127,17 @@ func isReservedHeader(hdr string) bool { } } +// isWhitelistedHttp2Header checks whether hdr belongs to HTTP2 headers +// that should be propagated into metadata visible to users. +func isWhitelistedHttp2Header(hdr string) bool { + switch hdr { + case ":authority", "Authority": + return true + default: + return false + } +} + func (d *decodeState) setErr(err error) { if d.err == nil { d.err = err From eec6ad361b34c22def5394048b3e4e89d7eba7f9 Mon Sep 17 00:00:00 2001 From: Michal Witkowski Date: Wed, 25 May 2016 11:38:25 +0100 Subject: [PATCH 3/4] authority: address comments from PR --- transport/handler_server.go | 2 +- transport/http2_client.go | 4 ++++ transport/http_util.go | 8 ++++---- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/transport/handler_server.go b/transport/handler_server.go index b194a7e6..7a4ae07b 100644 --- a/transport/handler_server.go +++ b/transport/handler_server.go @@ -97,7 +97,7 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request) (ServerTr } for k, vv := range r.Header { k = strings.ToLower(k) - if isReservedHeader(k) && !isWhitelistedHttp2Header(k){ + if isReservedHeader(k) && !isWhitelistedPseudoHeader(k){ continue } for _, v := range vv { diff --git a/transport/http2_client.go b/transport/http2_client.go index 8082fdc8..08716123 100644 --- a/transport/http2_client.go +++ b/transport/http2_client.go @@ -341,6 +341,10 @@ func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (_ *Strea if md, ok := metadata.FromContext(ctx); ok { hasMD = true for k, v := range md { + // HTTP doesn't allow you to set pseudoheaders after non pseudoheaders were set. + if isReservedHeader(k) { + continue + } for _, entry := range v { t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry}) } diff --git a/transport/http_util.go b/transport/http_util.go index d168acce..a4b1b07d 100644 --- a/transport/http_util.go +++ b/transport/http_util.go @@ -127,11 +127,11 @@ func isReservedHeader(hdr string) bool { } } -// isWhitelistedHttp2Header checks whether hdr belongs to HTTP2 headers +// isWhitelistedPseudoHeader checks whether hdr belongs to HTTP2 pseudoheaders // that should be propagated into metadata visible to users. -func isWhitelistedHttp2Header(hdr string) bool { +func isWhitelistedPseudoHeader(hdr string) bool { switch hdr { - case ":authority", "Authority": + case ":authority": return true default: return false @@ -173,7 +173,7 @@ func (d *decodeState) processHeaderField(f hpack.HeaderField) { case ":path": d.method = f.Value default: - if !isReservedHeader(f.Name) || isWhitelistedHttp2Header(f.Name) { + if !isReservedHeader(f.Name) || isWhitelistedPseudoHeader(f.Name) { if f.Name == "user-agent" { i := strings.LastIndex(f.Value, " ") if i == -1 { From fc590f40e9f51a0f8ec9f57767523af2487a2266 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Wed, 1 Jun 2016 16:40:26 -0700 Subject: [PATCH 4/4] Fix golint errors --- server.go | 2 ++ trace.go | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/server.go b/server.go index d3a8073d..52c1bb72 100644 --- a/server.go +++ b/server.go @@ -115,12 +115,14 @@ func CustomCodec(codec Codec) ServerOption { } } +// RPCCompressor returns a ServerOption that sets a compressor for outbound message. func RPCCompressor(cp Compressor) ServerOption { return func(o *options) { o.cp = cp } } +// RPCDecompressor returns a ServerOption that sets a decompressor for inbound message. func RPCDecompressor(dc Decompressor) ServerOption { return func(o *options) { o.dc = dc diff --git a/trace.go b/trace.go index cde04fbf..f6747e1d 100644 --- a/trace.go +++ b/trace.go @@ -101,9 +101,8 @@ type payload struct { func (p payload) String() string { if p.sent { return fmt.Sprintf("sent: %v", p.msg) - } else { - return fmt.Sprintf("recv: %v", p.msg) } + return fmt.Sprintf("recv: %v", p.msg) } type fmtStringer struct {