fix(router/expressions): http->https redirection now works with ATC e…

…xpressions routes (Kong#11166)
marc-charpentier · Jul 5, 2023 · 17d3f7a · 17d3f7a
1 parent 294fa43
commit 17d3f7a
Show file tree

Hide file tree

Showing 6 changed files with 97 additions and 36 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -77,6 +77,8 @@
 - Fix an issue where the router of flavor `expressions` can not work correctly
   when `route.protocols` is set to `grpc` or `grpcs`.
   [#11082](https://github.com/Kong/kong/pull/11082)
+- Fix an issue where the router of flavor `expressions` can not configure https redirection.
+  [#11166](https://github.com/Kong/kong/pull/11166)
 
 #### Admin API
 

diff --git a/kong/router/compat.lua b/kong/router/compat.lua
@@ -304,8 +304,8 @@ end
 
 local function get_exp_and_priority(route)
   if route.expression then
-    ngx_log(ngx_ERR, "expecting a traditional route while expression is given. ",
-                 "Likely it's a misconfiguration. Please check router_flavor")
+    ngx_log(ngx_ERR, "expecting a traditional route while it's not (probably an expressions route). ",
+                     "Likely it's a misconfiguration. Please check the 'router_flavor' config in kong.conf")
   end
 
   local exp      = get_expression(route)

diff --git a/kong/router/expressions.lua b/kong/router/expressions.lua
@@ -31,7 +31,14 @@ local function get_exp_and_priority(route)
     return
   end
 
-  local gen = gen_for_field("net.protocol", OP_EQUAL, route.protocols,
+  local protocols = route.protocols
+
+  -- give the chance for http redirection (301/302/307/308/426)
+  if protocols and #protocols == 1 and protocols[1] == "https" then
+    return exp, route.priority
+  end
+
+  local gen = gen_for_field("net.protocol", OP_EQUAL, protocols,
                             function(_, p)
                               return PROTOCOLS_OVERRIDE[p] or p
                             end)

diff --git a/spec/02-integration/05-proxy/06-ssl_spec.lua b/spec/02-integration/05-proxy/06-ssl_spec.lua
@@ -2,6 +2,7 @@ local ssl_fixtures = require "spec.fixtures.ssl"
 local helpers      = require "spec.helpers"
 local cjson        = require "cjson"
 local fmt          = string.format
+local atc_compat = require "kong.router.compat"
 
 
 local function get_cert(server_name)
@@ -16,7 +17,6 @@ end
 local mock_tls_server_port = helpers.get_available_port()
 
 local fixtures = {
-  dns_mock = helpers.dns_mock.new({ mocks_only = true }),
   http_mock = {
     test_upstream_tls_server = fmt([[
       server {
@@ -34,17 +34,58 @@ local fixtures = {
   },
 }
 
-fixtures.dns_mock:A {
-  name = "example2.com",
-  address = "127.0.0.1",
-}
+local function reload_router(flavor)
+  _G.kong = {
+    configuration = {
+      router_flavor = flavor,
+    },
+  }
+
+  helpers.setenv("KONG_ROUTER_FLAVOR", flavor)
+
+  package.loaded["spec.helpers"] = nil
+  package.loaded["kong.global"] = nil
+  package.loaded["kong.cache"] = nil
+  package.loaded["kong.db"] = nil
+  package.loaded["kong.db.schema.entities.routes"] = nil
+  package.loaded["kong.db.schema.entities.routes_subschemas"] = nil
+
+  helpers = require "spec.helpers"
+
+  helpers.unsetenv("KONG_ROUTER_FLAVOR")
+
+  fixtures.dns_mock = helpers.dns_mock.new({ mocks_only = true })
+  fixtures.dns_mock:A {
+    name = "example2.com",
+    address = "127.0.0.1",
+  }
+end
+
 
-for _, flavor in ipairs({ "traditional", "traditional_compatible" }) do
+local function gen_route(flavor, r)
+  if flavor ~= "expressions" then
+    return r
+  end
+
+  r.expression = atc_compat.get_expression(r)
+  r.priority = tonumber(atc_compat._get_priority(r))
+
+  r.hosts = nil
+  r.paths = nil
+  r.snis  = nil
+
+  return r
+end
+
+
+for _, flavor in ipairs({ "traditional", "traditional_compatible", "expressions" }) do
 for _, strategy in helpers.each_strategy() do
   describe("SSL [#" .. strategy .. ", flavor = " .. flavor .. "]", function()
     local proxy_client
     local https_client
 
+    reload_router(flavor)
+
     lazy_setup(function()
       local bp = helpers.get_db_utils(strategy, {
         "routes",
@@ -57,28 +98,28 @@ for _, strategy in helpers.each_strategy() do
         name = "global-cert",
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols = { "https" },
         hosts     = { "global.com" },
         service   = service,
-      }
+      }))
 
       local service2 = bp.services:insert {
         name = "api-1",
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols = { "https" },
         hosts     = { "example.com", "ssl1.com" },
         service   = service2,
-      }
+      }))
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols = { "https" },
         hosts     = { "sni.example.com" },
         snis      = { "sni.example.com" },
         service   = service2,
-      }
+      }))
 
       local service4 = bp.services:insert {
         name     = "api-3",
@@ -87,12 +128,12 @@ for _, strategy in helpers.each_strategy() do
         port     = helpers.mock_upstream_ssl_port,
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols     = { "https" },
         hosts         = { "ssl3.com" },
         service       = service4,
         preserve_host = true,
-      }
+      }))
 
       local service5 = bp.services:insert {
         name     = "api-4",
@@ -101,12 +142,12 @@ for _, strategy in helpers.each_strategy() do
         port     = helpers.mock_upstream_ssl_port,
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols     = { "https" },
         hosts         = { "no-sni.com" },
         service       = service5,
         preserve_host = false,
-      }
+      }))
 
       local service6 = bp.services:insert {
         name     = "api-5",
@@ -115,12 +156,12 @@ for _, strategy in helpers.each_strategy() do
         port     = helpers.mock_upstream_ssl_port,
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols     = { "https" },
         hosts         = { "nil-sni.com" },
         service       = service6,
         preserve_host = false,
-      }
+      }))
 
       local service7 = bp.services:insert {
         name     = "service-7",
@@ -129,14 +170,14 @@ for _, strategy in helpers.each_strategy() do
         port     = helpers.mock_upstream_ssl_port,
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols     = { "https" },
         hosts         = { "example.com" },
         paths         = { "/redirect-301" },
         https_redirect_status_code = 301,
         service       = service7,
         preserve_host = false,
-      }
+      }))
 
       local service8 = bp.services:insert {
         name     = "service-8",
@@ -145,14 +186,14 @@ for _, strategy in helpers.each_strategy() do
         port     = helpers.mock_upstream_ssl_port,
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols     = { "https" },
         hosts         = { "example.com" },
         paths         = { "/redirect-302" },
         https_redirect_status_code = 302,
         service       = service8,
         preserve_host = false,
-      }
+      }))
 
       local service_example2 = assert(bp.services:insert {
         name     = "service-example2",
@@ -161,19 +202,19 @@ for _, strategy in helpers.each_strategy() do
         port     = mock_tls_server_port,
       })
 
