ZooKeeper proxy filter by rgs1 · Pull Request #5991 · envoyproxy/envoy

rgs1 · 2019-02-18T00:34:54Z

This filter decodes the ZooKeeper wire protocol and emits
stats & metadata about requests. Responses and events
will be supported in a follow-up PR.

This wire protocol parsing is based on:

https://github.com/twitter/zktraffic
https://github.com/rgs1/zktraffic-cpp

The actual filter structure is based on the Mysql proxy filter.

Signed-off-by: Raul Gutierrez Segales [email protected]

This filter decodes the ZooKeeper wire protocol and emits stats & metadata about requests, responses and events. This wire protocol parsing is based on: https://github.com/twitter/zktraffic https://github.com/rgs1/zktraffic-cpp The actual filter structure is based on the Mysql proxy filter. Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 · 2019-02-18T00:35:15Z

Note: this is not finished yet, but early comments are welcomed :-)

Signed-off-by: Raul Gutierrez Segales <[email protected]>

source/extensions/filters/network/zookeeper_proxy/zookeeper_filter.cc

snowp

Left you a few comments to get you started.

Has the maintainer sponsorship story for this extension been figured out as per https://docs.google.com/document/d/1eDQQSxqx2khTXfa2vVm4vqkyRwXYkPzZCcbjxJ2_AvA/edit ?

source/extensions/filters/network/zookeeper_proxy/zookeeper_config.cc

source/extensions/filters/network/zookeeper_proxy/zookeeper_decoder.cc

source/extensions/filters/network/zookeeper_proxy/zookeeper_decoder.h

source/extensions/filters/network/zookeeper_proxy/zookeeper_utils.cc

test/extensions/filters/network/zookeeper_proxy/zookeeper_filter_test.cc

rgs1 · 2019-02-18T20:14:39Z

@snowp thanks for the comments! I'll address them right away.

Whereas wrt to a senior maintaner sponsoring this, I was thinking maybe @mattklein123 or @zuercher :-)

Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 · 2019-02-19T20:08:04Z

Left you a few comments to get you started.

Has the maintainer sponsorship story for this extension been figured out as per https://docs.google.com/document/d/1eDQQSxqx2khTXfa2vVm4vqkyRwXYkPzZCcbjxJ2_AvA/edit ?

Oops, my bad — I am somehow misread that I needed a senior maintainer...

@snowp if you have some cycles for sponsoring, that would be great :-)

Signed-off-by: Raul Gutierrez Segales <[email protected]>

docs/root/configuration/network_filters/zookeeper_proxy_filter.rst

source/extensions/filters/network/zookeeper_proxy/zookeeper_decoder.cc

source/extensions/filters/network/zookeeper_proxy/zookeeper_decoder.h

test/extensions/filters/network/zookeeper_proxy/zookeeper_filter_test.cc

source/extensions/filters/network/zookeeper_proxy/zookeeper_filter.cc

source/extensions/filters/network/zookeeper_proxy/zookeeper_config.cc

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Also, rename the Opcodes enum for consistency. Signed-off-by: Raul Gutierrez Segales <[email protected]>

Signed-off-by: Raul Gutierrez Segales <[email protected]>

snowp

Still missing some coverage on the create flags, otherwise I think this looks good

rgs1 · 2019-03-20T00:25:24Z

Still missing some coverage on the create flags, otherwise I think this looks good

added.

Signed-off-by: Raul Gutierrez Segales <[email protected]>

This is valid in the client protocol, so lets handle that. Also, this improves coverage for peekString(), since it tests the branch for zero-length strings. Signed-off-by: Raul Gutierrez Segales <[email protected]>

Signed-off-by: Raul Gutierrez Segales <[email protected]>

* the try/catch block was redundant (happens in the decoder already) * move clear dynamic metadata into its own method Signed-off-by: Raul Gutierrez Segales <[email protected]>

Signed-off-by: Raul Gutierrez Segales <[email protected]>

snowp

Just some minor comments, otherwise LGTM

source/extensions/filters/network/zookeeper_proxy/zookeeper_decoder.h

source/extensions/filters/network/zookeeper_proxy/zookeeper_filter.cc

source/extensions/filters/network/zookeeper_proxy/zookeeper_utils.cc

Signed-off-by: Raul Gutierrez Segales <[email protected]>

snowp

Ok this looks good to me as a starting point to iterate on in further PRs.

Any @envoyproxy/senior-maintainers able to take a look at this?

source/common/common/enum_to_int.h

alyssawilk

"LGTM" for source/common/common/enum_to_int.h pending the one nit :-)

Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 · 2019-03-25T15:42:18Z

/retest

repokitteh-read-only · 2019-03-25T15:42:24Z

🙀 failed invoking rebuild of ci/circleci: mac: 500 Internal Server Error

🐱

Caused by: a #5991 (comment) was created by @rgs1.

see: more, trace.

alxn · 2020-09-01T20:54:17Z

@rgs1 I have a question about this work. The two references are pcap interceptors, this change is an L7 proxy for zookeeper traffic?

rgs1 · 2020-09-01T20:57:15Z

@rgs1 I have a question about this work. The two references are pcap interceptors, this change is an L7 proxy for zookeeper traffic?

Correct -- it's L7 but it's also passive. It understands the ZK protocol, but just to emit stats and generate logging. It won't create its own clients or keep sessions on behalf of clients.

alxn · 2020-09-01T21:38:51Z

@rgs1 but in order to use this filter, you now have:

client <-first-tcp-session-> envoy <-second-tcp-session-> Zookeeper

rgs1 · 2020-09-01T21:45:54Z

@rgs1 but in order to use this filter, you now have:

client <-first-tcp-session-> envoy <-second-tcp-session-> Zookeeper

True. That would be true as well if you were using a network load balancer of any sort (e.g.: an NLB in Amazon).

Raul Gutierrez Segales added 3 commits February 17, 2019 20:53

Fix format issues

d2f50eb

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Fix docs

5a3bfe1

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Add missing words to dictionary.

e16f507

Signed-off-by: Raul Gutierrez Segales <[email protected]>

rshriram reviewed Feb 18, 2019

View reviewed changes

source/extensions/filters/network/zookeeper_proxy/zookeeper_filter.cc Outdated Show resolved Hide resolved

snowp suggested changes Feb 18, 2019

View reviewed changes

Move var declaration closer to usage.

ea2c5d6

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Tentatively adding @snowp as sponsor/co-maintainer :-)

71f7686

Signed-off-by: Raul Gutierrez Segales <[email protected]>

mattklein123 assigned snowp Feb 19, 2019

Raul Gutierrez Segales added 5 commits February 19, 2019 21:30

Make DecoderImpl's constructor explicit

ad8f315

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Use peekInt<>() to extract bool

cfa9f3f

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Use copyOut() instead of linearize

2698f55

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Simplify test helpers

a17402a

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Drop my copy of enumToInt()

f5a2877

Signed-off-by: Raul Gutierrez Segales <[email protected]>

snowp suggested changes Feb 20, 2019

View reviewed changes

Raul Gutierrez Segales added 12 commits February 22, 2019 12:19

Merge remote-tracking branch 'upstream/master' into add-zookeeper-filter

8482d4c

Fix order of version history items

6746876

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Consistently name stats that are requests

7911d6c

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Fix doc

dce2ca7

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Use enum class for XID codes

315e656

Also, rename the Opcodes enum for consistency. Signed-off-by: Raul Gutierrez Segales <[email protected]>

Expand enumToInt() to support signed ints.

0e2c698

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Add enumToIntSigned()

d839235

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Move Buffer::OwnedImpl instead of returning pointers

98e6896

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Emit decoderError() calls when len checks fail

c371cfb

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Parse delete & exists requests

73962e4

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Parse getacl & setacl requests

e19d918

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Parse sync requests

30546ce

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Raul Gutierrez Segales added 2 commits March 19, 2019 09:41

Encapsulate skip within the BufferHelper

e43cd1e

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Explain how offset and cursor_ are different.

fe401f4

Signed-off-by: Raul Gutierrez Segales <[email protected]>

snowp suggested changes Mar 19, 2019

View reviewed changes

Raul Gutierrez Segales added 10 commits March 19, 2019 17:25

Test all possible create flags

6e0f226

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Improve coverage fo ZooKeeperFilter::onCreateRequest

a80279d

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Add test for empty string as path

67227d7

This is valid in the client protocol, so lets handle that. Also, this improves coverage for peekString(), since it tests the branch for zero-length strings. Signed-off-by: Raul Gutierrez Segales <[email protected]>

Fix comment

2533893

Signed-off-by: Raul Gutierrez Segales <[email protected]>

DRY things up a bit

f18cbfe

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Merge remote-tracking branch 'upstream/master' into add-zookeeper-filter

58ce244

Tidy up ZooKeeperFilter::doDecode

0a1e14f

* the try/catch block was redundant (happens in the decoder already) * move clear dynamic metadata into its own method Signed-off-by: Raul Gutierrez Segales <[email protected]>

Add missing coverage for BufferHelper::peekString

d689794

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Fix

bda8df1

Signed-off-by: Raul Gutierrez Segales <[email protected]>

Merge remote-tracking branch 'upstream/master' into add-zookeeper-filter

cb117f3

snowp suggested changes Mar 22, 2019

View reviewed changes

More constness

3f3f466

Signed-off-by: Raul Gutierrez Segales <[email protected]>

snowp previously approved these changes Mar 23, 2019

View reviewed changes

alyssawilk reviewed Mar 25, 2019

View reviewed changes

source/common/common/enum_to_int.h Outdated Show resolved Hide resolved

alyssawilk previously approved these changes Mar 25, 2019

View reviewed changes

rgs1 dismissed stale reviews from alyssawilk and snowp via 3ec7cb4 March 25, 2019 15:28

Rename enumToIntSigned -> enumToSignedInt

3ec7cb4

Signed-off-by: Raul Gutierrez Segales <[email protected]>

snowp approved these changes Mar 25, 2019

View reviewed changes

snowp merged commit b771f99 into envoyproxy:master Mar 25, 2019

Conversation

rgs1 commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgs1 commented Feb 18, 2019

Uh oh!

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgs1 commented Feb 18, 2019

Uh oh!

rgs1 commented Feb 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

rgs1 commented Mar 20, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

rgs1 commented Mar 25, 2019

Uh oh!

repokitteh-read-only bot commented Mar 25, 2019

Uh oh!

alxn commented Sep 1, 2020

Uh oh!

rgs1 commented Sep 1, 2020

Uh oh!

alxn commented Sep 1, 2020

Uh oh!

rgs1 commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rgs1 commented Feb 18, 2019 •

edited

Loading

rgs1 commented Sep 1, 2020 •

edited

Loading