Skip to content
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

Move ingress rewrite to dashboard Nginx #910

Merged
merged 2 commits into from
Jan 3, 2019

Conversation

migmartri
Copy link
Contributor

Based on this discussion #897 (review) I've managed to extract the redirection logic to the Nginx configuration inside the dashboard container as well as made it generic. This is good since now, the redirect does not depend now on the specifics of the Ingress controller as well as the deployment mechanism (Helm chart).

In my previous tests the solution did not work because the redirection was including the internal port (8080)

NOTE: see location header in the following examples.

$ curl -I -H 'Cache-Control: no-cache' https://miguel-kubeapps-dev.k.dev.bitnami.net/kubeapps
HTTP/2 301 
server: nginx/1.15.6
date: Wed, 02 Jan 2019 21:16:01 GMT
content-type: text/html
content-length: 185
location: http://miguel-kubeapps-dev.k.dev.bitnami.net:8080/kubeapps/

I tried disabling the port via the directive port_in_redirect off; but that made the internal server_name to be shown. Adding also server_name_in_redirect did not help either.

$ curl -I -H 'Cache-Control: no-cache' https://miguel-kubeapps-dev.k.dev.bitnami.net/kubeapps
HTTP/2 301
server: nginx/1.15.6
date: Wed, 02 Jan 2019 21:18:43 GMT
content-type: text/html
content-length: 185
location: http://miguel-kubeapps-internal-dashboard/kubeapps/

Setting the redirects relative does the trick.

$ curl -I -H 'Cache-Control: no-cache' https://miguel-kubeapps-dev.k.dev.bitnami.net/kubeapps
HTTP/2 301 
server: nginx/1.15.6
date: Wed, 02 Jan 2019 21:14:43 GMT
content-type: text/html
content-length: 185
location: /kubeapps/

Refs #402

@prydonius prydonius mentioned this pull request Jan 3, 2019
Copy link
Contributor

@prydonius prydonius left a comment

Choose a reason for hiding this comment

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

this is great, thanks. Also fixes the current broken chart when path = /

@prydonius
Copy link
Contributor

Merging due to the current broken chart.

@prydonius prydonius merged commit cf81cbf into vmware-tanzu:master Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants