From c83517c4061feb48a934ce4290368a0c2cb78381 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Thu, 8 Dec 2022 19:00:46 +0100 Subject: [PATCH] Several improvements to RESTAPI interface #4001 This commit pack a couple of fixes and improvements for the RESTAPI: - Homogenization of GET/POST endpoint responses by ProxySQL. All responses should now be guaranteed to be valid JSON. - Fix JSON construction from parameters supplied to GET endpoint. - Add two new fields 'script_stdout' and 'script_stderr' to the JSON response when the target script fails to be executed. This is, when it exists with an error code other than zero. This makes the response homogeneous to when the scripts fails to produce a valid JSON output, and add extra information for debugging on client side. - Add a new debugging module 'PROXY_DEBUG_RESTAPI', to help tracing in debugging builds the requests to the endpoints. --- include/proxysql_structs.h | 1 + lib/ProxySQL_RESTAPI_Server.cpp | 217 ++++++++++++++++++-------------- lib/debug.cpp | 1 + 3 files changed, 128 insertions(+), 91 deletions(-) diff --git a/include/proxysql_structs.h b/include/proxysql_structs.h index 82a5ceed09..bb0ef79788 100644 --- a/include/proxysql_structs.h +++ b/include/proxysql_structs.h @@ -128,6 +128,7 @@ enum debug_module { PROXY_DEBUG_IPC, PROXY_DEBUG_QUERY_CACHE, PROXY_DEBUG_QUERY_STATISTICS, + PROXY_DEBUG_RESTAPI, PROXY_DEBUG_UNKNOWN // this module doesn't exist. It is used only to define the last possible module }; diff --git a/lib/ProxySQL_RESTAPI_Server.cpp b/lib/ProxySQL_RESTAPI_Server.cpp index 900d4f179d..94374f204b 100644 --- a/lib/ProxySQL_RESTAPI_Server.cpp +++ b/lib/ProxySQL_RESTAPI_Server.cpp @@ -3,14 +3,10 @@ #include "httpserver.hpp" #include -#include #include "ProxySQL_RESTAPI_Server.hpp" #include "proxysql_utils.h" -using namespace httpserver; - - #ifdef DEBUG #define DEB "_DEBUG" #else @@ -20,6 +16,9 @@ using namespace httpserver; extern ProxySQL_Admin *GloAdmin; +using namespace httpserver; +using nlohmann::json; + class sync_resource : public http_resource { private: void add_headers(std::shared_ptr &response) { @@ -29,61 +28,90 @@ class sync_resource : public http_resource { const std::shared_ptr find_script(const http_request& req, std::string& script, int &interval_ms) { char *error=NULL; - std::stringstream ss; - ss << "SELECT * FROM runtime_restapi_routes WHERE uri='" << req.get_path_piece(1) << "' and method='" << req.get_method() << "' and active=1"; - std::unique_ptr resultset = std::unique_ptr(GloAdmin->admindb->execute_statement(ss.str().c_str(), &error)); + const string req_uri { req.get_path_piece(1) }; + const string req_path { req.get_path() }; + const string select_query { + "SELECT * FROM runtime_restapi_routes WHERE uri='" + req_uri + "' and" + " method='" + req.get_method() + "' and active=1" + }; + + std::unique_ptr resultset { + std::unique_ptr(GloAdmin->admindb->execute_statement(select_query.c_str(), &error)) + }; + if (!resultset) { - proxy_error("Cannot query script for given method [%s] and uri [%s]\n", req.get_method().c_str(), req.get_path_piece(1).c_str()); - std::stringstream ss; + proxy_error( + "Cannot query script for given method [%s] and uri [%s]\n", req.get_method().c_str(), req_uri.c_str() + ); + const string not_found_err_msg { + "The script for method [" + req.get_method() + "] and route [" + req.get_path() + "] was not found." + }; + json j_err_resp {}; + if (error) { - ss << "{\"error\":\"The script for method [" << req.get_method() << "] and route [" << req.get_path() << "] was not found. Error: " << error << "Error: \"}"; - proxy_error("Path %s, error %s\n", req.get_path().c_str(), error); - } - else { - ss << "{\"error\":\"The script for method [" << req.get_method() << "] and route [" << req.get_path() << "] was not found.\"}"; - proxy_error("Path %s\n", req.get_path().c_str()); + j_err_resp = json { { "error", not_found_err_msg + " Error:" + error } }; + proxy_error("Path '%s', error '%s'\n", req_path.c_str(), error); + } else { + j_err_resp = json { { "error", not_found_err_msg } }; + proxy_error("Path '%s', error 'Path failed to be found on 'runtime_restapi_routes'\n", req_path.c_str()); } - auto response = std::shared_ptr(new string_response(ss.str(), http::http_utils::http_bad_request)); + + auto response = + std::shared_ptr(new string_response(j_err_resp.dump(), http::http_utils::http_bad_request)); add_headers(response); + return response; - } - if (resultset && resultset->rows_count != 1) { - std::stringstream ss; - ss << "{\"error\":\"The script for method [" << req.get_method() << "] and route [" << req.get_path() << "] was not found. Rows count returned [" << resultset->rows_count << "]\" }"; - proxy_error("Script for method [%s] and route [%s] was not found\n", req.get_method().c_str(), req.get_path().c_str()); - auto response = std::shared_ptr(new string_response(ss.str(), http::http_utils::http_bad_request)); + } else if (resultset && resultset->rows_count != 1) { + const string not_found_err_msg { + "The script for method [" + req.get_method() + "] and route [" + req_path + "] was not found." + " Rows count returned [" + std::to_string(resultset->rows_count) + "]" + }; + json j_err_resp { { "error", not_found_err_msg } }; + + proxy_error( + "Script for method [%s] and uri [%s] was not found\n", req.get_method().c_str(), req_uri.c_str() + ); + auto response = + std::shared_ptr(new string_response(j_err_resp.dump(), http::http_utils::http_bad_request)); add_headers(response); + return response; + } else { + script = resultset->rows[0]->fields[5]; + interval_ms = atoi(resultset->rows[0]->fields[2]); + + return std::shared_ptr(nullptr); } - script = resultset->rows[0]->fields[5]; - interval_ms = atoi(resultset->rows[0]->fields[2]); - return std::shared_ptr(nullptr); } const std::shared_ptr process_request(const http_request& req, const std::string& _params) { std::string params = req.get_content(); + const string req_path { req.get_path() }; + if (params.empty()) params = _params; if (params.empty()) { - proxy_error("Empty parameters\n"); - + proxy_error("Path '%s', error 'Supplied empty parameters'\n", req_path.c_str()); + json j_err_resp { { "type", "in" }, { "error", "Empty parameters" } }; auto response = std::shared_ptr( - new string_response("{\"error\":\"Empty parameters\"}", http::http_utils::http_bad_request) + new string_response(j_err_resp.dump(), http::http_utils::http_bad_request) ); + add_headers(response); return response; } try { nlohmann::json valid=nlohmann::json::parse(params); - } - catch(nlohmann::json::exception& e) { - std::stringstream ss; - ss << "{\"type\":\"in\", \"error\":\"" << e.what() << "\"}"; - proxy_error("Error parsing input json parameters. %s\n", ss.str().c_str()); + } catch(nlohmann::json::exception& e) { + const string p_err_msg { + "parsing input JSON parameters - params: `" + params + "`, error: '" + e.what() + "'" + }; + json j_err_resp { { "type", "in" }, { "error", "Error " + p_err_msg } }; + proxy_error("Path '%s', error %s\n", req_path.c_str(), p_err_msg.c_str()); auto response = std::shared_ptr( - new string_response(ss.str(), http::http_utils::http_bad_request) + new string_response(j_err_resp.dump(), http::http_utils::http_bad_request) ); add_headers(response); return response; @@ -107,7 +135,9 @@ class sync_resource : public http_resource { std::string script_stderr {""}; const std::vector args { const_cast(params.c_str()), NULL}; + proxy_debug(PROXY_DEBUG_RESTAPI, 2, "Starting script exec - script: '%s', params: `%s`\n", script.c_str(), params.c_str()); int script_err = wexecvp(script.c_str(), args, wexecvp_opts, script_stdout, script_stderr); + proxy_debug(PROXY_DEBUG_RESTAPI, 2, "Finished script exec - script: '%s', params: `%s`\n", script.c_str(), params.c_str()); int script_errno = errno; std::string str_response_err {}; @@ -115,10 +145,14 @@ class sync_resource : public http_resource { // 'execvp' failed to be executed, the error code comes directly from the child process if (script_err > 255) { - str_response_err = - std::string { "{\"type\":\"out\", \"error\":\"Script failed to be executed.\", \"error_code\":\"" } - + std::to_string(script_err / 256) - + "\"}"; + json j_err_resp { + { "type", "out" }, + { "error", "Script failed to be executed" }, + { "error_code", std::to_string(script_err / 256) }, + { "script_stdout", script_stdout }, + { "script_stderr", script_stderr } + }; + str_response_err = j_err_resp.dump(); proxy_error( "Script '%s' exited with errcode '%d': \n- script_stdout:\n'''\n%s'''\n- script_stderr:\n'''\n%s'''\n", script.c_str(), @@ -130,11 +164,12 @@ class sync_resource : public http_resource { // there was an internal error while calling the executable, or request timedout. else if (script_err < 256 && script_err != 0) { if (script_err == ETIME) { - str_response_err = - std::string { "{\"type\":\"out\", \"error\":\"Script execution timed out.\", \"error_code\":\"" } - + std::to_string(ETIME) - + "\"}"; - + json j_err_resp { + { "type", "out" }, + { "error", "Script execution timed out" }, + { "error_code", std::to_string(ETIME) } + }; + str_response_err = j_err_resp.dump(); proxy_error("Request to execute script '%s' timed out.\n", script.c_str()); } else if (script_err < 0) { // there was an internal error unrelated to script execution @@ -159,12 +194,12 @@ class sync_resource : public http_resource { failed_syscall = "unknown"; break; } - str_response_err = - std::string { - "{\"type\":\"out\"," } + - "\"error\":\"Internal error while executing script, '" + failed_syscall + "' syscall failed.\", " + - "\"error_code\":\"" + std::to_string(script_errno) - + "\"}"; + json j_err_resp { + { "type", "out" }, + { "error", "Internal error while executing script, '" + failed_syscall + "' syscall failed" }, + { "error_code", std::to_string(script_errno) } + }; + str_response_err = j_err_resp.dump(); proxy_error( "Internal error while executing script '%s'. '%s' syscall failed with error code: '%d'.\n", script.c_str(), @@ -172,43 +207,41 @@ class sync_resource : public http_resource { script_errno ); } else { - str_response_err = - "{\"type\":\"out\", \"error\":\"Terminated without exit code. Child exit status reported in" - " 'error_code'\", \"error_code\":\"" + std::to_string(script_err) + "\"}"; - proxy_error( - "Error while executing script '%s'. Child exit status: '%d'.\n", script.c_str(), script_err - ); + json j_err_resp { + { "type", "out" }, + { "error", "Terminated without exit code. Child exit status reported in 'error_code'" }, + { "error_code", std::to_string(script_err) } + }; + str_response_err = j_err_resp.dump(); + proxy_error("Error while executing script '%s'. Child exit status: '%d'.\n", script.c_str(), script_err); } } // script returned and empty output, invalid output, no need to parse it. else if (script_stdout.empty()) { - str_response_err = - std::string { - "{\"type\":\"out\"," } + - "\"error\":\"Script response is empty, only valid JSONs are accepted.\", " + - "\"error_code\":\"" + std::to_string(0) - + "\"}"; + json j_err_resp { + { "type", "out" }, + { "error", "Script response is empty, only valid JSONs are accepted" }, + { "error_code", std::to_string(0) } + }; + str_response_err = j_err_resp.dump(); proxy_error("Invalid empty response from script: '%s'\n", script.c_str()); } // execution completed successfully without timing out. else { try { nlohmann::json j { nlohmann::json::parse(script_stdout.c_str()) }; - } - catch(nlohmann::json::exception& e) { - std::string escaped_stdout { replace_str(script_stdout, std::string { '"' }, "\\\"") }; - - str_response_err = - std::string { "{" } + - std::string { "\"type\":\"out\", \"error\":\"" } + e.what() + - std::string { ", \"error_code\":\"" } + std::to_string(script_err / 256) + "\"" + - std::string { ", \"script_stdout\":\"" } + escaped_stdout + "\"" + - std::string { ", \"script_stderr\":\"" } + script_stderr + "\"" + - "}"; + } catch(nlohmann::json::exception& e) { + json j_err_resp { + { "type", "out" }, + { "error", e.what() }, + { "error_code", std::to_string(script_err / 256) }, + { "script_stdout", script_stdout }, + { "script_stderr", script_stderr } + }; + str_response_err = j_err_resp.dump(); proxy_error( "Error parsing script output from script: '%s'\n- script_stdout:\n'''\n%s'''\n", - script.c_str(), - script_stdout.c_str() + script.c_str(), script_stdout.c_str() ); } } @@ -240,35 +273,37 @@ class sync_resource : public http_resource { public: const std::shared_ptr render(const http_request& req) { proxy_info("Render generic request with method %s for uri %s\n", req.get_method().c_str(), req.get_path().c_str()); - std::stringstream ss; - ss << "{\"error\":\"HTTP method " << req.get_method().c_str() << " is not implemented\"}"; - - auto response = std::shared_ptr(new string_response(ss.str().c_str())); + json j_err_resp {{ "error", "HTTP method " + req.get_method() + " is not implemented" }}; + auto response = std::shared_ptr(new string_response(j_err_resp.dump())); response->with_header("Content-Type", "application/json"); response->with_header("Access-Control-Allow-Origin", "*"); + return response; } const std::shared_ptr render_GET(const http_request& req) { - auto args = req.get_args(); - - size_t last = 0; - std::stringstream params; - params << "{"; - for (auto arg : args) { - params << "\"" << arg.first << "\":\"" << arg.second << "\""; - if (last < args.size()-1) { - params << ","; - last++; - } + const auto args = req.get_args(); + + // Explicit object creation, otherwise 'array' is initialized + json input_params = json::object(); + for (const auto& arg : args) { + input_params.push_back({arg.first, arg.second}); } - params << "}"; - return process_request(req, params.str()); + const char* req_path { req.get_path().c_str() }; + const string s_params { input_params.dump() }; + const char* p_params { s_params.c_str() }; + proxy_debug(PROXY_DEBUG_RESTAPI, 1, "Processing GET - req: '%s', params: `%s`\n", req_path, p_params); + + return process_request(req, s_params); } const std::shared_ptr render_POST(const http_request& req) { std::string params=req.get_content(); + const char* req_path { req.get_path().c_str() }; + const char* p_params { params.c_str() }; + proxy_debug(PROXY_DEBUG_RESTAPI, 1, "Processing POST - req: '%s', params: `%s`\n", req_path, p_params); + return process_request(req, params); } diff --git a/lib/debug.cpp b/lib/debug.cpp index 08305b93e5..b963115a94 100644 --- a/lib/debug.cpp +++ b/lib/debug.cpp @@ -401,6 +401,7 @@ void init_debug_struct() { GloVars.global.gdbg_lvl[PROXY_DEBUG_IPC].name=(char *)"debug_ipc"; GloVars.global.gdbg_lvl[PROXY_DEBUG_QUERY_CACHE].name=(char *)"debug_query_cache"; GloVars.global.gdbg_lvl[PROXY_DEBUG_QUERY_STATISTICS].name=(char *)"debug_query_statistics"; + GloVars.global.gdbg_lvl[PROXY_DEBUG_RESTAPI].name=(char *)"debug_restapi"; for (i=0;i