From a35a1ac11b5060c4f6a21328dabdd0094b7d5c43 Mon Sep 17 00:00:00 2001 From: Michal Cichra Date: Fri, 24 Nov 2017 14:27:02 +0100 Subject: [PATCH 1/3] [spec] cleanup policy chain spec * 'require' calls are cached, so there can be just one --- spec/policy_chain_spec.lua | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/spec/policy_chain_spec.lua b/spec/policy_chain_spec.lua index 1aca127a3..8feb35ea5 100644 --- a/spec/policy_chain_spec.lua +++ b/spec/policy_chain_spec.lua @@ -1,4 +1,5 @@ local policy = require 'apicast.policy' +local _M = require 'apicast.policy_chain' describe('policy_chain', function() local phases = { @@ -9,10 +10,8 @@ describe('policy_chain', function() } it('defines a method for each of the nginx phases supported', function() - local policy_chain = require 'apicast.policy_chain' - for _, phase in ipairs(phases) do - assert.equals('function', type(policy_chain[phase])) + assert.equals('function', type(_M[phase])) end end) @@ -32,8 +31,7 @@ describe('policy_chain', function() stub(custom_policy_2, 'access') -- Build the policy chain - local policy_chain = require 'apicast.policy_chain' - local chain = policy_chain.build({ custom_policy_1, custom_policy_2 }) + local chain = _M.build({ custom_policy_1, custom_policy_2 }) chain:init() assert.stub(custom_policy_1.init).was_called() @@ -48,7 +46,6 @@ describe('policy_chain', function() it('uses APIcast as default when no policies are specified', function() local apicast = require 'apicast.policy.apicast' - local policy_chain = require 'apicast.policy_chain' -- Stub apicast methods to avoid calling them. We are just interested in -- knowing whether they were called. @@ -57,7 +54,7 @@ describe('policy_chain', function() end for _, phase in ipairs(phases) do - policy_chain[phase](policy_chain) + _M[phase](_M) assert.stub(apicast[phase]).was_called() end end) @@ -74,29 +71,24 @@ describe('policy_chain', function() end end - local policy_chain = require 'apicast.policy_chain' local sorted_policies = { policies[2], policies[3], policies[1] } - local chain = policy_chain.build(sorted_policies) + local chain = _M.build(sorted_policies) chain:init() assert.same({'2', '3', '1'}, execution_order) end) it('does not allow to modify phase methods after the chain has been built', function() - local policy_chain = require 'apicast.policy_chain' - for _, phase in ipairs(phases) do assert.has_error(function() - policy_chain[phase] = function() end + _M[phase] = function() end end, 'readonly table') end end) it('does not allow to add new methods to the chain after it has been built', function() - local policy_chain = require 'apicast.policy_chain' - assert.has_error(function() - policy_chain['new_phase'] = function() end + _M['new_phase'] = function() end end, 'readonly table') end) @@ -108,8 +100,7 @@ describe('policy_chain', function() local policy_2 = policy.new('2') policy_2.export = function() return { shared_data_2 = '2' } end - local policy_chain = require 'apicast.policy_chain' - local chain = policy_chain.build({ policy_1, policy_2 }) + local chain = _M.build({ policy_1, policy_2 }) local shared_data = chain:export() assert.equal('1', shared_data['shared_data_1']) @@ -120,8 +111,7 @@ describe('policy_chain', function() local policy_1 = policy.new('1') policy_1.export = function() return { shared_data = '1' } end - local policy_chain = require 'apicast.policy_chain' - local chain = policy_chain.build({ policy_1 }) + local chain = _M.build({ policy_1 }) local shared_data = chain:export() @@ -139,8 +129,7 @@ describe('policy_chain', function() local policy_2 = policy.new('custom_authorizer') policy_2.export = function() return { shared_data_1 = '2' } end - local policy_chain = require 'apicast.policy_chain' - local chain = policy_chain.build({ policy_1, policy_2 }) + local chain = _M.build({ policy_1, policy_2 }) local shared_data = chain:export() assert.equal('1', shared_data['shared_data_1']) From a0e44aa2e3f860e6d34ddd3309114092a8a5fa40 Mon Sep 17 00:00:00 2001 From: Michal Cichra Date: Fri, 24 Nov 2017 14:30:02 +0100 Subject: [PATCH 2/3] [policy_chain] allow inserting policies to the chain * freeze the chain when passed to the executor * after freezing not possible to add policies * provide default policy chain * ability to insert policy to a chain --- CHANGELOG.md | 1 + gateway/src/apicast/executor.lua | 18 +++--------- gateway/src/apicast/policy_chain.lua | 40 ++++++++++++++++++++++--- spec/executor_spec.lua | 13 ++++++-- spec/policy_chain_spec.lua | 44 ++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e27300571..09c14661b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - New policy chains system. This allows users to write custom policies to configure what Apicast can do on each of the Nginx phases [PR #450](https://github.com/3scale/apicast/pull/450) - Resolver can resolve nginx upstreams [PR #478](https://github.com/3scale/apicast/pull/478) - Calls 3scale backend with the 'no_body' option enabled. This reduces network traffic in cases where APIcast does not need to parse the response body [PR #483](https://github.com/3scale/apicast/pull/483) +- Methods to modify policy chains [PR #505](https://github.com/3scale/apicast/pull/505) ## Changed diff --git a/gateway/src/apicast/executor.lua b/gateway/src/apicast/executor.lua index 99dc23b29..126da45de 100644 --- a/gateway/src/apicast/executor.lua +++ b/gateway/src/apicast/executor.lua @@ -6,7 +6,7 @@ require('apicast.loader') -- to load code from deprecated paths -local policy_chain = require('apicast.policy_chain') +local PolicyChain = require('apicast.policy_chain') local policy = require('apicast.policy') local linked_list = require('apicast.linked_list') @@ -14,12 +14,6 @@ local setmetatable = setmetatable local _M = { } -local DEFAULT_POLICIES = { - 'apicast.policy.load_configuration', - 'apicast.policy.find_service', - 'apicast.policy.local_chain' -} - local mt = { __index = _M } -- forward all policy methods to the policy chain @@ -29,12 +23,8 @@ for _,phase in policy.phases() do end end -local function global_chain() - return policy_chain.build(DEFAULT_POLICIES) -end - -function _M.new() - return setmetatable({ policy_chain = global_chain() }, mt) +function _M.new(policy_chain) + return setmetatable({ policy_chain = policy_chain:freeze() }, mt) end local function build_context(executor) @@ -67,4 +57,4 @@ function _M:context(phase) return shared_build_context(self) end -return _M.new() +return _M.new(PolicyChain.default()) diff --git a/gateway/src/apicast/policy_chain.lua b/gateway/src/apicast/policy_chain.lua index 0462b25f5..7ca2f1aa0 100644 --- a/gateway/src/apicast/policy_chain.lua +++ b/gateway/src/apicast/policy_chain.lua @@ -10,10 +10,11 @@ local error = error local rawset = rawset local type = type local require = require +local insert = table.insert local noop = function() end local linked_list = require('apicast.linked_list') -local policy = require('apicast.policy') +local policy_phases = require('apicast.policy').phases local _M = { @@ -49,6 +50,19 @@ function _M.build(modules) return _M.new(chain) end + +local DEFAULT_POLICIES = { + 'apicast.policy.load_configuration', + 'apicast.policy.find_service', + 'apicast.policy.local_chain' +} + +--- Return new policy chain with default policies. +-- @treturn PolicyChain +function _M.default() + return _M.build(DEFAULT_POLICIES) +end + --- Load a module -- If the module is a string, returns the result of initializing it with the -- given arguments. Otherwise, this function simply returns the module @@ -76,7 +90,6 @@ function _M.new(list) local self = setmetatable(chain, mt) chain.config = self:export() - self.frozen = true return self end @@ -96,6 +109,25 @@ function _M:export() return chain end +--- Freeze the policy chain to prevent modifications. +-- After calling this method it won't be possible to insert more policies. +-- @treturn self +function _M:freeze() + self.frozen = true + return self +end + +--- Insert a policy into the chain +-- @tparam Policy policy the policy to be added to the chain +-- @tparam[opt] int position the position to add the policy to, defaults to last one +function _M:insert(policy, position) + if self.frozen then + return nil, 'frozen chain' + else + insert(self, position or #self+1, policy) + end +end + local function call_chain(phase_name) return function(self, ...) for i=1, #self do @@ -104,8 +136,8 @@ local function call_chain(phase_name) end end -for _,phase in policy.phases() do +for _,phase in policy_phases() do _M[phase] = call_chain(phase) end -return _M.build() +return _M.build():freeze() diff --git a/spec/executor_spec.lua b/spec/executor_spec.lua index 2c744dcba..409909e70 100644 --- a/spec/executor_spec.lua +++ b/spec/executor_spec.lua @@ -1,3 +1,6 @@ +local executor = require 'apicast.executor' +local policy_chain = require 'apicast.policy_chain' + describe('executor', function() local phases = { 'init', 'init_worker', @@ -7,8 +10,6 @@ describe('executor', function() } it('forwards all the policy methods to the policy chain', function() - local executor = require 'apicast.executor' - -- Policies included by default in the executor local default_executor_chain = { require 'apicast.policy.load_configuration', @@ -32,4 +33,12 @@ describe('executor', function() end end end) + + it('freezes the policy chain', function() + local chain = policy_chain.new({}) + assert.falsy(chain.frozen) + + executor.new(chain) + assert.truthy(chain.frozen) + end) end) diff --git a/spec/policy_chain_spec.lua b/spec/policy_chain_spec.lua index 8feb35ea5..0ed0cd69f 100644 --- a/spec/policy_chain_spec.lua +++ b/spec/policy_chain_spec.lua @@ -92,6 +92,38 @@ describe('policy_chain', function() end, 'readonly table') end) + describe('.insert', function() + + it('adds policy to the end of the chain', function() + local chain = _M.new({ 'one', 'two' }) + + chain:insert(policy) + + assert.equal(policy, chain[3]) + assert.equal(3, #chain) + end) + + it('adds a policy to specific position', function() + local chain = _M.new({ 'one', 'two'}) + + chain:insert(policy, 2) + assert.equal(policy, chain[2]) + assert.equal('one', chain[1]) + assert.equal('two', chain[3]) + assert.equal(3, #chain) + end) + + it('errors when inserting to frozen chain', function() + local chain = _M.new({}):freeze() + + local ok, err = chain:insert(policy, 1) + + assert.is_nil(ok) + assert.equal(err, 'frozen chain') + assert.equal(0, #chain) + end) + end) + describe('.export', function() it('returns the data exposed by each of its policies', function() local policy_1 = policy.new('1') @@ -136,4 +168,16 @@ describe('policy_chain', function() end) end) end) + + describe('.default', function() + it('returns a default policy chain', function() + local default = _M.default() + + assert(#default > 1, 'has <= 1 policy') + end) + + it('returns not frozen chain', function() + assert.falsy(_M.default().frozen) + end) + end) end) From bd35a01f9f019b6bf583670f15b671e29a45da07 Mon Sep 17 00:00:00 2001 From: Michal Cichra Date: Mon, 27 Nov 2017 18:06:00 +0100 Subject: [PATCH 3/3] [policy_chain] improve policy chain documentation --- gateway/src/apicast/policy_chain.lua | 18 ++++++++++++++---- spec/policy_chain_spec.lua | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/gateway/src/apicast/policy_chain.lua b/gateway/src/apicast/policy_chain.lua index 7ca2f1aa0..b62e20741 100644 --- a/gateway/src/apicast/policy_chain.lua +++ b/gateway/src/apicast/policy_chain.lua @@ -67,8 +67,8 @@ end -- If the module is a string, returns the result of initializing it with the -- given arguments. Otherwise, this function simply returns the module -- received. --- @tparam string/object The module --- @tparam[opt] params needed to initialize the module +-- @tparam string|table module the module or its name +-- @tparam ?table ... params needed to initialize the module -- @treturn object The module instantiated function _M.load(module, ...) if type(module) == 'string' then @@ -85,6 +85,8 @@ function _M.load(module, ...) end end +--- Initialize new @{PolicyChain}. +-- @treturn PolicyChain function _M.new(list) local chain = list or {} @@ -93,6 +95,10 @@ function _M.new(list) return self end +--------------------- +--- @type PolicyChain +-- An instance of @{policy_chain}. + --- Export the shared context of the chain -- @treturn linked_list The context of the chain. Note: the list returned is -- read-only. @@ -111,7 +117,7 @@ end --- Freeze the policy chain to prevent modifications. -- After calling this method it won't be possible to insert more policies. --- @treturn self +-- @treturn PolicyChain returns self function _M:freeze() self.frozen = true return self @@ -120,11 +126,15 @@ end --- Insert a policy into the chain -- @tparam Policy policy the policy to be added to the chain -- @tparam[opt] int position the position to add the policy to, defaults to last one +-- @treturn int lenght of the chain +-- @error frozen | returned when chain is not modifiable +-- @see freeze function _M:insert(policy, position) if self.frozen then - return nil, 'frozen chain' + return nil, 'frozen' else insert(self, position or #self+1, policy) + return #self end end diff --git a/spec/policy_chain_spec.lua b/spec/policy_chain_spec.lua index 0ed0cd69f..bbfb40e60 100644 --- a/spec/policy_chain_spec.lua +++ b/spec/policy_chain_spec.lua @@ -119,7 +119,7 @@ describe('policy_chain', function() local ok, err = chain:insert(policy, 1) assert.is_nil(ok) - assert.equal(err, 'frozen chain') + assert.equal(err, 'frozen') assert.equal(0, #chain) end) end)