From 226bd0edd940ec18dafe7aee3f4429ad83c3b95c Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 8 Jun 2018 15:32:14 +0200 Subject: [PATCH 1/5] policy/3scale_batcher: use cache handler from the context in case of backend downtime --- .../policy/3scale_batcher/3scale_batcher.lua | 43 ++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua b/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua index 4f76aec38..348d47915 100644 --- a/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua +++ b/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua @@ -1,6 +1,7 @@ local backend_client = require('apicast.backend_client') local AuthsCache = require('auths_cache') local ReportsBatcher = require('reports_batcher') +local keys_helper = require('keys_helper') local policy = require('apicast.policy') local errors = require('apicast.errors') local reporter = require('reporter') @@ -38,6 +39,12 @@ function _M.new(config) end self.semaphore_report_timer = semaphore_report_timer + -- Cache for authorizations to be used in the event of a 3scale backend + -- downtime. + -- This cache allows us to use this policy in combination with the caching + -- one. + self.backend_downtime_cache = ngx.shared.api_keys + return self end @@ -110,17 +117,42 @@ local function error(service, rejection_reason) end end -local function handle_backend_ok(self, transaction) +local function update_downtime_cache(cache, transaction, backend_status, cache_handler) + local key = keys_helper.key_for_cached_auth(transaction) + cache_handler(cache, key, backend_status) +end + +local function handle_backend_ok(self, transaction, cache_handler) + if cache_handler then + update_downtime_cache(self.backend_downtime_cache, transaction, 200, cache_handler) + end + self.auths_cache:set(transaction, 200) self.reports_batcher:add(transaction) end -local function handle_backend_denied(self, service, transaction, status, headers) +local function handle_backend_denied(self, service, transaction, status, headers, cache_handler) + if cache_handler then + update_downtime_cache(self.backend_downtime_cache, transaction, status, cache_handler) + end + local rejection_reason = rejection_reason_from_headers(headers) self.auths_cache:set(transaction, status, rejection_reason) return error(service, rejection_reason) end +local function handle_backend_error(self, service, transaction, cache_handler) + local cached = cache_handler and self.backend_downtime_cache:get(transaction) + + if cached == 200 then + self.reports_batcher:add(transaction) + else + -- The caching policy does not store the rejection reason, so we can only + -- return a generic error. + return error(service) + end +end + -- Note: when an entry in the cache expires, there might be several requests -- with those credentials and all of them will call auth() on backend with the -- same parameters until the auth status is cached again. In the future, we @@ -142,14 +174,15 @@ function _M:access(context) local formatted_usage = format_usage(usage) local backend_res = backend:authorize(formatted_usage, credentials) local backend_status = backend_res.status + local cache_handler = context.cache_handler -- Set by Caching policy if backend_status == 200 then - handle_backend_ok(self, transaction) + handle_backend_ok(self, transaction, cache_handler) elseif backend_status >= 400 and backend_status < 500 then handle_backend_denied( - self, service, transaction, backend_status, backend_res.headers) + self, service, transaction, backend_status, backend_res.headers, cache_handler) else - return error(service) + handle_backend_error(self, service, transaction, cache_handler) end else if cached_auth.status == 200 then From 4453f1e7dfa657a5cfe0304003d55f3598e0ec32 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 8 Jun 2018 15:32:49 +0200 Subject: [PATCH 2/5] spec/policy/3scale_batcher: test that it uses the cache_handler in the context when backend is down --- .../3scale_batcher/3scale_batcher_spec.lua | 75 ++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/spec/policy/3scale_batcher/3scale_batcher_spec.lua b/spec/policy/3scale_batcher/3scale_batcher_spec.lua index 3207cf007..31024cc74 100644 --- a/spec/policy/3scale_batcher/3scale_batcher_spec.lua +++ b/spec/policy/3scale_batcher/3scale_batcher_spec.lua @@ -42,11 +42,17 @@ describe('3scale batcher policy', function() batcher_policy.auths_cache = AuthsCache.new(lrucache.new(10), 10) stub(batcher_policy.reports_batcher, 'add') + stub(batcher_policy, 'backend_downtime_cache') + context = { service = service, usage = usage, - credentials = credentials + credentials = credentials, + -- cache_handler does nothing because we just need to check if it's called + cache_handler = function() end } + + stub(context, 'cache_handler') end) describe('when the request is cached', function() @@ -79,5 +85,72 @@ describe('3scale batcher policy', function() end) end) end) + + describe('when the request is not cached', function() + describe('and backend is available', function() + before_each(function() + local backend_client = require('apicast.backend_client') + stub(backend_client, 'authorize').returns({ status = 200 }) + end) + + it('updates the backend downtime cache using the handler in the context', function() + batcher_policy:access(context) + + assert.stub(context.cache_handler).was_called() + end) + end) + + describe('and backend is not available', function() + before_each(function() + local backend_client = require('apicast.backend_client') + stub(backend_client, 'authorize').returns({ status = 500 }) + end) + + describe('and the authorization is in the downtime cache', function() + describe('and it is OK', function() + before_each(function() + stub(batcher_policy.backend_downtime_cache, 'get').returns(200) + end) + + it('adds the report to the batcher', function() + batcher_policy:access(context) + + assert.stub(batcher_policy.reports_batcher.add).was_called_with( + batcher_policy.reports_batcher, transaction) + end) + end) + + describe('and it is denied', function() + before_each(function() + stub(batcher_policy.backend_downtime_cache, 'get').returns(409) + end) + + it('does not add the report to the batcher', function() + batcher_policy:access(context) + + assert.stub(batcher_policy.reports_batcher.add).was_not_called() + end) + + it('returns an error', function() + batcher_policy:access(context) + + assert.is_true(ngx.status >= 400 and ngx.status < 500) + end) + end) + end) + + describe('and the authorization is not in the downtime cache', function() + before_each(function() + stub(batcher_policy.backend_downtime_cache, 'get').returns(nil) + end) + + it('returns an error', function() + batcher_policy:access(context) + + assert.is_true(ngx.status >= 400 and ngx.status < 500) + end) + end) + end) + end) end) end) From fa69e6a44f8aceeb5e5f21d6d37c2500863719b2 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 8 Jun 2018 16:48:05 +0200 Subject: [PATCH 3/5] t/apicast-policy-3scale-batcher: test in combination with caching policy --- t/apicast-policy-3scale-batcher.t | 84 +++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/t/apicast-policy-3scale-batcher.t b/t/apicast-policy-3scale-batcher.t index 262b51c70..db8b62234 100644 --- a/t/apicast-policy-3scale-batcher.t +++ b/t/apicast-policy-3scale-batcher.t @@ -372,3 +372,87 @@ push $res, "GET /force_report_to_backend"; $res --- no_error_log [error] + +=== TEST 5: with caching policy (resilient mode) +The purpose of this test is to test that the 3scale batcher policy works +correctly when combined with the caching one. +In this case, the caching policy is configured as "resilient". We define a +backend that returns "limits exceeded" on the first request, and an error in +all the rest. The caching policy will cache the first result and return it +while backend is down. Notice that the caching policy does not store the +rejection reason, it just returns a generic error (403/Authentication failed). +To make sure that nothing is cached in the 3scale batcher policy, we flush its +auth cache on every request (see rewrite_by_lua_block). +--- http_config +include $TEST_NGINX_UPSTREAM_CONFIG; +lua_shared_dict api_keys 10m; +lua_shared_dict cached_auths 1m; +lua_shared_dict batched_reports 1m; +lua_shared_dict batched_reports_locks 1m; +lua_package_path "$TEST_NGINX_LUA_PATH"; + +init_by_lua_block { + require('apicast.configuration_loader').mock({ + services = { + { + id = 42, + backend_version = 1, + backend_authentication_type = 'service_token', + backend_authentication_value = 'token-value', + proxy = { + backend = { endpoint = "http://127.0.0.1:$TEST_NGINX_SERVER_PORT" }, + api_backend = "http://127.0.0.1:$TEST_NGINX_SERVER_PORT/api-backend/", + proxy_rules = { + { pattern = '/', http_method = 'GET', metric_system_name = 'hits', delta = 2 } + }, + policy_chain = { + { + name = 'apicast.policy.3scale_batcher', + configuration = { } + }, + { + name = 'apicast.policy.apicast' + }, + { + name = 'apicast.policy.caching', + configuration = { caching_type = 'resilient' } + } + } + } + } + } + }) +} + +rewrite_by_lua_block { + ngx.shared.cached_auths:flush_all() +} +--- config + include $TEST_NGINX_APICAST_CONFIG; + + location /transactions/authorize.xml { + content_by_lua_block { + local test_counter = ngx.shared.test_counter or 0 + if test_counter == 0 then + ngx.shared.test_counter = test_counter + 1 + ngx.header['3scale-rejection-reason'] = 'limits_exceeded' + ngx.status = 409 + ngx.exit(ngx.HTTP_OK) + else + ngx.shared.test_counter = test_counter + 1 + ngx.exit(502) + end + } + } + + location /api-backend { + echo 'yay, api backend'; + } +--- request eval +["GET /test?user_key=foo", "GET /foo?user_key=foo", "GET /?user_key=foo"] +--- response_body eval +["Limits exceeded", "Authentication failed", "Authentication failed"] +--- error_code eval +[ 429, 403, 403 ] +--- no_error_log +[error] From 920676207452fb4fc7a87b9163c06b7148c53e4f Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 8 Jun 2018 18:08:30 +0200 Subject: [PATCH 4/5] CHANGELOG: add fix for combining the 3scale batching policy with the caching one --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd698c5af..3f33d7ed7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - OpenTracing support [PR #669](https://github.com/3scale/apicast/pull/669) - Generate new policy scaffold from the CLI [PR #682](https://github.com/3scale/apicast/pull/682) -- 3scale batcher policy [PR #685](https://github.com/3scale/apicast/pull/685), [PR #710](https://github.com/3scale/apicast/pull/710) +- 3scale batcher policy [PR #685](https://github.com/3scale/apicast/pull/685), [PR #710](https://github.com/3scale/apicast/pull/710), [PR #757](https://github.com/3scale/apicast/pull/757) - Liquid templating support in the headers policy configuration [PR #716](https://github.com/3scale/apicast/pull/716) - Ability to modify query parameters in the URL rewriting policy [PR #724](https://github.com/3scale/apicast/pull/724) - 3scale referrer policy [PR #728](https://github.com/3scale/apicast/pull/728) From 4497c7312547a1d60f499ed6fae059cf51a4e70a Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Mon, 11 Jun 2018 11:35:02 +0200 Subject: [PATCH 5/5] policy/3scale_batcher/keys_helper: require table.new Seems to be needed with the new version of OpenResty. --- gateway/src/apicast/policy/3scale_batcher/keys_helper.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gateway/src/apicast/policy/3scale_batcher/keys_helper.lua b/gateway/src/apicast/policy/3scale_batcher/keys_helper.lua index a8acc61a5..0acab98e7 100644 --- a/gateway/src/apicast/policy/3scale_batcher/keys_helper.lua +++ b/gateway/src/apicast/policy/3scale_batcher/keys_helper.lua @@ -5,7 +5,7 @@ local concat = table.concat local sort = table.sort local unpack = table.unpack local ngx_re = ngx.re -local table = table +local new_tab = require('resty.core.base').new_tab local _M = {} @@ -20,7 +20,7 @@ local function creds_part_in_key(creds) end local function metrics_part_in_key(usage) - local usages = table.new(#usage.metrics, 0) + local usages = new_tab(#usage.metrics, 0) local deltas = usage.deltas