Mysql proxy filter by rshriram · Pull Request #4975 · envoyproxy/envoy

rshriram · 2018-11-06T19:19:04Z

This filter contains the logic to decode the mysql wire protocol and SQL queries (SQL99 only).
The code is based on our internal version at VMware. The SQL parser can be found at https://github.com/rshriram/sql-parser. Its a cleaned up version of Hyrise SQL parser. I am keeping the code as a separate library as importing the sources into envoy will cause a lot of changes to the code.

Signed-off-by: Giorgio Valentini gvalentini@vmware.com
Signed-off-by: Deepa Kalani dkalani@vmware.com
Signed-off-by: Shriram Rajagopalan shriramr@vmware.com
Signed-off-by: Venil Noronha veniln@vmware.com

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

mattklein123 · 2018-11-06T19:37:00Z

@rshriram what's the high level plan here for this? Can you provide some context on what this will do before we start thinking about reviewing? Note also that we have the older MySQL HC PR from AirBnb which was never finished.

rshriram · 2018-11-06T19:45:43Z

@mattklein123 this filter by itself is useful for decoding the connection phase of the mysql wire protocol (https://dev.mysql.com/doc/dev/mysql-server/latest/PAGE_PROTOCOL.html#protocol_overview) . It will emit stats like the database accessed (not the tables), the user, active sessions, failed login sessions, etc. So, the current PR by itself carries a decent amount of utility.

In order to decode the SQL queries in the command phase (https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_command_phase.html), we need a full fledged SQL parser based on lex/yacc etc. The code we have internally doesn't use such a parser. It extracts only the pieces we want. Thats unlikely to be useful to the rest of the community.

The MySQL health check stuff can be written on top of this code. IIRC, the mysql health check stuff was doing some minimal protocol processing just enough to decode the simple mysql ping queries, while this provides a full fledged parser for the connection phase.

api/envoy/config/filter/network/mysql_proxy/v2/mysql_proxy.proto

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram · 2018-11-09T17:00:29Z

ping.. any comments on this?

dio

Overall this is very interesting. As @mattklein123 comment, let's have this: #3502 to be built based on this?

Just curious, since it has WIP prefix, do you think you want to add more features in this PR?

test/extensions/filters/network/mysql_proxy/mysql_integration_test.cc

rshriram · 2018-11-13T04:28:10Z

@dio that’s my plan. Once this is in, we can build #3502 on top of this.

I put it as wip thinking there will be major changes to the design. Will remove.

dio · 2018-11-13T04:52:17Z

@rshriram cool. Apart from what the clang_tidy has found and that "multiple definitions of ..." when linking MysqlTestUtils in coverage CI (I think either to split the interface and the impl in the different files or put the impl directly in *.h should fix that?), I think this PR is good (tried it manually and it works great).

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram · 2019-01-15T19:59:39Z

@mattklein123 / @lizan
the coverage is missing for getters and setters, or empty virtual functions. I have added more tests for the parts that matter (length encoded integer and the fallback to plain tcp proxy if there is a decoder error)

rshriram · 2019-01-16T20:26:10Z

/retest

repokitteh-read-only · 2019-01-16T20:26:13Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #4975 (comment) was created by @rshriram.

see: more, trace.

dio

Thanks! I'll defer to Lizan and Matt.

lizan · 2019-01-16T22:18:48Z

@rshriram re: empty virtual functions, are they supposed not to be called? If so add NOT_IMPLEMENTED_GCOVR_EXCL_LINE, if not some test should cover them.

Signed-off-by: Giorgio Valentini <gvalentini@vmware.com>

mattklein123

Thanks, this is super cool. I skimmed the code and it seems fine to go in as an experimental filter.

Some doc requests, otherwise will defer to @lizan on any remaining code changes.

/wait

source/extensions/filters/network/mysql_proxy/BUILD

mattklein123 · 2019-01-17T02:24:46Z

docs/root/configuration/network_filters/mysql_proxy_filter.rst

@@ -0,0 +1,46 @@
+.. _config_network_filters_mysql_proxy:
+
+MySQL proxy


Can you add an experimental warning like we do for other new filters? Also, can you add a release note that introduces experimental MySQL support?

Optimally, can you also add a config snippet that shows how to setup the filter? That's typically done now for new filters and it makes it easier on new users.

It would also be super sweet if in a follow up you could add a docker compose example of getting all of this running so that people can use a sample app w/ mysql and actually see this all work.

I took a look at other filters. There is no mention of experimental in their docs. The proto stuff however has experimental tag. is that what you meant?

I added proto status experimental to the protos. And added config snippets along with some RBAC examples as well.

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram · 2019-01-17T17:04:22Z

@lizan / @mattklein123 I have added the docs with config samples, version history, and the experimental warning. The docker compose example would have to wait until we have access logging for MySQL.

mattklein123

LGTM with small version history at nit. Will defer to @lizan for final review and merge. Thank you for working on this! Super cool.

mattklein123 · 2019-01-17T17:55:46Z

docs/root/intro/version_history.rst

 * tls: enabled TLS 1.3 on the server-side (non-FIPS builds).
 * router: added per-route configuration of :ref:`internal redirects <envoy_api_field_route.RouteAction.internal_redirect_action>`.
 * upstream: add hash_function to specify the hash function for :ref:`ring hash<envoy_api_msg_Cluster.RingHashLbConfig>` as either xxHash or `murmurHash2 <https://sites.google.com/site/murmurhash>`_. MurmurHash2 is compatible with std::hash in GNU libstdc++ 3.4.20 or above. This is typically the case when compiled on Linux and not macOS.
+* network filters: added a MySQL proxy filter that is capable of parsing SQL queries over MySQL wire protocol. Refer to ::ref:`MySQL proxy<config_network_filters_mysql_proxy>` for more details.


nit: alpha order and also I would call this "mysql:"

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

doodlesbykumbi · 2019-02-06T19:46:14Z

source/extensions/filters/network/mysql_proxy/mysql_filter.cc

+  // Safety measure just to make sure that if we have a decoding error we keep going and lose stats.
+  // This can be removed once we are more confident of this code.
+  if (!sniffing_) {
+    buffer.drain(buffer.length());


Hi @rshriram. Why does the buffer need draining here ?
I tried out the filter with SSL and it hangs after draining for both onData and onWrite.
Taking out this line altogether results in a build that works as intended.

I was merely following the logic in mongo filter, that did the same.

void ProxyFilter::doDecode(Buffer::Instance& buffer) { if (!sniffing_ || !runtime_.snapshot().featureEnabled(MongoRuntimeConfig::get().ProxyEnabled, 100)) { // Safety measure just to make sure that if we have a decoding error we keep going and lose // stats. This can be removed once we are more confident of this code. buffer.drain(buffer.length()); return; }

This filter contains the logic to decode the mysql wire protocol and SQL queries (SQL99 only). The code is based on our internal version at VMware. The SQL parser can be found at https://github.com/rshriram/sql-parser. Its a cleaned up version of Hyrise SQL parser. I am keeping the code as a separate library as importing the sources into envoy will cause a lot of changes to the code. Signed-off-by: Giorgio Valentini <gvalentini@vmware.com> Signed-off-by: Deepa Kalani <dkalani@vmware.com> Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> Signed-off-by: Venil Noronha <veniln@vmware.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Shriram Rajagopalan added 5 commits November 6, 2018 12:33

mysql filter: initial dump

28e739f

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

format

6ea01b3

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

assorted fixes

235f1e3

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

formatting

b5a116c

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

typo

7626f3c

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram commented Nov 6, 2018

View reviewed changes

api/envoy/config/filter/network/mysql_proxy/v2/mysql_proxy.proto Outdated Show resolved Hide resolved

lizan requested a review from dio November 6, 2018 23:26

lizan assigned dio and mattklein123 Nov 6, 2018

Shriram Rajagopalan added 2 commits November 8, 2018 15:36

docs

d4a799a

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into mysql

99f080c

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

dio reviewed Nov 12, 2018

View reviewed changes

dio reviewed Nov 13, 2018

View reviewed changes

test/extensions/filters/network/mysql_proxy/mysql_integration_test.cc Outdated Show resolved Hide resolved

rshriram changed the title ~~WIP: Mysql proxy~~ Mysql proxy filter Nov 13, 2018

split mysql test utils

6c8bbb8

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram force-pushed the mysql branch from 318e382 to a1e03b3 Compare November 13, 2018 21:28

Shriram Rajagopalan added 9 commits November 13, 2018 21:29

doc nit

8fd66f1

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

build fixes

07fc5bf

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

buildifier

a4b52ff

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

version_

a39360b

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

fixes

cefe2af

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

docs fix

1aefa45

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

coverage fixes

60fb18a

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

undo some clang tidy

5d86191

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

format

6d55269

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

mattklein123 added the waiting label Jan 15, 2019

Shriram Rajagopalan added 2 commits January 15, 2019 18:45

cleanups

7e7a449

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

Merge remote-tracking branch 'upstream/master' into mysql

45c0abe

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

repokitteh-read-only bot removed the waiting label Jan 15, 2019

Shriram Rajagopalan added 2 commits January 15, 2019 19:51

undo incorrect offset calc. unit tests

09e15b9

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

test for fallback

fea2760

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

dio previously approved these changes Jan 16, 2019

View reviewed changes

define virtual functions as PURE

77c25c9

Signed-off-by: Giorgio Valentini <gvalentini@vmware.com>

gvalentinivmw dismissed dio’s stale review via 77c25c9 January 17, 2019 02:10

mattklein123 requested changes Jan 17, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 17, 2019

Shriram Rajagopalan added 2 commits January 17, 2019 06:47

doc nits

4cfbbc8

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

Merge remote-tracking branch 'upstream/master' into mysql

3b472b7

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

repokitteh-read-only bot removed the waiting label Jan 17, 2019

Shriram Rajagopalan added 2 commits January 17, 2019 17:02

version history and experimental warning

6e18ded

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

Merge remote-tracking branch 'upstream/master' into mysql

16eb30d

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

mattklein123 previously approved these changes Jan 17, 2019

View reviewed changes

alphabetical order

8070095

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram dismissed mattklein123’s stale review via 8070095 January 17, 2019 18:20

lizan approved these changes Jan 17, 2019

View reviewed changes

lizan merged commit b3be571 into envoyproxy:master Jan 17, 2019

doodlesbykumbi reviewed Feb 6, 2019

View reviewed changes

austin-zhu mentioned this pull request Oct 13, 2020

Add support of MYSQL health check #13551

Open

qinggniq mentioned this pull request Jan 14, 2021

Add More Features to MySQL Proxy #14697

Closed

		@@ -0,0 +1,46 @@
		.. _config_network_filters_mysql_proxy:

		MySQL proxy

Conversation

rshriram commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Nov 6, 2018

Uh oh!

rshriram commented Nov 6, 2018

Uh oh!

Uh oh!

rshriram commented Nov 9, 2018

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rshriram commented Nov 13, 2018

Uh oh!

dio commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rshriram commented Jan 15, 2019

Uh oh!

rshriram commented Jan 16, 2019

Uh oh!

repokitteh-read-only bot commented Jan 16, 2019

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

lizan commented Jan 16, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram commented Jan 17, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

doodlesbykumbi Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rshriram commented Nov 6, 2018 •

edited

Loading

dio commented Nov 13, 2018 •

edited

Loading