Open davidspek opened 1 year ago
Merging #1073 (eb4a176) into master (3a716f2) will decrease coverage by
0.44%
. The diff coverage is60.86%
.:exclamation: Current head eb4a176 differs from pull request most recent head 5542209. Consider uploading reports for the commit 5542209 to get more accurate results
@@ Coverage Diff @@
## master #1073 +/- ##
==========================================
- Coverage 78.17% 77.73% -0.44%
==========================================
Files 80 81 +1
Lines 3853 3979 +126
==========================================
+ Hits 3012 3093 +81
- Misses 566 603 +37
- Partials 275 283 +8
Impacted Files | Coverage Δ | |
---|---|---|
driver/configuration/provider_koanf.go | 88.13% <0.00%> (-0.76%) |
:arrow_down: |
rule/repository_memory.go | 75.92% <45.45%> (-9.61%) |
:arrow_down: |
rule/trie.go | 69.56% <69.56%> (ø) |
Hey David, thank you for this PR. We are also struggling ourselves with this problem (increasingly complex rules that are hard to read) but do not have a clear idea how to address these issues properly. Do you have some examples for the rule matching you're suggestion? I'm not quite sure if I understand the approach correctly.
In the future may I suggest to first create a design document to exchange ideas and follow up with an implementation afterwards? :) I know that we are not as responsive as in other repositories but design documents are a good way to work on topics such as this one.
Thanks!
Hi Aeneasr. I can create an issue with a design document for what I’ve done in this PR tomorrow. However, I find that it usually helps to have a (brief) synchronous discussion to get on the same page before creating long write-ups. I think this would be particularly helpful since this codebase is mostly foreign to me. Do you think you’d have time to have a short chat some day soon?
Currently rules are matched to an incoming request based on regex or glob pattern matching. Since only a single rule is allowed to match a request, the regex or glob patterns must be very precise and become increasingly complex as different paths need to be matched. While the used regex library supports negative lookahead, creating rules where requests are matched based on path prefixes is still difficult since you'd need to provide negative matches for all other rule regexes with the same base path. As glob doesn't support negative lookahead doing path prefix based routing is not possible. Even when using regex patterns with negative lookahead, the issue that arises is that the end user ends up needing to manage system with the state of all rules to then be able to create rules with the appropriate regexes including negative lookahead patterns for all other rules. Since the most common type of routing people likely would want to implement is prefixed based routing, the fact that doing so with oathkeeper is difficult and error prone is in my views its biggest drawback.
This PR introduces a system that allows for matching rules based on the longest matching path prefix between a request URL and the paths of all the rules using a Trie. With this trie, oathkeeper does not need to range over each rule when performing the matching which I expect will decrease latency.
Note, this implementation is likely incomplete and requires some further work which I would like to implement after some further discussion here. Some things that come to mind are:
Update: I've made the prefix matching a separate config from the matching strategy. This way, if multiple rules are found based on the path prefix those rules are then further filtered using the matching pattern. Matching patterns can only be used in the path of the URL and are not added into the Trie (since they would break the Trie). Thus, 2 rules with the same path prefix but different matching patterns will be added to the same node in the Trie. Then if a request comes in that matches those rules it will be matched using the pattern. Note that the intension for pattern matching combined with prefix matching is for use of the patterns in downstream handlers. It is not intended to be used for determining which rule an incoming request should match against, but it will fall back to doing so. In any case, it is much easier to create a negative lookahead for a single path section rather than all paths known to oathkeeper.
Related issue(s)
https://github.com/ory/oathkeeper/pull/1073 https://github.com/ory/oathkeeper/issues/441
Checklist
Further Comments