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

Implement “REPLACE" operation in the Envoyfilter API #27425

Closed
zhaohuabing opened this issue Sep 19, 2020 · 10 comments
Closed

Implement “REPLACE" operation in the Envoyfilter API #27425

zhaohuabing opened this issue Sep 19, 2020 · 10 comments

Comments

@zhaohuabing
Copy link
Member

zhaohuabing commented Sep 19, 2020

Describe the feature request

We want to use EnvoyFIlter API to add a Redis Proxy network filter in the listener, but it conflicts with the existing TCP proxy. Adding a "REPLACE" operation in the Envoyfilter API would solve this problem.

We can't use the Istio build-in Redis xDS implementation because it's too simple to support our Redis use case such as read policy and mirroring.

Describe alternatives you've considered

[ ] Docs
[ ] Installation
[*] Networking
[ ] Performance and Scalability
[ ] Extensions and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Additional context

zhaohuabing added a commit to zhaohuabing/istio that referenced this issue Sep 19, 2020
Add a "REPLACE" operation for EnvoyFilter patch. This operation only supports Network filter and HTTP filter.

This is the implementation of issue istio#27425

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@mandarjog mandarjog changed the title Add a "REPLACE" operation in the Envoyfilter API Implement “REPLACE" operation in the Envoyfilter API Sep 19, 2020
istio-testing pushed a commit that referenced this issue Sep 21, 2020
* Implement REPLACE operation for EnvoyFilter patch

Add a "REPLACE" operation for EnvoyFilter patch. This operation only supports Network filter and HTTP filter.

This is the implementation of issue #27425

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* address comments

Add debug info when no matching filter found.
Add negative test.
Change test case name to indicate http filter.

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* lint

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@howardjohn
Copy link
Member

#27426

zhaohuabing added a commit to zhaohuabing/istio that referenced this issue Nov 24, 2020
Add a "REPLACE" operation for EnvoyFilter patch. This operation only supports Network filter and HTTP filter.

This is the implementation of issue istio#27425

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@hzxuzhonghu
Copy link
Member

@zhaohuabing What's the background of implementing Replace? Especially only for NETWORK_FILTER and NETWORK_FILTER, isn't Remove & Add satisfying you?

@zhaohuabing
Copy link
Member Author

zhaohuabing commented Dec 21, 2021

@hzxuzhonghu The original reason is the one in the description of this issue:

We want to use EnvoyFIlter API to add a Redis Proxy network filter in the listener, but it conflicts with the existing TCP proxy. Adding a "REPLACE" operation in the Envoyfilter API would solve this problem.

Basically, sometimes we need to replace the TCP proxy in the listener with a more specific filter that can understand layer-7 traffic.

@hzxuzhonghu
Copy link
Member

Yeah, i understand it. What i am confused is can Remove and Add do that too?

@zhaohuabing
Copy link
Member Author

The semantic of "Remove & Add" is not exactly the same as "Replace", because order matters in a filter chain.

@hzxuzhonghu
Copy link
Member

Got it, how about InsertBefore/After

@zhaohuabing
Copy link
Member Author

Might work. But then the "Replace" logic would depend on other filters in the filter chain. I prefer "Replace" because it‘s more elegant. Is there any reason we want to eliminate the "Replace" operation?

@zhaohuabing
Copy link
Member Author

zhaohuabing commented Dec 21, 2021

Currently, Aeraki is heavily using "Replace" operation to replace the default TCP filter with protocol-specific filters to support other layer-7 protocols in Istio.

@hzxuzhonghu
Copy link
Member

Not eliminate, just to be clear about the use case

@mandarjog
Copy link
Contributor

@hzxuzhonghu replace is a cleaner version of merge, when you do not want merge semantics. Add-remove makes you handle handling, but replace does not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants