fix(balancer): ensure the notify callback is invoked only if defined when handling cached connection errors

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 <[email protected]>
-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

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

spec/fixtures/balancer/pure_nginx_balancer.conf

@@ -0,0 +1,33 @@
+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";
+    }
+}
+
+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)
+        }
+    }
+}
+