Vrf support for frr #264
Vrf support for frr #264
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @maxime-leroy - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `modules/infra/api/nexthop.c:47` </location>
<code_context>
struct gr_nexthop base = req->nh;
struct nexthop *nh = NULL;
-
+ struct iface *iface;
int ret;
</code_context>
<issue_to_address>
Assignment of base.vrf_id from iface may override user input unexpectedly.
If overriding is intended, document this for all callers. Otherwise, validate that the user-supplied vrf_id matches iface->vrf_id before assignment.
Suggested implementation:
```c
struct iface *iface;
int ret;
```
```c
/* Validate or assign vrf_id from iface */
if (iface) {
if (base.vrf_id) {
if (base.vrf_id != iface->vrf_id) {
/* User-supplied vrf_id does not match iface's vrf_id */
return api_out(EINVAL, 0);
}
/* else: user-supplied vrf_id matches iface, proceed */
} else {
/* No user-supplied vrf_id, assign from iface */
base.vrf_id = iface->vrf_id;
}
}
switch (base.af) {
default:
return api_out(ENOPROTOOPT, 0);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
smoke/vrf_forward_test.sh (1)
5-5: Fix shellcheck warning: Quote the dirname command substitution.To prevent word splitting issues, quote the command substitution.
-. $(dirname $0)/_init.sh +. "$(dirname "$0")/_init.sh"
🧹 Nitpick comments (1)
modules/infra/api/nexthop.c (1)
63-65: Consider adding more specific error message.When interface lookup fails, the current implementation returns the errno set by
iface_from_id(). Consider logging a more specific error message to aid debugging.iface = iface_from_id(base.iface_id); if (iface == NULL) - return api_out(errno, 0); + return api_out(errno, 0); // errno is set by iface_from_id() - likely ENODEV
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frr/if_grout.c(7 hunks)frr/if_grout.h(0 hunks)frr/rt_grout.c(13 hunks)frr/zebra_dplane_grout.c(1 hunks)modules/infra/api/nexthop.c(2 hunks)modules/infra/cli/nexthop.c(0 hunks)modules/infra/control/iface.c(2 hunks)smoke/_init_frr.sh(5 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(3 hunks)
💤 Files with no reviewable changes (2)
- frr/if_grout.h
- modules/infra/cli/nexthop.c
🧰 Additional context used
🧬 Code Graph Analysis (3)
modules/infra/api/nexthop.c (2)
modules/infra/control/gr_iface.h (1)
iface(15-21)main/gr_module.h (1)
api_out(16-19)
modules/infra/control/iface.c (1)
api/gr_errno.h (1)
errno_set(9-12)
frr/rt_grout.c (1)
modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(156-224)
🪛 Shellcheck (0.10.0)
smoke/vrf_forward_frr_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 7-7: run_id is referenced but not assigned.
(SC2154)
⏰ 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). (6)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
🔇 Additional comments (24)
smoke/vrf_forward_test.sh (2)
18-23: Good VRF route isolation testing.The test properly validates VRF isolation by:
- Adding routes for 16.x.0.0/16 networks within each VRF
- Assigning loopback addresses in network namespaces
- Testing connectivity to both directly connected and routed networks
This ensures VRF forwarding works correctly.
Also applies to: 32-37, 46-51
7-10: run_id Initialization ConfirmedThe
vrf_forward_test.shscript sourcessmoke/_init.shon line 5, and_init.shdefinesrun_idbefore it’s used. No further action is needed.modules/infra/api/nexthop.c (1)
47-67: Good improvement: VRF ID derived from interface.The change properly encapsulates VRF assignment by deriving it from the interface rather than accepting it directly from the request. This ensures consistency between interface and nexthop VRF assignments.
smoke/vrf_forward_frr_test.sh (2)
28-31: Proper VRF-aware routing configuration.The script correctly configures VRF-aware routes using the helper functions:
set_ip_addresswith VRF parameterset_ip_routewith VRF parameterThis validates the FRR integration with VRF support.
Also applies to: 44-47
12-15: create_interface parameters are correct
Thecreate_interfacefunction insmoke/_init_frr.shis defined as(port, mac, vrf)(withvrfdefaulting to 0). The calls insmoke/vrf_forward_frr_test.shpass:
$pXas the portf0:...as the MAC address- the third numeric argument as the VRF ID
No changes required.
frr/if_grout.c (3)
75-77: Good VRF-aware loopback interface handling.The code correctly identifies loopback interfaces with non-zero VRF IDs and sets the appropriate interface type. This enables proper VRF modeling through loopback interfaces.
113-121: Proper VRF context propagation to dataplane.The implementation correctly sets both table ID and VRF ID in the dataplane context when the interface has a non-zero VRF ID. The comment clearly explains the VRF modeling approach using loopback interfaces.
193-196: Good interface index validation.The added bounds check prevents invalid interface indices from being processed. This improves robustness.
modules/infra/control/iface.c (3)
50-58: Well-designed interface ID reservation function.The helper function provides clean abstraction for ID reservation with proper error handling using errno_set for consistency.
60-71: Clear interface ID allocation strategy.The comment clearly documents that IDs 1-255 are reserved for loopback interfaces. Starting allocation from MAX_VRFS prevents conflicts with VRF loopback interfaces.
95-100: Correct VRF loopback interface ID assignment.The implementation ensures that loopback interfaces with VRF IDs use their VRF ID as the interface ID, maintaining the 1:1 mapping between VRF and its loopback interface. The error handling properly cleans up on reservation failure.
frr/zebra_dplane_grout.c (2)
475-502: VRF initialization sequence looks correct.The function properly handles VRF backend initialization by terminating existing VRFs and recreating the default VRF with table ID 0. The use of
exit(1)on failure is appropriate here since the system cannot function without a default VRF.The comment explaining the table ID mapping (Grout uses VRF ID as table ID, while Linux defaults to 254) provides important context for understanding the VRF-to-table relationship.
508-511: VRF backend check is appropriate.The early exit when netns VRF backend is detected prevents incompatible configurations. The use of
exit(1)is justified by the comment noting thatzebra_dplane_start()doesn't check the return value.smoke/_init_frr.sh (3)
12-15: VRF parameter addition is correct.The optional VRF parameter with default value 0 maintains backward compatibility while enabling VRF-aware interface creation.
34-57: VRF-aware IP address assignment is properly implemented.The VRF parameter is correctly added and used to filter the address verification, ensuring addresses are checked within the correct VRF context.
70-99: VRF-aware route configuration is well implemented.The VRF naming convention (default for VRF 0, gr-loop for others) is properly handled, and both the FRR command and verification logic are correctly updated to support VRF contexts.
frr/rt_grout.c (8)
201-202: Direct interface ID mapping and VRF propagation are correct.The removal of interface index offset and addition of VRF ID assignment properly supports VRF-aware nexthop handling.
242-244: VRF-to-table mapping is clearly documented.The comment explaining that Grout lacks per-VRF routing tables and uses VRF ID as table ID provides important architectural context. This is consistent with the VRF initialization in
zebra_dplane_grout.c.
329-329: VRF ID propagation to route entry is correct.The use of
vrf_idinstead of a hardcoded value enables proper VRF-aware route management.
347-347: VRF-aware route deletion is properly implemented.The inclusion of
vrf_idin therib_deletecall ensures routes are deleted from the correct VRF context.
435-454: Enhanced SRv6 route logging with VRF context.The addition of VRF ID to debug messages improves troubleshooting capabilities for VRF-aware SRv6 routes.
569-581: VRF support for SRv6 local behaviors is properly implemented.Setting
out_vrf_idfrom the context's table field enables VRF-aware SRv6 behaviors for END_T, END_DT6, END_DT4, and END_DT46.
622-746: Comprehensive VRF support in route management.The changes consistently propagate VRF ID throughout all route operations:
- Proper extraction from dataplane context
- Inclusion in all route add/delete requests
- Enhanced debug logging with VRF context
This enables full VRF-aware route management.
829-829: Direct interface ID assignment for nexthops.The removal of interface index offset in nexthop handling is consistent with the overall VRF-aware design.
maxime-leroy
force-pushed
the
vrr_support_for_frr
branch
from
695dd34 to
8897adb
Compare
maxime-leroy
force-pushed
the
vrr_support_for_frr
branch
from
8897adb to
6a57a3b
Compare
maxime-leroy
force-pushed
the
vrr_support_for_frr
branch
from
6a57a3b to
a739736
Compare
maxime-leroy
force-pushed
the
vrr_support_for_frr
branch
from
a739736 to
0598039
Compare
maxime-leroy
force-pushed
the
vrr_support_for_frr
branch
from
0598039 to
e3d60f5
Compare
maxime-leroy
force-pushed
the
vrr_support_for_frr
branch
from
e3d60f5 to
1c956c1
Compare
| } | ||
| if (base.vrf_id >= MAX_VRFS) | ||
| return api_out(EOVERFLOW, 0); | ||
|
|
||
| if (iface_from_id(base.iface_id) == NULL) | ||
| iface = iface_from_id(base.iface_id); | ||
| if (iface == NULL) |
There was a problem hiding this comment.
Dereference of iface without handling NULL: missing return or error path before using iface->vrf_id.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
smoke/cross_vrf_forward_test.sh (2)
5-5: Fix shellcheck warning: Quote the dirname command substitution.
37-40: Add error handling for ping commands.smoke/vrf_forward_frr_test.sh (2)
5-5: Fix shellcheck warning: Quote the dirname command substitution.
49-52: Add error handling for ping commands.smoke/cross_vrf_forward_frr_test.sh (2)
5-5: Fix shellcheck warning: Quote the dirname command substitution.
38-41: Add error handling for ping commands.smoke/_init_frr.sh (1)
95-95: Fix shellcheck warning: Separate variable declaration and assignment.
🧹 Nitpick comments (1)
smoke/_init_frr.sh (1)
79-79: Fix shellcheck warning: Separate variable declaration and assignment.- local vrf_name="$(vrf_name_from_id "$vrf_id")" + local vrf_name + vrf_name="$(vrf_name_from_id "$vrf_id")"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
frr/if_grout.c(2 hunks)frr/rt_grout.c(11 hunks)frr/zebra_dplane_grout.c(2 hunks)modules/infra/control/iface.c(7 hunks)modules/infra/control/vrf.c(2 hunks)modules/infra/control/vrf_priv.h(1 hunks)modules/infra/datapath/loop_xvrf.c(1 hunks)modules/infra/datapath/meson.build(1 hunks)modules/ip/api/gr_ip4.h(1 hunks)modules/ip/cli/route.c(3 hunks)modules/ip/control/route.c(4 hunks)modules/ip6/api/gr_ip6.h(1 hunks)modules/ip6/cli/route.c(3 hunks)modules/ip6/control/route.c(4 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(1 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- modules/infra/control/vrf_priv.h
- modules/ip/control/route.c
- frr/zebra_dplane_grout.c
- modules/ip/cli/route.c
- modules/ip6/api/gr_ip6.h
- modules/infra/datapath/meson.build
- modules/ip/api/gr_ip4.h
- modules/ip6/control/route.c
- smoke/vrf_forward_test.sh
- modules/infra/control/vrf.c
- modules/infra/control/iface.c
- modules/ip6/cli/route.c
- frr/if_grout.c
- modules/infra/datapath/loop_xvrf.c
🧰 Additional context used
🧬 Code Graph Analysis (3)
frr/rt_grout.c (1)
modules/infra/api/gr_nexthop.h (1)
gr_nh_origin_name(156-224)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (5)
create_interface(9-29)set_ip_address(31-65)set_vrf_iface(77-86)set_ip_route(88-130)vrf_name_from_id(67-75)smoke/_init.sh (1)
netns_add(64-70)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(9-29)set_ip_address(31-65)set_ip_route(88-130)smoke/_init.sh (1)
netns_add(64-70)
🪛 Shellcheck (0.10.0)
smoke/_init_frr.sh
[warning] 79-79: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 95-95: Declare and assign separately to avoid masking return values.
(SC2155)
smoke/cross_vrf_forward_frr_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 7-7: run_id is referenced but not assigned.
(SC2154)
smoke/cross_vrf_forward_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 7-7: run_id is referenced but not assigned.
(SC2154)
smoke/vrf_forward_frr_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 7-7: run_id is referenced but not assigned.
(SC2154)
🪛 Gitleaks (8.27.2)
frr/rt_grout.c
449-449: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
🔇 Additional comments (21)
smoke/cross_vrf_forward_test.sh (3)
10-14: LGTM - VRF interface configuration.Proper setup of interfaces in different VRFs with distinct MAC addresses for cross-VRF forwarding test.
15-23: Well-designed asymmetric routing for cross-VRF testing.The routing configuration correctly implements different lookup paths: single lookup in one direction, double lookup in the other. Comments clearly explain the behavior.
24-35: LGTM - Network namespace setup.Proper isolation and configuration of test environments using namespaces.
smoke/vrf_forward_frr_test.sh (2)
12-31: LGTM - VRF-aware interface configuration.Proper use of enhanced helper functions to create interfaces in different VRFs and configure IP addresses through FRR.
33-47: LGTM - Namespace configuration for VRF 2.Proper setup of network namespaces with IP addresses and routing for VRF forwarding test.
smoke/cross_vrf_forward_frr_test.sh (1)
10-36: Excellent cross-VRF routing configuration.Advanced setup demonstrating VRF interface configuration and cross-VRF nexthop resolution. The asymmetric routing with
nexthop-vrfsupport showcases the enhanced VRF capabilities.smoke/_init_frr.sh (5)
12-15: LGTM - VRF support added to interface creation.Clean enhancement with backward compatibility. The VRF parameter defaults to 0 and is properly integrated into the grcli command.
34-34: LGTM - VRF support added to IP address configuration.Consistent enhancement adding VRF awareness to IP address assignment and verification.
Also applies to: 57-57
67-75: LGTM - Useful VRF naming helper.Clean function that maps VRF IDs to appropriate names with proper handling of default VRF.
91-92: Excellent cross-VRF routing support.Advanced enhancement supporting both VRF routing and cross-VRF nexthop resolution with proper FRR command construction.
Also applies to: 97-102
114-114: LGTM - VRF-aware route verification.Proper integration of VRF support in route verification with updated patterns and FRR commands.
Also applies to: 118-118, 122-122
frr/rt_grout.c (10)
246-248: LGTM - VRF restrictions removed.Proper extraction of VRF ID from nexthop enables VRF-aware routing. The comment clearly explains the table ID mapping.
299-299: LGTM - AF_UNSPEC nexthop support.Proper handling of unspecified address family aligns with kernel behavior for interface-only nexthops.
333-333: Check return value and approve VRF support.VRF ID properly passed to route entry creation. However, the return value should be checked for NULL as noted in past reviews.
351-351: LGTM - VRF support in route deletion.Consistent VRF parameter passing enables proper VRF-aware route deletion.
439-445: LGTM - Enhanced SRv6 logging with VRF context.Improved debugging with VRF ID included in route operation logs.
573-586: LGTM - VRF support in SRv6 local behaviors.Proper VRF ID assignment from context table for various SRv6 local behaviors.
626-626: LGTM - VRF context extraction.Proper extraction of VRF ID from dataplane context enables VRF-aware route processing.
679-681: LGTM - VRF ID in route requests.Proper VRF integration in route add/delete request structures.
705-712: LGTM - Comprehensive route logging.Excellent debug logging that includes VRF ID, origin, and nexthop ID for complete route operation context.
246-750: Excellent comprehensive VRF support implementation.The changes successfully remove VRF restrictions and enable full VRF-aware processing throughout nexthop and route handling. The implementation is consistent, well-integrated, and includes proper logging for debugging.
modules/infra/control/iface.c
Outdated
| ifid = conf->vrf_id; | ||
| if (reserve_ifid(ifid) < 0) | ||
| goto fail; | ||
| } else if (next_ifid(&ifid) < 0) | ||
| goto fail; | ||
| } else { | ||
| if (vrf_incref(conf->vrf_id) < 0) | ||
| goto fail; | ||
| vrf_ref = true; | ||
| iface->vrf_id = conf->vrf_id; | ||
|
|
||
| if (next_ifid(&ifid) < 0) | ||
| goto fail; | ||
| } |
There was a problem hiding this comment.
This is not correct. You may hit this code branch with conf->type == GR_IFACE_TYPE_LOOPBACK && conf->vrf_id == 0. This should be changed to:
if (conf->type != GR_IFACE_TYPE_LOOPBACK) {
if (vrf_incref(conf->vrf_id) < 0)
goto fail;
vrf_ref = true;
}
if (conf->type == GR_IFACE_TYPE_LOOPBACK && conf->vrf_id) {
ifid = conf->vrf_id;
if (reserve_ifid(ifid) < 0)
goto fail;
} else if (next_ifid(&ifid) < 0)
goto fail;
In FRR, a VRF must be created before any interface is attached to it. Grout uses POST_ADD events to create VRFs when interfaces reference them. This means the VRF is only created *after* the interface is processed, violating FRR’s requirement. Unlike the kernel, which uses an explicit API to create VRFs, Grout created them lazily. This patch moves VRF creation/deletion directly into iface_create/delete to ensure correct ordering. Signed-off-by: Maxime Leroy <[email protected]>
Kernel VRF ---------- - In Linux a VRF is represented by a net device; its **ifindex** is the VRF’s unique ID. - The device also stores the VRF’s routing‑table number. - FRR treats the kernel’s default namespace as “VRF 0” and, by default, uses routing table 254 (`rt_table_main_id`). Grout VRF --------- - Grout has no VRF device, only a numeric vrf_id (0–255). - Every VRF already owns a loopback interface gr‑loopX. - Grout maintains a single FIB per VRF; it has no separate “table” object. VRF support for zebra plugin ---------------------------- To map FRR/kernel’s VRF model onto Grout: 1. gr-loopbackX as VRF interface Zebra now advertises each gr-loopbackX as `ZEBRA_IF_VRF` (was `ZEBRA_IF_OTHER`), making it the functional equivalent of a Linux VRF‑master. 2. Stable IDs A previous patch enforces the invariant `ifindex(gr-loopbackX) == vrf_id`. FRR registers a VRF under the ifindex of its VRF interface, and Grout’s dataplane APIs expect a vrf_id. Thanks to the invariant, `dplane_ctx_get_vrf()` in `grout_add_del_route()` can pass the value it retrieves—the ifindex of gr-loopbackX—directly to Grout with no conversion. 3. Table ID == vrf_id Because Grout lacks per‑VRF tables, we set the routing table ID to vrf_id when syncing gr-loopbackX. This keeps the identity `ifindex == vrf_id == table_id` and allows removal of the SRv6 local table‑to‑VRF conversion code. 4. Default VRF (vrf 0) We force `rt_table_main_id = 0`, preserving the same `vrf_id == table_id` rule. gr-loopback0 is not synchronize as VRF interface. There is no VRF interface for default in kernel. Signed-off-by: Maxime Leroy <[email protected]>
Add some explicit route to *vrf_forward_test*, matching what ip_forward_test.sh already does. The next commit will clone this test for FRR: it will create the route from vtysh and verify that it is correctly synchronised to Grout. Signed-off-by: Maxime Leroy <[email protected]>
This new smoke test duplicates *vrf_forward_test* but drives the setup through FRR (via *vtysh*) instead of iproute2. Its primary purpose is to prove that FRR’s VRF plumbing—interface registration, VRF creation, and route installation—works end‑to‑end with Grout. Signed-off-by: Maxime Leroy <[email protected]>
Enable creation of routes specifying both a VRF and a nexthop IP, allowing basic cross-VRF routing scenarios. Applies to both IPv4 and IPv6 route handling in CLI and control path. Signed-off-by: Maxime Leroy <[email protected]>
Introduce two tests for cross-VRF forwarding using nexthop IP: one using gr-cli and another using vtysh to configure the routes. Signed-off-by: Maxime Leroy <[email protected]>
It allows packets to be routed from one VRF into another. Signed-off-by: Maxime Leroy <[email protected]>
maxime-leroy
force-pushed
the
vrr_support_for_frr
branch
from
e7cf2ea to
6b9ba4e
Compare
| for (uint16_t i = 0; i < nb_objs; i++) { | ||
| m = objs[i]; | ||
|
|
||
| if (m->packet_type & RTE_PTYPE_L3_IPV4) { |
There was a problem hiding this comment.
Fallback treats any non-IPv4 packet as IPv6; add explicit check for RTE_PTYPE_L3_IPV6 to avoid mis-processing non-IP packets and potential invalid mbuf data access.
|
|
||
| if (vrf_is_backend_netns()) { | ||
| gr_log_err("vrf backend netns is not supported with grout"); | ||
| exit(1); // Exit because zebra_dplane_start() does not check the return value |
There was a problem hiding this comment.
Calling exit(1) in library code will bypass upper-layer cleanup; consider returning an error instead of exiting to allow proper teardown.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
smoke/cross_vrf_forward_test.sh (1)
37-40: Add exit-status checks for ping commands
Same remark as in the previous review: pings should fail the test when connectivity is broken. Please append|| exit 1(or similar handling) after eachping.smoke/vrf_forward_frr_test.sh (2)
5-5: Quotedirnameexpansion – see earlier comment.
49-52: Ping results should gate test success
Persisting issue: exit if anypingfails to ensure the smoke test actually asserts forwarding correctness.smoke/cross_vrf_forward_frr_test.sh (2)
5-5: Quotedirnameexpansion – already noted in earlier review.
38-41: Add failure handling topingstatements
Identical feedback as last round; append|| exit 1(or equivalent) so the script fails when connectivity is absent.smoke/_init_frr.sh (1)
79-79: Separate declaration and assignment to appease ShellCheckLines 79 and 95 still trigger SC2155. Split into two lines:
- local vrf_name="$(vrf_name_from_id "$vrf_id")" + local vrf_name + vrf_name="$(vrf_name_from_id "$vrf_id")"Also applies to: 95-95
🧹 Nitpick comments (2)
smoke/cross_vrf_forward_test.sh (1)
5-5: Quotedirnameexpansion to avoid word-splitting
ShellCheck SC2046 is still reported here. Use double quotes around both the command substitution and the argument it produces.-. $(dirname $0)/_init.sh +. "$(dirname "$0")/_init.sh"smoke/_init_frr.sh (1)
6-6: Quotedirnameexpansion to silence SC2046-. $(dirname $0)/_init.sh +. "$(dirname "$0")/_init.sh"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/graph.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
frr/if_grout.c(2 hunks)frr/rt_grout.c(11 hunks)frr/zebra_dplane_grout.c(2 hunks)modules/infra/control/iface.c(7 hunks)modules/infra/control/vrf.c(2 hunks)modules/infra/control/vrf_priv.h(1 hunks)modules/infra/datapath/loop_xvrf.c(1 hunks)modules/infra/datapath/meson.build(1 hunks)modules/ip/api/gr_ip4.h(1 hunks)modules/ip/cli/route.c(3 hunks)modules/ip/control/route.c(4 hunks)modules/ip6/api/gr_ip6.h(1 hunks)modules/ip6/cli/route.c(3 hunks)modules/ip6/control/route.c(4 hunks)smoke/_init_frr.sh(5 hunks)smoke/cross_vrf_forward_frr_test.sh(1 hunks)smoke/cross_vrf_forward_test.sh(1 hunks)smoke/vrf_forward_frr_test.sh(1 hunks)smoke/vrf_forward_test.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- modules/ip6/cli/route.c
- frr/zebra_dplane_grout.c
- modules/infra/datapath/meson.build
- modules/ip/control/route.c
- frr/if_grout.c
- modules/ip/api/gr_ip4.h
- modules/ip6/api/gr_ip6.h
- modules/infra/control/iface.c
- modules/infra/control/vrf_priv.h
- modules/ip6/control/route.c
- smoke/vrf_forward_test.sh
- modules/ip/cli/route.c
- modules/infra/control/vrf.c
- modules/infra/datapath/loop_xvrf.c
- frr/rt_grout.c
🧰 Additional context used
🧬 Code Graph Analysis (2)
smoke/cross_vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (5)
create_interface(9-29)set_ip_address(31-65)set_vrf_iface(77-86)set_ip_route(88-130)vrf_name_from_id(67-75)smoke/_init.sh (1)
netns_add(64-70)
smoke/vrf_forward_frr_test.sh (2)
smoke/_init_frr.sh (3)
create_interface(9-29)set_ip_address(31-65)set_ip_route(88-130)smoke/_init.sh (1)
netns_add(64-70)
🪛 Shellcheck (0.10.0)
smoke/_init_frr.sh
[warning] 79-79: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 95-95: Declare and assign separately to avoid masking return values.
(SC2155)
smoke/cross_vrf_forward_frr_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 7-7: run_id is referenced but not assigned.
(SC2154)
smoke/cross_vrf_forward_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 7-7: run_id is referenced but not assigned.
(SC2154)
smoke/vrf_forward_frr_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 7-7: run_id is referenced but not assigned.
(SC2154)
⏰ 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). (6)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04)
- GitHub Check: build-cross-aarch64
🔇 Additional comments (3)
smoke/cross_vrf_forward_test.sh (1)
7-9: Ensurerun_idis set before use
Static analysis flagsrun_idas unassigned. If_init.shis expected to export it, assert this early to fail fast when it isn’t:: "${run_id:?run_id not initialised – did _init.sh run successfully?}"smoke/vrf_forward_frr_test.sh (1)
7-10: Verify thatrun_idis predefined
run_idcomes from the init script; add a sanity check (see previous suggestion) or export it explicitly in_init_frr.shto silence SC2154.smoke/cross_vrf_forward_frr_test.sh (1)
7-9: Confirmrun_idavailability
Insert the same assertion pattern to guaranteerun_idis set before it is expanded.
3 participants
Add VRF support for FRR and also disable interface/vrf sync from kernel into frr.
Note: Cross-VRF routing is still not supported.
Summary by Sourcery
Add VRF support by carrying vrf_id in nexthop, route, and interface code paths; introduce a kernel‐based VRF interface type and datapath, replace loopback modules, and provide a simple id pool for nexthop allocations; disable redundant kernel→FRR interface/VRF synchronization and update tests accordingly.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
Refactor