gRPC/JSON transcoder: enable preserving route after headers modified #2848

ilackarms · 2018-03-19T23:36:16Z

json-grpc transcoder filter: add option to skip clearing the route cache

Description:

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.

Docs Changes:

Link to Data Plane PR

Release Notes:

Link to Data Plane PR (Same as above, unless I misunderstood)

[Optional Fixes #Issue]

Addresses #2847

[Optional API Changes:]
Link to Data Plane PR

lizan · 2018-03-20T23:10:26Z

source/common/grpc/json_transcoder_filter.cc

@@ -217,7 +223,9 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h
  headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Post);
  headers.insertTE().value().setReference(Http::Headers::get().TEValues.Trailers);

-  decoder_callbacks_->clearRouteCache();
+  if (!config_.match_incoming_request_route()) {


Call decoder_callbacks_->route() in before the header conversion if match_incoming_request_route is set, to make sure we're not depending on the behavior that HTTP conn manager pre-calculate route. Since route is cached, this call won't impact performance.

@ilackarms friendly reminder.

htuch · 2018-03-22T22:34:43Z

source/common/grpc/json_transcoder_filter.cc

+  match_incoming_request_route_ = proto_config.match_incoming_request_route();
+}
+
+bool JsonTranscoderConfig::match_incoming_request_route() {


Nit: bool JsonTranscoderConfig::matchIncomingRequestRoute() const

htuch · 2018-03-22T22:36:01Z

source/common/grpc/json_transcoder_filter.h

@@ -62,6 +62,14 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config> {
                   std::unique_ptr<google::grpc::transcoding::Transcoder>& transcoder,
                   const Protobuf::MethodDescriptor*& method_descriptor);

+


This file and the .cc are out-of-sync (the newly declared field doesn't match). Can you update? Thanks?

noticed this and hadn't had a chance to push the change. should be fixed now

htuch · 2018-03-30T20:53:53Z

source/common/grpc/json_transcoder_filter.h

@@ -74,6 +82,8 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config> {
  google::grpc::transcoding::PathMatcherPtr<const Protobuf::MethodDescriptor*> path_matcher_;
  std::unique_ptr<google::grpc::transcoding::TypeHelper> type_helper_;
  Protobuf::util::JsonPrintOptions print_options_;
+
+  bool match_incoming_request_route_{false};


Nit: You can make this a const if you initialize it in the constructor initializor list.

Ping on this one.

are you sure i can? this gets set from the proto_config passed to the constructor

htuch · 2018-03-30T20:56:21Z

@ilackarms this still has build failures.

ilackarms · 2018-04-01T19:08:26Z

@htuch the build is failing because envoyproxy/data-plane-api#569 has not yet been merged.
i can update repository_locations.bzl with my fork of data-plane-api but i assumed it would be better to wait until that's merged

Signed-off-by: ilackarms <[email protected]>

htuch · 2018-04-02T16:44:46Z

source/common/grpc/json_transcoder_filter.h

+   * This allows Envoy to select the upstream cluster based on the incoming request
+   * rather than the outgoing.
+   */
+  bool matchIncomingRequestRoute();


Nit: bool matchIncomingRequestInfo() const;

Ping on this one.

ilackarms · 2018-04-04T17:30:43Z

sorry for the delay on this; having some trouble running bazel on my machine at the moment

Signed-off-by: ilackarms <[email protected]>

htuch

LGTM modulo some nits. @ilackarms if you can do a quick cleanup PR we can merge this.

htuch · 2018-04-06T15:16:06Z

source/common/grpc/json_transcoder_filter.h

+   * This allows Envoy to select the upstream cluster based on the incoming request
+   * rather than the outgoing.
+   */
+  bool matchIncomingRequestRoute();


Ping on this one.

htuch · 2018-04-06T15:18:24Z

test/common/grpc/json_transcoder_filter_test.cc

+public:
+  GrpcJsonTranscoderFilterSkipRecalculatingTest() : GrpcJsonTranscoderFilterTest(true) {}
+};
+


Move this down next to the relevant test.

htuch · 2018-04-06T15:18:57Z

test/common/grpc/json_transcoder_filter_test.cc

@@ -197,17 +205,20 @@ TEST_F(GrpcJsonTranscoderConfigTest, InvalidVariableBinding) {

 class GrpcJsonTranscoderFilterTest : public testing::Test {
 public:
-  GrpcJsonTranscoderFilterTest() : config_(bookstoreProtoConfig()), filter_(config_) {
+  GrpcJsonTranscoderFilterTest(const bool match_incoming_request_route = false)


Nit: const bool doesn't really do anything here, jsut make it bool.

htuch · 2018-04-06T15:19:05Z

source/common/grpc/json_transcoder_filter.h

@@ -74,6 +82,8 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config> {
  google::grpc::transcoding::PathMatcherPtr<const Protobuf::MethodDescriptor*> path_matcher_;
  std::unique_ptr<google::grpc::transcoding::TypeHelper> type_helper_;
  Protobuf::util::JsonPrintOptions print_options_;
+
+  bool match_incoming_request_route_{false};


Ping on this one.

Signed-off-by: ilackarms <[email protected]>

htuch

Thanks!

This was referenced Mar 20, 2018

gRPC/JSON transcoder: enable preserving route after headers modified envoyproxy/data-plane-api#569

Merged

allow preserving the route cache in the JSON-gRPC transcoder filter #2847

Closed

ilackarms force-pushed the skip-recalculating-route branch from b80ddd8 to f55ccd5 Compare March 20, 2018 22:47

lizan reviewed Mar 20, 2018

View reviewed changes

htuch assigned lizan and htuch Mar 21, 2018

htuch reviewed Mar 22, 2018

View reviewed changes

ilackarms force-pushed the skip-recalculating-route branch from f55ccd5 to 729be7f Compare March 29, 2018 20:03

htuch previously approved these changes Mar 30, 2018

View reviewed changes

ilackarms dismissed htuch’s stale review via 20d30d4 April 2, 2018 15:35

ilackarms force-pushed the skip-recalculating-route branch from 20d30d4 to 1f04e68 Compare April 2, 2018 15:36

add option to skip clearing the route cache in grpc transcoder filter

73d7d6d

Signed-off-by: ilackarms <[email protected]>

ilackarms force-pushed the skip-recalculating-route branch from 1f04e68 to 73d7d6d Compare April 2, 2018 15:54

htuch reviewed Apr 2, 2018

View reviewed changes

ilackarms mentioned this pull request Apr 4, 2018

Is there a way to expose a gRPC service as only HTTP and not as a gRPC service? #2991

Closed

ilackarms added 2 commits April 5, 2018 21:18

fix variable name in test

b90ce88

Signed-off-by: ilackarms <[email protected]>

Merge branch 'master' into skip-recalculating-route

9a7de69

htuch reviewed Apr 6, 2018

View reviewed changes

ilackarms added 2 commits April 10, 2018 09:59

nit changes

b877cdc

Signed-off-by: ilackarms <[email protected]>

format

d68c4da

Signed-off-by: ilackarms <[email protected]>

htuch approved these changes Apr 10, 2018

View reviewed changes

htuch merged commit d41d06e into envoyproxy:master Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC/JSON transcoder: enable preserving route after headers modified #2848

gRPC/JSON transcoder: enable preserving route after headers modified #2848

ilackarms commented Mar 19, 2018

lizan Mar 20, 2018

lizan Apr 1, 2018

htuch Mar 22, 2018

htuch Mar 22, 2018

ilackarms Mar 29, 2018

htuch Mar 30, 2018

htuch Apr 6, 2018

ilackarms Apr 10, 2018

htuch commented Mar 30, 2018

ilackarms commented Apr 1, 2018

htuch Apr 2, 2018

htuch Apr 6, 2018

ilackarms commented Apr 4, 2018

htuch left a comment

htuch Apr 6, 2018

htuch Apr 6, 2018

htuch Apr 6, 2018

htuch Apr 6, 2018

htuch left a comment

		@@ -62,6 +62,14 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config> {
		std::unique_ptr<google::grpc::transcoding::Transcoder>& transcoder,
		const Protobuf::MethodDescriptor*& method_descriptor);

gRPC/JSON transcoder: enable preserving route after headers modified #2848

gRPC/JSON transcoder: enable preserving route after headers modified #2848

Conversation

ilackarms commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Mar 30, 2018

ilackarms commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilackarms commented Apr 4, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment