Adds support for root context redirection #427

rikatz · 2017-03-12T22:09:41Z

This PR adds support for Root Context Redirection.

This is necessary for some Apps (as Java Apps) that aren't created containing a root context, but instead answers only its context.

An example, a Java App that exposes itself as '/app1'. When you create an Ingress, you need to redirect the call to 'app1.ingress.com/' to 'app1.ingress.com/app1'.

TODO:

Make some real test in my production environment to check the behaviour
Improve the rewrite module documentation.

After I finish documentating this, the PR is ready.

@jcmoraisjr Please take a look to see if this is applicable to our case.

k8s-reviewable · 2017-03-12T22:09:45Z

This change is

rikatz · 2017-03-13T15:33:31Z

@aledbf Don't know why CI failed, as those were simple changes.

Can you help me in this to check if this is a real failure?

Thanks!

coveralls · 2017-03-13T16:04:13Z

Coverage increased (+0.04%) to 43.277% when pulling 0e5d3ca on rikatz:app-root-redirect into 0cb8f59 on kubernetes:master.

rikatz · 2017-03-13T16:51:57Z

Tks :D

jcmoraisjr · 2017-03-15T00:45:27Z

examples/rewrite/README.md

@@ -1,3 +1,28 @@
+# Sticky Session
+


This should be Rewrite ?

jcmoraisjr · 2017-03-15T00:51:58Z

@jcmoraisjr Please take a look to see if this is applicable to our case.

@rikatz Sound good. Btw the 302 redirect is indeed a nice choice.

Awaiting this PR merged to add this feature to HAProxy Ingress as well =) Thanks!

coveralls · 2017-03-15T01:44:20Z

Coverage decreased (-0.08%) to 43.165% when pulling 98abd6d on rikatz:app-root-redirect into 0cb8f59 on kubernetes:master.

aledbf · 2017-03-15T22:35:10Z