-      assert(bp.routes:insert {
+      assert(bp.routes:insert(gen_route(flavor, {
         protocols     = { "http" },
         hosts         = { "example2.com" },
         paths         = { "/" },
         service       = service_example2,
-      })
+      })))
 
-      assert(bp.routes:insert {
+      assert(bp.routes:insert(gen_route(flavor, {
         protocols     = { "http" },
         hosts         = { "example-clear.com" },
         paths         = { "/" },
         service       = service8,
-      })
+      })))
 
       local cert = bp.certificates:insert {
         cert     = ssl_fixtures.cert,
@@ -555,6 +596,8 @@ for _, strategy in helpers.each_strategy() do
     end)
   end)
 
+  -- XXX: now flavor "expressions" does not support tcp/tls
+  if flavor ~= "expressions" then
   describe("TLS proxy [#" .. strategy .. ", flavor = " .. flavor .. "]", function()
     lazy_setup(function()
       local bp = helpers.get_db_utils(strategy, {
@@ -571,17 +614,17 @@ for _, strategy in helpers.each_strategy() do
         port = helpers.get_proxy_port(),
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols = { "tls" },
         snis     = { "example.com" },
         service   = service,
-      }
+      }))
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols = { "tls" },
         snis      = { "foobar.example.com." },
         service   = service,
-      }
+      }))
 
       local cert = bp.certificates:insert {
         cert     = ssl_fixtures.cert,
@@ -649,9 +692,12 @@ for _, strategy in helpers.each_strategy() do
       end)
     end)
   end)
+  end   -- if flavor ~= "expressions" then
 
   describe("SSL [#" .. strategy .. ", flavor = " .. flavor .. "]", function()
 
+    reload_router(flavor)
+
     lazy_setup(function()
       local bp = helpers.get_db_utils(strategy, {
         "routes",
@@ -664,11 +710,11 @@ for _, strategy in helpers.each_strategy() do
         name = "default-cert",
       }
 
-      bp.routes:insert {
+      bp.routes:insert(gen_route(flavor, {
         protocols = { "https" },
         hosts     = { "example.com" },
         service   = service,
-      }
+      }))
 
       local cert = bp.certificates:insert {
         cert     = ssl_fixtures.cert,
@@ -703,6 +749,8 @@ for _, strategy in helpers.each_strategy() do
   end)
 
   describe("kong.runloop.certificate invalid SNI [#" .. strategy .. ", flavor = " .. flavor .. "]", function()
+    reload_router(flavor)
+
     lazy_setup(function()
       assert(helpers.start_kong {
         router_flavor = flavor,
@@ -768,4 +816,4 @@ for _, strategy in helpers.each_strategy() do
 
   end)
 end
-end
+end   -- for flavor
diff --git a/spec/02-integration/05-proxy/19-grpc_proxy_spec.lua b/spec/02-integration/05-proxy/19-grpc_proxy_spec.lua
@@ -16,6 +16,8 @@ local function reload_router(flavor)
   helpers.setenv("KONG_ROUTER_FLAVOR", flavor)
 
   package.loaded["spec.helpers"] = nil
+  package.loaded["kong.global"] = nil
+  package.loaded["kong.cache"] = nil
   package.loaded["kong.db"] = nil
   package.loaded["kong.db.schema.entities.routes"] = nil
   package.loaded["kong.db.schema.entities.routes_subschemas"] = nil

diff --git a/spec/02-integration/05-proxy/21-grpc_plugins_triggering_spec.lua b/spec/02-integration/05-proxy/21-grpc_plugins_triggering_spec.lua
@@ -16,6 +16,8 @@ local function reload_router(flavor)
   helpers.setenv("KONG_ROUTER_FLAVOR", flavor)
 
   package.loaded["spec.helpers"] = nil
+  package.loaded["kong.global"] = nil
+  package.loaded["kong.cache"] = nil
   package.loaded["kong.db"] = nil
   package.loaded["kong.db.schema.entities.routes"] = nil
   package.loaded["kong.db.schema.entities.routes_subschemas"] = nil