nexthop: Support LWT encapsulation #1140
Conversation
WalkthroughAdds Encap support to nexthops: new public Changes
Sequence Diagram(s)sequenceDiagram
participant Kernel as Kernel (netlink attrs)
participant Decoder as decodeNexthopAttrs
participant Handler as nexthopAttrHandler
participant Nexthop as Nexthop
participant decodeEncap as decodeEncap()
rect rgb(245,250,255)
note right of Kernel: incoming nexthop attributes (attrs[])
end
Kernel->>Decoder: send attrs[]
Decoder->>Handler: for each attr -> handler.decode(nh, attr, attrs)
alt attr == NHA_ENCAP
Handler->>Handler: locate NHA_ENCAP_TYPE in attrs[]
Handler->>decodeEncap: decodeEncap(encapType, attr.Value)
decodeEncap-->>Handler: Encap or error
Handler->>Nexthop: nh.Encap = decoded Encap
else other attrs
Handler->>Nexthop: populate other fields
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)route_linux.go (2)
nexthop_test.go (5)
nexthop_linux.go (5)
🔇 Additional comments (8)
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: 1
🧹 Nitpick comments (1)
route_linux.go (1)
1582-1612: Good refactoring: centralized encap decoding.Extracting the encap type decoding logic into a helper function eliminates duplication and will be reused by the nexthop encap decoding. The function correctly handles all supported encap types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
nexthop.go(2 hunks)nexthop_linux.go(7 hunks)nexthop_test.go(2 hunks)route_linux.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
route_linux.go (2)
route.go (1)
Encap(40-46)nl/syscall.go (5)
LWTUNNEL_ENCAP_MPLS(62-62)LWTUNNEL_ENCAP_SEG6(66-66)LWTUNNEL_ENCAP_SEG6_LOCAL(68-68)LWTUNNEL_ENCAP_BPF(67-67)LWTUNNEL_ENCAP_IP6(65-65)
nexthop.go (1)
route.go (1)
Encap(40-46)
nexthop_test.go (5)
nexthop.go (1)
Nexthop(10-17)route.go (1)
Encap(40-46)route_linux.go (1)
SEG6Encap(202-205)nl/seg6_linux.go (1)
SEG6_IPTUN_MODE_ENCAP(42-42)nexthop_linux.go (2)
NexthopAdd(13-15)NexthopList(69-71)
nexthop_linux.go (5)
nexthop.go (1)
Nexthop(10-17)nl/nl_linux.go (2)
RtAttr(395-399)NewRtAttr(402-410)route.go (1)
Encap(40-46)nl/conntrack_linux.go (1)
NLA_F_NESTED(77-77)nl/syscall.go (1)
LWTUNNEL_ENCAP_NONE(61-61)
🔇 Additional comments (9)
nexthop.go (1)
16-16: LGTM! Encap field added to Nexthop struct.The addition of the
Encapfield follows the existing pattern and correctly uses theEncapinterface type.nexthop_test.go (1)
158-213: Well-structured test for nexthop encapsulation.The test properly validates the round-trip encoding/decoding of SEG6Encap, including type assertions and field verification.
route_linux.go (1)
1572-1577: LGTM! Inline decoding replaced with helper function.The refactoring to use
decodeEncapreduces code duplication while maintaining the same error handling behavior.nexthop_linux.go (6)
115-118: Good: Updated decode signature for multi-attribute context.The signature change to accept the full attribute list enables decoders like
NHA_ENCAPto access related attributes (e.g.,NHA_ENCAP_TYPE) for proper decoding.
129-179: LGTM! Existing decoders updated for new signature.The unused parameter
_ []*nl.RtAttrmaintains signature compatibility with the updated decode function type.
180-189: LGTM! NHA_ENCAP_TYPE encode handler.The encode function correctly emits the encapsulation type when
Encapis set. Note that this attribute has no decode function becauseNHA_ENCAP's decoder handles both attributes together.
252-252: Correct: Full attribute list passed to decoders.This change enables the
NHA_ENCAPdecoder to accessNHA_ENCAP_TYPEfrom the full attribute list.
301-302: NHA_ENCAP_TYPE and NHA_ENCAP added to encoding.Correctly includes both encapsulation attributes in the
prepareNewNexthopencoding path.
190-221: Encoding errors are silently ignored—confirmed, but by architectural design.The review comment is correct: lines 194-196 return
nilwhennh.Encap.Encode()fails, causing the attribute to be silently skipped. However, this behavior is not accidental—thenexthopAttrHandlersframework is architected this way.The encode signature
func(*Nexthop) *nl.RtAttr(line 112) deliberately supports only nil returns, with an explicit comment (line 111): "It should return nil if the attribute is not set." This means:
- Returning
nilon error is indistinguishable from the attribute being unsetencodeNexthopAttrs(line 234) skipsnilreturns:if attr != nil { append }- The result: encoding failures silently disappear without trace
The suggested fix requires either:
- Changing the encode signature across all
nexthopAttrHandlersto return errors, and updatingencodeNexthopAttrsto propagate them, or- Adding error logging within individual handlers like NHA_ENCAP
Verify with the maintainers whether silent attribute skips on encoding errors are acceptable for nexthop updates, or if the architectural constraint should be lifted to support error propagation.
YutaroHayakawa
force-pushed
the
yutaro/nexthop-lwt-encap
branch
from
6ace38b to
910cd5f
Compare
Add support for Light Weight Tunnel encapsulation in the nexthop object. Since the netlink encoding is exactly the same as the one for route, we can reuse most of the encoding/decoding code. What we needed to do was supporting NHA_ENCAP_TYPE and NHA_ENCAP types. Signed-off-by: Yutaro Hayakawa <[email protected]>
Handle nil receiver case and all-zero values case. Also added test cases for that so that we can check this for the fields added in the future. Signed-off-by: Yutaro Hayakawa <[email protected]>
YutaroHayakawa
force-pushed
the
yutaro/nexthop-lwt-encap
branch
from
910cd5f to
18c7bdd
Compare
1 participant
Add support for Light Weight Tunnel encapsulation in the nexthop object. Since the netlink encoding is exactly the same as the one for route, we can reuse most of the encoding/decoding code. What we needed to do was supporting NHA_ENCAP_TYPE and NHA_ENCAP types.
Summary by CodeRabbit