Prefer an implicit codec when using the gRPC default

[lrewega]

Some gRPC servers in the wild appear to only accept requests when the Content-Type subtype codec name is empty, implying the default of "proto". For example Google Cloud API endpoints appear to require Content-Type to be exactly application/grpc and disallow the equivalent explicit form application/grpc+proto, rejecting requests using the latter form by responding with a 404 and Content-Type: text/html.

It's unclear if this is a load balancer (mis)configuration issue, or a lack of support for specifying a codec in the server implementation, but the end result is the same.

Seeing as the subtype qualifier is optional for gRPC, let's try to prefer the implicit form when it is equivalent to the selected codec.

Note that this change only impacts gRPC. Elsewhere we continue to prefer being explicit. This change should not impact servers.

[lrewega]

I've opened this PR in hopes of fostering discussion. The workarounds I can see are to overwrite the header either via an interceptor or at the transport level, neither of which feel great. Alternatively we could create a whole new option to represent this behaviour, but that also doesn't feel great.

This change should not impact servers.

Note that this is not currently true as ErrorWriter is affected by this change, which I'm not sure is ideal. If that's problematic, and if there's interest in pursuing this change then we can always special case the client scenario.

edit: nevermind I can't read -- thanks @emcfarlane!

[lrewega]

Example of running buf curl against googleapis with and without this change:

bufbuild/buf @ HEAD:

$ go run ./cmd/buf/ curl --protocol grpc --schema buf.build/einride/googleapis https://pubsub.googleapis.com/google.pubsub.v1.Publisher/GetTopic
{
   "code": "unimplemented",
   "message": "HTTP status 404 Not Found"
}
exit status 96

Verbose output

``` buf: > (#1) POST /google.pubsub.v1.Publisher/GetTopic buf: > (#1) Accept-Encoding: identity buf: > (#1) Content-Type: application/grpc+proto buf: > (#1) Grpc-Accept-Encoding: gzip buf: > (#1) Grpc-Timeout: 119769m buf: > (#1) Te: trailers buf: > (#1) User-Agent: grpc-go-connect/1.13.0 (go1.21.5) buf/1.28.2-dev buf: > (#1) buf: } (#1) [5 bytes data] buf: * (#1) Finished upload buf: < (#1) HTTP/2.0 404 Not Found buf: < (#1) Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000 buf: < (#1) Content-Length: 1596 buf: < (#1) Content-Type: text/html; charset=UTF-8 buf: < (#1) Date: Fri, 15 Dec 2023 22:57:54 GMT buf: < (#1) Server: ESF buf: < (#1) X-Content-Type-Options: nosniff buf: < (#1) X-Frame-Options: SAMEORIGIN buf: < (#1) X-Xss-Protection: 0 buf: < (#1) buf: { (#1) [1596 bytes data] buf: * (#1) Call complete ```

With this patch applied:

$ go mod edit -replace=connectrpc.com/connect=github.com/lrewega/connect-go@v0.0.0-20231215222258-d993997062a3
$ go get ./...
$ go run ./cmd/buf/ curl --protocol grpc --schema buf.build/einride/googleapis https://pubsub.googleapis.com/google.pubsub.v1.Publisher/GetTopic
{
   "code": "permission_denied",
   "message": "The request is missing a valid API key."
}
exit status 56

Verbose output

``` buf: > (#1) POST /google.pubsub.v1.Publisher/GetTopic buf: > (#1) Accept-Encoding: identity buf: > (#1) Content-Type: application/grpc buf: > (#1) Grpc-Accept-Encoding: gzip buf: > (#1) Grpc-Timeout: 119800m buf: > (#1) Te: trailers buf: > (#1) User-Agent: grpc-go-connect/1.14.0-dev (go1.21.5) buf/1.28.2-dev buf: > (#1) buf: } (#1) [5 bytes data] buf: * (#1) Finished upload buf: < (#1) HTTP/2.0 200 OK buf: < (#1) Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000 buf: < (#1) Content-Type: application/grpc buf: < (#1) Date: Fri, 15 Dec 2023 22:58:55 GMT buf: < (#1) buf: < (#1) buf: < (#1) Grpc-Message: The request is missing a valid API key. buf: < (#1) Grpc-Status: 7 buf: * (#1) Call complete ```

[bufdev]

While technically compatible, I'd likely consider this breaking behavior post v1.0. Changing the content-type that is sent in the common case feels like something we shouldn't do at this point, just my two cents. If we wanted to expose this capability, and option seems more in line (if we need one to accomplish the task).

[bufdev]

Overwriting the header doesn't seem to be so bad to me.

[akshayjshah]

This actually feels pretty reasonable to me. I wouldn't think of it as backward-incompatible - the specification clearly indicates that these should be semantically equivalent.

However, I'd prefer to first file an issue with Google. Is there a good place to do that @lrewega?

[lrewega]

Small update: We've reported the issue to Google, so it's on their radar, but I don't have anything beyond that to share here.

Relatedly, it looks like there was an issue reported in 2020 but it appears to have been deleted: github.com/googleapis/googleapis.github.io/issues/27

Luckily there's a Wayback Machine archive of it here: https://web.archive.org/web/20201128224144/github.com/googleapis/googleapis.github.io/issues/27

Otherwise nothing too interesting.

The deleted issue is referenced a comment in another project facing the same problem.

[jhump]

Note that this changed passed the conformance tests in the CI run, because it's still compliant with the gRPC spec. So I'm fine with merging it, without any extra bits like options to opt-in or opt-out of the behavior. Since it's compliant to the spec, I don't think this poses any sort of compatibility issues as far as when/how we can release this change.