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 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 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/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/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/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/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." } ] 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 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 diff --git a/gateway/src/apicast/policy_loader.lua b/gateway/src/apicast/policy_loader.lua index e233e2dc6..0bb2c95b3 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') @@ -38,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')) @@ -71,35 +82,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 +138,13 @@ 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) + + 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/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() 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) 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 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 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')