From ba888f11e481d4a652c568d3ac83bc0721cd86c8 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Wed, 27 May 2020 21:48:19 +0000 Subject: [PATCH 01/10] Create common SQL library to be used by SQL filters. Signed-off-by: Christoph Pakulski --- source/extensions/common/sql/BUILD | 19 +++++++++ source/extensions/common/sql/sqlutils.cc | 39 +++++++++++++++++++ source/extensions/common/sql/sqlutils.h | 18 +++++++++ .../filters/network/mysql_proxy/BUILD | 2 +- .../network/mysql_proxy/mysql_filter.cc | 33 ++++------------ 5 files changed, 84 insertions(+), 27 deletions(-) create mode 100644 source/extensions/common/sql/BUILD create mode 100644 source/extensions/common/sql/sqlutils.cc create mode 100644 source/extensions/common/sql/sqlutils.h diff --git a/source/extensions/common/sql/BUILD b/source/extensions/common/sql/BUILD new file mode 100644 index 0000000000000..d79de709ee101 --- /dev/null +++ b/source/extensions/common/sql/BUILD @@ -0,0 +1,19 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "sqlutils_lib", + srcs = ["sqlutils.cc"], + hdrs = ["sqlutils.h"], + external_deps = ["sqlparser"], + deps = [ + "//source/common/protobuf:utility_lib", + ], +) diff --git a/source/extensions/common/sql/sqlutils.cc b/source/extensions/common/sql/sqlutils.cc new file mode 100644 index 0000000000000..226904b7dc835 --- /dev/null +++ b/source/extensions/common/sql/sqlutils.cc @@ -0,0 +1,39 @@ +#include "extensions/common/sql/sqlutils.h" + +namespace Envoy { +namespace Extensions { +namespace Common { +namespace SQLUtils { + +bool SQLutils::setMetadata(const std::string& query, ProtobufWkt::Struct& metadata) { + hsql::SQLParserResult result; + hsql::SQLParser::parse(query, &result); + + if (!result.isValid()) { + return false; + } + + // Set dynamic metadata + auto& fields = *metadata.mutable_fields(); + + for (auto i = 0u; i < result.size(); ++i) { + if (result.getStatement(i)->type() == hsql::StatementType::kStmtShow) { + continue; + } + hsql::TableAccessMap table_access_map; + result.getStatement(i)->tablesAccessed(table_access_map); + for (auto& it : table_access_map) { + auto& operations = *fields[it.first].mutable_list_value(); + for (const auto& ot : it.second) { + operations.add_values()->set_string_value(ot); + } + } + } + + return true; +} + +} // namespace SQLUtils +} // namespace Common +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/common/sql/sqlutils.h b/source/extensions/common/sql/sqlutils.h new file mode 100644 index 0000000000000..b342431a462a0 --- /dev/null +++ b/source/extensions/common/sql/sqlutils.h @@ -0,0 +1,18 @@ +#include "common/protobuf/utility.h" + +#include "include/sqlparser/SQLParser.h" + +namespace Envoy { +namespace Extensions { +namespace Common { +namespace SQLUtils { + +class SQLutils { +public: + static bool setMetadata(const std::string& query, ProtobufWkt::Struct& metadata); +}; + +} // namespace SQLUtils +} // namespace Common +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/mysql_proxy/BUILD b/source/extensions/filters/network/mysql_proxy/BUILD index bd27d007f8b2d..5bf9030b6a77e 100644 --- a/source/extensions/filters/network/mysql_proxy/BUILD +++ b/source/extensions/filters/network/mysql_proxy/BUILD @@ -36,7 +36,6 @@ envoy_cc_library( "mysql_session.h", "mysql_utils.h", ], - external_deps = ["sqlparser"], deps = [ "//include/envoy/network:filter_interface", "//include/envoy/server:filter_config_interface", @@ -44,6 +43,7 @@ envoy_cc_library( "//include/envoy/stats:stats_macros", "//source/common/buffer:buffer_lib", "//source/common/network:filter_lib", + "//source/extensions/common/sql:sqlutils_lib", "//source/extensions/filters/network:well_known_names", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], diff --git a/source/extensions/filters/network/mysql_proxy/mysql_filter.cc b/source/extensions/filters/network/mysql_proxy/mysql_filter.cc index 0d8be2394d150..7cd7b07ebf57f 100644 --- a/source/extensions/filters/network/mysql_proxy/mysql_filter.cc +++ b/source/extensions/filters/network/mysql_proxy/mysql_filter.cc @@ -6,10 +6,9 @@ #include "common/common/assert.h" #include "common/common/logger.h" +#include "extensions/common/sql/sqlutils.h" #include "extensions/filters/network/well_known_names.h" -#include "include/sqlparser/SQLParser.h" - namespace Envoy { namespace Extensions { namespace NetworkFilters { @@ -105,39 +104,21 @@ void MySQLFilter::onCommand(Command& command) { } // Parse a given query - hsql::SQLParserResult result; - hsql::SQLParser::parse(command.getData(), &result); + envoy::config::core::v3::Metadata& dynamic_metadata = + read_callbacks_->connection().streamInfo().dynamicMetadata(); + ProtobufWkt::Struct metadata( + (*dynamic_metadata.mutable_filter_metadata())[NetworkFilterNames::get().MySQLProxy]); + auto result = Common::SQLutils::SQLutils::setMetadata(command.getData(), metadata); ENVOY_CONN_LOG(trace, "mysql_proxy: query processed {}", read_callbacks_->connection(), command.getData()); - if (!result.isValid()) { + if (!result) { config_->stats_.queries_parse_error_.inc(); return; } config_->stats_.queries_parsed_.inc(); - // Set dynamic metadata - envoy::config::core::v3::Metadata& dynamic_metadata = - read_callbacks_->connection().streamInfo().dynamicMetadata(); - ProtobufWkt::Struct metadata( - (*dynamic_metadata.mutable_filter_metadata())[NetworkFilterNames::get().MySQLProxy]); - auto& fields = *metadata.mutable_fields(); - - for (auto i = 0u; i < result.size(); ++i) { - if (result.getStatement(i)->type() == hsql::StatementType::kStmtShow) { - continue; - } - hsql::TableAccessMap table_access_map; - result.getStatement(i)->tablesAccessed(table_access_map); - for (auto& it : table_access_map) { - auto& operations = *fields[it.first].mutable_list_value(); - for (const auto& ot : it.second) { - operations.add_values()->set_string_value(ot); - } - } - } - read_callbacks_->connection().streamInfo().setDynamicMetadata( NetworkFilterNames::get().MySQLProxy, metadata); } From c4f5a6adbc42ac244e4e5fece5378d7936b36d62 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Fri, 29 May 2020 17:38:40 +0000 Subject: [PATCH 02/10] Added unit tests to sqlutils library. Fixed a bug in mysql testing routines which masked wrong results returned by SQL parser. Signed-off-by: Christoph Pakulski --- .../extensions/common/{sql => sqlutils}/BUILD | 0 .../common/{sql => sqlutils}/sqlutils.cc | 2 +- .../common/{sql => sqlutils}/sqlutils.h | 0 .../filters/network/mysql_proxy/BUILD | 2 +- .../network/mysql_proxy/mysql_filter.cc | 4 +- test/extensions/common/sqlutils/BUILD | 20 +++ .../common/sqlutils/sqlutils_test.cc | 166 ++++++++++++++++++ .../network/mysql_proxy/mysql_command_test.cc | 15 +- 8 files changed, 201 insertions(+), 8 deletions(-) rename source/extensions/common/{sql => sqlutils}/BUILD (100%) rename source/extensions/common/{sql => sqlutils}/sqlutils.cc (95%) rename source/extensions/common/{sql => sqlutils}/sqlutils.h (100%) create mode 100644 test/extensions/common/sqlutils/BUILD create mode 100644 test/extensions/common/sqlutils/sqlutils_test.cc diff --git a/source/extensions/common/sql/BUILD b/source/extensions/common/sqlutils/BUILD similarity index 100% rename from source/extensions/common/sql/BUILD rename to source/extensions/common/sqlutils/BUILD diff --git a/source/extensions/common/sql/sqlutils.cc b/source/extensions/common/sqlutils/sqlutils.cc similarity index 95% rename from source/extensions/common/sql/sqlutils.cc rename to source/extensions/common/sqlutils/sqlutils.cc index 226904b7dc835..75d6e63714a74 100644 --- a/source/extensions/common/sql/sqlutils.cc +++ b/source/extensions/common/sqlutils/sqlutils.cc @@ -1,4 +1,4 @@ -#include "extensions/common/sql/sqlutils.h" +#include "extensions/common/sqlutils/sqlutils.h" namespace Envoy { namespace Extensions { diff --git a/source/extensions/common/sql/sqlutils.h b/source/extensions/common/sqlutils/sqlutils.h similarity index 100% rename from source/extensions/common/sql/sqlutils.h rename to source/extensions/common/sqlutils/sqlutils.h diff --git a/source/extensions/filters/network/mysql_proxy/BUILD b/source/extensions/filters/network/mysql_proxy/BUILD index 5bf9030b6a77e..49dae0b335d35 100644 --- a/source/extensions/filters/network/mysql_proxy/BUILD +++ b/source/extensions/filters/network/mysql_proxy/BUILD @@ -43,7 +43,7 @@ envoy_cc_library( "//include/envoy/stats:stats_macros", "//source/common/buffer:buffer_lib", "//source/common/network:filter_lib", - "//source/extensions/common/sql:sqlutils_lib", + "//source/extensions/common/sqlutils:sqlutils_lib", "//source/extensions/filters/network:well_known_names", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], diff --git a/source/extensions/filters/network/mysql_proxy/mysql_filter.cc b/source/extensions/filters/network/mysql_proxy/mysql_filter.cc index 7cd7b07ebf57f..f9335c387faa0 100644 --- a/source/extensions/filters/network/mysql_proxy/mysql_filter.cc +++ b/source/extensions/filters/network/mysql_proxy/mysql_filter.cc @@ -6,7 +6,7 @@ #include "common/common/assert.h" #include "common/common/logger.h" -#include "extensions/common/sql/sqlutils.h" +#include "extensions/common/sqlutils/sqlutils.h" #include "extensions/filters/network/well_known_names.h" namespace Envoy { @@ -108,7 +108,7 @@ void MySQLFilter::onCommand(Command& command) { read_callbacks_->connection().streamInfo().dynamicMetadata(); ProtobufWkt::Struct metadata( (*dynamic_metadata.mutable_filter_metadata())[NetworkFilterNames::get().MySQLProxy]); - auto result = Common::SQLutils::SQLutils::setMetadata(command.getData(), metadata); + auto result = Common::SQLUtils::SQLutils::setMetadata(command.getData(), metadata); ENVOY_CONN_LOG(trace, "mysql_proxy: query processed {}", read_callbacks_->connection(), command.getData()); diff --git a/test/extensions/common/sqlutils/BUILD b/test/extensions/common/sqlutils/BUILD new file mode 100644 index 0000000000000..d5b75130f0e75 --- /dev/null +++ b/test/extensions/common/sqlutils/BUILD @@ -0,0 +1,20 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_package", +) + +envoy_package() + +envoy_cc_test( + name = "sqlutils_tests", + srcs = [ + "sqlutils_test.cc", + ], + external_deps = ["sqlparser"], + deps = [ + "//source/extensions/common/sqlutils:sqlutils_lib", + ], +) diff --git a/test/extensions/common/sqlutils/sqlutils_test.cc b/test/extensions/common/sqlutils/sqlutils_test.cc new file mode 100644 index 0000000000000..5336d1ae405f2 --- /dev/null +++ b/test/extensions/common/sqlutils/sqlutils_test.cc @@ -0,0 +1,166 @@ +#include "extensions/common/sqlutils/sqlutils.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Common { +namespace SQLUtils { + +// MetadataFromSQLTest class is used for parameterized tests. +// The values in the tests are: +// std::string - SQL query +// bool - whether to expect SQL parsing to be successful +// std::map> map of expected tables accessed based on the query. +// The map is checked only when parsing was successful. Map is indexed by table name and points to +// list of operations performed on the table. For example table1: "select", "insert" says that there +// was SELECT and INSERT operations on table1. +class MetadataFromSQLTest + : public ::testing::TestWithParam< + std::tuple>>> {}; + +// Test takes SQL query as a parameter and checks if the parsing +// produces the correct metadata. +// Metadata is 2-level structure. First layer is list of resources +// over which the SQL query operates: in our case is list of tables. +// Under each table there is secondary list which contains operations performed +// on the table, like "select", "insert", etc. +TEST_P(MetadataFromSQLTest, ParsingAndMetadataTest) { + // Get the SQL query + std::string query = std::get<0>(GetParam()); + ProtobufWkt::Struct metadata; + + // Check if the parsing result is what expected. + ASSERT_EQ(std::get<1>(GetParam()), SQLutils::setMetadata(query, metadata)); + + // If parsing was expected to fail do not check parsing values. + if (!std::get<1>(GetParam())) { + return; + } + + // Access metadata fields, where parsing results are stored. + auto& fields = *metadata.mutable_fields(); + + // Get the names of resources which SQL query operates on. + std::map> expected_tables = std::get<2>(GetParam()); + // Check if query results return the same number of resources as expected. + ASSERT_EQ(expected_tables.size(), fields.size()); + for (const auto& i : fields) { + // Get from created metadata the list of operations on the resource + const auto& operations = i; + // Get the list of expected operations on the same resource from test param. + auto table_name_it = expected_tables.find(operations.first); + // Make sure that a resource (table) found in metadata is expected. + ASSERT_NE(expected_tables.end(), table_name_it); + auto& operations_list = table_name_it->second; + // The number of expected operations and created in metadata must be the same. + ASSERT_EQ(operations_list.size(), operations.second.list_value().values().size()); + // Now iterate over the operations list found in metadata and check if the same operation + // is listed as expected in test param. + for (const auto& j : operations.second.list_value().values()) { + // Find that operation in test params. + auto operation_it = + std::find(operations_list.begin(), operations_list.end(), j.string_value()); + ASSERT_NE(operations_list.end(), operation_it); + // Erase the operation. At the end of the test this list should be empty what means + // that we found all expected operations. + operations_list.erase(operation_it); + } + // Make sure that we went through all expected operations. + ASSERT_TRUE(operations_list.empty()); + // Remove the table from the list. At the end of the test this list must be empty. + expected_tables.erase(table_name_it); + } + + ASSERT_TRUE(expected_tables.empty()); +} + +#define TEST_VALUE(...) \ + std::tuple>> { __VA_ARGS__ } +INSTANTIATE_TEST_SUITE_P( + SQLUtilsTestSuite, MetadataFromSQLTest, + ::testing::Values( + TEST_VALUE("blahblah;", false, {}), + + TEST_VALUE("CREATE TABLE IF NOT EXISTS table1(Usr VARCHAR(40),Count INT);", true, + {{"table1", {"create"}}}), + TEST_VALUE("CREATE TABLE IF NOT EXISTS `table number 1`(Usr VARCHAR(40),Count INT);", true, + {{"table number 1", {"create"}}}), + TEST_VALUE( + "CREATE TABLE IF NOT EXISTS table1(Usr VARCHAR(40),Count INT); SELECT * from table1;", + true, {{"table1", {"select", "create"}}}), + TEST_VALUE( + "CREATE TABLE IF NOT EXISTS table1(Usr VARCHAR(40),Count INT); SELECT * from table2;", + true, {{"table1", {"create"}}, {"table2", {"select"}}}), + + TEST_VALUE("CREATE TABLE table1(Usr VARCHAR(40),Count INT);", true, + {{"table1", {"create"}}}), + TEST_VALUE("CREATE TABLE;", false, {}), + TEST_VALUE("CREATE TEMPORARY table table1(Usr VARCHAR(40),Count INT);", true, + {{"table1", {"create"}}}), + TEST_VALUE("DROP TABLE IF EXISTS table1", true, {{"table1", {"drop"}}}), + TEST_VALUE("ALTER TABLE table1 add column Id varchar (20);", true, {{"table1", {"alter"}}}), + TEST_VALUE("INSERT INTO table1 (Usr, Count) VALUES ('allsp2', 3);", true, + {{"table1", {"insert"}}}), + TEST_VALUE("INSERT LOW_PRIORITY INTO table1 (Usr, Count) VALUES ('allsp2', 3);", true, + {{"table1", {"insert"}}}), + TEST_VALUE("INSERT IGNORE INTO table1 (Usr, Count) VALUES ('allsp2', 3);", true, + {{"table1", {"insert"}}}), + TEST_VALUE("INSERT INTO table1 (Usr, Count) VALUES ('allsp2', 3);SELECT * from table1", + true, {{"table1", {"insert", "select"}}}), + TEST_VALUE("DELETE FROM table1 WHERE Count > 3;", true, {{"table1", {"delete"}}}), + TEST_VALUE("DELETE LOW_PRIORITY FROM table1 WHERE Count > 3;", true, + {{"table1", {"delete"}}}), + TEST_VALUE("DELETE QUICK FROM table1 WHERE Count > 3;", true, {{"table1", {"delete"}}}), + TEST_VALUE("DELETE IGNORE FROM table1 WHERE Count > 3;", true, {{"table1", {"delete"}}}), + + TEST_VALUE("SELECT * FROM table1 WHERE Count = 1;", true, {{"table1", {"select"}}}), + TEST_VALUE("SELECT * FROM table1 WHERE Count = 1;", true, {{"table1", {"select"}}}), + TEST_VALUE("SELECT Product.category FROM table1 WHERE Count = 1;", true, + {{"table1", {"select"}}, {"Product", {"unknown"}}}), + TEST_VALUE("SELECT DISTINCT Usr FROM table1;", true, {{"table1", {"select"}}}), + TEST_VALUE("SELECT Usr, Count FROM table1 ORDER BY Count DESC;", true, + {{"table1", {"select"}}}), + TEST_VALUE("SELECT 12 AS a, a FROM table1 GROUP BY a;", true, {{"table1", {"select"}}}), + TEST_VALUE("SELECT;", false, {}), TEST_VALUE("SELECT Usr, Count FROM;", false, {}), + TEST_VALUE("INSERT INTO table1 SELECT * FROM table2;", true, + {{"table1", {"insert"}}, {"table2", {"select"}}}), + TEST_VALUE("INSERT INTO table1 SELECT tbl_temp1.fld_order_id FROM table2;", true, + {{"tbl_temp1", {"unknown"}}, {"table2", {"select"}}, {"table1", {"insert"}}}), + TEST_VALUE("UPDATE table1 SET col1 = col1 + 1", true, {{"table1", {"update"}}}), + TEST_VALUE("UPDATE LOW_PRIORITY table1 SET col1 = col1 + 1", true, + {{"table1", {"update"}}}), + TEST_VALUE("UPDATE IGNORE table1 SET col1 = col1 + 1", true, {{"table1", {"update"}}}), + TEST_VALUE("UPDATE table1 SET column1=(SELECT * columnX from table2);", true, + {{"table1", {"update"}}, {"table2", {"select"}}}), + + // operations on database should not create any metadata + TEST_VALUE("CREATE DATABASE testdb;", true, {}), + TEST_VALUE("CREATE DATABASE IF NOT EXISTS testdb;", true, {}), + TEST_VALUE("ALTER DATABASE testdb CHARACTER SET charset_name;", true, {}), + TEST_VALUE("ALTER DATABASE testdb default CHARACTER SET charset_name;", true, {}), + TEST_VALUE("ALTER DATABASE testdb default CHARACTER SET = charset_name;", true, {}), + TEST_VALUE("ALTER SCHEMA testdb default CHARACTER SET = charset_name;", true, {}), + + // The following DROP DATABASE tests should not produce metadata, + // but the SQL parser returns info that table 'testdb' was accessed. + // This if a bug. Once SQL parser if fixed the following tests should + // be enabled. + // SPELLCHECKER(off) + // TEST_VALUE("DROP DATABASE testdb;", true, {}), + // TEST_VALUE("DROP DATABASE IF EXISTS testdb;", true, {}), + // SPELLCHECKER(on) + + // Schema. Should be parsed fine, but should not produce any metadata + TEST_VALUE("SHOW databases;", true, {}), TEST_VALUE("SHOW tables;", true, {}), + TEST_VALUE("SELECT * FROM;", false, {}), + TEST_VALUE("SELECT 1 FROM tabletest1;", true, {{"tabletest1", {"select"}}}) + + )); + +// std::tuple>>{ +//"SELECT 1 FROM tabletest1;", true, {{"tabletest1", {"select"}}}})); +} // namespace SQLUtils +} // namespace Common +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc b/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc index cce430facd773..1d17b97fa6e3c 100644 --- a/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc +++ b/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc @@ -203,7 +203,7 @@ class MySQLCommandTest : public testing::Test, public MySQLTestUtils { EXPECT_EQ(1UL, result.size()); EXPECT_EQ(statement_type, result.getStatement(0)->type()); hsql::TableAccessMap table_access_map; - if (expected_table_access_map.empty()) { + if (expected_table_access_map.empty() && (statement_type == hsql::StatementType::kStmtShow)) { return; } result.getStatement(0)->tablesAccessed(table_access_map); @@ -454,13 +454,17 @@ TEST_F(MySQLCommandTest, MySQLTest20) { std::string command = buildAlter(TestResource::TABLE, table, "add column Id varchar (20)"); hsql::SQLParserResult result; EXPECT_EQ(MYSQL_SUCCESS, encodeQuery(command, result)); - expectStatementTypeAndTableAccessMap(result, hsql::StatementType::kStmtAlter, {}); + expectStatementTypeAndTableAccessMap(result, hsql::StatementType::kStmtAlter, + {{table, {"alter"}}}); } /* * Test query: "DROP DATABASE " + * Test is disabled because of a bug in SQL parsing library. + * The library should return that none of database tables has been accessed, + * but it returns that operation drop has been performed on TABLE 'mysqldb'. */ -TEST_F(MySQLCommandTest, MySQLTest21) { +TEST_F(MySQLCommandTest, DISABLED_MySQLTest21) { std::string db = "mysqldb"; std::string command = buildDrop(TestResource::DB, false, db); hsql::SQLParserResult result; @@ -471,8 +475,11 @@ TEST_F(MySQLCommandTest, MySQLTest21) { /* * Test query with optional cmd: * "DROP DATABASE IF EXISTS " + * Test is disabled because of a bug in SQL parsing library. + * The library should return that none of database tables has been accessed, + * but it returns that operation drop has been performed on TABLE 'mysqldb'. */ -TEST_F(MySQLCommandTest, MySQLTest22) { +TEST_F(MySQLCommandTest, DISABLED_MySQLTest22) { std::string db = "mysqldb"; std::string command = buildDrop(TestResource::DB, true, db); hsql::SQLParserResult result; From 5dcc00477c6ffc763be0a220443ce1a44797126e Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Fri, 29 May 2020 17:51:37 +0000 Subject: [PATCH 03/10] Small corrections to code comments. Signed-off-by: Christoph Pakulski --- source/extensions/common/sqlutils/sqlutils.cc | 3 ++- test/extensions/common/sqlutils/sqlutils_test.cc | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/source/extensions/common/sqlutils/sqlutils.cc b/source/extensions/common/sqlutils/sqlutils.cc index 75d6e63714a74..f4ac2e0cb3ff5 100644 --- a/source/extensions/common/sqlutils/sqlutils.cc +++ b/source/extensions/common/sqlutils/sqlutils.cc @@ -13,7 +13,6 @@ bool SQLutils::setMetadata(const std::string& query, ProtobufWkt::Struct& metada return false; } - // Set dynamic metadata auto& fields = *metadata.mutable_fields(); for (auto i = 0u; i < result.size(); ++i) { @@ -21,9 +20,11 @@ bool SQLutils::setMetadata(const std::string& query, ProtobufWkt::Struct& metada continue; } hsql::TableAccessMap table_access_map; + // Get names of accessed tables. result.getStatement(i)->tablesAccessed(table_access_map); for (auto& it : table_access_map) { auto& operations = *fields[it.first].mutable_list_value(); + // For each table get names of operations performed on that table. for (const auto& ot : it.second) { operations.add_values()->set_string_value(ot); } diff --git a/test/extensions/common/sqlutils/sqlutils_test.cc b/test/extensions/common/sqlutils/sqlutils_test.cc index 5336d1ae405f2..59fcc99142744 100644 --- a/test/extensions/common/sqlutils/sqlutils_test.cc +++ b/test/extensions/common/sqlutils/sqlutils_test.cc @@ -158,8 +158,6 @@ INSTANTIATE_TEST_SUITE_P( )); -// std::tuple>>{ -//"SELECT 1 FROM tabletest1;", true, {{"tabletest1", {"select"}}}})); } // namespace SQLUtils } // namespace Common } // namespace Extensions From 6f09ee97876fcf91a409db5260083aa5f52c0062 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Fri, 29 May 2020 18:15:33 +0000 Subject: [PATCH 04/10] Updated CODEOWNERS file. Signed-off-by: Christoph Pakulski --- CODEOWNERS | 1 + 1 file changed, 1 insertion(+) diff --git a/CODEOWNERS b/CODEOWNERS index 200d8fbf9e307..17fd742943a3d 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -77,6 +77,7 @@ extensions/filters/common/original_src @snowp @klarose # common crypto extension /*/extensions/common/crypto @lizan @PiotrSikora @bdecoste /*/extensions/common/proxy_protocol @alyssawilk @wez470 +/*/extensions/common/sqlutils @cpakulski @dio /*/extensions/filters/http/grpc_http1_bridge @snowp @jose /*/extensions/filters/http/gzip @gsagula @dio /*/extensions/filters/http/fault @rshriram @alyssawilk From 505d007517dc1ddabacf3f1b5a47de0154bcddb6 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Fri, 29 May 2020 19:15:52 +0000 Subject: [PATCH 05/10] Clang-tidy changes. Signed-off-by: Christoph Pakulski --- test/extensions/common/sqlutils/sqlutils_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/extensions/common/sqlutils/sqlutils_test.cc b/test/extensions/common/sqlutils/sqlutils_test.cc index 59fcc99142744..1853a107e8919 100644 --- a/test/extensions/common/sqlutils/sqlutils_test.cc +++ b/test/extensions/common/sqlutils/sqlutils_test.cc @@ -27,7 +27,7 @@ class MetadataFromSQLTest // on the table, like "select", "insert", etc. TEST_P(MetadataFromSQLTest, ParsingAndMetadataTest) { // Get the SQL query - std::string query = std::get<0>(GetParam()); + const std::string& query = std::get<0>(GetParam()); ProtobufWkt::Struct metadata; // Check if the parsing result is what expected. @@ -49,7 +49,7 @@ TEST_P(MetadataFromSQLTest, ParsingAndMetadataTest) { // Get from created metadata the list of operations on the resource const auto& operations = i; // Get the list of expected operations on the same resource from test param. - auto table_name_it = expected_tables.find(operations.first); + const auto& table_name_it = expected_tables.find(operations.first); // Make sure that a resource (table) found in metadata is expected. ASSERT_NE(expected_tables.end(), table_name_it); auto& operations_list = table_name_it->second; @@ -59,7 +59,7 @@ TEST_P(MetadataFromSQLTest, ParsingAndMetadataTest) { // is listed as expected in test param. for (const auto& j : operations.second.list_value().values()) { // Find that operation in test params. - auto operation_it = + const auto operation_it = std::find(operations_list.begin(), operations_list.end(), j.string_value()); ASSERT_NE(operations_list.end(), operation_it); // Erase the operation. At the end of the test this list should be empty what means From 119c517ff0254383de57f06ab265addea37bcb07 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Thu, 4 Jun 2020 18:30:07 +0000 Subject: [PATCH 06/10] Changed unit tests to verify uppercase and lowercse versions of SQL test queries. Signed-off-by: Christoph Pakulski --- source/extensions/common/sqlutils/sqlutils.cc | 2 +- source/extensions/common/sqlutils/sqlutils.h | 2 +- .../network/mysql_proxy/mysql_filter.cc | 2 +- .../common/sqlutils/sqlutils_test.cc | 118 +++++++++++------- 4 files changed, 77 insertions(+), 47 deletions(-) diff --git a/source/extensions/common/sqlutils/sqlutils.cc b/source/extensions/common/sqlutils/sqlutils.cc index f4ac2e0cb3ff5..023a5529e0830 100644 --- a/source/extensions/common/sqlutils/sqlutils.cc +++ b/source/extensions/common/sqlutils/sqlutils.cc @@ -5,7 +5,7 @@ namespace Extensions { namespace Common { namespace SQLUtils { -bool SQLutils::setMetadata(const std::string& query, ProtobufWkt::Struct& metadata) { +bool SQLUtils::setMetadata(const std::string& query, ProtobufWkt::Struct& metadata) { hsql::SQLParserResult result; hsql::SQLParser::parse(query, &result); diff --git a/source/extensions/common/sqlutils/sqlutils.h b/source/extensions/common/sqlutils/sqlutils.h index b342431a462a0..fde1fa26409c3 100644 --- a/source/extensions/common/sqlutils/sqlutils.h +++ b/source/extensions/common/sqlutils/sqlutils.h @@ -7,7 +7,7 @@ namespace Extensions { namespace Common { namespace SQLUtils { -class SQLutils { +class SQLUtils { public: static bool setMetadata(const std::string& query, ProtobufWkt::Struct& metadata); }; diff --git a/source/extensions/filters/network/mysql_proxy/mysql_filter.cc b/source/extensions/filters/network/mysql_proxy/mysql_filter.cc index f9335c387faa0..648171e786e31 100644 --- a/source/extensions/filters/network/mysql_proxy/mysql_filter.cc +++ b/source/extensions/filters/network/mysql_proxy/mysql_filter.cc @@ -108,7 +108,7 @@ void MySQLFilter::onCommand(Command& command) { read_callbacks_->connection().streamInfo().dynamicMetadata(); ProtobufWkt::Struct metadata( (*dynamic_metadata.mutable_filter_metadata())[NetworkFilterNames::get().MySQLProxy]); - auto result = Common::SQLUtils::SQLutils::setMetadata(command.getData(), metadata); + auto result = Common::SQLUtils::SQLUtils::setMetadata(command.getData(), metadata); ENVOY_CONN_LOG(trace, "mysql_proxy: query processed {}", read_callbacks_->connection(), command.getData()); diff --git a/test/extensions/common/sqlutils/sqlutils_test.cc b/test/extensions/common/sqlutils/sqlutils_test.cc index 1853a107e8919..77319a0532272 100644 --- a/test/extensions/common/sqlutils/sqlutils_test.cc +++ b/test/extensions/common/sqlutils/sqlutils_test.cc @@ -28,53 +28,83 @@ class MetadataFromSQLTest TEST_P(MetadataFromSQLTest, ParsingAndMetadataTest) { // Get the SQL query const std::string& query = std::get<0>(GetParam()); - ProtobufWkt::Struct metadata; - - // Check if the parsing result is what expected. - ASSERT_EQ(std::get<1>(GetParam()), SQLutils::setMetadata(query, metadata)); - - // If parsing was expected to fail do not check parsing values. - if (!std::get<1>(GetParam())) { - return; - } + // vector of queries to check. + std::vector test_queries; + test_queries.push_back(std::string(query)); + + // Create uppercase and lowercase versions of the queries and put + // them into vector of queries to check + std::string lowercase_query = query; + std::transform(query.begin(), query.end(), lowercase_query.begin(), + [](unsigned char c) { return std::tolower(c); }); + test_queries.push_back(lowercase_query); + + std::string uppercase_query = query; + std::transform(query.begin(), query.end(), uppercase_query.begin(), + [](unsigned char c) { return std::toupper(c); }); + test_queries.push_back(uppercase_query); + + while (!test_queries.empty()) { + std::string test_query = test_queries.back(); + ProtobufWkt::Struct metadata; + + // Check if the parsing result is what expected. + ASSERT_EQ(std::get<1>(GetParam()), SQLUtils::setMetadata(test_query, metadata)); + + // If parsing was expected to fail do not check parsing values. + if (!std::get<1>(GetParam())) { + return; + } - // Access metadata fields, where parsing results are stored. - auto& fields = *metadata.mutable_fields(); - - // Get the names of resources which SQL query operates on. - std::map> expected_tables = std::get<2>(GetParam()); - // Check if query results return the same number of resources as expected. - ASSERT_EQ(expected_tables.size(), fields.size()); - for (const auto& i : fields) { - // Get from created metadata the list of operations on the resource - const auto& operations = i; - // Get the list of expected operations on the same resource from test param. - const auto& table_name_it = expected_tables.find(operations.first); - // Make sure that a resource (table) found in metadata is expected. - ASSERT_NE(expected_tables.end(), table_name_it); - auto& operations_list = table_name_it->second; - // The number of expected operations and created in metadata must be the same. - ASSERT_EQ(operations_list.size(), operations.second.list_value().values().size()); - // Now iterate over the operations list found in metadata and check if the same operation - // is listed as expected in test param. - for (const auto& j : operations.second.list_value().values()) { - // Find that operation in test params. - const auto operation_it = - std::find(operations_list.begin(), operations_list.end(), j.string_value()); - ASSERT_NE(operations_list.end(), operation_it); - // Erase the operation. At the end of the test this list should be empty what means - // that we found all expected operations. - operations_list.erase(operation_it); + // Access metadata fields, where parsing results are stored. + auto& fields = *metadata.mutable_fields(); + + // Get the names of resources which SQL query operates on. + std::map> expected_tables = std::get<2>(GetParam()); + // Check if query results return the same number of resources as expected. + ASSERT_EQ(expected_tables.size(), fields.size()); + for (const auto& i : fields) { + // Get from created metadata the list of operations on the resource + const auto& operations = i; + std::string table_name = operations.first; + + std::transform(table_name.begin(), table_name.end(), table_name.begin(), + [](unsigned char c) { return std::tolower(c); }); + // Get the list of expected operations on the same resource from test param. + const auto& table_name_it = expected_tables.find(table_name); + // Make sure that a resource (table) found in metadata is expected. + ASSERT_NE(expected_tables.end(), table_name_it); + auto& operations_list = table_name_it->second; + // The number of expected operations and created in metadata must be the same. + ASSERT_EQ(operations_list.size(), operations.second.list_value().values().size()); + // Now iterate over the operations list found in metadata and check if the same operation + // is listed as expected in test param. + for (const auto& j : operations.second.list_value().values()) { + // Find that operation in test params. + const auto operation_it = + std::find(operations_list.begin(), operations_list.end(), j.string_value()); + ASSERT_NE(operations_list.end(), operation_it); + // Erase the operation. At the end of the test this list should be empty what means + // that we found all expected operations. + operations_list.erase(operation_it); + } + // Make sure that we went through all expected operations. + ASSERT_TRUE(operations_list.empty()); + // Remove the table from the list. At the end of the test this list must be empty. + expected_tables.erase(table_name_it); } - // Make sure that we went through all expected operations. - ASSERT_TRUE(operations_list.empty()); - // Remove the table from the list. At the end of the test this list must be empty. - expected_tables.erase(table_name_it); - } - ASSERT_TRUE(expected_tables.empty()); + ASSERT_TRUE(expected_tables.empty()); + test_queries.pop_back(); + } } +// Note: This parameterized test's queries are converted to all lowercase and all uppercase +// to validate that parser is case-insensitive. The test routine converts to uppercase and +// lowercase entire query string, not only SQL keywords. This introduces a problem when comparing +// tables' names when verifying parsing result. Therefore the test converts table names to lowercase +// before comparing. It however requires that all table names in the queries below use lowercase +// only. #define TEST_VALUE(...) \ std::tuple>> { __VA_ARGS__ } INSTANTIATE_TEST_SUITE_P( @@ -116,8 +146,8 @@ INSTANTIATE_TEST_SUITE_P( TEST_VALUE("SELECT * FROM table1 WHERE Count = 1;", true, {{"table1", {"select"}}}), TEST_VALUE("SELECT * FROM table1 WHERE Count = 1;", true, {{"table1", {"select"}}}), - TEST_VALUE("SELECT Product.category FROM table1 WHERE Count = 1;", true, - {{"table1", {"select"}}, {"Product", {"unknown"}}}), + TEST_VALUE("SELECT product.category FROM table1 WHERE Count = 1;", true, + {{"table1", {"select"}}, {"product", {"unknown"}}}), TEST_VALUE("SELECT DISTINCT Usr FROM table1;", true, {{"table1", {"select"}}}), TEST_VALUE("SELECT Usr, Count FROM table1 ORDER BY Count DESC;", true, {{"table1", {"select"}}}), From 1bc23560188cb4e68b6ab8c68df8a2dca51c4da3 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Thu, 4 Jun 2020 19:32:40 +0000 Subject: [PATCH 07/10] Corrected format in BUILD files. Signed-off-by: Christoph Pakulski --- source/extensions/common/sqlutils/BUILD | 4 ++-- test/extensions/common/sqlutils/BUILD | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/extensions/common/sqlutils/BUILD b/source/extensions/common/sqlutils/BUILD index d79de709ee101..c0129c29cfc36 100644 --- a/source/extensions/common/sqlutils/BUILD +++ b/source/extensions/common/sqlutils/BUILD @@ -1,11 +1,11 @@ -licenses(["notice"]) # Apache 2 - load( "//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_package", ) +licenses(["notice"]) # Apache 2 + envoy_package() envoy_cc_library( diff --git a/test/extensions/common/sqlutils/BUILD b/test/extensions/common/sqlutils/BUILD index d5b75130f0e75..0277e47706b2d 100644 --- a/test/extensions/common/sqlutils/BUILD +++ b/test/extensions/common/sqlutils/BUILD @@ -1,11 +1,11 @@ -licenses(["notice"]) # Apache 2 - load( "//bazel:envoy_build_system.bzl", "envoy_cc_test", "envoy_package", ) +licenses(["notice"]) # Apache 2 + envoy_package() envoy_cc_test( From 93145089d0a2de3b3c3aa752464274546c8a0260 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Mon, 8 Jun 2020 14:51:55 +0000 Subject: [PATCH 08/10] Changed test routines to use abseil utilities for string manipulation. Signed-off-by: Christoph Pakulski --- source/extensions/common/sqlutils/sqlutils.h | 8 ++++++++ test/extensions/common/sqlutils/sqlutils_test.cc | 13 +++---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/source/extensions/common/sqlutils/sqlutils.h b/source/extensions/common/sqlutils/sqlutils.h index fde1fa26409c3..8519f21836fa7 100644 --- a/source/extensions/common/sqlutils/sqlutils.h +++ b/source/extensions/common/sqlutils/sqlutils.h @@ -9,6 +9,14 @@ namespace SQLUtils { class SQLUtils { public: + /** + * Method parses SQL query string and writes output to metadata. + * @param query supplies SQL statement. + * @param metadata supplies placeholder where metadata should be written. + * @return True if parsing was successful and False if parsing failed. + * If True was returned the metadata contains result of parsing. The results are + * stored in metadata.mutable_fields. + **/ static bool setMetadata(const std::string& query, ProtobufWkt::Struct& metadata); }; diff --git a/test/extensions/common/sqlutils/sqlutils_test.cc b/test/extensions/common/sqlutils/sqlutils_test.cc index 77319a0532272..a163c683a964c 100644 --- a/test/extensions/common/sqlutils/sqlutils_test.cc +++ b/test/extensions/common/sqlutils/sqlutils_test.cc @@ -30,19 +30,12 @@ TEST_P(MetadataFromSQLTest, ParsingAndMetadataTest) { const std::string& query = std::get<0>(GetParam()); // vector of queries to check. std::vector test_queries; - test_queries.push_back(std::string(query)); + test_queries.push_back(query); // Create uppercase and lowercase versions of the queries and put // them into vector of queries to check - std::string lowercase_query = query; - std::transform(query.begin(), query.end(), lowercase_query.begin(), - [](unsigned char c) { return std::tolower(c); }); - test_queries.push_back(lowercase_query); - - std::string uppercase_query = query; - std::transform(query.begin(), query.end(), uppercase_query.begin(), - [](unsigned char c) { return std::toupper(c); }); - test_queries.push_back(uppercase_query); + test_queries.push_back(absl::AsciiStrToLower(query)); + test_queries.push_back(absl::AsciiStrToUpper(query)); while (!test_queries.empty()) { std::string test_query = test_queries.back(); From ff93e52e0f197194407826a5bc4b6bf59bc615e4 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Wed, 10 Jun 2020 17:47:50 +0000 Subject: [PATCH 09/10] Moved sql-parser library to new revision. Enabled unit tests for DROP DATABASE after fix in sql-parser library. Signed-off-by: Christoph Pakulski --- bazel/repository_locations.bzl | 8 ++++---- test/extensions/common/sqlutils/sqlutils_test.cc | 11 +++-------- .../filters/network/mysql_proxy/mysql_command_test.cc | 4 ++-- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 8c8676f7b90b6..4127baaf1d87f 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -132,10 +132,10 @@ DEPENDENCY_REPOSITORIES = dict( cpe = "N/A", ), com_github_envoyproxy_sqlparser = dict( - sha256 = "b2d3882698cf85b64c87121e208ce0b24d5fe2a00a5d058cf4571f1b25b45403", - strip_prefix = "sql-parser-b14d010afd4313f2372a1cc96aa2327e674cc798", - # 2020-01-10 - urls = ["https://github.com/envoyproxy/sql-parser/archive/b14d010afd4313f2372a1cc96aa2327e674cc798.tar.gz"], + sha256 = "96c10c8e950a141a32034f19b19cdeb1da48fe859cf96ae5e19f894f36c62c71", + strip_prefix = "sql-parser-3b40ba2d106587bdf053a292f7e3bb17e818a57f", + # 2020-06-10 + urls = ["https://github.com/envoyproxy/sql-parser/archive/3b40ba2d106587bdf053a292f7e3bb17e818a57f.tar.gz"], use_category = ["dataplane"], cpe = "N/A", ), diff --git a/test/extensions/common/sqlutils/sqlutils_test.cc b/test/extensions/common/sqlutils/sqlutils_test.cc index a163c683a964c..74ce5c8dfef81 100644 --- a/test/extensions/common/sqlutils/sqlutils_test.cc +++ b/test/extensions/common/sqlutils/sqlutils_test.cc @@ -165,14 +165,9 @@ INSTANTIATE_TEST_SUITE_P( TEST_VALUE("ALTER DATABASE testdb default CHARACTER SET = charset_name;", true, {}), TEST_VALUE("ALTER SCHEMA testdb default CHARACTER SET = charset_name;", true, {}), - // The following DROP DATABASE tests should not produce metadata, - // but the SQL parser returns info that table 'testdb' was accessed. - // This if a bug. Once SQL parser if fixed the following tests should - // be enabled. - // SPELLCHECKER(off) - // TEST_VALUE("DROP DATABASE testdb;", true, {}), - // TEST_VALUE("DROP DATABASE IF EXISTS testdb;", true, {}), - // SPELLCHECKER(on) + // The following DROP DATABASE tests should not produce metadata. + TEST_VALUE("DROP DATABASE testdb;", true, {}), + TEST_VALUE("DROP DATABASE IF EXISTS testdb;", true, {}), // Schema. Should be parsed fine, but should not produce any metadata TEST_VALUE("SHOW databases;", true, {}), TEST_VALUE("SHOW tables;", true, {}), diff --git a/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc b/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc index 1d17b97fa6e3c..85413e0482bed 100644 --- a/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc +++ b/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc @@ -464,7 +464,7 @@ TEST_F(MySQLCommandTest, MySQLTest20) { * The library should return that none of database tables has been accessed, * but it returns that operation drop has been performed on TABLE 'mysqldb'. */ -TEST_F(MySQLCommandTest, DISABLED_MySQLTest21) { +TEST_F(MySQLCommandTest, MySQLTest21) { std::string db = "mysqldb"; std::string command = buildDrop(TestResource::DB, false, db); hsql::SQLParserResult result; @@ -479,7 +479,7 @@ TEST_F(MySQLCommandTest, DISABLED_MySQLTest21) { * The library should return that none of database tables has been accessed, * but it returns that operation drop has been performed on TABLE 'mysqldb'. */ -TEST_F(MySQLCommandTest, DISABLED_MySQLTest22) { +TEST_F(MySQLCommandTest, MySQLTest22) { std::string db = "mysqldb"; std::string command = buildDrop(TestResource::DB, true, db); hsql::SQLParserResult result; From a80b087b8c667e6f9c3128f244d1f5b0f40ddd4c Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Thu, 11 Jun 2020 01:53:47 +0000 Subject: [PATCH 10/10] Removed obsolete comment. Signed-off-by: Christoph Pakulski --- .../filters/network/mysql_proxy/mysql_command_test.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc b/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc index 85413e0482bed..55cacdc2ab532 100644 --- a/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc +++ b/test/extensions/filters/network/mysql_proxy/mysql_command_test.cc @@ -460,9 +460,6 @@ TEST_F(MySQLCommandTest, MySQLTest20) { /* * Test query: "DROP DATABASE " - * Test is disabled because of a bug in SQL parsing library. - * The library should return that none of database tables has been accessed, - * but it returns that operation drop has been performed on TABLE 'mysqldb'. */ TEST_F(MySQLCommandTest, MySQLTest21) { std::string db = "mysqldb"; @@ -475,9 +472,6 @@ TEST_F(MySQLCommandTest, MySQLTest21) { /* * Test query with optional cmd: * "DROP DATABASE IF EXISTS " - * Test is disabled because of a bug in SQL parsing library. - * The library should return that none of database tables has been accessed, - * but it returns that operation drop has been performed on TABLE 'mysqldb'. */ TEST_F(MySQLCommandTest, MySQLTest22) { std::string db = "mysqldb";