box/lua: disallow changing box options directly #8208

ligurio · 2023-01-25T08:43:23Z

Tarantool has a special table 'box.cfg' that includes configuration
parameters. User could view that table, but it's direct modification has
no effect - after assigning a new value it is actually "updated" but
actual value remains the same. Such behaviour is a counterintuitive for
our users and provides a bad experience.

Proposed patch change this behaviour: new value assigned to parameter
via direct access to table box.cfg raise an error.

Before the patch:

tarantool> box.cfg{}
<snipped>
tarantool> box.cfg.read_only=true
---
...

tarantool>

After the patch:

tarantool> box.cfg{}
<snipped>
tarantool> box.cfg.read_only=true
---
- error: 'builtin/box/load_cfg.lua:973: Use function box.cfg for modifications'
...

tarantool>

NO_DOC=bugfix

Closes #2867

Gumix · 2023-01-25T10:57:53Z

Hi, here is something wrong with the validation of the options:

tarantool> box.cfg{}
-- skipped --

tarantool> box.cfg.log_format = 'xxx'
---
- error: 'Incorrect value for option ''log_format'': expected ''plain'' or ''json'''
...

tarantool> box.cfg.log_format = 'xxx'
---
...

tarantool> box.cfg.log_format = 'yyy'
---
...

tarantool> box.cfg.log_format
---
- yyy
...

tarantool> box.cfg.log_level = 6
---
- error: 'Incorrect value for option ''log_format'': expected ''plain'' or ''json'''
...
-- ??? why log_format?

Here is another issue, box.cfg.log_level is updated correctly, while box.cfg still prints the old value:

tarantool> box.cfg{}
-- skipped --

tarantool> box.cfg.log_level
---
- 5
...

tarantool> box.cfg.log_level = 6
2023-01-25 10:53:43.764 [339315] main/103/interactive/box.load_cfg I> set 'log_level' configuration option to 6
---
...

tarantool> box.cfg.log_level
---
- 6
...

tarantool> box.cfg
---
- replication_sync_timeout: 0
  -- skipped --
  log_level: 5
  -- skipped --

igormunkin · 2023-01-26T12:52:43Z

@Gumix, nice catch, thanks for noticing! By the way, I've added you to reviewers list, since we touch box-related interface. I believe you're the best candidate, 'cause you're the only one from Core team, who has already touched the patch :)

coveralls · 2023-02-06T08:15:03Z

Coverage: 85.489% (+0.007%) from 85.481% when pulling 6f8993f on ligurio:ligurio/gh-2867-raise-on-raw-modifications-box.cfg into a2d5e54
on tarantool:master.

Gumix

OK for me, but I want to invite @nshy

src/box/lua/load_cfg.lua

test/box-luatest/gh_2867_raise_on_raw_box_cfg_modifications_test.lua

src/box/lua/load_cfg.lua

igormunkin · 2023-02-08T19:51:13Z

Well, there are two points regarding the current state of the PR:

It conflicts with the target branch (@ligurio, please fix it)
I added do not merge label, since it depends on the patch in tarantool/luajit, so the bump should go first.

test/box-luatest/gh_2867_raise_on_raw_box_cfg_modifications_test.lua

igormunkin

@ligurio, thanks for the patch! Please consider my comments below.

Furthermore, there are two confusing NOWRAP entries in the commit message: if these are just typos, please fix it either.

I also guess that adding a docbot request will be nice, since such behaviour change should be added to the docs.

igormunkin · 2023-02-08T21:43:42Z

src/lua/init.lua

+local function patched_pairs(object)
+    local mt = getmetatable(object)
+    local iterable = (mt and mt.__name == 'box_cfg') and
+                     debug.getmetatable(object).__index or


Well, this part is not quite right.

It's unsafe, since getmetatable yields the value of __metatable field if one is set (that can be e.g. string¹). You need either to check the type of mt variable or to use debug.getmetatable to obtain object's metatable. I'd rather use the latter approach.

There is no need in fetching object's metatable one more time in the line above (it's already stored in mt, isn't it?) so just use mt.__index for this.

Footnotes

Fortunately, all Lua strings share the metatable with overloaded index, so mt.__name in case of strings just yields nil, but anyway in general case this is not safe. ↩

Applied a patch where both issues fixed:

