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

Implement local cache for crowdsec #33

Closed
wants to merge 11 commits into from
Closed

Implement local cache for crowdsec #33

wants to merge 11 commits into from

Conversation

maxlerebourg
Copy link

#32
With better implementation

3 mode:

  • stream mode: use /decisions/stream to get in cache the decisions.
  • live mode: like before but with cached response from crowdsec
  • none mode: like before (every request ask crowdsec)

Copy link
Owner

@fbonalair fbonalair left a 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.

README.md Outdated Show resolved Hide resolved
controler/controler.go Outdated Show resolved Hide resolved
controler/controler.go Outdated Show resolved Hide resolved
controler/controler.go Outdated Show resolved Hide resolved
controler/controler.go Outdated Show resolved Hide resolved
controler/controler.go Outdated Show resolved Hide resolved
c.String(crowdsecBanResponseCode, crowdsecBanResponseMsg)
} else {
if crowdsecBouncerCacheMode == "live" {
cache.Set(key, []byte("f"), int(crowdsecBouncerDefaultCacheDuration.Seconds()))
Copy link
Owner

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.

Copy link
Author

@maxlerebourg maxlerebourg Sep 19, 2022

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 ?

controler/controler.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
controler/controler.go Show resolved Hide resolved
caches/caches.go Show resolved Hide resolved
caches/caches.go Show resolved Hide resolved
@maxlerebourg
Copy link
Author

maxlerebourg commented Sep 19, 2022

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.
I have fixed all the issues you brought up, but one of them is not understandable to me. I let it opened.

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.

@codeclimate
Copy link

codeclimate bot commented Sep 19, 2022

Code Climate has analyzed commit d748d1d and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 4

View more on Code Climate.

@maxlerebourg maxlerebourg closed this by deleting the head repository Apr 25, 2023
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.

2 participants