-
Notifications
You must be signed in to change notification settings - Fork 22
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
Implement local cache for crowdsec #33
Conversation
This reverts commit ecba24b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#21
Thank you a lot for the work !
Excellent idea for the go routine around the cache.
I added some quality of life comments around, feel free to check them out.
On more major note though is in the steam mode: make the server fail in case of error. For the bouncer I took a very defensive approach - it's a security component after all - we don't want a bouncer that is allowing any request because we couldn't update the cache so fail-fast. Thanks to you, user have other mode as fallback if they can't found/fix the issue.
For that in the function HandleStreamCache, you have a lot of returns in case of error but without handling them. In those case Panic with a descriptive log and make the whole bouncer fail.
controler/controler.go
Outdated
c.String(crowdsecBanResponseCode, crowdsecBanResponseMsg) | ||
} else { | ||
if crowdsecBouncerCacheMode == "live" { | ||
cache.Set(key, []byte("f"), int(crowdsecBouncerDefaultCacheDuration.Seconds())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using our value, could we use Crowdsec decision's duration value? The user would just have to configure it once, directly in Crowdsec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fbonalair can Crowdsec response with duration value when the IP is authorized ?
This reverts commit affb47a.
Hey, thanks for choosing my MR, the other one is made by a friend. We have a different approach to it, so you can go look at it too. Another point about the 503 return in case of the cache stream update fail. In your previous comment, you said "crash the bouncer" and in your PR comment "return 503 on /healthz", I choose the second, feel free to tell me the opposite was the good answer. |
Code Climate has analyzed commit d748d1d and detected 4 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
#32
With better implementation
3 mode: