Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow override for Content-Type header in order to support encoder selection based on Content Sub-Type #10959

Closed
sudo-nan0-RaySK opened this issue Feb 26, 2024 · 3 comments
Assignees

Comments

@sudo-nan0-RaySK
Copy link

sudo-nan0-RaySK commented Feb 26, 2024

Is your feature request related to a problem?

grpc-java with Netty transport currently considers Content-Type header as a reserved header and disregards the value of Content-Type provided by user (Can find a similar issue with User-Agent here at #5874).

This is problematic in cases where content sub-type based encoder selection is required at server side. (Check #7321)

Describe the solution you'd like

grpc-netty must not consider Content-Type header as a reserved header since a core gRPC functionality is not working because override of Content-Type from currently forced application/grpc to application/grpc+<content sub-type> is not possible.

This would require removal of:

headers.discardAll(CONTENT_TYPE_KEY);
and
headers.discardAll(CONTENT_TYPE_KEY);

Further, additional validations can be added to check if value of Content-Type is valid according to gRPC protocol specification. (Basically checking if value of Content-Type starts with application/grpc).

Describe alternatives you've considered

An alternative would be to deprecate registering global encoders in gRPC implementations for various languages (like Go) in favour of registering Marshallers like grpc-java's MethodDescriptor.Marshaller<T> which is not currently supported in gRPC implementations for these languages. This would be rather tedious to incorporate.

Additional context

This issue with not being able to set a custom override for Content-Type header is posing a problem for us as we need to communicate with a Go gRPC application using JSON content sub-type. The way encoding works on go-grpc side is that it chooses one of the registered encoders based on content subtype from Content-Type header, so application/grpc+json will use the registered encoder with name json. However since Netty transport library is overriding it with application/grpc (considering it a reserved header), go-grpc is falling back on using the default encoder with content sub-type name of proto.

Further, the Go server needs to support both protobuf and json based encoding.

bsideup added a commit to bsideup/grpc-java that referenced this issue Mar 16, 2024
@bsideup
Copy link

bsideup commented Mar 16, 2024

Confirming the need for it, see #11023

@ejona86
Copy link
Member

ejona86 commented Mar 22, 2024

You are requesting full control over Content-Type, but then say gRPC can require values be prefixed with application/grpc. So you are simply suggesting an API to provide sub-type support. I don't think we want an API looking like that, though. I think we'd want something plumbed through Marshaller like I mentioned before.

Further, the Go server needs to support both protobuf and json based encoding.

If the Go server supports both, why are you wanting to use JSON? (In other words, why can't you use protobuf?)


The sub-type format is very broken. It is being used for the opposite of what the standard says it means and its name is very confusing. "subtype" in Content-Type is the part after the "/", so for "application/grpc" "grpc" would be the subtype and for "text/plain" "plain" would be the subtype. Clearly "subtype" in the gRPC wire spec is not that.

The + syntax is described in RFC 6839. If something is marked +xml, then that means an XML parser can process it without knowing the rest of the type. Same with +json. It describes the outer structure of the document, not something nested within. application/grpc+json is thus completely broken, because a JSON parser can't parse grpc's 5-byte framing header.

Yes, grpc-go is using it. And yes, it is in the grpc protocol spec. But it is a bad idea to propagate it further than Go.

I think the easiest solution here is a separate header that holds the message content type. Something like "grpc-message-content-type".

(This could potentially also replace "grpc-message-type", which to my knowledge is unused in practice, as content-type could specify the message schema. For protobuf maybe application/x-protobuf; messageType="google.protobuf.Empty", and there's been similar proposals for json; this is non-standard though and I don't know how bad the parsing would be.)

(Another option would be to use parameters in grpc itself, like application/grpc; msg=application/x-protobuf. This might have compatibility issues, as some implementations might be only ignoring content after the +; we'd need to do a survey. And I don't know how much parsing would be necessary there, like if we need to wrap values in double-quotes and such.)

I have spoken to grpc-go folks about this, and it may be possible to migrate to a better approach while keeping the current API. But clearly there is some work involved.

@sergiitk sergiitk added the Waiting on reporter there was a request for more information without a response or answer or advice has been provided label Mar 27, 2024
@sergiitk sergiitk removed the Waiting on reporter there was a request for more information without a response or answer or advice has been provided label Apr 25, 2024
@ejona86
Copy link
Member

ejona86 commented Jun 20, 2024

This will be tracked in the grpc/grpc repo instead. One of grpc/grpc#36765 and grpc/grpc#36766 will track parts of this. We want to discourage the subtype approach and someone could work on figuring out what the replacement should be.

@ejona86 ejona86 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 20, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants