Ingress NGINX - Support Redirect#299
Conversation
|
Hi @jgreeer. 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. DetailsInstructions 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. |
|
|
|
tbh gwapi 1.5 is just around the corner. I think we can merge this as is. WDYT @mikemorris |
this doesn't have the 307/308 status codes, it emits warnings for them. so we can merge now and then make another PR later for the status code annotations. |
|
/ok-to-test |
|
Changed approach to the following:
Also, always override the path with "/" if no path is provided in the redirect annotation. Gateway api will keep the path from the match in the redirect, ingress-nginx does not. |
| continue | ||
| } | ||
|
|
||
| // Get ingress from the first backend source (all backends in a rule should come from the same ingress) |
There was a problem hiding this comment.
I don thtink this is true. What about canary
There was a problem hiding this comment.
It's not, but when you combine canary and redirect the redirect takes priority and no canary happens. Are there other situations where we might need to check other backend sources?
There was a problem hiding this comment.
Ah, I thought you were agreeing. So fix to account for when canary is enabled? What would that look like?
There was a problem hiding this comment.
Oh I was thinking of using'getNonCanaryIngress'
| } | ||
|
|
||
| // Warn about unsupported custom status code annotation | ||
| if ingress.Annotations[TemporalRedirectCodeAnnotation] != "" { |
There was a problem hiding this comment.
If its the TemporalRedirectCodeAnnotation is 301, its ok though. Also isn't the default 308 anyways? so we need to make a warning anyways.
There was a problem hiding this comment.
*what if the TemporalRedirectCodeAnnotation is already 301
There was a problem hiding this comment.
Default codes are 301/302. I will add a check for if the redirect code annotation is already a default though
Stevenjin8
left a comment
There was a problem hiding this comment.
@jgreeer could you add an integration test now that we have them
|
/approve |
|
@kkk777-7 @spencerhance @bexxmodd could one of you take a look or lgtm this. |
| // Check custom status code annotation | ||
| if codeStr := ingress.Annotations[TemporalRedirectCodeAnnotation]; codeStr != "" { | ||
| code, err := strconv.Atoi(codeStr) | ||
| if err != nil || code != 302 { |
There was a problem hiding this comment.
Isn't HTTPRequestedRedirectFilter supports 303, 307, and 308 now?
There was a problem hiding this comment.
yeah it's about to release with v1.5, i'll go ahead and add it to this PR now
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bexxmodd, jgreeer, Stevenjin8 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* feat(ingressnginx): add support for force-ssl-redirect and from-to-www-redirect annotations * fix(ingressnginx): remove force-ssl-redirect support and document compatibility with #299 * fix(ingressnginx): correct from-to-www-redirect hostnames and gateway generation * test(e2e): add e2e integration test for from-to-www-redirect and conform to GatewayAPI 301 validation * style: fix gofmt error in e2e verifiers * fix(ingressnginx): Fix from-to-www-redirect sectionName API validation and Lexicographic precedence * Fix syntax and redeclaration errors in merge resolution * rebased
What type of PR is this?
Fixes #268
Does this PR introduce a user-facing change?: