Add 'allow' mode in the caching policy #558
Conversation
davidor
force-pushed
the
cache-policy-allow-mode
branch
from
5d1c046 to
0a70362
Compare
| 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) |
There was a problem hiding this comment.
What about the race condition between read and write?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Solved in the new commit @mikz
davidor
force-pushed
the
cache-policy-allow-mode
branch
from
b983013 to
a1837ff
Compare
| @@ -38,13 +44,37 @@ local function resilient_handler(cache, cached_key, response, ttl) | |||
| end | |||
| end | |||
|
|
|||
| local function handle_500_allow_mode(cache, cached_key, ttl) | |||
There was a problem hiding this comment.
Does it warrant own function now?
There was a problem hiding this comment.
I don't have a strong preference. I'm inclined to say yes because of the long comment.
| @@ -5,7 +5,7 @@ | |||
| "properties": { | |||
| "exit": { | |||
| "type": "caching_type", | |||
| "enum": ["resilient", "strict", "none"] | |||
| "enum": ["resilient", "strict", "allow", "none"] | |||
There was a problem hiding this comment.
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.
|
Does this replace |
|
@mayorova this does not break compatibility. If this cache policy is not included, users can still use |
This mode 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).
Closes #126