Allow setting a root fs.FS in a parser. #393
Conversation
Codecov ReportBase: 76.78% // Head: 76.75% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## v3/dev #393 +/- ##
==========================================
- Coverage 76.78% 76.75% -0.04%
==========================================
Files 136 136
Lines 5957 5966 +9
==========================================
+ Hits 4574 4579 +5
- Misses 1113 1118 +5
+ Partials 270 269 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
jcchavezs
requested review from
piyushroshan and
jptosso
and removed request for
piyushroshan
|
LGTM |
|
I've read about this issue, caddyserver does the same for s3 mounts or similar in 2.6. I will take a look of the code but looks nice |
anuraaga
force-pushed
the
parser-root
branch
from
8dd0442
to
edd178f
Compare
|
BTW I could confirm it works well here https://github.com/anuraaga/coraza-wasm-filter/pull/18
Glob works fine too (added a commit :) ). |
| @@ -142,6 +145,7 @@ func (p *Parser) evaluate(data string) error { | |||
| p.options.Config.Set("last_profile_line", p.currentLine) | |||
| p.options.Config.Set("parser_config_file", p.currentFile) | |||
| p.options.Config.Set("parser_config_dir", p.currentDir) | |||
| p.options.Config.Set("parser_root", p.root) | |||
There was a problem hiding this comment.
This should replace the working_dir declared in https://github.com/corazawaf/coraza/pull/393/files#diff-f73e6b849d0fa747f9e706cbdb5ec76083f712530e21d64ea917cc37eb6347c5R153
There was a problem hiding this comment.
Sorry don't quite follow the link, do you mean remove p.options.Config.Set/Get("working_dir") and have p.root default to working dir if user doesn't call SetRoot?
In that case, this won't work even if the user doesn't call SetRoot
coraza/waf.exe
config/rules.conf
Since .. wouldn't work. While we are discussing being OK with ../ not working with an explicit call to SetRoot, I'm not sure about when it hasn't been called at all, I guess it should be allowed though.
|
I have gone ahead and removed Note that this PR is very important for the proxy-wasm plugin so I'm finding myself merging this branch locally to any other bugfixes like #399 while debugging ftw tests. Still no rush, but a sooner than later merge would be highly appreciated :D |
|
@jptosso Can you take a look at this? It's blocking a lot of work on the wasm filter. Thanks! |
fs.FSis a great abstraction that allows operating on files with the same code anywhere from the filesystem (except I had to implementio.OSFS,not sure why standard library doesn't provide that...found out why and added note),embed.FS,zip.Reader, and anything a user may implement, maybe even a remote cloud storage.By allowing setting a
fs.FS, we can support loading config from any of these locations. Notably, we are currently embedding rules into the Envoy filter due to current lack of support to access the filesystem, and while currently we take CRS and preprocess them to remove Include and FromFile statements, with this we should be able to remove most if not all such processing when embedding.