mysql_filter: add handling for partial messages by venilnoronha · Pull Request #6885 · envoyproxy/envoy

venilnoronha · 2019-05-10T02:07:40Z

Description: This adds logic to handle partially buffered protocol messages in the MySQL filter.
Risk Level: Med
Testing: Added a new test
Docs Changes: N/A
Release Notes: N/A
Fixes #6696

This adds logic to handle partially buffered protocol messages in the MySQL filter. Signed-off-by: Venil Noronha <veniln@vmware.com>

dkalani · 2019-05-11T02:58:11Z

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

-}
-
-void MySQLFilter::doDecode(Buffer::Instance& buffer) {
  // Safety measure just to make sure that if we have a decoding error we keep going and lose stats.


don't think this comment applies anymore?

I think it'd be valid as long as there's the sniffing_ variable.

stale · 2019-05-18T06:15:39Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

rshriram

Can you add a test for the sniffing code as well?

Signed-off-by: Venil Noronha <veniln@vmware.com>

rshriram · 2019-05-21T23:56:06Z

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

-  if (!sniffing_) {
-    buffer.drain(buffer.length());
-    return;
+  if (sniffing_) {


could you remind me how this sniffing works? the github diff is not showing the entire thing.. I remember this was a boolean that indicated whether we had a decoding error or not and if there was an error, it would simply forward all data as is. But you seem to have flipped the condition.. why?

If there was a decoding error, we used to empty the buffer. That isn't correct. We need to retain the information in the buffer as that's what is being passed to the next filter (and eventually to MySQL).

In this new logic, I'm not modifying the original buffer instance at all. I'm only copying its contents to a new read_buffer_/write_buffer_ instance if the sniffing_ flag is set to true. I've flipped the logic to just reorganize the code.

Now, if there's a decoding error, nothing will be copied over to the read/write buffers; however, the original contents of the message will still be forwarded to the next filter hoping that MySQL will be able to handle it correctly.

rshriram

LGTM

thanks for the fix handling partial messages

venilnoronha · 2019-05-22T00:10:53Z

@mattklein123 this is ready for a final pass.

mattklein123

Quickly skimmed and just caught one thing. Also please go through and make sure all the MySQL code has coverage. We are running up against the limit. Thank you!

/wait

mattklein123 · 2019-05-22T02:19:09Z

source/extensions/filters/network/mysql_proxy/mysql_codec.h


 protected:
-  virtual int parseMessage(Buffer::Instance& data, uint64_t& offset, int len) PURE;
+  virtual int parseMessage(Buffer::Instance& data, int len) PURE;


Not in this change but can you update int to a proper unsigned type? Same above for decode and other places it applies in this PR? Thank you.

Addressed in 9968c6a.

This updates variable types from int to uint32_t for the length field and uint8_t for the sequence ID field in the MySQL proxy filter. Signed-off-by: Venil Noronha <veniln@vmware.com>

mattklein123

Thanks!

* master: test: Add coverage for IsolatedStoreImpl::find* (envoyproxy#7043) server: add ProcessContext (envoyproxy#7018) config: Implement both versions of onConfigUpdate() everywhere (envoyproxy#6879) gzip: add test for various compression strategy and level (envoyproxy#7055) Fix typo in comment for rds.RouteConfiguration.validate_clusters (envoyproxy#7056) mysql_filter: add handling for partial messages (envoyproxy#6885) migrate from v2alpha to v2 (envoyproxy#7044) tests: fix tsan test flake (envoyproxy#7052) upstream: fix HostUtility::healthFlagsToString (envoyproxy#7051) tech debt: eliminate absl::make_unique (envoyproxy#7034) router: add a route name field in route.Route list (envoyproxy#6776) ext_authz: configurable HTTP status code for network errors. (envoyproxy#6669) stats: remove const-cast for symbol-table in edcs_filter_test.cc (envoyproxy#7045) build: bump libevent to 3b1864b. (envoyproxy#7012) stats: improve test-coverage for a few stats-related functions. (envoyproxy#7038) docs: fix csrf filter source origin note (envoyproxy#7041) Fix common typo: grcp -> grpc (envoyproxy#7040) snapshot (envoyproxy#7036) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

mysql_filter: add handling for partial messages

703cf42

This adds logic to handle partially buffered protocol messages in the MySQL filter. Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha requested a review from mattklein123 as a code owner May 10, 2019 02:07

mattklein123 assigned rshriram May 10, 2019

dkalani reviewed May 11, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 18, 2019

rshriram reviewed May 18, 2019

View reviewed changes

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 18, 2019

Add fallback test in the case of partial messages

394b5c5

Signed-off-by: Venil Noronha <veniln@vmware.com>

rshriram reviewed May 21, 2019

View reviewed changes

rshriram previously approved these changes May 22, 2019

View reviewed changes

mattklein123 requested changes May 22, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 22, 2019

Use unsigned types where possible

9968c6a

This updates variable types from int to uint32_t for the length field and uint8_t for the sequence ID field in the MySQL proxy filter. Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha dismissed rshriram’s stale review via 9968c6a May 22, 2019 23:25

repokitteh-read-only bot removed the waiting label May 22, 2019

mattklein123 approved these changes May 23, 2019

View reviewed changes

mattklein123 self-assigned this May 23, 2019

mattklein123 merged commit cfff658 into envoyproxy:master May 23, 2019

Conversation

venilnoronha commented May 10, 2019

Uh oh!

dkalani May 11, 2019

Choose a reason for hiding this comment

Uh oh!

venilnoronha May 11, 2019

Choose a reason for hiding this comment

Uh oh!

stale bot commented May 18, 2019

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

rshriram May 21, 2019

Choose a reason for hiding this comment

Uh oh!

venilnoronha May 22, 2019

Choose a reason for hiding this comment

Uh oh!

venilnoronha May 22, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

venilnoronha commented May 22, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 May 22, 2019

Choose a reason for hiding this comment

Uh oh!

venilnoronha May 22, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants