Conversation
Add trace output showing source and destination ports for UDP and TCP packets in l4_input_local. Also parse TCP headers to extract ports before falling through to management plane forwarding. Signed-off-by: Robin Jarry <rjarry@redhat.com>
VRF interfaces now get a random MAC address at creation time. This MAC will be used as the Router MAC for EVPN L3VNI type-5 routes. Signed-off-by: Robin Jarry <rjarry@redhat.com>
VXLAN interfaces now inherit from their VTEP VRF (encap_vrf_id) MAC address instead of a random one. Some EVPN endpoints make the assumption that there is a unique RMAC per VTEP. When a VXLAN interface is in VRF mode (or moves to one), also synchronize that MAC address to the VRF interface. This ensures the VXLAN interface's MAC matches the Router MAC that FRR advertises in EVPN type-5 routes, so incoming L3VPN packets pass eth_input's check. Signed-off-by: Robin Jarry <rjarry@redhat.com>
All GR_MBUF_PRIV_DATA_TYPE types overlay the same memory region. When ip_output resolves a nexthop on a VXLAN interface, it writes eth_output_mbuf_data->dst and ->ether_type which clobber iface_mbuf_data->vtep. By the time vxlan_output reads the vtep field, it contains garbage. Add a vtep field to eth_output_mbuf_data and set it from the nexthop gateway IP when the output interface is a VXLAN. In eth_output, save the vtep value before writing iface_mbuf_data (which shares the same memory) and restore it after. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Add a flag to mark nexthops learned from remote VTEPs via EVPN. These nexthops carry a known IP+MAC pair from the control plane and are set to GR_NH_S_REACHABLE with GR_NH_F_STATIC so they bypass ARP/ND probing and aging. Signed-off-by: Robin Jarry <rjarry@redhat.com>
EVPN type-5 routes require two pieces of state that FRR delivers out of order: the remote router MAC arrives via DPLANE_OP_NEIGH_INSTALL before the nexthop via DPLANE_OP_NH_INSTALL. Both need to be combined when grout_add_nexthop builds the GR_NH_ADD request. Add l3vni_map to maintain two hash tables on the dplane thread: vrf_id -> vxlan_iface_id: used to redirect nexthops from the VRF interface (FRR's SVI model) to the VXLAN interface so that ip_output routes packets into the tunnel. (vrf_id, vtep) -> RMAC: caches the remote router MAC until the matching nexthop install arrives. Both tables run exclusively on the dplane thread so no locking is needed. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Handle EVPN type-5 (IP prefix) routes with symmetric IRB over VXLAN. Present VRF-mode VXLAN interfaces to FRR as bridge slaves of the VRF interface. FRR requires an SVI derived from a bridge master to bring the L3VNI up and compute the Router MAC for type-5 routes. The VRF MAC (set in a previous commit) serves as the Router MAC. Handle DPLANE_OP_NEIGH_INSTALL/DELETE to cache remote router MACs delivered by FRR before the corresponding nexthop install arrives. When grout_add_nexthop processes an L3 nexthop in a VRF with an L3VNI, it redirects the interface from VRF to VXLAN and applies the cached RMAC. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Verify EVPN type-5 IP prefix route exchange and L3 connectivity over VXLAN between FRR+grout and a standalone FRR+Linux peer. Each side has a VRF with an L3 VNI (1000) and a host on a local subnet. BGP EVPN advertises connected prefixes as type-5 routes. Grout uses a VXLAN in VRF mode (no bridge needed). The peer uses the standard Linux bridge+SVI model. The test verifies L3VNI recognition, type-5 route exchange, route installation in the VRF, RMAC presence on the route nexthop, and end-to-end ping through the overlay. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Add a per-interface flag to control neighbor snooping. Configurable via grcli: interface set port p0 neigh_snoop on/off. When GR_IFACE_F_NEIGH_SNOOP is enabled on an interface, forward unsolicited ARP replies and Neighbor Advertisements to the control plane instead of dropping them. The existing arp_probe_input_cb and ndp_probe_input_cb callbacks already handle creating nexthops for unknown senders, so no control plane changes are needed. This allows grout to passively learn IP+MAC bindings from ARP/NDP traffic observed on an interface, similar to Linux's arp_accept sysctl. Signed-off-by: Robin Jarry <rjarry@redhat.com>
ARP/NDP-learned nexthops were created with GR_NH_ORIGIN_INTERNAL which suppresses event generation. Add GR_NH_ORIGIN_NEIGH so these nexthops produce GR_EVENT_NEXTHOP_NEW and GR_EVENT_NEXTHOP_UPDATE events that the FRR plugin can receive and translate into neighbor entries for zebra. The associated host routes (/32, /128) remain GR_NH_ORIGIN_INTERNAL since they are internal plumbing and should not be visible to FRR. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Add GR_NH_FLUSH API request and grcli command to delete all nexthops matching a given origin. This is useful for clearing ARP/NDP-learned neighbors (origin neigh) which don't have IDs. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Change GR_NH_DEL to accept a full struct gr_nexthop instead of just an ID. The handler uses nexthop_lookup() which first tries the ID if specified or falls back to the type-specific key-based lookup (e.g. vrf+addr for L3). This will allow the FRR plugin to delete remote nexthops installed from EVPN neighbor entries without tracking their auto-allocated IDs. Signed-off-by: Robin Jarry <rjarry@redhat.com>
When a grout nexthop event arrives for an L3 nexthop with a resolved MAC, push a DPLANE_OP_NEIGH_INSTALL to zebra. On deletion, push DPLANE_OP_NEIGH_DELETE. This makes FRR aware of locally learned neighbors so it can advertise them as Type-2 MAC+IP routes via BGP EVPN. The notification is called before the nh_id check in grout_nexthop_change so that nexthops with nh_id=0 (auto-allocated by the ARP/NDP control plane) still produce neighbor entries in zebra even though they are not registered as zebra nexthop groups. Signed-off-by: Robin Jarry <rjarry@redhat.com>
When the dplane plugin receives DPLANE_OP_NEIGH_INSTALL for a remote MAC/IP (from EVPN Type-2 routes), install the entry as a grout nexthop with GR_NH_F_REMOTE. The iface_id is resolved from the FRR ifindex which points to the bridge (SVI) interface. This allows the bridge suppress node to look up the nexthop and answer ARP/ND requests locally. Both IPv4 and IPv6 neighbors are supported. The existing L3VNI RMAC caching for Type-5 routes is preserved alongside. Signed-off-by: Robin Jarry <rjarry@redhat.com>
📝 WalkthroughWalkthroughThis pull request adds EVPN L3VNI symmetric IRB support, including an in-memory VRF→VXLAN mapping and remote VTEP MAC cache, VRF MAC management synced with VXLAN interfaces, neighbor snooping and bridge-based neighbor suppression datapath, nexthop extensions for remote/neigh origins and flush operations, bridge flag semantics switched to positive logic, CLI/API additions for neigh_snoop and nexthop flush, datapath metadata VTEP propagation, and multiple smoke tests exercising EVPN L3VPN and neighbor suppression scenarios. 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: 5
🧹 Nitpick comments (1)
modules/l2/cli/bridge.c (1)
87-93: Minor inconsistency in CLI syntax forneigh_suppress.The
neigh_suppressflag usesneigh_suppress/no_neigh_suppresstokens whilefloodandlearnuseon/offvalues. This inconsistency in the CLI interface isn't a bug, but the help text at lines 179-182 suggestson/offwhich doesn't match the actual token parsing here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/l2/cli/bridge.c` around lines 87 - 93, The CLI parsing for the neigh_suppress option is inconsistent with flood/learn: arg_str(...) currently checks literal tokens "neigh_suppress"/"no_neigh_suppress" while the help text and other options use on/off values; update the parsing in the function that reads args (the arg_str(...) checks around bridge->flags, GR_BRIDGE_F_NEIGH_SUPPRESS and set_attrs |= GR_BRIDGE_SET_FLAGS) to accept the same "on"/"off" style (or adjust the help text to match) so the token parsing and help text are consistent; modify the arg_* check to parse "on"/"off" for neigh_suppress and set/clear GR_BRIDGE_F_NEIGH_SUPPRESS and set_attrs accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frr/rt_grout.c`:
- Around line 676-681: The current code unconditionally retargets any L3 nexthop
in a VRF with an L3VNI to the VXLAN iface; change it to only rewrite when the
nexthop's original egress is the VRF/SVI interface. Concretely: call whatever
helper returns the VRF's SVI iface (e.g., vrf_get_svi_iface(req->nh.vrf_id) or
equivalent), then only assign req->nh.iface_id = vxlan_iface_id when
vxlan_iface_id != GR_IFACE_ID_UNDEF and req->nh.iface_id equals that VRF/SVI
iface id; leave other nexthops (static, self-originated, etc.) untouched. Ensure
you reference l3vni_get_vxlan(req->nh.vrf_id), req->nh.vrf_id, req->nh.iface_id
and GR_IFACE_ID_UNDEF in the change.
- Around line 1040-1045: The remote neighbor install/delete helpers are not
setting req->nh.vrf_id, causing lookups in bridge_neigh_suppress (which is
VRF-scoped) to miss entries keyed with GR_VRF_ID_UNDEF; set req->nh.vrf_id to
the correct VRF (the same VRF used for iface_id/neighbor) when building the key
(i.e., assign the VRF identifier into req->nh.vrf_id alongside
req->nh.type/origin/iface_id), and apply the identical fix in the other helper
block around the similar code at the referenced second site (lines shown
1083-1086) so both install and delete use the real VRF instead of leaving
GR_VRF_ID_UNDEF.
In `@modules/infra/cli/vrf.c`:
- Around line 85-86: Replace the incorrect VLAN flag with the VRF flag so the
MAC attr is actually applied: in the arg_eth_addr handling block where set_attrs
is updated (look for arg_eth_addr(p, "MAC", &info->mac) and the subsequent
set_attrs |= GR_VLAN_SET_MAC), change the flag to GR_VRF_SET_MAC so the daemon’s
VRF config check matches the set_attrs bit used when applying MACs.
In `@modules/infra/control/vrf.c`:
- Line 154: The call to netlink_link_set_mac(vrf->vrf_ifindex, &vrf->mac)
ignores its return value causing kernel and in-memory MAC to diverge; change
this to check the return int, and on failure log a descriptive error including
vrf->vrf_ifindex and the target MAC (use processLogger or existing logging
facility) and propagate or handle the error (e.g., return an error code from the
surrounding function or rollback the vrf creation) so state stays consistent;
ensure you reference netlink_link_set_mac, vrf->vrf_ifindex and vrf->mac when
implementing the check and error handling.
In `@modules/l2/control/vxlan.c`:
- Around line 155-163: The code reads conf->vrf_id even when iface_vxlan_init
passes a partially initialized conf, causing an indeterminate VRF; change the
logic in iface_vxlan_reconfig to avoid dereferencing conf->vrf_id unless the
caller explicitly provided VRF in set_attrs: compute VRF id from conf only when
(set_attrs & GR_IFACE_SET_VRF) is set, otherwise use iface->vrf_id (or skip
syncing), then call get_vrf_iface with that trusted VRF id and only call
iface_set_eth_addr(vrf, &mac) when vrf != NULL; update the branch around
get_vrf_iface/vrf to use this safe vrf_id selection so startup no longer reads
uninitialized conf->vrf_id.
---
Nitpick comments:
In `@modules/l2/cli/bridge.c`:
- Around line 87-93: The CLI parsing for the neigh_suppress option is
inconsistent with flood/learn: arg_str(...) currently checks literal tokens
"neigh_suppress"/"no_neigh_suppress" while the help text and other options use
on/off values; update the parsing in the function that reads args (the
arg_str(...) checks around bridge->flags, GR_BRIDGE_F_NEIGH_SUPPRESS and
set_attrs |= GR_BRIDGE_SET_FLAGS) to accept the same "on"/"off" style (or adjust
the help text to match) so the token parsing and help text are consistent;
modify the arg_* check to parse "on"/"off" for neigh_suppress and set/clear
GR_BRIDGE_F_NEIGH_SUPPRESS and set_attrs accordingly.
🪄 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: 5c7bdb9d-5f15-48d9-a7e6-2f52cad4c334
📒 Files selected for processing (38)
.clang-formatapi/gr_macro.hfrr/if_grout.cfrr/l3vni_map.cfrr/l3vni_map.hfrr/meson.buildfrr/rt_grout.cfrr/rt_grout.hfrr/zebra_dplane_grout.cmodules/infra/api/gr_infra.hmodules/infra/api/gr_nexthop.hmodules/infra/api/nexthop.cmodules/infra/cli/cli_iface.hmodules/infra/cli/iface.cmodules/infra/cli/nexthop.cmodules/infra/cli/vrf.cmodules/infra/control/nexthop.cmodules/infra/control/vrf.cmodules/infra/datapath/eth.hmodules/infra/datapath/eth_output.cmodules/ip/control/nexthop.cmodules/ip/datapath/arp_input_reply.cmodules/ip/datapath/ip_output.cmodules/ip6/control/address.cmodules/ip6/control/nexthop.cmodules/ip6/datapath/ip6_output.cmodules/ip6/datapath/ndp_na_input.cmodules/l2/api/gr_l2.hmodules/l2/cli/bridge.cmodules/l2/cli/fdb.cmodules/l2/control/vxlan.cmodules/l2/datapath/bridge_input.cmodules/l2/datapath/bridge_neigh_suppress.cmodules/l2/datapath/meson.buildmodules/l4/l4_input_local.csmoke/bridge_neigh_suppress_test.shsmoke/evpn_l3vpn_frr_test.shsmoke/evpn_neigh_suppress_frr_test.sh
Comment on lines
+676
to
+681
| // For L3 nexthops in VRFs with an L3VNI, redirect the iface from | ||
| // the VRF (SVI in FRR's model) to the VXLAN interface. Grout | ||
| // routes packets directly through the VXLAN tunnel. | ||
| vxlan_iface_id = l3vni_get_vxlan(req->nh.vrf_id); | ||
| if (vxlan_iface_id != GR_IFACE_ID_UNDEF) | ||
| req->nh.iface_id = vxlan_iface_id; |
There was a problem hiding this comment.
Don't retarget every L3 nexthop in an L3VNI VRF to the VXLAN iface.
This is keyed only on the VRF having an L3VNI, so any self-originated L3 route in that VRF—static routes included—gets forced onto the tunnel. Restrict the rewrite to nexthops whose original FRR egress is the VRF/SVI interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frr/rt_grout.c` around lines 676 - 681, The current code unconditionally
retargets any L3 nexthop in a VRF with an L3VNI to the VXLAN iface; change it to
only rewrite when the nexthop's original egress is the VRF/SVI interface.
Concretely: call whatever helper returns the VRF's SVI iface (e.g.,
vrf_get_svi_iface(req->nh.vrf_id) or equivalent), then only assign
req->nh.iface_id = vxlan_iface_id when vxlan_iface_id != GR_IFACE_ID_UNDEF and
req->nh.iface_id equals that VRF/SVI iface id; leave other nexthops (static,
self-originated, etc.) untouched. Ensure you reference
l3vni_get_vxlan(req->nh.vrf_id), req->nh.vrf_id, req->nh.iface_id and
GR_IFACE_ID_UNDEF in the change.
Comment on lines
+1040
to
+1045
| req->exist_ok = true; | ||
| req->nh.type = GR_NH_T_L3; | ||
| req->nh.origin = GR_NH_ORIGIN_BGP; | ||
| req->nh.iface_id = iface_id; | ||
| l3 = (struct gr_nexthop_info_l3 *)req->nh.info; | ||
| l3->flags = GR_NH_F_REMOTE; |
There was a problem hiding this comment.
Carry vrf_id in the key-based remote neighbor install/delete.
Both helpers leave req->nh.vrf_id unset. bridge_neigh_suppress does VRF-scoped neighbor lookups, so installs keyed with GR_VRF_ID_UNDEF won't be found for the bridge's actual VRF, and the delete helper repeats the same wrong key.
Possible fix
req->exist_ok = true;
req->nh.type = GR_NH_T_L3;
req->nh.origin = GR_NH_ORIGIN_BGP;
req->nh.iface_id = iface_id;
+ req->nh.vrf_id = vrf_frr_to_grout(dplane_ctx_get_vrf(ctx));
l3 = (struct gr_nexthop_info_l3 *)req->nh.info;
@@
req->missing_ok = true;
req->nh.type = GR_NH_T_L3;
req->nh.iface_id = iface_id;
+ req->nh.vrf_id = vrf_frr_to_grout(dplane_ctx_get_vrf(ctx));
l3 = (struct gr_nexthop_info_l3 *)req->nh.info;Also applies to: 1083-1086
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frr/rt_grout.c` around lines 1040 - 1045, The remote neighbor install/delete
helpers are not setting req->nh.vrf_id, causing lookups in bridge_neigh_suppress
(which is VRF-scoped) to miss entries keyed with GR_VRF_ID_UNDEF; set
req->nh.vrf_id to the correct VRF (the same VRF used for iface_id/neighbor) when
building the key (i.e., assign the VRF identifier into req->nh.vrf_id alongside
req->nh.type/origin/iface_id), and apply the identical fix in the other helper
block around the similar code at the referenced second site (lines shown
1083-1086) so both install and delete use the real VRF instead of leaving
GR_VRF_ID_UNDEF.
Comment on lines
+85
to
+86
| if (arg_eth_addr(p, "MAC", &info->mac) == 0) | ||
| set_attrs |= GR_VLAN_SET_MAC; |
There was a problem hiding this comment.
Wrong flag constant: GR_VLAN_SET_MAC should be GR_VRF_SET_MAC.
This uses the VLAN MAC flag (GR_BIT64(34)) instead of the VRF MAC flag (GR_BIT64(33)). The daemon checks for GR_VRF_SET_MAC when applying VRF configuration, so the MAC will never be applied.
🐛 Proposed fix
if (arg_eth_addr(p, "MAC", &info->mac) == 0)
- set_attrs |= GR_VLAN_SET_MAC;
+ set_attrs |= GR_VRF_SET_MAC;
else if (errno != ENOENT)
return 0;📝 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.
Suggested change
| if (arg_eth_addr(p, "MAC", &info->mac) == 0) | |
| set_attrs |= GR_VLAN_SET_MAC; | |
| if (arg_eth_addr(p, "MAC", &info->mac) == 0) | |
| set_attrs |= GR_VRF_SET_MAC; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/infra/cli/vrf.c` around lines 85 - 86, Replace the incorrect VLAN
flag with the VRF flag so the MAC attr is actually applied: in the arg_eth_addr
handling block where set_attrs is updated (look for arg_eth_addr(p, "MAC",
&info->mac) and the subsequent set_attrs |= GR_VLAN_SET_MAC), change the flag to
GR_VRF_SET_MAC so the daemon’s VRF config check matches the set_attrs bit used
when applying MACs.
| strerror(errno)); | ||
| return ret; | ||
| } | ||
| netlink_link_set_mac(vrf->vrf_ifindex, &vrf->mac); |
There was a problem hiding this comment.
Unchecked return value from netlink_link_set_mac.
If setting the MAC on the kernel VRF device fails, grout will have a different MAC stored than what the kernel has, potentially causing ARP/routing inconsistencies.
Proposed fix
- netlink_link_set_mac(vrf->vrf_ifindex, &vrf->mac);
+ if (netlink_link_set_mac(vrf->vrf_ifindex, &vrf->mac) < 0) {
+ LOG(WARNING, "failed to set MAC on VRF %s: %s", iface->name, strerror(errno));
+ }📝 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.
Suggested change
| netlink_link_set_mac(vrf->vrf_ifindex, &vrf->mac); | |
| if (netlink_link_set_mac(vrf->vrf_ifindex, &vrf->mac) < 0) { | |
| LOG(WARNING, "failed to set MAC on VRF: %s", strerror(errno)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/infra/control/vrf.c` at line 154, The call to
netlink_link_set_mac(vrf->vrf_ifindex, &vrf->mac) ignores its return value
causing kernel and in-memory MAC to diverge; change this to check the return
int, and on failure log a descriptive error including vrf->vrf_ifindex and the
target MAC (use processLogger or existing logging facility) and propagate or
handle the error (e.g., return an error code from the surrounding function or
rollback the vrf creation) so state stays consistent; ensure you reference
netlink_link_set_mac, vrf->vrf_ifindex and vrf->mac when implementing the check
and error handling.
Comment on lines
+155
to
+163
| // If configured for EVPN L3VNI, also synchronize the MAC on the interface VRF. | ||
| // So it will be advertised as RMAC by FRR. | ||
| vrf = NULL; | ||
| if (set_attrs & GR_IFACE_SET_VRF) | ||
| vrf = get_vrf_iface(conf->vrf_id); | ||
| else if (iface->mode == GR_IFACE_MODE_VRF) | ||
| vrf = get_vrf_iface(iface->vrf_id); | ||
| if (vrf != NULL && iface_set_eth_addr(vrf, &mac) < 0) | ||
| goto err; |
There was a problem hiding this comment.
Avoid reading conf->vrf_id from the partially initialized init-time config.
iface_vxlan_init() still builds a stack struct gr_iface conf and only fills conf.base before calling iface_vxlan_reconfig(IFACE_SET_ALL, ...). This new branch now dereferences conf->vrf_id, so startup can sync the MAC into an indeterminate VRF.
Possible fix
-static int iface_vxlan_init(struct iface *iface, const void *api_info) {
- struct gr_iface conf;
+static int iface_vxlan_init(struct iface *iface, const void *api_info) {
+ struct gr_iface conf = {0};
int ret;
@@
- conf.base = iface->base;
+ conf.base = iface->base;
+ conf.vrf_id = iface->vrf_id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/l2/control/vxlan.c` around lines 155 - 163, The code reads
conf->vrf_id even when iface_vxlan_init passes a partially initialized conf,
causing an indeterminate VRF; change the logic in iface_vxlan_reconfig to avoid
dereferencing conf->vrf_id unless the caller explicitly provided VRF in
set_attrs: compute VRF id from conf only when (set_attrs & GR_IFACE_SET_VRF) is
set, otherwise use iface->vrf_id (or skip syncing), then call get_vrf_iface with
that trusted VRF id and only call iface_set_eth_addr(vrf, &mac) when vrf !=
NULL; update the branch around get_vrf_iface/vrf to use this safe vrf_id
selection so startup no longer reads uninitialized conf->vrf_id.
All interface/nexthop flags use "presence" semantics. Having flags to signify the absence of a feature is confusing. Change the semantics of bridge flags to follow the same style. Signed-off-by: Robin Jarry <rjarry@redhat.com>
This macro will be useful to format all flag masks. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Use gr_flags_foreach to simplify formatting them. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Add flag to allow enabling ARP/ND suppression on bridge interfaces. When a bridge has GR_BRIDGE_F_NEIGH_SUPPRESS enabled, broadcast and multicast traffic is routed through bridge_neigh_suppress before flooding. The node intercepts ARP requests and IPv6 Neighbor Solicitations, looks up the target IP in the bridge's VRF nexthop table, and replies on behalf of known remote hosts (GR_NH_F_REMOTE with a reachable MAC). For ARP, the Ethernet and ARP headers are rewritten in-place. For ND, the packet is rebuilt from scratch since a Neighbor Advertisement has a different structure than a Neighbor Solicitation. In both cases, the reply is sent directly to iface_output, bypassing eth_output to avoid double Ethernet header prepending. Packets that cannot be suppressed (unknown target, non-ARP/ND, or non-remote nexthop) fall through to bridge_flood as usual. Also add a "remote" flag to the nexthop add l3 CLI command so remote nexthops can be configured manually for testing. Signed-off-by: Robin Jarry <rjarry@redhat.com>
rjarry
force-pushed
the
neigh-suppression
branch
from
8f47020 to
fab7eb3
Compare
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/l2/cli/bridge.c`:
- Around line 87-94: The CLI parsing block uses arg_str(p, "neigh_suppress") but
the argument was registered as "NEIGH_SUPPRESS", so the lookup always fails;
update the arg_str call to use the exact registered name "NEIGH_SUPPRESS" (or
change the registration to "neigh_suppress" if you prefer lowercase) so the
value is retrieved correctly, then keep the existing flag handling using
GR_BRIDGE_F_NEIGH_SUPPRESS and set_attrs |= GR_BRIDGE_SET_FLAGS.
In `@smoke/bridge_neigh_suppress_test.sh`:
- Around line 25-26: The test invocation for the bridge neighbor suppression
flag is missing the required value argument; update the command "grcli interface
add bridge br0 neigh_suppress" to supply the "on" argument (i.e., use
"neigh_suppress on") so the CLI schema's on/off parameter is satisfied before
running the subsequent "grcli address add 10.0.0.1/24 iface br0" step.
🪄 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: d6310ae9-f7b5-4ae8-aaeb-5e05b9d73be6
📒 Files selected for processing (12)
.clang-formatapi/gr_macro.hmodules/infra/api/gr_nexthop.hmodules/infra/cli/nexthop.cmodules/l2/api/gr_l2.hmodules/l2/cli/bridge.cmodules/l2/cli/fdb.cmodules/l2/datapath/bridge_input.cmodules/l2/datapath/bridge_neigh_suppress.cmodules/l2/datapath/meson.buildsmoke/bridge_neigh_suppress_test.shsmoke/evpn_neigh_suppress_frr_test.sh
✅ Files skipped from review due to trivial changes (3)
- .clang-format
- modules/l2/cli/fdb.c
- modules/l2/datapath/meson.build
🚧 Files skipped from review as they are similar to previous changes (2)
- api/gr_macro.h
- modules/infra/cli/nexthop.c
Comment on lines
+87
to
94
| on_off = arg_str(p, "neigh_suppress"); | ||
| if (on_off != NULL && strcmp(on_off, "on") == 0) { | ||
| bridge->flags |= GR_BRIDGE_F_NEIGH_SUPPRESS; | ||
| set_attrs |= GR_BRIDGE_SET_FLAGS; | ||
| } else if (on_off != NULL && strcmp(on_off, "off") == 0) { | ||
| bridge->flags &= ~GR_BRIDGE_F_NEIGH_SUPPRESS; | ||
| set_attrs |= GR_BRIDGE_SET_FLAGS; | ||
| } |
There was a problem hiding this comment.
Bug: Argument name mismatch causes neigh_suppress parsing to fail.
The argument is registered as "NEIGH_SUPPRESS" (uppercase) in the schema at line 181, but arg_str() looks up "neigh_suppress" (lowercase). This will never match, silently ignoring the CLI option.
🐛 Proposed fix
- on_off = arg_str(p, "neigh_suppress");
+ on_off = arg_str(p, "NEIGH_SUPPRESS");📝 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.
Suggested change
| on_off = arg_str(p, "neigh_suppress"); | |
| if (on_off != NULL && strcmp(on_off, "on") == 0) { | |
| bridge->flags |= GR_BRIDGE_F_NEIGH_SUPPRESS; | |
| set_attrs |= GR_BRIDGE_SET_FLAGS; | |
| } else if (on_off != NULL && strcmp(on_off, "off") == 0) { | |
| bridge->flags &= ~GR_BRIDGE_F_NEIGH_SUPPRESS; | |
| set_attrs |= GR_BRIDGE_SET_FLAGS; | |
| } | |
| on_off = arg_str(p, "NEIGH_SUPPRESS"); | |
| if (on_off != NULL && strcmp(on_off, "on") == 0) { | |
| bridge->flags |= GR_BRIDGE_F_NEIGH_SUPPRESS; | |
| set_attrs |= GR_BRIDGE_SET_FLAGS; | |
| } else if (on_off != NULL && strcmp(on_off, "off") == 0) { | |
| bridge->flags &= ~GR_BRIDGE_F_NEIGH_SUPPRESS; | |
| set_attrs |= GR_BRIDGE_SET_FLAGS; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/l2/cli/bridge.c` around lines 87 - 94, The CLI parsing block uses
arg_str(p, "neigh_suppress") but the argument was registered as
"NEIGH_SUPPRESS", so the lookup always fails; update the arg_str call to use the
exact registered name "NEIGH_SUPPRESS" (or change the registration to
"neigh_suppress" if you prefer lowercase) so the value is retrieved correctly,
then keep the existing flag handling using GR_BRIDGE_F_NEIGH_SUPPRESS and
set_attrs |= GR_BRIDGE_SET_FLAGS.
smoke/bridge_neigh_suppress_test.sh
Outdated
Comment on lines
+25
to
+26
| grcli interface add bridge br0 neigh_suppress | ||
| grcli address add 10.0.0.1/24 iface br0 |
There was a problem hiding this comment.
Missing on argument for neigh_suppress.
The CLI schema (line 167 of bridge.c) defines neigh_suppress as taking an on/off value argument. Without it, the flag won't be enabled.
🐛 Proposed fix
-grcli interface add bridge br0 neigh_suppress
+grcli interface add bridge br0 neigh_suppress on📝 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.
Suggested change
| grcli interface add bridge br0 neigh_suppress | |
| grcli address add 10.0.0.1/24 iface br0 | |
| grcli interface add bridge br0 neigh_suppress on | |
| grcli address add 10.0.0.1/24 iface br0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@smoke/bridge_neigh_suppress_test.sh` around lines 25 - 26, The test
invocation for the bridge neighbor suppression flag is missing the required
value argument; update the command "grcli interface add bridge br0
neigh_suppress" to supply the "on" argument (i.e., use "neigh_suppress on") so
the CLI schema's on/off parameter is satisfied before running the subsequent
"grcli address add 10.0.0.1/24 iface br0" step.
bridge_neigh_suppress_test verifies suppression with a manually configured remote nexthop: an ARP request for a known remote IP gets answered locally by grout with the correct MAC, and is not flooded to other bridge ports. evpn_neigh_suppress_frr_test verifies end-to-end suppression with FRR EVPN. The Linux peer has neigh_suppress on the VXLAN interface and advertise-svi-ip in BGP. Both sides have bridge SVIs and hosts with default routes through them. When hosts ARP for their gateways, VTEPs learn the IP+MAC bindings and advertise Type-2 MAC+IP routes. The remote VTEP installs the nexthops and the suppress node answers ARPs locally. Signed-off-by: Robin Jarry <rjarry@redhat.com>
rjarry
force-pushed
the
neigh-suppression
branch
from
fab7eb3 to
96d65cc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #566
Neighbor suppression and EVPN L3VNI implementation
L2 neighbor suppression datapath
Neighbor snooping and learning
EVPN L3VNI support and RMAC caching
FRR integration and neighbor-driven nexthop lifecycle
Nexthop API and control-plane changes
Datapath metadata and VTEP propagation
L4 parsing and tracing
CLI and configuration
Tests