[THREESCALE-2194] Ensure locks are released on configuration_loader.

CHANGELOG.md

@@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Fixed the name of the 3scale batching policy in the logs. Some logs showed "Caching policy" where it should have said "3scale Batcher" [PR #1029](https://github.com/3scale/APIcast/pull/1029)
 - Changed the schema of the IP check policy so it renders correctly in the UI [PR #1026](https://github.com/3scale/APIcast/pull/1026), [THREESCALE-1692](https://issues.jboss.org/browse/THREESCALE-1692)
 - Allow uppercase backend API in the service.[PR #1044](https://github.com/3scale/APIcast/pull/1044), [THREESCALE-2540](https://issues.jboss.org/browse/THREESCALE-2540)
+- Fixed lock issues on configuration loader when Lazy mode is enabled.[PR #1050](https://github.com/3scale/APIcast/pull/1050), [THREESCALE-2194](https://issues.jboss.org/browse/THREESCALE-2194)
 
 ### Removed
 

gateway/src/apicast/configuration_loader.lua

@@ -18,6 +18,8 @@ local assert = assert
 local pcall = pcall
 local tonumber = tonumber
 
+local lazy_load_timeout = 15
+
 local noop = function(...) return ... end
 
 local _M = {
@@ -196,33 +198,30 @@ function lazy.init(configuration)
   configuration.configured = true
 end
 
+local function lazy_load_config(configuration, host)
+    local config = _M.load(host)
+    if not config then
+      ngx.log(ngx.WARN, 'failed to get config for host: ', host)
+    end
+    _M.configure(configuration, config)
+end
+
 function lazy.rewrite(configuration, host)
   if not host then
     return nil, 'missing host'
   end
 
-  local sema = synchronization:acquire(host)
-
   if ttl() == 0 then
     configuration = configuration_store.new(configuration.cache_size)
   end
 
-  local ok, err = sema:wait(15)
-
-  if ok and not _M.configured(configuration, host) then
-    ngx.log(ngx.INFO, 'lazy loading configuration for: ', host)
-    local config = _M.load(host)
-    if not config then
-      ngx.log(ngx.WARN, 'failed to get config for host: ', host)
-    end
-    _M.configure(configuration, config)
+  if _M.configured(configuration, host) then
+    return configuration
   end
 
-  if ok then
-    synchronization:release(host)
-    sema:post()
-  else
-    ngx.log(ngx.WARN, 'failed to acquire lock to lazy load: ', host, ' error: ', err)
+  local ret, result = synchronization:run(host, lazy_load_timeout, lazy_load_config, configuration, host)
+  if not ret then
+    ngx.log(ngx.WARN, 'failed to load config for host: ', host, ' error: ', result)
   end
 
   return configuration

gateway/src/apicast/configuration_loader/remote_v2.lua

@@ -38,7 +38,6 @@ function _M.new(url, options)
   }
 
   local path = resty_url.split(endpoint or '')
-
   return setmetatable({
     endpoint = endpoint,
     path = path and path[6],

gateway/src/resty/oidc/discovery.lua

@@ -61,6 +61,12 @@ function _M:openid_configuration(issuer)
         return nil, 'no OIDC endpoint'
     end
 
+    local _, err = resty_url.parse(uri)
+    if err then
+      ngx.log(ngx.WARN, 'OIDC url is not valid, uri: "' .. uri ..'", error: ' .. err)
+      return nil, 'OIDC url is not valid, uri: "' .. uri ..'", error: ' .. err
+    end
+
     local res = http_client.get(uri)
 
     if res.status ~= 200 then

gateway/src/resty/synchronization.lua

@@ -3,6 +3,7 @@
 -- @classmod resty.synchronization
 
 local semaphore = require "ngx.semaphore"
+local safe_task = require('resty.concurrent.safe_task_executor')
 
 local rawset = rawset
 local setmetatable = setmetatable
@@ -15,7 +16,7 @@ local mt = {
 }
 --- initialize new synchronization table
 -- @tparam int size how many resources for each key
-function _M.new(_, size)
+function _M.new(size)
   local semaphore_mt = {
     __index = function(t, k)
       local sema = semaphore.new(size or 1)
@@ -25,7 +26,6 @@ function _M.new(_, size)
     end
   }
 
-
   local semaphores = setmetatable({}, semaphore_mt)
   return setmetatable({ semaphores = semaphores }, mt)
 end
@@ -53,4 +53,33 @@ function _M:release(key)
   semaphores[key] = nil
 end
 
+--- run a new function, callback using locks on the given key
+-- @tparam string key: key for the semaphore
+-- @tparam int timeout: timeout for getting the lock before raise an error.
+-- @tparam function callback: function to execute if the lock is acquired correctly
+-- @param ...: the variable number of arguments that are going to be send to the callback function.
+function _M:run(key, timeout, callback, ...)
+  local sema, err = self:acquire(key)
+  if err ~= key then
+    ngx.log(ngx.WARN, 'failed to acquire lock on key: ', key, ' error: ', err)
+    return false
+  end
+
+  local lock_acquired, acquire_err = sema:wait(timeout)
+  if not lock_acquired then
+    ngx.log(ngx.WARN, 'failed to acquire lock on key: ', key, ' error: ', acquire_err)
+    return false
+  end
+
+  local task = safe_task.new(callback)
+  local ret, result, execute_error = task:execute(...)
+
+  if lock_acquired then
+    self.release(key)
+    sema:post()
+  end
+
+  return ret, result, execute_error
+end
+
 return _M

spec/synchronization_spec.lua

@@ -0,0 +1,78 @@
+describe("Synchronization", function()
+  local synchronization
+  local key = "foo"
+
+  before_each(function()
+    synchronization = require('resty.synchronization').new(1)
+  end)
+
+  it("Valid run execution", function()
+    local callback = spy.new(function() return 1 end)
+
+    local ret, result = synchronization:run(key, 10, callback)
+    assert.same(ret, true)
+    assert.same(result, {1})
+    assert.spy(callback).was.called()
+  end)
+
+  it("Validates that run send correctly the arguments", function()
+    local callback = spy.new(function(k, v) return k, v end)
+
+    local ret, result = synchronization:run(key, 4, callback, "bob", "alice")
+    assert.same(ret, true)
+    assert.same(result, {"bob", "alice"})
+    assert.spy(callback).was_called()
+  end)
+
+  it("Validates that run does not break if a exception on the callback", function()
+    local callback = spy.new(function()
+      local data = {};
+      return data[3]
+    end)
+
+    local ret, result = synchronization:run(key, 4, callback)
+    assert.same(ret, true)
+    assert.same(result, {})
+    assert.spy(callback).was_called()
+  end)
+
+  it("Validates that run fails if key is locked and timeout", function()
+    local callback = spy.new(function() return 1 end)
+
+    local sema = synchronization:acquire(key)
+    local ok, err = sema:wait(5)
+    assert.same(ok, true)
+    assert.same(err, nil)
+
+    local ret, result = synchronization:run(key, 2, callback)
+    assert.same(ret, false)
+    assert.same(result, nil)
+    assert.spy(callback).was_not_called()
+
+    synchronization:release(key)
+    sema:post()
+  end)
+
+  it("Validate that acquire fails if not initialize", function()
+    local sync = require('resty.synchronization')
+    local sema, err = sync:acquire(key)
+    assert.same(sema, nil)
+    assert.same(err, "not initialized")
+  end)
+
+  it("validate that acquire/release workflow", function()
+
+    local sema, _ = synchronization:acquire(key)
+    assert.is.not_nil(synchronization.semaphores[key])
+    assert.is.not_nil(sema)
+
+    local ok, err = sema:wait(15)
+    assert.same(ok, true)
+    assert.same(err, nil)
+
+    ok, err = synchronization.release(key)
+    assert.same(ok, nil)
+    assert.is.not_nil(err, nil)
+  end)
+
+end)

t/configuration-loading-lazy.t

@@ -55,7 +55,8 @@ GET /t
 === TEST 3: load valid configuration
 should correctly route the request
 --- main_config
-env THREESCALE_PORTAL_ENDPOINT=http://127.0.0.1:$TEST_NGINX_SERVER_PORT;
+env THREESCALE_PORTAL_ENDPOINT=http://127.0.0.1:$TEST_NGINX_SERVER_PORT/;
+env APICAST_CONFIGURATION_LOADER=lazy;
 --- http_config
   include $TEST_NGINX_HTTP_CONFIG;
   include $TEST_NGINX_UPSTREAM_CONFIG;
@@ -95,3 +96,83 @@ GET /t?user_key=fake
   | ]
 ]
 
+=== TEST 4: load invalid json
+To validate that process does not died with invalid config
+--- main_config
+env THREESCALE_PORTAL_ENDPOINT=http://127.0.0.1:$TEST_NGINX_SERVER_PORT/;
+env APICAST_CONFIGURATION_LOADER=lazy;
+--- http_config
+  include $TEST_NGINX_UPSTREAM_CONFIG;
+  lua_package_path "$TEST_NGINX_LUA_PATH";
+--- config
+  include $TEST_NGINX_APICAST_CONFIG;
+  include $TEST_NGINX_BACKEND_CONFIG;
+
+  location = /admin/api/nginx/spec.json {
+    try_files /config.json =404;
+  }
+
+  location /api/ {
+    echo "all ok";
+  }
+--- request
+GET /t?user_key=fake
+--- error_code: 404
+--- user_files
+>>> config.json
+{Hello, world}
+--- no_error_log
+[error]
+
+=== TEST 5: load invalid oidc target url
+--- main_config
+env THREESCALE_PORTAL_ENDPOINT=http://127.0.0.1:$TEST_NGINX_SERVER_PORT/;
+env APICAST_CONFIGURATION_LOADER=lazy;
+--- http_config
+  include $TEST_NGINX_HTTP_CONFIG;
+  include $TEST_NGINX_UPSTREAM_CONFIG;
+  lua_package_path "$TEST_NGINX_LUA_PATH";
+--- config
+  include $TEST_NGINX_APICAST_CONFIG;
+  include $TEST_NGINX_BACKEND_CONFIG;
+
+  location = /admin/api/nginx/spec.json {
+    try_files /config.json =404;
+  }
+
+  location /api/ {
+    echo "all ok";
+  }
+--- request
+GET /t?user_key=fake
+--- error_code: 401
+--- user_files eval
+[
+  [ 'config.json', qq|
+  {
+    "services": [{
+      "id": 1,
+      "backend_version": 1,
+      "backend_version": "oauth",
+      "proxy": {
+        "api_backend": "http://127.0.0.1:$Test::Nginx::Util::ServerPortForClient/api/",
+        "service_id": 2555417794444,
+        "oidc_issuer_endpoint": "www.fgoodl/adasd",
+        "authentication_method": "oidc",
+        "service_backend_version": "oauth",
+        "hosts": [
+          "localhost"
+        ],
+        "backend": {
+          "endpoint": "http://127.0.0.1:$Test::Nginx::Util::ServerPortForClient"
+        },
+        "proxy_rules": [
+          { "pattern": "/t", "http_method": "GET", "metric_system_name": "test" }
+        ]
+      }
+    }]
+  }
+  | ]
+]
+--- error_log
+OIDC url is not valid, uri: