fix(encapsulation): use cilium_tunl for IPIP tunnel in Cilium mode#8
fix(encapsulation): use cilium_tunl for IPIP tunnel in Cilium mode#8
Conversation
When Cilium's enable-ipip-termination is active, it renames the kernel's tunl0 to cilium_tunl and creates a receive-only cilium_ipip4 device for DSR. The cilium_ipip4 interface cannot transmit packets (TX errors), so use cilium_tunl which supports both TX and RX. If cilium_tunl already exists (Cilium manages it), reuse it. Otherwise, create it so the interface name is consistent regardless of whether enable-ipip-termination is active. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
📝 WalkthroughWalkthroughThe changes introduce conditional tunnel ownership handling to Cilium's encapsulation module. Cilium now detects existing kernel Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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 |
Summary of ChangesHello @kvaps, 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 resolves an issue with IPIP tunnel encapsulation in Cilium compatibility mode by ensuring that the correct network interface, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
![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
The pull request correctly addresses the issue where cilium_ipip4 was unable to transmit packets in Cilium mode by switching to cilium_tunl. It also introduces a necessary check for interface ownership to prevent Kilo from destructively cleaning up interfaces managed by Cilium. However, there is a significant bug in the Init method of the cilium encapsulator: when an existing interface is reused, the code returns early, skipping critical configuration steps such as setting the MTU and ensuring the interface is administratively UP. This could lead to connectivity issues or suboptimal performance.
| if link, err := netlink.LinkByName(ciliumTunlIface); err == nil { | ||
| c.iface = link.Attrs().Index | ||
| c.ownsTunnel = false | ||
| return nil | ||
| } | ||
| iface, err := iproute.NewIPIPWithName(base, ciliumTunlIface) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create tunnel interface: %v", err) | ||
| } | ||
| if err := iproute.Set(iface, true); err != nil { | ||
| return fmt.Errorf("failed to set tunnel interface up: %v", err) | ||
| } | ||
| c.iface = iface | ||
| c.ownsTunnel = true | ||
| return nil |
There was a problem hiding this comment.
The current implementation of Init returns early if the cilium_tunl interface already exists. This bypasses the MTU configuration (performed inside NewIPIPWithName) and the call to iproute.Set(iface, true), which ensures the interface is UP. Even when reusing an existing interface, Kilo should ensure it is correctly configured for its use case.
Additionally, the ownership check can be integrated more cleanly to ensure the rest of the initialization logic always runs.
_, err := netlink.LinkByName(ciliumTunlIface)
c.ownsTunnel = err != nil
iface, err := iproute.NewIPIPWithName(base, ciliumTunlIface)
if err != nil {
return fmt.Errorf("failed to create tunnel interface: %v", err)
}
if err := iproute.Set(iface, true); err != nil {
return fmt.Errorf("failed to set tunnel interface up: %v", err)
}
c.iface = iface
return nil![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/encapsulation/cilium.go (1)
48-57:⚠️ Potential issue | 🟠 MajorClean up addresses even when reusing a Cilium-owned tunnel.
When Kilo reuses an existing
cilium_tunlinterface (ownsTunnelis false),Set()still adds an IP address to the interface. However,CleanUp()returns immediately without callingiproute.DeleteAddresses(), leaving Kilo-assigned addresses on the shared tunnel after shutdown. This causes a resource leak and could lead to routing conflicts on restart.The fix is to always clean up addresses, but skip interface removal only when the tunnel is not owned:
Proposed fix
func (c *cilium) CleanUp() error { - if !c.ownsTunnel { - return nil - } + if !c.ownsTunnel { + return iproute.DeleteAddresses(c.iface) + } if err := iproute.DeleteAddresses(c.iface); err != nil { return err } return iproute.RemoveInterface(c.iface) }
🤖 Fix all issues with AI agents
In `@pkg/encapsulation/cilium.go`:
- Around line 99-114: When Init finds an existing cilium tunnel (the branch that
sees netlink.LinkByName(ciliumTunlIface)), also bring the link up and ensure its
MTU matches base-20 before returning: set c.iface as you do now, then call the
same helpers used in the creation path (iproute.Set to bring the link up and the
appropriate iproute MTU setter to set MTU = base - 20), and only then set
c.ownsTunnel=false and return; reuse ciliumTunlIface, Init, iproute.Set and the
MTU-setting helper names to locate where to add these calls.
🧹 Nitpick comments (1)
pkg/iproute/ipip.go (1)
39-55: Potential issue:nameparameter is not sanitized before passing to shell command.Line 43 passes
namedirectly intoexec.Command("ip", "tunnel", "add", name, ...). Sinceexec.Commanduses argument-based invocation (not shell expansion), this is safe from injection. However, invalid interface names (e.g., names exceedingIFNAMSIZof 15 chars, or containing/) would produce confusing errors. Currently all callers pass compile-time constants (DefaultTunnelName,ciliumTunlIface), so the practical risk is negligible.
| func (c *cilium) Init(base int) error { | ||
| iface, err := iproute.NewIPIP(base) | ||
| if link, err := netlink.LinkByName(ciliumTunlIface); err == nil { | ||
| c.iface = link.Attrs().Index | ||
| c.ownsTunnel = false | ||
| return nil | ||
| } | ||
| iface, err := iproute.NewIPIPWithName(base, ciliumTunlIface) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create tunnel interface: %v", err) | ||
| } | ||
| if err := iproute.Set(iface, true); err != nil { | ||
| return fmt.Errorf("failed to set tunnel interface up: %v", err) | ||
| } | ||
| c.iface = iface | ||
| c.ownsTunnel = true | ||
| return nil |
There was a problem hiding this comment.
Reused tunnel skips MTU adjustment and link-up.
When cilium_tunl already exists (lines 100–104), Init sets c.iface and returns immediately without:
- Ensuring the link is administratively up (
iproute.Set(iface, true)). - Setting the MTU to
base MTU − 20.
If Cilium has already configured and brought up the tunnel, this is fine in practice. However, if the MTU of the base interface differs from what Cilium assumed, the tunnel MTU could be mismatched, leading to packet drops or fragmentation on the IPIP path.
Consider at minimum setting the MTU on the reused tunnel to stay consistent with the creation path.
Suggested enhancement
func (c *cilium) Init(base int) error {
if link, err := netlink.LinkByName(ciliumTunlIface); err == nil {
c.iface = link.Attrs().Index
c.ownsTunnel = false
+ // Ensure the MTU matches what Kilo expects.
+ baseLink, err := netlink.LinkByIndex(base)
+ if err != nil {
+ return fmt.Errorf("failed to get base device: %v", err)
+ }
+ mtu := baseLink.Attrs().MTU - 20
+ if err := netlink.LinkSetMTU(link, mtu); err != nil {
+ return fmt.Errorf("failed to set tunnel MTU: %v", err)
+ }
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *cilium) Init(base int) error { | |
| iface, err := iproute.NewIPIP(base) | |
| if link, err := netlink.LinkByName(ciliumTunlIface); err == nil { | |
| c.iface = link.Attrs().Index | |
| c.ownsTunnel = false | |
| return nil | |
| } | |
| iface, err := iproute.NewIPIPWithName(base, ciliumTunlIface) | |
| if err != nil { | |
| return fmt.Errorf("failed to create tunnel interface: %v", err) | |
| } | |
| if err := iproute.Set(iface, true); err != nil { | |
| return fmt.Errorf("failed to set tunnel interface up: %v", err) | |
| } | |
| c.iface = iface | |
| c.ownsTunnel = true | |
| return nil | |
| func (c *cilium) Init(base int) error { | |
| if link, err := netlink.LinkByName(ciliumTunlIface); err == nil { | |
| c.iface = link.Attrs().Index | |
| c.ownsTunnel = false | |
| // Ensure the MTU matches what Kilo expects. | |
| baseLink, err := netlink.LinkByIndex(base) | |
| if err != nil { | |
| return fmt.Errorf("failed to get base device: %v", err) | |
| } | |
| mtu := baseLink.Attrs().MTU - 20 | |
| if err := netlink.LinkSetMTU(link, mtu); err != nil { | |
| return fmt.Errorf("failed to set tunnel MTU: %v", err) | |
| } | |
| return nil | |
| } | |
| iface, err := iproute.NewIPIPWithName(base, ciliumTunlIface) | |
| if err != nil { | |
| return fmt.Errorf("failed to create tunnel interface: %v", err) | |
| } | |
| if err := iproute.Set(iface, true); err != nil { | |
| return fmt.Errorf("failed to set tunnel interface up: %v", err) | |
| } | |
| c.iface = iface | |
| c.ownsTunnel = true | |
| return nil |
🤖 Prompt for AI Agents
In `@pkg/encapsulation/cilium.go` around lines 99 - 114, When Init finds an
existing cilium tunnel (the branch that sees
netlink.LinkByName(ciliumTunlIface)), also bring the link up and ensure its MTU
matches base-20 before returning: set c.iface as you do now, then call the same
helpers used in the creation path (iproute.Set to bring the link up and the
appropriate iproute MTU setter to set MTU = base - 20), and only then set
c.ownsTunnel=false and return; reuse ciliumTunlIface, Init, iproute.Set and the
MTU-setting helper names to locate where to add these calls.
1 participant
Summary
cilium_tunlinstead ofcilium_ipip4for IPIP encapsulation in Cilium compatibility modeenable-ipip-terminationis active,cilium_ipip4is a receive-only device (DSR) that cannot transmit packets.cilium_tunl(the renamedtunl0) supports both TX and RXcilium_tunlalready exists (managed by Cilium), reuse it; otherwise create it as a named IPIP tunnelNewIPIPWithNamehelper to support creating IPIP tunnels with custom namesTest plan
cilium_ipip4shows TX errors (0 packets sent, thousands of errors)cilium_tunltransmits successfully (both TX and RX counters incrementing)kubectl execon worker nodes through IPIP+VxLAN path — works withcilium_tunlSummary by CodeRabbit
Release Notes
Bug Fixes
New Features