From e67281300b8a70f83ad99155669accbf29cef896 Mon Sep 17 00:00:00 2001 From: tzssangglass Date: Thu, 1 Feb 2024 16:22:02 +0800 Subject: [PATCH 1/2] fix(balancer): ensure the notify callback is invoked only if defined when handling cached connection errors address comments of #12346 Signed-off-by: tzssangglass --- ...ua-0.10.20_02-dyn_upstream_keepalive.patch | 32 ++++----------- .../05-proxy/10-balancer/08-retries_spec.lua | 41 +++++++++++++++++++ .../balancer/pure_nginx_balancer.conf | 35 ++++++++++++++++ 3 files changed, 85 insertions(+), 23 deletions(-) create mode 100644 spec/fixtures/balancer/pure_nginx_balancer.conf diff --git a/build/openresty/patches/ngx_lua-0.10.20_02-dyn_upstream_keepalive.patch b/build/openresty/patches/ngx_lua-0.10.20_02-dyn_upstream_keepalive.patch index 0ee6df76ae8..61d3fdfee15 100644 --- a/build/openresty/patches/ngx_lua-0.10.20_02-dyn_upstream_keepalive.patch +++ b/build/openresty/patches/ngx_lua-0.10.20_02-dyn_upstream_keepalive.patch @@ -1,44 +1,32 @@ -From 2d12ac3e4045258b7a174b0505d92f63c26d82fc Mon Sep 17 00:00:00 2001 -From: Thibault Charbonnier -Date: Tue, 17 Sep 2019 11:43:44 -0700 -Subject: [PATCH 1/3] feature: implemented keepalive pooling in - 'balancer_by_lua*'. - ---- - .../nginx-1.19.9/src/http/ngx_http_upstream.c | 1 + - .../nginx-1.19.9/src/http/ngx_http_upstream.h | 2 + - .../src/ngx_http_lua_balancer.c | 848 ++++++++++++++---- - .../ngx_lua-0.10.20/src/ngx_http_lua_common.h | 11 +- - .../ngx_lua-0.10.20/src/ngx_http_lua_module.c | 3 + - 5 files changed, 689 insertions(+), 176 deletions(-) - diff --git a/bundle/nginx-1.19.9/src/http/ngx_http_upstream.c b/bundle/nginx-1.19.9/src/http/ngx_http_upstream.c -index 4a6db93..98a8cfc 100644 +index 4a6db93..374d3ba 100644 --- a/bundle/nginx-1.19.9/src/http/ngx_http_upstream.c +++ b/bundle/nginx-1.19.9/src/http/ngx_http_upstream.c -@@ -4244,6 +4244,7 @@ ngx_http_upstream_next(ngx_http_request_t *r, ngx_http_upstream_t *u, +@@ -4244,6 +4244,9 @@ ngx_http_upstream_next(ngx_http_request_t *r, ngx_http_upstream_t *u, if (u->peer.cached && ft_type == NGX_HTTP_UPSTREAM_FT_ERROR) { /* TODO: inform balancer instead */ u->peer.tries++; -+ u->peer.notify(&u->peer, u->peer.data, NGX_HTTP_UPSTREAM_NOFITY_CACHED_CONNECTION_ERROR); ++ if (u->peer.notify) { ++ u->peer.notify(&u->peer, u->peer.data, NGX_HTTP_UPSTREAM_NOTIFY_CACHED_CONNECTION_ERROR); ++ } } switch (ft_type) { diff --git a/bundle/nginx-1.19.9/src/http/ngx_http_upstream.h b/bundle/nginx-1.19.9/src/http/ngx_http_upstream.h -index 0432617..50a10cb 100644 +index 0432617..0fdb45e 100644 --- a/bundle/nginx-1.19.9/src/http/ngx_http_upstream.h +++ b/bundle/nginx-1.19.9/src/http/ngx_http_upstream.h @@ -56,6 +56,8 @@ #define NGX_HTTP_UPSTREAM_IGN_VARY 0x00000200 -+#define NGX_HTTP_UPSTREAM_NOFITY_CACHED_CONNECTION_ERROR 0x1 ++#define NGX_HTTP_UPSTREAM_NOTIFY_CACHED_CONNECTION_ERROR 0x1 + typedef struct { ngx_uint_t status; ngx_msec_t response_time; diff --git a/bundle/ngx_lua-0.10.20/src/ngx_http_lua_balancer.c b/bundle/ngx_lua-0.10.20/src/ngx_http_lua_balancer.c -index e4ac57a..44e01cb 100644 +index e4ac57a..f00c676 100644 --- a/bundle/ngx_lua-0.10.20/src/ngx_http_lua_balancer.c +++ b/bundle/ngx_lua-0.10.20/src/ngx_http_lua_balancer.c @@ -16,46 +16,105 @@ @@ -718,7 +706,7 @@ index e4ac57a..44e01cb 100644 +ngx_http_lua_balancer_notify_peer(ngx_peer_connection_t *pc, void *data, + ngx_uint_t type) +{ -+ if (type == NGX_HTTP_UPSTREAM_NOFITY_CACHED_CONNECTION_ERROR) { ++ if (type == NGX_HTTP_UPSTREAM_NOTIFY_CACHED_CONNECTION_ERROR) { + pc->tries--; + } +} @@ -1208,5 +1196,3 @@ index 7358a95..21bf8f1 100644 * lscf->balancer.handler = NULL; * lscf->balancer.src = { 0, NULL }; * lscf->balancer.src_key = NULL; --- -2.34.1 diff --git a/spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua b/spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua index cfc25919986..6bb47f8cc1d 100644 --- a/spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua +++ b/spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua @@ -121,4 +121,45 @@ for _, strategy in helpers.each_strategy() do assert.equal(#entries[2].tries, 6) end) end) + + describe("Balancer: ensure Nginx's balancer retry work without Kong's lua balancer [#" .. strategy .. "]", function() + lazy_setup(function() + assert(helpers.start_kong({ + database = strategy, + nginx_conf = "spec/fixtures/custom_nginx.template", + nginx_http_include = "../spec/fixtures/balancer/pure_nginx_balancer.conf", + })) + end) + + lazy_teardown(function() + helpers.stop_kong() + end) + + it("Nginx's balancer work well", function() + local proxy_client = helpers.proxy_client(nil, 62340) + local res = assert(proxy_client:send { + method = "GET", + path = "/pure_ngx_balancer", + }) + + assert.res_status(200, res) + local body = res:read_body() + local up_port = tonumber(body) + assert.same(62341, up_port) + + res = assert(proxy_client:send { + method = "GET", + path = "/pure_ngx_balancer", + }) + + assert.res_status(502, res) + proxy_client:close() + + assert.logfile().has.line("free keepalive peer: saving connection") + assert.logfile().has.line("get keepalive peer: using connection") + assert.logfile().has.line("get rr peer") + assert.logfile().has.line("free rr peer") + assert.logfile().has.no.line("core dumped") + end) + end) end diff --git a/spec/fixtures/balancer/pure_nginx_balancer.conf b/spec/fixtures/balancer/pure_nginx_balancer.conf new file mode 100644 index 00000000000..418e532f586 --- /dev/null +++ b/spec/fixtures/balancer/pure_nginx_balancer.conf @@ -0,0 +1,35 @@ +lua_shared_dict req_count 1m; + +upstream my_upstream { + server 127.0.0.1:62341; + keepalive 16; +} + +server { + listen 62340; + + location /pure_ngx_balancer { + proxy_pass http://my_upstream; + proxy_set_header Connection "keep-alive"; + } +} + +limit_req_zone $binary_remote_addr zone=mylimit:10m rate=1r/m; + +server { + listen 62341; + location / { + content_by_lua_block { + local dict = ngx.shared.req_count + local count = dict:get("count") or 0 + + if count >= 1 then + ngx.exit(ngx.HTTP_CLOSE) + end + + dict:set("count", count + 1) + ngx.say(ngx.var.server_port) + } + } +} + From 064bac2cda14efd077b5d9091285f5c9bf6663ac Mon Sep 17 00:00:00 2001 From: tzssangglass Date: Thu, 1 Feb 2024 16:59:19 +0800 Subject: [PATCH 2/2] claen Signed-off-by: tzssangglass --- spec/fixtures/balancer/pure_nginx_balancer.conf | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/fixtures/balancer/pure_nginx_balancer.conf b/spec/fixtures/balancer/pure_nginx_balancer.conf index 418e532f586..677f62fc91f 100644 --- a/spec/fixtures/balancer/pure_nginx_balancer.conf +++ b/spec/fixtures/balancer/pure_nginx_balancer.conf @@ -14,8 +14,6 @@ server { } } -limit_req_zone $binary_remote_addr zone=mylimit:10m rate=1r/m; - server { listen 62341; location / {