thrift: allow translation between downstream and upstream protocols by zuercher · Pull Request #4136 · envoyproxy/envoy

zuercher · 2018-08-13T22:31:53Z

We use the new extension_protocol_options field on Cluster to allow clusters
to be configured with a transport and/or protocol. Downstream requests are
automatically translated to the upstream dialect and upstream responses are
translated back to the downstream's dialect.

Moves the TransportType and ProtocolType protobuf enums out of the
ThriftProxy message to allow their re-use in ThriftProtocolOptions.

Risk Level: low
Testing: integration test
Docs Changes: added thrift filter docs
Release Notes: n/a

Signed-off-by: Stephan Zuercher stephan@turbinelabs.io

We use the new extension_protocol_options field on Cluster to allow clusters to be configured with a transport and/or protocol. Downstream requests are automatically translated to the upstream dialect and upstream responses are translated back to the downstream's dialect. Moves the TransportType and ProtocolType protobuf enums out of the ThriftProxy message to allow their re-use in ThriftProtocolOptions. *Risk Level*: low *Testing*: integration test *Docs Changes*: added thrift filter docs *Release Notes*: n/a Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher · 2018-08-14T18:10:02Z

tag @rgs1, @fishcakez

brirams

lgtm!

rgs1

2 nits

rgs1 · 2018-08-15T01:30:03Z

source/extensions/filters/network/thrift_proxy/router/router_impl.cc

    return ThriftFilters::FilterStatus::StopIteration;
  }

+  std::shared_ptr<const ProtocolOptionsConfig> options =


rgs1 · 2018-08-15T01:30:58Z

source/extensions/filters/network/thrift_proxy/router/router_impl.cc

+          NetworkFilterNames::get().ThriftProxy);
+
+  TransportType transport_type = callbacks_->downstreamTransportType();
+  ProtocolType protocol_type = callbacks_->downstreamProtocolType();


nit: these could be const, if the conditional from below is folded into these statements with ternary operators

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

brian-pane

Thanks for adding this feature. Based on the latest commits, this PR lgtm.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher · 2018-08-16T21:46:18Z

@envoyproxy/maintainers would appreciate a quick review for style/c++ gotchas so that I can merge this.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher · 2018-08-17T01:10:11Z

(merged clang-6.0)

htuch

@zuercher sorry about the delayed review, it looks great, just the style/nits pass you were after.

htuch · 2018-08-17T12:05:43Z

api/envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.proto

+// [#comment:next free field: 3]
+message ThriftProtocolOptions {
+  // Supplies the type of transport that the Thrift proxy should use for upstream connections.
+  // Selecting `AUTO_TRANSPORT` causes the proxy to use the same transport as the downstream


Nit: optionally link back with :ref to the enum definition/values.

htuch · 2018-08-17T12:06:18Z

docs/root/configuration/network_filters/thrift_proxy_filter.rst

+Thrift proxy
+============
+
+* The Thrift proxy is not supported in the v1 API.


I'd drop any mention of v1 API; it's about to disappear at the next release cycle, so one fewer places to clean up.

htuch · 2018-08-17T12:09:18Z

source/extensions/filters/network/thrift_proxy/config.h

+  ProtocolType protocol(ProtocolType downstream_protocol) const override;
+
+private:
+  TransportType transport_;


Nit: these can be const.

htuch · 2018-08-17T12:14:22Z

source/extensions/filters/network/thrift_proxy/conn_manager.h

  virtual Router::Config& routerConfig() PURE;
 };

