From 3e0f75fa610471cf984cc1b4a05847129aea56da Mon Sep 17 00:00:00 2001 From: Anton Kuznets Date: Wed, 26 Feb 2025 14:55:56 +0300 Subject: [PATCH] router: protect replicasets from modification The function `vshard.router.routeall` is not marked as internal API, but it returns `router.replicasets` table, which is used by the router itself. And also `vshard.router.route` returns a replicaset object that is used internally. If user modifies the returned from `routeall` or `route` table, then router is broken and cannot continue to work properly after that. Now `routeall` returns a new copy of the replicaset array to user on each call. So user can do whatever he wants with this array, without affecting the internal behavior of the router. Also, each of the replicasets that are returned from `routeall` or `route` is a wrapper over the internal replicaset table. This wrapper contains the private field `_replicaset`, the value of which is the internal replicaset table. User should not touch this field. This wrapper only gives user access to the public replicaset methods: `call`, `callrw`, `callro`, `callbro`, `callre`, `callbre`. Closes tarantool/vshard#502 NO_DOC= --- test/failover/failover.result | 26 ++++---- test/failover/failover.test.lua | 26 ++++---- test/misc/check_uuid_on_connect.result | 2 +- test/misc/check_uuid_on_connect.test.lua | 2 +- test/multiple_routers/multiple_routers.result | 5 -- .../multiple_routers.test.lua | 3 - test/router-luatest/map_callrw_test.lua | 7 ++- test/router-luatest/router_2_2_test.lua | 61 ++++++++++++++++++- .../router_election_auto_master_test.lua | 3 +- test/router/reload.result | 6 +- test/router/reload.test.lua | 6 +- test/router/router.result | 4 +- test/router/router.test.lua | 4 +- test/router/router2.result | 4 +- test/router/router2.test.lua | 4 +- vshard/replicaset.lua | 39 +++++++++++- vshard/router/init.lua | 11 +++- 17 files changed, 155 insertions(+), 58 deletions(-) diff --git a/test/failover/failover.result b/test/failover/failover.result index ef31a5d1..0e40e1ed 100644 --- a/test/failover/failover.result +++ b/test/failover/failover.result @@ -120,15 +120,15 @@ priority_order() - - 1 - unknown zone ... -vshard.router.route(1).uuid == rs_uuid[1] +vshard.router.route(1)._replicaset.uuid == rs_uuid[1] --- - true ... -vshard.router.route(31).uuid == rs_uuid[2] +vshard.router.route(31)._replicaset.uuid == rs_uuid[2] --- - true ... -vshard.router.route(61).uuid == rs_uuid[3] +vshard.router.route(61)._replicaset.uuid == rs_uuid[3] --- - true ... @@ -371,15 +371,15 @@ priority_order() - - 1 - unknown zone ... -vshard.router.route(1).uuid == rs_uuid[1] +vshard.router.route(1)._replicaset.uuid == rs_uuid[1] --- - true ... -vshard.router.route(31).uuid == rs_uuid[2] +vshard.router.route(31)._replicaset.uuid == rs_uuid[2] --- - true ... -vshard.router.route(61).uuid == rs_uuid[3] +vshard.router.route(61)._replicaset.uuid == rs_uuid[3] --- - true ... @@ -409,15 +409,15 @@ priority_order() - - 1 - unknown zone ... -vshard.router.route(1).uuid == rs_uuid[1] +vshard.router.route(1)._replicaset.uuid == rs_uuid[1] --- - true ... -vshard.router.route(31).uuid == rs_uuid[2] +vshard.router.route(31)._replicaset.uuid == rs_uuid[2] --- - true ... -vshard.router.route(61).uuid == rs_uuid[3] +vshard.router.route(61)._replicaset.uuid == rs_uuid[3] --- - true ... @@ -447,15 +447,15 @@ priority_order() - - unknown zone - 1 ... -vshard.router.route(1).uuid == rs_uuid[1] +vshard.router.route(1)._replicaset.uuid == rs_uuid[1] --- - true ... -vshard.router.route(31).uuid == rs_uuid[2] +vshard.router.route(31)._replicaset.uuid == rs_uuid[2] --- - true ... -vshard.router.route(61).uuid == rs_uuid[3] +vshard.router.route(61)._replicaset.uuid == rs_uuid[3] --- - true ... @@ -482,7 +482,7 @@ while not test_run:grep_log('router_1', 'Ping error from', 1000) do fiber.sleep( t = string.rep('a', 1024 * 1024 * 500) --- ... -rs = vshard.router.route(31) +rs = vshard.router.route(31)._replicaset --- ... while rs.master ~= rs.replica do fiber.sleep(0.01) end diff --git a/test/failover/failover.test.lua b/test/failover/failover.test.lua index d054853f..7693bf9e 100644 --- a/test/failover/failover.test.lua +++ b/test/failover/failover.test.lua @@ -55,9 +55,9 @@ test_run:switch('router_1') vshard.router.cfg(cfg) while not test_run:grep_log('router_1', 'New replica box_1_d%(storage%@') do fiber.sleep(0.1) end priority_order() -vshard.router.route(1).uuid == rs_uuid[1] -vshard.router.route(31).uuid == rs_uuid[2] -vshard.router.route(61).uuid == rs_uuid[3] +vshard.router.route(1)._replicaset.uuid == rs_uuid[1] +vshard.router.route(31)._replicaset.uuid == rs_uuid[2] +vshard.router.route(61)._replicaset.uuid == rs_uuid[3] vshard.router.call(1, 'read', 'echo', {123}) test_run:switch('box_1_d') -- Not 0 - 'read' echo was called here. @@ -145,27 +145,27 @@ create_router('router_2') test_run:switch('router_2') vshard.router.cfg(cfg) priority_order() -vshard.router.route(1).uuid == rs_uuid[1] -vshard.router.route(31).uuid == rs_uuid[2] -vshard.router.route(61).uuid == rs_uuid[3] +vshard.router.route(1)._replicaset.uuid == rs_uuid[1] +vshard.router.route(31)._replicaset.uuid == rs_uuid[2] +vshard.router.route(61)._replicaset.uuid == rs_uuid[3] test_run:switch('default') create_router('router_3') test_run:switch('router_3') vshard.router.cfg(cfg) priority_order() -vshard.router.route(1).uuid == rs_uuid[1] -vshard.router.route(31).uuid == rs_uuid[2] -vshard.router.route(61).uuid == rs_uuid[3] +vshard.router.route(1)._replicaset.uuid == rs_uuid[1] +vshard.router.route(31)._replicaset.uuid == rs_uuid[2] +vshard.router.route(61)._replicaset.uuid == rs_uuid[3] test_run:switch('default') create_router('router_4') test_run:switch('router_4') vshard.router.cfg(cfg) priority_order() -vshard.router.route(1).uuid == rs_uuid[1] -vshard.router.route(31).uuid == rs_uuid[2] -vshard.router.route(61).uuid == rs_uuid[3] +vshard.router.route(1)._replicaset.uuid == rs_uuid[1] +vshard.router.route(31)._replicaset.uuid == rs_uuid[2] +vshard.router.route(61)._replicaset.uuid == rs_uuid[3] -- -- gh-169: do not close connections on too long ping when this is @@ -178,7 +178,7 @@ vshard.router.cfg(cfg) while not test_run:grep_log('router_1', 'Ping error from', 1000) do fiber.sleep(0.01) end t = string.rep('a', 1024 * 1024 * 500) -rs = vshard.router.route(31) +rs = vshard.router.route(31)._replicaset while rs.master ~= rs.replica do fiber.sleep(0.01) end future = nil -- Create strong reference to prevent Lua GC from closing the diff --git a/test/misc/check_uuid_on_connect.result b/test/misc/check_uuid_on_connect.result index 19f46d9c..92df215b 100644 --- a/test/misc/check_uuid_on_connect.result +++ b/test/misc/check_uuid_on_connect.result @@ -199,7 +199,7 @@ vshard.router.route(1) bucket_id: 1 ... -- Ok to work with correct replicasets. -vshard.router.route(2).uuid +vshard.router.route(2)._replicaset.uuid --- - cbf06940-0790-498b-948d-042b62cf3d29 ... diff --git a/test/misc/check_uuid_on_connect.test.lua b/test/misc/check_uuid_on_connect.test.lua index cf383727..1a0b8879 100644 --- a/test/misc/check_uuid_on_connect.test.lua +++ b/test/misc/check_uuid_on_connect.test.lua @@ -74,7 +74,7 @@ test_run:switch('bad_uuid_router') vshard.router.static.route_map[1] = nil vshard.router.route(1) -- Ok to work with correct replicasets. -vshard.router.route(2).uuid +vshard.router.route(2)._replicaset.uuid _ = test_run:cmd("switch default") test_run:cmd('stop server bad_uuid_router') diff --git a/test/multiple_routers/multiple_routers.result b/test/multiple_routers/multiple_routers.result index 9a932852..cd4e5655 100644 --- a/test/multiple_routers/multiple_routers.result +++ b/test/multiple_routers/multiple_routers.result @@ -79,11 +79,6 @@ vshard.router.call(1, 'read', 'do_select', {1}) --- - [[1, 1]] ... --- Test that static router is just a router object under the hood. -vshard.router.static:route(1) == vshard.router.route(1) ---- -- true -... -- Configure extra router. router_2 = vshard.router.new('router_2', configs.cfg_2) --- diff --git a/test/multiple_routers/multiple_routers.test.lua b/test/multiple_routers/multiple_routers.test.lua index e9be6b71..564108a1 100644 --- a/test/multiple_routers/multiple_routers.test.lua +++ b/test/multiple_routers/multiple_routers.test.lua @@ -30,9 +30,6 @@ _ = test_run:cmd("switch router_1") vshard.router.call(1, 'write', 'do_replace', {{1, 1}}) vshard.router.call(1, 'read', 'do_select', {1}) --- Test that static router is just a router object under the hood. -vshard.router.static:route(1) == vshard.router.route(1) - -- Configure extra router. router_2 = vshard.router.new('router_2', configs.cfg_2) router_2:bootstrap() diff --git a/test/router-luatest/map_callrw_test.lua b/test/router-luatest/map_callrw_test.lua index 7d225f8d..8ec25717 100644 --- a/test/router-luatest/map_callrw_test.lua +++ b/test/router-luatest/map_callrw_test.lua @@ -212,6 +212,7 @@ g.test_map_part_double_ref = function(cg) -- Make sure the location of the bucket is known. local rs, err = ivshard.router.route(bid) ilt.assert_equals(err, nil) + rs = rs._replicaset ilt.assert_equals(rs.uuid, uuid) end, {bid1, cg.rs1_uuid}) -- Then, move the bucket form rs1 to rs2. Now the router has an outdated @@ -251,7 +252,7 @@ g.test_map_part_double_ref = function(cg) ivshard.router.discovery_wakeup() local rs, err = ivshard.router.route(bid) ilt.assert_equals(err, nil) - ilt.assert_equals(rs.uuid, uuid) + ilt.assert_equals(rs._replicaset.uuid, uuid) end) end, {bid1, cg.rs1_uuid}) end @@ -273,7 +274,7 @@ g.test_map_part_ref_timeout = function(cg) for _, bid in ipairs(bids) do local rs, err = ivshard.router.route(bid) ilt.assert_equals(err, nil) - ilt.assert_equals(rs.uuid, uuid) + ilt.assert_equals(rs._replicaset.uuid, uuid) end end end, {{[cg.rs1_uuid] = {bid1, bid2}, [cg.rs2_uuid] = {bid3, bid4}}}) @@ -379,7 +380,7 @@ g.test_map_part_ref_timeout = function(cg) ivshard.router.discovery_wakeup() local rs, err = ivshard.router.route(bid) ilt.assert_equals(err, nil) - ilt.assert_equals(rs.uuid, uuid) + ilt.assert_equals(rs._replicaset.uuid, uuid) end) end, {bid2, cg.rs1_uuid}) end diff --git a/test/router-luatest/router_2_2_test.lua b/test/router-luatest/router_2_2_test.lua index 7736dd04..6f8a4c25 100644 --- a/test/router-luatest/router_2_2_test.lua +++ b/test/router-luatest/router_2_2_test.lua @@ -122,6 +122,65 @@ g.test_msgpack_args = function(g) t.assert_equals(res, 100, 'good result') end +local function router_check_replicaset_protection_init() + g.router:exec(function() + rawset(_G, "check_replicaset_protection", + function(rs) + t.assert_not_equals(rs._replicaset, nil) + t.assert_equals(next(rs, next(rs)), nil) + t.assert_not_equals(rs.call, nil) + t.assert_not_equals(rs.callrw, nil) + t.assert_not_equals(rs.callro, nil) + t.assert_not_equals(rs.callbro, nil) + t.assert_not_equals(rs.callre, nil) + t.assert_not_equals(rs.callbre, nil) + local err_msg = + "Modification of replicaset table is not allowed" + -- Adding a field is prohibited. + t.assert_error_msg_contains( + err_msg, function(t) t.new_field = 1 end, rs) + err_msg = "cannot change a protected metatable" + t.assert_error_msg_equals(err_msg, setmetatable, rs, {}) + end) + end) +end + +local function router_check_replicaset_protection_clear() + g.router:exec(function() + rawset(_G, "check_replicaset_protection", nil) + end) +end + +g.test_router_route_return_value_protection = function(g) + router_check_replicaset_protection_init() + g.router:exec(function() + _G.check_replicaset_protection(ivshard.router.route(1)) + end) + router_check_replicaset_protection_clear() +end + +g.test_router_routeall_return_value_protection = function(g) + router_check_replicaset_protection_init() + g.router:exec(function() + local replicasets = ivshard.router.routeall() + for _, rs in pairs(replicasets) do + _G.check_replicaset_protection(rs) + end + -- Adding a field. + replicasets.new_field = 1 + t.assert_equals(replicasets.new_field, 1) + replicasets = ivshard.router.routeall() + t.assert_equals(replicasets.new_field, nil) + -- Deleting a field. + local id, _ = next(replicasets) + replicasets[id] = nil + t.assert_equals(replicasets[id], nil) + replicasets = ivshard.router.routeall() + t.assert_not_equals(replicasets[id], nil) + end) + router_check_replicaset_protection_clear() +end + local function test_return_raw_template(g, mode) -- -- Normal call. @@ -1083,7 +1142,7 @@ g.test_retryable_transfer_in_progress = function(g) end) g.router:exec(function(bid, uuid) local rs = ivshard.router.route(bid) - ilt.assert_equals(rs.uuid, uuid) + ilt.assert_equals(rs._replicaset.uuid, uuid) end, {bid, g.replica_2_a:replicaset_uuid()}) -- Block bucket receive. diff --git a/test/router-luatest/router_election_auto_master_test.lua b/test/router-luatest/router_election_auto_master_test.lua index 336d4cb9..7efc9855 100644 --- a/test/router-luatest/router_election_auto_master_test.lua +++ b/test/router-luatest/router_election_auto_master_test.lua @@ -67,7 +67,8 @@ end) local function router_wait_for_leader(bid, uuid) ilt.helpers.retrying({timeout = ivtest.wait_timeout}, function() ivshard.router.master_search_wakeup() - ilt.assert_equals(ivshard.router.route(bid).master.uuid, uuid) + ilt.assert_equals( + ivshard.router.route(bid)._replicaset.master.uuid, uuid) end) end diff --git a/test/router/reload.result b/test/router/reload.result index ce6780d5..93d72008 100644 --- a/test/router/reload.result +++ b/test/router/reload.result @@ -203,7 +203,7 @@ check_reloaded() -- -- Outdate old replicaset and replica objects. -- -rs = vshard.router.route(1) +rs = vshard.router.route(1)._replicaset --- ... rs:callro('echo', {'some_data'}) @@ -233,7 +233,7 @@ rs.callro(rs, 'echo', {'some_data'}) vshard.router = require('vshard.router') --- ... -rs = vshard.router.route(1) +rs = vshard.router.route(1)._replicaset --- ... rs:callro('echo', {'some_data'}) @@ -258,7 +258,7 @@ cfg.connection_outdate_delay = old_connection_delay vshard.router.static.connection_outdate_delay = nil --- ... -rs_new = vshard.router.route(1) +rs_new = vshard.router.route(1)._replicaset --- ... rs_old = rs diff --git a/test/router/reload.test.lua b/test/router/reload.test.lua index 57c71b47..aab1825e 100644 --- a/test/router/reload.test.lua +++ b/test/router/reload.test.lua @@ -114,7 +114,7 @@ check_reloaded() -- -- Outdate old replicaset and replica objects. -- -rs = vshard.router.route(1) +rs = vshard.router.route(1)._replicaset rs:callro('echo', {'some_data'}) package.loaded["vshard.router"] = nil _ = require('vshard.router') @@ -122,7 +122,7 @@ _ = require('vshard.router') while not rs.is_outdated do fiber.sleep(0.001) end rs.callro(rs, 'echo', {'some_data'}) vshard.router = require('vshard.router') -rs = vshard.router.route(1) +rs = vshard.router.route(1)._replicaset rs:callro('echo', {'some_data'}) -- Test `connection_outdate_delay`. old_connection_delay = cfg.connection_outdate_delay @@ -130,7 +130,7 @@ cfg.connection_outdate_delay = 0.3 vshard.router.cfg(cfg) cfg.connection_outdate_delay = old_connection_delay vshard.router.static.connection_outdate_delay = nil -rs_new = vshard.router.route(1) +rs_new = vshard.router.route(1)._replicaset rs_old = rs _, replica_old = next(rs_old.replicas) rs_new:callro('echo', {'some_data'}) diff --git a/test/router/router.result b/test/router/router.result index 0425b3a9..1a6112d1 100644 --- a/test/router/router.result +++ b/test/router/router.result @@ -454,7 +454,7 @@ util.check_error(vshard.router.route) --- - 'Usage: router.route(bucket_id)' ... -conn = vshard.router.route(1).master.conn +conn = vshard.router.route(1)._replicaset.master.conn --- ... conn.state @@ -482,7 +482,7 @@ rs.master = master master.conn:close() --- ... -conn = vshard.router.route(1):connect() +conn = vshard.router.route(1)._replicaset:connect() --- ... conn:wait_connected() diff --git a/test/router/router.test.lua b/test/router/router.test.lua index c71b9d46..ccb70a80 100644 --- a/test/router/router.test.lua +++ b/test/router/router.test.lua @@ -169,7 +169,7 @@ _ = test_run:cmd('start server storage_2_a') vshard.router.route(vshard.consts.DEFAULT_BUCKET_COUNT + 100) util.check_error(vshard.router.route, 'asdfg') util.check_error(vshard.router.route) -conn = vshard.router.route(1).master.conn +conn = vshard.router.route(1)._replicaset.master.conn conn.state -- Test missing master. rs = vshard.router.static.replicasets[util.replicasets[2]] @@ -179,7 +179,7 @@ vshard.router.route(1).master rs.master = master -- Test reconnect on bucker_route(). master.conn:close() -conn = vshard.router.route(1):connect() +conn = vshard.router.route(1)._replicaset:connect() conn:wait_connected() conn.state diff --git a/test/router/router2.result b/test/router/router2.result index 684d8d60..5a0db964 100644 --- a/test/router/router2.result +++ b/test/router/router2.result @@ -352,7 +352,7 @@ vshard.consts.REPLICA_BACKOFF_INTERVAL = 0.1 | ... -- Indeed fails when called directly via netbox. -conn = vshard.router.route(1).master.conn +conn = vshard.router.route(1)._replicaset.master.conn | --- | ... ok, err = pcall(conn.call, conn, 'vshard.storage.call', \ @@ -473,7 +473,7 @@ test_run:switch('router_1') | --- | - true | ... -conn = vshard.router.route(1).master.conn +conn = vshard.router.route(1)._replicaset.master.conn | --- | ... ok, err = pcall(conn.call, conn, 'vshard.storage.call', \ diff --git a/test/router/router2.test.lua b/test/router/router2.test.lua index c5e0dd31..5abb59c0 100644 --- a/test/router/router2.test.lua +++ b/test/router/router2.test.lua @@ -139,7 +139,7 @@ test_run:switch('router_1') vshard.consts.REPLICA_BACKOFF_INTERVAL = 0.1 -- Indeed fails when called directly via netbox. -conn = vshard.router.route(1).master.conn +conn = vshard.router.route(1)._replicaset.master.conn ok, err = pcall(conn.call, conn, 'vshard.storage.call', \ {1, 'read', 'echo', {1}}) assert(not ok and err.code == box.error.ACCESS_DENIED) @@ -188,7 +188,7 @@ vshard.storage.call = nil -- Indeed fails when called directly via netbox. test_run:switch('router_1') -conn = vshard.router.route(1).master.conn +conn = vshard.router.route(1)._replicaset.master.conn ok, err = pcall(conn.call, conn, 'vshard.storage.call', \ {1, 'read', 'echo', {1}}) assert(not ok and err.code == box.error.NO_SUCH_PROC) diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua index d3bf0dba..b7df5f3f 100644 --- a/vshard/replicaset.lua +++ b/vshard/replicaset.lua @@ -1568,6 +1568,42 @@ local function cluster_calculate_etalon_balance(replicasets, bucket_count) end end +local replicaset_public_mt = { + __index = { + call = function(self, ...) + return self._replicaset:call(...) + end, + callrw = function(self, ...) + return self._replicaset:callrw(...) + end, + callro = function(self, ...) + return self._replicaset:callro(...) + end, + callbro = function(self, ...) + return self._replicaset:callbro(...) + end, + callre = function(self, ...) + return self._replicaset:callre(...) + end, + callbre = function(self, ...) + return self._replicaset:callbre(...) + end, + }, + __newindex = function() + error("Modification of replicaset table is not allowed") + end, + __metatable = "Replicaset table is protected", +} + +local function make_replicasets_public(replicasets) + local replicasets_public = {} + for key, rs in pairs(replicasets) do + replicasets_public[key] = + setmetatable({ _replicaset = rs, }, replicaset_public_mt) + end + return replicasets_public +end + -- -- Update/build replicasets from configuration -- @@ -2147,5 +2183,6 @@ return { create_workers = create_workers, locate_masters = locate_masters, -- Functions, exported for testing. - _worker_service_to_func = worker_service_to_func + _worker_service_to_func = worker_service_to_func, + make_replicasets_public = make_replicasets_public, } diff --git a/vshard/router/init.lua b/vshard/router/init.lua index d748585d..c74610b1 100644 --- a/vshard/router/init.lua +++ b/vshard/router/init.lua @@ -1168,7 +1168,11 @@ local function router_route(router, bucket_id) if type(bucket_id) ~= 'number' then error('Usage: router.route(bucket_id)') end - return bucket_resolve(router, bucket_id) + local replicaset, err = bucket_resolve(router, bucket_id) + if replicaset == nil then + return nil, err + end + return router.replicasets_public[replicaset.id], err end -- @@ -1176,7 +1180,7 @@ end -- @retval See self.replicasets map. -- local function router_routeall(router) - return router.replicasets + return table.copy(router.replicasets_public) end -------------------------------------------------------------------------------- @@ -1269,6 +1273,9 @@ local function router_cfg(router, cfg, is_reload) router.total_bucket_count = vshard_cfg.bucket_count router.current_cfg = cfg router.replicasets = new_replicasets + router.replicasets_public = + lreplicaset.make_replicasets_public(new_replicasets) + router.failover_ping_timeout = vshard_cfg.failover_ping_timeout router.sync_timeout = vshard_cfg.sync_timeout local old_route_map = router.route_map local known_bucket_count = 0