From 97a850646d6a5ad21712a63fb0ad7f0f230b16d7 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Thu, 1 Mar 2018 11:31:34 +0100 Subject: [PATCH 1/4] spec/policy/url_rewriting: make configs conform with the policy manifest --- .../url_rewriting/url_rewriting_spec.lua | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/spec/policy/url_rewriting/url_rewriting_spec.lua b/spec/policy/url_rewriting/url_rewriting_spec.lua index a1855a69b..02c81bee4 100644 --- a/spec/policy/url_rewriting/url_rewriting_spec.lua +++ b/spec/policy/url_rewriting/url_rewriting_spec.lua @@ -12,7 +12,9 @@ describe('URL rewriting policy', function() it('can rewrite URLs using sub', function() local config_with_sub = { - { op = 'sub', regex = 'to_be_replaced', replace = 'new' } + commands = { + { op = 'sub', regex = 'to_be_replaced', replace = 'new' } + } } local url_rewriting = URLRewriting.new(config_with_sub) @@ -24,7 +26,9 @@ describe('URL rewriting policy', function() it('can rewrite URLs using gsub', function() local config_with_gsub = { - { op = 'gsub', regex = 'to_be_replaced', replace = 'new' } + commands = { + { op = 'gsub', regex = 'to_be_replaced', replace = 'new' } + } } local url_rewriting = URLRewriting.new(config_with_gsub) @@ -35,9 +39,11 @@ describe('URL rewriting policy', function() it('applies the commands in order', function() local config_with_several_ops = { - { op = 'gsub', regex = 'to_be_replaced', replace = 'abc' }, - { op = 'gsub', regex = 'abc', replace = 'def' }, - { op = 'gsub', regex = 'def', replace = 'ghi' } + commands = { + { op = 'gsub', regex = 'to_be_replaced', replace = 'abc' }, + { op = 'gsub', regex = 'abc', replace = 'def' }, + { op = 'gsub', regex = 'def', replace = 'ghi' } + } } local url_rewriting = URLRewriting.new(config_with_several_ops) @@ -48,8 +54,10 @@ describe('URL rewriting policy', function() it('when there is a break, stops at the first match', function() local config_with_break = { - { op = 'gsub', regex = 'to_be_replaced', replace = 'abc', ['break'] = '1' }, - { op = 'gsub', regex = 'abc', replace = 'def' } -- Not applied + commands = { + { op = 'gsub', regex = 'to_be_replaced', replace = 'abc', ['break'] = '1' }, + { op = 'gsub', regex = 'abc', replace = 'def' } -- Not applied + } } local url_rewriting = URLRewriting.new(config_with_break) @@ -60,7 +68,9 @@ describe('URL rewriting policy', function() it('accepts options for the regexes, same as ngx.req.{sub, gsub}', function() local config_with_regex_opts = { - { op = 'gsub', regex = 'TO_BE_REPLACED', replace = 'new', options = 'i' } + commands = { + { op = 'gsub', regex = 'TO_BE_REPLACED', replace = 'new', options = 'i' } + } } local url_rewriting = URLRewriting.new(config_with_regex_opts) From 5d430e05cae5e63d2d10da71495ba75b118cf4e4 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Thu, 1 Mar 2018 11:32:00 +0100 Subject: [PATCH 2/4] t/apicast-policy-url-rewriting: make configs conform with the policy manifest --- t/apicast-policy-url-rewriting.t | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/t/apicast-policy-url-rewriting.t b/t/apicast-policy-url-rewriting.t index e99c2f942..8facb9789 100644 --- a/t/apicast-policy-url-rewriting.t +++ b/t/apicast-policy-url-rewriting.t @@ -29,10 +29,11 @@ __DATA__ { "name": "apicast.policy.apicast" }, { "name": "apicast.policy.url_rewriting", - "configuration": - [ + "configuration": { + "commands": [ { "op": "sub", "regex": "original", "replace": "new" } ] + } } ] } @@ -78,10 +79,11 @@ yay, api backend { "name": "apicast.policy.apicast" }, { "name": "apicast.policy.url_rewriting", - "configuration": - [ + "configuration": { + "commands": [ { "op": "gsub", "regex": "original", "replace": "new" } ] + } } ] } @@ -128,12 +130,13 @@ Substitutions are applied in the order specified. { "name": "apicast.policy.apicast" }, { "name": "apicast.policy.url_rewriting", - "configuration": - [ + "configuration": { + "commands": [ { "op": "gsub", "regex": "aaa", "replace": "bbb", "options": "i" }, { "op": "sub", "regex": "bbb", "replace": "ccc" }, { "op": "sub", "regex": "ccc", "replace": "ddd" } ] + } } ] } @@ -182,13 +185,14 @@ We need to test 2 things: { "name": "apicast.policy.apicast" }, { "name": "apicast.policy.url_rewriting", - "configuration": - [ + "configuration": { + "commands": [ { "op": "sub", "regex": "does_not_match", "replace": "a", "break": true }, { "op": "sub", "regex": "aaa", "replace": "bbb" }, { "op": "sub", "regex": "bbb", "replace": "ccc", "break": true }, { "op": "sub", "regex": "ccc", "replace": "ddd" } ] + } } ] } @@ -236,10 +240,11 @@ rules. "policy_chain": [ { "name": "apicast.policy.url_rewriting", - "configuration": - [ + "configuration": { + "commands": [ { "op": "sub", "regex": "original", "replace": "new" } ] + } }, { "name": "apicast.policy.apicast" } ] From 04ebfe89ec23e375e430b151da51dc393e0ee4bb Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Thu, 1 Mar 2018 11:32:22 +0100 Subject: [PATCH 3/4] policy/url_rewriting: parse config respecting the policy manifest The code was not taking into account that there's a `commands` attribute in the manifest. --- gateway/src/apicast/policy/url_rewriting/url_rewriting.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua b/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua index 66bab37a7..4b2bd7237 100644 --- a/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua +++ b/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua @@ -58,12 +58,12 @@ end -- the URL, it will be the last command applied. function _M.new(config) local self = new() - self.config = config or {} + self.commands = (config and config.commands) or {} return self end function _M:rewrite() - for _, command in ipairs(self.config) do + for _, command in ipairs(self.commands) do local rewritten = apply_rewrite_command(command) if rewritten and command['break'] then From fa5cb612be2bceb91c29a07979f45a571061ed6b Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 2 Mar 2018 11:43:40 +0100 Subject: [PATCH 4/4] CHANGELOG: add URL rewriting not respecting the manifest bugfix --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fad83caf..86591ac39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Error loading policy chain configuration JSON with null value [PR #626](https://github.com/3scale/apicast/pull/626) - Splitted `resolv.conf` in lines,to avoid commented lines [PR #618](https://github.com/3scale/apicast/pull/618) - Avoid `nameserver` repetion from `RESOLVER` variable and `resolv.conf` file [PR #636](https://github.com/3scale/apicast/pull/636) +- Bug in URL rewriting policy that ignored the `commands` attribute in the policy manifest [PR #641](https://github.com/3scale/apicast/pull/641) ## Added