--- a/src/lua/init.lua +++ b/src/lua/init.lua @@ -35,9 +35,9 @@ local ROCKS_LUA_TEMPLATES = { ROCKS_LUA_PATH .. '/?.lua', ROCKS_LUA_PATH .. '/?/ -- box.cfg. local builtin_pairs = rawget(_G, 'pairs') local function patched_pairs(object) - local mt = getmetatable(object) + local mt = debug.getmetatable(object) local iterable = (mt and mt.__name == 'box_cfg') and - debug.getmetatable(object).__index or + mt.__index or object return builtin_pairs(iterable) end

src/box/lua/load_cfg.lua

Totktonada · 2023-02-08T22:21:43Z

I glanced over the patch out of curiosity and would give a few suggestions about the code layout.

I would move our own pairs() into its own module src/lua/pairs.lua (just like we do for print()). src/lua/init.lua is bloated with many things unrelated to each other and I think that it should be decomposed in a future anyway.
I would eliminate the __name field and would the tables itself as keys in a weak table... well, it is easier to explain with code:

-- pairs.lua
local M = {}

M.raw_pairs = pairs

-- table -> iterator generator
M.tracked = setmetatable({}, {__mode = 'k'})

_G.pairs = function(t)
    local iter_gen = M.tracked[t] or M.raw_pairs
    return iter_gen(t)
end

function M.add(t, iter_gen)
    M.tracked[t] = iter_gen
end

function M.remove(t)
    M.tracked[t] = nil
end

return M

Usage:

local internal_pairs = require('internal.pairs')
local real_table = {}
local proxy_table = {_iterate_over = real_table}
internal_pairs.add(proxy_table, function(self)
    return pairs(self._iterate_over)
end)

-- or

local internal_pairs = require('internal.pairs')
local real_table = {}
local proxy_table = {}
internal_pairs.add(proxy_table, function(_self)
    return pairs(real_table)
end)

Feel free to ignore, I just pass here over.

ligurio · 2023-02-09T12:05:06Z

@igormunkin

Furthermore, there are two confusing NOWRAP entries in the commit message: if these are just typos, please fix it either.

NO_WRAP used correctly here, tag wraps a block with lines that exceed max length.
https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst#commit-message

I also guess that adding a docbot request will be nice, since such behaviour change should be added to the docs.

Added.

ligurio · 2023-02-09T12:24:23Z

I glanced over the patch out of curiosity and would give a few suggestions about the code layout.

As I got it right, these changes will make patched pairs available from outside:

[0] ~/sources/MRG/tarantool$ ./build/src/tarantool 
Tarantool 2.11.0-entrypoint-984-g7ababddc0
type 'help' for interactive help
tarantool> require("internal.print")
---
- raw_print: 'function: builtin#29'
...

On RFC discussion we decided to implement public version of patched pairs in scope of a separate ticket. Your snippet could be a good draft patch for this ticket.

igormunkin

@ligurio, thanks for all changes made! LGTM now (but I'll wait for your answer for my question below prior to merging the PR).

igormunkin · 2023-02-10T11:51:31Z

src/box/lua/load_cfg.lua

+            __newindex = function(self, key, value) -- luacheck: no unused args
+                if template_cfg[key] ~= nil then
+                    local v = '<...>'
+                    if type(value) ~= 'table' then


Minor: The only thing that bothers me here is the condition.

Is it possible to pass other "compex" types (such as cdata/udata/function) as a box.cfg parameter value? If not, then everything is fine; if there are doubts we can just leave it as is and fix if/when one is found: yeah, it's bad when there is garbage in the error message, but not critical.

Feel free to ignore.

It is a valid comment, thanks.

AFAIK, there are no values with non-printable types in box.cfg. I wrote a small script that recursively looking for values in box.cfg table:

local function handle_obj(obj) if type(obj) == "table" then for k, v in pairs(obj) do if not k then path = path .. "." .. tostring(k) end handle_obj(k, path) handle_obj(v, path) end end if type(obj) == "number" or type(obj) == "string" or type(obj) == "boolean" then return end local str = tostring(obj) if string.match(str, "^function") and string.match(str, "^table") and string.match(str, "^cdata") and string.match(str, "^userdata") then local keyword = path .. "." .. str print("\"" .. keyword .. "\"") end end for obj in pairs(box.cfg) do handle_obj(obj) end print("The end.") os.exit(0)

and output:

[0] ~/sources/MRG/tarantool$ ./build/src/tarantool -e "box.cfg{};" box_cfg_types.lua 2023-02-10 15:30:12.096 [146593] main/103/box_cfg_types.lua I> Tarantool 2.11.0-entrypoint-984-gf3880e244 <snipped> Start. The end. [0] ~/sources/MRG/tarantool$

@TarantoolBot

Tarantool has a special table 'box.cfg' that includes configuration parameters. User could view that table, but it's direct modification had no effect - after assigning a new value it is actually "updated", but actual value remains the same. Such behaviour is a counterintuitive for our users and provides a bad experience. Proposed patch change this behaviour: new value assigned to parameter via direct access to table box.cfg raise an error. Before the patch: tarantool> box.cfg{} <snipped> tarantool> box.cfg.read_only=true --- ... tarantool> After the patch: NO_WRAP tarantool> box.cfg{} <snipped> tarantool> box.cfg.read_only=true --- - error: 'builtin/box/load_cfg.lua:973: Use box.cfg{read_only = true} for update' ... tarantool> NO_WRAP Closes tarantool#2867 @TarantoolBot document Title: Document changed behaviour on setting options to box.cfg directly Tarantool has a special table 'box.cfg' that includes configuration parameters. User could view that table, but it's direct modification has no effect - after assigning a new value it is actually "updated" but actual value remains the same. Such behaviour is a counterintuitive for our users and provides a bad experience. Now new value assigned to parameter via direct access to table box.cfg raise an error.

With previous commit rawset is not required anymore because internally we start to use raw_cfg. Follows up tarantool#2867 NO_CHANGELOG=code health NO_DOC=code health NO_TEST=code health

ligurio requested review from igormunkin and Buristan January 25, 2023 08:43

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch 2 times, most recently from 9123d8c to 7f48134 Compare January 25, 2023 11:21

ligurio changed the title ~~box/lua: allow to change box options indirectly~~ box/lua: allow changing box options directly Jan 25, 2023

igormunkin requested a review from Gumix January 26, 2023 12:49

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch 2 times, most recently from 45dd19e to 1899e05 Compare January 26, 2023 14:12

igormunkin assigned igormunkin, Buristan, Gumix and ligurio and unassigned Gumix, igormunkin and Buristan Jan 26, 2023

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch 6 times, most recently from 122dddf to d3e17bc Compare February 3, 2023 14:38

ligurio changed the title ~~box/lua: allow changing box options directly~~ box/lua: disallow changing box options directly Feb 3, 2023

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch from d3e17bc to 8c1050d Compare February 6, 2023 08:03

ligurio added the full-ci Enables all tests for a pull request label Feb 6, 2023

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch from cfd0dfd to 7be80d6 Compare February 6, 2023 14:06

ligurio removed the request for review from Buristan February 7, 2023 09:56

Gumix approved these changes Feb 7, 2023

View reviewed changes

src/box/lua/load_cfg.lua Outdated Show resolved Hide resolved

Gumix requested a review from nshy February 7, 2023 13:45

nshy requested changes Feb 7, 2023

View reviewed changes

test/box-luatest/gh_2867_raise_on_raw_box_cfg_modifications_test.lua Outdated Show resolved Hide resolved

src/box/lua/load_cfg.lua Show resolved Hide resolved

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch from 7be80d6 to 7827f2d Compare February 7, 2023 15:47

nshy approved these changes Feb 8, 2023

View reviewed changes

igormunkin added the do not merge Not ready to be merged label Feb 8, 2023

igormunkin reviewed Feb 8, 2023

View reviewed changes

test/box-luatest/gh_2867_raise_on_raw_box_cfg_modifications_test.lua Show resolved Hide resolved

igormunkin suggested changes Feb 8, 2023

View reviewed changes

ligurio mentioned this pull request Feb 9, 2023

Disallow direct changing options for nested tables inside box.cfg #8279

Open

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch from 7827f2d to 6ddca75 Compare February 9, 2023 12:05

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch 5 times, most recently from 91ec30e to 26c2ace Compare February 10, 2023 08:11

igormunkin approved these changes Feb 10, 2023

View reviewed changes

igormunkin assigned igormunkin and unassigned ligurio Feb 10, 2023

ligurio added 2 commits February 10, 2023 16:07

box/lua: drop rawset in load_cfg

6f8993f

With previous commit rawset is not required anymore because internally we start to use raw_cfg. Follows up tarantool#2867 NO_CHANGELOG=code health NO_DOC=code health NO_TEST=code health

ligurio force-pushed the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch from 26c2ace to 6f8993f Compare February 10, 2023 13:09

igormunkin merged commit d28cc04 into tarantool:master Feb 10, 2023

igormunkin removed the do not merge Not ready to be merged label Feb 10, 2023

ligurio deleted the ligurio/gh-2867-raise-on-raw-modifications-box.cfg branch February 10, 2023 14:31

box/lua: disallow changing box options directly #8208

box/lua: disallow changing box options directly #8208

Conversation

ligurio commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gumix commented Jan 25, 2023

Uh oh!

igormunkin commented Jan 26, 2023

Uh oh!

coveralls commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gumix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igormunkin commented Feb 8, 2023

Uh oh!

Uh oh!

igormunkin left a comment

Choose a reason for hiding this comment

Uh oh!

igormunkin Feb 8, 2023

Choose a reason for hiding this comment

Footnotes

Uh oh!

ligurio Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Totktonada commented Feb 8, 2023

Uh oh!

ligurio commented Feb 9, 2023

Uh oh!

ligurio commented Feb 9, 2023

Uh oh!

igormunkin left a comment

Choose a reason for hiding this comment

Uh oh!

igormunkin Feb 10, 2023

Choose a reason for hiding this comment

Uh oh!

ligurio Feb 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ligurio commented Jan 25, 2023 •

edited

Loading

coveralls commented Feb 6, 2023 •

edited

Loading