Add support for redirect filters#249
Add support for redirect filters#249arkodg merged 5 commits intoenvoyproxy:mainfrom Alice-Lilith:alicewasko/dev/support-redirects
Conversation
Codecov Report
@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 58.12% 59.34% +1.22%
==========================================
Files 31 32 +1
Lines 2832 3215 +383
==========================================
+ Hits 1646 1908 +262
- Misses 1087 1199 +112
- Partials 99 108 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Alice-Lilith
changed the title
|
@AliceProxy please rebase when you have a moment. |
danehans
added
the
area/translator
|
Sorry about the recent pushes. I thought I was done with this but realized there were some problems that I needed to fix and I added some better test coverage. |
|
@skriss do you mind reviewing since you have a good understanding of this code? |
|
@AliceProxy can you investigate the CI failure? |
|
Yep it's on my list to review, just need to find some time! |
@danehans The most recent commit should resolve the CI failure. |
skriss
left a comment
skriss
left a comment
There was a problem hiding this comment.
Thanks @AliceProxy, overall this looks great! Had a few initial comments, still looking through the test cases.
|
One other note, just for future reference - our initial goal for EG is to meet "core" Gateway API conformance. As such, I think it'd be fine to start by only implementing the parts of these filters that are required for core conformance, and save "extended" features for later (that's the approach that's been taken with most of the existing Gateway API code). It might help keep the PRs a bit smaller. |
internal/gatewayapi/translator.go
Outdated
There was a problem hiding this comment.
We're ending up with a lot of silly code that looks like this. Perhaps we should add a utility function
func ptrTo[T any](x T) *T {
return &x
}There was a problem hiding this comment.
Very true, but I kind of feel like this should be a separate PR after this just to keep the logical size smaller.
|
@AliceProxy thanks for adding the tests in the IR and xDS libs ! LGTM from my end wrt code in those libs, hoping others can review the |
Signed-off-by: AliceProxy <[email protected]>
Signed-off-by: AliceProxy <[email protected]>
Signed-off-by: AliceProxy <[email protected]>
Signed-off-by: AliceProxy <[email protected]>
Signed-off-by: AliceProxy <[email protected]>
skriss
left a comment
skriss
left a comment
There was a problem hiding this comment.
LGTM! Will leave for other reviewers to merge when ready.
| # I believe the correct way to handle an invalid filter should be to allow the HTTPRoute to function | ||
| # normally but leave out the filter config and set the status, but this behaviour can be changed. |
There was a problem hiding this comment.
Yeah, I think the expected behavior here is unclear in the Gateway API spec -- we can drive some clarity upstream, but doesn't need to block this PR now.
Alice-Lilith
requested review from
LukeShu and
arkodg
and removed request for
LukeShu and
arkodg
6 participants
Adds support for gateway API Redirect Filters.
This is my first PR to the repo so feedback is greatly appreciated .
Fixes: #159