Add patch to send crowdsec stats - #16
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis pull request adds CrowdSec usage-metrics support to LemonLDAP::NG across manager configuration and portal request processing. The changes introduce two new configuration attributes (enablement toggle and flush interval), implement metrics collection and periodic HTTP POST delivery to CrowdSec LAPI, and extend the bouncer method's return signature to pass ban decision context. ChangesCrowdSec Metrics Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new crowdsec-stats.patch to extend the LemonLDAP::NG CrowdSec integration so the portal can push usage-metrics to the CrowdSec Local API, and wires the patch into the various Docker images (portal/uwsgi-portal/manager/full). It also adds Manager configuration knobs to control whether metrics are enabled and how frequently they flush.
Changes:
- Apply a new
crowdsec-stats.patchin the portal and uwsgi-portal images to record request counters and POST usage-metrics to CrowdSec LAPI. - Add new Manager configuration attributes (
crowdsecMetricsEnabled,crowdsecMetricsInterval) and expose them in the configuration tree + UI strings. - Update Dockerfiles and
Changes.mdto include the new patch.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uwsgi-portal/Dockerfile | Adds crowdsec-stats.patch to the patch-apply sequence. |
| uwsgi-portal/crowdsec-stats.patch | Adds request metrics recording + periodic usage-metrics POST to CrowdSec LAPI for uwsgi portal build. |
| portal/Dockerfile | Adds crowdsec-stats.patch to the patch-apply sequence. |
| portal/crowdsec-stats.patch | Adds request metrics recording + periodic usage-metrics POST to CrowdSec LAPI for portal build. |
| manager/Dockerfile | Adds crowdsec-stats.patch to the patch-apply sequence. |
| manager/crowdsec-stats.patch | Introduces Manager-side config attributes + tree entries + language strings for metrics settings. |
| full/Dockerfile | Adds crowdsec-stats.patch to the patch-apply sequence. |
| full/crowdsec-stats.patch | Same Manager-side config/strings patch applied in the full image. |
| Changes.md | Notes addition of the CrowdSec statistics patch in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
portal/Dockerfile (1)
67-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a safe glob when deleting patch files.
ShellCheck flags
rm -f *.patchwith SC2035: file names starting with dashes could be misinterpreted as options. Userm -f -- ./*.patchto safely handle any filename.🧹 Proposed fix
- rm -f *.patch && \ + rm -f -- ./*.patch && \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portal/Dockerfile` around lines 67 - 82, The RUN step in the Dockerfile uses a bare glob removal "rm -f *.patch" which ShellCheck flags (SC2035) because filenames beginning with dashes could be treated as options; update the removal in the RUN command that contains the patch loop and subsequent file operations (the line with "rm -f *.patch") to use a safe glob style by passing the option terminator and an explicit relative glob (e.g., use "-- ./*.patch") so that patch files are removed safely even if their names begin with "-" or contain special characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@portal/crowdsec-stats.patch`:
- Around line 104-109: The current code resets the in-memory window counters
unconditionally after attempting the POST, which drops metrics on transient
failures; change the logic in the push path so that $self->_metricsCounters(...)
and $self->_droppedByOrigin(...) are only reset when $resp->is_success is true
(i.e., move the counter reset into the success branch) and on failure leave the
counters intact so the next flush will retry sending the same window; keep the
existing warn using $resp->status_line and return/exit the function on failure
without clearing the window.
- Around line 39-42: The guard in _recordRequest is inverted so missing configs
enable metrics; update the condition to only proceed when crowdsecMetricsEnabled
is explicitly true. In function _recordRequest check
$self->conf->{crowdsecMetricsEnabled} (or use exists + truth check) and return
early when it is undefined or false, then increment
_metricsCounters->{processed} and perform posting; reference the _recordRequest
function and conf->{crowdsecMetricsEnabled} setting to locate and correct the
logic.
---
Outside diff comments:
In `@portal/Dockerfile`:
- Around line 67-82: The RUN step in the Dockerfile uses a bare glob removal "rm
-f *.patch" which ShellCheck flags (SC2035) because filenames beginning with
dashes could be treated as options; update the removal in the RUN command that
contains the patch loop and subsequent file operations (the line with "rm -f
*.patch") to use a safe glob style by passing the option terminator and an
explicit relative glob (e.g., use "-- ./*.patch") so that patch files are
removed safely even if their names begin with "-" or contain special characters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38ea55d4-6a3d-4bb5-ba4b-c97e92e230dd
📒 Files selected for processing (9)
Changes.mdfull/Dockerfilefull/crowdsec-stats.patchmanager/Dockerfilemanager/crowdsec-stats.patchportal/Dockerfileportal/crowdsec-stats.patchuwsgi-portal/Dockerfileuwsgi-portal/crowdsec-stats.patch
| +sub _recordRequest { | ||
| + my ( $self, $ip, $banDecision ) = @_; | ||
| + return unless defined $self->conf->{crowdsecMetricsEnabled} and !$self->conf->{crowdsecMetricsEnabled}; | ||
| + $self->_metricsCounters->{processed}++; |
There was a problem hiding this comment.
The metrics feature is effectively enabled when the key is missing.
Line 41 only returns when crowdsecMetricsEnabled is both defined and false. On older configs where the key is absent, _recordRequest() still counts requests and will eventually post usage metrics, which breaks the advertised opt-in behavior.
🔒 Proposed fix
sub _recordRequest {
my ( $self, $ip, $banDecision ) = `@_`;
- return unless defined $self->conf->{crowdsecMetricsEnabled} and !$self->conf->{crowdsecMetricsEnabled};
+ return unless $self->conf->{crowdsecMetricsEnabled};
$self->_metricsCounters->{processed}++;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +sub _recordRequest { | |
| + my ( $self, $ip, $banDecision ) = @_; | |
| + return unless defined $self->conf->{crowdsecMetricsEnabled} and !$self->conf->{crowdsecMetricsEnabled}; | |
| + $self->_metricsCounters->{processed}++; | |
| sub _recordRequest { | |
| my ( $self, $ip, $banDecision ) = `@_`; | |
| return unless $self->conf->{crowdsecMetricsEnabled}; | |
| $self->_metricsCounters->{processed}++; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portal/crowdsec-stats.patch` around lines 39 - 42, The guard in
_recordRequest is inverted so missing configs enable metrics; update the
condition to only proceed when crowdsecMetricsEnabled is explicitly true. In
function _recordRequest check $self->conf->{crowdsecMetricsEnabled} (or use
exists + truth check) and return early when it is undefined or false, then
increment _metricsCounters->{processed} and perform posting; reference the
_recordRequest function and conf->{crowdsecMetricsEnabled} setting to locate and
correct the logic.
Summary by CodeRabbit