Merge pull request #6037 from aledbf/redirect

Do not append a trailing slash on redirects

rootfs/etc/nginx/lua/lua_ingress.lua

@@ -145,11 +145,17 @@ function _M.rewrite(location_config)
   end
 
   if redirect_to_https(location_config) then
-    local uri = string_format("https://%s%s", redirect_host(), ngx.var.request_uri)
+    local request_uri = ngx.var.request_uri
+    -- do not append a trailing slash on redirects
+    if string.sub(request_uri, -1) == "/" then
+      request_uri = string.sub(request_uri, 1, -2)
+    end
+
+    local uri = string_format("https://%s%s", redirect_host(), request_uri)
 
     if location_config.use_port_in_redirects then
       uri = string_format("https://%s:%s%s", redirect_host(),
-        config.listen_ports.https, ngx.var.request_uri)
+        config.listen_ports.https, request_uri)
     end
 
     ngx_redirect(uri, config.http_redirect_code)

rootfs/etc/nginx/template/nginx.tmpl

@@ -554,12 +554,21 @@ http {
         }
         {{ end }}
 
-        {{ if ne $all.ListenPorts.HTTPS 443 }}
-        {{ $redirect_port := (printf ":%v" $all.ListenPorts.HTTPS) }}
-        return {{ $all.Cfg.HTTPRedirectCode }} $scheme://{{ $redirect.To }}{{ $redirect_port }}$request_uri;
-        {{ else }}
-        return {{ $all.Cfg.HTTPRedirectCode }} $scheme://{{ $redirect.To }}$request_uri;
-        {{ end }}
+        set_by_lua_block $redirect_to {
+            local request_uri = ngx.var.request_uri
+            if string.sub(request_uri, -1) == "/" then
+                request_uri = string.sub(request_uri, 1, -2)
+            end
+
+            {{ if ne $all.ListenPorts.HTTPS 443 }}
+            {{ $redirect_port := (printf ":%v" $all.ListenPorts.HTTPS) }}
+            return string.format("%s://%s%s%s", ngx.var.scheme, "{{ $redirect.To }}", "{{ $redirect_port }}", request_uri)
+            {{ else }}
+            return string.format("%s://%s%s", ngx.var.scheme, "{{ $redirect.To }}", request_uri)
+            {{ end }}
+        }
+
+        return {{ $all.Cfg.HTTPRedirectCode }} $redirect_to;
     }
     ## end server {{ $redirect.From }}
     {{ end }}

test/e2e/annotations/forcesslredirect.go

@@ -46,6 +46,6 @@ var _ = framework.DescribeAnnotation("force-ssl-redirect", func() {
 			WithHeader("Host", host).
 			Expect().
 			Status(http.StatusPermanentRedirect).
-			Header("Location").Equal("https://forcesslredirect.bar.com/")
+			Header("Location").Equal("https://forcesslredirect.bar.com")
 	})
 })

test/e2e/annotations/fromtowwwredirect.go

@@ -49,7 +49,7 @@ var _ = framework.DescribeAnnotation("from-to-www-redirect", func() {
 		f.WaitForNginxConfiguration(
 			func(cfg string) bool {
 				return strings.Contains(cfg, `server_name www.fromtowwwredirect.bar.com;`) &&
-					strings.Contains(cfg, `return 308 $scheme://fromtowwwredirect.bar.com$request_uri;`)
+					strings.Contains(cfg, `return 308 $redirect_to;`)
 			})
 
 		ginkgo.By("sending request to www.fromtowwwredirect.bar.com")
@@ -85,7 +85,7 @@ var _ = framework.DescribeAnnotation("from-to-www-redirect", func() {
 		f.WaitForNginxServer(toHost,
 			func(server string) bool {
 				return strings.Contains(server, fmt.Sprintf(`server_name %v;`, toHost)) &&
-					strings.Contains(server, fmt.Sprintf(`return 308 $scheme://%v$request_uri;`, fromHost))
+					strings.Contains(server, `return 308 $redirect_to;`)
 			})
 
 		ginkgo.By("sending request to www should redirect to domain")
@@ -98,7 +98,7 @@ var _ = framework.DescribeAnnotation("from-to-www-redirect", func() {
 			WithHeader("Host", toHost).
 			Expect().
 			Status(http.StatusPermanentRedirect).
-			Header("Location").Equal(fmt.Sprintf("https://%v/", fromHost))
+			Header("Location").Equal(fmt.Sprintf("https://%v", fromHost))
 
 		ginkgo.By("sending request to domain should not redirect to www")
 		f.HTTPTestClientWithTLSConfig(&tls.Config{

test/e2e/settings/tls.go

@@ -195,7 +195,7 @@ var _ = framework.DescribeSetting("[SSL] TLS protocols, ciphers and headers)", f
 			// we can not use gorequest here because it flattens the duplicate headers
 			// and specifically in case of Strict-Transport-Security it ignore extra headers
 			// intead of concatenating, rightfully. And I don't know of any API it provides for getting raw headers.
-			curlCmd := fmt.Sprintf("curl -I -k --fail --silent --resolve settings-tls:443:127.0.0.1 https://settings-tls/%v", "?hsts=true")
+			curlCmd := fmt.Sprintf("curl -I -k --fail --silent --resolve settings-tls:443:127.0.0.1 https://settings-tls%v", "?hsts=true")
 			output, err := f.ExecIngressPod(curlCmd)
 			assert.Nil(ginkgo.GinkgoT(), err)
 			assert.Contains(ginkgo.GinkgoT(), output, "strict-transport-security: max-age=86400; preload")
@@ -221,7 +221,7 @@ var _ = framework.DescribeSetting("[SSL] TLS protocols, ciphers and headers)", f
 				WithHeader("Host", host).
 				Expect().
 				Status(http.StatusPermanentRedirect).
-				Header("Location").Equal(fmt.Sprintf("https://%v/", host))
+				Header("Location").Equal(fmt.Sprintf("https://%v", host))
 		})
 
 		ginkgo.It("should not use ports or X-Forwarded-Host during the HTTP to HTTPS redirection", func() {
@@ -242,7 +242,7 @@ var _ = framework.DescribeSetting("[SSL] TLS protocols, ciphers and headers)", f
 				WithHeader("X-Forwarded-Host", "example.com:80").
 				Expect().
 				Status(http.StatusPermanentRedirect).
-				Header("Location").Equal("https://example.com/")
+				Header("Location").Equal("https://example.com")
 		})
 	})
 })

test/e2e/ssl/http_redirect.go

@@ -54,6 +54,6 @@ var _ = framework.IngressNginxDescribe("[SSL] redirect to HTTPS", func() {
 			WithHeader("Host", host).
 			Expect().
 			Status(http.StatusPermanentRedirect).
-			Header("Location").Equal("https://redirect.com/")
+			Header("Location").Equal("https://redirect.com")
 	})
 })