fix(router/atc): properly handle expressions building error

CHANGELOG.md

@@ -78,6 +78,9 @@
 - Fix issue in router building where when field contains an empty table,
   the generated expression is invalid.
   [#9451](https://github.com/Kong/kong/pull/9451)
+- Fix issue in router rebuilding where when paths field is invalid,
+  the router's mutex is not released properly.
+  [#9480](https://github.com/Kong/kong/pull/9480)
 
 ## [3.0.0]
 

kong/router/atc_compat.lua

@@ -37,6 +37,7 @@ local ngx_log       = ngx.log
 local get_method    = ngx.req.get_method
 local get_headers   = ngx.req.get_headers
 local ngx_WARN      = ngx.WARN
+local ngx_ERR       = ngx.ERR
 
 
 local sanitize_uri_postfix = utils.sanitize_uri_postfix
@@ -398,7 +399,7 @@ local function add_atc_matcher(inst, route, route_id,
   else
     atc = route.expression
     if not atc then
-      return
+      return nil, "could not find route expression"
     end
 
     priority = route.priority
@@ -414,7 +415,12 @@ local function add_atc_matcher(inst, route, route_id,
     assert(inst:remove_matcher(route_id))
   end
 
-  assert(inst:add_matcher(priority, route_id, atc))
+  local ok, err = inst:add_matcher(priority, route_id, atc)
+  if not ok then
+    return nil, "could not add route: " .. route_id .. ", err: " .. err
+  end
+
+  return true
 end
 
 
@@ -439,9 +445,17 @@ local function new_from_scratch(routes, is_traditional_compatible)
     routes_t[route_id] = route
     services_t[route_id] = r.service
 
-    add_atc_matcher(inst, route, route_id, is_traditional_compatible, false)
+    local ok, err = add_atc_matcher(inst, route, route_id,
+                                    is_traditional_compatible, false)
+    if ok then
+      new_updated_at = max(new_updated_at, route.updated_at or 0)
 
-    new_updated_at = max(new_updated_at, route.updated_at or 0)
+    else
+      ngx_log(ngx_ERR, err)
+
+      routes_t[route_id] = nil
+      services_t[route_id] = nil
+    end
 
     yield(true)
   end
@@ -489,16 +503,27 @@ local function new_from_previous(routes, is_traditional_compatible, old_router)
     old_routes[route_id] = route
     old_services[route_id] = r.service
 
+    local ok = true
+    local err
+
     if not old_route then
       -- route is new
-      add_atc_matcher(inst, route, route_id, is_traditional_compatible, false)
+      ok, err = add_atc_matcher(inst, route, route_id, is_traditional_compatible, false)
 
     elseif route_updated_at >= updated_at or route_updated_at ~= old_route.updated_at then
       -- route is modified (within a sec)
-      add_atc_matcher(inst, route, route_id, is_traditional_compatible, true)
+      ok, err = add_atc_matcher(inst, route, route_id, is_traditional_compatible, true)
     end
 
-    new_updated_at = max(new_updated_at, route_updated_at)
+    if ok then
+      new_updated_at = max(new_updated_at, route_updated_at)
+
+    else
+      ngx_log(ngx_ERR, err)
+
+      old_routes[route_id] = nil
+      old_services[route_id] = nil
+    end
 
     yield(true)
   end

spec/01-unit/08-router_spec.lua

@@ -1753,6 +1753,25 @@ for _, flavor in ipairs({ "traditional", "traditional_compatible", "expressions"
             assert.falsy(match_t)
           end)
 
+          it("update with wrong route", function()
+            local use_case = {
+              {
+                service = service,
+                route = {
+                  id = "e8fb37f1-102d-461e-9c51-6608a6bb8101",
+                  paths = { "~/delay/(?<delay>[^\\/]+)$", },
+                  updated_at = 100,
+                },
+              },
+            }
+
+            local ok, nrouter = pcall(new_router, use_case, router)
+
+            assert(ok)
+            assert.equal(nrouter, router)
+            assert.equal(#nrouter.routes, 0)
+          end)
+
           it("update skips routes if updated_at is unchanged", function()
             local use_case = {
               {
@@ -1812,7 +1831,6 @@ for _, flavor in ipairs({ "traditional", "traditional_compatible", "expressions"
               }
             }
 
-
             local nrouter = assert(new_router(use_case, router))
 
             assert.equal(nrouter, router)

spec/02-integration/05-proxy/02-router_spec.lua

@@ -2335,6 +2335,46 @@ for _, strategy in helpers.each_strategy() do
       local json = cjson.decode(body)
       assert.equal("no Service found with those values", json.message)
     end)
+
+    it("#db rebuilds router correctly after passing invalid route", function()
+      local admin_client = helpers.admin_client()
+
+      local res = assert(admin_client:post("/routes", {
+        headers = { ["Content-Type"] = "application/json" },
+        body = {
+          -- this is a invalid regex path
+          paths = { "~/delay/(?<delay>[^\\/]+)$", },
+        },
+      }))
+      assert.res_status(201, res)
+
+      helpers.wait_for_all_config_update()
+
+      local res = assert(admin_client:post("/routes", {
+        headers = { ["Content-Type"] = "application/json" },
+        body = {
+          paths = { "/foo" },
+        },
+      }))
+      assert.res_status(201, res)
+
+      admin_client:close()
+
+      helpers.wait_for_all_config_update()
+
+      proxy_client:close()
+      proxy_client = helpers.proxy_client()
+
+      local res = assert(proxy_client:send {
+        method  = "GET",
+        path    = "/foo",
+        headers = { ["kong-debug"] = 1 },
+      })
+
+      local body = assert.response(res).has_status(503)
+      local json = cjson.decode(body)
+      assert.equal("no Service found with those values", json.message)
+    end)
   end)
 end
 end