Use UsePortInRedirects only if enabled

[aledbf]

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

letsencrypt/pebble#200

Special notes for your reviewer:

[ElvinEfendi]

I think we should stick to $best_http_host and trip :<port> instead if it exists.

Otherwise requests coming through an another reverse proxy with X-Forwarded-Host will get an incorrect Location.

[aledbf]

I think we should stick to $best_http_host and trip :<port> instead if it exists.

Ok but that should be done in lua (runtime value). That's fine to you?

[ElvinEfendi]

Maybe something like

--- a/rootfs/etc/nginx/template/nginx.tmpl
+++ b/rootfs/etc/nginx/template/nginx.tmpl
@@ -1021,6 +1021,25 @@ stream {
             {{ end }}

             rewrite_by_lua_block {
+              {{/* redirect to HTTPS can be achieved forcing the redirect or having a SSL Certificate configured for the server */}}
+              {{ if (or $location.Rewrite.ForceSSLRedirect (and (not (empty $server.SSLCert.PemFileName)) $location.Rewrite.SSLRedirect)) }}
+              {{ if not (isLocationInLocationList $location $all.Cfg.NoTLSRedirectLocations) }}
+              # enforce ssl on server side
+              if ngx.var.redirect_to_https == 1 {
+                  {{ if $location.UsePortInRedirects }}
+                  -- using custom ports require a different rewrite directive
+                  {{ local redirect_port = (printf ":%v" $all.ListenPorts.HTTPS) }}
+                  -- I'm not sure about following
+                  -- error_page 497 ={{ $all.Cfg.HTTPRedirectCode }} https://$host{{ $redirect_port }}$request_uri;
+
+                  ngx.exit(497)
+                  {{ else }}
+                  return ngx.redirect("full url without port", {{ $all.Cfg.HTTPRedirectCode }})
+                  {{ end }}
+              }
+              {{ end }}
+              {{ end }}
+
                 balancer.rewrite()
             }

@@ -1121,24 +1140,6 @@ stream {
             set $proxy_upstream_name    "{{ buildUpstreamName $location }}";
             set $proxy_host             $proxy_upstream_name;

-            {{/* redirect to HTTPS can be achieved forcing the redirect or having a SSL Certificate configured for the server */}}
-            {{ if (or $location.Rewrite.ForceSSLRedirect (and (not (empty $server.SSLCert.PemFileName)) $location.Rewrite.SSLRedirect)) }}
-            {{ if not (isLocationInLocationList $location $all.Cfg.NoTLSRedirectLocations) }}
-            # enforce ssl on server side
-            if ($redirect_to_https) {
-                {{ if $location.UsePortInRedirects }}
-                # using custom ports require a different rewrite directive
-                {{ $redirect_port := (printf ":%v" $all.ListenPorts.HTTPS) }}
-                error_page 497 ={{ $all.Cfg.HTTPRedirectCode }} https://$host{{ $redirect_port }}$request_uri;
-
-                return 497;
-                {{ else }}
-                return {{ $all.Cfg.HTTPRedirectCode }} https://$best_http_host$request_uri;
-                {{ end }}
-            }
-            {{ end }}
-            {{ end }}
-

[aledbf]

@ElvinEfendi ready for another review

[ElvinEfendi]

Why's the else condition here? Why not only return host_port[0]?

[aledbf]

Using return host_port[0]; returns ""

[ElvinEfendi]

that's because in Lua indexes start from 1, so you need to return host_port[1]

[ElvinEfendi]

I think it should consider X-Forwarded-Host during redirection. This test is passing only because you aren't trusted x-forwarded-headers.

[ElvinEfendi]

@aledbf test failure is legit: https://travis-ci.org/kubernetes/ingress-nginx/jobs/496590904#L3976

[ElvinEfendi]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ElvinEfendi,aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment