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

make :authority propagate to MD #683

Merged
merged 3 commits into from
Jun 2, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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:
Expand Down
14 changes: 11 additions & 3 deletions test/end2end_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -167,15 +170,19 @@ func (s *testServer) UnaryCall(ctx context.Context, in *testpb.SimpleRequest) (*
if err != nil {
return nil, err
}

return &testpb.SimpleResponse{
Payload: payload,
}, nil
}

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")
}
}
Expand Down Expand Up @@ -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 _, <nil>", err)
}
// Streaming RPC
Expand Down
14 changes: 12 additions & 2 deletions transport/handler_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -108,7 +111,6 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request) (ServerTr
}
}
metakv = append(metakv, k, v)

}
}
st.headerMD = metadata.Pairs(metakv...)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down
8 changes: 8 additions & 0 deletions transport/http2_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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})
}
Expand Down Expand Up @@ -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})
}
Expand Down
13 changes: 12 additions & 1 deletion transport/http_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/isWhitelistedHttp2Header/isWhitelistedPseudoHeader/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

switch hdr {
case ":authority", "Authority":
Copy link
Contributor

Choose a reason for hiding this comment

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

How comes "Authority"? There is no such thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I thought this would be needed for http2 Handler implementation, but that required additional code anyway.

return true
default:
return false
}
}

func (d *decodeState) setErr(err error) {
if d.err == nil {
d.err = err
Expand Down Expand Up @@ -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) {
Copy link
Contributor

@tamird tamird May 16, 2016

Choose a reason for hiding this comment

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

@isn't !isReservedHeader(":authority") already true?

EDIT: nope, disregard

if f.Name == "user-agent" {
i := strings.LastIndex(f.Value, " ")
if i == -1 {
Expand Down