smoke: decouple nexthop assertions from route add events - #669
Conversation
Starting with FRR 10.8, routes are always installed via nexthop groups regardless of the number of nexthops. When a route has a single nexthop, grout resolves the group and stores the individual nexthop directly, but the initial "route add" event now carries the group reference instead of the resolved nexthop details. The smoke tests were matching the full nexthop description in the route add event pattern (e.g. "via type=L3 .*addr=172.16.0.2"). This broke with FRR 10.8 because the event now reads "via type=group ...". Split the route verification in two steps: first, wait for the route add event matching only on prefix and origin (which is stable across FRR versions). Then, look up the route's resolved nexthop via grcli and assert its attributes separately. Two new helpers (route_nh_id and assert_nexthop) handle the second step and work whether the route was installed with a nexthop group or a direct nexthop. Link: FRRouting/frr@99b1afa202d0d28273 Signed-off-by: Robin Jarry <rjarry@redhat.com>
📝 WalkthroughWalkthroughFRR smoke helpers now resolve route nexthop IDs and validate nexthop attributes through 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@smoke/_init_frr.sh`:
- Around line 158-164: Update the next-hop classification in the case statement
around assert_nexthop so dotted interface names such as p0.100 are treated as
interfaces, not addresses. Restrict the IPv4 branch to values matching a numeric
dotted-quad pattern, while preserving the existing IPv6 detection and .addr
validation for actual addresses and the .iface validation for interface names.
🪄 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 Plus
Run ID: 7dda97e8-4a3e-42d7-9ce9-973c0a2397dc
📒 Files selected for processing (3)
smoke/_init_frr.shsmoke/bgp6_srv6_frr_test.shsmoke/bgp_frr_test.sh
| case "$nh" in | ||
| *.*|*:*) | ||
| assert_nexthop "$nh_id" ".vrf == \"${nexthop_vrf_name:-$gr_vrf_name}\" and .addr ==\"$nh\"" | ||
| ;; | ||
| *) | ||
| assert_nexthop "$nh_id" ".vrf == \"${nexthop_vrf_name:-$gr_vrf_name}\" and .iface ==\"$nh\"" | ||
| ;; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle dotted interface names correctly.
Line 159 treats every value containing . as an address. A route via an interface such as p0.100 will be configured successfully but then fail validation against .addr. Restrict IPv4 detection to numeric dotted-quad values.
Proposed fix
- case "$nh" in
- *.*|*:*)
+ if [[ "$nh" == *:* || "$nh" =~ ^([0-9]{1,3}\.){3}[0-9]{1,3}$ ]]; then
assert_nexthop "$nh_id" ".vrf == \"${nexthop_vrf_name:-$gr_vrf_name}\" and .addr ==\"$nh\""
- ;;
- *)
+ else
assert_nexthop "$nh_id" ".vrf == \"${nexthop_vrf_name:-$gr_vrf_name}\" and .iface ==\"$nh\""
- ;;
- esac
+ fi📝 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.
| case "$nh" in | |
| *.*|*:*) | |
| assert_nexthop "$nh_id" ".vrf == \"${nexthop_vrf_name:-$gr_vrf_name}\" and .addr ==\"$nh\"" | |
| ;; | |
| *) | |
| assert_nexthop "$nh_id" ".vrf == \"${nexthop_vrf_name:-$gr_vrf_name}\" and .iface ==\"$nh\"" | |
| ;; | |
| if [[ "$nh" == *:* || "$nh" =~ ^([0-9]{1,3}\.){3}[0-9]{1,3}$ ]]; then | |
| assert_nexthop "$nh_id" ".vrf == \"${nexthop_vrf_name:-$gr_vrf_name}\" and .addr ==\"$nh\"" | |
| else | |
| assert_nexthop "$nh_id" ".vrf == \"${nexthop_vrf_name:-$gr_vrf_name}\" and .iface ==\"$nh\"" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@smoke/_init_frr.sh` around lines 158 - 164, Update the next-hop
classification in the case statement around assert_nexthop so dotted interface
names such as p0.100 are treated as interfaces, not addresses. Restrict the IPv4
branch to values matching a numeric dotted-quad pattern, while preserving the
existing IPv6 detection and .addr validation for actual addresses and the .iface
validation for interface names.
Starting with FRR 10.8, routes are always installed via nexthop groups regardless of the number of nexthops. When a route has a single nexthop, grout resolves the group and stores the individual nexthop directly, but the initial "route add" event now carries the group reference instead of the resolved nexthop details.
The smoke tests were matching the full nexthop description in the route add event pattern (e.g. "via type=L3 .*addr=172.16.0.2"). This broke with FRR 10.8 because the event now reads "via type=group ...".
Split the route verification in two steps: first, wait for the route add event matching only on prefix and origin (which is stable across FRR versions). Then, look up the route's resolved nexthop via grcli and assert its attributes separately. Two new helpers (route_nh_id and assert_nexthop) handle the second step and work whether the route was installed with a nexthop group or a direct nexthop.
Link: FRRouting/frr@99b1afa202d0d28273
Summary
grcli.