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 C4D1E43E891 for ; Wed, 25 Mar 2020 04:43:17 +0300 (MSK) From: Nikita Pettik Date: Wed, 25 Mar 2020 04:43:06 +0300 Message-Id: <3bb7c695b34356137897b953281543c46fe3bafa.1585097339.git.korablev@tarantool.org> In-Reply-To: References: In-Reply-To: References: Subject: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org This patch introduces support of stacked errors in IProto protocol and in net.box module. @TarantoolBot document Title: Stacked error diagnostic area Starting from now errors can be organized into lists. To achieve this Lua table representing error object is extended with .prev field and e:set_prev(err) method. .prev field returns previous error if any exist. e:set_prev(err) method expects err to be error object or nil and sets err as previous error of e. For instance: e1 = box.error.new({code = 111, reason = "cause"}) e2 = box.error.new({code = 111, reason = "cause of cause"}) e1:set_prev(e2) assert(e1.prev == e2) -- true Cycles are not allowed for error lists: e2:set_prev(e1) - error: 'builtin/error.lua: Cycles are not allowed' Nil is valid input to :set_prev() method: e1:set_prev(nil) assert(e1.prev == nil) -- true Note that error can be 'previous' only to the one error at once: e1:set_prev(e2) e3:set_prev(e2) assert(e1.prev == nil) -- true assert(e3.prev == e2) -- true Setting previous error does not erase its own previous members: -- e1 -> e2 -> e3 -> e4 e1:set_prev(e2) e2:set_prev(e3) e3:set_prev(e4) e2:set_prev(e5) -- Now there are two lists: e1->e2->e5 and e3->e4 assert(e1.prev == e2) -- true assert(e2.prev == e5) -- true assert(e3.prev == e4) -- true Alternatively: e1:set_prev(e2) e2:set_prev(e3) e3:set_prev(e4) e5:set_prev(e3) -- Now there are two lists: e1->e2 and e5->e3->e4 assert(e1.prev == e2) -- true assert(e2.prev == nil) -- true assert(e5.prev == e3) -- true assert(e3.prev == e4) -- true Stacked diagnostics is also supported by IProto protocol. Now responses containing errors always (even if there's only one error to be returned) include new IProto key: IPROTO_ERROR_STACK (0x51). So, body corresponding to error response now looks like: MAP{IPROTO_ERROR : string, IPROTO_ERROR_STACK : ARRAY[MAP{ERROR_CODE : uint, ERROR_MESSAGE : string}, MAP{...}, ...]} where IPROTO_ERROR is 0x31 key, IPROTO_ERROR_STACK is 0x51, ERROR_CODE is 0x01 and ERROR_MESSAGE is 0x02. Instances of older versions (without support of stacked errors in protocol) simply ignore unknown keys and still rely only on IPROTO_ERROR key. Closes #1148 --- src/box/error.cc | 17 +++++ src/box/error.h | 16 +++++ src/box/iproto_constants.h | 6 ++ src/box/lua/net_box.lua | 32 ++++++++- src/box/xrow.c | 88 ++++++++++++++++++++--- test/box-py/iproto.result | 6 +- test/box-py/iproto.test.py | 6 +- test/box/iproto.result | 141 +++++++++++++++++++++++++++++++++++++ test/box/iproto.test.lua | 62 ++++++++++++++++ test/box/net.box.result | 60 ++++++++++++++++ test/box/net.box.test.lua | 23 ++++++ 11 files changed, 440 insertions(+), 17 deletions(-) create mode 100644 test/box/iproto.result create mode 100644 test/box/iproto.test.lua diff --git a/src/box/error.cc b/src/box/error.cc index 824a4617c..e3197c8e6 100644 --- a/src/box/error.cc +++ b/src/box/error.cc @@ -102,6 +102,23 @@ box_error_construct(const char *file, unsigned line, uint32_t code, return e; } +int +box_error_add(const char *file, unsigned line, uint32_t code, + const char *fmt, ...) +{ + struct error *e = BuildClientError(file, line, ER_UNKNOWN); + ClientError *client_error = type_cast(ClientError, e); + if (client_error) { + client_error->m_errcode = code; + va_list ap; + va_start(ap, fmt); + error_vformat_msg(e, fmt, ap); + va_end(ap); + } + diag_add_error(&fiber()->diag, e); + return -1; +} + /* }}} */ struct rmean *rmean_error = NULL; diff --git a/src/box/error.h b/src/box/error.h index ef9cf3e76..24fc157bb 100644 --- a/src/box/error.h +++ b/src/box/error.h @@ -137,6 +137,22 @@ box_error_set(const char *file, unsigned line, uint32_t code, /** \endcond public */ +/** + * Add error to the diagnostic area. In contrast to box_error_set() + * it does not replace previous error being set, but rather link + * them into list. + * + * \param code IPROTO error code (enum \link box_error_code \endlink) + * \param format (const char * ) - printf()-like format string + * \param ... - format arguments + * \returns -1 for convention use + * + * \sa enum box_error_code + */ +int +box_error_add(const char *file, unsigned line, uint32_t code, + const char *fmt, ...); + /** * Construct error object without setting it in the diagnostics * area. On the memory allocation fail returns NULL. diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h index f9d413a31..7ed829645 100644 --- a/src/box/iproto_constants.h +++ b/src/box/iproto_constants.h @@ -126,6 +126,7 @@ enum iproto_key { /* Leave a gap between SQL keys and additional request keys */ IPROTO_REPLICA_ANON = 0x50, IPROTO_ID_FILTER = 0x51, + IPROTO_ERROR_STACK = 0x52, IPROTO_KEY_MAX }; @@ -150,6 +151,11 @@ enum iproto_ballot_key { IPROTO_BALLOT_IS_LOADING = 0x04, }; +enum iproto_error_key { + IPROTO_ERROR_CODE = 0x01, + IPROTO_ERROR_MESSAGE = 0x02, +}; + #define bit(c) (1ULL<errmsg); + + uint32_t err_cnt = 0; + for (const struct error *it = error; it != NULL; it = it->cause) + err_cnt++; + mpstream_encode_uint(stream, IPROTO_ERROR_STACK); + mpstream_encode_array(stream, err_cnt); + for (const struct error *it = error; it != NULL; it = it->cause) { + mpstream_encode_map(stream, 2); + mpstream_encode_uint(stream, IPROTO_ERROR_CODE); + mpstream_encode_uint(stream, box_error_code(it)); + mpstream_encode_uint(stream, IPROTO_ERROR_MESSAGE); + mpstream_encode_str(stream, it->errmsg); + } } int @@ -1070,19 +1083,66 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len, return 0; } +static int +iproto_decode_error_stack(const char **pos, const char *end) +{ + const char *reason = NULL; + static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\ + "expected to be larger than error message max length"); + /* + * Erase previously set diag errors. It is required since + * box_error_add() does not replace previous errors. + */ + box_error_clear(); + if (mp_check_array(*pos, end) != 0) + return -1; + uint32_t stack_sz = mp_decode_array(pos); + for (uint32_t i = 0; i < stack_sz; i++) { + uint32_t code = 0; + if (mp_typeof(**pos) != MP_MAP || mp_check_map(*pos, end) != 0) + return -1; + uint32_t map_sz = mp_decode_map(pos); + for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) { + if (mp_typeof(**pos) != MP_UINT || + mp_check_uint(*pos, end) != 0) + return -1; + uint8_t key = mp_decode_uint(pos); + if (key == IPROTO_ERROR_CODE) { + if (mp_typeof(**pos) != MP_UINT || + mp_check_uint(*pos, end) != 0) + return -1; + code = mp_decode_uint(pos); + } else if (key == IPROTO_ERROR_MESSAGE) { + if (mp_typeof(**pos) != MP_STR || + mp_check_strl(*pos, end) != 0) + return -1; + uint32_t len; + const char *str = mp_decode_str(pos, &len); + reason = tt_cstr(str, len); + } else { + mp_next(pos); + continue; + } + box_error_add(__FILE__, __LINE__, code, reason); + } + } + return 0; +} + void xrow_decode_error(struct xrow_header *row) { uint32_t code = row->type & (IPROTO_TYPE_ERROR - 1); char error[DIAG_ERRMSG_MAX] = { 0 }; - const char *pos; + const char *pos, *end; uint32_t map_size; if (row->bodycnt == 0) goto error; pos = (char *) row->body[0].iov_base; - if (mp_check(&pos, pos + row->body[0].iov_len)) + end = pos + row->body[0].iov_len; + if (mp_check(&pos, end)) goto error; pos = (char *) row->body[0].iov_base; @@ -1096,15 +1156,27 @@ xrow_decode_error(struct xrow_header *row) continue; } uint8_t key = mp_decode_uint(&pos); - if (key != IPROTO_ERROR || mp_typeof(*pos) != MP_STR) { - mp_next(&pos); /* value */ + if (key == IPROTO_ERROR && mp_typeof(*pos) == MP_STR) { + /* + * Obsolete way of sending error responses. + * To be deprecated but still should be supported + * to not break backward compatibility. + */ + uint32_t len; + const char *str = mp_decode_str(&pos, &len); + snprintf(error, sizeof(error), "%.*s", len, str); + box_error_set(__FILE__, __LINE__, code, error); + } else if (key == IPROTO_ERROR_STACK ) { + if (mp_check_array(pos, end) != 0) + goto error; + if (iproto_decode_error_stack(&pos, end) != 0) + continue; + } else { + mp_next(&pos); continue; } - - uint32_t len; - const char *str = mp_decode_str(&pos, &len); - snprintf(error, sizeof(error), "%.*s", len, str); } + return; error: box_error_set(__FILE__, __LINE__, code, error); diff --git a/test/box-py/iproto.result b/test/box-py/iproto.result index 900e6e24f..04e2e220c 100644 --- a/test/box-py/iproto.result +++ b/test/box-py/iproto.result @@ -169,9 +169,9 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe') # Test bugs gh-272, gh-1654 if the packet was incorrect, respond with # an error code and do not close connection -sync=0, {49: 'Invalid MsgPack - packet header'} -sync=1234, {49: "Missing mandatory field 'space id' in request"} -sync=5678, {49: "Read access to space '_user' is denied for user 'guest'"} +sync=0, Invalid MsgPack - packet header +sync=1234, Missing mandatory field 'space id' in request +sync=5678, Read access to space '_user' is denied for user 'guest' space = box.schema.space.create('test_index_base', { id = 568 }) --- ... diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py index 77637d8ed..cdd1a71c5 100644 --- a/test/box-py/iproto.test.py +++ b/test/box-py/iproto.test.py @@ -355,15 +355,15 @@ s = c._socket header = { "hello": "world"} body = { "bug": 272 } resp = test_request(header, body) -print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body']) +print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR)) header = { IPROTO_CODE : REQUEST_TYPE_SELECT } header[IPROTO_SYNC] = 1234 resp = test_request(header, body) -print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body']) +print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR)) header[IPROTO_SYNC] = 5678 body = { IPROTO_SPACE_ID: 304, IPROTO_KEY: [], IPROTO_LIMIT: 1 } resp = test_request(header, body) -print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body']) +print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR)) c.close() diff --git a/test/box/iproto.result b/test/box/iproto.result new file mode 100644 index 000000000..bb369be5a --- /dev/null +++ b/test/box/iproto.result @@ -0,0 +1,141 @@ +test_run = require('test_run').new() +--- +... +net_box = require('net.box') +--- +... +urilib = require('uri') +--- +... +msgpack = require('msgpack') +--- +... +IPROTO_REQUEST_TYPE = 0x00 +--- +... +IPROTO_INSERT = 0x02 +--- +... +IPROTO_SYNC = 0x01 +--- +... +IPROTO_SPACE_ID = 0x10 +--- +... +IPROTO_TUPLE = 0x21 +--- +... +IPROTO_ERROR = 0x31 +--- +... +IPROTO_ERROR_STACK = 0x52 +--- +... +IPROTO_ERROR_CODE = 0x01 +--- +... +IPROTO_ERROR_MESSAGE = 0x02 +--- +... +IPROTO_OK = 0x00 +--- +... +IPROTO_SCHEMA_VERSION = 0x05 +--- +... +IPROTO_STATUS_KEY = 0x00 +--- +... +-- gh-1148: test capabilities of stacked diagnostics bypassing net.box. +-- +lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]] +--- +... +box.schema.func.create('f1', {body = lua_func, is_deterministic = true, is_sandboxed = true}) +--- +... +s = box.schema.space.create('s') +--- +... +pk = s:create_index('pk') +--- +... +idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) +--- +... +box.schema.user.grant('guest', 'read, write, execute', 'universe') +--- +... +next_request_id = 16 +--- +... +header = { \ + [IPROTO_REQUEST_TYPE] = IPROTO_INSERT, \ + [IPROTO_SYNC] = next_request_id, \ +} +--- +... +body = { \ + [IPROTO_SPACE_ID] = s.id, \ + [IPROTO_TUPLE] = box.tuple.new({1}) \ +} +--- +... +uri = urilib.parse(box.cfg.listen) +--- +... +sock = net_box.establish_connection(uri.host, uri.service) +--- +... +response = iproto_request(sock, header, body) +--- +... +sock:close() +--- +- true +... +-- Both keys (obsolete and stack ones) are present in response. +-- +assert(response.body[IPROTO_ERROR_STACK] ~= nil) +--- +- true +... +assert(response.body[IPROTO_ERROR] ~= nil) +--- +- true +... +err = response.body[IPROTO_ERROR_STACK][1] +--- +... +assert(err[IPROTO_ERROR_MESSAGE] == body[IPROTO_ERROR]) +--- +- error: assertion failed! +... +err = response.body[IPROTO_ERROR_STACK][2] +--- +... +assert(err[IPROTO_ERROR_CODE] ~= nil) +--- +- true +... +assert(type(err[IPROTO_ERROR_CODE]) == 'number') +--- +- true +... +assert(err[IPROTO_ERROR_MESSAGE] ~= nil) +--- +- true +... +assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string') +--- +- true +... +box.schema.user.revoke('guest', 'read,write,execute', 'universe') +--- +... +s:drop() +--- +... +box.func.f1:drop() +--- +... diff --git a/test/box/iproto.test.lua b/test/box/iproto.test.lua new file mode 100644 index 000000000..c16fe84c1 --- /dev/null +++ b/test/box/iproto.test.lua @@ -0,0 +1,62 @@ +test_run = require('test_run').new() +net_box = require('net.box') +urilib = require('uri') +msgpack = require('msgpack') + +IPROTO_REQUEST_TYPE = 0x00 +IPROTO_INSERT = 0x02 +IPROTO_SYNC = 0x01 +IPROTO_SPACE_ID = 0x10 +IPROTO_TUPLE = 0x21 +IPROTO_ERROR = 0x31 +IPROTO_ERROR_STACK = 0x52 +IPROTO_ERROR_CODE = 0x01 +IPROTO_ERROR_MESSAGE = 0x02 +IPROTO_OK = 0x00 +IPROTO_SCHEMA_VERSION = 0x05 +IPROTO_STATUS_KEY = 0x00 + +-- gh-1148: test capabilities of stacked diagnostics bypassing net.box. +-- +lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]] + +box.schema.func.create('f1', {body = lua_func, is_deterministic = true, is_sandboxed = true}) +s = box.schema.space.create('s') +pk = s:create_index('pk') +idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) + +box.schema.user.grant('guest', 'read, write, execute', 'universe') + +next_request_id = 16 +header = { \ + [IPROTO_REQUEST_TYPE] = IPROTO_INSERT, \ + [IPROTO_SYNC] = next_request_id, \ +} + +body = { \ + [IPROTO_SPACE_ID] = s.id, \ + [IPROTO_TUPLE] = box.tuple.new({1}) \ +} + +uri = urilib.parse(box.cfg.listen) +sock = net_box.establish_connection(uri.host, uri.service) + +response = iproto_request(sock, header, body) +sock:close() + +-- Both keys (obsolete and stack ones) are present in response. +-- +assert(response.body[IPROTO_ERROR_STACK] ~= nil) +assert(response.body[IPROTO_ERROR] ~= nil) + +err = response.body[IPROTO_ERROR_STACK][1] +assert(err[IPROTO_ERROR_MESSAGE] == body[IPROTO_ERROR]) +err = response.body[IPROTO_ERROR_STACK][2] +assert(err[IPROTO_ERROR_CODE] ~= nil) +assert(type(err[IPROTO_ERROR_CODE]) == 'number') +assert(err[IPROTO_ERROR_MESSAGE] ~= nil) +assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string') + +box.schema.user.revoke('guest', 'read,write,execute', 'universe') +s:drop() +box.func.f1:drop() diff --git a/test/box/net.box.result b/test/box/net.box.result index e3dabf7d9..eeefc0e3f 100644 --- a/test/box/net.box.result +++ b/test/box/net.box.result @@ -3902,6 +3902,66 @@ sock:close() --- - true ... +-- gh-1148: test stacked diagnostics. +-- +test_run:cmd("push filter \"file: .*\" to \"file: \"") +--- +- true +... +lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]] +--- +... +box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true}) +--- +... +s = box.schema.space.create('s') +--- +... +pk = s:create_index('pk') +--- +... +idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) +--- +... +box.schema.user.grant('guest', 'read,write,execute', 'universe') +--- +... +c = net.connect(box.cfg.listen) +--- +... +f = function(...) return c.space.s:insert(...) end +--- +... +_, e = pcall(f, {1}) +--- +... +assert(e ~= nil) +--- +- true +... +e:unpack().message +--- +- 'Failed to build a key for functional index ''idx'' of space ''s'': can''t evaluate + function' +... +e.prev.message +--- +- error: '[string "return e.prev.message "]:1: attempt to index field ''prev'' (a + nil value)' +... +box.schema.user.revoke('guest', 'read,write,execute', 'universe') +--- +... +s:drop() +--- +... +box.func.f1:drop() +--- +... +test_run:cmd("clear filter") +--- +- true +... test_run:wait_log('default', 'Got a corrupted row.*', nil, 10) --- - 'Got a corrupted row:' diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua index 8e65ff470..7e3571d43 100644 --- a/test/box/net.box.test.lua +++ b/test/box/net.box.test.lua @@ -1574,6 +1574,29 @@ data = string.fromhex('3C'..string.rep(require('digest').sha1_hex('bcde'), 3)) sock:write(data) sock:close() +-- gh-1148: test stacked diagnostics. +-- +test_run:cmd("push filter \"file: .*\" to \"file: \"") +lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]] +box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true}) +s = box.schema.space.create('s') +pk = s:create_index('pk') +idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) + +box.schema.user.grant('guest', 'read,write,execute', 'universe') +c = net.connect(box.cfg.listen) +f = function(...) return c.space.s:insert(...) end +_, e = pcall(f, {1}) +assert(e ~= nil) + +e:unpack().message +e.prev.message + +box.schema.user.revoke('guest', 'read,write,execute', 'universe') +s:drop() +box.func.f1:drop() +test_run:cmd("clear filter") + test_run:wait_log('default', 'Got a corrupted row.*', nil, 10) test_run:wait_log('default', '00000000:.*', nil, 10) test_run:wait_log('default', '00000010:.*', nil, 10) -- 2.17.1