-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support ctnetlink messages #9
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
Signed-off-by: terassyi <[email protected]>
98ea7f8
to
2cb429b
Compare
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.
Just some cosmetic review comments.
Please give me more time(up to 2 weeks) to test and review the real functionality.
@@ -0,0 +1,4 @@ | |||
// SPDX-License-Identifier: MIT | |||
|
|||
pub mod message; |
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.
Let's not expose internal module path to public.
I prefer expose all types as netlink-packet-netfilter::conn_track::{ContrackAttr, etc}
.
}; | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq)] | ||
pub struct CtAttr { |
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.
How about ConnTrackAttribute
?
GetUnconfirmed(Option<Vec<FlowNla>>), | ||
Other { | ||
message_type: u8, | ||
nlas: Vec<DefaultNla>, |
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.
Unless kernel code confirmed future data will always a array of Nla, we should use Other((u8, DefaultNla))
.
|
||
// netflter/nfnetlink_conntrack.h | ||
// There is no definitions in rust-lang/libc | ||
pub const IPCTNL_MSG_CT_NEW: u8 = 0; |
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 netlink-packet-route has stopped exposing constants out.
I do not have time to polish here yet, but please do not add more lines to src/contants.rs
.
Please:
- Remoev
pub
. - Move constant to its user, this make our review easier.
]; | ||
|
||
#[test] | ||
fn test_ct_attr_parse() { |
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.
I am expecting test case looks like:
https://github.com/rust-netlink/netlink-packet-route/blob/main/src/link/tests/vrf.rs#L68
(You do not need to document every bits)
You may use nlmon to capture real netlink message:
https://github.com/rust-netlink/netlink-packet-route?tab=readme-ov-file#development
pub struct ProtocolInfoTcp { | ||
pub state: u8, | ||
pub wscale_original: u8, | ||
pub wscale_reply: u8, | ||
pub flgas_original: u16, | ||
pub flags_reply: u16, | ||
} |
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.
Please consider to use NlaBuffer
, example: https://github.com/rust-netlink/netlink-packet-route/blob/main/src/route/via.rs#L31
use crate::constants::CTA_STATUS; | ||
|
||
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
pub enum ConnectionStatusFlag { |
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.
Please use bitflags!
here. Example: https://github.com/rust-netlink/netlink-packet-route/blob/main/src/route/next_hops.rs#L22
// SPDX-License-Identifier: MIT | ||
|
||
pub mod ct_attr; | ||
pub mod flow; |
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.
do not expose module path out.
Invalid(u32), | ||
Ignore(u32), // no longer used | ||
Delete(u32), // no longer used | ||
DeleteList(u32), // no longer used |
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.
If no longer used
, please remove it.
@@ -4,4 +4,5 @@ pub(crate) mod buffer; | |||
pub mod constants; | |||
mod message; | |||
pub use message::{NetfilterHeader, NetfilterMessage, NetfilterMessageInner}; | |||
pub mod ctnetlink; |
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 crate name already contains netlink, how about conn_track
?
This PR supports CtNetlink messages and add some example code to use it.
This is based on #8.
And as a reference implementation, I'm developing conntrack command in Rust with rust-netlink.
https://github.com/terassyi/rconntrack