[Tarantool-patches] [PATCH v2 0/2] Set cfg via environment variables

[Tarantool-patches] [PATCH v2 0/2] Set cfg via environment variables

[Roman Khabibov via Tarantool-patches]

Issue: https://github.com/tarantool/tarantool/issues/5602
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-5602-environment-variables

Roman Khabibov (2):
  lua/log: remove 'module' option type
  box: set cfg via environment variables

 changelogs/unreleased/environment-cfg.md      |   5 +
 src/box/lua/load_cfg.lua                      | 124 +++++++++++++++++-
 test/app-tap/tarantoolctl.test.lua            |  33 ++++-
 .../gh-5602-environment-vars-cfg.result       |  41 ++++++
 .../gh-5602-environment-vars-cfg.test.lua     |  57 ++++++++
 test/box-tap/gh-5602-test-cases-script.lua    |  69 ++++++++++
 6 files changed, 323 insertions(+), 6 deletions(-)
 create mode 100755 changelogs/unreleased/environment-cfg.md
 create mode 100644 test/box-tap/gh-5602-environment-vars-cfg.result
 create mode 100755 test/box-tap/gh-5602-environment-vars-cfg.test.lua
 create mode 100755 test/box-tap/gh-5602-test-cases-script.lua

-- 
2.24.3 (Apple Git-128)

[Tarantool-patches] [PATCH v2 1/2] lua/log: remove 'module' option type

[Roman Khabibov via Tarantool-patches]

Assign lua types to log_* options instead of 'module' added in
a94a9b3. 'module' is no longer needed.

Needed for #5602
---
 src/box/lua/load_cfg.lua | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 885a0cac1..f90ba8a9a 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -144,10 +144,10 @@ local template_cfg = {
     vinyl_page_size           = 'number',
     vinyl_bloom_fpr           = 'number',
 
-    log                 = 'module',
-    log_nonblock        = 'module',
-    log_level           = 'module',
-    log_format          = 'module',
+    log                 = 'string',
+    log_nonblock        = 'boolean',
+    log_level           = 'number',
+    log_format          = 'string',
 
     io_collect_interval = 'number',
     readahead           = 'number',
@@ -492,7 +492,7 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
             end
             v = prepare_cfg(v, default_cfg[k], template_cfg[k],
                             module_cfg[k], modify_cfg[k], readable_name)
-        elseif template_cfg[k] == 'module' then
+        elseif module_cfg[k] ~= nil then
             local old_value = module_cfg[k].cfg_get(k, v)
             module_cfg_backup[k] = old_value or box.NULL
 
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH v2 1/2] lua/log: remove 'module' option type

[Leonid Vasiliev via Tarantool-patches]

Hi! Thank you for the patch.
LGTM.
Cyrill Gorcunov. please also see the patch.

On 4/13/21 3:45 PM, Roman Khabibov via Tarantool-patches wrote:
> Assign lua types to log_* options instead of 'module' added in
> a94a9b3. 'module' is no longer needed.
> 
> Needed for #5602
> ---
>   src/box/lua/load_cfg.lua | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 885a0cac1..f90ba8a9a 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -144,10 +144,10 @@ local template_cfg = {
>       vinyl_page_size           = 'number',
>       vinyl_bloom_fpr           = 'number',
>   
> -    log                 = 'module',
> -    log_nonblock        = 'module',
> -    log_level           = 'module',
> -    log_format          = 'module',
> +    log                 = 'string',
> +    log_nonblock        = 'boolean',
> +    log_level           = 'number',
> +    log_format          = 'string',
>   
>       io_collect_interval = 'number',
>       readahead           = 'number',
> @@ -492,7 +492,7 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
>               end
>               v = prepare_cfg(v, default_cfg[k], template_cfg[k],
>                               module_cfg[k], modify_cfg[k], readable_name)
> -        elseif template_cfg[k] == 'module' then
> +        elseif module_cfg[k] ~= nil then
>               local old_value = module_cfg[k].cfg_get(k, v)
>               module_cfg_backup[k] = old_value or box.NULL
>   
> 

Re: [Tarantool-patches] [PATCH v2 1/2] lua/log: remove 'module' option type

[Cyrill Gorcunov via Tarantool-patches]

On Tue, Apr 13, 2021 at 05:52:11PM +0300, Leonid Vasiliev wrote:
> Hi! Thank you for the patch.
> LGTM.
> Cyrill Gorcunov. please also see the patch.
> 

Guys, I don't like that we're dropping the "module" here. But
please gimme today's evening I'll try to come up with some
idea and code to share, ok? I'll write another email once
I get ready.

Re: [Tarantool-patches] [PATCH v2 1/2] lua/log: remove 'module' option type

[Leonid Vasiliev via Tarantool-patches]

On 4/13/21 6:03 PM, Cyrill Gorcunov wrote:
> On Tue, Apr 13, 2021 at 05:52:11PM +0300, Leonid Vasiliev wrote:
>> Hi! Thank you for the patch.
>> LGTM.
>> Cyrill Gorcunov. please also see the patch.
>>
> 
> Guys, I don't like that we're dropping the "module" here. But
> please gimme today's evening I'll try to come up with some
> idea and code to share, ok? I'll write another email once
> I get ready.
> 

Bro, of course we will wait for your feedback)

[Tarantool-patches] [PATCH v2 2/2] box: set cfg via environment variables

[Roman Khabibov via Tarantool-patches]

Add ability to set box.cfg options via env variables. These
variables should have name "TT_<OPTION>". When Tarantool instance
is started under tarantoolctl utility env variables options have
more priority than instance file's cfg.

Closes #5602

@TarantoolBot document
Title: Set box.cfg options via env variables

Now, it is possible to set box.cfg options via environment
variables. The name of variable should correspond the following
pattern: "TT_<NAME>", where <NAME> is an option name. For example:
"TT_LISTEN", "TT_READAHEAD".
If it is complex value, for example, "replication" can be set by a
table, the corresponding environment variable value has the syntax
of lua array (without keys):

`export TT_REPLICATION="{'uri1', 'uri2'}"`
---
 changelogs/unreleased/environment-cfg.md      |   5 +
 src/box/lua/load_cfg.lua                      | 114 ++++++++++++++++++
 test/app-tap/tarantoolctl.test.lua            |  33 ++++-
 .../gh-5602-environment-vars-cfg.result       |  41 +++++++
 .../gh-5602-environment-vars-cfg.test.lua     |  57 +++++++++
 test/box-tap/gh-5602-test-cases-script.lua    |  69 +++++++++++
 6 files changed, 318 insertions(+), 1 deletion(-)
 create mode 100755 changelogs/unreleased/environment-cfg.md
 create mode 100644 test/box-tap/gh-5602-environment-vars-cfg.result
 create mode 100755 test/box-tap/gh-5602-environment-vars-cfg.test.lua
 create mode 100755 test/box-tap/gh-5602-test-cases-script.lua

diff --git a/changelogs/unreleased/environment-cfg.md b/changelogs/unreleased/environment-cfg.md
new file mode 100755
index 000000000..894c40bf6
--- /dev/null
+++ b/changelogs/unreleased/environment-cfg.md
@@ -0,0 +1,5 @@
+## feature/core
+
+* Now, it is possibly to set box.cfg options with environment variables.
+The priority of sources of configuration options is the following (from low
+to high): default, tarantoolctl, environment, box.cfg{}. (gh-5602).
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index f90ba8a9a..df6a52e94 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -525,6 +525,26 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
     return new_cfg
 end
 
+---
+-- Transfer options from env_cfg to cfg.
+--
+local function apply_env_cfg(cfg, env_cfg)
+    if env_cfg == nil then
+        return
+    end
+
+    for k, v in pairs(env_cfg) do
+        -- If Tarantool started with tarantoolctl then we have to
+        -- ignore configuration from tarantoolctl, because they
+        -- has priority lower than environment variables.
+        if cfg[k] == nil or os.getenv('TARANTOOLCTL') == 'true' then
+            cfg[k] = v
+        elseif type(v) == 'table' then
+            apply_env_cfg(cfg[k], v)
+        end
+    end
+end
+
 local function apply_default_cfg(cfg, default_cfg, module_cfg)
     for k,v in pairs(default_cfg) do
         if cfg[k] == nil then
@@ -647,6 +667,98 @@ local function on_initial_config()
     end
 end
 
