ip,ip6: allow route replace from api #299
Conversation
📝 WalkthroughWalkthroughAdds internal helpers rib4_insert_or_replace and rib6_insert_or_replace that consolidate insert/replace semantics for IPv4 and IPv6 routes. Public rib4_insert/rib6_insert become thin wrappers calling these helpers with replace=false. Route add paths (route4_add/route6_add) now pass req->exist_ok to the helpers to allow conditional replacement. Changes introduce nexthop incref/decref, nh id↔ptr conversions, exact RIB lookup/insert, RN ext origin storage, FIB updates, GR event emission for non-INTERNAL origins, and setting gateway flags. smoke/config_test.sh adds default-route replacement checks for IPv4 and IPv6. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
modules/ip6/control/route.c (1)
298-312: Avoid setting GR_NH_F_GATEWAY in the API path; helper already does it and this sets it even on failure.
- rib6_insert_or_replace sets nh->flags |= GR_NH_F_GATEWAY on success. Doing it again in route6_add is redundant.
- Worse, if rib6_insert_or_replace fails, you still set the flag here, leaving a gateway bit on an NH not referenced by any route.
Remove both the redundant EEXIST folding (now unreachable with replace=true) and the unconditional flag set.
- if (ret == -EEXIST && req->exist_ok) - ret = 0; - - nh->flags |= GR_NH_F_GATEWAY; + if (ret == -EEXIST && req->exist_ok) { + /* With replace=true, we don't expect -EEXIST, keep for safety if semantics change. */ + ret = 0; + }If you prefer to keep an explicit flag set, guard it on success:
if (ret == 0) { nh->flags |= GR_NH_F_GATEWAY; // though redundant with helper, harmless }
🧹 Nitpick comments (4)
smoke/config_test.sh (1)
20-20: Guard the replace assertion in the smoke test.If replace isn’t accepted by the API yet, this line won’t fail the script. Make it explicit so the smoke test actually exercises/guards the behavior.
-grcli add ip route 0.0.0.0/0 via 10.0.0.1 +grcli add ip route 0.0.0.0/0 via 10.0.0.1 || fail "route replace should succeed"modules/ip/control/route.c (2)
168-178: Consider suppressing duplicate “ADD” events on idempotent updates.When replace=true and the route’s nexthop is identical, you still emit GR_EVENT_IP_ROUTE_ADD. Optional: skip the event if nexthop_equal(existing, nh) to avoid add-event churn on idempotent FRR “add” updates.
- if (origin != GR_NH_ORIGIN_INTERNAL) { + if (origin != GR_NH_ORIGIN_INTERNAL && !(existing && nexthop_equal(nh, existing))) { gr_event_push(
275-281: Unreachable EEXIST folding with replace=true.Because you pass req->exist_ok as the replace flag, -EEXIST cannot be returned in this path. The folding below is now dead code.
- if (ret == -EEXIST && req->exist_ok) - ret = 0;modules/ip6/control/route.c (1)
183-192: Optional: suppress duplicate “ADD” events on idempotent updates (same as v4).Same rationale as IPv4 helper to reduce noisy add-events on equal-NH replacements.
- if (origin != GR_NH_ORIGIN_INTERNAL) { + if (origin != GR_NH_ORIGIN_INTERNAL && !(existing && nexthop_equal(nh, existing))) { gr_event_push(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
modules/ip/control/route.c(4 hunks)modules/ip6/control/route.c(3 hunks)smoke/config_test.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
modules/ip/control/route.c (4)
modules/ip/datapath/fib4.c (1)
nh_id_to_ptr(86-88)modules/ip6/datapath/fib6.c (1)
nh_id_to_ptr(80-82)modules/infra/control/nexthop.c (2)
nexthop_equal(400-427)nexthop_decref(500-529)api/gr_errno.h (1)
errno_set(9-12)
modules/ip6/control/route.c (4)
modules/infra/control/nexthop.c (3)
nexthop_incref(531-533)nexthop_equal(400-427)nexthop_decref(500-529)modules/ip6/datapath/gr_fib6.h (1)
addr6_linklocal_scope(33-46)modules/ip/control/route.c (1)
nh_id_to_ptr(89-91)api/gr_errno.h (1)
errno_set(9-12)
🔇 Additional comments (4)
modules/ip/control/route.c (2)
125-168: Insert/replace consolidation looks correct (lookup-exact, create, NH compare, refcounting).The new helper is clean: exact-lookup then on-demand insert, correct nexthop equality gating (-EEXIST vs -EBUSY), and pre-incref with proper unwind. Nice.
180-187: Good: old nexthop decref only after rebinding.Replacing the RN’s NH before decref prevents premature free of the previous nexthop. Correct sequencing.
modules/ip6/control/route.c (2)
135-201: IPv6 insert/replace helper mirrors v4 correctly (scoping, exact-lookup+insert, NH compare, refcounts).Looks solid: link-local scoping, graceful creation, correct equal/busy handling, and proper decref of the replaced NH.
203-212: Wrapper preserves public API while centralizing logic.Good thin wrapper delegating to the new helper with replace=false.
modules/ip6/control/route.c
Outdated
| if (ret == -EEXIST && req->exist_ok) | ||
| ret = 0; |
There was a problem hiding this comment.
This seems redundant now? Also the change of flag just below should be moved in the rib6_insert_or_replace function I guess. On failure, we shouldn't set this flag.
modules/ip/control/route.c
Outdated
| if (ret == -EEXIST && req->exist_ok) | ||
| ret = 0; |
modules/ip/control/route.c
Outdated
| ret = -rte_errno; | ||
| rte_rib_get_nh(rn, &nh_id); | ||
| existing = nh_id_to_ptr(nh_id); | ||
| if (existing != NULL && replace == false) { |
There was a problem hiding this comment.
This logic is a bit confusing. Could you rewrite as follows:
if ((rn = rte_rib_lookup_exact(rib, rte_be_to_cpu_32(ip), prefixlen)) == NULL) {
rn = rte_rib_insert(rib, rte_be_to_cpu_32(ip), prefixlen);
if (rn == NULL) {
ret = -rte_errno;
goto fail;
}
} else if (!replace) {
uintptr_t nh_id;
rte_rib_get_nh(rn, &nh_id);
ret = nexthop_equal(nh, nh_id_to_ptr(nh_id)) ? -EEXIST : -EBUSY;
goto fail;
}|
|
||
| if ((rn = rte_rib6_lookup_exact(rib, scoped_ip, prefixlen)) == NULL) { | ||
| rn = rte_rib6_insert(rib, scoped_ip, prefixlen); | ||
| if (rn == NULL) { | ||
| ret = -rte_errno; | ||
| goto fail; | ||
| } | ||
| } | ||
|
|
||
| scoped_ip = addr6_linklocal_scope(ip, &tmp, iface_id); | ||
| if ((rn = rte_rib6_insert(rib, scoped_ip, prefixlen)) == NULL) { | ||
| ret = -rte_errno; | ||
| rte_rib6_get_nh(rn, &nh_id); | ||
| existing = nh_id_to_ptr(nh_id); | ||
| if (existing != NULL && replace == false) { | ||
| ret = nexthop_equal(nh, existing) ? -EEXIST : -EBUSY; | ||
| goto fail; | ||
| } |
There was a problem hiding this comment.
This logic is a bit confusing. Could you rewrite as follows:
if ((rn = rte_rib6_lookup_exact(rib, scoped_ip, prefixlen)) == NULL) {
rn = rte_rib6_insert(rib, scoped_ip, prefixlen);
if (rn == NULL) {
ret = -rte_errno;
goto fail;
}
} else if (!replace) {
uintptr_t nh_id;
rte_rib6_get_nh(rn, &nh_id);
ret = nexthop_equal(nh, nh_id_to_ptr(nh_id)) ? -EEXIST : -EBUSY;
goto fail;
}
christophefontaine
force-pushed
the
replace_route
branch
2 times, most recently
from
8e45bd3 to
2ece13a
Compare
christophefontaine
force-pushed
the
replace_route
branch
from
2ece13a to
9e6f03e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
modules/ip6/control/route.c (1)
174-174: Gateway flag now set inside helper — goodThis addresses the previous feedback about not flipping flags in the API layer and doing it only on a successful insert/replace.
🧹 Nitpick comments (2)
modules/ip/control/route.c (2)
160-176: Consider skipping updates/events on no-op replacementsIf replace=true and the existing nexthop is equal to the new one, you can early-return 0 without touching FIB/RIB or emitting an ADD event. This avoids unnecessary churn. Optional, but nice to have.
258-266: Nit: centralize gateway flag setting in rib4_insert_or_replace (align with IPv6)You set flags = GR_NH_F_GATEWAY when creating a new unresolved nexthop here, and also set nh->flags |= GR_NH_F_GATEWAY in rib4_insert_or_replace(). Redundant and slightly asymmetric with IPv6. Safe to remove the flag here and set it only inside the helper.
Example initializer without the flag:
nh = nexthop_new(&(struct gr_nexthop) { .type = GR_NH_T_L3, .af = GR_AF_IP4, .vrf_id = req->vrf_id, .iface_id = nh->iface_id, .ipv4 = req->nh, .origin = req->origin, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
modules/ip/control/route.c(4 hunks)modules/ip6/control/route.c(3 hunks)smoke/config_test.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
smoke/config_test.sh (1)
smoke/_init.sh (1)
fail(58-61)
modules/ip/control/route.c (5)
modules/ip6/control/route.c (1)
nh_id_to_ptr(88-90)modules/ip/datapath/fib4.c (1)
nh_id_to_ptr(86-88)modules/ip6/datapath/fib6.c (1)
nh_id_to_ptr(80-82)modules/infra/control/nexthop.c (2)
nexthop_equal(400-427)nexthop_decref(500-529)api/gr_errno.h (1)
errno_set(9-12)
modules/ip6/control/route.c (6)
modules/infra/control/nexthop.c (3)
nexthop_incref(531-533)nexthop_equal(400-427)nexthop_decref(500-529)modules/ip6/datapath/gr_fib6.h (1)
addr6_linklocal_scope(33-46)modules/ip/control/route.c (1)
nh_id_to_ptr(89-91)modules/ip/datapath/fib4.c (1)
nh_id_to_ptr(86-88)modules/ip6/datapath/fib6.c (1)
nh_id_to_ptr(80-82)api/gr_errno.h (1)
errno_set(9-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
🔇 Additional comments (7)
modules/ip/control/route.c (2)
187-195: Wrapper refactor is cleanrib4_insert() delegating to rib4_insert_or_replace() keeps the public API stable while centralizing logic. Good.
273-275: Exist-ok path correctly propagatedroute4_add now forwards exist_ok to the insert-or-replace helper. This matches the PR intent (accept replace from API).
smoke/config_test.sh (3)
20-20: LGTM: IPv4 replace smoke-checkAdding a second default route add with a must-succeed guard is a good end-to-end probe for the new replace semantics.
26-26: LGTM: IPv6 replace smoke-checkSame here for IPv6, good coverage for ::/0 replacement behavior.
19-21: exist_ok is set by default for IP/IP6 route add in the CLI
- modules/ip/cli/route.c:
gr_ip4_route_add_req req = {.exist_ok = true, .origin = …}- modules/ip6/cli/route.c:
gr_ip6_route_add_req req = {.exist_ok = true, .origin = …}No changes required.
modules/ip6/control/route.c (2)
201-210: Wrapper refactor is cleanrib6_insert() delegating to rib6_insert_or_replace() keeps the public API unchanged and centralizes logic.
296-304: Exist-ok path correctly propagatedroute6_add now forwards exist_ok to the insert-or-replace helper. Matches IPv4 and PR intent.
On a route update, FRR doesn't remove and add a route, but calls a route add. This call is rejected as the route already exists. Allow route modification from the API. Signed-off-by: Christophe Fontaine <[email protected]>
christophefontaine
force-pushed
the
replace_route
branch
from
9e6f03e to
88690cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
modules/ip6/control/route.c (1)
176-176: Good: gateway flag now set inside the helperMoving GR_NH_F_GATEWAY assignment here avoids flagging on failed inserts. This addresses prior feedback.
🧹 Nitpick comments (3)
modules/ip/control/route.c (2)
146-160: Avoid churn on no-op replace (existing == nh)When the existing nexthop pointer equals the new one and replace=true, we still mutate state, call fib4_insert, and emit an ADD event. This is a pure no-op and can be short-circuited to reduce unnecessary work and event noise, while still keeping the early incref balanced and the origin up to date.
Apply:
} else { uintptr_t nh_id; rte_rib_get_nh(rn, &nh_id); existing = nh_id_to_ptr(nh_id); + if (existing == nh) { + // No-op replacement: keep RIB/FIB unchanged, but ensure flags/origin are set, + // and balance the early incref. + nh->flags |= GR_NH_F_GATEWAY; + o = rte_rib_get_ext(rn); + *o = origin; + nexthop_decref(nh); + return 0; + } if (!replace) { ret = nexthop_equal(nh, existing) ? -EEXIST : -EBUSY; goto fail; } }
146-151: Tiny nit: avoid recomputing rte_be_to_cpu_32(ip)Store the converted address once to cut duplicate calls and tighten readability.
- if ((rn = rte_rib_lookup_exact(rib, rte_be_to_cpu_32(ip), prefixlen)) == NULL) { - rn = rte_rib_insert(rib, rte_be_to_cpu_32(ip), prefixlen); + uint32_t cpu_ip = rte_be_to_cpu_32(ip); + if ((rn = rte_rib_lookup_exact(rib, cpu_ip, prefixlen)) == NULL) { + rn = rte_rib_insert(rib, cpu_ip, prefixlen);modules/ip6/control/route.c (1)
160-174: Short-circuit no-op replace (existing == nh) to avoid unnecessary updatesSame as IPv4: when the RN already points to the same nexthop and replace=true, skip FIB/RIB updates and event emission; still set flags, update origin, and balance the early incref.
} else { uintptr_t nh_id; rte_rib6_get_nh(rn, &nh_id); existing = nh_id_to_ptr(nh_id); + if (existing == nh) { + nh->flags |= GR_NH_F_GATEWAY; + o = rte_rib6_get_ext(rn); + *o = origin; + nexthop_decref(nh); + return 0; + } if (!replace) { ret = nexthop_equal(nh, existing) ? -EEXIST : -EBUSY; goto fail; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
modules/ip/control/route.c(4 hunks)modules/ip6/control/route.c(3 hunks)smoke/config_test.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- smoke/config_test.sh
🧰 Additional context used
🧬 Code Graph Analysis (2)
modules/ip6/control/route.c (4)
modules/infra/control/nexthop.c (3)
nexthop_incref(531-533)nexthop_equal(400-427)nexthop_decref(500-529)modules/ip6/datapath/gr_fib6.h (1)
addr6_linklocal_scope(33-46)modules/ip/control/route.c (1)
nh_id_to_ptr(89-91)api/gr_errno.h (1)
errno_set(9-12)
modules/ip/control/route.c (5)
modules/ip6/control/route.c (1)
nh_id_to_ptr(88-90)modules/ip/datapath/fib4.c (1)
nh_id_to_ptr(86-88)modules/ip6/datapath/fib6.c (1)
nh_id_to_ptr(80-82)modules/infra/control/nexthop.c (2)
nexthop_equal(400-427)nexthop_decref(500-529)api/gr_errno.h (1)
errno_set(9-12)
🔇 Additional comments (9)
modules/ip/control/route.c (4)
125-132: Good encapsulation of insert/replace semanticsThe internal helper consolidates the add/replace behavior cleanly and keeps the public API unchanged via wrappers. Nice.
180-181: Refcount handling on replace is correctBalancing the early incref by decref’ing the previous RN’s nexthop (including the same-pointer case) keeps refcounts consistent across insert and replace.
189-197: Wrapper preserves public APIrib4_insert delegating to rib4_insert_or_replace(..., false) maintains API behavior while centralizing logic. LGTM.
275-277: exist_ok plumbed to enable conditional replacePassing req->exist_ok down to the helper achieves the desired FRR “add as update” behavior.
modules/ip6/control/route.c (5)
135-143: Parallel IPv6 helper is aligned and cleanThe rib6_insert_or_replace helper mirrors IPv4 well and preserves the public API via a wrapper.
153-158: Order of operations is soundComputing scoped_ip up front and deferring failure handling to a single path keeps the flow readable. Early incref is balanced in all exits.
194-196: Refcount on previous nexthop handled correctlyDropping the previous RN-held reference post-update ensures no leaks and properly frees when appropriate (QSBR guarded).
203-213: Wrapper preserves API semanticsrib6_insert forwarding to rib6_insert_or_replace(..., false) looks good.
298-306: exist_ok now enables replace from APIWiring req->exist_ok through route6_add to the helper achieves the intended behavior for FRR updates. Nice.
2 participants
On a route update, FRR doesn't remove and add a route, but calls a route add.
This call is rejected as the route already exists. Allow route modification from the API.
Summary by CodeRabbit
New Features
Tests