feat: add support for from-to-www-redirect#337
feat: add support for from-to-www-redirect#337k8s-ci-robot merged 8 commits intokubernetes-sigs:mainfrom
Conversation
|
Hi @chakravardhan. 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. |
|
/ok-to-test |
|
@chakravardhan the from-www-redirect seems reasonable to me, but have you how it would work with #299? what happens if you have from-www-redirect and that the ones in #299? I'm hoping #299 merges soon. Otherwise, the implementation seems good at a quick glance. As for force-ssl-redirect, I'm not sure if we want or event need this annotation. My understanding as follows:
The problem is that gateway API doesn't have a "listen on 443 with a runtime-generate self-signed cert". That is, we might redirect to HTTPS, but not have an HTTPS listener at all! Haven't looked at the code too much, but LMK what you think. |
Thanks for your reply! @Stevenjin8 Regarding You are absolutely right. Gateway API doesn't have the concept of generating self-signed listener certificates on the fly like NGINX does. If we force a redirect to 443 without actual TLS secrets configured, most Gateway implementations (like GKE) will just reject the route or fail to open the listener, leading to a broken state. Regarding In our current implementation, we generate an entirely separate Because #299 adds If there is an overlap where a user configures both |
|
@chakravardhan I don't think this is quite right. Do you mind writing an e2e/integration test just to verify that behavior is correct/taking a quick look at https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#redirect-fromto-www ? For example, the annotation can redirect host.com to www.host.com and from www.host.com to host.com, depending on the initial configuration. Also when I run it with apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: example-ingress
namespace: default
annotations:
nginx.ingress.kubernetes.io/from-to-www-redirect: "true"
spec:
ingressClassName: nginx
rules:
- host: example.com
http:
paths:
- path: /
pathType: Prefix
backend:
service:
name: httpbin
port:
number: 8000I get apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
annotations:
gateway.networking.k8s.io/generator: ingress2gateway-dev
name: nginx
namespace: default
spec:
gatewayClassName: nginx
listeners:
- hostname: example.com
name: example-com-http
port: 80
protocol: HTTP
status: {}
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
annotations:
gateway.networking.k8s.io/generator: ingress2gateway-dev
name: example-ingress-example-com
namespace: default
spec:
hostnames:
- example.com
parentRefs:
- name: nginx
rules:
- backendRefs:
- name: httpbin
port: 8000
matches:
- path:
type: PathPrefix
value: /
name: rule-0
status:
parents: []
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
annotations:
gateway.networking.k8s.io/generator: ingress2gateway-dev
name: example-ingress-example-com-www-redirect
namespace: default
spec:
hostnames:
- example.com
parentRefs:
- name: nginx
rules:
- filters:
- requestRedirect:
hostname: www.example.com
statusCode: 308
type: RequestRedirect
status:
parents: nullWe have two http routes with the same host name, the last httproute should be apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
annotations:
gateway.networking.k8s.io/generator: ingress2gateway-dev
name: example-ingress-example-com-www-redirect
namespace: default
spec:
hostnames:
- - example.com
+ - www.example.com
parentRefs:
- name: nginx
rules:
- filters:
- requestRedirect:
- hostname: www.example.com
+ hostname: example.com
statusCode: 308
type: RequestRedirect
status:
parents: nullAlso, the gateway would need to be updated apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
annotations:
gateway.networking.k8s.io/generator: ingress2gateway-dev
name: nginx
namespace: default
spec:
gatewayClassName: nginx
listeners:
- hostname: example.com
name: example-com-http
port: 80
protocol: HTTP
+ - hostname: www.example.com
+ name: www.example-com-http
+ port: 80
+ protocol: HTTP
status: {} |
|
Thanks @Stevenjin8, I misunderstood this! I've pushed a fix that addresses both issues:
|
|
@chakravardhan, integration tests go under |
@Stevenjin8, Added the e2e Test. I discovered that while I've updated the translator to natively map the |
|
@chakravardhan could you also have teh tests send to port 443? |
|
I think my main point is that the listeners you create do no have tls certs configured and I'm pretty sure ingress nginx would do that. |
@Stevenjin8, Fixed this in the latest commit! The code scans the Gateway's existing listeners for the dstHost (the target domain), grabs its gatewayv1.ListenerTLSConfig (which contains the test-cert SecretRef), and successfully injects it into the newly generated https redirect Listener. |
Stevenjin8
left a comment
There was a problem hiding this comment.
This will definitely conflict with #330, but I can figure that out.
939b361 to
7f5e1d9
Compare
done |
|
@chakravardhan We'll need to rebase it again. |
…w-redirect annotations
…orm to GatewayAPI 301 validation
…n and Lexicographic precedence
7f5e1d9 to
f739ead
Compare
|
@Stevenjin8, if this PR still makes sense, could you provide LGTM label on this? Thanks |
|
@chakravardhan id rather not lgtm and approve the same PR. Could you get @bexxmodd or @spencerhance to take a look? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bexxmodd, chakravardhan, 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
When the
from-to-www-redirectannotation is encountered and set to"true", the translator generates a completely separate, dedicatedHTTPRoutefor the redirect.Translation Logic:
When the
from-to-www-redirectannotation is encountered and set to"true":HTTPRoutefor the redirect listening on the alternative domain. For example, if the Ingress specifiesexample.com, this feature attaches a newHTTPRoutelistening onwww.example.comthat returns a 308 permanent redirect toexample.com.Gatewayand injects a newListenerfor the alternative domain. This ensures the Gateway accepts traffic for the redirect route.Compatibility with
permanent-redirect(#299)There is a potential edge case where a user specifies both
from-to-www-redirectandpermanent-redirecton the same Ingress resource.How this is handled:
from-to-www-redirect) generates a completely separateHTTPRoutededicated to the www/non-www redirect. Because they operate on differentHTTPRouteobjects or distinct hostnames, they do not overwrite each other during the Intermediate Representation (IR) generation. When emitted, the Gateway API handles this overlap natively and gracefully by selecting the most specificHostnamematch for routing the request.Testing
from-to-www-redirectscenario inconverter_test.goverifying the correct emission of dual Gateway listeners and appropriate side-carHTTPRoutes.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: