[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

@@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - Fixed config reloading even when reloading is disabled [PR #1468](https://github.com/3scale/APIcast/pull/1468)
 
+- 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
 
 - Bump openresty to 1.21.4.3 [PR #1461](https://github.com/3scale/APIcast/pull/1461) [THREESCALE-10601](https://issues.redhat.com/browse/THREESCALE-10601)

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)

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'

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
 

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 = {}

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()

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)