fix: cache compiled regexps in CORS wildcard origin matching - #4563
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesWildcard Regex Cache and Test Coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
87ddba5 to
1964953
Compare
1964953 to
21e7839
Compare
|
@matiasinsaurralde ❤️ for a kickass PR - could you please rebase it with dev? |
e48fafa to
c378dae
Compare
Just rebased, should be good now |
c378dae to
372f0b0
Compare
matchesWildcardPattern was calling regexp.Compile on every HTTP request for each wildcard pattern in AllowedOrigins. Add a sync.Map cache keyed by the raw pattern string so each regexp is compiled once and reused. Benchmarks show ~36x speedup sequential, ~350x under concurrency, with zero allocations on the hot path. Also adds unit tests for matchesWildcardPattern and IsOriginAllowed. Signed-off-by: Matías Insaurralde <matias@insaurral.de>
Use the actual stored value from LoadOrStore rather than the locally compiled regexp, making the concurrent-safety intent explicit without requiring readers to reason about functional equivalence of duplicate compiles. Signed-off-by: Matías Insaurralde <matias@insaurral.de>
Rename "scheme-less match" (want: false) to "scheme-less no match with scheme prefix" so the test name reflects the expected outcome. Signed-off-by: Matías Insaurralde <matias@insaurral.de>
372f0b0 to
ff68c93
Compare
* fix: cache compiled regexps in CORS wildcard origin matching matchesWildcardPattern was calling regexp.Compile on every HTTP request for each wildcard pattern in AllowedOrigins. Add a sync.Map cache keyed by the raw pattern string so each regexp is compiled once and reused. Benchmarks show ~36x speedup sequential, ~350x under concurrency, with zero allocations on the hot path. Also adds unit tests for matchesWildcardPattern and IsOriginAllowed. Signed-off-by: Matías Insaurralde <matias@insaurral.de> * fix: use LoadOrStore return value in wildcard regexp cache Use the actual stored value from LoadOrStore rather than the locally compiled regexp, making the concurrent-safety intent explicit without requiring readers to reason about functional equivalence of duplicate compiles. Signed-off-by: Matías Insaurralde <matias@insaurral.de> * fix: rename misleading test case for scheme-less wildcard pattern Rename "scheme-less match" (want: false) to "scheme-less no match with scheme prefix" so the test name reflects the expected outcome. Signed-off-by: Matías Insaurralde <matias@insaurral.de> --------- Signed-off-by: Matías Insaurralde <matias@insaurral.de> Co-authored-by: Akshay Deo <akshay@akshaydeo.com>
matchesWildcardPatternwas callingregexp.Compileon every HTTP request for each wildcard pattern inAllowedOrigins. Add async.Mapcache keyed by the raw pattern string so each regexp is compiled once and reused.Benchmarks show ~36x speedup sequential, ~350x under concurrency, with zero allocations on the hot path.
Also adds unit tests for
matchesWildcardPatternandIsOriginAllowed.Summary
matchesWildcardPatterncompiled a new*regexp.Regexpon every call — which happens on every HTTP request through the CORS middleware, plus every WebSocket upgrade. Since the wildcard patterns come from static config (AllowedOrigins), the same patterns were recompiled thousands of times per second for no reason.This adds a
sync.Mapcache so each pattern is compiled once on first encounter and reused thereafter. This follows the same pattern already used bycelMapKeyRegexCacheinplugins/governance/routing.gofor CEL map-key regex caching.Changes
wildcardRegexpCache sync.Mapinhandlers/utils.gowith cache-first lookup inmatchesWildcardPattern"sync"importhandlers/wildcard_test.gowith 26 test cases coveringmatchesWildcardPatternandIsOriginAllowedBenchmark (benchstat, 8 rounds, main vs fix)
MatchesWildcardPatternMatchesWildcardPattern_ParallelIsOriginAllowed_WithWildcardsIsOriginAllowed_WithWildcards_ParallelIsOriginAllowed_ExactOnlyMemory drops to 0 B/op, 0 allocs/op across all wildcard benchmarks (from 27-36 KiB and 340-422 allocs/op on main).
Benchmark:
wildcard_bench_test.txt
Type of change
Affected areas
How to test