server: fix a few issues where grpc server uses RST_STREAM for non-HTTP/2 errors #5893

jhump · 2022-12-23T18:12:48Z

In addition to the 415 for an unsupported content-type, this also has the server respond with a 400 in the face of malformed application header values (invalid grpc-timeout, invalid base64-encoded binary header). The NewServerHandlerTransport function (for using the server with existing net/http server) already handled such requests this way. So now they behave consistently.

Fixes #5892

RELEASE NOTES:

server: fix a few issues where grpc server uses RST_STREAM for non-HTTP/2 errors

…l errors

easwars · 2022-12-27T20:02:05Z

Also, do you mind adding tests for the behavior changes.

And, is there a reason the PR is in draft?

github-actions · 2023-01-02T20:45:56Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

jhump · 2023-01-03T15:56:40Z

And, is there a reason the PR is in draft?

@easwars, I was getting tests green and trying to figure out how to write new tests for this. It looks like there isn't much of a test fixture for this part of the code, so new tests will have a bit of setup boiler-plate. (If I'm wrong, please point me at an existing test that does similar setup as I'll need.)

easwars · 2023-01-03T20:27:09Z

grpc-go/internal/transport/transport_test.go

Line 1884 in f2fbb0e

func (s) TestHeadersHTTPStatusGRPCStatus(t *testing.T) {

is one of the tests which verifies the returned status code. Maybe you can add to that?

jhump · 2023-01-03T23:07:37Z

@easwars, thanks for the pointer! I've added new test cases and this PR is no longer a draft.

jhump · 2023-01-11T14:31:34Z

@easwars, will you have time to review this PR this week?

internal/transport/handler_server_test.go

internal/transport/http2_server.go

easwars · 2022-12-27T20:00:43Z

internal/transport/http2_server.go

@@ -445,8 +447,8 @@ func (t *http2Server) operateHeaders(frame *http2.MetaHeadersFrame, handle func(
 		if logger.V(logLevel) {
 			logger.Errorf("transport: %v", errMsg)
 		}
-		t.controlBuf.put(&earlyAbortStream{
-			httpStatus:     400,
+		_ = t.controlBuf.put(&earlyAbortStream{


Why is it required to assign the return value to a blank identifier instead of ignoring it? Here and other places in this PR.

My IDE warns about code that ignores errors, which is what this function returns. This is a common analysis in CI (via tools like errcheck), to prevent mistakes where code forgets to check errors. So I added the _ = to make it explicit that I am ignoring the returned error (which suppresses the yellow indicator in GoLand).

I am happy to undo this if you want.

We generally don't prefer the _ = option. If and where it makes sense, we prefer adding a small comment saying why it is safe to ignore the error. So, if you could do that, that would be great.

internal/transport/http2_server.go

easwars · 2023-01-11T19:54:35Z

internal/transport/transport_test.go

-			t.Fatalf("Client failed to dial: %v", err)
-		}
-		defer mconn.Close()
+		t.Run(test.name, func(t *testing.T) {


Did you change anything at all in this test's logic, or is it just indentation change because of adding a t.Run()?

Just indenting. You can confirm by hiding whitespace changes in the diff:

The only other change was augmenting the error message on line 2061.

easwars · 2023-01-11T19:55:06Z

@easwars, will you have time to review this PR this week?

Apologies for the delay. Holidays and sickness to blame.

jhump · 2023-01-11T21:21:19Z

Apologies for the delay. Holidays and sickness to blame.

@easwars, no problem! I hope you're feeling better. And Happy New Year!

I think I've answered your questions and pushed a change to address one of them.

github-actions · 2023-01-17T22:45:46Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

easwars

I think the changes look fine. Could you please handle the blank identifier assignment please.

easwars · 2023-01-18T00:19:25Z

internal/transport/http2_server.go

@@ -445,8 +447,8 @@ func (t *http2Server) operateHeaders(frame *http2.MetaHeadersFrame, handle func(
 		if logger.V(logLevel) {
 			logger.Errorf("transport: %v", errMsg)
 		}
-		t.controlBuf.put(&earlyAbortStream{
-			httpStatus:     400,
+		_ = t.controlBuf.put(&earlyAbortStream{


We generally don't prefer the _ = option. If and where it makes sense, we prefer adding a small comment saying why it is safe to ignore the error. So, if you could do that, that would be great.

internal/transport/http2_server.go

easwars · 2023-01-18T00:25:20Z

Adding @dfawley for second set of eyes and another ping on #5893 (comment).

dfawley

Thank you for the fix! One comment about condition ordering, otherwise LGTM.

dfawley · 2023-01-18T17:52:48Z

internal/transport/http2_server.go

+		})
+		return nil
+	}
+	if !isGRPC {


It seems to me like this should come first -- why complain about the syntax of some grpc headers if the other side isn't even speaking grpc?

Good point! Fixed.

…r is malformed

dfawley

Thanks!

fix a few issues where grpc server resets stream even for non-protoco…

7ad59b0

…l errors

jhump marked this pull request as draft December 23, 2022 20:48

update tests

ed2a3ea

easwars self-assigned this Dec 27, 2022

easwars added the Type: Bug label Dec 27, 2022

easwars added this to the 1.53 Release milestone Dec 27, 2022

easwars assigned jhump and unassigned easwars Dec 27, 2022

easwars added the Status: Requires Reporter Clarification label Dec 27, 2022

github-actions bot added the stale label Jan 2, 2023

github-actions bot removed the stale label Jan 3, 2023

arvindbr8 assigned easwars and unassigned jhump Jan 3, 2023

arvindbr8 removed the Status: Requires Reporter Clarification label Jan 3, 2023

easwars assigned jhump and unassigned easwars Jan 3, 2023

easwars added the Status: Requires Reporter Clarification label Jan 3, 2023

add new tests to verify changed behavior

4df33fd

jhump marked this pull request as ready for review January 3, 2023 23:06

easwars removed the Status: Requires Reporter Clarification label Jan 4, 2023

easwars assigned easwars and unassigned jhump Jan 4, 2023

easwars requested changes Jan 11, 2023

View reviewed changes

easwars assigned jhump and unassigned easwars Jan 11, 2023

easwars added the Status: Requires Reporter Clarification label Jan 11, 2023

always check http status code in test

cb062be

jhump removed their assignment Jan 11, 2023

github-actions bot added the stale label Jan 17, 2023

zasweq assigned easwars Jan 17, 2023

zasweq removed Status: Requires Reporter Clarification stale labels Jan 17, 2023

easwars approved these changes Jan 18, 2023

View reviewed changes

easwars requested a review from dfawley January 18, 2023 00:24

easwars assigned dfawley Jan 18, 2023

remove '_ = ...' idiom

de49eee

dfawley reviewed Jan 18, 2023

View reviewed changes

dfawley assigned jhump and unassigned dfawley Jan 18, 2023

dfawley added the Status: Requires Reporter Clarification label Jan 18, 2023

fail with 415 if content-type is bad, even if timeout or binary heade…

6e17575

…r is malformed

jhump removed their assignment Jan 18, 2023

dfawley approved these changes Jan 18, 2023

View reviewed changes

dfawley removed the Status: Requires Reporter Clarification label Jan 18, 2023

dfawley changed the title ~~fix a few issues where grpc server resets stream even for non-protocol errors~~ server: fix a few issues where grpc server uses RST_STREAM for non-HTTP/2 errors Jan 18, 2023

dfawley merged commit 9b9b381 into grpc:master Jan 18, 2023
1 check passed

github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: fix a few issues where grpc server uses RST_STREAM for non-HTTP/2 errors #5893

server: fix a few issues where grpc server uses RST_STREAM for non-HTTP/2 errors #5893

jhump commented Dec 23, 2022 •

edited by dfawley

easwars commented Dec 27, 2022

github-actions bot commented Jan 2, 2023

jhump commented Jan 3, 2023 •

edited

easwars commented Jan 3, 2023

jhump commented Jan 3, 2023

jhump commented Jan 11, 2023

easwars Dec 27, 2022

jhump Jan 11, 2023

easwars Jan 18, 2023

easwars Jan 11, 2023

jhump Jan 11, 2023

easwars commented Jan 11, 2023

jhump commented Jan 11, 2023 •

edited

github-actions bot commented Jan 17, 2023

easwars left a comment

easwars Jan 18, 2023

easwars commented Jan 18, 2023

dfawley left a comment

dfawley Jan 18, 2023

jhump Jan 18, 2023

dfawley left a comment

server: fix a few issues where grpc server uses RST_STREAM for non-HTTP/2 errors #5893

server: fix a few issues where grpc server uses RST_STREAM for non-HTTP/2 errors #5893

Conversation

jhump commented Dec 23, 2022 • edited by dfawley

easwars commented Dec 27, 2022

github-actions bot commented Jan 2, 2023

jhump commented Jan 3, 2023 • edited

easwars commented Jan 3, 2023

jhump commented Jan 3, 2023

jhump commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Jan 11, 2023

jhump commented Jan 11, 2023 • edited

github-actions bot commented Jan 17, 2023

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Jan 18, 2023

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

jhump commented Dec 23, 2022 •

edited by dfawley

jhump commented Jan 3, 2023 •

edited

jhump commented Jan 11, 2023 •

edited