thrift_proxy: simple thrift router

[zuercher]

Provides a very basic thrift router that can route to clusters
based on method name only. A Thrift DecoderFilter interface is
introduced, but the only available filter is the Router. The
Network filter and router are capable of translating transports
and protocols but presently cannot be configured to do so.

Relates to #2247.

Risk Level: low
Testing: unit and integration testing
Docs Changes: protobuf documentation updated
Release Notes: introduced a basic thrift_proxy routing extension

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

[zuercher]

My apologies that this turned out to be such a large change. In the
end, the direction I took with the Thrift filter needed a lot of
revisions to work with a Thrift router that's capable of translating
transports and protocols. I'm willing to take a stab at breaking this
up into smaller pieces if folks think that would make reviews easier.
Just let me know.

[rgs1]

@zuercher a bit of a pie in the sky question: would this be reusable within another filter? Lets say i've got incoming HTTP1.1 traffic, and I want to write a filter that decodes headers and then fans out to a thrift service to read/write something. Is that something you thought of or would it require a bit refactoring?

cc: @fishcakez

[brian-pane]

I'm half finished reading through this. I'll read the remaining half tomorrow.

[brian-pane]

Is the logic for this reversed? Upon a remote close event, this is incrementing the cx_destroy_local_with_active_rq_ counter. Should it be the cx_destroy_remote_with_active_rq_ counter?

[zuercher]

Yes, but it also turns out that the connection pool will record these, so I'm going to remove this code.

[zuercher]

Er. Sorry. No the conn pool tracks upstream destroy with active request. I'll fix this.

[rgs1]

did a quick pass, will do another one tomorrow

[rgs1]

nit: trailing ;

[rgs1]

is this case-sensitive?

[zuercher]

Yes.

[rgs1]

nit: return? given we know what the event is and it can't be something else

[zuercher]

@rgs1 (re: filter making thrift requests from the http filter stack) I think you could reuse parts of this, but not the ConnManager/Router directly. To send thrift requests to an external server (for auth or whatever), you'd likely want to extend the approach in app_exception_impl.h/.cc with some kind of code generator that would consume Thrift IDL and generate code for objects that can marshal/unmarshal themselves to/from buffers via a Protocol instance. You need some kind of AsyncThriftClient that would handle the request encoding/response decoding and transport and such. You look at the ext_authz filter for more-or-less how that would look.

[brian-pane]

Aside from nits, I have one big question: in the situation where the upstream and downstream use the same Thrift protocol (but potentially different Thrift transports), is it possible to skip the parsing, callbacks, and re-encoding of the message?

[brian-pane]

Can this be a const method?

[brian-pane]

I have the same question about many of the other methods of this class.

[zuercher]

Some can be const, I'll update them.

[zuercher]

The proxy has to modify the sequence id to avoid the situation where the same sequence id from separate downstream clients is sent to a single upstream (there's an outstanding TODO in the code to actually do this). Modifying the sequence id can (for, say, the compact protocol) change the size of the message, which may necessitate modifying the transport frame.

For transports which report size (e.g. framed and header, but not unframed), I think that the mechanism for this would be:

1. the ConnManager uses the frame size to track how much more data is in the current request
2. the Router::Upstream detects (during onPoolReady) that the downstream & upstream are using the same protocol and can compute the correct transport size (handle compact proto size change and/or header size change). Some thought will have to be given as to whether this can be done in the presence of additional arbitrary DecoderFilters. Perhaps there's a way to distinguish between a filter that might modify any part of the request vs. one that only deals with headers/message begin.
3. Router::Upstream writes the modified transport start and message begin and then calls back to ConnManager to indicate that it wishes to deal purely in Buffer::Instances going forward
4. ConnManager passes remaining bytes (based on size from 1) directly to the DecoderFilter as Buffer::Instances which are written directly to the upstream connection.

[fishcakez]

fbthrift has a few more exception types than apache thrift: https://github.com/facebook/fbthrift/blob/04ba53d4503d51a9a27f700b114eb7cea4678dc1/thrift/lib/cpp/TApplicationException.h#L57. Should these be supported too? As an example later in the code InternalError could be replaced by more specific code such as LoadShedding. Would that create sub-optimal experience for apache thrift as the enum value would not exist in the client services logic? Without using the fbthrift exception types there will be feature regressions for some client services as would no longer be able to differentiate.

Is apache thrift being treated as the standard here when there are competing standards?

[zuercher]

I think short-term, we'll stick with the apache definitions. Longer term, we'll probably want to use internal identifiers for various error conditions and then provide a configuration point to control whether they get mapped to apache or fb's values.