Cache policy #546

davidor · 2018-01-09T16:29:42Z

First version of the caching policy.

The functionality is almost the same as the behavior that can be achieved using APICAST_BACKEND_CACHE_HANDLER. Apart from the two modes supported (strict, resilient), this policy adds the possibility of configure Apicast to not use a cache and make all the calls synchronously.

This does not break compatibility. If this policy is not included, users can still use APICAST_BACKEND_CACHE_HANDLER.

This PR duplicates some of the code in cache_handler.lua. I think that's fine because we'll get rid of the duplication once we split Apicast into smaller policies.

mikz · 2018-01-09T16:31:43Z

gateway/src/apicast/policy/caching/schema.json

+  "properties": {
+    "exit": {
+      "type": "caching_type",
+      "enum": ["resilient", "strict"]


We could offer none as suggested in https://issues.jboss.org/browse/THREESCALE-587.

mikz · 2018-01-15T12:15:36Z

gateway/src/apicast/backend/cache_handler.lua

    -- cached_key is set in post_action and it is in in authorize
    -- so to not write the cache twice lets write it just in authorize

    if fetch_cached_key(cached_key) ~= cached_key then
-      ngx.log(ngx.INFO, 'apicast cache write key: ', cached_key, ', ttl: ', ttl, ' sub: ')
+      ngx.log(ngx.INFO, 'apicast cache write key: ', cached_key,
+                        ', ttl: ', ttl, ' sub: ')


Hm. The sub: there does not really say anything. I guess 3e can remove it as it is some copy paste error.

Ah you're right. I didn't pay attention to this line and then copied it to the new policy.

mikz · 2018-01-15T12:17:06Z

gateway/src/apicast/policy/caching/policy.lua

+local function strict_handler(cache, cached_key, status_code, ttl)
+  if status_code == 200 then
+    ngx.log(ngx.INFO, 'apicast cache write key: ', cached_key,
+                      ', ttl: ', ttl, ' sub: ')


Here is also sub: without anything after it.

mikz · 2018-01-15T12:18:11Z

gateway/src/apicast/policy/caching/policy.lua

+end
+
+local function disabled_cache_handler()
+  return function() end


This is going to allocate function every time this is executed. It is not necessary no?

Right. We can choose one in the initializer.

This line can be removed, right?
Or maybe we could add a debug log line saying that it skipped cache.

mikz · 2018-01-15T12:19:04Z

gateway/src/apicast/policy/caching/policy.lua

+  if not config.caching_type then
+    ngx.log(ngx.ERR, 'Caching type not specified. Cache disabled.')
+    res.caching_type = 'none'
+  elseif config.caching_type == 'resilient' or


This would be nicer by checking a key in a table no?

mikz · 2018-01-15T12:23:11Z

gateway/src/apicast/policy/caching/policy.lua

+    return
+  end
+
+  context.proxy.cache_handler = handler(self.config.caching_type)


What if this policy would just export the correct key in :export() method?
Then the APIcast policy could set the cache_handler to the proxy if it is in the context.

Then the "ownership" of the proxy object would still be in the APIcast module.
I'd be a bit worried that this is pretty deep access into internals of a module from some unrelated part of code. APIcast module initializes the Proxy object, so it could pass the cache handler as an argument for example.

Definitely. I was thinking about the phases (rewrite, access, etc.) and forgot about export().

It turns out that export() is not working correctly for policies included in the local policy chain.

I suggest we address that problem on a separate PR. I've been investigating a bit and didn't find a straightforward solution.

To address your concern, in the rewrite phase of the cache policy, I included the cache_handler in the context. Later, the Apicast policy checks if it has been included, and if so, overrides the cache_handler method of the instance of Proxy that it owns.

mikz · 2018-01-15T12:25:11Z

gateway/src/apicast/backend/cache_handler.lua

@@ -34,28 +34,29 @@ local function fetch_cached_key()
  return ok and stored
 end

-function _M.handlers.strict(cache, cached_key, response, ttl)
-  if response.status == 200 then
+function _M.handlers.strict(cache, cached_key, status_code, ttl)


This is breaking change for someone who written a custom handler, right?

Yes. Didn't think about users overridding this method.

This is based on the current CacheHandler module. Many parts of the code are duplicated, but this duplication will disappear once we split the current Apicast policy into smaller ones. This does not break the current Apicast cache. Users can still use the APICAST_BACKEND_CACHE_HANDLER to configure the cache.

davidor · 2018-01-17T11:24:17Z

@mikz I addressed your comments. Ready for review.

mikz

Looking good!

I think it would be good to address super minor: https://github.com/3scale/apicast/pull/546/files#r162066396

mikz · 2018-01-18T10:06:11Z

gateway/src/apicast/policy/caching/policy.lua

+local handlers = {
+  resilient = resilient_handler,
+  strict = strict_handler,
+  none = disabled_cache_handler


@davidor we also should have "allow" handler that will allow the the request when backend is down.

octobot assigned davidor Jan 9, 2018

mikz reviewed Jan 9, 2018

View reviewed changes

davidor force-pushed the cache-policy branch 4 times, most recently from 69cf214 to 37f2f06 Compare January 15, 2018 11:14

davidor changed the title ~~[WIP] Cache policy~~ Cache policy Jan 15, 2018

mikz reviewed Jan 15, 2018

View reviewed changes

davidor force-pushed the cache-policy branch 2 times, most recently from f3b4837 to f449e36 Compare January 15, 2018 17:53

davidor changed the title ~~Cache policy~~ [WIP] Cache policy Jan 15, 2018

davidor added 3 commits January 17, 2018 11:54

policy/caching: add json schema

fb3215a

t: add tests for caching policy

c110c0c

davidor force-pushed the cache-policy branch from f449e36 to c2c8f65 Compare January 17, 2018 11:13

spec: add tests for caching policy

add2112

davidor force-pushed the cache-policy branch from c2c8f65 to cd8fd1c Compare January 17, 2018 11:22

davidor changed the title ~~[WIP] Cache policy~~ Cache policy Jan 17, 2018

davidor requested a review from mikz January 17, 2018 11:24

mikz approved these changes Jan 17, 2018

View reviewed changes

davidor added 4 commits January 17, 2018 15:40

policy/caching: add 'none' cache type

7e2aaef

policy/spec: add tests with disabled cache

661b9df

t/apicast-policy-caching: test disabled cache

0fd95c5

CHANGELOG: add caching policy entry

3b00ece

davidor added 2 commits January 17, 2018 15:40

cache_handler: remove empty log part

4cf52d4

policy/apicast: override proxy's cache_handler if received via context

2016d41

davidor force-pushed the cache-policy branch from cd8fd1c to 2016d41 Compare January 17, 2018 14:41

davidor merged commit db74364 into master Jan 17, 2018

davidor deleted the cache-policy branch January 17, 2018 15:28

mikz reviewed Jan 18, 2018

View reviewed changes

mikz mentioned this pull request Jul 10, 2018

APICast Authentication in seperate policy #805

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache policy #546

Cache policy #546

davidor commented Jan 9, 2018 •

edited

Loading

mikz Jan 9, 2018

davidor Jan 9, 2018

mikz Jan 15, 2018

davidor Jan 15, 2018 •

edited

Loading

mikz Jan 15, 2018

mikz Jan 15, 2018

davidor Jan 15, 2018

mikz Jan 17, 2018

davidor Jan 17, 2018

mikz Jan 15, 2018

mikz Jan 15, 2018

davidor Jan 15, 2018

davidor Jan 17, 2018

mikz Jan 15, 2018

davidor Jan 15, 2018

davidor commented Jan 17, 2018

mikz left a comment

mikz Jan 18, 2018

Cache policy #546

Cache policy #546

Conversation

davidor commented Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor commented Jan 17, 2018

mikz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor commented Jan 9, 2018 •

edited

Loading

davidor Jan 15, 2018 •

edited

Loading