Skip to content

thrift: EncoderFilter and BidirectionalFilter support #20816

Merged
zuercher merged 32 commits intoenvoyproxy:mainfrom
JuniorHsu:encoder
May 5, 2022
Merged

thrift: EncoderFilter and BidirectionalFilter support #20816
zuercher merged 32 commits intoenvoyproxy:mainfrom
JuniorHsu:encoder

Conversation

@JuniorHsu
Copy link
Copy Markdown
Contributor

@JuniorHsu JuniorHsu commented Apr 14, 2022

Commit Message:
EncoderFilter and BidirectionalFilter support in thrift_proxy filter.

In this diff, the EncoderFilter and BidirectionalFilter support

  • peek the metadata and content for encoding
  • modify the metadata and content for encoding

Do not support

  • pass through data separately for encode and decode, e.g., passThroughData in encoding but not passThroughData in decoding
  • StopIteration in encoding (would intentionally crash)

Additional Description:
Risk Level: low
Testing: Unit test
Docs Changes:
Release Notes:
[Optional Fixes #Issue] #20497

kuochunghsu added 7 commits April 13, 2022 17:42
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu JuniorHsu requested a review from zuercher as a code owner April 14, 2022 01:03
@JuniorHsu
Copy link
Copy Markdown
Contributor Author

Still need more tests and documentation but it would be good to have some early feedback.

cc @mattklein123 @zuercher @rgs1 @fishcakez

kuochunghsu added 4 commits April 14, 2022 13:50
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

Left an idea on how to potentially avoid some of the boilerplate that's needed.

kuochunghsu added 2 commits April 19, 2022 16:26
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu
Copy link
Copy Markdown
Contributor Author

Currently the connection manager and the upstream request won't expect the encoder filter would StopIteration
https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/network/thrift_proxy/conn_manager.cc#L213
https://github.com/envoyproxy/envoy/blob/c90728a183eab491f1603ecbbf45d23cab0d08d6/source/extensions/filters/network/thrift_proxy/router/upstream_request.cc#129
That is, if all the response data arrive, the upstream request would expect we got a final response synchronously.

Therefore I'd like to not supporting StopIteration (i.e., throw if the encoder filter returns StopIteration). I'll update the limitation in the summary

kuochunghsu added 3 commits April 20, 2022 18:05
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu
Copy link
Copy Markdown
Contributor Author

I'm trying to test if modification of values works (i.e., boolValue, byteValue, etc.)

But the stringValue passes the string_view, which indicates not to modify.

virtual FilterStatus stringValue(absl::string_view value) PURE;

@JuniorHsu
Copy link
Copy Markdown
Contributor Author

JuniorHsu commented Apr 21, 2022

Ideally we could pass through data separately for encode and decode, e.g., passThroughData in encoding but not passThroughData in decoding.

request check

bool ConnectionManager::passthroughEnabled() const {

response check

bool ConnectionManager::ResponseDecoder::passthroughEnabled() const {

kuochunghsu added 5 commits April 21, 2022 16:40
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
kuochunghsu added 2 commits April 25, 2022 12:45
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu JuniorHsu requested a review from rgs1 April 25, 2022 19:46
@alyssawilk
Copy link
Copy Markdown
Contributor

@zuercher ping?

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
kuochunghsu added 2 commits April 28, 2022 11:55
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu JuniorHsu changed the title thrift: EncoderFilter and BidirectionFilter support thrift: EncoderFilter and BidirectionalFilter support Apr 28, 2022
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu JuniorHsu requested a review from zuercher April 28, 2022 19:06
@zuercher
Copy link
Copy Markdown
Member

@rgs1 did you have any other comments for this PR?

Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks! Just two more questions.

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu JuniorHsu requested a review from rgs1 April 29, 2022 21:09
Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

LGTM modulo one comment nit. Thanks!

kuochunghsu added 3 commits May 3, 2022 09:33
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
…ogs/1.23.0.yaml

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu
Copy link
Copy Markdown
Contributor Author

@zuercher could you help to move this forward? thanks!

@JuniorHsu
Copy link
Copy Markdown
Contributor Author

@alyssawilk Sorry for random ping. Any chance to move this forward to avoid keeping rebase on a 2K+ lines diff?
All the comments from @.zuercher and @.rgs1 are already addressed and @.rgs1 approved.
Thanks!

@zuercher zuercher merged commit 4afdf50 into envoyproxy:main May 5, 2022
@JuniorHsu JuniorHsu deleted the encoder branch May 5, 2022 20:14
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
EncoderFilter and BidirectionalFilter support in thrift_proxy filter.

In this diff, the EncoderFilter and BidirectionalFilter support peek at the metadata
and content for encoding and modify the metadata and content for encoding.
Does not support:
* pass through data separately for encode and decode, e.g.,  passThroughData in encoding but not passThroughData in decoding.
* StopIteration in encoding (would intentionally crash)

Risk Level: low
Testing: Unit test
Docs Changes: n/a
Release Notes: n/a
Fixes envoyproxy#20497

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants