transcoding: Teach transcoding filter to be aware of per vhost / per route settings by gagata · Pull Request #11188 · envoyproxy/envoy

gagata · 2020-05-14T01:14:34Z

Commit Message:
Adding support for per-route grpc json transcoding filter.

Additional Description:
Risk Level: low (adds a new usage for the transcoding filter, doesn't change the current behavior / use cases)
Testing:
Docs Changes:
Release Notes:

Fixes #11038

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

repokitteh-read-only · 2020-05-14T01:14:39Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11188 was opened by gagata.

see: more, trace.

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

htuch · 2020-05-15T03:28:26Z

api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto

Informational: this is another place that #8669 would help.

htuch · 2020-05-15T03:29:47Z

api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto

NIt: trasncoder typo

api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

euroelessar

Looks great! few minor comments

euroelessar · 2020-05-29T20:14:50Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h

  Protobuf::DescriptorPool descriptor_pool_;
  google::grpc::transcoding::PathMatcherPtr<MethodInfoSharedPtr> path_matcher_;
-  std::unique_ptr<google::grpc::transcoding::TypeHelper> type_helper_;
+  std::shared_ptr<google::grpc::transcoding::TypeHelper> type_helper_;


what does make this change needed?

euroelessar · 2020-05-29T20:15:41Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h

+  void initPerRouteConfig();

  JsonTranscoderConfig& config_;
+  const JsonTranscoderConfig* per_route_config_{nullptr};


nit: const JsonTranscoderConfig* per_route_config_{}; is equivalent and looks like is more favored in the code base

sounds good, changed in the whole file so it's all consistent

euroelessar · 2020-05-29T21:02:05Z

@htuch Can you advice please, what's a recommended way to make this filter opt-in? For instance, we want to disable it by default but then enable (with different configs) for subset of virtual hosts.
It is acceptable to lift requirement for minimum one field in list of services? Or what are other options?

qiwzhang · 2020-05-29T22:14:39Z

We could remove the two validation rules for descriptor and services to allow empty config at filter config, allow per_route_config to specify a valid one. An empty "descriptor' and services config will be treated as "disabled" since the method will not be matched in the path_matcher.

htuch · 2020-06-01T11:33:02Z

Yeah, per-route filter config override seems a good way to provide per-vhost opt-in.

- services list can be empty (and means that filter is then disabled) - left the requirement on the descriptor set to be still specified (so that it's more explicit that the intention is to disable the filter / won't accidentally end up with missing descriptor field) Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

gagata · 2020-06-05T01:15:31Z

We could remove the two validation rules for descriptor and services to allow empty config at filter config, allow per_route_config to specify a valid one. An empty "descriptor' and services config will be treated as "disabled" since the method will not be matched in the path_matcher.

so I left required validation for descriptor, thinking that it's just going to be more explicit in the configuration... do we prefer to simply not have it specified at all?

api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto

htuch · 2020-06-05T17:36:24Z

api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto

  // the service names specified here, but they won't be translated.
-  repeated string services = 2 [(validate.rules).repeated = {min_items: 1}];
+  // If the list of services is empty, filter is considered disabled.
+  repeated string services = 2 [(validate.rules).repeated = {min_items: 0}];


You can just drop the annotation completely if min_items is 0.

euroelessar · 2020-12-28T21:14:29Z

@htuch @lizan @qiwzhang Can you check the pull request, please? It's in a limbo mode for a long time now.

mattklein123 · 2020-12-28T23:34:55Z

What is the status of this PR? Is it ready to review? Can someone merge main into it?

/wait

qiwzhang · 2020-12-29T00:34:20Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h

+  JsonTranscoderConfig(
+      const envoy::extensions::filters::http::grpc_json_transcoder::v3::GrpcJsonTranscoder&
+          proto_config,
+      Api::Api& api, bool disabled);


is this still needed?

qiwzhang · 2020-12-29T00:46:36Z

test/proto/BUILD

 )

