Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis pull request adds ICMP rate limiting functionality across the stack. Changes include a new CLI command handler that accepts an interval parameter, API definitions for the rate-limit request, a control-plane handler to initialize the rate limiter, a new rate-limiter implementation with initialization and permission-checking functions, and integration into the ICMP input datapath to gate Echo Request handling using the rate-limit checks. 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/ip/control/icmp_rl.c`:
- Around line 11-25: The global rate-limiter state (icmp_rl of type time_space)
is accessed concurrently by icmp_rl_init() and icmp_rl_allow(), causing races;
serialize access by protecting reads/writes to icmp_rl.last_ts and
icmp_rl.interval_ms with a concurrency primitive (e.g., a mutex/spinlock or
atomic CAS) so only one worker can update/check the timestamp at a time. Modify
icmp_rl_init() to acquire the lock before setting icmp_rl.interval_ms and
icmp_rl.last_ts and release it afterwards, and modify icmp_rl_allow() to
atomically read/compare/update icmp_rl.last_ts (or acquire the same lock around
the now - last_ts check and the assignment) to ensure the interval logic is
enforced across workers; keep identifiers icmp_rl, icmp_rl_init, icmp_rl_allow,
last_ts, interval_ms and time_space for locating the changes.
- Around line 13-25: icmp_rl_init currently sets icmp_rl.last_ts to now which
forces the first icmp_rl_allow() call to wait a full interval; instead subtract
the configured interval (in microseconds) from the current timestamp so the
first reply is allowed immediately, and when comparing/multiplying use 64-bit
arithmetic: convert icmp_rl.interval_ms to microseconds with a uint64_t cast
(e.g. (uint64_t)icmp_rl.interval_ms * 1000) and use that value both when
backdating icmp_rl.last_ts in icmp_rl_init and when comparing in icmp_rl_allow.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e39c273e-63bf-413b-9e47-df4d6e32a9f7
📒 Files selected for processing (7)
modules/infra/cli/icmp.cmodules/ip/api/gr_ip4.hmodules/ip/control/icmp.cmodules/ip/control/icmp_rl.cmodules/ip/control/icmp_rl.hmodules/ip/control/meson.buildmodules/ip/datapath/icmp_input.c
| static struct time_space icmp_rl = {0}; | ||
|
|
||
| void icmp_rl_init(uint32_t interval_ms) { | ||
| icmp_rl.interval_ms = interval_ms; | ||
| icmp_rl.last_ts = gr_clock_us(); | ||
| } | ||
|
|
||
| bool icmp_rl_allow() { | ||
| uint64_t now = gr_clock_us(); | ||
|
|
||
| if ((now - icmp_rl.last_ts) < (icmp_rl.interval_ms * 1000)) | ||
| return false; | ||
|
|
||
| icmp_rl.last_ts = now; | ||
| return true; |
There was a problem hiding this comment.
Serialize the shared limiter state.
icmp_rl_allow() and icmp_rl_init() both read/write the same global icmp_rl object, but this code is exercised from the ICMP input node, which can run on multiple workers. As written, concurrent packets can race through the check/update and both be accepted, so the rate limit is not enforced reliably.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/ip/control/icmp_rl.c` around lines 11 - 25, The global rate-limiter
state (icmp_rl of type time_space) is accessed concurrently by icmp_rl_init()
and icmp_rl_allow(), causing races; serialize access by protecting reads/writes
to icmp_rl.last_ts and icmp_rl.interval_ms with a concurrency primitive (e.g., a
mutex/spinlock or atomic CAS) so only one worker can update/check the timestamp
at a time. Modify icmp_rl_init() to acquire the lock before setting
icmp_rl.interval_ms and icmp_rl.last_ts and release it afterwards, and modify
icmp_rl_allow() to atomically read/compare/update icmp_rl.last_ts (or acquire
the same lock around the now - last_ts check and the assignment) to ensure the
interval logic is enforced across workers; keep identifiers icmp_rl,
icmp_rl_init, icmp_rl_allow, last_ts, interval_ms and time_space for locating
the changes.
| void icmp_rl_init(uint32_t interval_ms) { | ||
| icmp_rl.interval_ms = interval_ms; | ||
| icmp_rl.last_ts = gr_clock_us(); | ||
| } | ||
|
|
||
| bool icmp_rl_allow() { | ||
| uint64_t now = gr_clock_us(); | ||
|
|
||
| if ((now - icmp_rl.last_ts) < (icmp_rl.interval_ms * 1000)) | ||
| return false; | ||
|
|
||
| icmp_rl.last_ts = now; | ||
| return true; |
There was a problem hiding this comment.
Don't arm the limiter with the current timestamp.
icmp_rl_init() currently makes the first Echo Reply after configuration wait a full interval, and the interval_ms * 1000 conversion is still done in 32-bit arithmetic. Please backdate last_ts by the configured interval and widen the microsecond conversion to uint64_t.
Possible fix
void icmp_rl_init(uint32_t interval_ms) {
+ uint64_t interval_us = (uint64_t)interval_ms * 1000ULL;
icmp_rl.interval_ms = interval_ms;
- icmp_rl.last_ts = gr_clock_us();
+ icmp_rl.last_ts = gr_clock_us() - interval_us;
}
bool icmp_rl_allow() {
uint64_t now = gr_clock_us();
+ uint64_t interval_us = (uint64_t)icmp_rl.interval_ms * 1000ULL;
- if ((now - icmp_rl.last_ts) < (icmp_rl.interval_ms * 1000))
+ if ((now - icmp_rl.last_ts) < interval_us)
return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/ip/control/icmp_rl.c` around lines 13 - 25, icmp_rl_init currently
sets icmp_rl.last_ts to now which forces the first icmp_rl_allow() call to wait
a full interval; instead subtract the configured interval (in microseconds) from
the current timestamp so the first reply is allowed immediately, and when
comparing/multiplying use 64-bit arithmetic: convert icmp_rl.interval_ms to
microseconds with a uint64_t cast (e.g. (uint64_t)icmp_rl.interval_ms * 1000)
and use that value both when backdating icmp_rl.last_ts in icmp_rl_init and when
comparing in icmp_rl_allow.
Implement a basic global ICMP rate limiter based on a minimum time space between responses. Signed-off-by: Farid Mihoub <farid.mihoub@yandex.com>
|
Hi, thanks a lot for your contribution ! |
|
Hi @christophefontaine I guess I should close the current PR. I would like to see if there is a specific subject I can work on. |
There are many things which would require some work. They are more or less fast/easy to implement. Some of them require extensive design work. Is there anything you need for a use case or are you doing this for exploration purposes? Here are a few random ideas:
|
I have no specific use case, just looking to make open-source contributions. Grout caught my eye for two reasons: it aligns with my work (DPI, networking, DPDK integration...) and will definitely help harden my skills, and I genuinely appreciate your commitment to clean code and software architecture. Such high-quality work! |
|
@FRIDM636 it seems you have some experience on FRR, any help on this topic will be appreciate. You can join your channel on stack: https://dpdkproject.slack.com/archives/C07NAFWE1MG |
4 participants
Hello,
I've been studying the grout codebase and I'm looking forward to contributing to it.
As a first step, I have implemented a basic ICMP rate limiter based on time-spacing.
Before extending it, I would like your feedback on:
Thanks.
ICMP Rate Limiter Implementation
Implements a basic global ICMP rate limiter based on minimum time spacing between Echo Request responses.
Core Implementation
modules/ip/control/icmp_rl.h and modules/ip/control/icmp_rl.c
time_spacestructure trackinglast_ts(microsecond timestamp) andinterval_ms(millisecond interval).icmp_rl_init(interval_ms)initializes the limiter with caller-provided interval and captures current time viagr_clock_us().icmp_rl_allow()compares elapsed microseconds against the configured interval; rejects requests occurring before the interval elapses (returnsfalse), updates timestamp on acceptance (returnstrue).Control Plane Integration
modules/ip/api/gr_ip4.h
GR_IP4_ICMP_RATE_LIMITrequest type withstruct gr_ip4_icmp_rl_reqcontaining auint32_t rate_limitfield.modules/ip/control/icmp.c
icmp_rate_limit()API handler that receives the interval value and callsicmp_rl_init().Data Path Integration
modules/ip/datapath/icmp_input.c
RATE_LIMITEDedge to the ICMP input node state machine.icmp_rl_allow()check.icmp_rate_limiteddrop path instead of echo reply output.User Interface
modules/infra/cli/icmp.c
icmp rate-limit (INTERVAL)CLI command accepting interval in milliseconds (1 to UINT32_MAX).modules/ip/control/meson.build
icmp_rl.cto build source list.