thrift_proxy: Support thrift Header payload passthrough

[fishcakez]

Commit Message: Support thrift Header payload passthrough
Additional Description: Framed was the only transport that supported payload passthrough. However the payload
of Header transport is the same as Framed. Therefore its possible to also use payload
passthrough with Header to Header, Framed to Header and Header to Framed. Therefore
allow those extra three combinations and add integration tests. Note that in future if Header
transforms become supported then passthroughData will need to undo any transforms.

New metrics request_passthrough and response_passthrough are added to show when payload
passthrough occurs. Note that previously response_success was always incremented when
performing payload passthrough, and that is corrected to only occur when parsing the
payload.

This will enable payload passthrough for combinations of downstream/upstream transports that did not
previously perform passthrough. If a private filter does not implement passthroughEnabled or passthroughData
correctly, has enabled payload passthrough and is using Header to Header, Framed to Header or Header to Framed
then the filter may have issues.
Risk Level: Medium
Testing: Extended existing integration tests and verified on local deployment.
Docs Changes: Updated ThriftProxy proto docs.
Release Notes: Minor behavior change.
Platform Specific Features: N/A

[repokitteh-read-only]

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

🐱

Caused by: #18296 was opened by fishcakez.

see: more, trace.

[fishcakez]

cc @caitong93

[fishcakez]

@htuch this doesnt change an API but does increase the scope of a boolean flag, which I believe has the intention to perform the scope increase in this PR.

[htuch]

API change is fine, @zuercher for implementation review.

[htuch]

I would like to get a take though on whether it's likely there is a use case that today there is some traffic that is being decoded based on Header and someone configures this feature and they have some framed traffic that is pass-thru? Will we break some legitimate traffic patterns or are these super weird and unlikely?

[zuercher]

I would like to get a take though on whether it's likely there is a use case that today there is some traffic that is being decoded based on Header and someone configures this feature and they have some framed traffic that is pass-thru? Will we break some legitimate traffic patterns or are these super weird and unlikely?

I think it's very unlikely that this would change behavior for any users. And the cases were it would require either incorrectly implemented thrift filters in Envoy or a bug.

My reasoning:
Envoy's thrift extension makes a distinction between passthrough being enabled and passthrough being supported. Both have to be true for passthrough to actually occur. "Enabled" means configured and all configured thrift filters support passthrough. "Supported" means framed (framed or header in this PR) transport and non-Twitter protocol without translation. The supported check occurs when the message is received, so if translation is necessary or an unframed transport message is sent passthrough is disabled for the connection.

The net effect of the enabled and supported conditions is that passthrough can only happen when it is configured and protocol-level data is not modified by Envoy. That is, in cases where Envoy's parsing of the protocol-level data only serves to validate the message.

If someone were using mixed header and framed transports with a common protocol, the header transport messages would now be passed through, but only if they weren't being modified in the first place.

Possible changes in behavior occur if:

1. a thrift filter indicates passthrough support but modifies data. The existing docs for thrift filters explicitly say passthrough data is not to be modified, so I think this an acceptable risk.
2. if an invalid protocol message is received, Envoy will now pass it through without error. I would expect the upstream to reject it with a protocol error, but it might not. I think this is pretty unlikely and indicates that either the upstream server or Envoy misinterprets the thrift spec.

[htuch]

/lgtm API

@zuercher ack, thanks for the detailed rationale.

[repokitteh-read-only]

no relevant owners for "API"

🐱

Caused by: a #18296 (comment) was created by @htuch.

see: more, trace.

[htuch]

/lgtm api

[fishcakez]

New metrics request_passthrough and response_passthrough are added to show when payload
passthrough occurs. Note that previously response_success was always incremented when
performing payload passthrough, and that is corrected to only occur when parsing the
payload.

Unfortunately there is a similar bug that appears for the upstream metrics too, except we always record an error. I think that it makes sense to fix both the downstream/upstream reply (success v error) metrics in a separate PR before continuing this one. In a separate PR we can also move the success/error detection logic into the DecoderStateMachine::messageBegin so it is not replicated in multiple places (conn manager and shadow request). This will also enable us to significantly simplify the success/error detection code and include that information inside message metadata. Thats a nice improvement over this current PR which skips that detection completely and doesn't increment the metrics.

[zuercher]

Looks good.