diff --git a/CHANGELOG.md b/CHANGELOG.md index cc0278eb1..9048af187 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Vagrantfile b/Vagrantfile index b14e0cde7..53d8e0e4d 100644 --- a/Vagrantfile +++ b/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 diff --git a/doc/performance.md b/doc/performance.md index 5304697c0..944c06501 100644 --- a/doc/performance.md +++ b/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 ``` diff --git a/gateway/conf.d/apicast.conf b/gateway/conf.d/apicast.conf index 9c9fd5d47..2fc7381c4 100644 --- a/gateway/conf.d/apicast.conf +++ b/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() } diff --git a/gateway/src/apicast/policy/apicast/policy.lua b/gateway/src/apicast/policy/apicast/policy.lua index de8589fb4..da6e45294 100644 --- a/gateway/src/apicast/policy/apicast/policy.lua +++ b/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,34 +49,20 @@ 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 @@ -92,10 +70,8 @@ function _M:access(context) 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 diff --git a/gateway/src/resty/ctx.lua b/gateway/src/resty/ctx.lua new file mode 100644 index 000000000..928f9228e --- /dev/null +++ b/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 diff --git a/t/apicast-policy-chains.t b/t/apicast-policy-chains.t index ff740e3f6..69dd3daf2 100644 --- a/t/apicast-policy-chains.t +++ b/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 diff --git a/t/apicast.t b/t/apicast.t index 4f946606f..9dfc43d83 100644 --- a/t/apicast.t +++ b/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 diff --git a/t/resty-ctx.t b/t/resty-ctx.t new file mode 100644 index 000000000..c09b37db7 --- /dev/null +++ b/t/resty-ctx.t @@ -0,0 +1,132 @@ +use lib 't'; +use Test::APIcast 'no_plan'; + +run_tests(); + +__DATA__ + +=== TEST 1: get context reference +get context reference number +--- http_config + lua_package_path "$TEST_NGINX_LUA_PATH"; +--- config + location = /t { + content_by_lua_block { + ngx.say(require('resty.ctx').ref()) + } + } +--- request +GET /t +--- response_body +1 +--- no_error_log +[error] + + + +=== TEST 2: stash context reference +--- http_config + lua_package_path "$TEST_NGINX_LUA_PATH"; +--- config + set $ctx_ref -1; + + location = /t { + rewrite_by_lua_block { + require('resty.ctx').stash() + } + + content_by_lua_block { + ngx.say(ngx.var.ctx_ref) + } + } +--- request +GET /t +--- response_body +1 +--- no_error_log +[error] + + + +=== TEST 3: apply context reference +--- http_config + lua_package_path "$TEST_NGINX_LUA_PATH"; +--- config + set $ctx_ref -1; + + location = /t { + rewrite_by_lua_block { + require('resty.ctx').stash() + ngx.ctx.foo = 'bar' + } + + content_by_lua_block { + ngx.exec('@redirect') + } + } + + location @redirect { + internal; + + rewrite_by_lua_block { + require('resty.ctx').apply() + ngx.say(ngx.ctx.foo) + } + } +--- request +GET /t +--- response_body +bar +--- no_error_log +[error] + + + +=== TEST 4: context is not garbage collected +--- http_config + lua_package_path "$TEST_NGINX_LUA_PATH"; +--- config + set $ctx_ref -1; + + location = /t { + rewrite_by_lua_block { + require('resty.ctx').stash() + ngx.ctx.foo = 'bar' + } + + content_by_lua_block { + ngx.exec('@redirect') + } + } + + location @redirect { + internal; + + rewrite_by_lua_block { + collectgarbage() + require('resty.ctx').apply() + } + + post_action @out_of_band; + + content_by_lua_block { + collectgarbage() + ngx.say(ngx.ctx.foo) + } + } + + location @out_of_band { + rewrite_by_lua_block { + collectgarbage() + require('resty.ctx').apply() + } + + content_by_lua_block { + collectgarbage() + ngx.say(ngx.ctx.foo) + } + } +--- request +GET /t +--- response_body +bar