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.

Sign up for GitHub

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

Jump to bottom

Thrift to Metadata filter proto design #33919

Merged
merged 11 commits into from
May 24, 2024
Merged

Thrift to Metadata filter proto design #33919

merged 11 commits into from
May 24, 2024

Conversation

JuniorHsu
Copy link
Contributor

Commit Message:
For apache thrift compatible HTTP requests and responses, this filter parses the thrift metadata and put them into filter dynamic metadata for other filter usage.

This is the initial proto design, which refers to other filters like json_to_metadata and payload_to_metadata.

Additional Description:
Risk Level: low
Testing: build
Docs Changes: yes
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #29371
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Thrift to Metadata filter proto design
5990f89 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@repokitteh-read-only 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 @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #33919 was opened by JuniorHsu.

see: more, trace.

kuochunghsu added 3 commits May 2, 2024 14:53
fix format
40d2f0b 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
proto fix
caf0e7d 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
proto fix
e6e94fe 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu
Copy link
Contributor Author

@htuch could i have an early api review? the test failure is coverage which is expected

@phlax
Copy link
Member

phlax commented May 10, 2024

@zuercher is it ok to assign this one to you?

@zuercher zuercher self-assigned this May 13, 2024
zuercher
zuercher reviewed May 13, 2024
View reviewed changes
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.

As a design this makes sense to me. Given that there's no implementation here, do you want to mark all the protobuf as not yet implemented and move forward with this PR just to get the protobuf API in place?

api/envoy/extensions/filters/http/thrift_to_metadata/v3/thrift_to_metadata.proto Outdated Show resolved Hide resolved
api/envoy/extensions/filters/http/thrift_to_metadata/v3/thrift_to_metadata.proto Outdated Show resolved Hide resolved
api/envoy/extensions/filters/http/thrift_to_metadata/v3/thrift_to_metadata.proto Show resolved Hide resolved
docs/root/configuration/http/http_filters/_include/thrift-to-metadata-filter.yaml Show resolved Hide resolved
docs/root/configuration/http/http_filters/thrift_to_metadata_filter.rst Outdated Show resolved Hide resolved
@JuniorHsu
Copy link
Contributor Author

do you want to mark all the protobuf as not yet implemented and move forward with this PR just to get the protobuf API in place

Sure I'll have a very basic unit test to pass the coverage so we can move on the wip filter. Thanks for the great idea.

@JuniorHsu
Copy link
Contributor Author

do you want to mark all the protobuf as not yet implemented

just notice i already set option (xds.annotations.v3.file_status).work_in_progress = true;

kuochunghsu added 2 commits May 15, 2024 21:53
address comments and add unit test for coverage
ad528df 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
format
922e88f 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu
Copy link
Contributor Author

@zuercher I addressed all the comments except this #33919 (comment) which needs some input. Also add some trivial unit test, hopefully pass the coverage test

@JuniorHsu JuniorHsu requested a review from zuercher May 16, 2024 16:47
zuercher
zuercher previously approved these changes May 23, 2024
View reviewed changes
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.

This looks good to me. I think it needs approval from someone on @envoyproxy/api-shepherds.

api/envoy/extensions/filters/http/thrift_to_metadata/v3/thrift_to_metadata.proto Show resolved Hide resolved
@zuercher
Copy link
Member

do you want to mark all the protobuf as not yet implemented

just notice i already set option (xds.annotations.v3.file_status).work_in_progress = true;

Sorry -- I didn't realize that could be done at the file level vs. per-message.

@JuniorHsu JuniorHsu requested a review from htuch May 23, 2024 04:16
kuochunghsu added 3 commits May 23, 2024 15:05
disambiguate metadata
e61c8fd 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
more explanation on field_selector
5c887cd 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
fix
80daa87 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu JuniorHsu requested a review from htuch May 24, 2024 01:32
typo
a61b646 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
fix ci
1173133 
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
htuch
htuch reviewed May 24, 2024
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

/lgtm api

@JuniorHsu
Copy link
Contributor Author

/retest

@JuniorHsu
Copy link
Contributor Author

we got approval and api reviewed so i think we're good to merge.
#33919 (review)

@htuch htuch merged commit 7081e56 into envoyproxy:main May 24, 2024
53 checks passed
@JuniorHsu JuniorHsu mentioned this pull request May 25, 2024
Merged
@JuniorHsu JuniorHsu deleted the thrift_to_metadata_proto branch June 11, 2024 17:03
zuercher pushed a commit that referenced this pull request Jun 12, 2024
@JuniorHsu
thrift to metadata implementation (#34360)
82dc695 
Implement filter config from #33919.
doc:
https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/thrift_to_metadata_filter.html#envoy-thrift-to-metadata-filter

Risk Level: low (new filter)
Testing: unit
Docs Changes: n/a
Release Notes: added
Fixes #29371
Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
Co-authored-by: kuochunghsu <kuochunghsu@pinterest.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch approved these changes

@zuercher zuercher zuercher left review comments

Assignees

@zuercher zuercher

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JuniorHsu @phlax @zuercher @htuch