Forwarding Connection header breaks after updating grpc/grpc-go to 1.42.0

[MakDon]

🐛 Bug Report

grpc-gateway breaks after updating grpc/grpc-go to 1.42.0

After the update introduced by this PR, it will report stream terminated by RST_STREAM with error code: PROTOCOL_ERROR if connection in headers, which breaks down all of the http connections that comes with connect:keep-alive

To Reproduce

Step 1:
Update google.golang.org/grpc to v1.42.0 in go mod

Step 2：
invoke http request with header connect:keep-alive

Expected behavior

grpc-gateway works well or reports warning, instead of returning PROTOCOL_ERROR

Actual Behavior

it returns stream terminated by RST_STREAM with error code: PROTOCOL_ERROR

Your Environment

go.mod:
google.golang.org/grpc v1.42.0, github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.0

[johanbrandhorst]

Hm, this is curious, what should the behavior be here? Should the grpc-gateway strip these headers? I'm surprised we're forwarding this header, it should only forward explicitly added headers or permanent IANA headers (listed here:

grpc-gateway/runtime/context.go

Line 261 in 094a6fe

func isPermanentHTTPHeader(hdr string) bool {

). Would you be willing to submit a test and a fix for this?

[MakDon]

Hm, this is curious, what should the behavior be here? Should the grpc-gateway strip these headers? I'm surprised we're forwarding this header, it should only forward explicitly added headers or permanent IANA headers (listed here:

grpc-gateway/runtime/context.go

Line 261 in 094a6fe

func isPermanentHTTPHeader(hdr string) bool {

). Would you be willing to submit a test and a fix for this?

ok, I'd like to submit a PR for it later.

[MakDon]

Hm, this is curious, what should the behavior be here? Should the grpc-gateway strip these headers? I'm surprised we're forwarding this header, it should only forward explicitly added headers or permanent IANA headers (listed here:

grpc-gateway/runtime/context.go

Line 261 in 094a6fe

func isPermanentHTTPHeader(hdr string) bool {

). Would you be willing to submit a test and a fix for this?

I found that my server breaks because i was using func(key string) (string, bool) {return key, true} header matcher for backward compatibility, which passthrough Connect header to grpc server and deserves a RST_STREAM

Connection header does not belong to permanent request headers maintained by IANA, so it seems not to be a good idea to add it into isPermanentHTTPHeader function body. And it would not breaks servers that uses DefaultHeaderMatcher cause it is not begins with Grpc-Metadata-

But for a further concern, is it a good idea that provides a backward-Compatible header matcher, which drops "malformed" headers for grpc and returns others headers as-is? It can be implemented like this:

func PassThroughHeaderMatcher(key string) (string, bool) {
	key = textproto.CanonicalMIMEHeaderKey(key)
	if isMalformedHTTPHeader(key) {
		return MetadataPrefix + key, true
	} else {
		return key, true
	}
}

Thanks very much.

[johanbrandhorst]

That is an interesting question. It would be nice to automatically help users who might suffer from this problem, but I'm also hesitant to do this implicitly. Maybe we could detect if the header matcher would match Connection on startup and log a warning?

[MakDon]

That is an interesting question. It would be nice to automatically help users who might suffer from this problem, but I'm also hesitant to do this implicitly. Maybe we could detect if the header matcher would match Connection on startup and log a warning?

Agree. I tend to add a pre-matcher to match Connection and log a warning. Is it probable sulution?

[johanbrandhorst]

I think we can just try running any matcher with the Connection header when creating it in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/mux.go#L115-L119 and log a warning if it matches. Would you be willing to contribute such a fix?

[MakDon]

Ok i am working on it.

[johanbrandhorst]

Fixed in #2455