From 812ebf59a730f5eebac3a46a21366b067b0dde38 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 2 Mar 2018 11:38:26 +0100 Subject: [PATCH 01/11] Add PolicyConfigValidator --- .../src/apicast/policy_config_validator.lua | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 gateway/src/apicast/policy_config_validator.lua diff --git a/gateway/src/apicast/policy_config_validator.lua b/gateway/src/apicast/policy_config_validator.lua new file mode 100644 index 000000000..dbc9eb0da --- /dev/null +++ b/gateway/src/apicast/policy_config_validator.lua @@ -0,0 +1,20 @@ +--- Policy Config Validator +-- @module policy_config_validator +-- Validates a policy configuration against a policy config JSON schema. + +local jsonschema = require('jsonschema') + +local _M = { } + +--- Validate a policy configuration +-- Checks if a policy configuration is valid according to the given schema. +-- @tparam table config Policy configuration +-- @tparam table config_schema Policy configuration schema +-- @treturn boolean True if the policy configuration is valid. False otherwise. +-- @treturn string Error message only when the policy config is invalid. +function _M.validate_config(config, config_schema) + local validator = jsonschema.generate_validator(config_schema or {}) + return validator(config or {}) +end + +return _M From 258a7469bcbabb638b02682ef9d602c50ffcfd52 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 2 Mar 2018 11:38:39 +0100 Subject: [PATCH 02/11] spec: add specs for PolicyConfigValidator --- spec/policy_config_validator_spec.lua | 58 +++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 spec/policy_config_validator_spec.lua diff --git a/spec/policy_config_validator_spec.lua b/spec/policy_config_validator_spec.lua new file mode 100644 index 000000000..555398403 --- /dev/null +++ b/spec/policy_config_validator_spec.lua @@ -0,0 +1,58 @@ +local cjson = require('cjson') +local policy_config_validator = require('apicast.policy_config_validator') + +describe('policy_config_validator', function() + -- We use the echo policy schema as an example for the tests. + local test_config_schema = cjson.decode([[ + { + "type": "object", + "properties": { + "status": { + "description": "HTTP status code to be returned", + "type": "integer" + }, + "exit": { + "description": "Exit mode", + "type": "string", + "oneOf": [{ + "enum": ["request"], + "description": "Interrupts the processing of the request." + }, + { + "enum": ["set"], + "description": "Only skips the rewrite phase." + } + ] + } + } + } + ]]) + + it('returns true with a config that conforms with the schema', function() + local valid_config = { status = 200, exit = "request" } + + local is_valid, err = policy_config_validator.validate_config( + valid_config, test_config_schema) + + assert.is_true(is_valid) + assert.is_nil(err) + end) + + it('returns false with a config that does not conform with the schema', function() + local invalid_config = { status = "not_an_integer" } + + local is_valid, err = policy_config_validator.validate_config( + invalid_config, test_config_schema) + + assert.is_false(is_valid) + assert.is_not_nil(err) + end) + + it('returns true when the schema is empty', function() + assert.is_true(policy_config_validator.validate_config({ a_param = 1 }, {})) + end) + + it('returns true when the schema is nil', function() + assert.is_true(policy_config_validator.validate_config({ a_param = 1 }, nil)) + end) +end) From b6ae1730dd67fc1272f0db64d90899f55447aa9b Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 2 Mar 2018 15:42:34 +0100 Subject: [PATCH 03/11] policy: pass config to policy.new() --- gateway/src/apicast/policy/caching/caching.lua | 2 +- gateway/src/apicast/policy/cors/cors.lua | 2 +- gateway/src/apicast/policy/headers/headers.lua | 2 +- .../apicast/policy/token_introspection/token_introspection.lua | 2 +- gateway/src/apicast/policy/url_rewriting/url_rewriting.lua | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/gateway/src/apicast/policy/caching/caching.lua b/gateway/src/apicast/policy/caching/caching.lua index a43ccf757..8f88f4366 100644 --- a/gateway/src/apicast/policy/caching/caching.lua +++ b/gateway/src/apicast/policy/caching/caching.lua @@ -98,7 +98,7 @@ end -- @tparam[opt] table config -- @field caching_type Caching type (strict, resilient, allow, none) function _M.new(config) - local self = new() + local self = new(config) self.cache_handler = handler(config or {}) return self end diff --git a/gateway/src/apicast/policy/cors/cors.lua b/gateway/src/apicast/policy/cors/cors.lua index a96c8d425..b12ccf018 100644 --- a/gateway/src/apicast/policy/cors/cors.lua +++ b/gateway/src/apicast/policy/cors/cors.lua @@ -25,7 +25,7 @@ local new = _M.new -- @field[opt] allow_origin Allowed origins (e.g. 'http://example.com', '*') -- @field[opt] allow_credentials Boolean function _M.new(config) - local self = new() + local self = new(config) self.config = config or {} return self end diff --git a/gateway/src/apicast/policy/headers/headers.lua b/gateway/src/apicast/policy/headers/headers.lua index b29e28782..3a8dceace 100644 --- a/gateway/src/apicast/policy/headers/headers.lua +++ b/gateway/src/apicast/policy/headers/headers.lua @@ -106,7 +106,7 @@ end -- 2) When the header is set, it creates a new header with the same name and -- the given value. function _M.new(config) - local self = new() + local self = new(config) self.config = init_config(config) return self end diff --git a/gateway/src/apicast/policy/token_introspection/token_introspection.lua b/gateway/src/apicast/policy/token_introspection/token_introspection.lua index 3fabd1a02..472413601 100644 --- a/gateway/src/apicast/policy/token_introspection/token_introspection.lua +++ b/gateway/src/apicast/policy/token_introspection/token_introspection.lua @@ -10,7 +10,7 @@ local resty_env = require('resty.env') local new = _M.new function _M.new(config) - local self = new() + local self = new(config) self.config = config or {} --- authorization for the token introspection endpoint. -- https://tools.ietf.org/html/rfc7662#section-2.2 diff --git a/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua b/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua index 4b2bd7237..167d0fd44 100644 --- a/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua +++ b/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua @@ -57,7 +57,7 @@ end -- - break[opt]: defaults to false. When set to true, if the command rewrote -- the URL, it will be the last command applied. function _M.new(config) - local self = new() + local self = new(config) self.commands = (config and config.commands) or {} return self end From a14f2efbdbd37da9f2724c69cad9f56cb7801d34 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 2 Mar 2018 16:24:24 +0100 Subject: [PATCH 04/11] policy: replace consts with enums of 1 elem 'ljsonschema' uses the draft-04 version of the JSON schema. 'const' was introduced later and so we can't use them. An enum of 1 element is mostly equivalent and a valid option for our use case. --- gateway/src/apicast/policy/caching/apicast-policy.json | 8 ++++---- gateway/src/apicast/policy/echo/apicast-policy.json | 4 ++-- gateway/src/apicast/policy/headers/apicast-policy.json | 6 +++--- .../src/apicast/policy/url_rewriting/apicast-policy.json | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/gateway/src/apicast/policy/caching/apicast-policy.json b/gateway/src/apicast/policy/caching/apicast-policy.json index 12a45ead8..27ba5e8dc 100644 --- a/gateway/src/apicast/policy/caching/apicast-policy.json +++ b/gateway/src/apicast/policy/caching/apicast-policy.json @@ -27,19 +27,19 @@ "type": "string", "oneOf": [ { - "const": "resilient", + "enum": ["resilient"], "description": "Authorize according to last request when backend is down." }, { - "const": "strict", + "enum": ["strict"], "description": "It only caches authorized calls." }, { - "const": "allow", + "enum": ["allow"], "description": "When backend is down, allow everything unless seen before and denied." }, { - "const": "none", + "enum": ["none"], "description": "Disables caching." } ] diff --git a/gateway/src/apicast/policy/echo/apicast-policy.json b/gateway/src/apicast/policy/echo/apicast-policy.json index 7055aafdc..cff351cc2 100644 --- a/gateway/src/apicast/policy/echo/apicast-policy.json +++ b/gateway/src/apicast/policy/echo/apicast-policy.json @@ -18,11 +18,11 @@ "type": "string", "oneOf": [ { - "const": "request", + "enum": ["request"], "description": "Interrupts the processing of the request." }, { - "const": "set", + "enum": ["set"], "description": "Only skips the rewrite phase." } ] diff --git a/gateway/src/apicast/policy/headers/apicast-policy.json b/gateway/src/apicast/policy/headers/apicast-policy.json index 739685115..f17291813 100644 --- a/gateway/src/apicast/policy/headers/apicast-policy.json +++ b/gateway/src/apicast/policy/headers/apicast-policy.json @@ -22,15 +22,15 @@ "type": "string", "oneOf": [ { - "const": "add", + "enum": ["add"], "description": "Adds a value to an existing header." }, { - "const": "set", + "enum": ["set"], "description": "Creates the header when not set, replaces its value when set." }, { - "const": "push", + "enum": ["push"], "description": "Creates the header when not set, adds the value when set." } ] diff --git a/gateway/src/apicast/policy/url_rewriting/apicast-policy.json b/gateway/src/apicast/policy/url_rewriting/apicast-policy.json index a7b3f2f54..719abb10d 100644 --- a/gateway/src/apicast/policy/url_rewriting/apicast-policy.json +++ b/gateway/src/apicast/policy/url_rewriting/apicast-policy.json @@ -23,11 +23,11 @@ "type": "string", "oneOf": [ { - "const": "sub", + "enum": ["sub"], "description": "Substitutes the first match of the regex applied." }, { - "const": "gsub", + "enum": ["gsub"], "description": "Substitutes all the matches of the regex applied." } ] From e9727604666ede70be5a342de7a490c855c69195 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Mon, 5 Mar 2018 14:34:18 +0100 Subject: [PATCH 05/11] t/fixtures: add example policy --- .../example_policy/1.0.0/apicast-policy.json | 16 ++++++++++++++ .../example_policy/1.0.0/example_policy.lua | 21 +++++++++++++++++++ .../policies/example_policy/1.0.0/init.lua | 1 + 3 files changed, 38 insertions(+) create mode 100644 t/fixtures/policies/example_policy/1.0.0/apicast-policy.json create mode 100644 t/fixtures/policies/example_policy/1.0.0/example_policy.lua create mode 100644 t/fixtures/policies/example_policy/1.0.0/init.lua diff --git a/t/fixtures/policies/example_policy/1.0.0/apicast-policy.json b/t/fixtures/policies/example_policy/1.0.0/apicast-policy.json new file mode 100644 index 000000000..343ab8e19 --- /dev/null +++ b/t/fixtures/policies/example_policy/1.0.0/apicast-policy.json @@ -0,0 +1,16 @@ +{ + "$schema": "http://apicast.io/policy-v1/schema#manifest#", + "name": "Example policy", + "description": "An example policy to be used in integration tests.", + "version": "1.0.0", + "configuration": { + "type": "object", + "properties": { + "message": { + "type": "string", + "description": "A message." + } + }, + "required": ["message"] + } +} diff --git a/t/fixtures/policies/example_policy/1.0.0/example_policy.lua b/t/fixtures/policies/example_policy/1.0.0/example_policy.lua new file mode 100644 index 000000000..12c858476 --- /dev/null +++ b/t/fixtures/policies/example_policy/1.0.0/example_policy.lua @@ -0,0 +1,21 @@ +local _M = require('apicast.policy').new('example_policy', '1.0.0') + +local new = _M.new + +function _M.new(configuration) + local policy = new(configuration) + + policy.message = '' + + if configuration then + policy.message = configuration.message + end + + return policy +end + +function _M:content() + ngx.say(self.message) +end + +return _M diff --git a/t/fixtures/policies/example_policy/1.0.0/init.lua b/t/fixtures/policies/example_policy/1.0.0/init.lua new file mode 100644 index 000000000..b2bdba2aa --- /dev/null +++ b/t/fixtures/policies/example_policy/1.0.0/init.lua @@ -0,0 +1 @@ +return require('example_policy') From 9b9eadbf1c16a17475cb9325b73804ee3949dc9f Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Mon, 5 Mar 2018 14:34:41 +0100 Subject: [PATCH 06/11] t: add test for invalid policy configuration --- t/apicast-policy-invalid-config.t | 44 +++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 t/apicast-policy-invalid-config.t diff --git a/t/apicast-policy-invalid-config.t b/t/apicast-policy-invalid-config.t new file mode 100644 index 000000000..dc48a730b --- /dev/null +++ b/t/apicast-policy-invalid-config.t @@ -0,0 +1,44 @@ +use lib 't'; +use Test::APIcast::Blackbox 'no_plan'; + +use Cwd qw(abs_path); + +BEGIN { + $ENV{TEST_NGINX_APICAST_POLICY_LOAD_PATH} = 't/fixtures/policies'; +} + +env_to_apicast( + 'APICAST_POLICY_LOAD_PATH' => abs_path($ENV{TEST_NGINX_APICAST_POLICY_LOAD_PATH}), +); + +run_tests(); + +__DATA__ + +=== TEST 1: policy with invalid configuration +In this test, we use an example policy that requires a 'message' param in the +configuration. We are going to initialize the policy with an empty +configuration and check that Apicast exits. +--- configuration +{ + "services": [ + { + "id": 42, + "backend_version": 1, + "backend_authentication_type": "service_token", + "backend_authentication_value": "token-value", + "proxy": { + "policy_chain": [ + { "name": "example_policy", "version": "1.0.0", "configuration": { } } + ], + "api_backend": "http://test:$TEST_NGINX_SERVER_PORT/", + "proxy_rules": [ + { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 } + ] + } + } + ] +} +--- request +GET /?user_key=value +--- must_die From 883b3e937bceaf324e97834faa55c362f028d166 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Tue, 6 Mar 2018 11:55:50 +0100 Subject: [PATCH 07/11] policy_loader: add a config validator func in the policies returned --- gateway/src/apicast/policy_loader.lua | 42 +++++++++++++++++++++------ 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/gateway/src/apicast/policy_loader.lua b/gateway/src/apicast/policy_loader.lua index e233e2dc6..72a39d1be 100644 --- a/gateway/src/apicast/policy_loader.lua +++ b/gateway/src/apicast/policy_loader.lua @@ -14,6 +14,8 @@ local insert = table.insert local setmetatable = setmetatable local pcall = pcall +local policy_config_validator = require('apicast.policy_config_validator') + local _M = {} local resty_env = require('resty.env') @@ -71,35 +73,55 @@ local function load_manifest(name, version, path) return nil, lua_load_path(path) end +local function with_config_validator(policy, policy_config_schema) + local original_new = policy.new + + local new_with_validator = function(config) + local is_valid, err = policy_config_validator.validate_config( + config, policy_config_schema) + + if not is_valid then + error(format('Invalid config for policy: %s', err)) + end + + return original_new(config) + end + + return setmetatable( + { new = new_with_validator }, + { __index = policy } + ) +end + function _M:load_path(name, version, paths) local failures = {} for _, path in ipairs(paths or self.policy_load_paths()) do - local ok, load_path = load_manifest(name, version, format('%s/%s/%s', path, name, version) ) + local manifest, load_path = load_manifest(name, version, format('%s/%s/%s', path, name, version) ) - if ok then - return load_path + if manifest then + return load_path, manifest.configuration else insert(failures, load_path) end end if version == 'builtin' then - local ok, load_path = load_manifest(name, version, format('%s/%s', self.builtin_policy_load_path(), name) ) + local manifest, load_path = load_manifest(name, version, format('%s/%s', self.builtin_policy_load_path(), name) ) - if ok then - return load_path + if manifest then + return load_path, manifest.configuration else insert(failures, load_path) end end - return nil, failures + return nil, nil, failures end function _M:call(name, version, dir) local v = version or 'builtin' - local load_path, invalid_paths = self:load_path(name, v, dir) + local load_path, policy_config_schema, invalid_paths = self:load_path(name, v, dir) local loader = sandbox.new(load_path and { load_path } or invalid_paths) @@ -107,7 +129,9 @@ function _M:call(name, version, dir) -- passing the "exclusive" flag for the require so it does not fallback to native require -- it should load only policies and not other code and fail if there is no such policy - return loader('init', true) + local res = loader('init', true) + + return with_config_validator(res, policy_config_schema) end function _M:pcall(name, version, dir) From 76040fa814c55a3e4d609be4d98ceaefeae35a31 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Tue, 6 Mar 2018 12:05:33 +0100 Subject: [PATCH 08/11] spec/policy/caching: remove test that no longer applies Now we have a way to check that we do not use invalid policy configs in tests. --- spec/policy/caching/caching_spec.lua | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/spec/policy/caching/caching_spec.lua b/spec/policy/caching/caching_spec.lua index 4b51670b6..419d19a00 100644 --- a/spec/policy/caching/caching_spec.lua +++ b/spec/policy/caching/caching_spec.lua @@ -24,16 +24,6 @@ describe('Caching policy', function() ctx.cache_handler(cache, 'a_key', { status = 200 }, nil) assert.is_nil(cache:get('a_key')) end) - - it('disables caching when invalid caching type is specified', function() - local config = { caching_type = 'invalid_caching_type' } - local caching_policy = require('apicast.policy.caching').new(config) - local ctx = {} - caching_policy:rewrite(ctx) - - ctx.cache_handler(cache, 'a_key', { status = 200 }, nil) - assert.is_nil(cache:get('a_key')) - end) end) describe('.access', function() From 786e1489a0e18b3a6fcedfad73a2777d995d6f11 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Wed, 7 Mar 2018 11:26:28 +0100 Subject: [PATCH 09/11] policy_loader: make policy configs validation optional We only want to use this option in tests. --- gateway/src/apicast/policy_loader.lua | 15 ++++++++++++++- spec/spec_helper.lua | 3 +++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/gateway/src/apicast/policy_loader.lua b/gateway/src/apicast/policy_loader.lua index 72a39d1be..0bb2c95b3 100644 --- a/gateway/src/apicast/policy_loader.lua +++ b/gateway/src/apicast/policy_loader.lua @@ -40,6 +40,15 @@ do end end +-- Returns true if config validation has been enabled via ENV or if we are +-- running Test::Nginx integration tests. We know that the framework always +-- sets TEST_NGINX_BINARY so we can use it to detect whether we are running the +-- tests. +local function policy_config_validation_is_enabled() + return resty_env.enabled('APICAST_VALIDATE_POLICY_CONFIGS') + or resty_env.value('TEST_NGINX_BINARY') +end + local function read_manifest(path) local handle = io.open(format('%s/%s', path, 'apicast-policy.json')) @@ -131,7 +140,11 @@ function _M:call(name, version, dir) -- it should load only policies and not other code and fail if there is no such policy local res = loader('init', true) - return with_config_validator(res, policy_config_schema) + if policy_config_validation_is_enabled() then + return with_config_validator(res, policy_config_schema) + else + return res + end end function _M:pcall(name, version, dir) diff --git a/spec/spec_helper.lua b/spec/spec_helper.lua index a336f7bc4..947e210b5 100644 --- a/spec/spec_helper.lua +++ b/spec/spec_helper.lua @@ -38,6 +38,9 @@ local function reset() previous_env = {} + -- To make sure that we are using valid policy configs in the tests. + set('APICAST_VALIDATE_POLICY_CONFIGS', true) + env.reset() end From d7a689d99f184b02dde397ac85cf218811ce18d4 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Wed, 7 Mar 2018 14:15:21 +0100 Subject: [PATCH 10/11] Roverfile: add our fork of ljsonschema --- gateway/Roverfile | 1 + gateway/Roverfile.lock | 2 ++ 2 files changed, 3 insertions(+) diff --git a/gateway/Roverfile b/gateway/Roverfile index 1250db089..579cc7169 100644 --- a/gateway/Roverfile +++ b/gateway/Roverfile @@ -4,6 +4,7 @@ luarocks { group 'testing' { module { 'busted' }, + module { 'ljsonschema' }, }, group 'development' { diff --git a/gateway/Roverfile.lock b/gateway/Roverfile.lock index 25256f64a..334fee13f 100644 --- a/gateway/Roverfile.lock +++ b/gateway/Roverfile.lock @@ -4,6 +4,7 @@ dkjson 2.5-2||testing inspect 3.1.1-0||production ldoc 1.4.6-2||development liquid 0.1.0-1||production +ljsonschema 0.1.0-1||testing lua-resty-env 0.4.0-1||production lua-resty-execvp 0.1.0-1||production lua-resty-http 0.12-0||production @@ -18,6 +19,7 @@ luassert 1.7.10-0||testing luasystem 0.2.1-0||testing markdown 0.33-1||development mediator_lua 1.1.2-0||testing +net-url 0.9-1||testing nginx-lua-prometheus 0.20171117-4||production penlight 1.5.4-1||production,development,testing router 2.1-0||production From 8c2df394c55038dc10127450ba94b36f66c44a3d Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Wed, 7 Mar 2018 14:42:10 +0100 Subject: [PATCH 11/11] CHANGELOG: add entry for validation of policy configs --- CHANGELOG.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86591ac39..2e843a26a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - New property `summary` in the policy manifests [PR #633](https://github.com/3scale/apicast/pull/633) - OAuth2.0 Token Introspection policy [PR #619](https://github.com/3scale/apicast/pull/619) +- New `metrics` phase that runs when prometheus is collecting metrics [PR #629](https://github.com/3scale/apicast/pull/629) +- Validation of policy configs both in integration and unit tests [PR #646](https://github.com/3scale/apicast/pull/646) ## Fixed @@ -18,10 +20,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - 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 - -- New `metrics` phase that runs when prometheus is collecting metrics [PR #629](https://github.com/3scale/apicast/pull/629) - ## [3.2.0-beta1] - 2018-02-20 ## Added