docs: add frr document and examples #295
Conversation
📝 WalkthroughWalkthroughAdded a new manpage source docs/grout-frr.7.md describing FRR and Linux networking steps to establish BGP peering between Grout and an upstream router without assigning an IP to gr-loop0. Document covers packet handling, rp_filter and sysctl settings, fwmark and policy routing, local routing table, udev automation for gr-loop0, FRR invocation (zebra with plugin) and sample bgpd config including ip-transparent and update-source. Updated docs/meson.build to include the new manpage in the build/install list. Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes 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
|
christophefontaine
force-pushed
the
docs
branch
from
374dc5a to
0f32b55
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
docs/frr.md (9)
3-4: Capitalize Linux and remove superfluous commaTighten grammar.
-between Grout and an upstream router, without assigning IP addresses directly to the linux interface. +between Grout and an upstream router without assigning IP addresses directly to the Linux interface.
7-11: Sync the mini-TOC with actual section titles and fix capitalizationMatch headings, capitalize Grout, and fix “IP”.
-0. Context -1. How packets are forwarded from Grout to gr-loop0 -2. Linux configuration to peer without adding a local ip -3. BGPD configuration +0. Ease of Use +1. Packets from Grout to gr-loop0 +2. Linux configuration to peer without adding a local IP +3. FRR configuration
15-21: Call out prerequisites (kernel support, FRR build, privileges) for IP_TRANSPARENTAdd a brief note so operators know what’s required.
This is achieved by utilizing the IP_TRANSPARENT socket option and FRR's capability to bind to a specific interface. +Note: This approach requires a Linux kernel with IP_TRANSPARENT support and an FRR build that exposes the ip-transparent knob. The bgpd process must have sufficient privileges (e.g., CAP_NET_ADMIN) to set the socket option.
26-29: Fix capitalization and minor wordingProject name is “Grout”; “VXLAN tunnel” not “Tunnel”.
-All packets received to a **local** IP configured in grout will be processed by the node "ip_input_local/ip6_input_local". +All packets received to a **local** IP configured in Grout will be processed by the node "ip_input_local/ip6_input_local". @@ -If still unhandled (e.g., VXLAN Tunnel), the packet is sent to the corresponding gr-loopX interface associated with the VRF. +If still unhandled (e.g., VXLAN tunnel), the packet is sent to the corresponding gr-loopX interface associated with the VRF.
59-61: Consider interface lifecycle and defaults for rp_filterIf gr-loop0 is recreated, ensure rp_filter stays disabled. If appropriate for your environment, consider documenting:
- net.ipv4.conf.default.rp_filter = 0 (careful: affects future interfaces)
- Or extend udev with a sysctl apply command on add.
67-71: Plugin path: prefer packaged/module path over a build-tree absolute pathUsing a build-tree path under /root is brittle. Consider documenting the installed module path (e.g., /usr/lib/frr/modules/frr_dplane_grout.so) or a distro-friendly location.
91-95: Grammar and clarity fixes; fix “non existant”Tighten the phrasing and fix typos.
-Three additional configuration for a neighbor is required: -- source interface, "gr-loop0", only required for the default VRF -- update source IP, local Grout IP -- ip-transparent, to allow bgpd to bind on the non existant IP. +Three additional neighbor configurations are required: +- Source interface (“gr-loop0”) — only required in the default VRF +- Update-source IP — the local Grout IP +- ip-transparent — allow bgpd to bind to the nonexistent IP
32-45: Optional: Add IPv6 variant for completenessIf you expect v6 peering, mirror the rules (ip6tables/nft) and mention ip -6 rule/route to table 42.
I can provide a short IPv6 block mirroring the v4 steps if useful.
24-31: Minor heading consistencyUse “Grout” capitalization in headings too for consistency.
-## 1. Packets from grout to gr-loop0 +## 1. Packets from Grout to gr-loop0
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/frr.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/frr.md
[grammar] ~3-~3: There might be a mistake here.
Context: ...inux networking to establish BGP peering between Grout and an upstream router, wi...
(QB_NEW_EN)
[grammar] ~7-~7: There might be a mistake here.
Context: ...tly to the linux interface. 0. Context 1. How packets are forwarded from Grout to ...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...ets are forwarded from Grout to gr-loop0 2. Linux configuration to peer without addi...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...onfiguration for a neighbor is required: - source interface, "gr-loop0", only requi...
(QB_NEW_EN)
⏰ 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). (1)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled)
🔇 Additional comments (2)
docs/frr.md (2)
99-112: Examples look good and helpfulThe BGP summary output is clear and matches the described setup.
115-132: Routing example is clearGood demonstration of BGP-learned routes and locally connected prefixes.
| To ensure all incoming packets from gr-loop0 are accepted by the network stack, execute the following commands: | ||
| ```sh | ||
| a) sysctl -w net.ipv4.conf.gr-loop0.rp_filter=0 | ||
| b) iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1 | ||
| c) ip rule add fwmark 0x1 lookup 42 | ||
| d) ip route add local default dev gr-loop0 table 42 | ||
| ``` | ||
| a) Disables Reverse Path Filtering | ||
| b) All packets received on the gr-loop0 interface will have a specific mark | ||
| c) based on this mark, lookup on a specific table | ||
| d) accept ALL packets as if the destination was local. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security hardening: mark only BGP traffic and set rule priority
Marking all packets on gr-loop0 and accepting “local default” makes the host accept any destination as local via that interface. Restrict the mark to TCP/179 (and optionally the neighbor IP) and make the rule priority explicit.
a) sysctl -w net.ipv4.conf.gr-loop0.rp_filter=0
- b) iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1
- c) ip rule add fwmark 0x1 lookup 42
+ b) iptables -t mangle -A PREROUTING -i gr-loop0 -p tcp --dport 179 -j MARK --set-mark 0x1
+ # Optionally restrict further with: -s <PEER_IP/32>
+ c) ip rule add fwmark 0x1 lookup 42 priority 42
d) ip route add local default dev gr-loop0 table 42
@@
-d) accept ALL packets as if the destination was local.
+d) accept marked BGP packets as if the destination was local.
+
+Security note: Avoid marking all traffic arriving on gr-loop0. Restrict to TCP/179 and, if possible, to the neighbor IP/subnet.
+
+Alternative (nftables) equivalent:
+```sh
+nft add table inet grout
+nft 'add chain inet grout prerouting { type filter hook prerouting priority mangle; }'
+nft 'add rule inet grout prerouting iifname "gr-loop0" tcp dport 179 meta mark set 0x1'
+```| # /etc/udev/rules.d/99-gr-loop0.rules | ||
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1" | ||
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route add local default dev gr-loop0 table 42" | ||
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule add fwmark 0x1 lookup 42 priority 42" | ||
|
|
||
| ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule del fwmark 0x1 lookup 42 priority 42" | ||
| ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i gr-loop0 -j MARK --set-mark 0x1" | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
udev cleanup on removal and minor idempotency/consistency fixes
- Remove the local route on device removal (currently leaked).
- Match the iptables restriction to TCP/179 here, too.
- Use route replace to be idempotent.
# /etc/udev/rules.d/99-gr-loop0.rules
-ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1"
-ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route add local default dev gr-loop0 table 42"
-ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule add fwmark 0x1 lookup 42 priority 42"
+ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -A PREROUTING -i gr-loop0 -p tcp --dport 179 -j MARK --set-mark 0x1"
+ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route replace local default dev gr-loop0 table 42"
+ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule add fwmark 0x1 lookup 42 priority 42"
ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule del fwmark 0x1 lookup 42 priority 42"
ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i gr-loop0 -j MARK --set-mark 0x1"
+ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route del table 42 local default dev gr-loop0"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/frr.md around lines 49 to 56, the udev rules currently leak the "ip
route add local default" on device removal, don't restrict the iptables rule to
TCP/179, and use non-idempotent "ip route add"; update the RUN lines so that on
ACTION=="add" you use "ip route replace local default dev gr-loop0 table 42"
(idempotent) and have the iptables PREROUTING rule limit to "-p tcp --dport 179"
(match iptables restriction), and on ACTION=="remove" delete the corresponding
route with "ip route del local default dev gr-loop0 table 42" in addition to
removing the ip rule and the iptables rule so the route is not leaked.
| neighbor 192.168.210.3 interface gr-loop0 | ||
| neighbor 192.168.210.3 update-source 192.168.210.1 | ||
| neighbor 192.168.210.3 ip-transparent | ||
| exit |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify FRR directive name and availability for ip-transparent
Double-check the exact CLI knob and minimum FRR version for “ip-transparent” to avoid config drift across releases.
🌐 Web query:
Is `ip-transparent` a valid FRR BGP neighbor configuration command, and from which FRR version is it available? Provide an example usage.
💡 Result:
Yes — ip-transparent is a valid bgpd neighbor command in FRR. It was merged to FRR upstream on May 28, 2025 and shipped in the FRR 10.4.0 release (released July 18, 2025). (github.com, frrouting.org)
What it does: it sets the underlying TCP socket to use IP_TRANSPARENT so bgpd can accept or initiate sessions to/from an IP address not configured on the host (useful for containers, VIP/VRRP handovers, transparent firewalls). The command is documented in the FRR BGP CLI reference. (docs.frrouting.org, github.com)
Example usage (IPv4):
router bgp 65001
neighbor 198.51.100.1 remote-as 65002
neighbor 198.51.100.1 update-source 203.0.113.10
neighbor 198.51.100.1 ip-transparent
Notes:
- The BGP TCP source address still needs to be set (for example with neighbor update-source A.B.C.D) because ip-transparent allows using an address you don't own but does not automatically choose a source address. (docs.frrouting.org)
If you want, I can also show an IPv6 example or a peer-group example.
Citations:
- 1: bgpd: add neighbor ip-transparent FRRouting/frr#18789
- 2: https://frrouting.org/release/10.4.0/?utm_source=chatgpt.com
- 3: https://docs.frrouting.org/en/latest/bgp.html?utm_source=chatgpt.com
- 4: bgpd: add neighbor ip-transparent FRRouting/frr#18789
- 5: https://docs.frrouting.org/en/latest/bgp.html?utm_source=chatgpt.com
Add FRR minimum-version note: ip-transparent requires FRR ≥ 10.4.0
Confirmed: ip-transparent is a valid bgpd neighbor command (merged 2025-05-28; shipped in FRR 10.4.0 — 2025-07-18). Example usage:
router bgp 65001
neighbor 198.51.100.1 remote-as 65002
neighbor 198.51.100.1 update-source 203.0.113.10
neighbor 198.51.100.1 ip-transparent
Note: ip-transparent sets IP_TRANSPARENT on the socket but you still must set the BGP TCP source (e.g., update-source).
Files to update:
- docs/frr.md — lines 83-86: add a short note like “Requires FRR >= 10.4.0 for
ip-transparent” next to the shown snippet.
🤖 Prompt for AI Agents
In docs/frr.md around lines 83 to 86, add a short note after the shown neighbor
snippet indicating that the ip-transparent neighbor option requires FRR version
10.4.0 or newer; update the text to read something like “Note: Requires FRR >=
10.4.0 for `ip-transparent`” placed near the snippet so readers see the
minimum-version requirement and keep the existing note about also setting the
BGP TCP source (e.g., `update-source`).
0f32b55 to
09b0843
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
docs/grout-frr.7.md (10)
6-8: Capitalize “Linux”Minor grammar fix to keep consistency with the rest of the docs.
Apply:
-This guide demonstrates how to configure FRR (Free Range Routing) and Linux networking to establish BGP peering -between Grout and an upstream router, without assigning IP addresses directly to the linux interface. +This guide demonstrates how to configure FRR (Free Range Routing) and Linux networking to establish BGP peering +between Grout and an upstream router, without assigning IP addresses directly to the Linux interface.
10-14: Fix ToC/title mismatches and capitalizationAlign the table of contents with the actual section titles and fix “IP” capitalization.
-0. Context -1. How packets are forwarded from Grout to gr-loop0 -2. Linux configuration to peer without adding a local ip -3. BGPD configuration +0. Ease of Use +1. Packets from Grout to gr-loop0 +2. Linux configuration to peer without adding a local IP +3. FRR Configuration Overview
27-33: Polish section title and phrasingCapitalize “Grout” and clean up protocol names.
-## 1. Packets from grout to gr-loop0 +## 1. Packets from Grout to gr-loop0 @@ -All packets received to a **local** IP configured in grout will be processed by the node "ip_input_local/ip6_input_local". -If the protocol isn't handled locally (ICMP, Neighbor Disc, ...), the packets are forwarded to "l4_input_local". -If still unhandled (e.g., VXLAN Tunnel), the packet is sent to the corresponding gr-loopX interface associated with the VRF. +All packets received for a **local** IP configured in Grout are processed by the node "ip_input_local/ip6_input_local". +If the protocol isn't handled locally (e.g., ICMP, Neighbor Discovery), packets are forwarded to "l4_input_local". +If still unhandled (for example, a VXLAN tunnel), the packet is sent to the corresponding gr-loopX interface associated with the VRF.
61-64: Mention applying sysctl persistentlyAfter adding to /etc/sysctl.d, recommend running “sysctl --system” so changes apply without reboot.
70-75: Generalize the zebra plugin pathHardcoding a build-path under /root is brittle. Prefer the installed module path and mention that it may vary by distro.
-# /etc/frr/daemons, for example: +# /etc/frr/daemons, for example (adjust path as needed): @@ -zebra_options=" -A 127.0.0.1 -s 90000000 -M /root/grout/build/frr/frr_dplane_grout.so" +zebra_options=" -A 127.0.0.1 -s 90000000 -M /usr/lib/frr/modules/frr_dplane_grout.so"
77-92: Validate “neighbor … interface gr-loop0” usage and consider interface-based update-sourceThe “neighbor … interface gr-loop0” line is unusual given the neighbor is on 192.168.210.3 via cv0. Please confirm this is required for IP_TRANSPARENT behavior in your setup. Alternatively, consider using interface-based update-source to decouple from IP changes.
Potential alternative:
- neighbor 192.168.210.3 interface gr-loop0 - neighbor 192.168.210.3 update-source 192.168.210.1 + neighbor 192.168.210.3 update-source cv0If the ip-transparent knob mandates binding via gr-loop0, keep your original, but please add a short rationale.
94-98: Tighten wording and fix grammar-Three additional configuration for a neighbor is required: -- source interface, "gr-loop0", only required for the default VRF -- update source IP, local Grout IP -- ip-transparent, to allow bgpd to bind on the non existent IP. +Three additional neighbor settings are required: +- Source interface: "gr-loop0" (required only in the default VRF) +- Update-source IP: the local Grout IP +- ip-transparent: allow bgpd to bind to the nonexistent IP
102-115: Fix inconsistency in sample output (Peers vs Total neighbors)“Peers 2” conflicts with “Total number of neighbors 1”. Adjust to avoid confusion.
-Peers 2, using 47 KiB of memory +Peers 1, using 47 KiB of memory @@ -Total number of neighbors 1 +Total number of neighbors 1
4-4: Unify heading style (markdownlint MD003)Current headings mix setext and atx styles. Either disable MD003 for manpages or switch these to setext for consistency (matches the top title style).
Example conversions:
-# FRR + Grout: Transparent BGP Peering via Loopback Interfaces +FRR + Grout: Transparent BGP Peering via Loopback Interfaces +------------------------------------------------------------ -## 0. Ease of Use +0. Ease of Use +-------------- -## 1. Packets from Grout to gr-loop0 +1. Packets from Grout to gr-loop0 +---------------------------------- -## 2. Linux configuration +2. Linux configuration +---------------------- -## 3. FRR Configuration Overview +3. FRR Configuration Overview +----------------------------- -# AUTHORS +AUTHORS +-------Also applies to: 16-16, 27-27, 35-35, 68-68, 137-137
37-48: Security hardening: restrict acceptance to intended trafficRouting all packets as local on gr-loop0 is powerful. Consider documenting a filter to limit accepted traffic (e.g., TCP/179 to/from the BGP peer) to reduce exposure.
Example (iptables):
iptables -A INPUT -i gr-loop0 -p tcp --dport 179 -s 192.168.210.3 -j ACCEPT iptables -A INPUT -i gr-loop0 -j DROPnftables example available on request.
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/grout-frr.7.md(1 hunks)docs/meson.build(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/grout-frr.7.md
[grammar] ~6-~6: There might be a mistake here.
Context: ...inux networking to establish BGP peering between Grout and an upstream router, wi...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...tly to the linux interface. 0. Context 1. How packets are forwarded from Grout to ...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...ets are forwarded from Grout to gr-loop0 2. Linux configuration to peer without addi...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...onfiguration for a neighbor is required: - source interface, "gr-loop0", only requi...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
docs/grout-frr.7.md
4-4: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
16-16: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
27-27: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
35-35: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
68-68: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
137-137: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ 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-15, none, debugoptimized, ubuntu-22.04)
- 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)
🔇 Additional comments (1)
docs/meson.build (1)
18-18: LGTM: man page wired into build/installAdding grout-frr.7.md to man_src correctly integrates the new manpage into the existing md2man flow and installs it under man7.
| To ensure all incoming packets from gr-loop0 are accepted by the network stack, execute the following commands: | ||
| ```sh | ||
| a) sysctl -w net.ipv4.conf.gr-loop0.rp_filter=0 | ||
| b) iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1 | ||
| c) ip rule add fwmark 0x1 lookup 42 | ||
| d) ip route add local default dev gr-loop0 table 42 | ||
| ``` | ||
| a) Disables Reverse Path Filtering | ||
| b) All packets received on the gr-loop0 interface will have a specific mark | ||
| c) based on this mark, lookup on a specific table | ||
| d) accept ALL packets as if the destination was local. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make fwmark handling robust and non-destructive; add mask and rule priority
- Use MARK --or-mark to avoid clobbering existing marks.
- Match with a mask in the policy rule to avoid exact-value conflicts.
- Set a deterministic rule priority in the quick-commands section (matches the udev example).
-iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1
-ip rule add fwmark 0x1 lookup 42
+iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --or-mark 0x1
+ip rule add fwmark 0x1/0x1 lookup 42 priority 42Also consider documenting IPv6 equivalents:
# IPv6 equivalents (if peering over IPv6)
ip6tables -t mangle -A PREROUTING -i gr-loop0 -j MARK --or-mark 0x1
ip -6 rule add fwmark 0x1/0x1 lookup 42 priority 42
ip -6 route add local default dev gr-loop0 table 42If you’d like, I can provide an nftables variant too.
🤖 Prompt for AI Agents
In docs/grout-frr.7.md around lines 37 to 48, make the fwmark handling
non-destructive and deterministic: change the iptables/ip6tables MARK invocation
to use --or-mark so existing marks aren’t clobbered; change the ip rule add to
include a mask (e.g., fwmark 0x1/0x1) and supply an explicit priority (e.g.,
priority 42) instead of relying on default ordering; keep the ip route/ip -6
route add lines but ensure the examples show the matching IPv6 equivalents and
mention using ip6tables/nftables variants as needed.
| ```sh | ||
| # /etc/udev/rules.d/99-gr-loop0.rules | ||
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1" | ||
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route add local default dev gr-loop0 table 42" | ||
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule add fwmark 0x1 lookup 42 priority 42" | ||
|
|
||
| ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule del fwmark 0x1 lookup 42 priority 42" | ||
| ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i gr-loop0 -j MARK --set-mark 0x1" | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden udev rules: wildcard for gr-loopX, idempotency, and replace semantics
- Support multiple VRFs (gr-loopX), not just gr-loop0.
- Avoid duplicate rules by checking before appending.
- Prefer ip route replace to be idempotent.
- Match the fwmark rule with priority and mask as above.
-# /etc/udev/rules.d/99-gr-loop0.rules
-ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1"
-ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route add local default dev gr-loop0 table 42"
-ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule add fwmark 0x1 lookup 42 priority 42"
-
-ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule del fwmark 0x1 lookup 42 priority 42"
-ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i gr-loop0 -j MARK --set-mark 0x1"
+# /etc/udev/rules.d/99-gr-loop.rules
+ACTION=="add", KERNEL=="gr-loop*", RUN+="/bin/sh -c '/usr/sbin/iptables -t mangle -C PREROUTING -i %k -j MARK --or-mark 0x1 || /usr/sbin/iptables -t mangle -A PREROUTING -i %k -j MARK --or-mark 0x1'"
+ACTION=="add", KERNEL=="gr-loop*", RUN+="/usr/sbin/ip route replace local default dev %k table 42"
+ACTION=="add", KERNEL=="gr-loop*", RUN+="/bin/sh -c '/usr/sbin/ip rule show | grep -q \"fwmark 0x1/0x1.*lookup 42.*pref 42\" || /usr/sbin/ip rule add fwmark 0x1/0x1 lookup 42 priority 42'"
+
+ACTION=="remove", KERNEL=="gr-loop*", RUN+="/usr/sbin/ip rule del fwmark 0x1/0x1 lookup 42 priority 42"
+ACTION=="remove", KERNEL=="gr-loop*", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i %k -j MARK --or-mark 0x1"Note: On systems using nftables, consider documenting an nftables equivalent.
📝 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.
| ```sh | |
| # /etc/udev/rules.d/99-gr-loop0.rules | |
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1" | |
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route add local default dev gr-loop0 table 42" | |
| ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule add fwmark 0x1 lookup 42 priority 42" | |
| ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule del fwmark 0x1 lookup 42 priority 42" | |
| ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i gr-loop0 -j MARK --set-mark 0x1" | |
| ``` | |
| # /etc/udev/rules.d/99-gr-loop.rules | |
| ACTION=="add", KERNEL=="gr-loop*", RUN+="/bin/sh -c '/usr/sbin/iptables -t mangle -C PREROUTING -i %k -j MARK --or-mark 0x1 || /usr/sbin/iptables -t mangle -A PREROUTING -i %k -j MARK --or-mark 0x1'" | |
| ACTION=="add", KERNEL=="gr-loop*", RUN+="/usr/sbin/ip route replace local default dev %k table 42" | |
| ACTION=="add", KERNEL=="gr-loop*", RUN+="/bin/sh -c '/usr/sbin/ip rule show | grep -q \"fwmark 0x1/0x1.*lookup 42.*pref 42\" || /usr/sbin/ip rule add fwmark 0x1/0x1 lookup 42 priority 42'" | |
| ACTION=="remove", KERNEL=="gr-loop*", RUN+="/usr/sbin/ip rule del fwmark 0x1/0x1 lookup 42 priority 42" | |
| ACTION=="remove", KERNEL=="gr-loop*", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i %k -j MARK --or-mark 0x1" |
Provide a quick doc about Grout and Zebra configuration. Signed-off-by: Christophe Fontaine <[email protected]>
christophefontaine
force-pushed
the
docs
branch
from
09b0843 to
664857f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/grout-frr.7.md (2)
96-100: Good addition: ip-transparent prerequisites called out; minor wording tweakThis addresses the earlier ask to document kernel/FRR requirements. Minor nit: use ASCII hyphen and tighten phrasing.
-Note: The IP_TRANSPARENT socket option and FRR's BGP `neighbor <peer> ip-transparent` are supported -only if the Linux kernel provides IP_TRANSPARENT and FRRouting supports the feature (added in FRR 10.4.0). -The FRR daemon must run with sufficient network privileges to create raw/non‑local sockets and manage routing -(for example, CAP_NET_ADMIN and CAP_NET_RAW), which shouldn't be an issue. +Note: The IP_TRANSPARENT socket option and FRR’s BGP `neighbor <peer> ip-transparent` require a Linux kernel with IP_TRANSPARENT support and an FRRouting version that implements the feature (FRR 10.4.0 or newer). +The FRR daemon must run with sufficient network privileges to create raw and non-local sockets and manage routing +(for example, CAP_NET_ADMIN and CAP_NET_RAW).
53-61: Harden udev rules: wildcard gr-loop, idempotency checks, mask/priority, and consistent removal*Support multiple gr-loopX devices, avoid duplicates, and align with the non-destructive mark/masked fwmark. Removal should mirror additions.
```sh -# /etc/udev/rules.d/99-gr-loop0.rules -ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1" -ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route replace local default dev gr-loop0 table 42" -ACTION=="add", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule add fwmark 0x1 lookup 42 priority 42" - -ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip route del table 42 local default dev gr-loop0" -ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/ip rule del fwmark 0x1 lookup 42 priority 42" -ACTION=="remove", KERNEL=="gr-loop0", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i gr-loop0 -j MARK --set-mark 0x1" +# /etc/udev/rules.d/99-gr-loop.rules +ACTION=="add", KERNEL=="gr-loop*", RUN+="/bin/sh -c '/usr/sbin/iptables -t mangle -C PREROUTING -i %k -j MARK --or-mark 0x1 || /usr/sbin/iptables -t mangle -A PREROUTING -i %k -j MARK --or-mark 0x1'" +ACTION=="add", KERNEL=="gr-loop*", RUN+="/usr/sbin/ip route replace local default dev %k table 42" +ACTION=="add", KERNEL=="gr-loop*", RUN+="/bin/sh -c '/usr/sbin/ip rule show | grep -q \"fwmark 0x1/0x1.*lookup 42.*pref 42\" || /usr/sbin/ip rule add fwmark 0x1/0x1 lookup 42 priority 42'" + +ACTION=="remove", KERNEL=="gr-loop*", RUN+="/usr/sbin/ip rule del fwmark 0x1/0x1 lookup 42 priority 42" +ACTION=="remove", KERNEL=="gr-loop*", RUN+="/usr/sbin/ip route del table 42 local default dev %k" +ACTION=="remove", KERNEL=="gr-loop*", RUN+="/usr/sbin/iptables -t mangle -D PREROUTING -i %k -j MARK --or-mark 0x1"
🧹 Nitpick comments (5)
docs/grout-frr.7.md (5)
10-14: Fix ToC naming/capitalization to match sectionsAlign ToC entries with actual headings and capitalize IP.
-0. Context +0. Ease of Use 1. How packets are forwarded from Grout to gr-loop0 -2. Linux configuration to peer without adding a local ip -3. BGPD configuration +2. Linux configuration to peer without adding a local IP +3. FRR configuration overview
6-7: Capitalize LinuxMinor editorial fix.
-between Grout and an upstream router, without assigning IP addresses directly to the linux interface. +between Grout and an upstream router, without assigning IP addresses directly to the Linux interface.
102-106: Grammar/wording: pluralization and hyphenationTighten the phrasing and fix “non existent”.
-Three additional configuration for a neighbor is required: +Three additional settings are required for a neighbor: ... -- ip-transparent, to allow bgpd to bind on the non existent IP. +- ip-transparent, to allow bgpd to bind on the non-existent IP.
48-49: Capitalize TCP and streamline the sentenceEditorial polish for clarity.
-While we could mark only the packets matching the tcp port 179 (for BGP), we accept all packets for other applications. +While we could mark only packets matching TCP port 179 (for BGP), we accept all packets to support other applications.
1-3: Silence markdownlint MD003 for manpage-style headingsThis file mixes manpage setext title with ATX subheadings. If retaining ATX, disable MD003 for this page.
+<!-- markdownlint-disable MD003 --> GROUT-FRR 7 @DATE@ "grout @VERSION@" ================================
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/grout-frr.7.md(1 hunks)docs/meson.build(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/meson.build
🧰 Additional context used
🪛 LanguageTool
docs/grout-frr.7.md
[grammar] ~6-~6: There might be a mistake here.
Context: ...inux networking to establish BGP peering between Grout and an upstream router, wi...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...tly to the linux interface. 0. Context 1. How packets are forwarded from Grout to ...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...ets are forwarded from Grout to gr-loop0 2. Linux configuration to peer without addi...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...packets as if the destination was local. While we could mark only the packets mat...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...raw/non‑local sockets and manage routing (for example, CAP_NET_ADMIN and CAP_NET_...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...onfiguration for a neighbor is required: - source interface, "gr-loop0", only requi...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
docs/grout-frr.7.md
4-4: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
16-16: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
27-27: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
35-35: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
70-70: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
145-145: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ 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-18, none, debugoptimized, ubuntu-24.04)
- GitHub Check: build-and-tests (clang-16, 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)
| To ensure all incoming packets from gr-loop0 are accepted by the network stack, execute the following commands: | ||
| ```sh | ||
| a) sysctl -w net.ipv4.conf.gr-loop0.rp_filter=0 | ||
| b) iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1 | ||
| c) ip rule add fwmark 0x1 lookup 42 | ||
| d) ip route add local default dev gr-loop0 table 42 | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make fwmark handling non-destructive and deterministic; add IPv6 equivalents
Use --or-mark so you don’t clobber existing marks, match with a mask in the rule, and set an explicit priority. Also add IPv6 examples.
```sh
a) sysctl -w net.ipv4.conf.gr-loop0.rp_filter=0
-b) iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --set-mark 0x1
-c) ip rule add fwmark 0x1 lookup 42
+b) iptables -t mangle -A PREROUTING -i gr-loop0 -j MARK --or-mark 0x1
+c) ip rule add fwmark 0x1/0x1 lookup 42 priority 42
d) ip route add local default dev gr-loop0 table 42
+
+# IPv6 equivalents (if peering over IPv6)
+ip6tables -t mangle -A PREROUTING -i gr-loop0 -j MARK --or-mark 0x1
+ip -6 rule add fwmark 0x1/0x1 lookup 42 priority 42
+ip -6 route add local default dev gr-loop0 table 42🤖 Prompt for AI Agents
In docs/grout-frr.7.md around lines 37 to 43, the fwmark handling is destructive
and lacks determinism and IPv6 equivalents; change the iptables/ip6tables MARK
target to use --or-mark so existing marks are preserved, change the ip rule/ip
-6 rule to match with a mask (e.g. /0x1) and include an explicit priority, and
add IPv6 equivalents for the mangle, rule, and route entries so both IPv4 and
IPv6 peerings are handled predictably.
|
LGTM |
3 participants
Provide a quick doc about Grout and Zebra configuration.
Summary by CodeRabbit
Documentation
Chores