+class ProtocolOptionsConfig : public Upstream::ProtocolOptionsConfig {


Comments, since this is an abstract interface?

htuch · 2018-08-17T12:18:41Z

test/extensions/filters/network/thrift_proxy/integration.cc

+namespace ThriftProxy {
+
+const std::string& PayloadOptions::modeName() const {
+  static const std::string success = "success";


nit: technically a violation of the static initialization fiasco rule. The easy workaround is to make these char[] or just inline them below, which is clearer IMHO.

htuch · 2018-08-17T12:24:11Z

test/extensions/filters/network/thrift_proxy/integration.cc

+                                                Buffer::Instance& response_buffer) {
+  std::vector<std::string> args = {
+      TestEnvironment::runfilesPath(
+          "test/extensions/filters/network/thrift_proxy/driver/generate_fixture.sh"),


Oh wow, I know this is refactored code, but this is quite the elaborate input generation system. How do you guarantee the availability of the thrift Python libraries and tooling? I had a quick look, it was unclear how Bazel's hermeticity is respected.

The libraries were added to the external dependencies: 56a047b

As far as tooling, the generated python bindings are checked in under the test hierarchy so no tools are needed at build or test time. The upshot is that if ever one wanted to change the service or object definitions you'd need to install apache-thrift to regenerate them, but I think this is better than making the whole of apache-thrift a build dependency.

This seemed like the least invasive way to allow Thrift integration tests. I considered running the client.py/server.py rather than generating fixtures, but orchestrating the processes and ports seemed like more trouble than it was worth.

htuch · 2018-08-17T12:24:47Z

test/extensions/filters/network/thrift_proxy/integration.cc

+
+  std::ifstream::pos_type len = is.tellg();
+  if (len > 0) {
+    std::vector<char> bytes(len, 0);


Could just use https://github.com/envoyproxy/envoy/blob/master/source/common/filesystem/filesystem_impl.h#L56

htuch · 2018-08-17T12:26:18Z

test/extensions/filters/network/thrift_proxy/translation_integration_test.cc

+namespace NetworkFilters {
+namespace ThriftProxy {
+namespace {
+std::string thrift_config;


Why not make this a member of the test fixture below? The name should be thrift_config_ in any case, to prevent confusion with local variables.

htuch · 2018-08-17T12:27:05Z

test/extensions/filters/network/thrift_proxy/translation_integration_test.cc

+        GetParam();
+
+    envoy::config::filter::network::thrift_proxy::v2alpha1::TransportType upstream_transport_proto;
+    switch (upstream_transport) {


Factor these switches out to utility functions in a common library? Seems generally useful across tests, etc.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

htuch

LGTM, feel free to merge if you feel this is final.

* origin/master: (34 commits) fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189) docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194) fix sometimes returns response body to HEAD requests (envoyproxy#3985) admin: fix config dump order (envoyproxy#4197) thrift: allow translation between downstream and upstream protocols (envoyproxy#4136) Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120) upstream: allow extension protocol options to be used with http filters (envoyproxy#4165) [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169) docs: improve gRPC-JSON filter doc (envoyproxy#4184) stats: refactoring MetricImpl without strings (envoyproxy#4190) fuzz: coverage profile generation instructions. (envoyproxy#4193) upstream: rebuild cluster when health check config is changed (envoyproxy#4075) build: use clang-6.0. (envoyproxy#4168) thrift_proxy: introduce header transport (envoyproxy#4082) tcp: allow connection pool callers to store protocol state (envoyproxy#4131) configs: match latest API changes (envoyproxy#4185) Fix wrong mock function name. (envoyproxy#4187) Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) Converting envoy configs to V2 (envoyproxy#2957) Add timestamp to HealthCheckEvent definition (envoyproxy#4119) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

zuercher assigned brian-pane Aug 13, 2018

fix linker problem

843b823

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

brirams previously approved these changes Aug 14, 2018

View reviewed changes

rgs1 reviewed Aug 15, 2018

View reviewed changes

zuercher added 2 commits August 15, 2018 11:33

review comments

25aff1d

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'lyft-master' into stephan/thrift-translation

b9ad8fc

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher dismissed brirams’s stale review via b9ad8fc August 15, 2018 18:37

brian-pane previously approved these changes Aug 15, 2018

View reviewed changes

Merge branch 'lyft-master' into stephan/thrift-translation

3232c68

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher dismissed brian-pane’s stale review via 3232c68 August 15, 2018 19:50

brirams previously approved these changes Aug 15, 2018

View reviewed changes

zuercher added 2 commits August 16, 2018 16:41

Merge branch 'lyft-master' into stephan/thrift-translation

882bd59

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

insure protocol type is set for upstream requests

4102bb7

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher dismissed brirams’s stale review via 4102bb7 August 16, 2018 23:45

brirams previously approved these changes Aug 16, 2018

View reviewed changes

Merge branch 'lyft-master' into stephan/thrift-translation

def044c

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

htuch suggested changes Aug 17, 2018

View reviewed changes

htuch self-assigned this Aug 17, 2018

address review comments

712e1cf

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher dismissed brirams’s stale review via 712e1cf August 17, 2018 19:12

now with more const

4e9b735

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

htuch approved these changes Aug 20, 2018

View reviewed changes

zuercher merged commit c91625e into envoyproxy:master Aug 20, 2018

Conversation

zuercher commented Aug 13, 2018

Uh oh!

zuercher commented Aug 14, 2018

Uh oh!

brirams left a comment

Choose a reason for hiding this comment

Uh oh!

rgs1 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!

brian-pane left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher commented Aug 16, 2018

Uh oh!

zuercher commented Aug 17, 2018

Uh oh!

htuch 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!

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!

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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants