Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to go-re2 #1005

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Switch to go-re2 #1005

wants to merge 4 commits into from

Conversation

monkburger
Copy link

@monkburger monkburger commented Feb 15, 2024

When running Coraza within Caddy, profiling was showing elevated regexp.* usage. After discussions on Slack, it was demonstrated that the Go-Re2 offers much lower CPU / slightly better performance with Go-Re2 compared to the standard regexp. (There are legions of discussions about Go's regexp engine being slow on the web)

Since this is a drop-in replacement, existing testing doesn't require adjustment as far as I can see. (if this is an issue I can send another PR)

Make sure that you've checked the boxes below before you submit PR:

Thanks for your contribution ❤️

Go's Regexp seems to be very costly for Coraza (and perhaps others).

Profiling Coraza/Caddy (w/OWASP4), with the 'wrk' benchmark tool shows
the following:

  31.68%  caddy    caddy              [.] regexp.(*machine).add
  16.80%  caddy    caddy              [.] regexp.(*machine).step
  16.55%  caddy    caddy              [.] regexp.(*machine).match
   4.47%  caddy    caddy              [.] regexp.(*inputString).step
   3.45%  caddy    caddy              [.] regexp.(*machine).alloc
   3.13%  caddy    caddy              [.] regexp.lazyFlag.match

After switching over to Re2, those disappear.
@monkburger monkburger requested a review from a team as a code owner February 15, 2024 23:52
@anuraaga
Copy link
Contributor

We provide a plugin to make it easy to swap in go-re2

https://github.com/corazawaf/coraza-wasilibs

go-re2 is a pretty heavyweight dependency so it's probably better to still keep it opt-in rather than only supporting it. I think for caddy we just want to provide a build tag that uses coraza-wasilibs

@jcchavezs
Copy link
Member

I don't think we should migrate right away but defintively we need a section on how to use wasilibs. Mind working that out @monkburger? see https://github.com/corazawaf/coraza-wasilibs

@jptosso
Copy link
Member

jptosso commented Feb 19, 2024

Maybe for regex engines, we should have build flags... I was also working on hyperscan integration

@fzipi
Copy link
Member

fzipi commented Apr 27, 2024

@monkburger Are you up for working on #1005 (comment)?

@monkburger
Copy link
Author

@monkburger Are you up for working on #1005 (comment)?

Sorry. I haven't had any free time in the last few months to look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants