diff --git a/build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch b/build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch index f0b20bdd12d1..aa339c32a9c7 100644 --- a/build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch +++ b/build/openresty/patches/ngx_lua-0.10.25_01-dyn_upstream_keepalive.patch @@ -1,8 +1,33 @@ +diff --git a/bundle/nginx-1.21.4/src/http/ngx_http_upstream.c b/bundle/nginx-1.21.4/src/http/ngx_http_upstream.c +index b07e564..9e25905 100644 +--- a/bundle/nginx-1.21.4/src/http/ngx_http_upstream.c ++++ b/bundle/nginx-1.21.4/src/http/ngx_http_upstream.c +@@ -4304,6 +4304,7 @@ 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); + } + + switch (ft_type) { +diff --git a/bundle/nginx-1.21.4/src/http/ngx_http_upstream.h b/bundle/nginx-1.21.4/src/http/ngx_http_upstream.h +index a385222..1cd214c 100644 +--- a/bundle/nginx-1.21.4/src/http/ngx_http_upstream.h ++++ b/bundle/nginx-1.21.4/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 ++ + typedef struct { + ngx_uint_t status; + ngx_msec_t response_time; diff --git a/bundle/ngx_lua-0.10.25/src/ngx_http_lua_balancer.c b/bundle/ngx_lua-0.10.25/src/ngx_http_lua_balancer.c -index af4da73..407c115 100644 +index af4da73..99d073a 100644 --- a/bundle/ngx_lua-0.10.25/src/ngx_http_lua_balancer.c +++ b/bundle/ngx_lua-0.10.25/src/ngx_http_lua_balancer.c -@@ -16,46 +16,104 @@ +@@ -16,46 +16,106 @@ #include "ngx_http_lua_directive.h" @@ -96,6 +121,8 @@ index af4da73..407c115 100644 - ngx_http_request_t *r); static void ngx_http_lua_balancer_free_peer(ngx_peer_connection_t *pc, void *data, ngx_uint_t state); ++static void ngx_http_lua_balancer_notify_peer(ngx_peer_connection_t *pc, ++ void *data, ngx_uint_t type); +static ngx_int_t ngx_http_lua_balancer_create_keepalive_pool(lua_State *L, + ngx_log_t *log, ngx_str_t *cpool_name, ngx_uint_t cpool_size, + ngx_http_lua_balancer_keepalive_pool_t **cpool); @@ -127,7 +154,7 @@ index af4da73..407c115 100644 ngx_int_t -@@ -102,6 +160,61 @@ ngx_http_lua_balancer_handler_inline(ngx_http_request_t *r, +@@ -102,6 +162,61 @@ ngx_http_lua_balancer_handler_inline(ngx_http_request_t *r, } @@ -189,7 +216,7 @@ index af4da73..407c115 100644 char * ngx_http_lua_balancer_by_lua_block(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) -@@ -125,18 +238,20 @@ char * +@@ -125,18 +240,20 @@ char * ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) { @@ -218,7 +245,7 @@ index af4da73..407c115 100644 if (cmd->post == NULL) { return NGX_CONF_ERROR; } -@@ -188,11 +303,42 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd, +@@ -188,11 +305,42 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd, lscf->balancer.src_key = cache_key; @@ -261,7 +288,7 @@ index af4da73..407c115 100644 } uscf->peer.init_upstream = ngx_http_lua_balancer_init; -@@ -208,14 +354,18 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd, +@@ -208,14 +356,18 @@ ngx_http_lua_balancer_by_lua(ngx_conf_t *cf, ngx_command_t *cmd, static ngx_int_t @@ -284,7 +311,7 @@ index af4da73..407c115 100644 us->peer.init = ngx_http_lua_balancer_init_peer; return NGX_OK; -@@ -226,33 +376,38 @@ static ngx_int_t +@@ -226,33 +378,39 @@ static ngx_int_t ngx_http_lua_balancer_init_peer(ngx_http_request_t *r, ngx_http_upstream_srv_conf_t *us) { @@ -317,6 +344,7 @@ index af4da73..407c115 100644 + r->upstream->peer.data = bp; r->upstream->peer.get = ngx_http_lua_balancer_get_peer; r->upstream->peer.free = ngx_http_lua_balancer_free_peer; ++ r->upstream->peer.notify = ngx_http_lua_balancer_notify_peer; #if (NGX_HTTP_SSL) + bp->original_set_session = r->upstream->peer.set_session; @@ -334,7 +362,7 @@ index af4da73..407c115 100644 return NGX_OK; } -@@ -260,25 +415,26 @@ ngx_http_lua_balancer_init_peer(ngx_http_request_t *r, +@@ -260,25 +418,26 @@ ngx_http_lua_balancer_init_peer(ngx_http_request_t *r, static ngx_int_t ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data) { @@ -372,7 +400,7 @@ index af4da73..407c115 100644 if (ctx == NULL) { ctx = ngx_http_lua_create_ctx(r); if (ctx == NULL) { -@@ -296,21 +452,23 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data) +@@ -296,21 +455,23 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data) ctx->context = NGX_HTTP_LUA_CONTEXT_BALANCER; @@ -403,7 +431,7 @@ index af4da73..407c115 100644 if (rc == NGX_ERROR) { return NGX_ERROR; } -@@ -332,79 +490,88 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data) +@@ -332,79 +493,88 @@ ngx_http_lua_balancer_get_peer(ngx_peer_connection_t *pc, void *data) } } @@ -537,7 +565,7 @@ index af4da73..407c115 100644 return rc; } -@@ -413,24 +580,354 @@ static void +@@ -413,24 +583,364 @@ static void ngx_http_lua_balancer_free_peer(ngx_peer_connection_t *pc, void *data, ngx_uint_t state) { @@ -677,6 +705,16 @@ index af4da73..407c115 100644 +} + + ++static void ++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) { ++ pc->tries--; ++ } ++} ++ ++ +static ngx_int_t +ngx_http_lua_balancer_create_keepalive_pool(lua_State *L, ngx_log_t *log, + ngx_str_t *cpool_name, ngx_uint_t cpool_size, @@ -795,15 +833,17 @@ index af4da73..407c115 100644 + + if (lua_isnil(L, -1)) { + lua_pop(L, 1); /* orig stack */ -+ return; -+ } -+ + return; + } + +- /* fallback */ + ngx_http_lua_assert(lua_istable(L, -1)); + + lua_pushlstring(L, (const char *)cpool->cpool_name.data, cpool->cpool_name.len); + lua_pushnil(L); /* pools nil */ + lua_rawset(L, -3); /* pools */ -+ + +- ngx_http_upstream_free_round_robin_peer(pc, data, state); + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, log, 0, + "lua balancer: keepalive free pool, " + "name: %V, cpool: %p", @@ -876,16 +916,14 @@ index af4da73..407c115 100644 + goto close; + } + - return; - } - -- /* fallback */ ++ return; ++ } ++ +close: + + item = c->data; + c->log = ev->log; - -- ngx_http_upstream_free_round_robin_peer(pc, data, state); ++ + ngx_http_lua_balancer_close(c); + + ngx_queue_remove(&item->queue); @@ -897,7 +935,7 @@ index af4da73..407c115 100644 } -@@ -441,12 +938,12 @@ ngx_http_lua_balancer_set_session(ngx_peer_connection_t *pc, void *data) +@@ -441,12 +951,12 @@ ngx_http_lua_balancer_set_session(ngx_peer_connection_t *pc, void *data) { ngx_http_lua_balancer_peer_data_t *bp = data; @@ -912,7 +950,7 @@ index af4da73..407c115 100644 } -@@ -455,13 +952,12 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data) +@@ -455,13 +965,12 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data) { ngx_http_lua_balancer_peer_data_t *bp = data; @@ -928,7 +966,7 @@ index af4da73..407c115 100644 } #endif -@@ -469,14 +965,14 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data) +@@ -469,14 +978,14 @@ ngx_http_lua_balancer_save_session(ngx_peer_connection_t *pc, void *data) int ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, @@ -950,7 +988,7 @@ index af4da73..407c115 100644 if (r == NULL) { *err = "no request found"; -@@ -501,18 +997,6 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, +@@ -501,18 +1010,6 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, return NGX_ERROR; } @@ -969,7 +1007,7 @@ index af4da73..407c115 100644 ngx_memzero(&url, sizeof(ngx_url_t)); url.url.data = ngx_palloc(r->pool, addr_len); -@@ -536,6 +1020,8 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, +@@ -536,6 +1033,8 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, return NGX_ERROR; } @@ -978,7 +1016,7 @@ index af4da73..407c115 100644 if (url.addrs && url.addrs[0].sockaddr) { bp->sockaddr = url.addrs[0].sockaddr; bp->socklen = url.addrs[0].socklen; -@@ -546,6 +1032,72 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, +@@ -546,6 +1045,72 @@ ngx_http_lua_ffi_balancer_set_current_peer(ngx_http_request_t *r, return NGX_ERROR; } @@ -1051,7 +1089,7 @@ index af4da73..407c115 100644 return NGX_OK; } -@@ -555,14 +1107,13 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r, +@@ -555,14 +1120,13 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r, long connect_timeout, long send_timeout, long read_timeout, char **err) { @@ -1069,7 +1107,7 @@ index af4da73..407c115 100644 if (r == NULL) { *err = "no request found"; -@@ -587,15 +1138,9 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r, +@@ -587,15 +1151,9 @@ ngx_http_lua_ffi_balancer_set_timeouts(ngx_http_request_t *r, return NGX_ERROR; } @@ -1087,7 +1125,7 @@ index af4da73..407c115 100644 if (!bp->cloned_upstream_conf) { /* we clone the upstream conf for the current request so that * we do not affect other requests at all. */ -@@ -650,12 +1195,10 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r, +@@ -650,12 +1208,10 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r, int count, char **err) { #if (nginx_version >= 1007005) @@ -1103,7 +1141,7 @@ index af4da73..407c115 100644 ngx_http_lua_balancer_peer_data_t *bp; if (r == NULL) { -@@ -681,13 +1224,7 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r, +@@ -681,13 +1237,7 @@ ngx_http_lua_ffi_balancer_set_more_tries(ngx_http_request_t *r, return NGX_ERROR; } @@ -1118,7 +1156,7 @@ index af4da73..407c115 100644 #if (nginx_version >= 1007005) max_tries = r->upstream->conf->next_upstream_tries; -@@ -713,12 +1250,10 @@ int +@@ -713,12 +1263,10 @@ int ngx_http_lua_ffi_balancer_get_last_failure(ngx_http_request_t *r, int *status, char **err) { @@ -1134,7 +1172,7 @@ index af4da73..407c115 100644 if (r == NULL) { *err = "no request found"; -@@ -743,13 +1278,7 @@ ngx_http_lua_ffi_balancer_get_last_failure(ngx_http_request_t *r, +@@ -743,13 +1291,7 @@ ngx_http_lua_ffi_balancer_get_last_failure(ngx_http_request_t *r, return NGX_ERROR; } diff --git a/changelog/unreleased/kong/balancer_respect_max_retries.yml b/changelog/unreleased/kong/balancer_respect_max_retries.yml new file mode 100644 index 000000000000..1884ad1ce9f0 --- /dev/null +++ b/changelog/unreleased/kong/balancer_respect_max_retries.yml @@ -0,0 +1,3 @@ +message: Fix an issue that the actual number of retry times exceeds the `retries` setting. +type: bugfix +scope: Core 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 new file mode 100644 index 000000000000..b3245055dfe3 --- /dev/null +++ b/spec/02-integration/05-proxy/10-balancer/08-retries_spec.lua @@ -0,0 +1,128 @@ +local helpers = require "spec.helpers" +local cjson = require "cjson" + +local function get_log(typ, n) + local entries + helpers.wait_until(function() + local client = assert(helpers.http_client(helpers.mock_upstream_host, + helpers.mock_upstream_port)) + local res = client:get("/read_log/" .. typ, { + headers = { + Accept = "application/json" + } + }) + local raw = assert.res_status(200, res) + local body = cjson.decode(raw) + + entries = body.entries + return #entries > 0 + end, 10) + if n then + assert(#entries == n, "expected " .. n .. " log entries, but got " .. #entries) + end + return entries +end + +for _, strategy in helpers.each_strategy() do + describe("Balancer: respect max retries [#" .. strategy .. "]", function() + local service + + lazy_setup(function() + local bp = helpers.get_db_utils(strategy, { + "routes", + "services", + "plugins", + }) + + service = bp.services:insert { + name = "retry_service", + host = "127.0.0.1", + port = 62351, + retries = 5, + } + + local route = bp.routes:insert { + service = service, + paths = { "/hello" }, + strip_path = false, + } + + bp.plugins:insert { + route = { id = route.id }, + name = "http-log", + config = { + queue = { + max_batch_size = 1, + max_coalescing_delay = 0.1, + }, + http_endpoint = "http://" .. helpers.mock_upstream_host + .. ":" + .. helpers.mock_upstream_port + .. "/post_log/http" + } + } + + local fixtures = { + http_mock = {} + } + + fixtures.http_mock.my_server_block = [[ + server { + listen 0.0.0.0:62351; + location /hello { + content_by_lua_block { + local request_counter = ngx.shared.request_counter + local first_request = request_counter:get("first_request") + if first_request == nil then + request_counter:set("first_request", "yes") + ngx.say("hello") + else + ngx.exit(ngx.HTTP_CLOSE) + end + } + } + } + ]] + + assert(helpers.start_kong({ + database = strategy, + nginx_conf = "spec/fixtures/custom_nginx.template", + nginx_http_lua_shared_dict = "request_counter 1m", + }, nil, nil, fixtures)) + end) + + lazy_teardown(function() + helpers.stop_kong() + end) + + it("exceeded limit", function() + -- First request should succeed and save connection to upstream in keepalive pool + local proxy_client1 = helpers.proxy_client() + local res = assert(proxy_client1:send { + method = "GET", + path = "/hello", + }) + + assert.res_status(200, res) + + proxy_client1:close() + + -- Second request should failed 1 times and retry 5 times and then return 502 + local proxy_client2 = helpers.proxy_client() + + res = assert(proxy_client2:send { + method = "GET", + path = "/hello", + }) + + assert.res_status(502, res) + + -- wait for the http-log plugin to flush the log + ngx.sleep(1) + + local entries = get_log("http", 2) + assert.equal(#entries[2].tries, 6) + assert.equal(entries[2].upstream_status, "502, 502, 502, 502, 502, 502") + end) + end) +end