From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A4E3346970F for ; Fri, 22 Nov 2019 19:33:43 +0300 (MSK) Date: Fri, 22 Nov 2019 19:31:37 +0300 From: Igor Munkin Message-ID: <20191122163137.GI18878@tarantool.org> References: <20191122120347.66629-1-arkholga@tarantool.org> <20191122120347.66629-3-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191122120347.66629-3-arkholga@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 2/2] box: protects box.cfg from raw data modification List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia Cc: tarantool-patches@dev.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 | | 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 | | replication: | - 127.0.0.1:3301 | - 127.0.0.2:3301 | | ... | | 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 | | replication: | - QQ | - 127.0.0.2:3301 | | ... 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