Merge pull request #1488 from tkan145/THREESCALE-10130-invalid-stale-…

…configuration-cache

[THREESCALE-10130] APIcast using stale configuration for a deleted Product

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
 

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
 

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)