From 4b777458981588aa90d8c7538e63e63ff70251b4 Mon Sep 17 00:00:00 2001 From: An Tran Date: Mon, 22 Jul 2024 13:42:54 +1000 Subject: [PATCH 1/2] [configuration] Reset configuration cache on reload When reloading the configuration, APIcast only updates the services that present in the new configuration and leaves the old/deleted services in the cache. When a new request is sent to the deleted service, APIcast will reuse the stale configuration in the cache, leading to unexpected behavior. This PR ensures that APIcast always resets the cache when reloading the configuration --- CHANGELOG.md | 1 + gateway/src/apicast/configuration_loader.lua | 14 +++- gateway/src/apicast/configuration_store.lua | 8 +- .../load_configuration/load_configuration.lua | 2 +- spec/configuration_loader/remote_v2_spec.lua | 1 - spec/configuration_loader_spec.lua | 18 +++++ spec/configuration_store_spec.lua | 77 +++++++++++++------ 7 files changed, 89 insertions(+), 32 deletions(-) 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..61fc23216 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,7 +235,7 @@ local function lazy_load_config(configuration, host) if not config then ngx.log(ngx.WARN, 'failed to get config for host: ', host) end - _M.configure(configuration, config) + _M.configure(configuration, config, host) end function lazy.rewrite(configuration, host) 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) From e77139d899c9c6e9743f14ee103c1204e75626ed Mon Sep 17 00:00:00 2001 From: An Tran Date: Wed, 28 Aug 2024 16:58:36 +1000 Subject: [PATCH 2/2] Don't reset the cache in lazy mode --- gateway/src/apicast/configuration_loader.lua | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gateway/src/apicast/configuration_loader.lua b/gateway/src/apicast/configuration_loader.lua index 61fc23216..0b3d5d66a 100644 --- a/gateway/src/apicast/configuration_loader.lua +++ b/gateway/src/apicast/configuration_loader.lua @@ -235,7 +235,9 @@ local function lazy_load_config(configuration, host) if not config then ngx.log(ngx.WARN, 'failed to get config for host: ', host) end - _M.configure(configuration, config, host) + -- Lazy load will never returned stale data, so no need to reset the + -- cache + _M.configure(configuration, config) end function lazy.rewrite(configuration, host)