-
Notifications
You must be signed in to change notification settings - Fork 451
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
Backendref httproute filter request redirect #3134
base: main
Are you sure you want to change the base?
Backendref httproute filter request redirect #3134
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arnaud-tincelin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @arnaud-tincelin! |
Hi @arnaud-tincelin. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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.
Thanks for this PR, @arnaud-tincelin!
I left some more specific comments in the review, but from a general standpoint I'd like to ask you to stick to the approach posed by the FilterRedirect
tests we already have in place:
- https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/httproute-redirect-host-and-status.go
- https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/httproute-redirect-path.go
- https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/httproute-redirect-port-and-scheme.go
- https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/httproute-redirect-port.go
- https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/httproute-redirect-scheme.go
In this test you are only testing path redirect, which is fine but we should highlight this aspect in the file name and tests names.
Have you tested this against any implementation?
Description: "A single HTTPRoute with a redirection filter for different backends", | ||
Features: []features.SupportedFeature{ | ||
features.SupportGateway, | ||
features.SupportHTTPRoute, |
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.
We need to add here the new feature SupportHTTPRouteFilterRequestRedirect
.
port: 8080 | ||
filters: | ||
- type: FilterRequestRedirect | ||
filterRequestRedirect: |
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.
the filter name is just requestRedirect
pkg/features/features.go
Outdated
@@ -150,6 +150,9 @@ const ( | |||
|
|||
// This option indicates support for HTTPRoute with a backendref with an appProtoocol 'kubernetes.io/ws' (extended conformance) | |||
SupportHTTPRouteBackendProtocolWebSocket SupportedFeature = "HTTPRouteBackendProtocolWebSocket" | |||
|
|||
// This option indicates support for HTTPRoute filter request redirect (extended conformance) | |||
SupportHTTPRouteFilterRequestRedirect SupportedFeature = "HTTPRouteFilterRequestRedirect" |
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.
Since this test is about backend filters, and we already have the SupportHTTPRoute*Redirect
features in place, I'd suggest to rename this one to SupportHTTPRouteBackendPathRedirect
. The same renaming should be applied to the tests.
thank you @mlavacca, it's a lot clearer! I am still working on the code based on your comments and I am trying to run the tests on an AKS with istio gateway or azure gateway but in both cases the tests end up in timeout. Is there something I am missing? The tests seem to be stuck on this:
and I started the tests with
|
Hey folks, just to give an update on this PR - this test can't really work currently as the feature is not yet implemented on the various Gateway API implementations. I started to write the Cilium implementation: cilium/cilium@main...tormath1:cilium:tormath1/backend-request-redirect-filter and I can get the test to pass with this (with one backend at the moment). |
Thanks to @tormath1 to ping us in Cilium slack, and to highlight the potential issue with multiple backends in the current Cilium implementation 🙇. |
What type of PR is this?
/kind test
/area conformance
What this PR does / why we need it:
Which issue(s) this PR fixes:
#2936
Does this PR introduce a user-facing change?: