Add range checks for issues.set_limits (#1543)

Nowadays, only types are checked when calling `issues.set_limits`.
However, there are some restrictions for these values (e.g.
`clock_delta_threshold_warning` cannot be negative). This patch
makes limits validation stricter.

Author: keepsilence2012

cartridge.lua

@@ -706,6 +706,11 @@ local function cfg(opts, box_opts)
         return nil, err
     end
 
+    local ok, err = issues.validate_limits(issue_limits)
+    if not ok then
+        return nil, err
+    end
+
     issues.set_limits(issue_limits)
 
     if opts.upload_prefix ~= nil then

cartridge/issues.lua

@@ -58,7 +58,7 @@ local fio = require('fio')
 local log = require('log')
 local fun = require('fun')
 local fiber = require('fiber')
-local checks = require('checks')
+local errors = require('errors')
 local membership = require('membership')
 
 local pool = require('cartridge.pool')
@@ -67,6 +67,8 @@ local failover = require('cartridge.failover')
 local confapplier = require('cartridge.confapplier')
 local lua_api_proxy = require('cartridge.lua-api.proxy')
 
+local ValidateConfigError = errors.new_class('ValidateConfigError')
+
 local vars = require('cartridge.vars').new(mod_name)
 
 --- Thresholds for issuing warnings.
@@ -80,6 +82,15 @@ local default_limits = {
     fragmentation_threshold_warning  = 0.6, -- number: *default*: 0.6.
     clock_delta_threshold_warning    = 5, -- number: *default*: 5.
 }
+
+-- Min and max values for issues_limits theshold
+-- range[1] <= value <= range[2]
+local limits_ranges = {
+    fragmentation_threshold_warning = {0, 1},
+    fragmentation_threshold_critical = {0, 1},
+    clock_delta_threshold_warning = {0, math.huge},
+}
+
 vars:new('limits', default_limits)
 
 local function describe(uri)
@@ -476,12 +487,63 @@ local function list_on_cluster()
     return ret, err
 end
 
+--- Validate limits configuration.
+--
+-- @function validate_limits
+-- @local
+-- @tparam table limits
+-- @treturn[1] boolean true
+-- @treturn[2] nil
+-- @treturn[2] table Error description
+local function validate_limits(limits)
+    if type(limits) ~= 'table' then
+        return nil, ValidateConfigError:new(
+            'limits must be a table, got %s', type(limits)
+        )
+    end
+
+    for name, value in pairs(limits) do
+        if type(name) ~= 'string' then
+            return nil, ValidateConfigError:new(
+                'limits table keys must be string, got %s', type(name)
+            )
+        end
+
+        local range = limits_ranges[name]
+        if range == nil then
+            return nil, ValidateConfigError:new(
+                'unknown limits key %q', name
+            )
+        elseif type(value) ~= 'number' then
+            return nil, ValidateConfigError:new(
+                'limits.%s must be a number, got %s',
+                name, type(value)
+            )
+        elseif not (value >= range[1] and value <= range[2]) then
+            return nil, ValidateConfigError:new(
+                'limits.%s must be in range [%g, %g]',
+                name, range[1], range[2]
+            )
+        end
+    end
+
+    return true
+end
+
+--- Update limits configuration.
+--
+-- @function set_limits
+-- @local
+-- @tparam table limits
+-- @treturn[1] boolean true
+-- @treturn[2] nil
+-- @treturn[2] table Error description
 local function set_limits(limits)
-    checks({
-        fragmentation_threshold_critical = '?number',
-        fragmentation_threshold_warning = '?number',
-        clock_delta_threshold_warning = '?number',
-    })
+    local ok, err = validate_limits(limits)
+    if not ok then
+        return nil, err
+    end
+
     vars.limits = fun.chain(vars.limits, limits):tomap()
     return true
 end
@@ -490,5 +552,7 @@ _G.__cartridge_issues_list_on_instance = list_on_instance
 
 return {
     list_on_cluster = list_on_cluster,
+    default_limits = default_limits,
+    validate_limits = validate_limits,
     set_limits = set_limits,
 }

test/integration/issues_test.lua

@@ -25,9 +25,9 @@ g.before_all(function()
             }},
         }},
         env = {
-            TARANTOOL_CLOCK_DELTA_THRESHOLD_WARNING = 100000000,
-            TARANTOOL_FRAGMENTATION_THRESHOLD_WARNING = 100000000,
-            TARANTOOL_FRAGMENTATION_THRESHOLD_CRITICAL = 100000000,
+            TARANTOOL_CLOCK_DELTA_THRESHOLD_WARNING = math.huge,
+            TARANTOOL_FRAGMENTATION_THRESHOLD_WARNING = 1,
+            TARANTOOL_FRAGMENTATION_THRESHOLD_CRITICAL = 1,
         }
     })
     g.cluster:start()
@@ -63,21 +63,20 @@ end)
 
 function g.test_issues_limits()
     local server = g.cluster:server('master')
