httproute: introduce RequestRedirect filter #683
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbagdi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
apis/v1alpha2/httproute_types.go
Outdated
| // header in the response. | ||
| AutoHTTPS bool `json:"auto_https,omitempty"` | ||
| // TODO if defaulting is possible here (not sure about defaulting in filter), default this? | ||
| // What status codes should be allowed? 301,302,307,308? |
There was a problem hiding this comment.
If we have bounded ones one option we should consider is status code number vs an enum? like MOVED_PERMANENTLY, etc.
There was a problem hiding this comment.
My concern with enum is it is one more piece of configuration that the user needs to learn and remember.
We could prevent errors via validation here if that is the concern.
There was a problem hiding this comment.
Agree that I think numbers will likely be easier to understand here.
apis/v1alpha2/httproute_types.go
Outdated
|
|
||
| // HTTPRequestRedirect defines configuration for the RequestRedirect filter. | ||
| type HTTPRequestRedirect struct { | ||
| // AutoHTTPS if set to true redirects any incoming HTTP requests to HTTPS. |
There was a problem hiding this comment.
Gateway owner wants to perform HTTP->HTTPS on all requests. Could this be a property of GatewayClass?
Would it be possible to define a route for * and set auto_https?
There was a problem hiding this comment.
Even better if our use case also supports ACME where we probably want to redirect eveything but /.wellknown/acme
There was a problem hiding this comment.
Even better if our use case also supports ACME where we probably want to redirect eveything but /.wellknown/acme
Solid point!
Would it be possible to define a route for * and set auto_https?
Gateway admin can but it has limitations. The most specific route matches a request and if that doesn't have the filter defined, then HTTP requests will be accepted.
There was a problem hiding this comment.
But in that case you could have a listener on 80 that only selected the auto redirect route and a listener on 443 that selected all the other routes? This seems like it could become a common pattern.
apis/v1alpha2/httproute_types.go
Outdated
| RedirectStatusCode *int `json:"redirect_status_code,omitempty"` | ||
| // Location denotes the value of location header. | ||
| // +optional | ||
| Location *string `json:"location,omitempty"` |
There was a problem hiding this comment.
I am a bit concerned with having the raw header here, rather than something a bit more abstract, like
redirect:
uri: /v1/bookRatings
authority: newratings.default.svc.cluster.local
The reason is that I can no longer make it generic - I cannot just say redirect /v1 to /v2, I need to now also include http vs https, so now the route is aware of the TLS layer which is leaking from Gateway
There was a problem hiding this comment.
Does it makes sense to have arbitrary headers here (and do proxies support that)? If so, you can use the same name list structure as the other parts of the API.
There was a problem hiding this comment.
I need to now also include http vs https
IME this is not an issue and users want to be as explicit as possible. But you do make a good point that the user now has to be aware of authority as well, which could be abstracted away in Gateway for some use cases. I'll update.
dc784d0 to
3619b0a
Compare
|
Outstanding questions:
|
It looks like 301 and 302 are fairly broadly supported, but it trails off quickly after that. (See GCP, AWS, and Azure docs). Maybe 301 and 302 values would be considered core and the rest would be extended?
I think we should consider this out of scope for now. |
Having a conformance at that level would be very confusing for our users. How about we start with only 301 and 302 and have that included as core conformance? Also, I'm assuming this filter would be under core conformance,. Thoughts? |
|
@hbagdi Agree with both of those - makes sense to start with just 301 and 302 and consider the filter a core feature. |
3619b0a to
309e53c
Compare
|
Updated the PR to take into account (I think) all the discussions we have had so far. |
|
Thanks for the work on this @hbagdi! This mostly LGTM, just a couple requests:
|
<
5B9E
a title="httproute: introduce RequestRedirect filter" data-pjax="true" class="Link--secondary markdown-title" href="/kubernetes-sigs/gateway-api/pull/683/commits/84f15d0a046e3dd2b62658f50d3c8c71a49e2c6f">httproute: introduce RequestRedirect filter
309e53c to
84f15d0
Compare
This was found while working on kubernetes-sigs#683. This was assumed to be fixed with kubernetes-sigs#670 but as it turns out we have two kubectl apply in our shell script.
This was found while working on kubernetes-sigs#683. This was assumed to be fixed with kubernetes-sigs#670 but as it turns out we have two kubectl apply in our shell script.
|
/lgtm |
|
I had put in a hold when this was a draft. Removing it now (which will merge this in). |
| // +optional | ||
| // +kubebuilder:default=302 | ||
| // +kubebuilder:validation=301;302 | ||
| StatusCode *int `json:"redirect_status_code,omitempty"` |
There was a problem hiding this comment.
Sorry to bring this up after the merge, but should the json tag be lower camel-case?
| StatusCode *int `json:"redirect_status_code,omitempty"` | |
| StatusCode *int `json:"statusCode,omitempty"` |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Introduces a new filter for configurable redirects.
Which issue(s) this PR fixes:
Fixes #200 (funny :)
Does this PR introduce a user-facing change?:
This draft PR is to get the conversation started on what we would like to see here.
I opted to do a filter to not further add to the surface of the API and a filter seemed like a good place to add this functionality.
One use case that this proposal doesn't solve:
Would love to hear more thoughts here.
/cc @robscott @bowei @jpeach @danehans
/hold