+---
+-- Erase quotes at the begin and the end of string.
+--
+local function dequote(str)
+    if string.sub(str, 1, 1) == '\'' and string.sub(str, -1) == '\'' or
+       string.sub(str, 1, 1) == '"' and string.sub(str, -1) == '"' then
+        return string.sub(str, 2, -2)
+    end
+    return str
+end
+
+---
+-- Convert raw option value becoming from the corresponding
+-- environment variable represented as string to string, number or
+-- boolean (depending on option).
+--
+local function process_option_value(option, raw_value)
+    local param_type = template_cfg[option]
+
+    -- May be peculiar to "feedback*" parameters.
+    if param_type == nil then
+        return nil
+    end
+
+    local value = nil
+    local err_msg_fmt = 'Environment variable TT_%s has ' ..
+        'incorrect value for option \'%s\': should be %s'
+
+    -- "log_level" option should be a number.
+    if param_type == 'number' then
+        value = tonumber(raw_value)
+        if value == nil then
+            error(err_msg_fmt:format(string.upper(option), option,
+                    "convertible to number"))
+        end
+    -- "log_nonblock" option should be boolean.
+    elseif param_type == 'boolean' then
+        if raw_value == 'true' then
+            value = true
+        elseif raw_value == 'false' then
+            value = false
+        else
+            error(err_msg_fmt:format(string.upper(option), option,
+                    "'true' or 'false'"))
+        end
+    elseif param_type == 'string, number' then
+        -- Firstly, try to convert to number.
+        value = tonumber(raw_value)
+        if value == nil then
+            value = raw_value
+        end
+    elseif param_type == 'string, number, table' then
+        if string.sub(raw_value, 1, 1) == '{' and
+           string.sub(raw_value, -1) == '}' then
+            value = string.sub(raw_value, 2, -2)
+            value = require('csv').load(value)[1]
+            for i, item in pairs(value) do
+                item = dequote(item)
+                value[i] = tonumber(item)
+                if value[i] == nil then
+                    value[i] = item
+                end
+            end
+        else
+            value = tonumber(raw_value)
+            if value == nil then
+                value = raw_value
+            end
+        end
+    else
+        value = raw_value
+    end
+
+    return value
+end
+
+---
+-- Pull and process configuration values from environment.
+--
+local function collect_env_cfg()
+    local env_cfg = {}
+    for option, _ in pairs(template_cfg) do
+        local env_var_name = 'TT_' .. string.upper(option)
+        local raw_value = os.getenv(env_var_name)
+        if raw_value ~= nil then
+            env_cfg[option] = process_option_value(option, raw_value)
+        end
+    end
+
+    return env_cfg
+end
+
 -- Whether box is loaded.
 --
 -- `false` when box is not configured or when the initialization
@@ -669,6 +781,8 @@ local function load_cfg(cfg)
     cfg = upgrade_cfg(cfg, translate_cfg)
     cfg = prepare_cfg(cfg, default_cfg, template_cfg,
                       module_cfg, modify_cfg)
+    local env_cfg = collect_env_cfg()
+    apply_env_cfg(cfg, env_cfg)
     apply_default_cfg(cfg, default_cfg, module_cfg);
     -- Save new box.cfg
     box.cfg = cfg
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 3c3023d34..060316714 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -160,7 +160,7 @@ local function merge(...)
 end
 
 local test = tap.test('tarantoolctl')
-test:plan(8)
+test:plan(9)
 
 -- basic start/stop test
 -- must be stopped afterwards
@@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
     end
 end)
 
+-- gh-5602: Check that environment cfg variables are more
+-- prioritized than tarantoolctl cfg.
+do
+    local dir = fio.tempdir()
+    local code = [[ box.cfg ]]
+    create_script(dir, 'show_cfg.lua', code)
+    local code = [[ box.cfg{checkpoint_interval=3302, readahead=10001} ]]
+    create_script(dir, 'cfg_script.lua', code)
+
+    local status, err = pcall(function()
+        test:test("check answers in case of call", function(test_i)
+            test_i:plan(4)
+            os.setenv('TT_CHECKPOINT_INTERVAL', '3301')
+            os.setenv('TT_READAHEAD', '10000')
+            check_ok(test_i, dir, 'start', 'cfg_script', 0)
+            tctl_wait_start(dir, 'cfg_script')
+            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
+                     '---\n- 3301\n- 10000\n...', nil)
+            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
+        end)
+    end)
+
+    cleanup_instance(dir, 'cfg_script')
+    recursive_rmdir(dir)
+
+    if status == false then
+        print(("Error: %s"):format(err))
+        os.exit()
+    end
+end
+
 os.exit(test:check() == true and 0 or -1)
diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
new file mode 100644
index 000000000..c90dd301b
--- /dev/null
+++ b/test/box-tap/gh-5602-environment-vars-cfg.result
@@ -0,0 +1,41 @@
+TAP version 13
+1..5
+ok - box.cfg is successful
+ok - listen
+ok - readahead
+ok - strip_core
+ok - log_format is not set
+TAP version 13
+1..7
+ok - box.cfg is successful
+ok - listen
+ok - replication is table
+ok - replication URI 1
+ok - replication URI 2
+ok - replication_connect_timeout
+ok - replication_synchro_quorum
+TAP version 13
+1..3
+ok - box.cfg is successful
+ok - box.cfg{} background value is prioritized
+ok - box.cfg{} vinyl_timeout value is prioritized
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad sql_cache_size value
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad strip_core value
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad replication value
+TAP version 13
+1..6
+ok - set 1
+ok - set 2
+ok - set 3
+ok - set 4
+ok - set 5
+ok - set 6
diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
new file mode 100755
index 000000000..3d19fdc90
--- /dev/null
+++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
@@ -0,0 +1,57 @@
+#!/usr/bin/env tarantool
+
+local os = require('os')
+local fio = require('fio')
+local tap = require('tap')
+
+local test = tap.test('gh-5602')
+
+-- gh-5602: Check that environment cfg variables working.
+local TARANTOOL_PATH = arg[-1]
+local script_name = 'gh-5602-test-cases-script.lua'
+local path_to_script = fio.pathjoin(
+        os.getenv('PWD'),
+        'box-tap',
+        script_name)
+
+---
+-- Generate shell command where set is sequence of environment
+-- variables, then path to tarantool, the first arg is path to
+-- test script and the second one is number of set.
+--
+local function shell_command(set, i)
+    return ('%s %s %s %d'):format(
+        set,
+        TARANTOOL_PATH,
+        path_to_script,
+        i)
+end
+
+local set_1 = ('%s %s %s %s'):format('TT_LISTEN=3301', 'TT_READAHEAD=10000',
+    'TT_STRIP_CORE=false', 'TT_LOG_FORMAT=json')
+local set_2 = ('%s %s %s %s'):format('TT_LISTEN=3301',
+    'TT_REPLICATION=\"{\'0.0.0.0:12345\', \'1.1.1.1:12345\'}\"',
+    'TT_REPLICATION_CONNECT_TIMEOUT=0.01',
+    'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\'')
+local set_3 = 'TT_BACKGROUND=true TT_VINYL_TIMEOUT=60.1'
+local set_4 = 'TT_SQL_CACHE_SIZE=a'
+local set_5 = 'TT_STRIP_CORE=a'
+local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', \'1.1.1.1:12345\'}\"'
+
+local sets = {}
+sets[1] = set_1
+sets[2] = set_2
+sets[3] = set_3
+sets[4] = set_4
+sets[5] = set_5
+sets[6] = set_6
+
+test:plan(#sets)
+for i, set in ipairs(sets) do
+    local tmpdir = fio.tempdir()
+    local new_path = fio.pathjoin(tmpdir, script_name)
+    fio.copyfile(path_to_script, new_path)
+    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
+end
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/box-tap/gh-5602-test-cases-script.lua b/test/box-tap/gh-5602-test-cases-script.lua
new file mode 100755
index 000000000..a8b2fc987
--- /dev/null
+++ b/test/box-tap/gh-5602-test-cases-script.lua
@@ -0,0 +1,69 @@
+local tap = require('tap')
+
+local test = tap.test('gh-5602')
+
+local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
+
+-- Check that environment cfg values are set correctly.
+if arg[1] == '1' then
+    test:plan(5)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['listen'], 3301, 'listen')
+    test:is(box.cfg['readahead'], 10000, 'readahead')
+    test:is(box.cfg['strip_core'], false, 'strip_core')
+    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
+end
+if arg[1] == '2' then
+    test:plan(7)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['listen'], 3301, 'listen')
+    local replication = box.cfg['replication']
+    test:is(type(replication), 'table', 'replication is table')
+    test:ok(replication[1] == '0.0.0.0:12345' or
+            replication[1] == '1.1.1.1:12345', 'replication URI 1')
+    test:ok(replication[2] == '0.0.0.0:12345' or
+            replication[2] == '1.1.1.1:12345', 'replication URI 2')
+    test:is(box.cfg['replication_connect_timeout'], 0.01,
+            'replication_connect_timeout')
+    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
+            'replication_synchro_quorum')
+end
+
+-- Check that box.cfg{} values are more prioritized than
+-- environment cfg values.
+if arg[1] == '3' then
+    test:plan(3)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['background'], false,
+            'box.cfg{} background value is prioritized')
+    test:is(box.cfg['vinyl_timeout'], 70.1,
+            'box.cfg{} vinyl_timeout value is prioritized')
+end
+
+local err_msg_fmt = 'Environment variable TT_%s has '..
+    'incorrect value for option \'%s\': should be %s'
+
+-- Check bad environment cfg values.
+if arg[1] == '4' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(err, err_msg_fmt:format('SQL_CACHE_SIZE',
+        'sql_cache_size', 'convertible to number'))
+    test:ok(index ~= nil, 'bad sql_cache_size value')
+end
+if arg[1] == '5' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(err, err_msg_fmt:format('STRIP_CORE',
+        'strip_core', '\'true\' or \'false\''))
+    test:ok(index ~= nil, 'bad strip_core value')
+end
+if arg[1] == '6' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(tostring(err), 'Incorrect value for option '..
+        '\'replication\': expected host:service or /unix.socket')
+    test:ok(index ~= nil, 'bad replication value')
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH v2 2/2] box: set cfg via environment variables

[Leonid Vasiliev via Tarantool-patches]

Hi! Thank you for the patch.

On 4/13/21 3:45 PM, Roman Khabibov via Tarantool-patches wrote:
> Add ability to set box.cfg options via env variables. These
> variables should have name "TT_<OPTION>". When Tarantool instance
> is started under tarantoolctl utility env variables options have
> more priority than instance file's cfg.
> 
> Closes #5602
> 
> @TarantoolBot document
> Title: Set box.cfg options via env variables
> 
> Now, it is possible to set box.cfg options via environment
> variables. The name of variable should correspond the following
> pattern: "TT_<NAME>", where <NAME> is an option name. For example:
> "TT_LISTEN", "TT_READAHEAD".
> If it is complex value, for example, "replication" can be set by a
> table, the corresponding environment variable value has the syntax
> of lua array (without keys):
> 
> `export TT_REPLICATION="{'uri1', 'uri2'}"`
> ---
>   changelogs/unreleased/environment-cfg.md      |   5 +
>   src/box/lua/load_cfg.lua                      | 114 ++++++++++++++++++
>   test/app-tap/tarantoolctl.test.lua            |  33 ++++-
>   .../gh-5602-environment-vars-cfg.result       |  41 +++++++
>   .../gh-5602-environment-vars-cfg.test.lua     |  57 +++++++++
>   test/box-tap/gh-5602-test-cases-script.lua    |  69 +++++++++++
>   6 files changed, 318 insertions(+), 1 deletion(-)
>   create mode 100755 changelogs/unreleased/environment-cfg.md
>   create mode 100644 test/box-tap/gh-5602-environment-vars-cfg.result
>   create mode 100755 test/box-tap/gh-5602-environment-vars-cfg.test.lua
>   create mode 100755 test/box-tap/gh-5602-test-cases-script.lua
> 
> diff --git a/changelogs/unreleased/environment-cfg.md b/changelogs/unreleased/environment-cfg.md
> new file mode 100755
> index 000000000..894c40bf6
> --- /dev/null
> +++ b/changelogs/unreleased/environment-cfg.md
> @@ -0,0 +1,5 @@
> +## feature/core
> +
> +* Now, it is possibly to set box.cfg options with environment variables.
> +The priority of sources of configuration options is the following (from low
> +to high): default, tarantoolctl, environment, box.cfg{}. (gh-5602).
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index f90ba8a9a..df6a52e94 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -525,6 +525,26 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
>       return new_cfg
>   end
>   
> +---
> +-- Transfer options from env_cfg to cfg.
> +--

1) Just `--- Transfer options from env_cfg to cfg.`

> +local function apply_env_cfg(cfg, env_cfg)
> +    if env_cfg == nil then
> +        return
> +    end
> +
> +    for k, v in pairs(env_cfg) do
> +        -- If Tarantool started with tarantoolctl then we have to
> +        -- ignore configuration from tarantoolctl, because they
> +        -- has priority lower than environment variables.
> +        if cfg[k] == nil or os.getenv('TARANTOOLCTL') == 'true' then
> +            cfg[k] = v

2) I thought, and it;s strange that we use knowledge about the control
tool inside the core code. Maybe do the additional env processing in 
tarantoolctl?

> +        elseif type(v) == 'table' then

3) What is this case?

> +            apply_env_cfg(cfg[k], v)
> +        end
> +    end
> +end
> +
>   local function apply_default_cfg(cfg, default_cfg, module_cfg)
>       for k,v in pairs(default_cfg) do
>           if cfg[k] == nil then
> @@ -647,6 +667,98 @@ local function on_initial_config()
>       end
>   end
>   
> +---
> +-- Erase quotes at the begin and the end of string.
> +--

4) Just `---Erase quotes at the begin and the end of string.`

> +local function dequote(str)
> +    if string.sub(str, 1, 1) == '\'' and string.sub(str, -1) == '\'' or
> +       string.sub(str, 1, 1) == '"' and string.sub(str, -1) == '"' then
> +        return string.sub(str, 2, -2)
> +    end
> +    return str
> +end
> +
> +---
> +-- Convert raw option value becoming from the corresponding

5) `---Convert raw option value becoming from the corresponding`

> +-- environment variable represented as string to string, number or
> +-- boolean (depending on option).
> +--

6) No extra blank line is needed(`--`).

> +local function process_option_value(option, raw_value)

7) Add `env` to the function name.
For example:`process_env_option_value`

> +    local param_type = template_cfg[option]
> +
> +    -- May be peculiar to "feedback*" parameters.
> +    if param_type == nil then
> +        return nil
> +    end
> +
> +    local value = nil
> +    local err_msg_fmt = 'Environment variable TT_%s has ' ..
> +        'incorrect value for option \'%s\': should be %s'
> +
> +    -- "log_level" option should be a number.

8) This comment is deprecated.

> +    if param_type == 'number' then
> +        value = tonumber(raw_value)
> +        if value == nil then
> +            error(err_msg_fmt:format(string.upper(option), option,
> +                    "convertible to number"))

9) Use one `tab` and single quotes.

> +        end
> +    -- "log_nonblock" option should be boolean.

10) This comment is deprecated.

> +    elseif param_type == 'boolean' then
> +        if raw_value == 'true' then
> +            value = true
> +        elseif raw_value == 'false' then
> +            value = false
> +        else
> +            error(err_msg_fmt:format(string.upper(option), option,
> +                    "'true' or 'false'"))

11) Use one `tab` and single quotes.
12) Why are "TRUE" and "FALSE" is invalid?

> +        end
> +    elseif param_type == 'string, number' then
> +        -- Firstly, try to convert to number.
> +        value = tonumber(raw_value)
> +        if value == nil then
> +            value = raw_value
> +        end
> +    elseif param_type == 'string, number, table' then

13) Move this case to a separate function. It's very hard to read now.

> +        if string.sub(raw_value, 1, 1) == '{' and
> +           string.sub(raw_value, -1) == '}' then
> +            value = string.sub(raw_value, 2, -2)
> +            value = require('csv').load(value)[1]
> +            for i, item in pairs(value) do

14) Add a comment that now it can only be a string or a number (bool and
table are not supported).

> +                item = dequote(item)
> +                value[i] = tonumber(item)
> +                if value[i] == nil then
> +                    value[i] = item
> +                end
> +            end
> +        else
> +            value = tonumber(raw_value)
> +            if value == nil then
> +                value = raw_value
> +            end
> +        end
> +    else
> +        value = raw_value
> +    end
> +
> +    return value
> +end
> +
> +---
> +-- Pull and process configuration values from environment.
> +--

15) Just `--- Pull and process configuration values from environment.`

> +local function collect_env_cfg()
> +    local env_cfg = {}
> +    for option, _ in pairs(template_cfg) do
> +        local env_var_name = 'TT_' .. string.upper(option)
> +        local raw_value = os.getenv(env_var_name)
> +        if raw_value ~= nil then
> +            env_cfg[option] = process_option_value(option, raw_value)
> +        end
> +    end
> +
> +    return env_cfg
> +end
> +
>   -- Whether box is loaded.
>   --
>   -- `false` when box is not configured or when the initialization
> @@ -669,6 +781,8 @@ local function load_cfg(cfg)
>       cfg = upgrade_cfg(cfg, translate_cfg)
>       cfg = prepare_cfg(cfg, default_cfg, template_cfg,
>                         module_cfg, modify_cfg)
> +    local env_cfg = collect_env_cfg()
> +    apply_env_cfg(cfg, env_cfg)
>       apply_default_cfg(cfg, default_cfg, module_cfg);
>       -- Save new box.cfg
>       box.cfg = cfg
> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
> index 3c3023d34..060316714 100755
> --- a/test/app-tap/tarantoolctl.test.lua
> +++ b/test/app-tap/tarantoolctl.test.lua
> @@ -160,7 +160,7 @@ local function merge(...)
>   end
>   
>   local test = tap.test('tarantoolctl')
> -test:plan(8)
> +test:plan(9)
>   
>   -- basic start/stop test
>   -- must be stopped afterwards
> @@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
>       end
>   end)
>   
> +-- gh-5602: Check that environment cfg variables are more
> +-- prioritized than tarantoolctl cfg.
> +do
> +    local dir = fio.tempdir()
> +    local code = [[ box.cfg ]]
> +    create_script(dir, 'show_cfg.lua', code)
> +    local code = [[ box.cfg{checkpoint_interval=3302, readahead=10001} ]]
> +    create_script(dir, 'cfg_script.lua', code)
> +
> +    local status, err = pcall(function()
> +        test:test("check answers in case of call", function(test_i)
> +            test_i:plan(4)
> +            os.setenv('TT_CHECKPOINT_INTERVAL', '3301')
> +            os.setenv('TT_READAHEAD', '10000')
> +            check_ok(test_i, dir, 'start', 'cfg_script', 0)
> +            tctl_wait_start(dir, 'cfg_script')
> +            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
> +                     '---\n- 3301\n- 10000\n...', nil)
> +            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
> +        end)
> +    end)
> +
> +    cleanup_instance(dir, 'cfg_script')
> +    recursive_rmdir(dir)
> +
> +    if status == false then
> +        print(("Error: %s"):format(err))
> +        os.exit()
> +    end
> +end
> +
>   os.exit(test:check() == true and 0 or -1)
> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
> new file mode 100644
> index 000000000..c90dd301b
> --- /dev/null
> +++ b/test/box-tap/gh-5602-environment-vars-cfg.result
> @@ -0,0 +1,41 @@
> +TAP version 13
> +1..5
> +ok - box.cfg is successful
> +ok - listen
> +ok - readahead
> +ok - strip_core
> +ok - log_format is not set
> +TAP version 13
> +1..7
> +ok - box.cfg is successful
> +ok - listen
> +ok - replication is table
> +ok - replication URI 1
> +ok - replication URI 2
> +ok - replication_connect_timeout
> +ok - replication_synchro_quorum
> +TAP version 13
> +1..3
> +ok - box.cfg is successful
> +ok - box.cfg{} background value is prioritized
> +ok - box.cfg{} vinyl_timeout value is prioritized
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +ok - bad sql_cache_size value
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +ok - bad strip_core value
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +ok - bad replication value
> +TAP version 13
> +1..6
> +ok - set 1
> +ok - set 2
> +ok - set 3
> +ok - set 4
> +ok - set 5
> +ok - set 6
> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
> new file mode 100755
> index 000000000..3d19fdc90
> --- /dev/null
> +++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
> @@ -0,0 +1,57 @@
> +#!/usr/bin/env tarantool
> +
> +local os = require('os')
> +local fio = require('fio')
> +local tap = require('tap')
> +
> +local test = tap.test('gh-5602')
> +
> +-- gh-5602: Check that environment cfg variables working.
> +local TARANTOOL_PATH = arg[-1]
> +local script_name = 'gh-5602-test-cases-script.lua'
> +local path_to_script = fio.pathjoin(
> +        os.getenv('PWD'),
> +        'box-tap',
> +        script_name)
> +
> +---
> +-- Generate shell command where set is sequence of environment

16) `--- Generate shell command where set is sequence of environment`

> +-- variables, then path to tarantool, the first arg is path to
> +-- test script and the second one is number of set.
> +--

17) No extra blank line is needed(`--`).

> +local function shell_command(set, i)
> +    return ('%s %s %s %d'):format(
> +        set,
> +        TARANTOOL_PATH,
> +        path_to_script,
> +        i)
> +end
> +
> +local set_1 = ('%s %s %s %s'):format('TT_LISTEN=3301', 'TT_READAHEAD=10000',
> +    'TT_STRIP_CORE=false', 'TT_LOG_FORMAT=json')
> +local set_2 = ('%s %s %s %s'):format('TT_LISTEN=3301',
> +    'TT_REPLICATION=\"{\'0.0.0.0:12345\', \'1.1.1.1:12345\'}\"',
> +    'TT_REPLICATION_CONNECT_TIMEOUT=0.01',
> +    'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\'')
> +local set_3 = 'TT_BACKGROUND=true TT_VINYL_TIMEOUT=60.1'
> +local set_4 = 'TT_SQL_CACHE_SIZE=a'
> +local set_5 = 'TT_STRIP_CORE=a'
> +local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', \'1.1.1.1:12345\'}\"'
> +
> +local sets = {}
> +sets[1] = set_1
> +sets[2] = set_2
> +sets[3] = set_3
> +sets[4] = set_4
> +sets[5] = set_5
> +sets[6] = set_6
> +
> +test:plan(#sets)
> +for i, set in ipairs(sets) do
> +    local tmpdir = fio.tempdir()
> +    local new_path = fio.pathjoin(tmpdir, script_name)
> +    fio.copyfile(path_to_script, new_path)
> +    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
> +end
> +
> +os.exit(test:check() and 0 or 1)
> diff --git a/test/box-tap/gh-5602-test-cases-script.lua b/test/box-tap/gh-5602-test-cases-script.lua

18) `gh-5602-environment-cfg-test-cases.lua`

