-
Notifications
You must be signed in to change notification settings - Fork 360
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
feat: support custom HTTP filter ordering #3273
Conversation
884326f
to
61586ce
Compare
/retest |
6aff883
to
42c3626
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3273 +/- ##
==========================================
+ Coverage 66.51% 66.94% +0.42%
==========================================
Files 161 163 +2
Lines 22673 23370 +697
==========================================
+ Hits 15080 15644 +564
- Misses 6720 6816 +96
- Partials 873 910 +37 ☔ View full report in Codecov by Sentry. |
42c3626
to
b50a0b2
Compare
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
e46dd33
to
e7b276c
Compare
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
} | ||
|
||
// Sort the filters in the custom order. | ||
for i := 0; i < len(filterOrder); i++ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a topological sorting problem? Can we use/reference an established algorithm (and possibly library) to create this sort? It would make it a bit easier to be convinced in the functional correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic could be simplified
- init - ordered list
- loop across filterOrder (overrideList)
- find
name
, and delete it from list - find
before
orafter
and insert it before/after in list
Its still m (orig list)*n (override list) with both sizes being fairly small, but easier to comprehend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a topological sorting problem? Can we use/reference an established algorithm (and possibly library) to create this sort? It would make it a bit easier to be convinced in the functional correctness.
I didn't find a good way to combine two DAGs: the original one and the custom one. Since we have checked the cycle dependencies in the Gateway API reconciler, would the current implementation be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to validate that we don't have cycles like:
spec:
filterOrder:
- name: envoy.filters.http.jwt_authn
before: envoy.filters.http.cors
- name: envoy.filters.http.cors
before: envoy.filters.http.jwt_authn
imo if we apply in order, we wont hit this issue, right ? |
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Added a check for a more friendly UI. |
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
6f69b50
to
e4f6a93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks
thanks for adding those extra comments which will help the next dev looking at the code !
This PR allows users to adjust HTTP filter orders to suit their specific use cases.
It also:
EnvoyExtensionPolicy
to have a maximum of 16 wasm/extproc extensions.FilterPosition
APINote: this PR doesn't handle the ordering of individual wasm/extproc extensions. This will be addressed in a follow-up PR, as it involves API change and may need more discussion.
Related: #2571 #2993