From 4d059408f35a076232eb688c1f32b6981f218391 Mon Sep 17 00:00:00 2001 From: An Tran Date: Fri, 17 Nov 2023 15:55:30 +1000 Subject: [PATCH 1/6] Extract @upstream content to a seperate file This commit extract @upstream config to a sepearate file so we can re-use the generic config for different upstream block by including the upstream_shared.conf file --- gateway/conf.d/apicast.conf | 65 +--------------------------- gateway/conf.d/upstream_shared.conf | 66 +++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 64 deletions(-) create mode 100644 gateway/conf.d/upstream_shared.conf diff --git a/gateway/conf.d/apicast.conf b/gateway/conf.d/apicast.conf index e8a0aa845..2feed1d65 100644 --- a/gateway/conf.d/apicast.conf +++ b/gateway/conf.d/apicast.conf @@ -78,71 +78,8 @@ location @upstream { require('resty.ctx').apply() } - #{% capture proxy_cache_valid %} - #{#} proxy_cache $cache_zone; - #{#} proxy_cache_key $scheme$request_method$proxy_host$request_uri$service_id; - #{#} proxy_no_cache $cache_request; - #{#} proxy_cache_valid {{ env.APICAST_CACHE_STATUS_CODES | default: '200 302'}} {{ env.APICAST_CACHE_MAX_TIME | default: '1m' }}; - #{% endcapture %} - #{{ proxy_cache_valid | replace: "#{#}", "" }} - # - - #{% if opentelemetry != empty %} - # {% capture opentelemetry_propagate_directive %} - #{#} opentelemetry_propagate; - # {% endcapture %} - # {{ opentelemetry_propagate_directive | replace: "#{#}", "" }} - #{% endif %} - - proxy_pass $proxy_pass; + #{% include "conf.d/upstream_shared.conf" %} - proxy_http_version 1.1; - proxy_set_header X-Real-IP $remote_addr; - proxy_set_header Host $http_host; - proxy_set_header X-3scale-proxy-secret-token $secret_token; - proxy_set_header X-3scale-debug ""; - proxy_set_header Connection $upstream_connection_header; - proxy_set_header Upgrade $upstream_upgrade_header; - - # This is a bit tricky. It uses liquid to set a SSL client certificate. In - # NGINX, all this is not executed as it is commented with '#'. However, in - # Liquid, all this will be evaluated. As a result, the following directives - # are set optionally: proxy_ssl_certificate, proxy_ssl_certificate_key, - # proxy_ssl_session_reuse, and proxy_ssl_password_file. - - # {% if proxy_ssl_certificate != empty and proxy_ssl_certificate_key != empty %} - # {% capture proxy_ssl %} - #{#} proxy_ssl_certificate {{ proxy_ssl_certificate }}; - #{#} proxy_ssl_certificate_key {{ proxy_ssl_certificate_key }}; - # {% endcapture %} - # {{ proxy_ssl | replace: "#{#}", "" }} - # - # {% if proxy_ssl_password_file != empty %} - # {% capture proxy_ssl %} - #{#} proxy_ssl_password_file {{ proxy_ssl_password_file }}; - # {% endcapture %} - # {{ proxy_ssl | replace: "#{#}", "" }} - # {% endif %} - # - # {% if proxy_ssl_session_reuse != empty %} - # {% capture proxy_ssl %} - #{#} proxy_ssl_session_reuse {{ proxy_ssl_session_reuse }}; - # {% endcapture %} - # {{ proxy_ssl | replace: "#{#}", "" }} - # {% endif %} - # {% endif %} - - # When 'upstream_retry_cases' is empty, apply the same default as NGINX. - # If the proxy_next_upstream directive is not declared, the retry policy - # will never retry. - # {% if upstream_retry_cases != empty %} - # {% capture proxy_next_upstream %} - #{#} proxy_next_upstream {{ upstream_retry_cases }}; - # {% endcapture %} - # {{ proxy_next_upstream | replace: "#{#}", "" }} - # {% else %} - # proxy_next_upstream error timeout; - # {% endif %} # these are duplicated so when request is redirected here those phases are executed post_action @out_of_band_authrep_action; body_filter_by_lua_block { require('apicast.executor'):body_filter() } diff --git a/gateway/conf.d/upstream_shared.conf b/gateway/conf.d/upstream_shared.conf new file mode 100644 index 000000000..1cce2a225 --- /dev/null +++ b/gateway/conf.d/upstream_shared.conf @@ -0,0 +1,66 @@ +#{% capture proxy_cache_valid %} +#{#} proxy_cache $cache_zone; +#{#} proxy_cache_key $scheme$request_method$proxy_host$request_uri$service_id; +#{#} proxy_no_cache $cache_request; +#{#} proxy_cache_valid {{ env.APICAST_CACHE_STATUS_CODES | default: '200 302'}} {{ env.APICAST_CACHE_MAX_TIME | default: '1m' }}; +#{% endcapture %} +#{{ proxy_cache_valid | replace: "#{#}", "" }} +# + +#{% if opentelemetry != empty %} +# {% capture opentelemetry_propagate_directive %} +#{#} opentelemetry_propagate; +# {% endcapture %} +# {{ opentelemetry_propagate_directive | replace: "#{#}", "" }} +#{% endif %} + +proxy_pass $proxy_pass; + +proxy_http_version 1.1; + +proxy_set_header X-Real-IP $remote_addr; +proxy_set_header Host $http_host; +proxy_set_header X-3scale-proxy-secret-token $secret_token; +proxy_set_header X-3scale-debug ""; +proxy_set_header Connection $upstream_connection_header; +proxy_set_header Upgrade $upstream_upgrade_header; + +# This is a bit tricky. It uses liquid to set a SSL client certificate. In +# NGINX, all this is not executed as it is commented with '#'. However, in +# Liquid, all this will be evaluated. As a result, the following directives +# are set optionally: proxy_ssl_certificate, proxy_ssl_certificate_key, +# proxy_ssl_session_reuse, and proxy_ssl_password_file. + +# {% if proxy_ssl_certificate != empty and proxy_ssl_certificate_key != empty %} +# {% capture proxy_ssl %} +#{#} proxy_ssl_certificate {{ proxy_ssl_certificate }}; +#{#} proxy_ssl_certificate_key {{ proxy_ssl_certificate_key }}; +# {% endcapture %} +# {{ proxy_ssl | replace: "#{#}", "" }} +# +# {% if proxy_ssl_password_file != empty %} +# {% capture proxy_ssl %} +#{#} proxy_ssl_password_file {{ proxy_ssl_password_file }}; +# {% endcapture %} +# {{ proxy_ssl | replace: "#{#}", "" }} +# {% endif %} +# +# {% if proxy_ssl_session_reuse != empty %} +# {% capture proxy_ssl %} +#{#} proxy_ssl_session_reuse {{ proxy_ssl_session_reuse }}; +# {% endcapture %} +# {{ proxy_ssl | replace: "#{#}", "" }} +# {% endif %} +# {% endif %} + +# When 'upstream_retry_cases' is empty, apply the same default as NGINX. +# If the proxy_next_upstream directive is not declared, the retry policy +# will never retry. +# {% if upstream_retry_cases != empty %} +# {% capture proxy_next_upstream %} +#{#} proxy_next_upstream {{ upstream_retry_cases }}; +# {% endcapture %} +# {{ proxy_next_upstream | replace: "#{#}", "" }} +# {% else %} +# proxy_next_upstream error timeout; +# {% endif %} From 204d4837d37ed09e3d297dd0d0eeb74d82620215 Mon Sep 17 00:00:00 2001 From: An Tran Date: Fri, 17 Nov 2023 15:55:37 +1000 Subject: [PATCH 2/6] Add new @upstream blocks for unbuffered request --- gateway/conf.d/apicast.conf | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/gateway/conf.d/apicast.conf b/gateway/conf.d/apicast.conf index 2feed1d65..5cd1ce4e2 100644 --- a/gateway/conf.d/apicast.conf +++ b/gateway/conf.d/apicast.conf @@ -78,6 +78,23 @@ location @upstream { require('resty.ctx').apply() } + proxy_request_buffering on; + #{% include "conf.d/upstream_shared.conf" %} + + # these are duplicated so when request is redirected here those phases are executed + post_action @out_of_band_authrep_action; + body_filter_by_lua_block { require('apicast.executor'):body_filter() } + header_filter_by_lua_block { require('apicast.executor'):header_filter() } +} + +location @upstream_request_unbuffered { + internal; + + rewrite_by_lua_block { + require('resty.ctx').apply() + } + + proxy_request_buffering off; #{% include "conf.d/upstream_shared.conf" %} # these are duplicated so when request is redirected here those phases are executed From 110522c68c51f2b10eaff413ebd7337232d2e6fc Mon Sep 17 00:00:00 2001 From: An Tran Date: Fri, 17 Nov 2023 15:55:43 +1000 Subject: [PATCH 3/6] Introduce request_unbuffered policy This policy allow user to disable request buffering. With this change, the upstream location is changed based on the value provided in the context. --- .../request_unbuffered/apicast-policy.json | 13 ++ .../policy/request_unbuffered/init.lua | 1 + .../request_unbuffered/request_unbuffered.lua | 22 ++ gateway/src/apicast/upstream.lua | 15 +- spec/upstream_spec.lua | 16 ++ t/apicast-policy-request-unbuffered.t | 210 ++++++++++++++++++ 6 files changed, 274 insertions(+), 3 deletions(-) create mode 100644 gateway/src/apicast/policy/request_unbuffered/apicast-policy.json create mode 100644 gateway/src/apicast/policy/request_unbuffered/init.lua create mode 100644 gateway/src/apicast/policy/request_unbuffered/request_unbuffered.lua create mode 100644 t/apicast-policy-request-unbuffered.t diff --git a/gateway/src/apicast/policy/request_unbuffered/apicast-policy.json b/gateway/src/apicast/policy/request_unbuffered/apicast-policy.json new file mode 100644 index 000000000..9b26e5ea7 --- /dev/null +++ b/gateway/src/apicast/policy/request_unbuffered/apicast-policy.json @@ -0,0 +1,13 @@ +{ + "$schema": "http://apicast.io/policy-v1/schema#manifest#", + "name": "Request Unbuffered", + "summary": "Disable request buffering", + "description": [ + "Disable request buffering. This is useful when proxying big payloads with HTTP/1.1 chunked encoding" + ], + "version": "builtin", + "configuration": { + "type": "object", + "properties": {} + } +} diff --git a/gateway/src/apicast/policy/request_unbuffered/init.lua b/gateway/src/apicast/policy/request_unbuffered/init.lua new file mode 100644 index 000000000..b5a678161 --- /dev/null +++ b/gateway/src/apicast/policy/request_unbuffered/init.lua @@ -0,0 +1 @@ +return require('request_unbuffered') diff --git a/gateway/src/apicast/policy/request_unbuffered/request_unbuffered.lua b/gateway/src/apicast/policy/request_unbuffered/request_unbuffered.lua new file mode 100644 index 000000000..a3113dcce --- /dev/null +++ b/gateway/src/apicast/policy/request_unbuffered/request_unbuffered.lua @@ -0,0 +1,22 @@ +-- Request Unbuffered policy +-- This policy will disable request buffering + +local policy = require('apicast.policy') +local _M = policy.new('request_unbuffered') + +local new = _M.new + +--- Initialize a buffering +-- @tparam[opt] table config Policy configuration. +function _M.new(config) + local self = new(config) + return self +end + +function _M:export() + return { + request_unbuffered = true, + } +end + +return _M diff --git a/gateway/src/apicast/upstream.lua b/gateway/src/apicast/upstream.lua index 23bbb2716..0aff47359 100644 --- a/gateway/src/apicast/upstream.lua +++ b/gateway/src/apicast/upstream.lua @@ -210,6 +210,15 @@ function _M:set_keepalive_key(context) end end +local function get_upstream_location_name(context) + if context.upstream_location_name then + return context.upstream_location_name + end + if context.request_unbuffered then + return "@upstream_request_unbuffered" + end +end + --- Execute the upstream. --- @tparam table context any table (policy context, ngx.ctx) to store the upstream for later use by balancer function _M:call(context) @@ -242,9 +251,9 @@ function _M:call(context) self:set_keepalive_key(context or {}) if not self.servers then self:resolve() end - if context.upstream_location_name then - self.location_name = context.upstream_location_name - end + + local upstream_location_name = get_upstream_location_name(context) + self:update_location(upstream_location_name) context[self.upstream_name] = self return exec(self) diff --git a/spec/upstream_spec.lua b/spec/upstream_spec.lua index 343f4bceb..6ab12e432 100644 --- a/spec/upstream_spec.lua +++ b/spec/upstream_spec.lua @@ -217,6 +217,22 @@ describe('Upstream', function() assert.spy(ngx.exec).was_called_with(upstream.location_name) end) + it('executes the upstream location when request_unbuffered provided in the context', function() + local contexts = { + ["buffered_request"] = {ctx={}, upstream_location="@upstream"}, + ["unbuffered_request"] = {ctx={request_unbuffered=true}, upstream_location="@upstream_request_unbuffered"}, + ["upstream_location and buffered_request"] = {ctx={upstream_location_name="@grpc", request_unbuffered=true}, upstream_location="@grpc"}, + ["upstream_location and unbuffered_request"] = {ctx={upstream_location_name="@grpc"}, upstream_location="@grpc"}, + } + + for _, value in pairs(contexts) do + local upstream = Upstream.new('http://localhost') + upstream:call(value.ctx) + + assert.spy(ngx.exec).was_called_with(value.upstream_location) + end + end) + it('skips executing the upstream location when missing', function() local upstream = Upstream.new('http://localhost') upstream.location_name = nil diff --git a/t/apicast-policy-request-unbuffered.t b/t/apicast-policy-request-unbuffered.t new file mode 100644 index 000000000..64d05ff9b --- /dev/null +++ b/t/apicast-policy-request-unbuffered.t @@ -0,0 +1,210 @@ +use lib 't'; +use Test::APIcast::Blackbox 'no_plan'; + +sub large_body { + my $res = ""; + for (my $i=0; $i <= 1024; $i++) { + $res = $res . "1111111 1111111 1111111 1111111\n"; + } + return $res; +} + +$ENV{'LARGE_BODY'} = large_body(); + +require("policies.pl"); + +run_tests(); + +__DATA__ + +=== TEST 1: request_unbuffered policy with big file +--- configuration +{ + "services": [ + { + "backend_version": 1, + "proxy": { + "api_backend": "http://test-upstream.lvh.me:$TEST_NGINX_SERVER_PORT/", + "proxy_rules": [ + { "pattern": "/", "http_method": "POST", "metric_system_name": "hits", "delta": 2 } + ], + "policy_chain": [ + { + "name": "request_unbuffered", + "version": "builtin", + "configuration": {} + }, + { + "name": "apicast", + "version": "builtin", + "configuration": {} + } + ] + } + } + ] +} +--- backend +location /transactions/authrep.xml { + content_by_lua_block { + ngx.exit(200) + } +} +--- upstream +server_name test-upstream.lvh.me; + location / { + echo_read_request_body; + echo_request_body; + } +--- request eval +"POST /?user_key= \n" . $ENV{LARGE_BODY} +--- response_body eval chomp +$ENV{LARGE_BODY} +--- error_code: 200 +--- grep_error_log +a client request body is buffered to a temporary file +--- grep_error_log_out +--- no_error_log +[error] + + + +=== TEST 2: with small chunked request +--- configuration +{ + "services": [ + { + "backend_version": 1, + "proxy": { + "api_backend": "http://test-upstream.lvh.me:$TEST_NGINX_SERVER_PORT/", + "proxy_rules": [ + { "pattern": "/", "http_method": "POST", "metric_system_name": "hits", "delta": 2 } + ], + "policy_chain": [ + { + "name": "request_unbuffered", + "version": "builtin", + "configuration": {} + }, + { + "name": "apicast", + "version": "builtin", + "configuration": {} + } + ] + } + } + ] +} +--- backend +location /transactions/authrep.xml { + content_by_lua_block { + ngx.exit(200) + } +} +--- upstream +server_name test-upstream.lvh.me; + location / { + access_by_lua_block { + assert = require('luassert') + ngx.say("yay, api backend") + + -- Nginx will read the entire body in one chunk, the upstream request will not be chunked + -- and Content-Length header will be added. + local content_length = ngx.req.get_headers()["Content-Length"] + local encoding = ngx.req.get_headers()["Transfer-Encoding"] + assert.equal('12', content_length) + assert.falsy(encoding, "chunked") + } + } +--- more_headers +Transfer-Encoding: chunked +--- request eval +"POST /test?user_key=value +7\r +hello, \r +5\r +world\r +0\r +\r +" +--- error_code: 200 +--- no_error_log +[error] + + + +=== TEST 3: With big chunked request +--- configuration +{ + "services": [ + { + "backend_version": 1, + "proxy": { + "api_backend": "http://test-upstream.lvh.me:$TEST_NGINX_SERVER_PORT/", + "proxy_rules": [ + { "pattern": "/", "http_method": "POST", "metric_system_name": "hits", "delta": 2 } + ], + "policy_chain": [ + { + "name": "request_unbuffered", + "version": "builtin", + "configuration": {} + }, + { + "name": "apicast", + "version": "builtin", + "configuration": {} + } + ] + } + } + ] +} +--- backend +location /transactions/authrep.xml { + content_by_lua_block { + ngx.exit(200) + } +} +--- upstream +server_name test-upstream.lvh.me; + location / { + access_by_lua_block { + assert = require('luassert') + local content_length = ngx.req.get_headers()["Content-Length"] + local encoding = ngx.req.get_headers()["Transfer-Encoding"] + assert.equal('chunked', encoding) + assert.falsy(content_length) + } + echo_read_request_body; + echo_request_body; + } +--- more_headers +Transfer-Encoding: chunked +--- request eval +$::data = ''; +for (my $i = 0; $i < 16384; $i++) { + my $c = chr int rand 128; + $::data .= $c; +} +my $s = "POST https://localhost/test?user_key=value +". +sprintf("%x\r\n", length $::data). +$::data +."\r +0\r +\r +"; +open my $out, '>/tmp/out.txt' or die $!; +print $out $s; +close $out; +$s +--- response_body eval +$::data +--- error_code: 200 +--- grep_error_log +a client request body is buffered to a temporary file +--- grep_error_log_out +--- no_error_log +[error] From c1a7cacd0c25b8ec1630aa603ddd79f4f871ac9f Mon Sep 17 00:00:00 2001 From: An Tran Date: Sat, 18 Nov 2023 22:34:16 +1000 Subject: [PATCH 4/6] Add README file --- .../apicast/policy/request_unbuffered/README.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 gateway/src/apicast/policy/request_unbuffered/README.md diff --git a/gateway/src/apicast/policy/request_unbuffered/README.md b/gateway/src/apicast/policy/request_unbuffered/README.md new file mode 100644 index 000000000..ebc7b3508 --- /dev/null +++ b/gateway/src/apicast/policy/request_unbuffered/README.md @@ -0,0 +1,14 @@ +# APICast Request Unbuffered + +This policy allows to disable request buffering + +## Example configuration + +``` +{ + "name": "request_unbuffered", + "version": "builtin", + "configuration": {} +} +``` + From 26edbf7d79509a78e88dd95253c9491cffdef2c8 Mon Sep 17 00:00:00 2001 From: An Tran Date: Sat, 18 Nov 2023 22:34:38 +1000 Subject: [PATCH 5/6] Update CHANGELOG --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c40d37b49..bea15a552 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,9 +16,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Added - Detect number of CPU shares when running on Cgroups V2 [PR #1410](https://github.com/3scale/apicast/pull/1410) [THREESCALE-10167](https://issues.redhat.com/browse/THREESCALE-10167) -### Added -* Add support to use Basic Authentication with the forward proxy. [PR #1409](https://github.com/3scale/APIcast/pull/1409) +- Add support to use Basic Authentication with the forward proxy. [PR #1409](https://github.com/3scale/APIcast/pull/1409) + +- Added request unbuffered policy [PR #1408](https://github.com/3scale/APIcast/pull/1408) [THREESCALE-9542](https://issues.redhat.com/browse/THREESCALE-9542) ## [3.14.0] 2023-07-25 From 5bda7fce5b43401a2b822e618ff5da107945c195 Mon Sep 17 00:00:00 2001 From: An Tran Date: Mon, 20 Nov 2023 17:02:40 +1000 Subject: [PATCH 6/6] Correct the integration test --- t/apicast-policy-request-unbuffered.t | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/t/apicast-policy-request-unbuffered.t b/t/apicast-policy-request-unbuffered.t index 64d05ff9b..89368661e 100644 --- a/t/apicast-policy-request-unbuffered.t +++ b/t/apicast-policy-request-unbuffered.t @@ -61,9 +61,10 @@ server_name test-upstream.lvh.me; --- response_body eval chomp $ENV{LARGE_BODY} --- error_code: 200 ---- grep_error_log -a client request body is buffered to a temporary file +--- grep_error_log eval +qr/a client request body is buffered to a temporary file/ --- grep_error_log_out +a client request body is buffered to a temporary file --- no_error_log [error] @@ -107,7 +108,6 @@ server_name test-upstream.lvh.me; location / { access_by_lua_block { assert = require('luassert') - ngx.say("yay, api backend") -- Nginx will read the entire body in one chunk, the upstream request will not be chunked -- and Content-Length header will be added. @@ -116,6 +116,8 @@ server_name test-upstream.lvh.me; assert.equal('12', content_length) assert.falsy(encoding, "chunked") } + echo_read_request_body; + echo_request_body; } --- more_headers Transfer-Encoding: chunked @@ -128,6 +130,8 @@ world\r 0\r \r " +--- response_body chomp +hello, world --- error_code: 200 --- no_error_log [error] @@ -203,8 +207,9 @@ $s --- response_body eval $::data --- error_code: 200 ---- grep_error_log -a client request body is buffered to a temporary file +--- grep_error_log eval +qr/a client request body is buffered to a temporary file/ --- grep_error_log_out +a client request body is buffered to a temporary file --- no_error_log [error]