host-device: copy host interface IP addresses and routes into container - #1257
host-device: copy host interface IP addresses and routes into container#1257SchSeba wants to merge 1 commit into
Conversation
0feea32 to
df398aa
Compare
|
Hi @s1061123 @squeed @LionelJouin if you have time please take a look on the PR. |
| localRouteTable = 255 | ||
| ) | ||
|
|
||
| // HostNetworkStateFile holds the captured host-side L3 configuration |
There was a problem hiding this comment.
personal preference no comment unless exported
There was a problem hiding this comment.
Done - removed doc comments from all unexported symbols.
|
|
||
| // HostNetworkStateFile holds the captured host-side L3 configuration | ||
| // (addresses, routes, and rules) that should be applied to the container interface. | ||
| type HostNetworkStateFile struct { |
There was a problem hiding this comment.
prefix ^File$ not very nice not really a file. May InterfaceInfo, InterfaceConfig, or just Interface
There was a problem hiding this comment.
Good call - renamed HostNetworkStateFile to HostNetworkState throughout.
| type HostNetworkStateFile struct { | ||
| HostIfName string `json:"hostIfName"` | ||
| HostLinkWasUp bool `json:"hostLinkWasUp"` | ||
| Addresses []string `json:"addresses,omitempty"` |
There was a problem hiding this comment.
can we use netlink.Addr, netlink.routes, rule
There was a problem hiding this comment.
The string-based representation is intentional here - netlink.Addr, netlink.Route and netlink.Rule contain net.IP / *net.IPNet fields that don't round-trip cleanly through JSON (net.IP marshals as a base64 byte array, *net.IPNet isn't directly marshalable). Using strings gives us portable, human-readable JSON and avoids coupling the serialization format to the netlink library's internal types.
We do convert back to netlink types when applying (applyOnLink), so the actual netlink interaction is the same.
There was a problem hiding this comment.
I still do not think we need serialization or string
| ) | ||
|
|
||
| // TestUseInterfaceNetwork verifies useInterfaceNetwork boolean behavior. | ||
| func TestUseInterfaceNetwork(t *testing.T) { |
There was a problem hiding this comment.
what exactly this test is doing?
There was a problem hiding this comment.
Removed this test - it was only validating the trivial boolean guard function which is already covered implicitly by the integration tests.
| } | ||
|
|
||
| // TestStateJSONHasNoNeighbors verifies state serialization excludes neighbors. | ||
| func TestStateJSONHasNoNeighbors(t *testing.T) { |
There was a problem hiding this comment.
what exactly this test is doing? Not sure the tests in the file really add value
There was a problem hiding this comment.
Removed this test as well. Kept the TestMergeNetworkState* and TestLoadConf* tests since those exercise actual logic (result merging, config parsing, DPDK rejection).
df398aa to
20dc60e
Compare
s1061123
left a comment
There was a problem hiding this comment.
This PR introduces new parameter, 'useInterfaceNetwork', so could you please create another PR in https://github.com/containernetworking/cni.dev/pulls to modify host-device CNI document as well?
| RuntimeConfig struct { | ||
| DeviceID string `json:"deviceID,omitempty"` | ||
| } `json:"runtimeConfig,omitempty"` | ||
| UseInterfaceNetwork bool `json:"useInterfaceNetwork,omitempty"` |
There was a problem hiding this comment.
Recommend to add comment to quickly mention what is 'UseInterfaceNetwork' because the option name is not intuitive (what 'useInterfaceNetwork' is used?)
There was a problem hiding this comment.
Done - added an inline comment: // When true, copy the host interface's IP addresses and routes into the container before IPAM runs.
There was a problem hiding this comment.
I also open the PR for the documentation site containernetworking/cni.dev#156
72eec2d to
10936b3
Compare
|
I am a bit unsure if we capture everything that need to be re-applied or if capture something we should not. Some AI generated list which seems possible |
|
Thanks for the thorough review @karampok - addressing your IPv6/SLAAC concerns: Addressed in the latest push:
Out of scope for this PR (can be follow-ups):
Also added IPv6-related filtering (link-local unicast routes were already skipped). |
a2cbe4c to
c7b50c6
Compare
c7b50c6 to
5272625
Compare
karampok
left a comment
There was a problem hiding this comment.
About the DEL and not restoring: this means that once we have pod using this interface and then delete the pod, the secondary interface remains unusable until node reboots. right?
| return fmt.Errorf("failed to find host device: %v", err) | ||
| } | ||
|
|
||
| networkState = &HostNetworkState{ |
There was a problem hiding this comment.
HostNetworkState is partially initialized here from hostDev (name, link-up flag), then hostDev is passed again to captureHostNetworkState which extracts more fields from the same object. Consider a single constructor (e.g. newHostNetworkState(hostDev, interfaceNetworkEnabled)) that does all initialization in one place.
| return err | ||
| } | ||
| if cfg.IPAM.Type == "" { | ||
| return printLinkWithNetworkState(contDev, cfg.CNIVersion, containerNs, networkState) |
There was a problem hiding this comment.
This early return for the no-IPAM + useInterfaceNetwork case is buried inside the interfaceNetworkEnabled block. Consider consolidating all no-IPAM exit paths together:
if cfg.IPAM.Type == "" {
if cfg.DPDKMode {
return types.PrintResult(result, cfg.CNIVersion)
}
if interfaceNetworkEnabled {
return printLinkWithNetworkState(contDev, cfg.CNIVersion, containerNs, networkState)
}
return printLink(contDev, cfg.CNIVersion, containerNs)
}The control flow reads top-down: no IPAM? pick the right printer.
| return types.PrintResult(&result, cniVersion) | ||
| } | ||
|
|
||
| func routeStateToCNIRoute(route routeState) *types.Route { |
There was a problem hiding this comment.
routeStateToCNIRoute belongs in host-network-state.go alongside the routeState type definition and the rest of the state handling logic.
| Rules []ruleState `json:"rules,omitempty"` | ||
| } | ||
|
|
||
| type routeState struct { |
There was a problem hiding this comment.
Why is routeState needed as an intermediate type instead of using types.Route directly?
The HostNetworkState is never serialized to JSON -- it is captured in cmdAdd, applied in the same call, and discarded. And routeStateToCNIRoute converts back to types.Route for printing anyway.
The only field routeState has that types.Route lacks is Source. That could be handled with a thin wrapper or by keeping netlink.Route directly for the capture/apply path.
Also, the field naming (Destination/Gateway/Metric) diverges from both CNI (Dst/GW/Priority) and netlink (Dst/Gw/Priority) conventions.
| if route.Protocol == syscall.RTPROT_KERNEL { | ||
| continue | ||
| } | ||
| // Skip RA-learned routes: they carry a kernel-managed lifetime and |
There was a problem hiding this comment.
The comment is incorrect. There is no "RA daemon" on the receiving side -- the kernel processes Router Advertisements natively via the accept_ra sysctl.
When the interface moves into the container namespace, the router on the link continues sending RAs. If accept_ra is enabled in the container netns (kernel default), the kernel will install fresh RA routes with proper lifetimes automatically.
Skipping RA routes is correct, but for the opposite reason: you skip them because the container will get new ones with proper lifetimes, not because it won't.
| return nil | ||
| } | ||
|
|
||
| func isAlreadyExistsErr(err error) bool { |
There was a problem hiding this comment.
The string matching fallback in isAlreadyExistsErr is unnecessary. errors.Is(err, syscall.EEXIST) is sufficient for netlink operations (vishvananda/netlink returns syscall.Errno values). The "object already exists" string does not come from any standard errno path.
Consider the controller-runtime IgnoreAlreadyExists pattern:
func ignoreExists(err error) error {
if errors.Is(err, syscall.EEXIST) {
return nil
}
return err
}Then: return ignoreExists(netlink.AddrAdd(link, &addr)) -- cleaner than the bool check and branch.
| type HostNetworkStateFile struct { | ||
| HostIfName string `json:"hostIfName"` | ||
| HostLinkWasUp bool `json:"hostLinkWasUp"` | ||
| Addresses []string `json:"addresses,omitempty"` |
There was a problem hiding this comment.
I still do not think we need serialization or string
|
Hi @karampok thanks for the comments!
no when the interface is back on the host network namespace networkManager for example sees the device and run the configuration again on the device it can be static,dhcp or what every is convigured in the device |
0f94e10 to
d388d1f
Compare
| } | ||
|
|
||
| for _, rule := range state.Rules { | ||
| nlRule := netlink.NewRule() |
There was a problem hiding this comment.
state.Rules is []netlink.Rule and nlRule is also netlink.Rule -- creating a new rule and copying fields from the same type is redundant leftover from the old ruleState string type. Simplify:
for _, rule := range state.Rules {
if err := ignoreExists(netlink.RuleAdd(&rule)); err != nil {
return fmt.Errorf("failed to add copied rule (src=%v table=%d): %w", rule.Src, rule.Table, err)
}
}
you assume they are all using NM (and auto connect feature) which probably is not true. I would have thought there is a cloudinit script that does once on boot the network config but I have not worked with clouds for many years, I do not have strong opinion here. |
I tested this on two cluster provides for k8s deployments (GCP,IBM cloud) and they both has the same beavior. |
d388d1f to
2f1cd7e
Compare
There was a problem hiding this comment.
nit: looks like you accidentally added a binary file.
cfbf85c to
d14016b
Compare
Add a new configuration option `useInterfaceNetwork` that instructs the
host-device plugin to capture the interface's IP addresses and routes
from the host before moving the device into the container namespace,
and then apply them inside the container.
This is critical for virtual environments (AWS, IBM Cloud, GPC) where
the cloud provider configures IP addresses and routes directly on the
network device. In these environments, there is no traditional IPAM
source; the ground truth for L3 configuration lives on the host
interface itself.
When `useInterfaceNetwork` is enabled, the plugin:
- Captures all global-scope addresses and non-local routes from the
host device before moving it into the container namespace.
- Applies the captured addresses and routes to the interface inside
the container.
- Reports the addresses and routes in the CNI result (merged with
any IPAM result if an IPAM plugin is also configured).
NOTE: The interface configuration on the host node must be persistent.
When the device is moved back to the host (via DEL) and renamed to its
original name, the system's network management service (e.g.
NetworkManager, systemd-networkd, cloud-init, or cloud-specific agents)
is expected to detect the device and re-apply the IP addresses and
routes. This plugin does NOT re-configure the host interface on DEL; it
relies on the node's network configuration being declarative and
reconciled by the platform's networking stack.
Also implements the STATUS command to verify the host device exists,
replacing the previous TODO stub.
Signed-off-by: Sebastian Sch <sebassch@gmail.com>
d14016b to
3aa77ea
Compare
Add a new configuration option
useInterfaceNetworkthat instructs the host-device plugin to capture the interface's IP addresses and routes from the host before moving the device into the container namespace, and then apply them inside the container.This is critical for virtual environments (AWS, IBM Cloud, GPC) where the cloud provider configures IP addresses and routes directly on the network device. In these environments, there is no traditional IPAM source; the ground truth for L3 configuration lives on the host interface itself.
When
useInterfaceNetworkis enabled, the plugin:NOTE: The interface configuration on the host node must be persistent. When the device is moved back to the host (via DEL) and renamed to its original name, the system's network management service (e.g. NetworkManager, systemd-networkd, cloud-init, or cloud-specific agents) is expected to detect the device and re-apply the IP addresses and routes. This plugin does NOT re-configure the host interface on DEL; it relies on the node's network configuration being declarative and reconciled by the platform's networking stack.
Also implements the STATUS command to verify the host device exists, replacing the previous TODO stub.