router: forbid using cdata as bucket_id

Using cdata (e.g. 1ULL) as bucket id was allowed in all requests of the
router. But using it caused the growth of the `route_map` table on every
request and full scan of the cluster in order to find, where the bucket
is. This happened due to the fact, that cdata is compared by its pointer,
not the value, and consequently, as there was no such pointer key in the
`route_map`, it was added and scanned every time.

It was decided to prohibit such usage, from now on we expect users to do
`tonumber(bucket_id)`, if it's needed. The motivation for such solution
is the fact, that all functions except calls already prohibit using
cdata as bucket_id, and calls should be consistent with it. Another
point is the fact, that the behavior of the `map_callrw` with cdata as
bucket ids is very untrivial.

The alternative solution of allowing usage of cdata in calls and manually
converting values to number in `bucket_set`, `bucket_reset` and
`bucket_resolved` was rejected due to the above mentioned reasons.

Unfortunately, the behavior of `map_callrw` with array of cdata is still
untrivial, as {10ULL, 20ULL} is interpreted as {1 = 10, 2 = 20}, and the
request is executed on the replicaset with 1st and 2nd bucket.

Closes #594

NO_DOC=bugfix

Author: Serpentian

test/router-luatest/map_callrw_test.lua

@@ -647,3 +647,41 @@ g.test_map_callrw_array_encoded_as_map = function(cg)
         end
     end, nil, bids)
 end
+
+--
+-- gh-594: using cdata as bucket_id should be prohibited, as it causes fullscan
+-- of the cluster and subsequent memory leak in the route_map.
+--
+g.test_map_callrw_with_cdata_bucket_id = function(cg)
+    cg.router:exec(function()
+        -- vshard.router._buckets_group is checked for the sake of unit
+        -- testing of the map_callrw() protection.
+        local msg = 'is not a number'
+        local _, err = ivshard.router._buckets_group({1ULL, 2ULL}, 1)
+        ilt.assert_str_contains(err.message, msg)
+        _, err = ivshard.router._buckets_group({1, 2ULL}, 1)
+        ilt.assert_str_contains(err.message, msg)
+        _, err = ivshard.router._buckets_group({1ULL, 2}, 1)
+        ilt.assert_str_contains(err.message, msg)
+
+        -- Map_callrw in general forbids using cdata.
+        _, err = ivshard.router.map_callrw('assert', {'a'},
+            {bucket_ids = {1, 2ULL}})
+        ilt.assert_str_contains(err.message, msg)
+        _, err = ivshard.router.map_callrw('assert', {'a'},
+            {bucket_ids = {[1] = 'a', [2ULL] = 'b'}})
+        ilt.assert_str_contains(err.message, msg)
+        _, err = ivshard.router.map_callrw('assert', {'a'},
+            {bucket_ids = {[1ULL] = 'a', [2ULL] = 'b'}})
+        ilt.assert_str_contains(err.message, msg)
+
+        local res
+        -- But there's one case, where it's not possible to forbid that,
+        -- since it's interpreted as {1 = 5ULL, 2 = 6ULL}. If there's a way,
+        -- which will allow to forbid that, it'd be great.
+        res, err = ivshard.router.map_callrw('assert', {'a'},
+            {bucket_ids = {5ULL, 6ULL}})
+        ilt.assert(res)
+        ilt.assert_not(err)
+    end)
+end

test/router-luatest/router_2_2_test.lua

@@ -1165,3 +1165,39 @@ g.test_discovery_can_be_restarted_fast = function(g)
     t.assert_not(g.router:grep_log('assertion failed'))
     vtest.router_cfg(g.router, global_cfg)
 end
+
+--
+-- gh-594: using cdata as bucket_id should be prohibited, as it causes fullscan
+-- of the cluster and subsequent memory leak in the route_map.
+--
+g.test_cdata_as_bucket_id_is_prohibited = function(g)
+    g.router:exec(function()
+        local msg = 'Usage: '
+        ilt.assert_error_msg_contains(msg, function()
+            ivshard.router.buckets_info(1ULL, 2ULL)
+        end)
+        ilt.assert_error_msg_contains(msg, function()
+            ivshard.router.call(1ULL, 'read', 'assert', {'a'})
+        end)
+        ilt.assert_error_msg_contains(msg, function()
+            ivshard.router.callro(1ULL, 'assert', {'a'})
+        end)
+        ilt.assert_error_msg_contains(msg, function()
+            ivshard.router.callbro(1ULL, 'assert', {'a'})
+        end)
+        ilt.assert_error_msg_contains(msg, function()
+            ivshard.router.callrw(1ULL, 'assert', {'a'})
+        end)
+        ilt.assert_error_msg_contains(msg, function()
+            ivshard.router.callre(1ULL, 'assert', {'a'})
+        end)
+        ilt.assert_error_msg_contains(msg, function()
+            ivshard.router.callbre(1ULL, 'assert', {'a'})
+        end)
+        ilt.assert_error_msg_contains(msg, function()
+            ivshard.router.route(1ULL)
+        end)
+        -- vshard.router._bucket_reset() is not public and doesn't require
+        -- protection from the misusage.
+    end)
+end

vshard/router/init.lua

@@ -206,6 +206,10 @@ local function buckets_group(router, bucket_ids, timeout)
         if bucket_map[bucket_id] then
             goto continue
         end
+        if type(bucket_id) ~= 'number' then
+            local msg = string.format("Bucket '%s' is not a number", bucket_id)
+            return nil, lerror.make(msg)
+        end
         -- Avoid duplicates.
         bucket_map[bucket_id] = true
         if fiber_clock() > deadline then
@@ -609,6 +613,9 @@ local function router_call_impl(router, bucket_id, mode, prefer_replica,
     end
     local replicaset, err
     local tend = fiber_clock() + timeout
+    if type(bucket_id) ~= 'number' then
+        error('Usage: call(bucket_id, mode, func, args, opts)')
+    end
     if bucket_id > router.total_bucket_count or bucket_id <= 0 then
         error('Bucket is unreachable: bucket id is out of range')
     end
@@ -1140,8 +1147,10 @@ local function router_map_callrw(router, func, args, opts)
         timeout, err, err_id, rid, replicasets_to_map =
             router_ref_storage_by_buckets(router, plain_bucket_ids, timeout)
         -- Grouped arguments are only possible with partial Map-Reduce.
-        grouped_args =
-            router_group_map_callrw_args(router, plain_bucket_ids, bucket_ids)
+        if timeout then
+            grouped_args = router_group_map_callrw_args(
+                router, plain_bucket_ids, bucket_ids)
+        end
     else
         timeout, err, err_id, rid, replicasets_to_map =
             router_ref_storage_all(router, timeout)