fix(upstream): should not override default keepalive value (#5054)

Signed-off-by: spacewander <[email protected]>

Author: spacewander

apisix/balancer.lua

@@ -19,7 +19,8 @@ local balancer          = require("ngx.balancer")
 local core              = require("apisix.core")
 local priority_balancer = require("apisix.balancer.priority")
 local ipairs            = ipairs
-local enable_keepalive = balancer.enable_keepalive
+local is_http           = ngx.config.subsystem == "http"
+local enable_keepalive = balancer.enable_keepalive and is_http
 local set_more_tries   = balancer.set_more_tries
 local get_last_failure = balancer.get_last_failure
 local set_timeouts     = balancer.set_timeouts
@@ -261,12 +262,29 @@ _M.pick_server = pick_server
 local set_current_peer
 do
     local pool_opt = {}
+    local default_keepalive_pool
 
     function set_current_peer(server, ctx)
         local up_conf = ctx.upstream_conf
         local keepalive_pool = up_conf.keepalive_pool
 
-        if keepalive_pool and enable_keepalive then
+        if enable_keepalive then
+            if not keepalive_pool then
+                if not default_keepalive_pool then
+                    local local_conf = core.config.local_conf()
+                    local up_keepalive_conf =
+                        core.table.try_read_attr(local_conf, "nginx_config",
+                                                 "http", "upstream")
+                    default_keepalive_pool = {}
+                    default_keepalive_pool.idle_timeout =
+                        core.config_util.parse_time_unit(up_keepalive_conf.keepalive_timeout)
+                    default_keepalive_pool.size = up_keepalive_conf.keepalive
+                    default_keepalive_pool.requests = up_keepalive_conf.keepalive_requests
+                end
+
+                keepalive_pool = default_keepalive_pool
+            end
+
             local idle_timeout = keepalive_pool.idle_timeout
             local size = keepalive_pool.size
             local requests = keepalive_pool.requests

apisix/core/config_util.lua

@@ -15,7 +15,10 @@
 -- limitations under the License.
 --
 local core_tab = require("apisix.core.table")
+local str_byte = string.byte
+local str_char = string.char
 local setmetatable = setmetatable
+local tostring = tostring
 local type = type
 
 
@@ -65,4 +68,90 @@ function _M.cancel_clean_handler(item, idx, fire)
 end
 
 
+-- Time intervals can be specified in milliseconds, seconds, minutes, hours, days and so on,
+-- using the following suffixes:
+-- ms	milliseconds
+-- s	seconds
+-- m	minutes
+-- h	hours
+-- d	days
+-- w	weeks
+-- M	months, 30 days
+-- y	years, 365 days
+-- Multiple units can be combined in a single value by specifying them in the order from the most
+-- to the least significant, and optionally separated by whitespace.
+-- A value without a suffix means seconds.
+function _M.parse_time_unit(s)
+    local typ = type(s)
+    if typ == "number" then
+        return s
+    end
+
+    if typ ~= "string" or #s == 0 then
+        return nil, "invalid data: " .. tostring(s)
+    end
+
+    local size = 0
+    local size_in_unit = 0
+    local step = 60 * 60 * 24 * 365
+    local with_ms = false
+    for i = 1, #s do
+        local scale
+        local unit = str_byte(s, i)
+        if unit == 121 then -- y
+            scale = 60 * 60 * 24 * 365
+        elseif unit == 77 then -- M
+            scale = 60 * 60 * 24 * 30
+        elseif unit == 119 then -- w
+            scale = 60 * 60 * 24 * 7
+        elseif unit == 100 then -- d
+            scale = 60 * 60 * 24
+        elseif unit == 104 then -- h
+            scale = 60 * 60
+        elseif unit == 109 then -- m
+            unit = str_byte(s, i + 1)
+            if unit == 115 then -- ms
+                size = size * 1000
+                with_ms = true
+                step = 0
+                break
+            end
+
+            scale = 60
+
+        elseif unit == 115 then -- s
+            scale = 1
+        elseif 48 <= unit and unit <= 57 then
+            size_in_unit = size_in_unit * 10 + unit - 48
+        elseif unit ~= 32 then
+            return nil, "invalid data: " .. str_char(unit)
+        end
+
+        if scale ~= nil then
+            if scale > step then
+                return nil, "unexpected unit: " .. str_char(unit)
+            end
+
+            step = scale
+            size = size + scale * size_in_unit
+            size_in_unit = 0
+        end
+    end
+
+    if size_in_unit > 0 then
+        if step == 1 then
+            return nil, "specific unit conflicts with the default unit second"
+        end
+
+        size = size + size_in_unit
+    end
+
+    if with_ms then
+        size = size / 1000
+    end
+
+    return size
+end
+
+
 return _M

t/core/config_util.t

@@ -0,0 +1,72 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+    my ($block) = @_;
+
+    if (!$block->request) {
+        $block->set_value("request", "GET /t");
+    }
+
+    if (!$block->no_error_log && !$block->error_log) {
+        $block->set_value("no_error_log", "[error]\n[alert]");
+    }
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: parse_time_unit
+--- config
+    location /t {
+        content_by_lua_block {
+            local parse_time_unit = require("apisix.core.config_util").parse_time_unit
+            for _, case in ipairs({
+                {exp = 1, input = "1"},
+                {exp = 1, input = "1s"},
+                {exp = 60, input = "60s"},
+                {exp = 1.1, input = "1s100ms"},
+                {exp = 10.001, input = "10s1ms"},
+                {exp = 3600, input = "60m"},
+                {exp = 3600.11, input = "60m110ms"},
+                {exp = 3710, input = "1h110"},
+                {exp = 5400, input = "1h  30m"},
+                {exp = 34822861.001, input = "1y1M1w1d1h1m1s1ms"},
+            }) do
+                assert(case.exp == parse_time_unit(case.input),
+                       string.format("input %s, got %s", case.input,
+                            parse_time_unit(case.input)))
+            end
+
+            for _, case in ipairs({
+                {exp = "invalid data: -", input = "-1"},
+                {exp = "unexpected unit: h", input = "1m1h"},
+                {exp = "invalid data: ", input = ""},
+                {exp = "specific unit conflicts with the default unit second", input = "1s1"},
+            }) do
+                local _, err = parse_time_unit(case.input)
+                assert(case.exp == err,
+                       string.format("input %s, got %s", case.input, err))
+            end
+        }
+    }

t/node/upstream-keepalive-pool.t

@@ -215,3 +215,63 @@ lua balancer: keepalive create pool, crc32: \S+, size: 1
 lua balancer: keepalive no free connection, cpool: \S+
 lua balancer: keepalive saving connection \S+, cpool: \S+, connections: 1
 $/
+
+
+
+=== TEST 6: set upstream without keepalive_pool
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/upstreams/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "type": "roundrobin",
+                    "nodes": {
+                        "127.0.0.1:1980": 1
+                    }
+                }]]
+            )
+            if code >= 300 then
+                ngx.status = code
+                ngx.print(body)
+                return
+            end
+        }
+    }
+
+
+
+=== TEST 7: should not override default value
+--- config
+    location /t {
+        content_by_lua_block {
+            local http = require "resty.http"
+            local uri = "http://127.0.0.1:" .. ngx.var.server_port
+                        .. "/hello"
+            for i = 1, 3 do
+                local httpc = http.new()
+                local res, err = httpc:request_uri(uri)
+                if not res then
+                    ngx.say(err)
+                    return
+                end
+                ngx.print(res.body)
+            end
+        }
+    }
+--- response_body
+hello world
+hello world
+hello world
+--- grep_error_log eval
+qr/lua balancer: keepalive .*/
+--- grep_error_log_out eval
+qr/^lua balancer: keepalive create pool, crc32: \S+, size: 320
+lua balancer: keepalive no free connection, cpool: \S+
+lua balancer: keepalive saving connection \S+, cpool: \S+, connections: 1
+lua balancer: keepalive reusing connection \S+, requests: 1, cpool: \S+
+lua balancer: keepalive saving connection \S+, cpool: \S+, connections: 1
+lua balancer: keepalive reusing connection \S+, requests: 2, cpool: \S+
+lua balancer: keepalive saving connection \S+, cpool: \S+, connections: 1
+$/