Prefer an implicit codec when using the gRPC default #655
Conversation
|
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.
edit: nevermind I can't read -- thanks @emcfarlane! |
|
Example of running bufbuild/buf @ HEAD: Verbose outputWith this patch applied: Verbose output |
|
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). |
|
Overwriting the header doesn't seem to be so bad to me. |
|
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? |
|
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. |
|
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. |
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.
akshayjshah
force-pushed
the
lrewega/grpc-prefer-implicit-codec-
branch
from
d993997 to
95e6796
Compare
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/grpcand disallow the equivalent explicit formapplication/grpc+proto, rejecting requests using the latter form by responding with a 404 andContent-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.