Merge pull request #3798 from ElvinEfendi/rewrite-refactoring

Move some configuration logic from Nginx config to Lua code

internal/ingress/controller/template/template.go

@@ -132,6 +132,8 @@ var (
 		"buildRateLimitZones":        buildRateLimitZones,
 		"buildRateLimit":             buildRateLimit,
 		"buildResolversForLua":       buildResolversForLua,
+		"configForLua":               configForLua,
+		"locationConfigForLua":       locationConfigForLua,
 		"buildResolvers":             buildResolvers,
 		"buildUpstreamName":          buildUpstreamName,
 		"isLocationInLocationList":   isLocationInLocationList,
@@ -263,6 +265,51 @@ func buildResolversForLua(res interface{}, disableIpv6 interface{}) string {
 	return strings.Join(r, ", ")
 }
 
+// configForLua returns some general configuration as Lua table represented as string
+func configForLua(input interface{}) string {
+	all, ok := input.(config.TemplateConfig)
+	if !ok {
+		klog.Errorf("expected a 'config.TemplateConfig' type but %T was given", input)
+		return "{}"
+	}
+
+	return fmt.Sprintf(`{
+		use_forwarded_headers = %t,
+		is_ssl_passthrough_enabled = %t,
+		http_redirect_code = %v,
+		listen_ports = { ssl_proxy = "%v", https = "%v" },
+	}`, all.Cfg.UseForwardedHeaders, all.IsSSLPassthroughEnabled, all.Cfg.HTTPRedirectCode, all.ListenPorts.SSLProxy, all.ListenPorts.HTTPS)
+}
+
+// locationConfigForLua formats some location specific configuration into Lua table represented as string
+func locationConfigForLua(l interface{}, s interface{}, a interface{}) string {
+	location, ok := l.(*ingress.Location)
+	if !ok {
+		klog.Errorf("expected an '*ingress.Location' type but %T was given", l)
+		return "{}"
+	}
+
+	server, ok := s.(*ingress.Server)
+	if !ok {
+		klog.Errorf("expected an '*ingress.Server' type but %T was given", s)
+		return "{}"
+	}
+
+	all, ok := a.(config.TemplateConfig)
+	if !ok {
+		klog.Errorf("expected a 'config.TemplateConfig' type but %T was given", a)
+		return "{}"
+	}
+
+	forceSSLRedirect := location.Rewrite.ForceSSLRedirect || (len(server.SSLCert.PemFileName) > 0 && location.Rewrite.SSLRedirect)
+	forceSSLRedirect = forceSSLRedirect && !isLocationInLocationList(l, all.Cfg.NoTLSRedirectLocations)
+
+	return fmt.Sprintf(`{
+		force_ssl_redirect = %t,
+		use_port_in_redirects = %t,
+	}`, forceSSLRedirect, location.UsePortInRedirects)
+}
+
 // buildResolvers returns the resolvers reading the /etc/resolv.conf file
 func buildResolvers(res interface{}, disableIpv6 interface{}) string {
 	// NGINX need IPV6 addresses to be surrounded by brackets

rootfs/etc/nginx/lua/lua_ingress.lua

@@ -1,7 +1,14 @@
+local ngx_re_split = require("ngx.re").split
+
+local original_randomseed = math.randomseed
+local string_format = string.format
+local ngx_redirect = ngx.redirect
+
 local _M = {}
 
 local seeds = {}
-local original_randomseed = math.randomseed
+-- general Nginx configuration passed by controller to be used in this module
+local config
 
 local function get_seed_from_urandom()
   local seed
@@ -47,8 +54,73 @@ local function randomseed()
   math.randomseed(seed)
 end
 
+local function redirect_to_https()
+  return ngx.var.pass_access_scheme == "http" and (ngx.var.scheme == "http" or ngx.var.scheme == "https")
+end
+
+local function redirect_host()
+  local host_port, err = ngx_re_split(ngx.var.best_http_host, ":")
+  if err then
+    ngx.log(ngx.ERR, "could not parse variable: ", err)
+    return ngx.var.best_http_host;
+  end
+
+  return host_port[1];
+end
+
 function _M.init_worker()
   randomseed()
 end
 
+function _M.set_config(new_config)
+  config = new_config
+end
+
+-- rewrite gets called in every location context.
+-- This is where we do variable assignments to be used in subsequent
+-- phases or redirection
+function _M.rewrite(location_config)
+  ngx.var.pass_access_scheme = ngx.var.scheme
+  ngx.var.pass_server_port = ngx.var.server_port
+  ngx.var.best_http_host = ngx.var.http_host or ngx.var.host
+
+  if config.use_forwarded_headers then
+    -- trust http_x_forwarded_proto headers correctly indicate ssl offloading
+    if ngx.var.http_x_forwarded_proto then
+      ngx.var.pass_access_scheme = ngx.var.http_x_forwarded_proto
+    end
+
+    if ngx.var.http_x_forwarded_port then
+      ngx.var.pass_server_port = ngx.var.http_x_forwarded_port
+    end
+
+    -- Obtain best http host
+    if ngx.var.http_x_forwarded_host then
+      -- TODO(elvinefendi) https://github.com/kubernetes/ingress-nginx/issues/3790 can
+      -- be fixed here by splitting the value of ngx.var.http_x_forwarded_host by ','
+      -- and taking the first portion
+      ngx.var.best_http_host = ngx.var.http_x_forwarded_host
+    end
+  end
+
+  ngx.var.pass_port = ngx.var.pass_server_port
+  if config.is_ssl_passthrough_enabled then
+    if ngx.var.pass_server_port == config.listen_ports.ssl_proxy then
+      ngx.var.pass_port = 443
+    end
+  elseif ngx.var.pass_server_port == config.listen_ports.https then
+    ngx.var.pass_port = 443
+  end
+
+  if location_config.force_ssl_redirect and redirect_to_https() then
+    local uri = string_format("https://%s%s", redirect_host(), ngx.var.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)
+    end
+
+    ngx_redirect(uri, config.http_redirect_code)
+  end
+end
+
 return _M

rootfs/etc/nginx/template/nginx.tmpl

@@ -66,6 +66,7 @@ http {
           error("require failed: " .. tostring(res))
         else
           lua_ingress = res
+          lua_ingress.set_config({{ configForLua $all }})
         end
 
         ok, res = pcall(require, "configuration")
@@ -293,64 +294,6 @@ http {
     {{ end }}
     }
 
-    {{ if $cfg.UseForwardedHeaders }}
-    # trust http_x_forwarded_proto headers correctly indicate ssl offloading
-    map $http_x_forwarded_proto $pass_access_scheme {
-        default          $http_x_forwarded_proto;
-        ''               $scheme;
-    }
-
-    map $http_x_forwarded_port $pass_server_port {
-        default           $http_x_forwarded_port;
-        ''                $server_port;
-    }
-
-    # Obtain best http host
-    map $http_host $this_host {
-        default          $http_host;
-        ''               $host;
-    }
-
-    map $http_x_forwarded_host $best_http_host {
-        default          $http_x_forwarded_host;
-        ''               $this_host;
-    }
-    {{ else }}
-    map '' $pass_access_scheme {
-        default          $scheme;
-    }
-
-    map '' $pass_server_port {
-        default          $server_port;
-    }
-
-    # Obtain best http host
-    map $http_host $best_http_host {
-        default          $http_host;
-        ''               $host;
-    }
-    {{ end }}
-
-    # validate $pass_access_scheme and $scheme are http to force a redirect
-    map "$scheme:$pass_access_scheme" $redirect_to_https {
-        default          0;
-        "http:http"      1;
-        "https:http"     1;
-    }
-
-    {{ if $all.IsSSLPassthroughEnabled }}
-    # map port {{ $all.ListenPorts.SSLProxy }} to 443 for header X-Forwarded-Port
-    map $pass_server_port $pass_port {
-        {{ $all.ListenPorts.SSLProxy }}              443;
-        default          $pass_server_port;
-    }
-    {{ else }}
-    map $pass_server_port $pass_port {
-        {{ $all.ListenPorts.HTTPS }}              443;
-        default          $pass_server_port;
-    }
-    {{ end }}
-
     # Reverse proxies can detect if a client provides a X-Request-ID header, and pass it on to the backend server.
     # If no such header is provided, it can provide a random value.
     map $http_x_request_id $req_id {
@@ -871,6 +814,10 @@ stream {
         {{ end }}
         {{ end }}
         set $proxy_upstream_name "-";
+        set $pass_access_scheme $scheme;
+        set $pass_server_port $server_port;
+        set $best_http_host $http_host;
+        set $pass_port $pass_server_port;
 
         {{/* Listen on {{ $all.ListenPorts.SSLProxy }} because port {{ $all.ListenPorts.HTTPS }} is used in the TLS sni server */}}
         {{/* This listener must always have proxy_protocol enabled, because the SNI listener forwards on source IP info in it. */}}
@@ -1027,6 +974,7 @@ stream {
             {{ end }}
 
             rewrite_by_lua_block {
+                lua_ingress.rewrite({{ locationConfigForLua $location $server $all }})
                 balancer.rewrite()
             }
 
@@ -1127,35 +1075,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) {
-                set_by_lua_block $redirect_host {
-                    local ngx_re = require "ngx.re"
-
-                    local host_port, err = ngx_re.split(ngx.var.best_http_host, ":")
-                    if err then
-                        ngx.log(ngx.ERR, "could not parse variable: ", err)
-                        return ngx.var.best_http_host;
-                    end
-
-                    return host_port[1];
-                }
-
-                {{ if $location.UsePortInRedirects }}
-                # using custom ports require a different rewrite directive
-                # https://forum.nginx.org/read.php?2,155978,155978#msg-155978
-                error_page 497 ={{ $all.Cfg.HTTPRedirectCode }} https://$redirect_host{{ printf ":%v" $all.ListenPorts.HTTPS }}$request_uri;
-                return 497;
-                {{ else }}
-                return {{ $all.Cfg.HTTPRedirectCode }} https://$redirect_host$request_uri;
-                {{ end }}
-            }
-            {{ end }}
-            {{ end }}
-
             {{ if (or $location.ModSecurity.Enable $all.Cfg.EnableModsecurity) }}
             modsecurity on;
 

test/e2e/annotations/forcesslredirect.go

@@ -46,12 +46,6 @@ var _ = framework.IngressNginxDescribe("Annotations - Forcesslredirect", func()
 		ing := framework.NewSingleIngress(host, "/", host, f.Namespace, "http-svc", 80, &annotations)
 		f.EnsureIngress(ing)
 
-		f.WaitForNginxServer(host,
-			func(server string) bool {
-				return Expect(server).Should(ContainSubstring(`if ($redirect_to_https) {`)) &&
-					Expect(server).Should(ContainSubstring(`return 308 https://$redirect_host$request_uri;`))
-			})
-
 		resp, _, errs := gorequest.New().
 			Get(f.GetURL(framework.HTTP)).
 			Retry(10, 1*time.Second, http.StatusNotFound).

test/e2e/settings/tls.go

@@ -181,12 +181,6 @@ var _ = framework.IngressNginxDescribe("Settings - TLS)", func() {
 
 		framework.WaitForTLS(f.GetURL(framework.HTTPS), tlsConfig)
 
-		f.WaitForNginxServer(host,
-			func(server string) bool {
-				return Expect(server).Should(ContainSubstring(`if ($redirect_to_https) {`)) &&
-					Expect(server).Should(ContainSubstring(`return 308 https://$redirect_host$request_uri;`))
-			})
-
 		resp, _, errs := gorequest.New().
 			Get(fmt.Sprintf(f.GetURL(framework.HTTP))).
 			Retry(10, 1*time.Second, http.StatusNotFound).
@@ -211,12 +205,6 @@ var _ = framework.IngressNginxDescribe("Settings - TLS)", func() {
 
 		framework.WaitForTLS(f.GetURL(framework.HTTPS), tlsConfig)
 
-		f.WaitForNginxServer(host,
-			func(server string) bool {
-				return Expect(server).Should(ContainSubstring(`if ($redirect_to_https) {`)) &&
-					Expect(server).Should(ContainSubstring(`return 308 https://$redirect_host$request_uri;`))
-			})
-
 		resp, _, errs := gorequest.New().
 			Get(fmt.Sprintf(f.GetURL(framework.HTTP))).
 			Retry(10, 1*time.Second, http.StatusNotFound).