-
Notifications
You must be signed in to change notification settings - Fork 81
add nexthop support #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
add nexthop support #149
Conversation
Summary of ChangesHello @nanamicat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the library's networking capabilities by introducing full support for nexthop management. It provides a structured and type-safe way to interact with nexthop entries, enabling programmatic control over how packets are forwarded. This enhancement is crucial for applications requiring fine-grained control over routing decisions and network topology. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for managing nexthop objects, equivalent to ip nexthop commands. It introduces a new nexthop module with handles, requests, and a message builder, following the existing patterns in the crate. The changes also include adding nexthop_id to the route builder to allow routes to use nexthop objects. The implementation is comprehensive and well-integrated. I have one suggestion to improve the robustness of the NexthopMessageBuilder by adding validation for consistent IP address families when setting the gateway.
| pub fn gateway(mut self, addr: IpAddr) -> Self { | ||
| let (family, bytes) = match addr { | ||
| IpAddr::V4(addr) => (AddressFamily::Inet, addr.octets().to_vec()), | ||
| IpAddr::V6(addr) => (AddressFamily::Inet6, addr.octets().to_vec()), | ||
| }; | ||
| // Only set family if not already set or if explicitly different (though usually we trust the caller) | ||
| if self.message.header.family == AddressFamily::Unspec { | ||
| self.message.header.family = family; | ||
| } | ||
| self.message.nlas.push(NexthopAttribute::Gateway(bytes)); | ||
| self | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of gateway() can lead to an inconsistent NexthopMessage if it's called multiple times with IP addresses of different families. The address family is set based on the first call, but this is not validated on subsequent calls. This could result in a message with family = Inet but a gateway with an IPv6 address, which the kernel would likely reject.
To ensure correctness, the method should validate that any subsequent calls use an IP address of the same family. Panicking on misuse is an acceptable and common approach for builders to enforce correct usage.
The comment on line 157 is also a bit confusing and can be removed as part of this change.
pub fn gateway(mut self, addr: IpAddr) -> Self {
let (family, bytes) = match addr {
IpAddr::V4(addr) => (AddressFamily::Inet, addr.octets().to_vec()),
IpAddr::V6(addr) => (AddressFamily::Inet6, addr.octets().to_vec()),
};
if self.message.header.family == AddressFamily::Unspec {
self.message.header.family = family;
} else if self.message.header.family != family {
panic!("Attempted to set gateway with an inconsistent IP address family");
}
self.message.nlas.push(NexthopAttribute::Gateway(bytes));
self
}
https://www.man7.org/linux/man-pages/man8/ip-nexthop.8.html