/assign @samuelkarp

https://github.com/opencontainers/runtime-spec/blob/main/features.md should be updated too

https://github.com/opencontainers/runtime-spec/blob/main/features.md should be updated too

updated and addressed the comments

AI @aojea (document the cleanup and destroy of the network interfaces)

From the in-person discussion today:

• Net device lifecycle should follow the network namespace lifecycle
• @aojea will follow up to determine whether any cleanup actions need to be taken by the OCI runtime on a container being deleted
• @kad was concerned about restarts and error handling
• Should we prohibit the new netdev addition to an existing netns? IOW only allow this for containers where a new netns is created? What about containers where the root netns is used?

om the in-person discussion today:

• Net device lifecycle should follow the network namespace lifecycle
• @aojea will follow up to determine whether any cleanup actions need to be taken by the OCI runtime on a container being deleted
• @kad was concerned about restarts and error handling
• Should we prohibit the new netdev addition to an existing netns? IOW only allow this for containers where a new netns is created? What about containers where the root netns is used?

Pushed a new commit addressing those comments, the changelog is

• the network namespace lifecycle will move migratebale network devices and destroy virtual devides, the runtime MAY decide to do cleanup actions
• runtime MUST check the container has enough privileges and an associated network namespace and fail if the check fail
• removed the Mask field and use the Address field with CIDR notation (IP/Prefix) to deal with IPv4 and IPv6 addresses. Only one IP is allowed to be specified on purpose to simplify the operations and reduce risks
• Add a HardwareAddress field for use cases that require to set a
  specific mac or infiniband address

@aojea can you squash the commits? Otherwise LGTM, and this is definitely something we can build upon.

@aojea can you squash the commits? Otherwise LGTM, and this is definitely something we can build upon.

@kolyshkin squashed, thanks

for things that are easily available via few netlink api calls, I don't see any big reasons why it be too complex to implement in runtime

The problem in my mind is where do you draw the line? At what point does "a few netlink api calls" become "a few too many" and now it's no longer acceptable?

I agree with Kir that we should keep the scope really limited here so we don't accidentally stray too far into network management in the low level runtime, which is something we've traditionally left as a higher level concern for a lot of good reasons.

One more comment -- why is the data structure a map? A map makes sense when we want to quickly find an element based on a key. Is it needed here?

Perhaps a simple array is easier.

One more comment -- why is the data structure a map? A map makes sense when we want to quickly find an element based on a key. Is it needed here?

interfaces names are unique per namespace, a map guarantees this avoiding user fat finger problems or misconfiguration, otherwise we need to validate that the names in the array are unique

interfaces names are unique per namespace, a map guarantees this avoiding user fat finger problems or misconfiguration, otherwise we need to validate that the names in the array are unique

I don't think the uniqueness requirement justifies using map, and I don't think we should be concerned much about uniqueness. In case a network device will be listed twice, the second entry will fail with "no such device" because the first one will already be moved.

Also, we have other similar fields (Devices, Namespaces, Mounts etc.) as arrays. I see that time namespace offsets is implemented via maps (and I think it was a mistake, for similar reasons -- an array would work just fine).

Having said that, this is not a deal breaker. Also, I'm sure other maintainers may have different opinions. WDYT @opencontainers/runtime-spec-maintainers ?

I agree with Kir that we should keep the scope really limited here so we don't accidentally stray too far into network management in the low level runtime, which is something we've traditionally left as a higher level concern for a lot of good reasons.

With the IP configuration part now removed, am I correct that the "easy" solution configuring IP networking for additional interfaces is still via a custom runtime hook? In our case the containers are static and cannot be amended with IP configuration tools or the necessary networking capabilities, and I doubt we are able to inject another container into the pod without causing a stir. IP (and yes, a route configuration) would have been a good and standard solution for simple secondary network needs.

for things that are easily available via few netlink api calls, I don't see any big reasons why it be too complex to implement in runtime

The problem in my mind is where do you draw the line? At what point does "a few netlink api calls" become "a few too many" and now it's no longer acceptable?

My "border line" is about features and fields provided by kernel netlink api. If something is done with it, I don't see big difference between call "move interface xyz to namespace abc" vs. "set property foo to value bar on interface xyz in namespace abc". Or to what it worth, "put value 123 into cgroup foobar".
Things like DHCP that require more than plain kernel api call are clearly outside of the boundaries that can be implemented in runtime.

for things that are easily available via few netlink api calls, I don't see any big reasons why it be too complex to implement in runtime

The problem in my mind is where do you draw the line? At what point does "a few netlink api calls" become "a few too many" and now it's no longer acceptable?

My "border line" is about features and fields provided by kernel netlink api. If something is done with it, I don't see big difference between call "move interface xyz to namespace abc" vs. "set property foo to value bar on interface xyz in namespace abc". Or to what it worth, "put value 123 into cgroup foobar". Things like DHCP that require more than plain kernel api call are clearly outside of the boundaries that can be implemented in runtime.

To me, this sounds like a runtime should implement most of what ip(8) is capable of. Which is a lot.

If that's the case, here's a crazy idea.

Considering that ip(8) is a de facto standard on Linux, perhaps such configuration can be achieved by supplying a sequence of ip command arguments in runtime-spec. A runtime when can use ip binary on the host to get to the needed configuration.

Something like

"linux": {
        "netDevices": [
            "name": "enp34s0u2u1u2",
            "ct_name": "eth0",
            "config": [
                "ip addr add 10.2.3.4/25 dev $IF",
                "ip link set $IF mtu 1350",
                "ip route add 10.10.0.0/16 via 10.2.3.99 dev $IF proto static metric 100"
                "ip route add default dev $IF"
            ]
        ]
    }

Which will result in runtime running the host's ip binary in the container's network namespace (after moving the device into container's namespace and giving it a new name). It can even be a single ip invocation if -b (batch mode) is supported.

The upsides are:

• this uses a well known, de facto standard "domain language" for network-related configuration;
• this allows for a quite complex configuration to be performed (limited to whatever ip understands);
• this seems trivial to implement for any runtime;
• this doesn't impose any specific container image requirements (no need for any extra binaries etc.);
• this should be secure (as ip is running inside a netns);
• some advanced runtimes can implement a subset of this internally (i.e. without the call to ip);

The biggest downside, I guess, is the scope is not well defined, as ip is evolving.

I don't like the idea of execing commands, specially in golang implementations that are problematic with goroutines and namespaces
I think that once we have just the network interfaces on the runtimes we'll get much better feedback and signal from the implementations to evaluate the next steps and to know if something else is needed and what exactly

I still think that preventing to detect failures at runtime is desirable, you can argue you should not add duplicates, but bugs exist and if somebody accidentally add a duplicate interface name it will cause a problem in production we could have avoided just not allowing to define it

Considering that ip(8) is a de facto standard on Linux, perhaps such configuration can be achieved by supplying a sequence of ip command arguments in runtime-spec. A runtime when can use ip binary on the host to get to the needed configuration.

I believe that this is really bad idea, compared to have limited set of config options implemented. exec() is always slower and less reliable than calling native kernel APIs. It opens doors to all various injection vulnerabilities and other potential misuses and scope creeps than strict API subset usage. As well as Antonio mentioned proper error handling.

To provide more insights on our use case, here is snip of the code that we want to get rid of, currently implemented as OCI runtime hook (btw, including already unreliable call to ip in it): https://github.com/HabanaAI/habana-container-runtime/blob/main/cmd/habana-container-cli/netdevices.go#L131-L182

Let me move the milestone from vNext (v1.2.1?) to vNextNext (v1.3.0?)

Do you have an aprox estimation on how long can take 1.3.0? I have some dependencies on this feature and it will be nice to be able to account for that time

@AkihiroSuda It looks like 1.2.1 got tagged yesterday: https://github.com/opencontainers/runtime-spec/releases/tag/v1.2.1. Is there anything blocking the merge of this PR into main and setting it up for the 1.3.0 release?

I still don't think it's appropriate to expect the runtime to set up the network interfaces and believe we're opening a can of worms here, but not strongly enough to block it outright (especially with Kir approving it, and thus the implied maintenance of it in runc). ❤️

I still don't think it's appropriate to expect the runtime to set up the network interfaces and believe we're opening a can of worms here,

I don't see risk meanwhile we stick to network interfaces, the moment we leak networks , we start to create runtimes dependencies and I fully agree with you. That is also the reason why the last proposal removed entirely the IP initialization bits #1271 (comment), I have some ideas on how to solve this problem without modifying the spec, but my priority now is to solve the existing problem in the ecosystem.

The problem this solves is that today there is hardware that needs to use network interfaces, mainly GPUs (but there are other cases). Since there is no way to declaratively move these interfaces to the container, everyone solves the problem from a different angle, by hooking into the Pod/Container network namespace creation using:

• CNI plugins
• OCI hooks
• out of band channels using the Kubernetes API directly
• ...

All these solutions are brittle , requires high privileges that exposes security problems, and are hard to troubleshoot since run in different places during the container creation and cause fragmentation in the ecosystem and bad user experience.

The goal here is that developers can just patch the OCI spec to say "move eth2 to this container with /dev/gpu0" simple to do with CDI and NRI and no need for extra privileges ... I personally don't feel that meanwhile we stick to this definition, we will open that pandora box that is the network (that non of us want)

@aojea I ignore this, but how does this works in practice? Is it expected that the host network interface will be configured by the host (i.e. IP address, mtu, etc.) and then moved into the container? All of the configuration and all "just works" when moved into the container or CNI or some other entity will need to do something?

The goal is to decouple the interface lifecycle and configuration from the oci runtime, that is the part that SHOULD NOT be handled by the OCI runtimes or actors of the pod/container creation. I think there are two scenarios:

• interface admin preconfigure the interface before moving it and runc moves it as is. This is already the case on most cases since interfaces use to be part of the host. For those cases that interfaces are hotplaggubale or are dynamically created, the existing preprovisioning hooks in DRA NodePrepareResources can do that, then it is just indicate the OCI runtime to move it inside the container.
• container application configure the interface. We are already doing this with VMs, see cloud-init and network configuration, there are also use cases where the interface is going to be consumed directly by the application and exposed as configureable to the users, see telco use cases that run router or firewall like apps in the container.

Okay, we were talking over slack and there are two things that we think we still need to answer:

1. Is the configuration (address, mtu, etc.) lost when moved to the container netns?
2. For the cases that we need to configure the interface, how can we expect the configuration to happen?

I ask 2. for two reasons: a) be clear that is the concern of another part of the stack; b) understand how this will be used and understand if nothing else is missing here (i.e. if CNI is expected to handle it, make sure it is not running too late, or it has all the info to realize there is an extra interface to configure, if another component is expected to handle, see that it can, etc.)

Thanks @rata for your help today, I updated the PR to implement it in runc with integration tests that show the behavior

1. Is the configuration (address, mtu, etc.) lost when moved to the container netns?
2. For the cases that we need to configure the interface, how can we expect the configuration to happen?

the interface configuration is preserved, so users can set down the interface in the host namespace, configure the interface (ip address, mtu, hw address) and the runtime will move it to the network namespace maintaining that configuration, this removes the need to include network configuration in the runtime and allow for implementations to use the preparation of the device to configure it without risks (kubernetes use case)

Users can still decide to use a process inside the container to configure the network configuration, use dhcp or some sort of bootstrap ala cloud-init

rebased and added this last requirement to preserve the network config

iff --git a/config-linux.md b/config-linux.md
index 6682e16..1f0e808 100644
--- a/config-linux.md
+++ b/config-linux.md
@@ -201,6 +201,8 @@ This schema focuses solely on moving existing network devices identified by name
 
 The runtime MUST check that is possible to move the network interface to the container namespace and MUST [generate an error](runtime.md#errors) if the check fails.
 
+The runtime MUST preserve the existing network interface attributes, like MTU, MAC and IP addresses, enabling users to preconfigure the interfaces.
+
 The runtime MUST set the network device state to "up" after moving it to the network namespace to allow the container to send and receive network traffic through that device.