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.

Sign up for GitHub

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

Jump to bottom

client: handle HTTP header parsing error correctly #2599

Merged

lyuxuan merged 7 commits into grpc:master from lyuxuan:fixhttp2err

Mar 6, 2019

Contributor

lyuxuan commented Jan 28, 2019 •

edited by menghanl

Loading

https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md

lyuxuan requested a review from dfawley

January 28, 2019 23:15

dfawley reviewed

View reviewed changes

Member

dfawley left a comment

One comment from when I started reviewing this yesterday. With the most recent commit, travis is failing - please make sure travis is green and then reassign to me when you are ready. Thanks!

internal/transport/http2_client.go Outdated

    
            @@ -1139,9 +1139,9 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
          
              		return

              	}

              	atomic.StoreUint32(&s.bytesReceived, 1)

              	var state decodeState

              	state := decodeState{isTrailer: atomic.LoadUint32(&s.headerDone) == 1}

Member

dfawley Jan 29, 2019

This (gRPC trailer = second HTTP/2 HEADERS frame received) is not right, or the name isTrailer is misleading if this is how you have intended it to behave. Just below we use the end stream bit in the frame to determine whether this is trailers. That will result in true for trailers-only responses, whereas the check added here would be false.

dfawley assigned lyuxuan

lyuxuan force-pushed the fixhttp2err branch 2 times, most recently from 526bea6 to c0e5a41 Compare

January 30, 2019 01:01

lyuxuan assigned dfawley and unassigned lyuxuan

dfawley reviewed

View reviewed changes

internal/transport/http_util.go Outdated

@@ @@ -120,8 +118,32 @@ type decodeState struct { @@
               	statsTags      []byte
               	statsTrace     []byte
               	contentSubtype string
+              }
+              func newDecodeState(serverSide bool, ignoreContentType bool) *decodeState {

Member

dfawley Feb 11, 2019

I think it is actual preferable to not have a constructor here. Consider:

state := newDecodeState(true, false)
and
state := &decodeState{serverSide: true, ignoreContentType: false}

The latter is more readable. Unless there's state that needs to always be initialized for correctness, a constructor is not necessary.

Contributor Author

lyuxuan Feb 20, 2019

done

internal/transport/http_util.go Outdated

+              	}
+              }
+              // Records the states during HPACK decoding. Must be reset once the

Member

dfawley Feb 11, 2019

This comment applies more for parsedHeaderData than decodeState, doesn't it?

Contributor Author

lyuxuan Feb 20, 2019

moved comments.

internal/transport/http_util.go Outdated

+              }
+              // Records the states during HPACK decoding. Must be reset once the
+              // decoding of the entire headers are finished.

Member

dfawley Feb 11, 2019

Is this "reset" comment correct? Do we ever reset it? I thought we just make new ones every time.

Contributor Author

lyuxuan Feb 20, 2019

Deleted the reset. I don't think reset is done, we just create a new struct every time. This is old comments from before.

internal/transport/http_util.go Outdated

+              	// content-type is grpc or not.
+              	//
+              	// If we've already received a HEADERS frame which indicates gRPC peer, then we can ignore
+              	// content-type for the following HEADERS frame (i.e. Trailers).

Member

dfawley Feb 11, 2019

Trailers (after headers) shouldn't even have a content-type - or if it does, we should always ignore it, no?

Contributor Author

lyuxuan Feb 20, 2019

rephrased. Mentioned Trailers (after headers) shouldn't even have a content-type, and we ignore checking it.

internal/transport/http_util.go Outdated

+              		// we got a valid content-type that starts with "applicatin/grpc", so we are operating in grpc
+              		// mode.
+              		if hf.Name == "content-type" {
+              			isGRPC = true

Member

dfawley Feb 11, 2019

It may be cleaner to put "isGRPC" into parsedHeaderData to avoid setting it here based on knowledge of how processHeaderField works (i.e. no error and field name = "content-type" implies GRPC).

Maybe the above logic can all be moved there too? (The switch and the knowledge of skipping certain fields.)

Contributor Author

lyuxuan Feb 20, 2019

done

internal/transport/http_util.go Outdated Show resolved Hide resolved

internal/transport/http_util.go Outdated Show resolved Hide resolved

internal/transport/http_util.go Outdated Show resolved Hide resolved

test/end2end_test.go Outdated

@@ @@ -7127,3 +7129,241 @@ func (s) TestRPCWaitsForResolver(t *testing.T) { @@
               		t.Fatalf("TestService/UnaryCall(_, _) = _, %v, want _, nil", err)
               	}
               }
+              // A copy from http_util.go for testing.

Member

dfawley Feb 11, 2019

Export from transport instead of copying?

menghanl assigned lyuxuan and unassigned dfawley

lyuxuan force-pushed the fixhttp2err branch 2 times, most recently from a54f089 to 3796c19 Compare

February 20, 2019 00:49

menghanl assigned dfawley and unassigned lyuxuan

dfawley reviewed

View reviewed changes

internal/transport/http_util.go Outdated

               	for _, hf := range frame.Fields {
-              		if hf.Name != "content-type" && hf.Name != ":status" && grpcErr != nil {
+              		if hf.Name != "content-type" && hf.Name != ":status" && d.data.grpcErr != nil {

Member

dfawley Feb 27, 2019

Do we still need this? Is it not safe to parse everything? (If things must be skipped, can we move this logic to processHeaderField?)

If we're just skipping parsing some header fields in the name of efficiency, I don't think extra code complexity is worth optimizing error cases (unless it's a substantial difference).

Contributor Author

lyuxuan Feb 27, 2019

done

internal/transport/http_util.go

               		}
-              		d.httpStatus = &code
+              		d.data.httpStatus = &code

Member

dfawley Feb 27, 2019

Is it possible to eliminate httpStatus from data and set httpErr here instead?

Contributor Author

lyuxuan Feb 27, 2019

It's possible, but requires tricky ways, i.e compare error string is empty or not, and decide the final status error message. Decided not to do that from offline discussion.

lyuxuan added 7 commits

February 27, 2019 14:32


          client: handle HTTP fallback correctly

d0d1fba


          try fix

824a7c0


          fixed

5ccd80f


          fix some offline comments

3b71bc3


          include logic to check more than two HEADERS frames

291add4


          fix reviews

54a6601


          fix reviews

6d948d0

lyuxuan force-pushed the fixhttp2err branch from d551c4c to 6d948d0 Compare

February 27, 2019 22:35

dfawley approved these changes

View reviewed changes

dfawley mentioned this pull request

Prioritize HTTP errors over gRPC errors #1924

Closed

lyuxuan merged commit 79c9bc6 into grpc:master

dfawley added Type: Dependencies Type: Bug and removed Type: Dependencies labels

dfawley added this to the 1.20 Release milestone

menghanl mentioned this pull request

Error message for incorrect endpoint does not provide enough information to debug #2064

Closed

dfawley mentioned this pull request

Google Cloud healthcheck 503 after upgrade to [email protected] #2988

Closed

lock bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels