contrib: add Hyperscan regex engine #21973

zhxie · 2022-07-01T02:36:06Z

Signed-off-by: Xie Zhihao [email protected]

Commit Message: contrib: add Hyperscan regex engine
Additional Description: Hyperscan has been introduced as an input matcher earlier this year. Since the regex engine interface has been completed, the patch extent the usage of Hyperscan into a contrib regex engine.
Risk Level: Low
Testing: Unit
Docs Changes: API
Release Notes: N/A
Platform Specific Features: Requires processor with SSSE3 support (nearly any modern x86 processor)

Signed-off-by: Xie Zhihao <[email protected]>

repokitteh-read-only · 2022-07-01T02:36:14Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #21973 was opened by zhxie.

see: more, trace.

Signed-off-by: Xie Zhihao <[email protected]>

wrowe · 2022-07-06T15:08:06Z

/assign-from envoyproxy/envoy-maintainers

repokitteh-read-only · 2022-07-06T15:08:11Z

envoyproxy/envoy-maintainers assignee is @jmarantz

🐱

Caused by: a #21973 (comment) was created by @wrowe.

see: more, trace.

wrowe · 2022-07-06T15:08:40Z

Ping @moderation for deps review?

moderation · 2022-07-06T15:29:48Z

Metadata only change for the dependency

/lgtm deps

phlax

one small nit but otherwise lgtm from docs pov

docs/root/api-v3/config/contrib/hyperscan/regex_engine.rst

Signed-off-by: Xie Zhihao <[email protected]>

jmarantz

Flushing some comments. Lots to dig into here.

api/contrib/envoy/extensions/matching/input_matchers/hyperscan/v3alpha/hyperscan.proto

api/contrib/envoy/extensions/regex_engines/hyperscan/v3alpha/hyperscan.proto

contrib/hyperscan/matching/input_matchers/source/config.cc

contrib/hyperscan/matching/input_matchers/source/matcher.cc

contrib/hyperscan/matching/input_matchers/source/config.cc

contrib/hyperscan/matching/input_matchers/source/matcher.cc

contrib/hyperscan/matching/input_matchers/source/matcher.h

Signed-off-by: Xie Zhihao <[email protected]>

zhxie · 2022-07-29T02:10:48Z

Replied offline about end users consideration.

Scalable path evaluation is not conflict with regex engine update. In fact, a regex engine can be used everywhere in Envoy, especially in security region (including WAF-like extensions like RBAC).

zhxie · 2022-07-29T07:00:16Z

/retest flaky

repokitteh-read-only · 2022-07-29T07:00:20Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21973 (comment) was created by @zhxie.

see: more, trace.

jmarantz

looks good -- just a few more small questions.

jmarantz · 2022-07-29T12:49:18Z

api/contrib/envoy/extensions/matching/input_matchers/hyperscan/v3alpha/hyperscan.proto

-// hybrid automata techniques to allow simultaneous matching of large numbers of regular
-// expressions across streams of data.
-//
-// The matcher follows PCRE pattern syntax, and the regex string must adhere to the documented


why did we drop this paragraph?

The paragraph has been moved to api/contrib/envoy/extensions/regex_engines/hyperscan/v3alpha/hyperscan.proto since it is the introduction of the engine itself.

jmarantz · 2022-07-29T12:52:54Z

contrib/hyperscan/matching/input_matchers/source/config.cc

+      if (regex.allow_empty()) {
+        flag |= HS_FLAG_ALLOWEMPTY;
+      }
+      if (regex.utf8()) {


I'm not 100% sure I understand -- are you saying that you'll flip the defaults in the .proto file file to try to match re2 behavior?

jmarantz · 2022-07-29T13:07:26Z

contrib/hyperscan/matching/input_matchers/source/matcher.cc

+  if (som_database) {
+    err = hs_alloc_scratch(som_database, &scratch_);
+    if (err != HS_SUCCESS) {
+      throw EnvoyException(


Yeah I think what I really want to express here is that the line of code can't be hit.

There's certainly a concern with lines of code that can't be tested, and whether anyone would catch this assert successfully and what they would do about that if there was no memory left.

We used to have a macro to indicate specifically that a line of code can't be hit, and previously the toolchain running the coverage analysis would not count those lines, but that stopped working at some point when the toolchain evolved.

WDYT of just using RELEASE_ASSERT here rather than throwing an exception?

jmarantz · 2022-07-29T20:24:20Z

I think @zhxie did a great job with the code in this PR -- thanks for all the iteration.

I still have some higher level concerns

what are the exact steps someone would take to rebuild Envoy with this feature
what changes would a user need to existing re2 regexes?
wouldn't the user (eg in High CPU usage for matching routes (re2::DFA::InlinedSearchLoop) #20147) be better off using https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/common/matcher/v3/matcher.proto ?

I am worried about propagating an alternate regex syntax (even as contrib/...) and bifurcating the community. I also wish Hyperscan didn't bring in Boost.

I thought also about whether some of the Hyperscan techniques should just be ported to re2 but that seems unlikely. There was some enlightening discussion on google/re2#383 . Notably -- there are microbenchmarks showing Hyperscan's raw speed but no obvious benchmarks exposed in the context of Envoy operations.

@mattklein123

In light of the above I'd like to get senior-maintainer opinion on this.

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2022-07-29T20:24:24Z

@envoyproxy/senior-maintainers assignee is @alyssawilk

🐱

Caused by: a #21973 (comment) was created by @jmarantz.

see: more, trace.

Signed-off-by: Xie Zhihao <[email protected]>

alyssawilk · 2022-08-01T13:38:48Z

I had largely thought that with contrib we didn't have to monitor things like dependency bloat from boost or extra regex engines. I'm going to throw senior maintianer pass over to Matt as he's back tomorrow and he was driving the recent changes to contrib.

mattklein123 · 2022-08-02T15:52:20Z

In light of the above I'd like to get senior-maintainer opinion on this.

I'm OK with this one coming into contrib as I've heard of multiple requests for using HyperScan to speed up matching (for example in WAF scenarios). IMO it's OK to let this into contrib and see how it gets used? Happy to defer to other opinions if they are strong however.

alyssawilk · 2022-08-02T16:20:51Z

SGTM. @moderation can we get one more deps LGTM so we can merge this in?

jmarantz · 2022-08-02T16:25:08Z

OK with me then. @zhxie were you going to change defaults in this PR to make it as easy as possible for a configuration that works with re2 to also work with hyperscan?

zhxie · 2022-08-03T01:28:27Z

@jmarantz of course. As I have mentioned, I have removed all options in Hyperscan regex engine proto to align the behavior with RE2.

envoy/api/contrib/envoy/extensions/regex_engines/hyperscan/v3alpha/hyperscan.proto

Lines 24 to 25 in e09647f

    
           message Hyperscan { 
        
           }

jmarantz · 2022-08-03T01:41:33Z

Sorry still not totally getting it.

I was thinking when reviewing code earlier that some of the out-of-the-box options for Hyperscan around embedded newlines and unicode were different than what I think RE2 behavior is, and that we'd need explicit defaults to make them as compatible as practical.

jmarantz

Per slack discussion the defaults are set to maximize compatibility with RE2. Thanks for pushing this all the way through!

moderation · 2022-08-03T15:32:37Z

/lgtm deps

This is a metadata only dependency change. The initial Hyperscan contrib extension that brings in unwanted dependencies like Boost was added prior to the current contrib extension policy - https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md#contrib-extensions. The Hyperscan input matcher and regex engine would likely no longer be approved without an end user sponsor. These extensions appear to be R&D activities for a vendor without end users.

sambercovici · 2023-01-19T13:31:35Z

I see that hyperscan is compiled without "FAT_RUNTIME": "on"
Was there any reason fore this?

zhxie added 12 commits June 30, 2022 15:50

contrib: add Hyperscan engine

ed3a08a

Signed-off-by: Xie Zhihao <[email protected]>

contrib: support replaceAll in Hyperscan matcher

6d1be2f

Signed-off-by: Xie Zhihao <[email protected]>

contrib: better performance on replaceAll

076746e

Signed-off-by: Xie Zhihao <[email protected]>

contrib: update due to interface change

4822317

Signed-off-by: Xie Zhihao <[email protected]>

contrib: minor fix

4de45ef

Signed-off-by: Xie Zhihao <[email protected]>

contrib: fix format

d8d2660

Signed-off-by: Xie Zhihao <[email protected]>

contrib: compile SOM database on demand

907790d

Signed-off-by: Xie Zhihao <[email protected]>

contrib: use lambda expression instead of compare function object

3c73421

Signed-off-by: Xie Zhihao <[email protected]>

api: forbid allow_empty in regex engine

231cfb8

Signed-off-by: Xie Zhihao <[email protected]>

misc: adjust sequence

b4bd4d5

Signed-off-by: Xie Zhihao <[email protected]>

docs: add Hyperscan regex engine

f0a2d2b

Signed-off-by: Xie Zhihao <[email protected]>

contrib: decouple

005b0aa

Signed-off-by: Xie Zhihao <[email protected]>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jul 1, 2022

repokitteh-read-only bot assigned moderation Jul 1, 2022

zhxie added 3 commits July 1, 2022 11:20

Merge remote-tracking branch 'envoyproxy/main' into hscan-engine

fd702b7

api: remove unused import

f0ec4e2

Signed-off-by: Xie Zhihao <[email protected]>

test: reconstruct

3c8629b

Signed-off-by: Xie Zhihao <[email protected]>

repokitteh-read-only bot assigned jmarantz Jul 6, 2022

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jul 6, 2022

phlax reviewed Jul 6, 2022

View reviewed changes

docs/root/api-v3/config/contrib/hyperscan/regex_engine.rst Show resolved Hide resolved

docs: add document title

b0d0cbf

Signed-off-by: Xie Zhihao <[email protected]>

jmarantz reviewed Jul 14, 2022

View reviewed changes

zhxie added 2 commits July 15, 2022 13:50

api: use user-friendly names

8bb0f56

Signed-off-by: Xie Zhihao <[email protected]>

nit: add comments

ebd68d8

Signed-off-by: Xie Zhihao <[email protected]>

zhxie added 2 commits July 29, 2022 10:00

nit: change exception

be3f2be

Signed-off-by: Xie Zhihao <[email protected]>

nit: update comments

369f51e

Signed-off-by: Xie Zhihao <[email protected]>

repokitteh-read-only bot removed the waiting label Jul 29, 2022

jmarantz reviewed Jul 29, 2022

View reviewed changes

repokitteh-read-only bot assigned alyssawilk Jul 29, 2022

contrib: use IS_ENVOY_BUG instead

e09647f

Signed-off-by: Xie Zhihao <[email protected]>

alyssawilk assigned mattklein123 and unassigned alyssawilk Aug 1, 2022

jmarantz approved these changes Aug 3, 2022

View reviewed changes

mattklein123 removed their assignment Aug 3, 2022

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Aug 3, 2022

jmarantz merged commit 725d0b4 into envoyproxy:main Aug 3, 2022

zhxie deleted the hscan-engine branch September 7, 2022 08:31

zhxie mentioned this pull request Mar 7, 2023

Enable Hyperscan regex engine extension istio/proxy#4495

Closed

zhxie mentioned this pull request Mar 29, 2023

Enable Hyperscan input matcher and regex engine extension istio/proxy#4545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib: add Hyperscan regex engine #21973

contrib: add Hyperscan regex engine #21973

zhxie commented Jul 1, 2022

repokitteh-read-only bot commented Jul 1, 2022

wrowe commented Jul 6, 2022

repokitteh-read-only bot commented Jul 6, 2022

wrowe commented Jul 6, 2022

moderation commented Jul 6, 2022

phlax left a comment

jmarantz left a comment

zhxie commented Jul 29, 2022

zhxie commented Jul 29, 2022

repokitteh-read-only bot commented Jul 29, 2022

jmarantz left a comment

jmarantz Jul 29, 2022

zhxie Aug 1, 2022

jmarantz Jul 29, 2022

jmarantz Jul 29, 2022

jmarantz commented Jul 29, 2022

repokitteh-read-only bot commented Jul 29, 2022

alyssawilk commented Aug 1, 2022

mattklein123 commented Aug 2, 2022

alyssawilk commented Aug 2, 2022

jmarantz commented Aug 2, 2022

zhxie commented Aug 3, 2022

jmarantz commented Aug 3, 2022

jmarantz left a comment

moderation commented Aug 3, 2022

sambercovici commented Jan 19, 2023

contrib: add Hyperscan regex engine #21973

contrib: add Hyperscan regex engine #21973

Conversation

zhxie commented Jul 1, 2022

repokitteh-read-only bot commented Jul 1, 2022

wrowe commented Jul 6, 2022

repokitteh-read-only bot commented Jul 6, 2022

wrowe commented Jul 6, 2022

moderation commented Jul 6, 2022

phlax left a comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

zhxie commented Jul 29, 2022

zhxie commented Jul 29, 2022

repokitteh-read-only bot commented Jul 29, 2022

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz Jul 29, 2022

Choose a reason for hiding this comment

zhxie Aug 1, 2022

Choose a reason for hiding this comment

jmarantz Jul 29, 2022

Choose a reason for hiding this comment

jmarantz Jul 29, 2022

Choose a reason for hiding this comment

jmarantz commented Jul 29, 2022

repokitteh-read-only bot commented Jul 29, 2022

alyssawilk commented Aug 1, 2022

mattklein123 commented Aug 2, 2022

alyssawilk commented Aug 2, 2022

jmarantz commented Aug 2, 2022

zhxie commented Aug 3, 2022

jmarantz commented Aug 3, 2022

jmarantz left a comment

Choose a reason for hiding this comment

moderation commented Aug 3, 2022

sambercovici commented Jan 19, 2023