Skip to content

Commit a54f089

Browse files
lyuxuanlyuxuan
committed
fix reviews
1 parent c0e5a41 commit a54f089

File tree

4 files changed

+68
-94
lines changed

4 files changed

+68
-94
lines changed

Diff for: ‎internal/transport/http2_client.go

+17-1
Original file line numberDiff line numberDiff line change
@@ -1150,7 +1150,23 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
11501150
return
11511151
}
11521152

1153-
state := newDecodeState(false, !initialHeader)
1153+
state := &decodeState{
1154+
serverSide: false,
1155+
ignoreContentType: !initialHeader,
1156+
}
1157+
// isGRPC indicates whether the peer is speaking gRPC (otherwise HTTP).
1158+
//
1159+
// We are in gRPC mode (peer speaking gRPC) if:
1160+
// * We are client side and have already received a HEADER frame that indicates gRPC peer.
1161+
// * The header contains valid a content-type, i.e. a string starts with "application/grpc"
1162+
// And we should handle error specific to gRPC.
1163+
//
1164+
// Otherwise (i.e. a content-type string starts without "application/grpc", or does not exist), we
1165+
// are in HTTP fallback mode, and should handle error specific to HTTP.
1166+
//
1167+
// d.ignoreContentType is only set on client side after a gRPC ResponseHeader has been received (indicating
1168+
// peer speaking gRPC). Therefore, we can initialized isGRPC to d.ignoreContentType.
1169+
state.data.isGRPC = state.ignoreContentType
11541170
if err := state.decodeHeader(frame); err != nil {
11551171
t.closeStream(s, err, true, http2.ErrCodeProtocol, status.Convert(err), nil, endStream)
11561172
return

Diff for: ‎internal/transport/http2_server.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,10 @@ func newHTTP2Server(conn net.Conn, config *ServerConfig) (_ ServerTransport, err
286286
// operateHeader takes action on the decoded headers.
287287
func (t *http2Server) operateHeaders(frame *http2.MetaHeadersFrame, handle func(*Stream), traceCtx func(context.Context, string) context.Context) (fatal bool) {
288288
streamID := frame.Header().StreamID
289-
state := newDecodeState(true, false)
289+
state := &decodeState{
290+
serverSide: true,
291+
ignoreContentType: false,
292+
}
290293
if err := state.decodeHeader(frame); err != nil {
291294
if se, ok := status.FromError(err); ok {
292295
t.controlBuf.put(&cleanupStream{

Diff for: ‎internal/transport/http_util.go

+42-68
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,8 @@ var (
7878
codes.ResourceExhausted: http2.ErrCodeEnhanceYourCalm,
7979
codes.PermissionDenied: http2.ErrCodeInadequateSecurity,
8080
}
81-
httpStatusConvTab = map[int]codes.Code{
81+
// HTTPStatusConvTab is the HTTP status code to gRPC error code conversion table.
82+
HTTPStatusConvTab = map[int]codes.Code{
8283
// 400 Bad Request - INTERNAL.
8384
http.StatusBadRequest: codes.Internal,
8485
// 401 Unauthorized - UNAUTHENTICATED.
@@ -118,31 +119,28 @@ type parsedHeaderData struct {
118119
statsTags []byte
119120
statsTrace []byte
120121
contentSubtype string
121-
}
122122

123-
func newDecodeState(serverSide bool, ignoreContentType bool) *decodeState {
124-
return &decodeState{
125-
serverSide: serverSide,
126-
ignoreContentType: ignoreContentType,
127-
}
123+
isGRPC bool
124+
grpcErr error
125+
httpErr error
126+
contentTypeErr string
128127
}
129128

130-
// Records the states during HPACK decoding. Must be reset once the
131-
// decoding of the entire headers are finished.
129+
// decodeState configures decoding criteria and records the decoded data.
132130
type decodeState struct {
133131
// whether decoding on server side or not
134132
serverSide bool
135133
// ignoreContentType indicates whether when processing the HEADERS frame, ignoring checking the
136134
// content-type is grpc or not.
137135
//
138-
// If we've already received a HEADERS frame which indicates gRPC peer, then we can ignore
139-
// content-type for the following HEADERS frame (i.e. Trailers).
136+
// Trailers (after headers) should not have a content-type. And thus we will ignore checking the
137+
// content-type.
140138
//
141139
// For server, this field is always false.
142140
ignoreContentType bool
143141

144-
// data struct will be filled with info parsed from HTTP HEADERS frame once decodeHeader function
145-
// has been invoked and returned.
142+
// Records the states during HPACK decoding. It will be filled with info parsed from HTTP HEADERS
143+
// frame once decodeHeader function has been invoked and returned.
146144
data parsedHeaderData
147145
}
148146

@@ -267,49 +265,19 @@ func (d *decodeState) decodeHeader(frame *http2.MetaHeadersFrame) error {
267265
return status.Error(codes.Internal, "peer header list size exceeded limit")
268266
}
269267

270-
// isGRPC indicates whether the peer is speaking gRPC (otherwise HTTP).
271-
//
272-
// We are in gRPC mode (peer speaking gRPC) if:
273-
// * We are client side and have already received a HEADER frame that indicates gRPC peer.
274-
// * The header contains valid a content-type, i.e. a string starts with "application/grpc"
275-
// And we should handle error specific to gRPC.
276-
//
277-
// Otherwise (i.e. a content-type string starts without "application/grpc", or does not exist), we
278-
// are in HTTP fallback mode, and should handle error specific to HTTP.
279-
//
280-
// d.ignoreContentType is only set on client side after a gRPC ResponseHeader has been received (indicating
281-
// peer speaking gRPC). Therefore, we can initialized isGRPC to d.ignoreContentType.
282-
isGRPC := d.ignoreContentType
283-
var grpcErr, httpErr, contentTypeErr error
284268
for _, hf := range frame.Fields {
285-
if hf.Name != "content-type" && hf.Name != ":status" && grpcErr != nil {
269+
if hf.Name != "content-type" && hf.Name != ":status" && d.data.grpcErr != nil {
286270
// if we've already encountered grpc related field parsing error, then we skip processing
287271
// all following grpc related field.
288272
continue
289273
}
290274

291-
if err := d.processHeaderField(hf); err != nil {
292-
switch hf.Name {
293-
case "content-type":
294-
contentTypeErr = err
295-
case ":status":
296-
httpErr = err // In gRPC mode, we don't care about HTTP field parsing error, so we store it separately.
297-
default:
298-
grpcErr = err // store the first encountered gRPC field parsing error.
299-
}
300-
continue
301-
}
302-
303-
// we got a valid content-type that starts with "applicatin/grpc", so we are operating in grpc
304-
// mode.
305-
if hf.Name == "content-type" {
306-
isGRPC = true
307-
}
275+
d.processHeaderField(hf)
308276
}
309277

310-
if isGRPC {
311-
if grpcErr != nil {
312-
return grpcErr
278+
if d.data.isGRPC {
279+
if d.data.grpcErr != nil {
280+
return d.data.grpcErr
313281
}
314282
if d.serverSide {
315283
return nil
@@ -328,8 +296,8 @@ func (d *decodeState) decodeHeader(frame *http2.MetaHeadersFrame) error {
328296
}
329297

330298
// HTTP fallback mode
331-
if httpErr != nil {
332-
return httpErr
299+
if d.data.httpErr != nil {
300+
return d.data.httpErr
333301
}
334302

335303
var (
@@ -338,33 +306,33 @@ func (d *decodeState) decodeHeader(frame *http2.MetaHeadersFrame) error {
338306
)
339307

340308
if d.data.httpStatus != nil {
341-
code, ok = httpStatusConvTab[*(d.data.httpStatus)]
309+
code, ok = HTTPStatusConvTab[*(d.data.httpStatus)]
342310
if !ok {
343311
code = codes.Unknown
344312
}
345313
}
346314

347-
return status.Error(code, d.constructHTTPErrMsg(contentTypeErr))
315+
return status.Error(code, d.constructHTTPErrMsg())
348316
}
349317

350318
// constructErrMsg constructs error message to be returned in HTTP fallback mode.
351319
// Format: HTTP status code and its corresponding message + content-type error message.
352-
func (d *decodeState) constructHTTPErrMsg(contentTypeErr error) string {
320+
func (d *decodeState) constructHTTPErrMsg() string {
353321
var errMsgs []string
354322

355323
if d.data.httpStatus == nil {
356-
errMsgs = append(errMsgs, "malformed header: in HTTP fallback mode, but doesn't contain HTTP status")
324+
errMsgs = append(errMsgs, "malformed header: missing HTTP status")
357325
} else {
358326
errMsgs = append(errMsgs, fmt.Sprintf("%s: HTTP status code %d", http.StatusText(*(d.data.httpStatus)), *d.data.httpStatus))
359327
}
360328

361-
if contentTypeErr == nil {
329+
if d.data.contentTypeErr == "" {
362330
errMsgs = append(errMsgs, "transport: missing content-type field")
363331
} else {
364-
errMsgs = append(errMsgs, status.Convert(contentTypeErr).Message())
332+
errMsgs = append(errMsgs, d.data.contentTypeErr)
365333
}
366334

367-
return strings.Join(errMsgs, "\t")
335+
return strings.Join(errMsgs, "; ")
368336
}
369337

370338
func (d *decodeState) addMetadata(k, v string) {
@@ -374,64 +342,72 @@ func (d *decodeState) addMetadata(k, v string) {
374342
d.data.mdata[k] = append(d.data.mdata[k], v)
375343
}
376344

377-
func (d *decodeState) processHeaderField(f hpack.HeaderField) error {
345+
func (d *decodeState) processHeaderField(f hpack.HeaderField) {
378346
switch f.Name {
379347
case "content-type":
380348
contentSubtype, validContentType := contentSubtype(f.Value)
381349
if !validContentType {
382-
return status.Errorf(codes.Internal, "transport: received the unexpected content-type %q", f.Value)
350+
d.data.contentTypeErr = fmt.Sprintf("transport: received the unexpected content-type %q", f.Value)
351+
return
383352
}
384353
d.data.contentSubtype = contentSubtype
385354
// TODO: do we want to propagate the whole content-type in the metadata,
386355
// or come up with a way to just propagate the content-subtype if it was set?
387356
// ie {"content-type": "application/grpc+proto"} or {"content-subtype": "proto"}
388357
// in the metadata?
389358
d.addMetadata(f.Name, f.Value)
359+
d.data.isGRPC = true
390360
case "grpc-encoding":
391361
d.data.encoding = f.Value
392362
case "grpc-status":
393363
code, err := strconv.Atoi(f.Value)
394364
if err != nil {
395-
return status.Errorf(codes.Internal, "transport: malformed grpc-status: %v", err)
365+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-status: %v", err)
366+
return
396367
}
397368
d.data.rawStatusCode = &code
398369
case "grpc-message":
399370
d.data.rawStatusMsg = decodeGrpcMessage(f.Value)
400371
case "grpc-status-details-bin":
401372
v, err := decodeBinHeader(f.Value)
402373
if err != nil {
403-
return status.Errorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
374+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
375+
return
404376
}
405377
s := &spb.Status{}
406378
if err := proto.Unmarshal(v, s); err != nil {
407-
return status.Errorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
379+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
380+
return
408381
}
409382
d.data.statusGen = status.FromProto(s)
410383
case "grpc-timeout":
411384
d.data.timeoutSet = true
412385
var err error
413386
if d.data.timeout, err = decodeTimeout(f.Value); err != nil {
414-
return status.Errorf(codes.Internal, "transport: malformed time-out: %v", err)
387+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed time-out: %v", err)
415388
}
416389
case ":path":
417390
d.data.method = f.Value
418391
case ":status":
419392
code, err := strconv.Atoi(f.Value)
420393
if err != nil {
421-
return status.Errorf(codes.Internal, "transport: malformed http-status: %v", err)
394+
d.data.httpErr = status.Errorf(codes.Internal, "transport: malformed http-status: %v", err)
395+
return
422396
}
423397
d.data.httpStatus = &code
424398
case "grpc-tags-bin":
425399
v, err := decodeBinHeader(f.Value)
426400
if err != nil {
427-
return status.Errorf(codes.Internal, "transport: malformed grpc-tags-bin: %v", err)
401+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-tags-bin: %v", err)
402+
return
428403
}
429404
d.data.statsTags = v
430405
d.addMetadata(f.Name, string(v))
431406
case "grpc-trace-bin":
432407
v, err := decodeBinHeader(f.Value)
433408
if err != nil {
434-
return status.Errorf(codes.Internal, "transport: malformed grpc-trace-bin: %v", err)
409+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-trace-bin: %v", err)
410+
return
435411
}
436412
d.data.statsTrace = v
437413
d.addMetadata(f.Name, string(v))
@@ -442,11 +418,9 @@ func (d *decodeState) processHeaderField(f hpack.HeaderField) error {
442418
v, err := decodeMetadataHeader(f.Name, f.Value)
443419
if err != nil {
444420
errorf("Failed to decode metadata header (%q, %q): %v", f.Name, f.Value, err)
445-
return nil
446421
}
447422
d.addMetadata(f.Name, v)
448423
}
449-
return nil
450424
}
451425

452426
type timeoutUnit uint8

Diff for: ‎test/end2end_test.go

+5-24
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ import (
6363
"google.golang.org/grpc/internal/grpctest"
6464
"google.golang.org/grpc/internal/leakcheck"
6565
"google.golang.org/grpc/internal/testutils"
66+
"google.golang.org/grpc/internal/transport"
6667
"google.golang.org/grpc/keepalive"
6768
"google.golang.org/grpc/metadata"
6869
"google.golang.org/grpc/peer"
@@ -7130,30 +7131,10 @@ func (s) TestRPCWaitsForResolver(t *testing.T) {
71307131
}
71317132
}
71327133

7133-
// A copy from http_util.go for testing.
7134-
var httpStatusConvTab = map[int]codes.Code{
7135-
// 400 Bad Request - INTERNAL.
7136-
http.StatusBadRequest: codes.Internal,
7137-
// 401 Unauthorized - UNAUTHENTICATED.
7138-
http.StatusUnauthorized: codes.Unauthenticated,
7139-
// 403 Forbidden - PERMISSION_DENIED.
7140-
http.StatusForbidden: codes.PermissionDenied,
7141-
// 404 Not Found - UNIMPLEMENTED.
7142-
http.StatusNotFound: codes.Unimplemented,
7143-
// 429 Too Many Requests - UNAVAILABLE.
7144-
http.StatusTooManyRequests: codes.Unavailable,
7145-
// 502 Bad Gateway - UNAVAILABLE.
7146-
http.StatusBadGateway: codes.Unavailable,
7147-
// 503 Service Unavailable - UNAVAILABLE.
7148-
http.StatusServiceUnavailable: codes.Unavailable,
7149-
// 504 Gateway timeout - UNAVAILABLE.
7150-
http.StatusGatewayTimeout: codes.Unavailable,
7151-
}
7152-
71537134
func (s) TestHTTPHeaderFrameErrorHandlingHTTPMode(t *testing.T) {
71547135
// Non-gRPC content-type fallback path.
7155-
for httpCode := range httpStatusConvTab {
7156-
doHTTPHeaderTest(t, httpStatusConvTab[int(httpCode)], []string{
7136+
for httpCode := range transport.HTTPStatusConvTab {
7137+
doHTTPHeaderTest(t, transport.HTTPStatusConvTab[int(httpCode)], []string{
71577138
":status", fmt.Sprintf("%d", httpCode),
71587139
"content-type", "text/html", // non-gRPC content type to switch to HTTP mode.
71597140
"grpc-status", "1", // Make up a gRPC status error
@@ -7162,8 +7143,8 @@ func (s) TestHTTPHeaderFrameErrorHandlingHTTPMode(t *testing.T) {
71627143
}
71637144

71647145
// Missing content-type fallback path.
7165-
for httpCode := range httpStatusConvTab {
7166-
doHTTPHeaderTest(t, httpStatusConvTab[int(httpCode)], []string{
7146+
for httpCode := range transport.HTTPStatusConvTab {
7147+
doHTTPHeaderTest(t, transport.HTTPStatusConvTab[int(httpCode)], []string{
71677148
":status", fmt.Sprintf("%d", httpCode),
71687149
// Omitting content type to switch to HTTP mode.
71697150
"grpc-status", "1", // Make up a gRPC status error

0 commit comments

Comments
 (0)