Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thrift: Payload to metadata filter #23409

Merged
merged 32 commits into from
Oct 28, 2022
Merged

Conversation

JuniorHsu
Copy link
Contributor

Commit Message:
Add a thrift filter to support adding dynamic metadata by payload

Additional Description:
This filter is configured with request_rules that will be matched against requests. A field_selector of a rule represents the head of a linked list, each node of the linked list has a name for logging and an id for matching. The field_selector is tied to a payload field when the linked list corresponds to a downward path which rooted in the top-level of the request message structure. on_present is triggered when corresponding the payload is present. Otherwise, on_missing is triggered.

This filter is designed to support payload passthrough. By performing payload to metadata filter can do deserialization once, and pass the metadata to other filters. This means that load balancing decisions, consumed from log and routing could all use payload information with a single parse. Also notably performing the parsing in payload passthrough buffer will mean deserialization once and not re-serializing, which is the most performant outcome.

Risk Level: low
Testing: unit
Docs Changes: multiple rst
[Optional Fixes #Issue] #23322

kuochunghsu added 11 commits October 5, 2022 11:06
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
doc
Signed-off-by: kuochunghsu <[email protected]>
doc
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #23409 was opened by JuniorHsu.

see: more, trace.

Signed-off-by: kuochunghsu <[email protected]>
kuochunghsu added 6 commits October 7, 2022 21:13
Signed-off-by: kuochunghsu <[email protected]>
doc
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
@JuniorHsu
Copy link
Contributor Author

I address everything on top of my head except allowing multiple rules in a payload node.
https://github.com/envoyproxy/envoy/pull/23409/files#diff-7c9ea70a9bcf34c08c4213b100d808ad3c5462ba16624cbc37dace202cd87621R63

Will address this and do more testing on next biz days

kuochunghsu added 5 commits October 9, 2022 07:40
@JuniorHsu
Copy link
Contributor Author

/retest

Signed-off-by: kuochunghsu <[email protected]>
Signed-off-by: kuochunghsu <[email protected]>
rgs1
rgs1 previously approved these changes Oct 20, 2022
Copy link
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, thanks!

Over to @zuercher for merging (after you've merged main).

@JuniorHsu
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #23409 (comment) was created by @JuniorHsu.

see: more, trace.

@zuercher

This comment was marked as outdated.

@repokitteh-read-only

This comment was marked as outdated.

@zuercher
Copy link
Member

/assign-from @envoyproxy/api-shepherds

@repokitteh-read-only
Copy link

@envoyproxy/api-shepherds assignee is @lizan

🐱

Caused by: a #23409 (comment) was created by @zuercher.

see: more, trace.

changelogs/current.yaml Outdated Show resolved Hide resolved
zuercher
zuercher previously approved these changes Oct 21, 2022
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

I’ll leave the discussion about the google.protobuf.Int32 vs int32 for the api shepherds.

@JuniorHsu
Copy link
Contributor Author

@lizan do you mind to take a look at api part again? Thanks.

@lizan lizan merged commit cd208a5 into envoyproxy:main Oct 28, 2022
@JuniorHsu JuniorHsu deleted the payloadToMetadata branch October 28, 2022 17:24
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.

5 participants