Skip to content

Commit 3796c19

Browse files
lyuxuanlyuxuan
committed
fix reviews
1 parent c0e5a41 commit 3796c19

File tree

4 files changed

+68
-94
lines changed

4 files changed

+68
-94
lines changed

Diff for: ‎internal/transport/http2_client.go

+7-1
Original file line numberDiff line numberDiff line change
@@ -1150,7 +1150,13 @@ 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+
// Initialize isGRPC value to be !initialHeader, since if a gRPC ResponseHeader has been received
1158+
// which indicates peer speaking gRPC, we are in gRPC mode.
1159+
state.data.isGRPC = !initialHeader
11541160
if err := state.decodeHeader(frame); err != nil {
11551161
t.closeStream(s, err, true, http2.ErrCodeProtocol, status.Convert(err), nil, endStream)
11561162
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

+52-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,37 @@ 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 field indicates whether the peer is speaking gRPC (otherwise HTTP).
124+
//
125+
// We are in gRPC mode (peer speaking gRPC) if:
126+
// * We are client side and have already received a HEADER frame that indicates gRPC peer.
127+
// * The header contains valid a content-type, i.e. a string starts with "application/grpc"
128+
// And we should handle error specific to gRPC.
129+
//
130+
// Otherwise (i.e. a content-type string starts without "application/grpc", or does not exist), we
131+
// are in HTTP fallback mode, and should handle error specific to HTTP.
132+
isGRPC bool
133+
grpcErr error
134+
httpErr error
135+
contentTypeErr string
128136
}
129137

130-
// Records the states during HPACK decoding. Must be reset once the
131-
// decoding of the entire headers are finished.
138+
// decodeState configures decoding criteria and records the decoded data.
132139
type decodeState struct {
133140
// whether decoding on server side or not
134141
serverSide bool
135142
// ignoreContentType indicates whether when processing the HEADERS frame, ignoring checking the
136143
// content-type is grpc or not.
137144
//
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).
145+
// Trailers (after headers) should not have a content-type. And thus we will ignore checking the
146+
// content-type.
140147
//
141148
// For server, this field is always false.
142149
ignoreContentType bool
143150

144-
// data struct will be filled with info parsed from HTTP HEADERS frame once decodeHeader function
145-
// has been invoked and returned.
151+
// Records the states during HPACK decoding. It will be filled with info parsed from HTTP HEADERS
152+
// frame once decodeHeader function has been invoked and returned.
146153
data parsedHeaderData
147154
}
148155

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

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
284277
for _, hf := range frame.Fields {
285-
if hf.Name != "content-type" && hf.Name != ":status" && grpcErr != nil {
278+
if hf.Name != "content-type" && hf.Name != ":status" && d.data.grpcErr != nil {
286279
// if we've already encountered grpc related field parsing error, then we skip processing
287280
// all following grpc related field.
288281
continue
289282
}
290283

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-
}
284+
d.processHeaderField(hf)
308285
}
309286

310-
if isGRPC {
311-
if grpcErr != nil {
312-
return grpcErr
287+
if d.data.isGRPC {
288+
if d.data.grpcErr != nil {
289+
return d.data.grpcErr
313290
}
314291
if d.serverSide {
315292
return nil
@@ -328,8 +305,8 @@ func (d *decodeState) decodeHeader(frame *http2.MetaHeadersFrame) error {
328305
}
329306

330307
// HTTP fallback mode
331-
if httpErr != nil {
332-
return httpErr
308+
if d.data.httpErr != nil {
309+
return d.data.httpErr
333310
}
334311

335312
var (
@@ -338,33 +315,33 @@ func (d *decodeState) decodeHeader(frame *http2.MetaHeadersFrame) error {
338315
)
339316

340317
if d.data.httpStatus != nil {
341-
code, ok = httpStatusConvTab[*(d.data.httpStatus)]
318+
code, ok = HTTPStatusConvTab[*(d.data.httpStatus)]
342319
if !ok {
343320
code = codes.Unknown
344321
}
345322
}
346323

347-
return status.Error(code, d.constructHTTPErrMsg(contentTypeErr))
324+
return status.Error(code, d.constructHTTPErrMsg())
348325
}
349326

350327
// constructErrMsg constructs error message to be returned in HTTP fallback mode.
351328
// Format: HTTP status code and its corresponding message + content-type error message.
352-
func (d *decodeState) constructHTTPErrMsg(contentTypeErr error) string {
329+
func (d *decodeState) constructHTTPErrMsg() string {
353330
var errMsgs []string
354331

355332
if d.data.httpStatus == nil {
356-
errMsgs = append(errMsgs, "malformed header: in HTTP fallback mode, but doesn't contain HTTP status")
333+
errMsgs = append(errMsgs, "malformed header: missing HTTP status")
357334
} else {
358335
errMsgs = append(errMsgs, fmt.Sprintf("%s: HTTP status code %d", http.StatusText(*(d.data.httpStatus)), *d.data.httpStatus))
359336
}
360337

361-
if contentTypeErr == nil {
338+
if d.data.contentTypeErr == "" {
362339
errMsgs = append(errMsgs, "transport: missing content-type field")
363340
} else {
364-
errMsgs = append(errMsgs, status.Convert(contentTypeErr).Message())
341+
errMsgs = append(errMsgs, d.data.contentTypeErr)
365342
}
366343

367-
return strings.Join(errMsgs, "\t")
344+
return strings.Join(errMsgs, "; ")
368345
}
369346

370347
func (d *decodeState) addMetadata(k, v string) {
@@ -374,64 +351,72 @@ func (d *decodeState) addMetadata(k, v string) {
374351
d.data.mdata[k] = append(d.data.mdata[k], v)
375352
}
376353

377-
func (d *decodeState) processHeaderField(f hpack.HeaderField) error {
354+
func (d *decodeState) processHeaderField(f hpack.HeaderField) {
378355
switch f.Name {
379356
case "content-type":
380357
contentSubtype, validContentType := contentSubtype(f.Value)
381358
if !validContentType {
382-
return status.Errorf(codes.Internal, "transport: received the unexpected content-type %q", f.Value)
359+
d.data.contentTypeErr = fmt.Sprintf("transport: received the unexpected content-type %q", f.Value)
360+
return
383361
}
384362
d.data.contentSubtype = contentSubtype
385363
// TODO: do we want to propagate the whole content-type in the metadata,
386364
// or come up with a way to just propagate the content-subtype if it was set?
387365
// ie {"content-type": "application/grpc+proto"} or {"content-subtype": "proto"}
388366
// in the metadata?
389367
d.addMetadata(f.Name, f.Value)
368+
d.data.isGRPC = true
390369
case "grpc-encoding":
391370
d.data.encoding = f.Value
392371
case "grpc-status":
393372
code, err := strconv.Atoi(f.Value)
394373
if err != nil {
395-
return status.Errorf(codes.Internal, "transport: malformed grpc-status: %v", err)
374+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-status: %v", err)
375+
return
396376
}
397377
d.data.rawStatusCode = &code
398378
case "grpc-message":
399379
d.data.rawStatusMsg = decodeGrpcMessage(f.Value)
400380
case "grpc-status-details-bin":
401381
v, err := decodeBinHeader(f.Value)
402382
if err != nil {
403-
return status.Errorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
383+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
384+
return
404385
}
405386
s := &spb.Status{}
406387
if err := proto.Unmarshal(v, s); err != nil {
407-
return status.Errorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
388+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
389+
return
408390
}
409391
d.data.statusGen = status.FromProto(s)
410392
case "grpc-timeout":
411393
d.data.timeoutSet = true
412394
var err error
413395
if d.data.timeout, err = decodeTimeout(f.Value); err != nil {
414-
return status.Errorf(codes.Internal, "transport: malformed time-out: %v", err)
396+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed time-out: %v", err)
415397
}
416398
case ":path":
417399
d.data.method = f.Value
418400
case ":status":
419401
code, err := strconv.Atoi(f.Value)
420402
if err != nil {
421-
return status.Errorf(codes.Internal, "transport: malformed http-status: %v", err)
403+
d.data.httpErr = status.Errorf(codes.Internal, "transport: malformed http-status: %v", err)
404+
return
422405
}
423406
d.data.httpStatus = &code
424407
case "grpc-tags-bin":
425408
v, err := decodeBinHeader(f.Value)
426409
if err != nil {
427-
return status.Errorf(codes.Internal, "transport: malformed grpc-tags-bin: %v", err)
410+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-tags-bin: %v", err)
411+
return
428412
}
429413
d.data.statsTags = v
430414
d.addMetadata(f.Name, string(v))
431415
case "grpc-trace-bin":
432416
v, err := decodeBinHeader(f.Value)
433417
if err != nil {
434-
return status.Errorf(codes.Internal, "transport: malformed grpc-trace-bin: %v", err)
418+
d.data.grpcErr = status.Errorf(codes.Internal, "transport: malformed grpc-trace-bin: %v", err)
419+
return
435420
}
436421
d.data.statsTrace = v
437422
d.addMetadata(f.Name, string(v))
@@ -442,11 +427,10 @@ func (d *decodeState) processHeaderField(f hpack.HeaderField) error {
442427
v, err := decodeMetadataHeader(f.Name, f.Value)
443428
if err != nil {
444429
errorf("Failed to decode metadata header (%q, %q): %v", f.Name, f.Value, err)
445-
return nil
430+
return
446431
}
447432
d.addMetadata(f.Name, v)
448433
}
449-
return nil
450434
}
451435

452436
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)