Add support for redirect filters #249
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. |
AliceProxy
force-pushed
the
alicewasko/dev/support-redirects
branch
from
91a8d4b
to
c6ac729
Compare
AliceProxy
changed the title
|
@AliceProxy please rebase when you have a moment. |
AliceProxy
force-pushed
the
alicewasko/dev/support-redirects
branch
2 times, most recently
from
5805f4e
to
af23eca
Compare
danehans
added
the
area/translator
AliceProxy
force-pushed
the
alicewasko/dev/support-redirects
branch
2 times, most recently
from
bc7d2b7
to
3a6b523
Compare
|
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. |
AliceProxy
force-pushed
the
alicewasko/dev/support-redirects
branch
from
940c169
to
037147f
Compare
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
| redirectPort := uint32(*redirect.Port) | ||
| irRedirect.Port = &redirectPort |
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 |
AliceProxy
force-pushed
the
alicewasko/dev/support-redirects
branch
from
fa24b9e
to
2a01b8d
Compare
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
AliceProxy
force-pushed
the
alicewasko/dev/support-redirects
branch
from
2a01b8d
to
8b52e35
Compare
| # 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.
AliceProxy
requested review from
arkodg and
LukeShu
and removed request for
arkodg and
LukeShu
Adds support for gateway API Redirect Filters.
This is my first PR to the repo so feedback is greatly appreciated .
Fixes: #159