link: add support of ipip interface#33
Merged
Merged
Conversation
Signed-off-by: Gris Ge <cnfourt@gmail.com>
Add integration tests covering all ipip tunnel options. Signed-off-by: Gris Ge <cnfourt@gmail.com>
cathay4t
enabled auto-merge (rebase)
June 10, 2026 00:37
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for IP-in-IP (IpIp) tunnel interfaces, including configuration parsing, formatting, and unit tests. The review identified a critical compilation error in show.rs due to a temporary value lifetime issue, which can be resolved using std::borrow::Cow. Additionally, there is a correctness bug in iptun.rs where encapsulation flags overwrite each other instead of accumulating, and a behavior mismatch where the path MTU discovery fallback should default to enabled to match iproute2. A minor typo was also noted in AGENTS.md.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
88
to
98
| let display_name = if let Some(link_name) = &self.link { | ||
| link_name | ||
| } else if let Some(link_index) = self.link_index { | ||
| &format!("if{link_index}") | ||
| if link_index == 0 { | ||
| "NONE" | ||
| } else { | ||
| &format!("if{link_index}") | ||
| } | ||
| } else { | ||
| "NONE" | ||
| }; |
There was a problem hiding this comment.
This nested if expression inside the else if block will cause a compilation error: temporary value dropped while still in use.
In Rust, temporary lifetime extension only applies to the immediate initializing expression of a let binding. Because the format! macro is now nested inside an inner if/else block, the temporary String is dropped at the end of the else block, leaving display_name as a dangling reference.
To fix this, we can use std::borrow::Cow<'_: str> to represent either a borrowed or owned string, which avoids unnecessary allocations while resolving the lifetime issue.
Suggested change
| let display_name = if let Some(link_name) = &self.link { | |
| link_name | |
| } else if let Some(link_index) = self.link_index { | |
| &format!("if{link_index}") | |
| if link_index == 0 { | |
| "NONE" | |
| } else { | |
| &format!("if{link_index}") | |
| } | |
| } else { | |
| "NONE" | |
| }; | |
| let display_name = if let Some(link_name) = &self.link { | |
| std::borrow::Cow::Borrowed(link_name.as_str()) | |
| } else if let Some(link_index) = self.link_index { | |
| if link_index == 0 { | |
| std::borrow::Cow::Borrowed("NONE") | |
| } else { | |
| std::borrow::Cow::Owned(format!("if{link_index}")) | |
| } | |
| } else { | |
| std::borrow::Cow::Borrowed("NONE") | |
| }; |
Comment on lines
+235
to
+402
| let mut builder = LinkIpIp::new(&self.name); | ||
| let mut metadata = false; | ||
|
|
||
| let mut iter = self.iface_specific.iter(); | ||
| while let Some(key) = iter.next() { | ||
| let mut next_val = || { | ||
| iter.next().ok_or_else(|| { | ||
| CliError::from(format!("ipip {key} requires a value")) | ||
| }) | ||
| }; | ||
| match key.as_str() { | ||
| "local" => { | ||
| let v = next_val()?; | ||
| let addr: Ipv4Addr = parse_ip(v, "local")?; | ||
| builder = builder.local(addr); | ||
| } | ||
| "remote" => { | ||
| let v = next_val()?; | ||
| let addr: Ipv4Addr = parse_ip(v, "remote")?; | ||
| builder = builder.remote(addr); | ||
| } | ||
| "dev" => { | ||
| let v = next_val()?; | ||
| let ifindex = self.get_ifindex_by_name(handle, v).await?; | ||
| builder = builder.dev(ifindex); | ||
| } | ||
| "ttl" | "hoplimit" | "hlim" => { | ||
| let v = next_val()?; | ||
| match v.as_str() { | ||
| "inherit" => { | ||
| builder = builder.ttl(0); | ||
| } | ||
| _ => { | ||
| let ttl: u8 = v.parse().map_err(|_| { | ||
| CliError::from(format!("invalid TTL: {v}")) | ||
| })?; | ||
| builder = builder.ttl(ttl); | ||
| } | ||
| } | ||
| } | ||
| "tos" | "tclass" | "tc" | "dsfield" => { | ||
| let v = next_val()?; | ||
| match v.as_str() { | ||
| "inherit" => { | ||
| builder = builder.tos(1); | ||
| } | ||
| _ => { | ||
| let tos: u8 = parse_dsfield(v)?; | ||
| builder = builder.tos(tos); | ||
| } | ||
| } | ||
| } | ||
| "pmtudisc" => { | ||
| builder = builder.pmtudisc(true); | ||
| } | ||
| "nopmtudisc" => { | ||
| builder = builder.pmtudisc(false); | ||
| } | ||
| "mode" => { | ||
| let v = next_val()?; | ||
| match v.as_str() { | ||
| "ipip" | "ipv4/ipv4" | "ip4ip4" => { | ||
| builder = builder.protocol(IpProtocol::Ipip); | ||
| } | ||
| "mplsip" | "mpls/ipv4" => { | ||
| let proto = IpProtocol::from(137u8); | ||
| builder = builder.protocol(proto); | ||
| } | ||
| "any" | "any/ipv4" => { | ||
| let proto = IpProtocol::from(0u8); | ||
| builder = builder.protocol(proto); | ||
| } | ||
| _ => { | ||
| return Err(CliError::from(format!( | ||
| "Cannot guess tunnel mode: {v}" | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
| "external" => { | ||
| metadata = true; | ||
| } | ||
| "noencap" => { | ||
| builder = builder.encap_type(TunnelEncapType::None); | ||
| } | ||
| "encap" => { | ||
| let v = next_val()?; | ||
| match v.as_str() { | ||
| "fou" => { | ||
| builder = builder.encap_type(TunnelEncapType::Fou); | ||
| } | ||
| "gue" => { | ||
| builder = builder.encap_type(TunnelEncapType::Gue); | ||
| } | ||
| "none" => { | ||
| builder = builder.encap_type(TunnelEncapType::None); | ||
| } | ||
| _ => { | ||
| return Err(CliError::from(format!( | ||
| "Invalid encap type: {v}" | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
| "encap-sport" => { | ||
| let v = next_val()?; | ||
| if v == "auto" { | ||
| builder = builder.encap_sport(0); | ||
| } else { | ||
| let port = parse_u16(v, "encap-sport")?; | ||
| builder = builder.encap_sport(port); | ||
| } | ||
| } | ||
| "encap-dport" => { | ||
| let v = next_val()?; | ||
| let port = parse_u16(v, "encap-dport")?; | ||
| builder = builder.encap_dport(port); | ||
| } | ||
| "encap-csum" => { | ||
| let flags = TunnelEncapFlags::CSum; | ||
| builder = builder.encap_flags(flags); | ||
| } | ||
| "noencap-csum" => { | ||
| let flags = TunnelEncapFlags::empty(); | ||
| builder = builder.encap_flags(flags); | ||
| } | ||
| "encap-udp6-csum" => { | ||
| let flags = TunnelEncapFlags::CSum6; | ||
| builder = builder.encap_flags(flags); | ||
| } | ||
| "noencap-udp6-csum" => { | ||
| let flags = TunnelEncapFlags::empty(); | ||
| builder = builder.encap_flags(flags); | ||
| } | ||
| "encap-remcsum" => { | ||
| let flags = TunnelEncapFlags::RemCSum; | ||
| builder = builder.encap_flags(flags); | ||
| } | ||
| "noencap-remcsum" => { | ||
| let flags = TunnelEncapFlags::empty(); | ||
| builder = builder.encap_flags(flags); | ||
| } | ||
| "fwmark" => { | ||
| let v = next_val()?; | ||
| let mark = if let Some(hex) = v.strip_prefix("0x") { | ||
| u32::from_str_radix(hex, 16) | ||
| } else { | ||
| v.parse() | ||
| }; | ||
| let mark = mark.map_err(|_| { | ||
| CliError::from(format!("invalid fwmark: {v}")) | ||
| })?; | ||
| builder = builder.fwmark(mark); | ||
| } | ||
| _ => { | ||
| return Err(CliError::from(format!( | ||
| "Unknown ipip argument: {key}" | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if metadata { | ||
| builder = builder.collect_metadata(true); | ||
| } | ||
|
|
||
| Ok(builder) | ||
| } |
There was a problem hiding this comment.
There is a correctness bug in how encapsulation flags are handled.
Currently, each encapsulation flag option (e.g., encap-csum, encap-udp6-csum, encap-remcsum) completely overwrites the previous flags because builder.encap_flags(flags) replaces the entire flags field rather than combining them. Additionally, the noencap-* options clear all flags by setting them to TunnelEncapFlags::empty(), rather than just clearing the specific flag.
To fix this, we should accumulate the flags in a local encap_flags variable and apply them to the builder after the loop.
let mut builder = LinkIpIp::new(&self.name);
let mut metadata = false;
let mut encap_flags = TunnelEncapFlags::empty();
let mut has_encap_flags = false;
let mut iter = self.iface_specific.iter();
while let Some(key) = iter.next() {
let mut next_val = || {
iter.next().ok_or_else(|| {
CliError::from(format!("ipip {key} requires a value"))
})
};
match key.as_str() {
"local" => {
let v = next_val()?;
let addr: Ipv4Addr = parse_ip(v, "local")?;
builder = builder.local(addr);
}
"remote" => {
let v = next_val()?;
let addr: Ipv4Addr = parse_ip(v, "remote")?;
builder = builder.remote(addr);
}
"dev" => {
let v = next_val()?;
let ifindex = self.get_ifindex_by_name(handle, v).await?;
builder = builder.dev(ifindex);
}
"ttl" | "hoplimit" | "hlim" => {
let v = next_val()?;
match v.as_str() {
"inherit" => {
builder = builder.ttl(0);
}
_ => {
let ttl: u8 = v.parse().map_err(|_| {
CliError::from(format!("invalid TTL: {v}"))
})?;
builder = builder.ttl(ttl);
}
}
}
"tos" | "tclass" | "tc" | "dsfield" => {
let v = next_val()?;
match v.as_str() {
"inherit" => {
builder = builder.tos(1);
}
_ => {
let tos: u8 = parse_dsfield(v)?;
builder = builder.tos(tos);
}
}
}
"pmtudisc" => {
builder = builder.pmtudisc(true);
}
"nopmtudisc" => {
builder = builder.pmtudisc(false);
}
"mode" => {
let v = next_val()?;
match v.as_str() {
"ipip" | "ipv4/ipv4" | "ip4ip4" => {
builder = builder.protocol(IpProtocol::Ipip);
}
"mplsip" | "mpls/ipv4" => {
let proto = IpProtocol::from(137u8);
builder = builder.protocol(proto);
}
"any" | "any/ipv4" => {
let proto = IpProtocol::from(0u8);
builder = builder.protocol(proto);
}
_ => {
return Err(CliError::from(format!(
"Cannot guess tunnel mode: {v}"
)));
}
}
}
"external" => {
metadata = true;
}
"noencap" => {
builder = builder.encap_type(TunnelEncapType::None);
}
"encap" => {
let v = next_val()?;
match v.as_str() {
"fou" => {
builder = builder.encap_type(TunnelEncapType::Fou);
}
"gue" => {
builder = builder.encap_type(TunnelEncapType::Gue);
}
"none" => {
builder = builder.encap_type(TunnelEncapType::None);
}
_ => {
return Err(CliError::from(format!(
"Invalid encap type: {v}"
)));
}
}
}
"encap-sport" => {
let v = next_val()?;
if v == "auto" {
builder = builder.encap_sport(0);
} else {
let port = parse_u16(v, "encap-sport")?;
builder = builder.encap_sport(port);
}
}
"encap-dport" => {
let v = next_val()?;
let port = parse_u16(v, "encap-dport")?;
builder = builder.encap_dport(port);
}
"encap-csum" => {
encap_flags.insert(TunnelEncapFlags::CSum);
has_encap_flags = true;
}
"noencap-csum" => {
encap_flags.remove(TunnelEncapFlags::CSum);
has_encap_flags = true;
}
"encap-udp6-csum" => {
encap_flags.insert(TunnelEncapFlags::CSum6);
has_encap_flags = true;
}
"noencap-udp6-csum" => {
encap_flags.remove(TunnelEncapFlags::CSum6);
has_encap_flags = true;
}
"encap-remcsum" => {
encap_flags.insert(TunnelEncapFlags::RemCSum);
has_encap_flags = true;
}
"noencap-remcsum" => {
encap_flags.remove(TunnelEncapFlags::RemCSum);
has_encap_flags = true;
}
"fwmark" => {
let v = next_val()?;
let mark = if let Some(hex) = v.strip_prefix("0x") {
u32::from_str_radix(hex, 16)
} else {
v.parse()
};
let mark = mark.map_err(|_| {
CliError::from(format!("invalid fwmark: {v}"))
})?;
builder = builder.fwmark(mark);
}
_ => {
return Err(CliError::from(format!(
"Unknown ipip argument: {key}"
)));
}
}
}
if has_encap_flags {
builder = builder.encap_flags(encap_flags);
}
if metadata {
builder = builder.collect_metadata(true);
}
Ok(builder)
Comment on lines
+180
to
+188
| if let Some(pmtudisc) = self.pmtudisc { | ||
| if pmtudisc { | ||
| emit!("pmtudisc"); | ||
| } else { | ||
| emit!("nopmtudisc"); | ||
| } | ||
| } else { | ||
| emit!("nopmtudisc"); | ||
| } |
There was a problem hiding this comment.
In iproute2, if the IFLA_IPTUN_PMTUDISC attribute is missing, it defaults to enabling path MTU discovery (pmtudisc).
Currently, the fallback defaults to nopmtudisc when self.pmtudisc is None. We should change this to pmtudisc to match the behavior of iproute2.
Suggested change
| if let Some(pmtudisc) = self.pmtudisc { | |
| if pmtudisc { | |
| emit!("pmtudisc"); | |
| } else { | |
| emit!("nopmtudisc"); | |
| } | |
| } else { | |
| emit!("nopmtudisc"); | |
| } | |
| if let Some(pmtudisc) = self.pmtudisc { | |
| if pmtudisc { | |
| emit!("pmtudisc"); | |
| } else { | |
| emit!("nopmtudisc"); | |
| } | |
| } else { | |
| emit!("pmtudisc"); | |
| } |
|
|
||
| * Netlink attribute parsing and emitting code should be done by other | ||
| rust-netlink crates which are also locally maintained, change code in | ||
| local folder and use local folder temporally before upstream merge. |
There was a problem hiding this comment.
Typo: "temporally" should be "temporarily".
Suggested change
| local folder and use local folder temporally before upstream merge. | |
| local folder and use local folder temporarily before upstream merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add integration tests covering all ipip tunnel options.