Re: [Tarantool-patches] [PATCH v2 2/2] box: protects box.cfg from raw data modification

[Igor Munkin <imun@tarantool.org>]

Olya,

Thanks for the patch, it looks similar to the one proposed by Mons long
time ago. However, as a result of the offline discussion with him, we
faced the fact that the provided patch still allows a modification of
nested objects, e.g. replication. Consider the following:
| $ ./src/tarantool
| Tarantool 2.2.1-114-g6c8acacef
| type 'help' for interactive help
| tarantool> box.cfg{ replication_connect_quorum=0, replication={"127.0.0.1:3301","127.0.0.2:3301"} }
| 2019-11-22 17:51:14.861 [29536] main/102/interactive C> Tarantool 2.2.1-114-g6c8acacef
| 2019-11-22 17:51:14.861 [29536] main/102/interactive C> log level 5
| 2019-11-22 17:51:14.861 [29536] main/102/interactive I> mapping 268435456 bytes for memtx tuple arena...
| 2019-11-22 17:51:14.862 [29536] main/102/interactive I> mapping 134217728 bytes for vinyl tuple arena...
| 2019-11-22 17:51:14.876 [29536] main/102/interactive I> instance uuid 78b17737-e0bd-4773-885e-bb15fbecc406
| 2019-11-22 17:51:14.876 [29536] main/102/interactive I> connecting to 2 replicas
| <snip>
| 2019-11-22 17:51:44.935 [29536] snapshot/101/main I> done
| 2019-11-22 17:51:44.937 [29536] main/102/interactive I> ready to accept requests
| 2019-11-22 17:51:44.937 [29536] main/104/checkpoint_daemon I> scheduled next checkpoint for Fri Nov 22 19:08:25 2019
| 2019-11-22 17:51:44.938 [29536] main/102/interactive I> set 'replication_connect_quorum' configuration option to 0
| 2019-11-22 17:51:44.938 [29536] main/102/interactive I> set 'replication' configuration option to ["127.0.0.1:3301","127.0.0.2:3301"]
| ---
| ...
| tarantool> box.cfg
| ---
| - vinyl_run_count_per_level: 2
|   <snip>
|   replication:
|   - 127.0.0.1:3301
|   - 127.0.0.2:3301
|   <snip>
| ...
|
| tarantool> box.cfg.replication = nil
| ---
| - error: 'builtin/box/load_cfg.lua:541: Attempt to modify a read-only table'
| ...
|
| tarantool> box.cfg.replication[1] = 'QQ'
| ---
| ...
|
| tarantool> box.cfg
| ---
| - vinyl_run_count_per_level: 2
|   <snip>
|   replication:
|   - QQ
|   - 127.0.0.2:3301
|   <snip>
| ...
Thereby some fields in box.cfg are still mutable after the patch.

Besides, I left some comments below related to the test you attached to
the patch. Please consider them too.

On 22.11.19, Olga Arkhangelskaia wrote:
> Forbids the possibility of the raw modification for box.cfg table.
> Now the only way to change table value is box.cfg{}.
> 
> Closes #2867
> ---
>  src/box/lua/load_cfg.lua  | 14 +++++++++++++-
>  test/box-tap/cfg.test.lua | 11 ++++++++++-
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 85617c8f0..27ac6bb77 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -533,12 +533,24 @@ local function load_cfg(cfg)
>      end
>      setmetatable(box, nil)
>      box_configured = nil
> -    box.cfg = setmetatable(cfg,
> +
> +    local actual = cfg
> +    box.cfg = setmetatable({},
>          {
>              __newindex = function(table, index)
>                  error('Attempt to modify a read-only table')
>              end,
>              __call = locked(reload_cfg),
> +            __index = function (self, k)
> +                return actual[k]
> +            end,
> +            __serialize = function() return actual end,
> +            __pairs = function(self)
> +                local function iter(actual, k)
> +                    return next(actual, k)
> +                end
> +                return iter, actual, nil
> +            end
>          })
>      private.cfg_load()
>      for key, fun in pairs(dynamic_cfg) do
> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> index d529447bb..443dfafbc 100755
> --- a/test/box-tap/cfg.test.lua
> +++ b/test/box-tap/cfg.test.lua
> @@ -6,7 +6,7 @@ local socket = require('socket')
>  local fio = require('fio')
>  local uuid = require('uuid')
>  local msgpack = require('msgpack')
> -test:plan(104)
> +test:plan(105)
>  
>  --------------------------------------------------------------------------------
>  -- Invalid values
> @@ -592,6 +592,15 @@ box.cfg{read_only=true}
>  ]]
>  test:is(run_script(code), PANIC, "panic on bootstrapping a read-only instance as master")
>  
> +--
> +-- gf-2867 raise on raw modifications of box.cfg values
> +--
> +code = [[
> +box.cfg{}
> +box.cfg["read_only"] = true
> +]]
> +
> +test:is(run_script(code), PANIC, "attempt to modify a read-only table")

The provided test case is fine, but I see we can extend it with
following checks:
* check the one can't add new values to box.cfg, e.g. a QQ
* check that pairs iterator yields the correct values
* consider adding a separate case related to replication table
  modification

>  
>  test:check()
>  os.exit(0)
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Best regards,
IM