From 6f37185d8d74b461c3d3cd4f0f04d4bbf0f1e5f2 Mon Sep 17 00:00:00 2001 From: kevprice83 Date: Mon, 18 Jan 2021 15:25:07 +0100 Subject: [PATCH 1/5] adds method matching check to rewrite url captures policy --- .../rewrite_url_captures/apicast-policy.json | 56 +++++++++++++++++++ .../named_args_matcher.lua | 26 ++++++++- .../rewrite_url_captures.lua | 3 +- t/apicast-policy-rewrite-url-captures.t | 48 ++++++++++++++++ 4 files changed, 131 insertions(+), 2 deletions(-) diff --git a/gateway/src/apicast/policy/rewrite_url_captures/apicast-policy.json b/gateway/src/apicast/policy/rewrite_url_captures/apicast-policy.json index aa385109d..0fe784494 100644 --- a/gateway/src/apicast/policy/rewrite_url_captures/apicast-policy.json +++ b/gateway/src/apicast/policy/rewrite_url_captures/apicast-policy.json @@ -11,6 +11,59 @@ "'/123/456' will be transformed into '/sales/v2/123?account=456'"], "version": "builtin", "configuration": { + "definitions": { + "methods": { + "description": "Array of HTTP methods this rule must be applied to. If left blank it will be applied to all HTTP methods", + "type": "array", + "items": { + "type": "string", + "oneOf": [ + { + "enum": [ + "GET" + ], + "title": "GET" + }, + { + "enum": [ + "POST" + ], + "title": "POST" + }, + { + "enum": [ + "PUT" + ], + "title": "PUT" + }, + { + "enum": [ + "PATCH" + ], + "title": "PATCH" + }, + { + "enum": [ + "DELETE" + ], + "title": "DELETE" + }, + { + "enum": [ + "HEAD" + ], + "title": "HEAD" + }, + { + "enum": [ + "OPTIONS" + ], + "title": "OPTIONS" + } + ] + } + } + }, "type": "object", "properties": { "transformations": { @@ -25,6 +78,9 @@ "template": { "type": "string", "description": "Template in which the matched args are replaced" + }, + "methods": { + "$ref": "#/definitions/methods" } } } diff --git a/gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua b/gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua index 30036a782..4383b3fcf 100644 --- a/gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua +++ b/gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua @@ -99,16 +99,35 @@ end -- with "{}". For example: "/{var_1}/something/{var_2}". -- @tparam string template Template in which the named args matched will be -- replaced. For example: "/v2/something/{var_1}?my_arg={var_2}". -function _M.new(match_rule, template) +function _M.new(match_rule, template, methods) local self = setmetatable({}, mt) self.named_args = extract_named_args(match_rule) self.regex_rule = transform_rule_to_regex(match_rule) self.template = template + self.methods = methods return self end +-- Returns true if the Method of the request is in the methods of the command meaning the rewrite rule should be applied +-- Returns true if no Method is provided in the config for backwardscompatibility +local function is_match_methods(methods) + + local request_method = ngx.req.get_method() + + if methods == nil or next(methods) == nil then + return true + end + + for _,v in pairs(methods) do + if v == request_method then + return true + end + end + return false +end + --- Match a path -- @tparam string path The path of the URL -- @treturn boolean True if there's a match, false otherwise @@ -121,6 +140,11 @@ end -- this is so callers can iterate through them with pairs (jitted) instead of -- ipairs (non-jitted). function _M:match(path) + + if not is_match_methods(self.methods) then + return false + end + local matches = re_match(path, self.regex_rule, 'oj') if not matches or #self.named_args ~= #matches then diff --git a/gateway/src/apicast/policy/rewrite_url_captures/rewrite_url_captures.lua b/gateway/src/apicast/policy/rewrite_url_captures/rewrite_url_captures.lua index 71dcc3421..318161b6a 100644 --- a/gateway/src/apicast/policy/rewrite_url_captures/rewrite_url_captures.lua +++ b/gateway/src/apicast/policy/rewrite_url_captures/rewrite_url_captures.lua @@ -27,7 +27,8 @@ function _M.new(config) for _, transformation in ipairs(config.transformations or {}) do local matcher = NamedArgsMatcher.new( transformation.match_rule, - transformation.template + transformation.template, + transformation.methods ) insert(self.matchers, matcher) diff --git a/t/apicast-policy-rewrite-url-captures.t b/t/apicast-policy-rewrite-url-captures.t index 872296ac2..cb6bacb1f 100644 --- a/t/apicast-policy-rewrite-url-captures.t +++ b/t/apicast-policy-rewrite-url-captures.t @@ -233,3 +233,51 @@ yay, api backend --- error_code: 200 --- no_error_log [error] + +=== TEST 5: one transformation with method that doesn't match +--- configuration +{ + "services": [ + { + "id": 42, + "proxy": { + "policy_chain": [ + { + "name": "rewrite_url_captures", + "configuration": { + "transformations": [ + { + "match_rule": "/{var_1}/{var_2}", + "template": "/{var_2}?my_arg={var_1}", + "methods": ["PUT", "OPTIONS"] + } + ] + } + }, + { + "name": "upstream", + "configuration": { + "rules": [ + { + "regex": "/", + "url": "http://test:$TEST_NGINX_SERVER_PORT" + } + ] + } + } + ] + } + } + ] +} +--- upstream + location /abc/def { + content_by_lua_block { + ngx.exit(404) + } + } +--- request +GET /abc/def?user_key=xyz +--- error_code: 404 +--- no_error_log +[error] \ No newline at end of file From dafb843e044756a5d28c2dbc1b5e2f89bba43cce Mon Sep 17 00:00:00 2001 From: kevprice83 Date: Mon, 18 Jan 2021 20:35:39 +0100 Subject: [PATCH 2/5] adds unit test for rewrite url captures module --- .../rewrite_url_captures_spec.lua | 54 +++++++++++++++++-- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/spec/policy/rewrite_url_captures/rewrite_url_captures_spec.lua b/spec/policy/rewrite_url_captures/rewrite_url_captures_spec.lua index 3ee402be6..902df9e68 100644 --- a/spec/policy/rewrite_url_captures/rewrite_url_captures_spec.lua +++ b/spec/policy/rewrite_url_captures/rewrite_url_captures_spec.lua @@ -11,19 +11,27 @@ describe('Capture args policy', function() before_each(function() query_params = { set = stub.new() } stub(QueryParams, 'new').returns(query_params) - + stub(ngx.req, 'get_method', function() return 'GET' end) set_uri_stub = stub(ngx.req, 'set_uri') ngx.var = { uri = '/abc/def' } matching_transformation = { match_rule = '/{var_1}/{var_2}', - template = '/{var_2}?my_arg={var_1}' + template = '/{var_2}?my_arg={var_1}', + methods = {'GET'} } non_matching_transformation = { match_rule = '/i_dont_match/{var_1}/{var_2}', - template = '/{var_2}?my_arg={var_1}' + template = '/{var_2}?my_arg={var_1}', + methods = {'GET'} + } + + matching_transformation_no_method = { + match_rule = '/{var1}/{var2}', + template = '/{var2}?my_arg={var1}', + methods = {} } end) @@ -33,7 +41,44 @@ describe('Capture args policy', function() non_matching_transformation, matching_transformation } + local rewrite_url_captures = RewriteUrlCapturesPolicy.new( + { transformations = transformations } + ) + + rewrite_url_captures:rewrite() + + assert.stub(set_uri_stub).was_called_with('/def') + assert.stub(query_params.set).was_called_with( + query_params, 'my_arg', 'abc') + end) + end) + + describe('when there is a match but method does not match', function() + it('does not modifiy the path nor the params', function() + local transformations = { + non_matching_transformation, + matching_transformation + } + ngx.req.get_method:revert() + stub(ngx.req, 'get_method', function() return 'PUT' end) + local rewrite_url_captures = RewriteUrlCapturesPolicy.new( + { transformations = transformations } + ) + rewrite_url_captures:rewrite() + + assert.stub(set_uri_stub).was_not_called() + assert.stub(query_params.set).was_not_called() + end) + end) + + describe('when there is a match but no method is defined', function() + it('modifies the path and the params', function() + local transformations = { + non_matching_transformation, + matching_transformation, + matching_transformation_no_method + } local rewrite_url_captures = RewriteUrlCapturesPolicy.new( { transformations = transformations } ) @@ -52,7 +97,8 @@ describe('Capture args policy', function() match_rule = '/{var_1}/{var_2}', -- Swap var_1 and var_2 in the original one. - template = '/{var_1}?my_arg={var_2}' + template = '/{var_1}?my_arg={var_2}', + methods = {'POST'} } local transformations = { From 3bf3c276b507bebb618cbc8c410823a9986be544 Mon Sep 17 00:00:00 2001 From: kevprice83 Date: Wed, 20 Jan 2021 12:29:08 +0100 Subject: [PATCH 3/5] adds named args matcher unit tests & changelog entry --- CHANGELOG.md | 1 + .../named_args_matcher_spec.lua | 102 ++++++++++++++++-- 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12d121efa..05c6280fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Added +- Add methods to transformations in rewrite url captures policy [PR #1253](https://github.com/3scale/APIcast/pull/1253) [THREESCALE-6270](https://issues.redhat.com/browse/THREESCALE-6270) - Add Access-Control-Max-Age [PR #1247](https://github.com/3scale/APIcast/pull/1247) [THREESCALE-6556](https://issues.redhat.com/browse/THREESCALE-6556) - Add HTTP codes policy [PR #1236](https://github.com/3scale/APIcast/pull/1236) [THREESCALE-6255](https://issues.redhat.com/browse/THREESCALE-6255) - Buffer access log on chunks [PR #1248](https://github.com/3scale/APIcast/pull/1248) [THREESCALE-6563](https://issues.redhat.com/browse/THREESCALE-6563) diff --git a/spec/policy/rewrite_url_captures/named_args_matcher_spec.lua b/spec/policy/rewrite_url_captures/named_args_matcher_spec.lua index bfac457ea..5882bbe25 100644 --- a/spec/policy/rewrite_url_captures/named_args_matcher_spec.lua +++ b/spec/policy/rewrite_url_captures/named_args_matcher_spec.lua @@ -1,13 +1,35 @@ local NamedArgsMatcher = require('apicast.policy.rewrite_url_captures.named_args_matcher') +before_each(function() + stub(ngx.req, 'get_method', function() return 'GET' end) +end) + describe('named_args_matcher', function() describe('.match', function() describe('when there is a match', function() - describe('and there are query args in the template', function() + describe('and there are query args in the template and no methods', function() it('returns true, the new url and query args', function() local match_rule = "/{var_1}/blah/{var_2}/{var_3}" local template = "/{var_2}/something/{var_1}?my_arg={var_3}" - local matcher = NamedArgsMatcher.new(match_rule, template) + local methods = {} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) + local request_path = "/abc/blah/def/ghi" + + local matched, new_uri, args, arg_vals = matcher:match(request_path) + + assert.is_true(matched) + assert.equals('/def/something/abc', new_uri) + assert.same({ 'my_arg' }, args) + assert.same({ 'ghi' }, arg_vals) + end) + end) + + describe('and there are query args in the template and methods', function() + it('returns true, the new url and query args', function() + local match_rule = "/{var_1}/blah/{var_2}/{var_3}" + local template = "/{var_2}/something/{var_1}?my_arg={var_3}" + local methods = {"GET", "PUT"} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) local request_path = "/abc/blah/def/ghi" local matched, new_uri, args, arg_vals = matcher:match(request_path) @@ -19,11 +41,29 @@ describe('named_args_matcher', function() end) end) - describe('and there are no query args in the template', function() + describe('and there are no query args in the template and no methods', function() it('returns true, the new url and an empty list of args', function() local match_rule = "/{var_1}/blah/{var_2}" local template = "/{var_2}/something/{var_1}" - local matcher = NamedArgsMatcher.new(match_rule, template) + local methods = {} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) + local request_path = "/abc/blah/def" + + local matched, new_uri, args, arg_vals = matcher:match(request_path) + + assert.is_true(matched) + assert.equals('/def/something/abc', new_uri) + assert.same({}, args) + assert.same({}, arg_vals) + end) + end) + + describe('and there are methods in the template but no query args', function() + it('returns true, the new url and an empty list of args', function() + local match_rule = "/{var_1}/blah/{var_2}" + local template = "/{var_2}/something/{var_1}" + local methods = {"GET", "PUT"} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) local request_path = "/abc/blah/def" local matched, new_uri, args, arg_vals = matcher:match(request_path) @@ -39,7 +79,8 @@ describe('named_args_matcher', function() it('returns true, the new url and an empty list of args', function() local match_rule = "/v2/{var_1}" local template = "/?my_arg={var_1}" - local matcher = NamedArgsMatcher.new(match_rule, template) + local methods = {} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) local request_path = "/v2/abc" local matched, new_uri, args, arg_vals = matcher:match(request_path) @@ -50,6 +91,23 @@ describe('named_args_matcher', function() assert.same({ 'abc' }, arg_vals) end) end) + + describe('and only the params part of the template has args and methods are defined', function() + it('returns true, the new url and an empty list of args', function() + local match_rule = "/v2/{var_1}" + local template = "/?my_arg={var_1}" + local methods = {"GET", "PUT"} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) + local request_path = "/v2/abc" + + local matched, new_uri, args, arg_vals = matcher:match(request_path) + + assert.is_true(matched) + assert.equals('/', new_uri) + assert.same({ 'my_arg' }, args) + assert.same({ 'abc' }, arg_vals) + end) + end) end) describe('when there is not a match', function() @@ -57,7 +115,22 @@ describe('named_args_matcher', function() it('returns false', function() local match_rule = "/{var_1}/blah/{var_2}/{var_3}" local template = "/{var_2}/something/{var_1}?my_arg={var_3}" - local matcher = NamedArgsMatcher.new(match_rule, template) + local methods = {} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) + local request_path = "/" + + local matched = matcher:match(request_path) + + assert.is_false(matched) + end) + end) + + describe('because no args and no methods matched', function() + it('returns false', function() + local match_rule = "/{var_1}/blah/{var_2}/{var_3}" + local template = "/{var_2}/something/{var_1}?my_arg={var_3}" + local methods = {"PUT", "POST"} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) local request_path = "/" local matched = matcher:match(request_path) @@ -70,7 +143,22 @@ describe('named_args_matcher', function() it('returns false', function() local match_rule = "/{var_1}/blah/{var_2}/{var_3}" local template = "/{var_2}/something/{var_1}?my_arg={var_3}" - local matcher = NamedArgsMatcher.new(match_rule, template) + local methods = {} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) + local request_path = "/abc/blah" + + local matched = matcher:match(request_path) + + assert.is_false(matched) + end) + end) + + describe('because only some args matched and no methods matched', function() + it('returns false', function() + local match_rule = "/{var_1}/blah/{var_2}/{var_3}" + local template = "/{var_2}/something/{var_1}?my_arg={var_3}" + local methods = {"PUT", "POST"} + local matcher = NamedArgsMatcher.new(match_rule, template, methods) local request_path = "/abc/blah" local matched = matcher:match(request_path) From a7e7f3788bb4c097100359edee374386abe3b3d1 Mon Sep 17 00:00:00 2001 From: kevprice83 Date: Wed, 20 Jan 2021 13:05:28 +0100 Subject: [PATCH 4/5] adds 2 more test cases as requested --- t/apicast-policy-rewrite-url-captures.t | 108 +++++++++++++++++++++++- 1 file changed, 107 insertions(+), 1 deletion(-) diff --git a/t/apicast-policy-rewrite-url-captures.t b/t/apicast-policy-rewrite-url-captures.t index cb6bacb1f..8d2ffacb1 100644 --- a/t/apicast-policy-rewrite-url-captures.t +++ b/t/apicast-policy-rewrite-url-captures.t @@ -280,4 +280,110 @@ yay, api backend GET /abc/def?user_key=xyz --- error_code: 404 --- no_error_log -[error] \ No newline at end of file +[error] + +=== TEST 6: one transformation with method that matches +--- configuration +{ + "services": [ + { + "id": 42, + "proxy": { + "policy_chain": [ + { + "name": "rewrite_url_captures", + "configuration": { + "transformations": [ + { + "match_rule": "/{var_1}/{var_2}", + "template": "/{var_2}?my_arg={var_1}", + "methods": ["GET"] + } + ] + } + }, + { + "name": "upstream", + "configuration": { + "rules": [ + { + "regex": "/", + "url": "http://test:$TEST_NGINX_SERVER_PORT" + } + ] + } + } + ] + } + } + ] +} +--- upstream + location / { + content_by_lua_block { + local assert = require('luassert') + assert.equals('/def', ngx.var.uri) + assert.equals('abc', ngx.req.get_uri_args()['my_arg']) + ngx.say('yay, api backend'); + } + } +--- request +GET /abc/def?user_key=value +--- response_body +yay, api backend +--- error_code: 200 +--- no_error_log +[error] + +=== TEST 7: one transformation including an array of methods that matches +--- configuration +{ + "services": [ + { + "id": 42, + "proxy": { + "policy_chain": [ + { + "name": "rewrite_url_captures", + "configuration": { + "transformations": [ + { + "match_rule": "/{var_1}/{var_2}", + "template": "/{var_2}?my_arg={var_1}", + "methods": ["GET", "PUT", "POST"] + } + ] + } + }, + { + "name": "upstream", + "configuration": { + "rules": [ + { + "regex": "/", + "url": "http://test:$TEST_NGINX_SERVER_PORT" + } + ] + } + } + ] + } + } + ] +} +--- upstream + location / { + content_by_lua_block { + local assert = require('luassert') + assert.equals('/def', ngx.var.uri) + assert.equals('abc', ngx.req.get_uri_args()['my_arg']) + ngx.say('yay, api backend'); + } + } +--- request +PUT /abc/def?user_key=value +--- response_body +yay, api backend +--- error_code: 200 +--- no_error_log +[error] From 5b5114dc109edbb2078cbaf4c389e29a9eb4b413 Mon Sep 17 00:00:00 2001 From: kevprice83 Date: Wed, 20 Jan 2021 13:07:35 +0100 Subject: [PATCH 5/5] moves get_method function after conditional check --- .../policy/rewrite_url_captures/named_args_matcher.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua b/gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua index 4383b3fcf..851a52d4d 100644 --- a/gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua +++ b/gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua @@ -114,12 +114,12 @@ end -- Returns true if no Method is provided in the config for backwardscompatibility local function is_match_methods(methods) - local request_method = ngx.req.get_method() - if methods == nil or next(methods) == nil then return true end + local request_method = ngx.req.get_method() + for _,v in pairs(methods) do if v == request_method then return true