> new file mode 100755
> index 000000000..a8b2fc987
> --- /dev/null
> +++ b/test/box-tap/gh-5602-test-cases-script.lua
> @@ -0,0 +1,69 @@
> +local tap = require('tap')
> +
> +local test = tap.test('gh-5602')
> +
> +local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
> +
> +-- Check that environment cfg values are set correctly.
> +if arg[1] == '1' then
> +    test:plan(5)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['listen'], 3301, 'listen')
> +    test:is(box.cfg['readahead'], 10000, 'readahead')
> +    test:is(box.cfg['strip_core'], false, 'strip_core')
> +    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
> +end
> +if arg[1] == '2' then
> +    test:plan(7)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['listen'], 3301, 'listen')
> +    local replication = box.cfg['replication']
> +    test:is(type(replication), 'table', 'replication is table')
> +    test:ok(replication[1] == '0.0.0.0:12345' or
> +            replication[1] == '1.1.1.1:12345', 'replication URI 1')
> +    test:ok(replication[2] == '0.0.0.0:12345' or
> +            replication[2] == '1.1.1.1:12345', 'replication URI 2')
> +    test:is(box.cfg['replication_connect_timeout'], 0.01,
> +            'replication_connect_timeout')
> +    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
> +            'replication_synchro_quorum')
> +end
> +
> +-- Check that box.cfg{} values are more prioritized than
> +-- environment cfg values.
> +if arg[1] == '3' then
> +    test:plan(3)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['background'], false,
> +            'box.cfg{} background value is prioritized')
> +    test:is(box.cfg['vinyl_timeout'], 70.1,
> +            'box.cfg{} vinyl_timeout value is prioritized')
> +end
> +
> +local err_msg_fmt = 'Environment variable TT_%s has '..
> +    'incorrect value for option \'%s\': should be %s'
> +
> +-- Check bad environment cfg values.
> +if arg[1] == '4' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    local index = string.find(err, err_msg_fmt:format('SQL_CACHE_SIZE',
> +        'sql_cache_size', 'convertible to number'))
> +    test:ok(index ~= nil, 'bad sql_cache_size value')
> +end
> +if arg[1] == '5' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    local index = string.find(err, err_msg_fmt:format('STRIP_CORE',
> +        'strip_core', '\'true\' or \'false\''))
> +    test:ok(index ~= nil, 'bad strip_core value')
> +end
> +if arg[1] == '6' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    local index = string.find(tostring(err), 'Incorrect value for option '..
> +        '\'replication\': expected host:service or /unix.socket')
> +    test:ok(index ~= nil, 'bad replication value')
> +end
> +
> +os.exit(test:check() and 0 or 1)
> 

Re: [Tarantool-patches] [PATCH v2 2/2] box: set cfg via environment variables

[Cyrill Gorcunov via Tarantool-patches]

Guys, I must confess I don't like dropping 'module' from template_cfg,
the reason it has been introduced in first place is that modules might
be used _before_ the box.cfg{} even called. So that the modules should
carry own config and provide it back to the load_lua code.

Currently we have one such 'early' module -- it is logger. But we might
extend it in future. Moreover spreading types here and there won't make
code anyhow easier to read and modify.

Also the question raises -- the environment variables are considered
only at box.cfg{} call, but should not be they also handled by modules
which support early use? If yes -- then we need to add such support
into the logger code. If no -- then it should be pointed in documentation
that log module ignores env variables if set up before box.cfg{} call.
Anoter option -- defer such support to the next release.

Now back to types. Current Roman's code test for basic types from @template_cfg
which means later when these values are setting into real @cfg variable they
are passing same testing again

prepare_cfg
  ...
            local good_types = string.gsub(template_cfg[k], ' ', '');
            if (string.find(',' .. good_types .. ',', ',' .. type(v) .. ',') == nil) then
                box.error(box.error.CFG, readable_name, "should be one of types "..
                    template_cfg[k])
            end

so the question is why do we need to do this double work? As far as I understand
the only thing what we need to do is to *fetch* data from environment variables
and set them into @cfg variable. Then loader will process every entry and report
an error if something goes wrong. Though I think we might need to dequote variables
just like it is done in the patch.

Now back to types. If you really think that we still need the validation of types
on the stage when we fetch the values from env variables then please don't revert
the commit with 'module' keyword but rather provide the types separately so
we rework them if needed. Modules should be extendable not squashed into defaults.

Say for now we could use
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Tue, 13 Apr 2021 20:50:14 +0300
Subject: [PATCH] cfg: provide types for logger options

This needed for fast type check when fetching
options from environment variable.

Part-of #5602

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/lua/load_cfg.lua | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index d31c9eb2c..7f57fcbb1 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -118,6 +118,18 @@ local module_cfg = {
     log_format          = log.box_api,
 }
 
+-- cfg types for modules, probably better to
+-- provide some API with type enumeration or
+-- similar. Currently it has use for environment
+-- processing only.
+local module_cfg_type = {
+    -- logging
+    log                 = 'string',
+    log_nonblock        = 'boolean',
+    log_level           = 'number, string',
+    log_format          = 'string',
+}
+
 -- types of available options
 -- could be comma separated lua types or 'any' if any type is allowed
 local template_cfg = {
@@ -686,6 +698,10 @@ end
 local function process_option_value(option, raw_value)
     local param_type = template_cfg[option]
 
+    if param_type == 'module' then
+        param_type = module_cfg_type[option]
+    end
+
     -- May be peculiar to "feedback*" parameters.
     if param_type == nil then
         return nil
-- 
2.30.2