gRPC/JSON transcoder: enable preserving route after headers modified #569
Conversation
ilackarms
changed the title
ilackarms
changed the title
|
At a high level seems reasonable to me. @lizan can you take a look? |
|
While the change here and the code looks reasonable, as I questioned in envoyproxy/envoy#2847, I'm curious if this is JSON-gRPC specific or it should be handled generically, e.g. a flag to allow Envoy to preserve the route entry of an incoming request and not let filters to modify. |
|
@lizan yup let's find out more details first. |
|
@lizan I'm not sure about other use cases for In theory, if we had a filter that was intentionally designed to modify a request that could otherwise be routed before the filter acted on it, we'd want to delegate to the filter in that case. However it's satisfactory for my use case that this option be generalized, so long as it's available for the gRPC transcoder filter 😄 |
| // If set to true, Envoy will determine the cluster to | ||
| // route to based on the route for the incoming request | ||
| // Defaults to false. | ||
| bool skip_recalculating_route = 5; |
There was a problem hiding this comment.
Name this like use_incoming_request_route? Also document this is not recommended in the situation that native-gRPC and JSON-gRPC co-exists.
There was a problem hiding this comment.
how about match_incoming_request_route? (match is more descriptive than use)
ilackarms
force-pushed
the
skip-recalculating-route
branch
from
0c7e29e to
3c2d865
Compare
| @@ -55,4 +55,16 @@ message GrpcJsonTranscoder { | |||
| // `JsonPrintOptions <https://developers.google.com/protocol-buffers/docs/reference/cpp/ | |||
| // google.protobuf.util.json_util#JsonPrintOptions>`_. | |||
| PrintOptions print_options = 3; | |||
|
|
|||
|
|
|||
| // Whether to skip recalculating the route after the | |||
There was a problem hiding this comment.
"Whether to keep the incoming request route"? To be precise, recalculation doesn't happen with in the filter, it is calculated when it is necessary.
Also please reflow the comment, the column limit is 100.
There was a problem hiding this comment.
afaict there are no columns past 100 (max is 83 that i see)
can you show me the line that is over?
There was a problem hiding this comment.
I'm not saying you're past 100, but you can reflow (fit more words in one line as long as it is below 100), I usually put the paragraph in one line and let tools/check_format.py fix to fix them.
| // Note: this will require routes to be added to match | ||
| // the incoming HTTP requests rather than the outgoing | ||
| // requests (after transcoding). This means routes that | ||
| // point directly to gRPC services cannot be reused for |
There was a problem hiding this comment.
Please clarify what is meant here. I think you're saying that if a route in a RouteConfiguration is used for a gRPC service that is not transcoded, we can't make use of it in combination with match_incoming_request_route to perform transcoding. Is this correct?
There was a problem hiding this comment.
yes, that is correct. Would you like me to clarify that in the comment?
|
@ilackarms friendly ping. |
|
@ilackarms friendly ping again. |
|
@htuch sorry for the delay |
|
|
||
| // Whether to keep the incoming request route after the outgoing headers have been transformed to | ||
| // the match the upstream gRPC service. Note: This means that routes for gRPC services that are | ||
| // not transcoded cannot be used in combination with match_incoming_request_route |
There was a problem hiding this comment.
nit: Please italicize "match_incoming_request_route" and end sentence with a period.
ilackarms
force-pushed
the
skip-recalculating-route
branch
from
02c1d01 to
748fe07
Compare
|
@ilackarms can you fix DCO? Feel free to squash/rebase and force push if needed. Sorry for the trouble. |
ilackarms
force-pushed
the
skip-recalculating-route
branch
from
748fe07 to
b8d492e
Compare
|
@mattklein123 i added the sign off but it looks like DCO is still a red X. any idea what i'm missing? |
|
@ilackarms You need Sign-off for every non-merge commit, feel free to squash all commit to one and add sign-off to that. |
Signed-off-by: ilackarms <sdw35@cornell.edu>
ilackarms
force-pushed
the
skip-recalculating-route
branch
from
b8d492e to
6951506
Compare
|
got it. thanks @lizan |
…2848) the purpose of this PR is to add an option to the JSON-gRPC transcoder filter to allow preserving the original routing decision made by Envoy before this filter is called. Sometimes it is not desired that Envoy will not recalculate the route for a transcoded request; this option makes available that use case for the user. This change depends on envoyproxy/data-plane-api#569 where the option to skip has been added as a parameter for the filter config Discussion can be found here: #2847 Risk Level: Medium Testing: A single unit test that verifies that, when the option is enabled, decoder_callbacks_->clearRouteCache() is not called. The change was also tested manually, where the outgoing route was matched based on the incoming :path header, rather than the outgoing. Addresses #2847 Signed-off-by: ilackarms <sdw35@cornell.edu>
the purpose of this PR is to add an option to the JSON-gRPC transcoder filter to allow preserving the original routing decision made by Envoy before this filter is called. Sometimes it is not desired that Envoy will not recalculate the route for a transcoded request; this option makes available that use case for the user.
Discussion can be found here: envoyproxy/envoy#2847