@rikatz just one change, plese move examples/rewrite/* to examples/rewrite/nginx/

rikatz · 2017-03-16T02:08:07Z

OK, made it as asked.

coveralls · 2017-03-16T02:33:30Z

Coverage increased (+0.02%) to 43.266% when pulling 6f25329 on rikatz:app-root-redirect into 0cb8f59 on kubernetes:master.

rikatz · 2017-03-16T02:36:53Z

@aledbf Travis / Coveralls seems to be broken :( My last commit only moved the Example as you requested, but the CI broke :(

coveralls · 2017-03-16T09:54:34Z

Coverage increased (+0.02%) to 43.266% when pulling 6f25329 on rikatz:app-root-redirect into 0cb8f59 on kubernetes:master.

aledbf · 2017-03-16T10:32:27Z

/lgtm

aledbf · 2017-03-16T10:32:39Z

@rikatz thanks!

jasonbrownbridge · 2017-03-23T12:19:30Z

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl

@@ -236,7 +236,19 @@ http {
        ssl_verify_client on;
        ssl_verify_depth {{ $location.CertificateAuth.ValidationDepth }};
        {{ end }}
+
+        {{ if (or $location.Redirect.ForceSSLRedirect (and (not (empty $server.SSLCertificate)) $location.Redirect.SSLRedirect)) }}


@aledbf this seems broken. I think it should be moved back into the location {{path}} block.

What happens if we have one location with ssl-redirect: "false" and one with ssl-redirect: "true" - won't they now both be redirected?

@rikatz please check ^^

@aledbf I've only changed the piece of code from it's location. The idea is first make it's HTTPs redirection based on the configs (as defined previous in lines 282:287 and then moved to this new location) and then redirect (if it exists) to the root context.

One doesn't exclude other, this code existed previously. It only orders which rules will be followed first :)

By the way, acording to NGINX docs, the IF and redirects can happen inside a location, a if and a server directive. The usage of this directive inside the 'location' limits the redirection only for the current location (maybe only the '/' path), not containing the other paths that might exist.

I'm on vacation (and traveling) right now, but will take a look again in this behaviour ASAP.

@jasonbrownbridge How have you detected this? Can you please reproduce the taken steps, so I can try here and take a look.

@rikatz I was thinking something like this (substituting in appropriate ssl-wildcard secret):

--- apiVersion: extensions/v1beta1 kind: Ingress metadata: name: default-route annotations: kubernetes.io/ingress.class: "nginx" kubernetes.io/ssl-redirect: "true" spec: tls: - hosts: - "*.example.com" secretName: "ssl-wildcard" rules: - host: "www.example.com" http: paths: - path: / backend: serviceName: route-default servicePort: 80 --- apiVersion: extensions/v1beta1 kind: Ingress metadata: name: check-route annotations: kubernetes.io/ingress.class: "nginx" ingress.kubernetes.io/ssl-redirect: "false" spec: rules: - host: "www.example.com" http: paths: - path: /check backend: serviceName: route-check servicePort: 80

My thought is this should create something like:

server { server_name www.example.com; ... location / { if ($pass_access_scheme = http) { return 301 https://$host$request_uri; } ... } location /check { ... }

Instead this code would generate (which always does ssl redirect):

server { server_name www.example.com; ... if ($pass_access_scheme = http) { return 301 https://$host$request_uri; } location / { ... } location /check { ... }

@rikatz will try do a more concrete test case tomorrow (UTC+2 here)

Also @rikatz I'm not blocked here (I use a custom template anyway) - so enjoy your vacation :)

@jasonbrownbridge We all do :P I've removed all the passthrough stuff, as I use only HTTP/s traffic.

Anyway, I think the idea is to always redirect the SSL, as this is a global annotation, isn't it?

This '/check', by the otherside could be inside the same Ingress Object (of the vhost that contains the TLS config). I suppose that Ingress Controller assumes the first created ingress and discards the conflicting one. (you have two ingress objects pointing to the same host, in this case www.example.com)

Also, by the previous template (before this commit), the HTTPs redirection would be inserted anyway in all existing 'path' rules :)

But let me know about your tests!

@aledbf Am I right with this conclusion?

piclemx · 2017-04-04T14:15:30Z

Do you know when this will be in a beta?

This is needed to avoid ingress definitions with different settings for SSL redirection conflicting with each other. NB: This was discussed in the review of kubernetes#427, but ultimately not addressed.

Ricardo Pchevuzinske Katz and others added 2 commits March 12, 2017 19:06

Adds support for root context redirection

04af55a

Merge 04af55a into 0cb8f59

c41e6bd

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2017

Adds support for root redirection, and improves rewrite documentation

0e5d3ca

rikatz changed the title ~~WIP: Adds support for root context redirection~~ Adds support for root context redirection Mar 13, 2017

aledbf closed this Mar 13, 2017

aledbf reopened this Mar 13, 2017

jcmoraisjr reviewed Mar 15, 2017

View reviewed changes

examples/rewrite/README.md Outdated

@@ -1,3 +1,28 @@

# Sticky Session

Copy link

Contributor

jcmoraisjr Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Rewrite ?

Corrects the README header from Rewrite docs

98abd6d

aledbf self-assigned this Mar 15, 2017

Changes the NGINX Rewrite Example location

6f25329

aledbf closed this Mar 16, 2017

aledbf reopened this Mar 16, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2017

aledbf merged commit c25936d into kubernetes:master Mar 16, 2017

jasonbrownbridge reviewed Mar 23, 2017

View reviewed changes

rikatz deleted the app-root-redirect branch March 27, 2017 23:50

rikatz mentioned this pull request Jun 12, 2017

Redirect from root path #263

Closed

ankon mentioned this pull request Jun 28, 2017

Apply the 'ssl-redirect' annotation per-location #919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for root context redirection #427

Adds support for root context redirection #427

rikatz commented Mar 12, 2017 •

edited

Loading

k8s-reviewable commented Mar 12, 2017

rikatz commented Mar 13, 2017

coveralls commented Mar 13, 2017

rikatz commented Mar 13, 2017

jcmoraisjr Mar 15, 2017

jcmoraisjr commented Mar 15, 2017

coveralls commented Mar 15, 2017

aledbf commented Mar 15, 2017 •

edited

Loading

rikatz commented Mar 16, 2017

coveralls commented Mar 16, 2017

rikatz commented Mar 16, 2017

coveralls commented Mar 16, 2017

aledbf commented Mar 16, 2017

aledbf commented Mar 16, 2017

jasonbrownbridge Mar 23, 2017

aledbf Mar 23, 2017

rikatz Mar 27, 2017

rikatz Mar 27, 2017

jasonbrownbridge Mar 27, 2017

jasonbrownbridge Mar 27, 2017

jasonbrownbridge Mar 27, 2017

rikatz Mar 27, 2017

piclemx commented Apr 4, 2017

Adds support for root context redirection #427

Adds support for root context redirection #427

Conversation

rikatz commented Mar 12, 2017 • edited Loading

k8s-reviewable commented Mar 12, 2017

rikatz commented Mar 13, 2017

coveralls commented Mar 13, 2017

rikatz commented Mar 13, 2017

Choose a reason for hiding this comment

jcmoraisjr commented Mar 15, 2017

coveralls commented Mar 15, 2017

aledbf commented Mar 15, 2017 • edited Loading

rikatz commented Mar 16, 2017

coveralls commented Mar 16, 2017

rikatz commented Mar 16, 2017

coveralls commented Mar 16, 2017

aledbf commented Mar 16, 2017

aledbf commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piclemx commented Apr 4, 2017

rikatz commented Mar 12, 2017 •

edited

Loading

aledbf commented Mar 15, 2017 •

edited

Loading