Add support for Resilient Next-hop Groups Operations - #1205
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNexthop definitions now support multipath and resilient groups. Linux netlink encoding, decoding, validation, and ChangesNexthop group support
Interrupted dump retries
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Nexthop
participant NexthopAttrHandlers
participant LinuxNetlink
Nexthop->>NexthopAttrHandlers: provide group and resilient-group fields
NexthopAttrHandlers->>LinuxNetlink: encode group attributes
LinuxNetlink-->>NexthopAttrHandlers: return group attributes
NexthopAttrHandlers-->>Nexthop: decode members and timer values
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
nexthop.go (1)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
NexthopResGroupand its field units; fix a typo above.
NexthopResGrouphas no doc comment and none of its fields are documented, unlikeNexthopGroupMpath. Since nexthop_linux.go silently convertsIdleTimer/UnbalancedTimer/UnbalancedTimebetween seconds and kernel clock ticks, callers of this public struct have no indication these fields are expressed in seconds. Also, theNexthopGroupMpathdoc comment above has a typo ("nexthtop").♻️ Proposed doc improvements
-// NexthopGroupMpath represents one member of a nexthtop group +// NexthopGroupMpath represents one member of a nexthop group type NexthopGroupMpath struct { // ID of an existing nexthop to include in the group ID uint32 // Relative weight, 1-256. Zero is treated as 1 Weight uint16 } +// NexthopResGroup represents the parameters of a resilient nexthop group. type NexthopResGroup struct { + // Number of buckets in the hash table Buckets uint16 + // Idle timer, in seconds IdleTimer uint32 + // Unbalanced timer, in seconds UnbalancedTimer uint32 + // Time out of balance, in seconds (read-only) UnbalancedTime uint64 }🤖 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 `@nexthop.go` around lines 26 - 32, Document the public NexthopResGroup type and each field, explicitly stating that IdleTimer, UnbalancedTimer, and UnbalancedTime use seconds while Buckets is the bucket count; keep the descriptions accurate to the kernel conversion behavior. Also correct the “nexthtop” typo in the existing NexthopGroupMpath documentation.
🤖 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 `@nexthop_linux.go`:
- Around line 357-364: Add the missing fmt import to nexthop_linux.go so
prepareNewNexthop can compile its existing fmt.Errorf validation paths; leave
the validation logic unchanged.
In `@nexthop.go`:
- Around line 18-24: Change NexthopGroupMpath.ID from uint16 to uint32 so it
matches Nexthop.ID and the uint32 encoding/decoding in the nexthop group
serialization logic. Leave Weight and the surrounding NexthopGroupMpath
structure unchanged.
---
Nitpick comments:
In `@nexthop.go`:
- Around line 26-32: Document the public NexthopResGroup type and each field,
explicitly stating that IdleTimer, UnbalancedTimer, and UnbalancedTime use
seconds while Buckets is the bucket count; keep the descriptions accurate to the
kernel conversion behavior. Also correct the “nexthtop” typo in the existing
NexthopGroupMpath documentation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 302c24e9-8d37-4bc0-a541-12db7ab00e4f
📒 Files selected for processing (3)
nexthop.gonexthop_linux.gonl/nl_linux.go
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nexthop_linux.go (2)
247-268: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winResolve the undefined resilient-group test type.
The supplied typecheck reports
nexthop_test.go:16:12: undefined: NexthopResilientGroupAttrs. This decoder constructsNexthopResilientGroup. Align the test and callers with the exported type, or add the intended compatibility type. The package cannot build until the names match.🤖 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 `@nexthop_linux.go` around lines 247 - 268, The resilient-group decoder uses NexthopResilientGroup, but callers or tests still reference the undefined NexthopResilientGroupAttrs type. Update those references, including nexthop_test.go, to use the exported NexthopResilientGroup type, or define an intentional compatibility alias if that name must remain supported.Source: Linters/SAST tools
191-198: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject partial
NHA_GROUPpayloads.The decoder accepts every complete 8-byte prefix and silently ignores a trailing partial member. A malformed attribute can therefore produce a partial
nh.Group. Validate that the payload length is divisible bysizeofNexthopGroupMemberbefore decoding, and assignnh.Grouponly after the complete payload passes validation. (raw.githubusercontent.com)🛠️ Proposed fix
decode: func(nh *Nexthop, attr *nl.RtAttr) { - nh.Group = nil + if len(attr.Data)%sizeofNexthopGroupMember != 0 { + return + } + group := make([]NexthopGroupMember, 0, len(attr.Data)/sizeofNexthopGroupMember) for off := 0; off+sizeofNexthopGroupMember <= len(attr.Data); off += sizeofNexthopGroupMember { - nh.Group = append(nh.Group, NexthopGroupMember{ + group = append(group, NexthopGroupMember{ ID: native.Uint32(attr.Data[off : off+4]), Weight: uint16(attr.Data[off+4]) + 1, }) } + nh.Group = group },🤖 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 `@nexthop_linux.go` around lines 191 - 198, Update the Nexthop group decoder to reject payloads whose length is not divisible by sizeofNexthopGroupMember before iterating. Decode into a temporary group and assign nh.Group only after validation and complete decoding succeed, preventing partial NHA_GROUP data from being accepted.
🧹 Nitpick comments (1)
nexthop.go (1)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
String()does not reflect the new group fields.
Nexthopnow carriesGroup,GroupType, andResilientGroup, butString()(Lines 48-57) still only printsID,Blackhole,OIF,Gateway, andProtocol. Logging or debugging a group nexthop withString()silently drops its group membership and resilient configuration.♻️ Proposed extension to `String()`
func (h *Nexthop) String() string { elems := []string{ "ID: " + strconv.FormatUint(uint64(h.ID), 10), "Blackhole: " + strconv.FormatBool(h.Blackhole), "OIF: " + strconv.FormatUint(uint64(h.OIF), 10), "Gateway: " + h.Gateway.String(), "Protocol: " + h.Protocol.String(), } + if len(h.Group) > 0 { + elems = append(elems, fmt.Sprintf("GroupType: %d", h.GroupType)) + } return fmt.Sprintf("{%s}", strings.Join(elems, " ")) }🤖 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 `@nexthop.go` around lines 35 - 46, Update Nexthop.String() to include the Group, GroupType, and ResilientGroup fields in its formatted output, while preserving the existing ID, Blackhole, OIF, Gateway, and Protocol details.
🤖 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 `@CHANGELOG.md`:
- Around line 19-22: Update the changelog entries to use the exported API
identifiers from nexthop.go: replace NexthopGroupMpath with NexthopGroupMember,
NexthopResGroup with NexthopResilientGroup, and Nexthop.ResGroup with
Nexthop.ResilientGroup; keep the documented fields and group-type constants
unchanged.
In `@examples/nexthop/main.go`:
- Around line 69-89: Update buildNexthop so the OIF assignment occurs only after
the *blackhole branch returns; blackhole nexthops must contain the ID and
Blackhole fields without setting OIF, while regular gateway nexthops retain the
existing link interface assignment.
In `@nexthop_linux.go`:
- Around line 230-237: Update the resilient timer encoding in the nexthop
attribute construction to multiply IdleTimer and UnbalancedTimer using a wider
integer type, reject values whose product exceeds uint32, and propagate the
resulting error instead of encoding a wrapped value. Add boundary tests covering
42_949_672 as valid and 42_949_673 as rejected for both timer fields.
In `@nexthop_test.go`:
- Around line 14-25: Update TestNexthopGroupMemberAndResilientGroupTypes to
instantiate the existing NexthopResilientGroup type instead of the undefined
NexthopResilientGroupAttrs, preserving the Buckets and IdleTimer assertions.
---
Outside diff comments:
In `@nexthop_linux.go`:
- Around line 247-268: The resilient-group decoder uses NexthopResilientGroup,
but callers or tests still reference the undefined NexthopResilientGroupAttrs
type. Update those references, including nexthop_test.go, to use the exported
NexthopResilientGroup type, or define an intentional compatibility alias if that
name must remain supported.
- Around line 191-198: Update the Nexthop group decoder to reject payloads whose
length is not divisible by sizeofNexthopGroupMember before iterating. Decode
into a temporary group and assign nh.Group only after validation and complete
decoding succeed, preventing partial NHA_GROUP data from being accepted.
---
Nitpick comments:
In `@nexthop.go`:
- Around line 35-46: Update Nexthop.String() to include the Group, GroupType,
and ResilientGroup fields in its formatted output, while preserving the existing
ID, Blackhole, OIF, Gateway, and Protocol details.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf092ae8-a031-4f75-b071-4fcf6eeedabb
⛔ Files ignored due to path filters (3)
examples/nexthop-group/go.sumis excluded by!**/*.sumexamples/nexthop/go.sumis excluded by!**/*.sumexamples/resilient-nexthop-group/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
CHANGELOG.mdexamples/nexthop-group/go.modexamples/nexthop-group/main.goexamples/nexthop/go.modexamples/nexthop/main.goexamples/resilient-nexthop-group/go.modexamples/resilient-nexthop-group/main.gonexthop.gonexthop_linux.gonexthop_test.go
| if nh.ResilientGroup.IdleTimer > 0 { | ||
| b := make([]byte, 4) | ||
| native.PutUint32(b, nh.ResilientGroup.IdleTimer*nexthopResilientGroupUserHz) | ||
| attr.AddRtAttr(nl.NHA_RES_GROUP_IDLE_TIMER, b) | ||
| } | ||
| if nh.ResilientGroup.UnbalancedTimer > 0 { | ||
| b := make([]byte, 4) | ||
| native.PutUint32(b, nh.ResilientGroup.UnbalancedTimer*nexthopResilientGroupUserHz) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)nexthop_linux\.go$|nexthop|rtattr|resilient|timer' || true
echo "== nexthop_linux outline =="
ast-grep outline nexthop_linux.go --view expanded || true
echo "== relevant lines =="
sed -n '1,120p' nexthop_linux.go
echo "== 120-330 =="
sed -n '120,330p' nexthop_linux.go
echo "== searches =="
rg -n "nexthopResilientGroupUserHz|IdleTimer|UnbalancedTimer|NHA_RES_GROUP|PutUint32|ResilientGroup" .Repository: vishvananda/netlink
Length of output: 25770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== nexthop.go relevant =="
sed -n '1,70p' nexthop.go
echo "== preparation/encoding relevant =="
sed -n '330,390p' nexthop_linux.go
sed -n '360,385p' nexthop_linux.go
echo "== ready-only wrapper behavior probe =="
go version
cat > /tmp/wrap_probe.go <<'EOF'
package main
import (
"encoding/binary"
"encoding/hex"
"fmt"
"math"
)
const nexthopResilientGroupUserHz = 100
func main() {
for _, v := range []uint32{42_949_672, 42_949_673, 100_000_000, math.MaxUint32} {
product := v * nexthopResilientGroupUserHz
encoded := make([]byte, 4)
binary.LittleEndian.PutUint32(encoded, product)
fmt.Printf("sec=%d product_uint32=%d hex=%s\n", v, product, hex.EncodeToString(encoded))
}
}
EOF
go run /tmp/wrap_probe.go
echo "== kernel source checks online via local shell if reachable =="
for url in \
"https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/plain/include/uapi/linux/rtnetlink.h?h=v6.8" \
"https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/plain/net/ipv4/fib_nexthops.c?h=v6.8"
do
echo "--- $url ---"
curl -fsSL "$url" | rg -n "NHA_RES_GROUP_IDLE_TIMER|NHA_RES_GROUP_UNBALANCED_TIMER|idle_timer|unbalanced_timer|struct nexthop_res_bucket|clock_t" | head -80 || true
doneRepository: vishvananda/netlink
Length of output: 4758
🌐 Web query:
Linux nexthop.c NHA_RES_GROUP_IDLE_TIMER validation u32 clock_t oversized timer
💡 Result:
The attribute NHA_RES_GROUP_IDLE_TIMER in the Linux kernel's nexthop subsystem is used to configure the idle timer for resilient nexthop group buckets, measured in units of clock_t [1][2]. A known issue involving potential integer overflow was identified in the iproute2 user-space tool (the primary interface for configuring these nexthops) regarding the validation of this timer value [3]. Specifically, earlier implementations used the expression idle_timer >= ~0UL / 100 to validate the user-provided input [3]. On 64-bit systems, ~0UL is a 64-bit value, which allowed the validation check to pass for values that, when subsequently multiplied by 100 in the kernel or helper functions, would overflow a 32-bit unsigned integer (as the netlink attribute NHA_RES_GROUP_IDLE_TIMER is defined as a u32) [2][3]. To resolve this, the validation logic was updated to use UINT32_MAX / 100 instead of ~0UL / 100 [3]. This ensures that the provided idle timer value is constrained such that it will not result in an integer overflow after the unit conversion (scaling by 100) before being passed to the kernel [3]. The kernel itself defines the attribute in the netlink policy as NLA_U32, and internal kernel code converts jiffies to clock_t using jiffies_to_clock_t when reporting this value back to user space [4][5][6].
Citations:
- 1: https://docs.kernel.org/networking/nexthop-group-resilient.html
- 2: https://github.com/torvalds/linux/blob/master/include/uapi/linux/nexthop.h
- 3: https://lists.openwall.net/netdev/2021/11/17/225
- 4: https://gbmc.googlesource.com/linux/+/33f36147be36c5fa0919d32d20d501ca97981d9a/net/ipv4/nexthop.c
- 5: https://gbmc.googlesource.com/linux/+/6be5e47b69d8bb7c3dba8d84807624954243c23a/net/ipv4/nexthop.c
- 6: https://gbmc.googlesource.com/linux/+/64b4aef17ea65c423fbcdee323b503d8e3ffb3a2/net/ipv4/nexthop.c
Guard the resilient timer conversion against uint32 wraparound.
IdleTimer and UnbalancedTimer are uint32, so value*nexthopResilientGroupUserHz wraps modulo 2^32. Values above 42_949_672 encode a much shorter timer, including 42_949_673 encoding as 0x04. Compute the product in a wider type and return an error if it exceeds uint32; add boundary tests for 42_949_672 and 42_949_673.
🤖 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 `@nexthop_linux.go` around lines 230 - 237, Update the resilient timer encoding
in the nexthop attribute construction to multiply IdleTimer and UnbalancedTimer
using a wider integer type, reject values whose product exceeds uint32, and
propagate the resulting error instead of encoding a wrapped value. Add boundary
tests covering 42_949_672 as valid and 42_949_673 as rejected for both timer
fields.
Removed unreleased section detailing nexthop support for resilient next-hop groups, including background and new API types.
1 participant
Changelog
This PR brings support for nexthop group and nexthop resilient group
*What existed in upstream
The upstream library previously had only basic nexthop support (from #1139 ):
nlpackage (nl.Nhmsg,NewNexthopRequest).NexthopAdd/NexthopDel/NexthopList/NexthopReplaceAPIs.NHA_ID,NHA_BLACKHOLE,NHA_OIF,NHA_GATEWAY, andProtocol.NHIDfield onRouteto attach an existing nexthop object to a route.What this PR brings
NexthopGroupMember{ID, Weight}- a member (nexthop ID + relative weight, 1–256; 0 treated as 1).NexthopResilientGroup{Buckets, IdleTimer, UnbalancedTimer, UnbalancedTime}- resilient-group configuration.NEXTHOP_GRP_TYPE_MPATHandNEXTHOP_GRP_TYPE_RES.Nexthopstruct withGroup []NexthopGroupMember,GroupType uint16, andResilientGroup *NexthopResilientGroup.NHA_GROUP(with weight encodingwire = weight - 1),NHA_GROUP_TYPE, and the nestedNHA_RES_GROUP(NHA_RES_GROUP_BUCKETS,NHA_RES_GROUP_IDLE_TIMER,NHA_RES_GROUP_UNBALANCED_TIMER,NHA_RES_GROUP_UNBALANCED_TIME), where timers are converted to/from kernelclock_tunits.nlpackage constants:NHA_RES_GROUP,NHA_RES_BUCKET,NHA_RES_GROUP_*,NHA_RES_GROUP_PAD.prepareNewNexthop(resilient group requiresNEXTHOP_GRP_TYPE_RES; non-empty group for a set group type) and family derivation (FAMILY_ALL) for group nexthops.examples/resilient-nexthop-groupip nexthop add id <N> group <members> type res buckets <n> idle_timer <s> unbalanced_timer <s>.Summary by CodeRabbit