+
     t.assert_equals(
         server:eval("return require('cartridge.vars').new('cartridge.issues').limits"),
         {
-            clock_delta_threshold_warning = 100000000,
-            fragmentation_threshold_warning = 100000000,
-            fragmentation_threshold_critical = 100000000
+            clock_delta_threshold_warning = math.huge,
+            fragmentation_threshold_warning = 1,
+            fragmentation_threshold_critical = 1,
         }
     )
 
-    -- restore to defaults by calling set_limits
-    server:eval([[require("cartridge.issues").set_limits({
-        clock_delta_threshold_warning = 5,
-        fragmentation_threshold_warning = 0.6,
-        fragmentation_threshold_critical = 0.9
-    })]])
+    server:eval([[
+        local cartridge_issues = require("cartridge.issues")
+        cartridge_issues.set_limits(cartridge_issues.default_limits)
+    ]])
     t.assert_equals(
         server:eval("return require('cartridge.vars').new('cartridge.issues').limits"),
         {

test/unit/cfg_errors_test.lua

@@ -197,6 +197,35 @@ g.test_http = function()
     end)
 end
 
+-- issues_limits --------------------------------------------------------------
+-------------------------------------------------------------------------------
+
+g.test_issues_limits = function()
+    helpers.run_remotely(g.server, function()
+        local t = require('luatest')
+        os.setenv('TARANTOOL_CLOCK_DELTA_THRESHOLD_WARNING', '-1')
+
+        local ok, err = require('cartridge').cfg({
+            swim_broadcast = false,
+            advertise_uri = 'localhost:13301',
+            http_enabled = false,
+            workdir = os.getenv('TARANTOOL_WORKDIR'),
+            roles = {},
+        })
+
+        t.assert_equals(ok, nil)
+        t.assert_covers(err, {
+            class_name = 'ValidateConfigError',
+            err = 'limits.clock_delta_threshold_warning must be in range [0, inf]',
+        })
+    end)
+end
+g.after_test('test_issues_limits', function()
+    helpers.run_remotely(g.server, function()
+        os.setenv('TARANTOOL_CLOCK_DELTA_THRESHOLD_WARNING', nil)
+    end)
+end)
+
 -- roles ----------------------------------------------------------------------
 -------------------------------------------------------------------------------
 g.test_roles = function()

test/unit/issues_limits_test.lua

@@ -0,0 +1,88 @@
+#!/usr/bin/env tarantool
+
+local t = require('luatest')
+local g = t.group()
+
+local issues = require("cartridge.issues")
+
+function g.test_positive()
+    local function check_ok(limits)
+        return t.assert_equals(
+            {issues.validate_limits(limits)},
+            {true, nil}
+        )
+    end
+
+    check_ok({})
+    check_ok(issues.default_limits)
+
+    check_ok({clock_delta_threshold_warning = 0})
+    check_ok({clock_delta_threshold_warning = math.huge})
+
+    check_ok({fragmentation_threshold_warning = 0})
+    check_ok({fragmentation_threshold_warning = 1})
+
+    check_ok({fragmentation_threshold_critical = 0})
+    check_ok({fragmentation_threshold_critical = 1})
+end
+
+function g.test_negative()
+    local function check_error(limits, err_expected)
+        local ok, err = issues.validate_limits(limits)
+        t.assert_equals(ok, nil)
+        t.assert_covers(err, {
+            class_name = "ValidateConfigError",
+            err = err_expected,
+        })
+    end
+
+    local nan = 0/0
+    assert(nan ~= nan)
+
+    check_error(0,        'limits must be a table, got number')
+    check_error(nil,      'limits must be a table, got nil')
+    check_error(box.NULL, 'limits must be a table, got cdata')
+
+    check_error({{}},       'limits table keys must be string, got number')
+    check_error({[{}] = 1}, 'limits table keys must be string, got table')
+
+    check_error({unknown_limit = 0}, 'unknown limits key "unknown_limit"')
+
+    -- clock_delta_threshold_warning
+    check_error(
+        {clock_delta_threshold_warning = 'yes'},
+        'limits.clock_delta_threshold_warning must be a number, got string'
+    )
+    check_error(
+        {clock_delta_threshold_warning = 12ULL},
+        'limits.clock_delta_threshold_warning must be a number, got cdata'
+    )
+    check_error(
+        {clock_delta_threshold_warning = -1e-7},
+        'limits.clock_delta_threshold_warning must be in range [0, inf]'
+    )
+    check_error(
+        {clock_delta_threshold_warning = nan},
+        'limits.clock_delta_threshold_warning must be in range [0, inf]'
+    )
+
+    -- fragmentation_threshold_warning
+    check_error(
+        {fragmentation_threshold_warning = 0 - 1e-7},
+        'limits.fragmentation_threshold_warning must be in range [0, 1]'
+    )
+    check_error(
+        {fragmentation_threshold_warning = 1 + 1e-7},
+        'limits.fragmentation_threshold_warning must be in range [0, 1]'
+    )
+
+    -- fragmentation_threshold_critical
+    check_error(
+        {fragmentation_threshold_critical = 0 - 1e-7},
+        'limits.fragmentation_threshold_critical must be in range [0, 1]'
+    )
+    check_error(
+        {fragmentation_threshold_critical = 1 + 1e-7},
+        'limits.fragmentation_threshold_critical must be in range [0, 1]'
+    )
+end