Merge pull request #539 from 3scale/post-action-phase

[policy] trigger post_action phase

CHANGELOG.md

@@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Fix loading renamed APIcast code [PR #525](https://github.com/3scale/apicast/pull/525)
 - Fix `apicast` command when installed from luarocks [PR #527](https://github.com/3scale/apicast/pull/527)
 - Fix lua docs formatting in the CORS policy [PR #530](https://github.com/3scale/apicast/pull/530)
+- `post_action` phase not being called in the policy_chain [PR #539](https://github.com/3scale/apicast/pull/539)
 
 ## Changed
 
@@ -31,6 +32,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Add `src` directory to the Lua load path when using CLI [PR #533](https://github.com/3scale/apicast/pull/533)
 - Move rejection reason parsing from CacheHandler to Proxy [PR #541](https://github.com/3scale/apicast/pull/541)
 - Propagate full package.path and cpath from the CLI to Nginx [PR #538](https://github.com/3scale/apicast/pull/538)
+- `post_action` phase now shares `ngx.ctx` with the main request [PR #539](https://github.com/3scale/apicast/pull/539)
 
 ## [3.2.0-alpha2] - 2017-11-30
 

Vagrantfile

@@ -21,6 +21,8 @@ Vagrant.configure("2") do |config|
   # your network.
   config.vm.network "private_network", type: 'dhcp'
 
+  config.vm.network "forwarded_port", guest: 8080, host: 8080, auto_correct: true
+
   # Share an additional folder to the guest VM. The first argument is
   # the path on the host to the actual folder. The second argument is
   # the path on the guest to mount the folder. And the optional third
@@ -41,10 +43,10 @@ Vagrant.configure("2") do |config|
   config.vm.synced_folder ".", "/vagrant", type: 'virtualbox'
 
   config.vm.synced_folder ".", "/home/vagrant/app", type: 'rsync',
-    rsync__exclude: %w[lua_modules .git .vagrant],
+    rsync__exclude: %w[lua_modules .git .vagrant node_modules t/servroot t/servroot* ],
     rsync__args: %w[--verbose --archive --delete -z --links ]
 
-  config.vm.provision "shell", inline: <<-'SHELL'
+  config.vm.provision "shell", inline: <<~'SHELL'
      set -x -e
      dnf -y install dnf-plugins-core
 
@@ -59,6 +61,8 @@ Vagrant.configure("2") do |config|
      yum -y groupinstall 'Development Tools'
      yum -y install openssl-devel libev-devel
 
+     dnf debuginfo-install -y kernel-core-$(uname -r)
+
      # Clone various utilities
      git clone https://github.com/openresty/stapxx.git /usr/local/stapxx || (cd /usr/local/stapxx && git pull)
      git clone https://github.com/brendangregg/FlameGraph.git /usr/local/flamegraph || (cd /usr/local/flamegraph && git pull)
@@ -111,14 +115,25 @@ Vagrant.configure("2") do |config|
 
      # Start redis needed for tests
      systemctl  start redis
+     systemctl disable openresty
+     systemctl stop openresty
 SHELL
 
-  config.vm.provision 'shell', privileged: false, name: "Install APIcast dependencies", inline: <<-'SHELL'
+  config.vm.provision 'shell', privileged: false, name: "Install APIcast dependencies", inline: <<~'SHELL'
     set -x -e
     pip install --user hererocks
     pushd app
     hererocks lua_modules -r^ -l 5.1 --no-readline
     curl -L https://raw.githubusercontent.com/3scale/s2i-openresty/ffb1c55533be866a97466915d7ef31c12bae688c/site_config.lua -o lua_modules/share/lua/5.1/luarocks/site_config.lua
     make dependencies cpan
+
+    mkdir -p ~/.systemtap
+    # needed for complete backtraces
+    # increase this if you start seeing stacks collapsed in impossible ways
+    # also try https://github.com/openresty/stapxx/commit/59ba231efba8725a510cd8d1d585aedf94670404
+    # to avoid MAXACTTION problems
+    cat <<- EOF > ~/.systemtap/rc
+    -D MAXSTRINGLEN=1024
+    EOF
 SHELL
 end

doc/performance.md

@@ -17,7 +17,7 @@ bin/apicast
 For profiling with stapxx it is recommended to start just one process and worker:
 
 ```shell
-bin/apicast -m off -w 1 -c examples/configuration/echo.json  > /dev/null
+bin/apicast -c examples/configuration/echo.json  > /dev/null
 ```
 
 Then by opening another terminal you can use vegeta to create traffic:
@@ -35,5 +35,5 @@ wrk --connections 100 --threads 10 --duration 300 'http://localhost:8080/?user_k
 And in another terminal you can create flamegraphs:
 
 ```shell
-lj-lua-stacks.sxx  -x `pgrep openresty` --skip-badvars --arg time=30 | fix-lua-bt - | stackcollapse-stap.pl | flamegraph.pl > /vagrant/graph.svg
+lj-lua-stacks.sxx  -x `pgrep -f 'nginx: worker'` --skip-badvars --arg time=10 | fix-lua-bt - | stackcollapse-stap.pl | flamegraph.pl > /vagrant/graph.svg
 ```

gateway/conf.d/apicast.conf

@@ -48,7 +48,10 @@ location @out_of_band_authrep_action {
 
   set_by_lua $original_request_time 'return ngx.var.request_time';
 
-  content_by_lua_block { require('apicast.executor'):post_action() }
+  content_by_lua_block {
+    require('resty.ctx').apply()
+    require('apicast.executor'):post_action()
+ }
 
   log_by_lua_block {
     ngx.var.post_action_impact = ngx.var.request_time - ngx.var.original_request_time
@@ -63,25 +66,24 @@ location / {
   set $service_id null;
   set $proxy_pass null;
   set $secret_token null;
-  set $resp_body null;
-  set $resp_headers null;
-
-  set $client_id null;
-  set $redirect_url null;
 
   set $backend_host 'backend';
-  set $post_action_backend_endpoint '';
   set $backend_authentication_type null;
   set $backend_authentication_value null;
   set $version '';
   set $real_url null;
 
+  set $ctx_ref -1;
+
   set $post_action_impact null;
   set $original_request_id null;
 
   proxy_ignore_client_abort on;
 
-  rewrite_by_lua_block { require('apicast.executor'):rewrite() }
+  rewrite_by_lua_block {
+    require('resty.ctx').stash()
+    require('apicast.executor'):rewrite()
+  }
   access_by_lua_block { require('apicast.executor'):access() }
   body_filter_by_lua_block { require('apicast.executor'):body_filter() }
   header_filter_by_lua_block { require('apicast.executor'):header_filter() }

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

@@ -18,12 +18,6 @@ local mt = {
 --- This is called when APIcast boots the master process.
 function _M.new()
   return setmetatable({
-    -- So there is no way to use ngx.ctx between request and post_action.
-    -- We somehow need to share the instance of the proxy between those.
-    -- This table is used to store the proxy object with unique reqeust id key
-    -- and removed in the post_action. Because it there is just one instance
-    -- of this module in each worker.
-    post_action_proxy = {}
   }, mt)
 end
 
@@ -46,8 +40,6 @@ end
 function _M:rewrite(context)
   ngx.on_abort(self.cleanup)
 
-  ngx.var.original_request_id = ngx.var.request_id
-
   -- load configuration if not configured
   -- that is useful when lua_code_cache is off
   -- because the module is reloaded and has to be configured again
@@ -57,45 +49,29 @@ function _M:rewrite(context)
   ngx.ctx.proxy = p
 end
 
-function _M:post_action()
-  local request_id = ngx.var.original_request_id
-  local post_action_proxy = self.post_action_proxy
-
-  if not post_action_proxy then
-    return nil, 'not initialized'
-  end
-
-  local p = ngx.ctx.proxy or post_action_proxy[request_id]
-
-  post_action_proxy[request_id] = nil
+function _M:post_action(context)
+  local p = context and context.proxy or ngx.ctx.proxy or self.proxy
 
   if p then
     return p:post_action()
   else
-    ngx.log(ngx.INFO, 'could not find proxy for request id: ', request_id)
+    ngx.log(ngx.ERR, 'could not find proxy for request')
     return nil, 'no proxy for request'
   end
 end
 
 function _M:access(context)
-  local p = ngx.ctx.proxy
+  local p = context and context.proxy or ngx.ctx.proxy or self.proxy
   ngx.ctx.service = context.service
-  local post_action_proxy = self.post_action_proxy
-
-  if not post_action_proxy then
-    return nil, 'not initialized'
-  end
 
   local access, handler = p:call(context.service) -- proxy:access() or oauth handler
 
   local ok, err
 
   if access then
     ok, err = access()
-    post_action_proxy[ngx.var.original_request_id] = p
   elseif handler then
     ok, err = handler()
-    -- no proxy because that would trigger post action
   end
 
   return ok, err

gateway/src/resty/ctx.lua

@@ -0,0 +1,84 @@
+--- resty.ctx
+-- Module for sharing ngx.ctx to subrequests.
+-- @module resty.ctx
+
+local ffi = require 'ffi'
+local debug = require 'debug'
+local base = require "resty.core.base"
+
+-- to get FFI definitions
+require 'resty.core.ctx'
+
+local registry = debug.getregistry()
+local getfenv = getfenv
+local C = ffi.C
+local FFI_NO_REQ_CTX = base.FFI_NO_REQ_CTX
+local error = error
+local tonumber = tonumber
+
+local _M = {
+}
+
+--- Return ctx reference number
+-- @raise no request found, no request ctx found
+-- @treturn int
+function _M.ref()
+  local r = getfenv(0).__ngx_req
+
+  if not r then
+    return error("no request found")
+  end
+
+  local _ = ngx.ctx -- load context
+
+  local ctx_ref = C.ngx_http_lua_ffi_get_ctx_ref(r)
+
+  if ctx_ref == FFI_NO_REQ_CTX then
+    return error("no request ctx found")
+  end
+
+  -- The context should not be garbage collected until all the subrequests are completed.
+  -- That includes internal redirects and post action.
+
+  return ctx_ref
+end
+
+_M.var = 'ctx_ref'
+
+--- Store ctx reference in ngx.var
+-- @tparam ?string var variable name, defaults to ctx_ref
+function _M.stash(var)
+  ngx.var[var or _M.var] = _M.ref()
+end
+
+local function get_ctx(ref)
+  local r = getfenv(0).__ngx_req
+
+  if not r then
+    return error("no request found")
+  end
+
+  local ctx_ref = tonumber(ref)
+  if not ctx_ref then
+    return
+  end
+
+  return registry.ngx_lua_ctx_tables[ctx_ref] or error("no request ctx found")
+end
+
+--- Apply stored ctx to the current request
+-- @tparam ?string var variable name, defaults to ctx_ref
+-- @raise no request found
+-- @treturn table
+function _M.apply(var)
+  local ctx = get_ctx(ngx.var[var or _M.var])
+
+  -- this will actually store the reference again
+  -- so each request that gets the context applied will hold own reference
+  -- this is a very safe way to ensure it is not GC'd or released by another requests
+  ngx.ctx = ctx
+
+  return ctx
+end
+
+return _M

t/apicast-policy-chains.t

@@ -65,3 +65,4 @@ running phase: access
 running phase: balancer
 running phase: header_filter
 running phase: body_filter
+running phase: post_action

t/apicast.t

@@ -108,7 +108,7 @@ Content-Type: text/plain; charset=utf-8
 no mapping rules!
 --- error_code: 404
 --- error_log
-could not find proxy for request
+skipping after action, no cached key
 
 === TEST 5: no mapping rules matched configurable error
 The message is configurable and status also.
@@ -136,7 +136,7 @@ GET /?user_key=value
 no mapping rules!
 --- error_code: 412
 --- error_log
-could not find proxy for request
+skipping after action, no cached key
 
 === TEST 6: authentication credentials invalid default error
 There are defaults defined for the error message, the content-type, and the