+envoy_proto_descriptor(
+    name = "helloworld_proto_descriptor",


is this descriptor used?

qiwzhang · 2020-12-29T00:51:58Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc


 void JsonTranscoderFilter::doTrailers(Http::ResponseHeaderOrTrailerMap& headers_or_trailers) {
-  if (error_ || !transcoder_) {
+  if (error_ || !transcoder_ || per_route_config_->disabled()) {


is it possible that per_route_config_ is not initialized in this call? e.g. decodeHeader() is not called.

Same question for maybeConvertGrpcStatus() function

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

…ue-11038 Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

mattklein123

Thanks LGTM with a small comment.

/wait

mattklein123 · 2021-01-02T23:47:36Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

+    google::grpc::transcoding::TranscoderInputStream& response_input,
+    std::unique_ptr<Transcoder>& transcoder, MethodInfoSharedPtr& method_info) const {
+
+  if (disabled_) {


Generally we don't do defensive coding like this. I would switch this to an ASSERT with a comment if you like.

…ue-11038 Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

mattklein123

LGTM but I think this needs a release note?

/wait

…-11038 Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

…-11038 Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

gagata · 2021-01-28T03:37:23Z

@mattklein123 i adjusted the release notes, do you think it's ok?

mattklein123

Thanks!

Agata Cieplik added 2 commits May 13, 2020 17:55

Implement GRPC transcoding filter per route

814287e

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

integration tests

0444f24

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

gagata requested a review from lizan as a code owner May 14, 2020 01:14

repokitteh-read-only bot added the api label May 14, 2020

dio assigned euroelessar May 14, 2020

qiwzhang reviewed May 14, 2020

View reviewed changes

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc Outdated Show resolved Hide resolved

htuch reviewed May 15, 2020

View reviewed changes

api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto Outdated

Copy link

Member

htuch May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIt: trasncoder typo

htuch reviewed May 15, 2020

View reviewed changes

api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto Outdated Show resolved Hide resolved

gagata requested review from PiotrSikora, alyssawilk, dio, junr03, mattklein123, snowp and zuercher as code owners May 22, 2020 23:07

Merge branch 'master' of github.com:gagata/envoy into issue-11038

2930989

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

gagata marked this pull request as draft May 23, 2020 00:51

spellcheck + addressing review comments

0122662

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

gagata force-pushed the issue-11038 branch from b919e76 to 0122662 Compare May 23, 2020 01:21

fix unnecessary constructor definition

86f1db7

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

gagata marked this pull request as ready for review May 27, 2020 03:52

euroelessar reviewed May 29, 2020

View reviewed changes

htuch reviewed Jun 5, 2020

View reviewed changes

mattklein123 self-assigned this Dec 28, 2020

repokitteh-read-only bot added the waiting label Dec 28, 2020

qiwzhang reviewed Dec 29, 2020

View reviewed changes

Agata Cieplik added 2 commits December 29, 2020 21:27

review updates

5e6cc98

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into iss…

8427d0a

…ue-11038 Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

repokitteh-read-only bot removed the waiting label Dec 30, 2020

qiwzhang previously approved these changes Dec 30, 2020

View reviewed changes

mattklein123 requested changes Jan 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 2, 2021

Agata Cieplik added 2 commits January 14, 2021 18:23

Merge branch 'master' of https://github.com/envoyproxy/envoy into iss…

58da4ec

…ue-11038 Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

review update (removing the disabled_ check)

1ce6453

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

gagata dismissed qiwzhang’s stale review via 1ce6453 January 15, 2021 03:41

repokitteh-read-only bot removed the waiting label Jan 15, 2021

mattklein123 requested changes Jan 15, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 15, 2021

Base automatically changed from master to main January 15, 2021 23:01

Agata Cieplik added 2 commits January 22, 2021 14:07

Merge branch 'main' of https://github.com/envoyproxy/envoy into issue…

d980425

…-11038 Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

Release note

6082b1c

Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

repokitteh-read-only bot removed the waiting label Jan 22, 2021

gagata requested a review from mattklein123 January 24, 2021 02:10

Merge branch 'main' of https://github.com/envoyproxy/envoy into issue…

312c43e

…-11038 Signed-off-by: Agata Cieplik <cieplik@dropbox.com>

mattklein123 approved these changes Jan 29, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jan 29, 2021

mattklein123 merged commit 5ef3f8d into envoyproxy:main Jan 29, 2021

Conversation

gagata commented May 14, 2020

Uh oh!

repokitteh-read-only bot commented May 14, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

euroelessar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

euroelessar commented May 29, 2020

Uh oh!

qiwzhang commented May 29, 2020

Uh oh!

htuch commented Jun 1, 2020

Uh oh!

gagata commented Jun 5, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

euroelessar commented Dec 28, 2020

Uh oh!

mattklein123 commented Dec 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

gagata commented Jan 28, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants