diff --git a/CHANGELOG.md b/CHANGELOG.md index 1288bff4f..c5356f746 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed confusing log display when APIcast listens on HTTPS and path routing is enabled [PR #1486](https://github.com/3scale/APIcast/pull/1486/files) [THREESCALE #8486](https://issues.redhat.com/browse/THREESCALE-8486) - Fixed Conditional policy evaluating incorrectly: second policy in policy chain that implement export() always triggers [PR #1485](https://github.com/3scale/APIcast/pull/1485) [THREESCALE-9320](https://issues.redhat.com/browse/THREESCALE-9320) +- Fix APIcast using stale configuration for deleted products [PR #1488](https://github.com/3scale/APIcast/pull/1488) [THREESCALE-10130](https://issues.redhat.com/browse/THREESCALE-10130) ### Added diff --git a/gateway/src/apicast/configuration_loader.lua b/gateway/src/apicast/configuration_loader.lua index 9f4fb7470..0b3d5d66a 100644 --- a/gateway/src/apicast/configuration_loader.lua +++ b/gateway/src/apicast/configuration_loader.lua @@ -1,4 +1,4 @@ -local configuration_store = require('apicast.configuration_store') +local configuration_store = require 'apicast.configuration_store' local configuration_parser = require 'apicast.configuration_parser' local mock_loader = require 'apicast.configuration_loader.mock' local file_loader = require 'apicast.configuration_loader.file' @@ -75,7 +75,7 @@ function _M.global(contents) return _M.configure(context.configuration, contents) end -function _M.configure(configuration, contents) +function _M.configure(configuration, contents, reset_cache) if not configuration then return nil, 'not initialized' end @@ -90,6 +90,12 @@ function _M.configure(configuration, contents) end if config then + -- We have the configuration available at this point so it's safe to purge the + -- cache and remove old items (deleted services) + if reset_cache then + ngx.log(ngx.DEBUG, "flushing caches as part of the configuration reload") + configuration:reset() + end configuration:store(config, ttl()) collectgarbage() return config @@ -160,7 +166,7 @@ end local function refresh_configuration(configuration) local config = _M.load() - local init, err = _M.configure(configuration, config) + local init, err = _M.configure(configuration, config, true) if init then ngx.log(ngx.DEBUG, 'updated configuration via timer: ', config) @@ -229,6 +235,8 @@ local function lazy_load_config(configuration, host) if not config then ngx.log(ngx.WARN, 'failed to get config for host: ', host) end + -- Lazy load will never returned stale data, so no need to reset the + -- cache _M.configure(configuration, config) end diff --git a/gateway/src/apicast/configuration_store.lua b/gateway/src/apicast/configuration_store.lua index 5e8409c16..b85ac7732 100644 --- a/gateway/src/apicast/configuration_store.lua +++ b/gateway/src/apicast/configuration_store.lua @@ -165,16 +165,18 @@ function _M.store(self, config, ttl) return config end -function _M.reset(self, cache_size) +--- Flush all LRU cache +function _M.reset(self) if not self.services then return nil, 'not initialized' end - self.services = lrucache.new(cache_size or _M.cache_size) - self.cache = lrucache.new(cache_size or _M.cache_size) + self.services:flush_all() + self.cache:flush_all() self.configured = false end + function _M.add(self, service, ttl) if not self.services then return nil, 'not initialized' diff --git a/gateway/src/apicast/policy/load_configuration/load_configuration.lua b/gateway/src/apicast/policy/load_configuration/load_configuration.lua index 481e2ffbd..317a1212b 100644 --- a/gateway/src/apicast/policy/load_configuration/load_configuration.lua +++ b/gateway/src/apicast/policy/load_configuration/load_configuration.lua @@ -2,7 +2,7 @@ local _M = require('apicast.policy').new('Load Configuration') local ssl = require('ngx.ssl') local configuration_loader = require('apicast.configuration_loader').new() -local configuration_store = require('apicast.configuration_store', 'builtin') +local configuration_store = require('apicast.configuration_store') local new = _M.new diff --git a/spec/configuration_loader/remote_v2_spec.lua b/spec/configuration_loader/remote_v2_spec.lua index e8cfd14c2..7854f9599 100644 --- a/spec/configuration_loader/remote_v2_spec.lua +++ b/spec/configuration_loader/remote_v2_spec.lua @@ -3,7 +3,6 @@ local test_backend_client = require 'resty.http_ng.backend.test' local cjson = require 'cjson' local user_agent = require 'apicast.user_agent' local env = require 'resty.env' -local format = string.format local service_generator = function(n) local services = {} diff --git a/spec/configuration_loader_spec.lua b/spec/configuration_loader_spec.lua index 39b6f7894..6ab732e75 100644 --- a/spec/configuration_loader_spec.lua +++ b/spec/configuration_loader_spec.lua @@ -90,6 +90,24 @@ insulate('Configuration object', function() assert.truthy(config:find_by_id('42')) end) + + it('should reset cache', function() + local config = configuration_store.new() + + assert.truthy(_M.configure(config, cjson.encode({ services = { + { id = 42, proxy = { hosts = { 'localhost' } } }, + { id = 43, proxy = { hosts = { 'localhost' } } } + }}))) + + assert.truthy(config:find_by_id('42')) + assert.truthy(config:find_by_id('43')) + + assert.truthy(_M.configure(config, cjson.encode({ services = { + { id = 42, proxy = { hosts = { 'localhost' } } }, + }}), true)) + assert.truthy(config:find_by_id('42')) + assert.falsy(config:find_by_id('43')) + end) end) describe('lazy loader', function() diff --git a/spec/configuration_store_spec.lua b/spec/configuration_store_spec.lua index 9fc7b3280..1051a2fb6 100644 --- a/spec/configuration_store_spec.lua +++ b/spec/configuration_store_spec.lua @@ -2,6 +2,35 @@ local configuration = require('apicast.configuration_store') local tablex = require("pl.tablex") describe('Configuration Store', function() + describe('.new', function() + it('should not store more than the size of the services cache', function() + local service1 = { id = '42', hosts = { 'example1.com' } } + local service2 = { id = '43', hosts = { 'example2.com' } } + + local store = configuration.new(1) + + -- Add 2, as size is 1 only the second will be kept. + store:add(service1) + store:add(service2) + + assert.is_nil(store:find_by_id('42')) + assert.equal(service2, store:find_by_id('43')) + end) + + it('should not store mote then the size of the hosts cache', function() + local service1 = { id = '42', hosts = { 'example1.com' } } + local service2 = { id = '43', hosts = { 'example2.com' } } + + local store = configuration.new(1) + + -- Add 2, as size is 1 only the second will be kept. + store:add(service1) + store:add(service2) + + assert.same({}, store:find_by_host('example1.com'), false) + assert.same({ service2 }, store:find_by_host('example2.com'), false) + end) + end) describe('.store', function() it('stores configuration', function() @@ -197,48 +226,50 @@ describe('Configuration Store', function() assert.same({}, store.cache.hasht) end) - it('deletes all services', function() - store.services['42'] = {} + it('should not serve old hosts', function() + local service1 = { id = '41', hosts = { 'example1.com' } } + local service2 = { id = '42', hosts = { 'example2.com' } } + local service3 = { id = '43', hosts = { 'example3.com' } } + store:add(service1) + store:add(service2) store:reset() + store:add(service3) - assert.same({}, store.services.hasht) + assert.same({}, store:find_by_host('example1.com'), false) + assert.same({}, store:find_by_host('example2.com'), false) + assert.same({ service3 }, store:find_by_host('example3.com'), false) end) - it('sets configured flag', function() - store.configured = true + it('deletes all services', function() + store.services['42'] = {} store:reset() - assert.falsy(store.configured) + assert.same({}, store.services.hasht) end) - it('resets de size of the services cache', function() - local service1 = { id = '42', hosts = { 'example1.com' } } - local service2 = { id = '43', hosts = { 'example2.com' } } - - store:reset(1) + it('should not serve old services', function() + local service1 = { id = '41', hosts = { 'example1.com' } } + local service2 = { id = '42', hosts = { 'example2.com' } } + local service3 = { id = '43', hosts = { 'example3.com' } } - -- Add 2, as size is 1 only the second will be kept. store:add(service1) store:add(service2) + store:reset() + store:add(service3) + assert.is_nil(store:find_by_id('41')) assert.is_nil(store:find_by_id('42')) - assert.equal(service2, store:find_by_id('43')) + assert.equal(service3, store:find_by_id('43')) end) - it('resets de size of the hosts cache', function() - local service1 = { id = '42', hosts = { 'example1.com' } } - local service2 = { id = '43', hosts = { 'example2.com' } } - - store:reset(1) + it('sets configured flag', function() + store.configured = true - -- Add 2, as size is 1 only the second will be kept. - store:add(service1) - store:add(service2) + store:reset() - assert.same({}, store:find_by_host('example1.com'), false) - assert.same({ service2 }, store:find_by_host('example2.com'), false) + assert.falsy(store.configured) end) end) end)