[grpc-transcoding] Transcode unknown variable bindings

[jhance]

I have a use-case where I have a grpc server that is acting as a sort of proxy and needs to forward unknown variable bindings to its upstream. I plan to do the implementation but I would like to upstream this feature so that we don't have to carry forward an internal patch indefinitely and would like early feedback on the idea/implementation notes.

Currently variable bindings from path matching can only be transcoded if the request message contains them, e.g. a route for /route/{path=**} will need a message like this:

message Request {
  string path = 1;
  google.api.HttpBody body = 2;
}

I propose for unknown variable bindings, we recognize a special protobuf in the same way we recognize HttpBody:

message Request {
  // I don't know what package this should go in
  envoy.google.api.extensions.UnknownVariableBindings unknown_bindings = 1;
}

Where this particular message is defined as

message  UnknownVariableBindings {
  map<string, string> bindings = 1;
}

Implementation wise, we can locate the UnknownVariableBindings at the same time we recognize HttpBody and store it in MethodInfoPtr alongside there.

To transcode the actual value, we will take advantage of the fact that an embedded message in protobuf is the same wire format as a string/bytes field. This means we can construct one additional BindingInfo here https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc#L359 (by accumulating the list of unknown bindings in the loop above, and then creating the BindingInfo later, by constructing the protobuf map, serializing it to bytes, and putting the serialized protobuf as the value for the BindingInfo. In this way we won't need any modifications to the upstream grpc transcoder library.

We can either serialize the protobuf by hand in the above loop (to avoid copying) or assume that the amount of keys is rather small and construct then serialize at the end (for readability).

[jhance]

Suggestion by @euroelessar that I also like: We can place this in HttpBody instead.

However, variable bindings don't have much to do with the http body, and extensions is supposedly meant for response, so I'm unsure on this. (The code for writing HttpBody is also more confusing to me). The API is also less ergonomic for user, as theres an Any in extensions, and thus typing is compromised at that location. It also wouldn't let the interface define whether the upstream is interested in unknown bindings, which seems vaguely ideal.

[qiwzhang]

My feeling is such usage is very special, it requires special codes in the grpc_transcoder filter to handle it, it may de-stabilize the code which is used by many products. It will make the code too complicated that it is not easy to maintain.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[euroelessar]

Hi @qiwzhang,

From usage point of view, we have a use case where individual grpc endpoints can serve hundreds of individual paths using the same logic. The caveat is that this endpoint needs to know some additional metadata derived from the path segments.
One of potential solutions is to pass original url to the server and replicate all the parsing/matching flow but that's arguable too much of a burden for the application (+ the complexity of ensuring that route configuration is in sync between envoy and the application).

From complexity point of view, the proposed change of populating extensions field with unknown bindings is 3 semantically-meaningful lines of code (+ mechanical addition of new argument to some functions + tests), so it's likely not too bad.
It can be gated by a boolean option to ensure that it doesn't affect any products by default.

Another alternative would be to provide support of extensions for transcoding logic (e.g. allow users to write own extensions based on extensions field of HttpBody in response + similar flow in request. This is more powerful but arguably much more invasive change.

Any thoughts?

[qiwzhang]

@euroelessar @jhance I am still not fully understand the proposal. How about a draft pr to see how complex it is?

[jhance]

@qiwzhang Here is draft PR: #15338

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.