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

http: added a new dual header mutation filter that could be used as downstream/upstream filter #25658

Merged
merged 21 commits into from
Mar 9, 2023
Merged

http: added a new dual header mutation filter that could be used as downstream/upstream filter #25658

merged 21 commits into from
Mar 9, 2023

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Feb 20, 2023

Commit Message: http: added a new dual header mutation filter that could be used as downstream/upstream filter
Additional Description:

Check #24100 for more detailed context.

Risk Level: low. new L7 extension.
Testing: unit.
Docs Changes: added.
Release Notes: added.
Platform Specific Features: n/a.

@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 @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #25658 was opened by wbpcode.

see: more, trace.

@wbpcode
Copy link
Member Author

wbpcode commented Feb 20, 2023

cc @alyssawilk here is the pr for #24100. This PR is consist of three commits.

The first commit added a new HeaderEvaluator implementation to the common/http. The second PR refactor previous early_header_mutation to use the new implementation in the common/http. The final commit create a new L7 filter.

wbpcode added 2 commits February 20, 2023 08:40
fix format
568a758 
Signed-off-by: wbpcode <[email protected]>
fix metadata
2e0d9ef 
Signed-off-by: wbpcode <[email protected]>
@wbpcode
Copy link
Member Author

wbpcode commented Feb 22, 2023

/assign-from @envoyproxy/api-shepherds

@repokitteh-read-only
Copy link

@envoyproxy/api-shepherds assignee is @htuch

🐱

Caused by: a #25658 (comment) was created by @wbpcode.

see: more, trace.

address all comments
8795e18 
Signed-off-by: wbpcode <[email protected]>
htuch
htuch suggested changes Mar 1, 2023
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.

Looks good at API / design level! Some minor docs nits and a couple of Qs.

changelogs/current.yaml Outdated Show resolved Hide resolved
docs/root/configuration/http/http_filters/header_mutation_filter.rst Outdated Show resolved Hide resolved
docs/root/configuration/http/http_filters/header_mutation_filter.rst Outdated Show resolved Hide resolved
docs/root/configuration/http/http_filters/header_mutation_filter.rst Outdated Show resolved Hide resolved
docs/root/configuration/http/http_filters/header_mutation_filter.rst Show resolved Hide resolved
api/envoy/extensions/filters/http/header_mutation/v3/header_mutation.proto Outdated Show resolved Hide resolved
code and others added 8 commits March 1, 2023 14:01
@htuch
Update changelogs/current.yaml
d830226 
Co-authored-by: htuch <[email protected]>
Signed-off-by: code <[email protected]>
@htuch
Update docs/root/configuration/http/http_filters/header_mutation_filt…
030f818 
…er.rst

Co-authored-by: htuch <[email protected]>
Signed-off-by: code <[email protected]>
@htuch
Update docs/root/configuration/http/http_filters/header_mutation_filt…
fc25876 
…er.rst

Co-authored-by: htuch <[email protected]>
Signed-off-by: code <[email protected]>
Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-h…
126633f 
…eader-mutation-filter
update docs
712fcc8 
Signed-off-by: wbpcode <[email protected]>
update docs
e87cd9d 
Signed-off-by: wbpcode <[email protected]>
non-empty filter configuration
b800fd2 
Signed-off-by: wbpcode <[email protected]>
Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-h…
5aa7f73 
…eader-mutation-filter
htuch
htuch reviewed Mar 6, 2023
View reviewed changes
docs/root/configuration/http/http_filters/header_mutation_filter.rst
and used as downstream or upstream HTTP filter. The filter can be configured to apply the header mutations to the request, response, or both.


In most cases, this filter would be a more flexible alternative to the ``request_headers_to_add``, ``request_headers_to_remove``,
Copy link
Member

Choose a reason for hiding this comment

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

Is it fair to say this alternative use would sometimes also be done via composite filter?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by the composite filter here? The envoy.filters.http.composite filter or multiple header mutation filters?

Copy link
Member

Choose a reason for hiding this comment

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

envoy.filters.http.composite

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the answer is yes. The compositer filter could provides very flexible matching. Although for most users, route level configuration is flexible enough.

source/extensions/filters/http/header_mutation/header_mutation.cc Show resolved Hide resolved
source/extensions/filters/http/header_mutation/header_mutation.cc Outdated Show resolved Hide resolved
test/common/http/header_mutation_test.cc Show resolved Hide resolved
wbpcode added 4 commits March 6, 2023 11:27
no check the request headers
b3c9c92 
Signed-off-by: wbpcode <[email protected]>
add integration test
7bb8bc7 
Signed-off-by: wbpcode <[email protected]>
fix test
6a5272f 
Signed-off-by: wbpcode <[email protected]>
fix test
7a3fdc7 
Signed-off-by: wbpcode <[email protected]>
update test
40e44d7 
Signed-off-by: wbpcode <[email protected]>
@htuch
Copy link
Member

htuch commented Mar 8, 2023

/coverage

@repokitteh-read-only
Copy link

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/25658/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

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

see: more, trace.

@htuch
Copy link
Member

htuch commented Mar 8, 2023

LGTM modulo comment, thanks, this is really nice.

htuch
htuch approved these changes Mar 9, 2023
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, thanks!

@repokitteh-read-only repokitteh-read-only bot removed the api label Mar 9, 2023
@htuch htuch merged commit d448b84 into envoyproxy:main Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@botengyao botengyao botengyao left review comments

@htuch htuch htuch approved these changes

Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wbpcode @htuch @botengyao @markdroth