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

Fix redirect https if path changed with rewrite-target #179

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

jcmoraisjr
Copy link
Owner

@jcmoraisjr jcmoraisjr commented Jun 22, 2018

Fix redirect-https if used with rewrite-target.

Option 1 (current): use an acl so reqrep will only execute if redirect https isn't executed
Option 2: write the whole new location using txn.path instead of redirect https

Fixes #178 .

@qwylz
Copy link

qwylz commented Jun 22, 2018

Thanks for quickly addressing this (and thanks for a generally great ingress, too!).

I had a look at the patched template. It fixes the issue when the specific ingress has ssl-redirect enabled, but not when that is the default.

Don't we need something like the following (only showing one of the modified reqrep lines)?

reqrep ^([^\ :]*)\ {{ $location.Path }}/?(.*$) \1\ {{ $rewriteTarget }}\2 if {{ if $server.SSLRedirect }}from-https {{ else }}{{ if $location.SSLRedirect }}from-https {{ end }}{{end}}{{ $server.ACLLabel }} { var(txn.path) -m beg {{ $location.Path }} }

@jcmoraisjr
Copy link
Owner Author

Indeed. What about server or location inside the same if statement?

@jcmoraisjr
Copy link
Owner Author

... but not when that is the default

Indeed. What about server or location inside the same if statement?

Reverting ... $location actually inherit the default configuration. Did you see any issue with default ssl-redirect config?

@qwylz
Copy link

qwylz commented Jun 25, 2018

Ha! You're right. I didn't realize that and was making assumptions based on other parts of the template file that tested whether ssl redirection was enabled at the server level and location level separately. 84fd631 works correctly. :-)

@jcmoraisjr jcmoraisjr merged commit dae1259 into master Jul 13, 2018
@jcmoraisjr jcmoraisjr deleted the jm-fix-redirect branch July 13, 2018 23:08
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