Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'allow' mode in the caching policy #558

Merged
merged 5 commits into from
Jan 22, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- URL rewriting policy [PR #529](https://github.com/3scale/apicast/pull/529)
- Liquid template can find files in current folder too [PR #533](https://github.com/3scale/apicast/pull/533)
- `bin/apicast` respects `APICAST_OPENRESTY_BINARY` and `TEST_NGINX_BINARY` environment [PR #540](https://github.com/3scale/apicast/pull/540)
- Caching policy [PR #546](https://github.com/3scale/apicast/pull/546)
- Caching policy [PR #546](https://github.com/3scale/apicast/pull/546), [PR #558](https://github.com/3scale/apicast/pull/558)
- New phase: `content` for generating content or getting the upstream response [PR #535](https://github.com/3scale/apicast/pull/535)

## Fixed
Expand Down
34 changes: 33 additions & 1 deletion gateway/src/apicast/policy/caching/policy.lua
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@
-- invalidate the cache. This allows us to authorize and deny calls
-- according to the result of the last request made even when backend is
-- down.
-- - Allow: caches authorized and denied calls. When backend is unavailable,
-- it will cache an authorization. In practice, this means that when
-- backend is down _any_ request will be authorized unless last call to
-- backend for that request returned 'deny' (status code = 4xx).
-- Make sure to understand the implications of that before using this mode.
-- It makes sense only in very specific use cases.
-- - None: disables caching.

local policy = require('apicast.policy')
Expand Down Expand Up @@ -38,13 +44,39 @@ local function resilient_handler(cache, cached_key, response, ttl)
end
end

local function handle_500_allow_mode(cache, cached_key, ttl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it warrant own function now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference. I'm inclined to say yes because of the long comment.

local current_value = cache:get(cached_key)
local cached_4xx = current_value and current_value >= 400 and current_value < 500

if not cached_4xx then
ngx.log(ngx.WARN, 'Backend seems to be unavailable. "Allow" mode is ',
'enabled in the cache policy, so next request will be ',
'authorized')
cache:set(cached_key, 200, ttl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the race condition between read and write?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Unfortunately, I don't see any 'compare-and-set' operation in the API of ngx.shared.dict: https://github.com/openresty/lua-nginx-module#ngxshareddict

I see that there's this library: https://github.com/openresty/lua-resty-lock but that would mean introducing a dependency just for this policy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidor lua-resty-lock is part of the openresty bundle, so it is part of stdlib.

I think we could take some time and think about how to do this without the lock anyway. Possibly using the add operation. If the stdict value would be nil, we could add 200.
Not sure how exactly that should work, but I see it as the only option without using the lock because it is the only compare-and-set atomic (compare to nil) operation on shdict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that add would work in this case. There are only 3 possibilities:

  • If there was a 2XX, we don't need to overwrite it with the same thing.
  • If there was a 4XX, we don't need to write anything.
  • If it was nil, we need to write a 200.

Let me try this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved in the new commit @mikz

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

end
end

local function allow_handler(cache, cached_key, response, ttl)
local status = response.status

if status and status < 500 then
ngx.log(ngx.INFO, 'apicast cache write key: ', cached_key,
' status: ', status, ', ttl: ', ttl)

cache:set(cached_key, status, ttl or 0)
else
handle_500_allow_mode(cache, cached_key, ttl or 0)
end
end

local function disabled_cache_handler()
ngx.log(ngx.DEBUG, 'Caching is disabled. Skipping cache handler.')
end

local handlers = {
resilient = resilient_handler,
strict = strict_handler,
allow = allow_handler,
none = disabled_cache_handler
}

Expand All @@ -66,7 +98,7 @@ end

--- Initialize a Caching policy.
-- @tparam[opt] table config
-- @field caching_type Caching type (strict, resilient)
-- @field caching_type Caching type (strict, resilient, allow, none)
function _M.new(config)
local self = new()
self.cache_handler = handler(config or {})
Expand Down
2 changes: 1 addition & 1 deletion gateway/src/apicast/policy/caching/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"properties": {
"exit": {
"type": "caching_type",
"enum": ["resilient", "strict", "none"]
"enum": ["resilient", "strict", "allow", "none"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to change anything, but I wonder if there is a way to describe each individual enum value so we can show it nicely in the form.

}
}
}
43 changes: 43 additions & 0 deletions spec/policy/caching/policy_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,49 @@ describe('policy', function()
end)
end)

describe('when configured as allow', function()
local caching_policy
local cache
local ctx -- the caching policy will add the handler here

before_each(function()
local config = { caching_type = 'allow' }
caching_policy = require('apicast.policy.caching').new(config)
ctx = { }
caching_policy:rewrite(ctx)
cache = resty_lrucache.new(1)
end)

it('caches authorized requests', function()
ctx.cache_handler(cache, 'a_key', { status = 200 }, nil)
assert.equals(200, cache:get('a_key'))
end)

it('caches denied requests', function()
ctx.cache_handler(cache, 'a_key', { status = 403 }, nil)
assert.equals(403, cache:get('a_key'))
end)

describe('and backend returns 5XX', function()
it('does not invalidate the cache entry if there was a 4XX', function()
cache:set('a_key', 403)
ctx.cache_handler(cache, 'a_key', { status = 500 }, nil)
assert.equals(403, cache:get('a_key'))
end)

it('caches a 200 if there was nothing in the cache entry', function()
ctx.cache_handler(cache, 'a_key', { status = 500 }, nil)
assert.equals(200, cache:get('a_key'))
end)

it('caches a 200 if there was something != 4XX in the cache entry', function()
cache:set('a_key', 200)
ctx.cache_handler(cache, 'a_key', { status = 500 }, nil)
assert.equals(200, cache:get('a_key'))
end)
end)
end)

describe('when disabled', function()
local caching_policy
local cache
Expand Down
102 changes: 102 additions & 0 deletions t/apicast-policy-caching.t
Original file line number Diff line number Diff line change
Expand Up @@ -175,3 +175,105 @@ auth error in the odd ones.
["yay, api backend\x{0a}", "Authentication failed", "yay, api backend\x{0a}", "Authentication failed"]
--- error_code eval
[ 200, 403, 200, 403 ]

=== TEST 4: Caching policy configured as 'allow' with unseen request
When the cache is configured as 'allow', all requests after the first one will
be authorized when backend returns a 5XX if they do not have a 'denied' entry
in the cache.
--- configuration
{
"services": [
{
"id": 42,
"backend_version": 1,
"backend_authentication_type": "service_token",
"backend_authentication_value": "token-value",
"proxy": {
"policy_chain": [
{
"name": "apicast.policy.caching",
"configuration": { "caching_type": "allow" }
},
{
"name": "apicast.policy.apicast"
}
],
"api_backend": "http://test:$TEST_NGINX_SERVER_PORT/",
"proxy_rules": [
{ "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
]
}
}
]
}
--- backend
location /transactions/authrep.xml {
content_by_lua_block {
ngx.exit(502)
}
}
--- upstream
location / {
echo 'yay, api backend';
}
--- request eval
["GET /?user_key=uk1", "GET /?user_key=uk1", "GET /?user_key=uk1"]
--- response_body eval
["Authentication failed", "yay, api backend\x{0a}", "yay, api backend\x{0a}"]
--- error_code eval
[403, 200, 200]

=== TEST 5: Caching policy configured as 'allow' with previously denied request
When the cache is configured as 'allow', requests will be denied if the last
successful request to backend returned 'denied'.
In order to test this, we use a backend that returns 403 on the first call, and
502 on the rest.
--- configuration
{
"services": [
{
"id": 42,
"backend_version": 1,
"backend_authentication_type": "service_token",
"backend_authentication_value": "token-value",
"proxy": {
"policy_chain": [
{
"name": "apicast.policy.caching",
"configuration": { "caching_type": "allow" }
},
{
"name": "apicast.policy.apicast"
}
],
"api_backend": "http://test:$TEST_NGINX_SERVER_PORT/",
"proxy_rules": [
{ "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
]
}
}
]
}
--- backend
location /transactions/authrep.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.exit(403)
else
ngx.shared.test_counter = test_counter + 1
ngx.exit(502)
end
}
}
--- upstream
location / {
echo 'yay, api backend';
}
--- request eval
["GET /?user_key=uk1", "GET /?user_key=uk1", "GET /?user_key=uk1"]
--- response_body eval
["Authentication failed", "Authentication failed", "Authentication failed"]
--- error_code eval
[403, 403, 403]