thrift proxy: add request shadowing support

[rgs1]

This is the last PR (5/5) in the series dedicated to adding mirroring support.
It adds:

• the ShadowWriter which is used by the Router filter to mirror requests
• a ShadowRouter which is a RequestOwner that reuses the existing UpstreamRequest

There's still room for more code sharing and better interfaces, but that
can happen in future iterations.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

[repokitteh-read-only]

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

🐱

Caused by: #17544 was opened by rgs1.

see: more, trace.

[adisuissa]

General API question, as I'm probably missing some context here:
Why not use the RequestMirrorPolicy type that is part of envoy.config.route.v3.RouteAction?

[adisuissa]

I think you should remove the "or if the key is not present, the default value," as it complicates things, and you already wrote above "If not specified, all requests..." (I suggest moving this sentence to be the last, so you first explain what this field is, and then note the default value).

[rgs1]

This comes verbatim from the HTTP message, shall I clean up the description for both in a follow-up? Per my other comment, I rather keep them separate but happy to clarify both of them in one go.

[rgs1]

@zuercher here's a quick exegesis of this implementation, before you jump into the details:

• all copy/pasta is gone, thanks to the previous refactoring PRs (there's a few nits for improvement, but really small).

• UpstreamRequest was originally designed around two things: a) the upstream connection being ready and b) events generated by the downstream connection (namely, parts of the request message becoming available). Given that – in the case of a shadow request – these two things will happen independently of the original request, I introduced a std::list to save pending request events so that we can delay things until the shadow request's upstream connection becomes available. I am more than happy to discuss more appropriate data structures for this, but I do think the scheme of saving events from the original request for whenever the shadow request is ready probably works OK given it allows us to avoid additional plumbing to UpstreamRequest.

• Finally, there's the dance between the Router and the ShadowRouter – they are technically independent but there are a few check points in which they need to coordinate. I think there's room for improvement for how this is implemented now (basically the Router signaling that it's done and thus allowing the ShadowRouter to clean itself up), so happy to explore alternative approaches.

There's also the fact that there's no timeouts for shadow requests in this first iteration, but I think that can be done in a follow-up. And of course I'll address the low coverage issue once we agree on the general direction of the PR.

cc: @fishcakez for feedback as well.

[zuercher]

We'll need to address this:

Code coverage for source/extensions/filters/network/thrift_proxy is lower than limit of 96.6 (93.4)
Code coverage for source/extensions/filters/network/thrift_proxy/router is lower than limit of 96.6 (77.5)

I expect it's probably because the pending_callbacks_ path isn't exercise in the tests (or isn't exercised for all the struct/field/list/map/set begin/end pairs.

[zuercher]

I'm out next week, but I think this is fine to merge once the code coverage failure is resolved.

[rgs1]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17544 (comment) was created by @rgs1.

see: more, trace.

[rgs1]

Windows failure is unrelated:

//test/integration:proxy_proto_integration_test                          FAILED in 2 out of 2 in 6.0s

[rgs1]

General API question, as I'm probably missing some context here:
Why not use the RequestMirrorPolicy type that is part of envoy.config.route.v3.RouteAction?

We could potentially, however I am hesitant to reuse given the comments there have HTTP-isms in them, e.g.: the host/authority headers and there's also the trace span bits which currently aren't implemented for ThriftProxy.

So I think it's probably better to have a bit of duplication but a cleaner self contained configuration message.