type safety: make Socket aware of namespace #1299
Conversation
Reviewer's GuideThis PR adds compile-time namespace safety to netlink sockets by parameterizing the Socket type with HostNS and ContainerNS marker types and updating all related function signatures and utilities to enforce correct namespace usage without runtime overhead. Class diagram for updated NamespaceOptions and open_netlink_socketsclassDiagram
class NamespaceOptions~N: netlink::Namespace~ {
+ File file
+ netlink::Socket<N> netlink
}
class open_netlink_sockets {
+ open_netlink_sockets(netns_path: &str) NetavarkResult<(
NamespaceOptions<netlink::HostNS>,
NamespaceOptions<netlink::ContainerNS>
)>
}
Class diagram for updated NetworkDriver trait and implementorsclassDiagram
class NetworkDriver {
<<trait>>
+ setup(&self, netlink_sockets: (&mut netlink::Socket, &mut netlink::Socket<netlink::ContainerNS>))
+ teardown(&self, netlink_sockets: (&mut netlink::Socket, &mut netlink::Socket<netlink::ContainerNS>))
}
class Bridge
class Vlan
class PluginDriver
NetworkDriver <|.. Bridge
NetworkDriver <|.. Vlan
NetworkDriver <|.. PluginDriver
Class diagram for trait Address and its updated methodsclassDiagram
class Address~T~ {
<<trait>>
+ new(l: &Lease, interface: &str) Result<Self, ProxyError>
+ add_ip(&self, nls: &mut Socket<netlink::ContainerNS>) Result<(), ProxyError>
+ add_gws(&self, nls: &mut Socket<netlink::ContainerNS>) Result<(), ProxyError>
}
class MacVLAN
Address <|.. MacVLAN
Class diagram for updated dhcp_teardown functionclassDiagram
class dhcp_teardown {
+ dhcp_teardown(info: &DriverInfo, sock: &mut netlink::Socket<netlink::ContainerNS>) NetavarkResult<()>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Luap99 - I've reviewed your changes - here's some feedback:
- Consider adding type aliases like
HostSocketandContainerSocketto reduce repetitive use ofSocket<HostNS>/Socket<ContainerNS>in function signatures and improve readability. - Provide explicit constructors (e.g.
Socket::new_host()andSocket::new_container()) to avoid manual generic annotations and make the intent of each namespace socket clearer. - Reevaluate the default
HostNStype parameter onSocket—removing the default could help catch accidental namespace mismatches by forcing explicit type distinctions everywhere.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding type aliases like `HostSocket` and `ContainerSocket` to reduce repetitive use of `Socket<HostNS>`/`Socket<ContainerNS>` in function signatures and improve readability.
- Provide explicit constructors (e.g. `Socket::new_host()` and `Socket::new_container()`) to avoid manual generic annotations and make the intent of each namespace socket clearer.
- Reevaluate the default `HostNS` type parameter on `Socket`—removing the default could help catch accidental namespace mismatches by forcing explicit type distinctions everywhere.
## Individual Comments
### Comment 1
<location> `src/network/driver.rs:41` </location>
<code_context>
fn setup(
&self,
- netlink_sockets: (&mut netlink::Socket, &mut netlink::Socket),
+ netlink_sockets: (
+ &mut netlink::Socket,
+ &mut netlink::Socket<netlink::ContainerNS>,
+ ),
) -> NetavarkResult<(StatusBlock, Option<AardvarkEntry>)> {
</code_context>
<issue_to_address>
The trait now enforces correct netlink socket types, but the first socket is still untyped.
Consider specifying the first socket as Socket<HostNS> to ensure type safety for both sockets.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| netlink_sockets: ( | ||
| &mut netlink::Socket, | ||
| &mut netlink::Socket<netlink::ContainerNS>, |
There was a problem hiding this comment.
suggestion: The trait now enforces correct netlink socket types, but the first socket is still untyped.
Consider specifying the first socket as Socket to ensure type safety for both sockets.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, sourcery-ai[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Using a PhantomData and zero sized struct we can attach an additional generic data for either the host or container namespace to the struct. Because it is zero sized and not used it is optimized away and only the type checker sees it to enforce the right types are used. With that we basically create two different netlink socket types Socket<HostNS> and Socket<ContainerNS> so they must be used in all type signatures from now on. To keep the changes smaller I have set HostNS as default generic for the struct so we don't need to change most function signatures. For al call sides where we pass sockets around the compiler now enforces that we use the right ones avoid any possible mix ups. Signed-off-by: Paul Holzinger <[email protected]>
There was a problem hiding this comment.
The is not a review but me attempting to understand this PR by explaining it back to you for confirmation.
Before this patch we just have a Socket struct which holds the actual netlink socket and a buffer and sequence_number to use with it.
With this patch you are first initializing a new trait called Namespace, you then initialize two new structs called HostNS and ContainerNS. The key point is they are empty and thus will be removed during compile time. You then set both HostNS and ContainerNS to having the type of Namespace
You then change the Socket struct to be a generic of type N, the <N: netlink::Namespace> part is a trait bound meaning N must be a type that implements Namespace and = HostNS is setting the default.
Then you set _marker: std::marker::PhantomData<N> which ties the Socket struct to N because _marker acts as though it stores a value of type N, even though it doesn’t really.
Now we have Socket<HostNS> and Socket<ContainerNS>, two different types so you can's mix them up. A function can now be defined to accept the exact type of socket you want to avoid issues, confusion and any mix ups.
https://doc.rust-lang.org/rust-by-example/generics/bounds.html
https://doc.rust-lang.org/std/marker/struct.PhantomData.html
From someone inexperienced with Rust this seemed at face value like more of a compiler hack but the the use of marker types and PhantomData is an idiomatic Rust technique for zero-cost compile-time guarantees. Really awesome @Luap99
|
yeah this is just so functions can clearly define if they use the host or netns socket. That way we cannot even compile code that might accidentally is given the wrong socket. While testing would likely catch it having it enforced at compile time is even better and it also means your IDE, etc... should already tell you this doesn't work... The most obvious case is add_default_routes() function, we only ever want add routes inside the container. So we prevent passing the host socket. |
|
LGTM. Had no idea this was a thing. |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Using a PhantomData and zero sized struct we can attach an additional generic data for either the host or container namespace to the struct. Because it is zero sized and not used it is optimized away and only the type checker sees it to enforce the right types are used.
With that we basically create two different netlink socket types Socket and Socket so they must be used in all type signatures from now on. To keep the changes smaller I have set HostNS as default generic for the struct so we don't need to change most function signatures.
For al call sides where we pass sockets around the compiler now enforces that we use the right ones avoid any possible mix ups.
Summary by Sourcery
Enforce compile-time namespace safety by parameterizing netlink sockets with phantom generic types for host and container namespaces and updating all socket usages accordingly.
Enhancements: