ignoring http status when content type is application/grpc #8486

Fixes #8486

When a gRPC response is received with content type application/grpc,
we then do not expect any information in the http status and the status information
needs to be conveyed by gRPC status only.

In case of missing gRPC status, we will throw an Internal error instead of Unknown
in accordance with https://grpc.io/docs/guides/status-codes/

	Changes :
	- Ignore http status in case of content type application/grpc
	- Change the default rawStatusCode to return Internal for missing grpc status

RELEASE NOTES:
* client : Ignore http status for gRPC mode and return Internal error for
missing grpc status

Author: mbissa

internal/transport/http2_client.go

@@ -1467,15 +1467,14 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
 		recvCompress   string
 		httpStatusCode *int
 		httpStatusErr  string
-		rawStatusCode  = codes.Unknown
+		rawStatusCode  = codes.Internal
 		// headerError is set if an error is encountered while parsing the headers
 		headerError string
 	)
 
 	if initialHeader {
 		httpStatusErr = "malformed header: missing HTTP status"
 	}
-
 	for _, hf := range frame.Fields {
 		switch hf.Name {
 		case "content-type":
@@ -1534,7 +1533,8 @@ func (t *http2Client) operateHeaders(frame *http2.MetaHeadersFrame) {
 		}
 	}
 
-	if !isGRPC || httpStatusErr != "" {
+	//if response is not grpc or endstream doesn't receive a grpc status, fall back to http error code
+	if !isGRPC {
 		var code = codes.Internal // when header does not include HTTP status, return INTERNAL
 
 		if httpStatusCode != nil {

internal/transport/transport_test.go

@@ -2618,6 +2618,8 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
 		metaHeaderFrame *http2.MetaHeadersFrame
 		// output
 		wantStatus *status.Status
+		// end stream output
+		wantStatusEndStream *status.Status
 	}{
 		{
 			name: "valid header",
@@ -2695,34 +2697,55 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
 			),
 		},
 		{
-			name: "bad status in grpc mode",
+			name: "ignoring bad http status in grpc mode",
 			metaHeaderFrame: &http2.MetaHeadersFrame{
 				Fields: []hpack.HeaderField{
 					{Name: "content-type", Value: "application/grpc"},
 					{Name: "grpc-status", Value: "0"},
 					{Name: ":status", Value: "504"},
 				},
 			},
-			wantStatus: status.New(
-				codes.Unavailable,
-				"unexpected HTTP status code received from server: 504 (Gateway Timeout)",
-			),
+			wantStatus: status.New(codes.OK, ""),
 		},
 		{
-			name: "missing http status",
+			name: "missing http status and grpc status",
 			metaHeaderFrame: &http2.MetaHeadersFrame{
 				Fields: []hpack.HeaderField{
 					{Name: "content-type", Value: "application/grpc"},
 				},
 			},
-			wantStatus: status.New(
-				codes.Internal,
-				"malformed header: missing HTTP status",
-			),
+			wantStatus:          status.New(codes.OK, ""),
+			wantStatusEndStream: status.New(codes.Internal, ""),
+		},
+		{
+			name: "ignore http status and fail for grpc status missing in trailer",
+			metaHeaderFrame: &http2.MetaHeadersFrame{
+				Fields: []hpack.HeaderField{
+					{Name: "content-type", Value: "application/grpc"},
+					{Name: ":status", Value: "504"},
+				},
+			},
+			wantStatus:          status.New(codes.OK, ""),
+			wantStatusEndStream: status.New(codes.Internal, ""),
+		},
+		{
+			name: "trailer only grpc timeout ignores http status",
+			metaHeaderFrame: &http2.MetaHeadersFrame{
+				Fields: []hpack.HeaderField{
+					{Name: "content-type", Value: "application/grpc"},
+					{Name: "grpc-status", Value: "4"},
+					{Name: "grpc-message", Value: "Request timed out: Internal error"},
+					{Name: ":status", Value: "200"},
+				},
+			},
+			wantStatusEndStream: status.New(codes.DeadlineExceeded, "Request timed out: Internal error"),
 		},
 	} {
 
 		t.Run(test.name, func(t *testing.T) {
+			if test.wantStatus == nil {
+				t.Skip()
+			}
 			ts := testStream()
 			s := testClient(ts)
 
@@ -2733,7 +2756,6 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
 			}
 
 			s.operateHeaders(test.metaHeaderFrame)
-
 			got := ts.status
 			want := test.wantStatus
 			if got.Code() != want.Code() || got.Message() != want.Message() {
@@ -2755,6 +2777,9 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
 
 			got := ts.status
 			want := test.wantStatus
+			if test.wantStatusEndStream != nil {
+				want = test.wantStatusEndStream
+			}
 			if got.Code() != want.Code() || got.Message() != want.Message() {
 				t.Fatalf("operateHeaders(%v); status = \ngot: %s\nwant: %s", test.metaHeaderFrame, got, want)
 			}

test/http_header_end2end_test.go

@@ -103,10 +103,9 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) {
 		errCode codes.Code
 	}{
 		{
-			name: "missing gRPC status",
+			name: "missing gRPC content type",
 			header: []string{
 				":status", "403",
-				"content-type", "application/grpc",
 			},
 			errCode: codes.PermissionDenied,
 		},
@@ -120,23 +119,23 @@ func (s) TestHTTPHeaderFrameErrorHandlingInitialHeader(t *testing.T) {
 			errCode: codes.Internal,
 		},
 		{
-			name: "Malformed grpc-tags-bin field",
+			name: "Malformed grpc-tags-bin field ignores http status",
 			header: []string{
 				":status", "502",
 				"content-type", "application/grpc",
 				"grpc-status", "0",
 				"grpc-tags-bin", "???",
 			},
-			errCode: codes.Unavailable,
+			errCode: codes.Internal,
 		},
 		{
-			name: "gRPC status error",
+			name: "gRPC status error ignoring http status",
 			header: []string{
 				":status", "502",
 				"content-type", "application/grpc",
 				"grpc-status", "3",
 			},
-			errCode: codes.Unavailable,
+			errCode: codes.InvalidArgument,
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
@@ -161,7 +160,7 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) {
 		errCode        codes.Code
 	}{
 		{
-			name: "trailer missing grpc-status",
+			name: "trailer missing grpc-status to ignore http status",
 			responseHeader: []string{
 				":status", "200",
 				"content-type", "application/grpc",
@@ -170,10 +169,10 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) {
 				// trailer missing grpc-status
 				":status", "502",
 			},
-			errCode: codes.Unavailable,
+			errCode: codes.Internal,
 		},
 		{
-			name: "malformed grpc-status-details-bin field with status 404",
+			name: "malformed grpc-status-details-bin field with status 404 to be ignored due to content type",
 			responseHeader: []string{
 				":status", "404",
 				"content-type", "application/grpc",
@@ -183,20 +182,19 @@ func (s) TestHTTPHeaderFrameErrorHandlingNormalTrailer(t *testing.T) {
 				"grpc-status", "0",
 				"grpc-status-details-bin", "????",
 			},
-			errCode: codes.Unimplemented,
+			errCode: codes.Internal,
 		},
 		{
-			name: "malformed grpc-status-details-bin field with status 200",
+			name: "malformed grpc-status-details-bin field with status 404 and no content type",
 			responseHeader: []string{
-				":status", "200",
-				"content-type", "application/grpc",
+				":status", "404",
 			},
 			trailer: []string{
 				// malformed grpc-status-details-bin field
 				"grpc-status", "0",
 				"grpc-status-details-bin", "????",
 			},
-			errCode: codes.Internal,
+			errCode: codes.Unimplemented,
 		},
 	}
 	for _, test := range tests {