diff --git a/doc/ChangeLog.md b/doc/ChangeLog.md index b17a6cd4..f9a2ebe4 100644 --- a/doc/ChangeLog.md +++ b/doc/ChangeLog.md @@ -30,6 +30,8 @@ All notable changes to the project are documented in this file. ### Fixes +- Fix #941: a VETH pair can now connect two containers directly, with both + ends assigned to containers. - Enabling IP masquerading in the firewall no longer enables IP forwarding on all interfaces. This has been an issue ever since the firewall support was introduced in v25.10.0 diff --git a/doc/container.md b/doc/container.md index eb6de623..4f3ab2d7 100644 --- a/doc/container.md +++ b/doc/container.md @@ -668,11 +668,9 @@ set: For an example of both, see the next section. -> [!IMPORTANT] -> **VETH Pair Limitation:** When using VETH pairs with containers, at least -> one side of the pair must remain in the host namespace. It is currently -> not possible to create VETH pairs where both ends are assigned to different -> containers. One end must always be accessible from the host. +> [!TIP] +> Both ends of a VETH pair may be assigned to containers, connecting two +> containers directly without involving the host namespace. [^3]: Something which the container bridge network type does behind the scenes with one end of an automatically created VETH pair. diff --git a/src/confd/src/cni.c b/src/confd/src/cni.c index d70a863a..9f7980aa 100644 --- a/src/confd/src/cni.c +++ b/src/confd/src/cni.c @@ -414,6 +414,16 @@ int cni_netdag_gen_iface(struct dagger *net, const char *ifname, return -EIO; fprintf(fp, "container -a -f delete network %s >/dev/null\n", ifname); + + /* If this end belongs to a veth pair, the kernel keeps the pair + * alive after CNI host-device returns the interface to the host + * namespace. Remove it here, once the container is gone, so the + * pair does not linger and block a later re-creation. Tolerant: + * the peer's teardown may already have removed it. + */ + if (lydx_get_child(dif, "veth")) + fprintf(fp, "ip link del dev %s 2>/dev/null || true\n", ifname); + fclose(fp); if (cni_type == IFT_BRIDGE) diff --git a/src/confd/src/if-veth.c b/src/confd/src/if-veth.c index 306c61f2..ce6bdda3 100644 --- a/src/confd/src/if-veth.c +++ b/src/confd/src/if-veth.c @@ -21,23 +21,28 @@ bool veth_is_primary(struct lyd_node *cif) { struct lyd_node *peer, *veth; + bool self_cni, peer_cni; const char *peername; veth = lydx_get_child(cif, "veth"); peername = lydx_get_cattr(veth, "peer"); peer = lydx_find_by_name(lyd_parent(cif), "interface", peername); - /* At the moment, CNI code relies on one side of the pair - * remaining in the host namespace, and that that interface - * takes care of creating the pair. - */ - if (lydx_get_child(cif, "container-network")) - return false; - if (lydx_get_child(peer, "container-network")) - return true; + self_cni = lydx_get_child(cif, "container-network") != NULL; + peer_cni = lydx_get_child(peer, "container-network") != NULL; - return strcmp(lydx_get_cattr(cif, "name"), - lydx_get_cattr(veth, "peer")) < 0; + /* When exactly one end is handed to a container (CNI host-device), + * the other end stays in the host namespace and creates the pair. + */ + if (self_cni != peer_cni) + return peer_cni; + + /* Neither or both ends are container interfaces: pick a stable + * primary by name so exactly one end creates the pair. When both + * ends are containers the pair is still created in the host + * namespace first, then moved into each container by CNI host-device. + */ + return strcmp(lydx_get_cattr(cif, "name"), peername) < 0; } int ifchange_cand_infer_veth(sr_session_ctx_t *session, const char *path) diff --git a/src/confd/src/interfaces.c b/src/confd/src/interfaces.c index 132ccca8..db01484c 100644 --- a/src/confd/src/interfaces.c +++ b/src/confd/src/interfaces.c @@ -518,6 +518,14 @@ static int veth_gen_del(struct lyd_node *dif, FILE *sh) if (!veth_is_primary(dif)) return 0; + /* When the primary end is itself a container interface it currently + * lives in the container's namespace, so a host-namespace delete here + * would fail and abort the teardown. Its removal is handled after the + * container is gone, see cni_netdag_gen_iface(). + */ + if (lydx_get_child(dif, "container-network")) + return 0; + return link_gen_del(dif, sh); } @@ -571,6 +579,28 @@ static int netdag_gen_iface_del(struct dagger *net, struct lyd_node *dif, return 0; } +/* + * Both ends of a veth pair can be handed to containers, leaving no + * host-side interface to create the pair. Have the primary end create it + * in the host namespace early (NETDAG_INIT_PHYS, before the container is + * (re)started); CNI host-device then moves each end into its container. + */ +static int veth_gen_host(struct dagger *net, struct lyd_node *dif, struct lyd_node *cif) +{ + const char *ifname = lydx_get_cattr(cif, "name"); + FILE *ip; + int err; + + ip = dagger_fopen_net_init(net, ifname, NETDAG_INIT_PHYS, "init.ip"); + if (!ip) + return -EIO; + + err = veth_gen(dif, cif, ip); + fclose(ip); + + return err; +} + static sr_error_t netdag_gen_iface_timeout(struct dagger *net, const char *ifname, const char *iftype) { if (!strcmp(iftype, "infix-if-type:ethernet")) { @@ -604,8 +634,13 @@ static sr_error_t netdag_gen_iface(sr_session_ctx_t *session, struct dagger *net if ((err = cni_netdag_gen_iface(net, ifname, dif, cif))) { /* error or managed by CNI/podman */ - if (err > 0) + if (err > 0) { err = 0; /* done, nothing more to do here */ + + if (op == LYDX_OP_CREATE && lydx_get_child(cif, "veth") && + veth_is_primary(cif)) + err = veth_gen_host(net, dif, cif); + } goto err; } diff --git a/src/confd/yang/confd/infix-if-container.yang b/src/confd/yang/confd/infix-if-container.yang index 2a2e1bd9..f496aa4f 100644 --- a/src/confd/yang/confd/infix-if-container.yang +++ b/src/confd/yang/confd/infix-if-container.yang @@ -59,11 +59,7 @@ submodule infix-if-container { identity host { base container-network; - description "Host device, e.g., one end of a VETH pair or other host interface. - - Note: When using VETH pairs, at least one side must remain in the - host namespace. Both ends of a VETH pair cannot be assigned to - different containers."; + description "Host device, e.g., one end of a VETH pair or other host interface."; } /* diff --git a/src/confd/yang/confd/infix-if-veth.yang b/src/confd/yang/confd/infix-if-veth.yang index d997ba36..bd29d434 100644 --- a/src/confd/yang/confd/infix-if-veth.yang +++ b/src/confd/yang/confd/infix-if-veth.yang @@ -13,11 +13,7 @@ submodule infix-if-veth { organization "KernelKit"; contact "kernelkit@googlegroups.com"; - description "Linux virtual Ethernet pair extension for ietf-interfaces. - - Note: When using VETH pairs with containers, at least one side - of the pair must remain in the host namespace. Both ends of a - VETH pair cannot be assigned to different containers."; + description "Linux virtual Ethernet pair extension for ietf-interfaces."; revision 2023-06-05 { description "Initial revision."; diff --git a/test/case/containers/all.yaml b/test/case/containers/all.yaml index abf9c65d..ea4ab2d8 100644 --- a/test/case/containers/all.yaml +++ b/test/case/containers/all.yaml @@ -18,6 +18,9 @@ - name: Container with VETH Pair case: veth/test.py +- name: VETH Pair Between Two Containers + case: internal_link/test.py + - name: Container Volume Persistence case: volume/test.py diff --git a/test/case/containers/internal_link/Readme.adoc b/test/case/containers/internal_link/Readme.adoc new file mode 120000 index 00000000..ae32c841 --- /dev/null +++ b/test/case/containers/internal_link/Readme.adoc @@ -0,0 +1 @@ +test.adoc \ No newline at end of file diff --git a/test/case/containers/internal_link/test.adoc b/test/case/containers/internal_link/test.adoc new file mode 100644 index 00000000..f52c311a --- /dev/null +++ b/test/case/containers/internal_link/test.adoc @@ -0,0 +1,33 @@ +=== VETH Pair Between Two Containers + +ifdef::topdoc[:imagesdir: {topdoc}../../test/case/containers/internal_link] + +==== Description + +Verify that a VETH pair can connect two containers directly, with *both* +ends handed to containers and neither remaining in the host namespace. + +.... + .------------. .------------. + | left | | right | + | veth0a ===|========= veth ===========|=== veth0b | + '------------' 10.0.0.1 10.0.0.2 '------------' +.... + +The pair is created in the host namespace then each end is moved into +its container when starting up. Connectivity is verified by pinging +across the pair, from inside one container's network namespace to the +other end's address. + +==== Topology + +image::topology.svg[VETH Pair Between Two Containers topology, align=center, scaledwidth=75%] + +==== Sequence + +. Set up topology and attach to target DUT +. Create VETH pair with both ends assigned to containers +. Verify both containers have started +. Verify {LEFT} reaches {RIGHT} over the internal VETH pair + + diff --git a/test/case/containers/internal_link/test.py b/test/case/containers/internal_link/test.py new file mode 100755 index 00000000..26c342eb --- /dev/null +++ b/test/case/containers/internal_link/test.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 +r"""VETH Pair Between Two Containers + +Verify that a VETH pair can connect two containers directly, with *both* +ends handed to containers and neither remaining in the host namespace. + +.... + .------------. .------------. + | left | | right | + | veth0a ===|========= veth ===========|=== veth0b | + '------------' 10.0.0.1 10.0.0.2 '------------' +.... + +The pair is created in the host namespace then each end is moved into +its container when starting up. Connectivity is verified by pinging +across the pair, from inside one container's network namespace to the +other end's address. + +""" + +import infamy +from infamy.util import until + +# Regression test for #941: previously, when both ends of a pair were +# assigned to a container, neither side created the pair. +with infamy.Test() as test: + LEFT, IFACE_LEFT, IP_LEFT = "left", "veth0a", "10.0.0.1" + RIGHT, IFACE_RIGHT, IP_RIGHT = "right", "veth0b", "10.0.0.2" + IMAGE = f"oci-archive:{infamy.Container.HTTPD_IMAGE}" + + with test.step("Set up topology and attach to target DUT"): + env = infamy.Env() + target = env.attach("target", "mgmt") + tgtssh = env.attach("target", "mgmt", "ssh") + + if not target.has_model("infix-containers"): + test.skip() + + with test.step("Create VETH pair with both ends assigned to containers"): + target.put_config_dicts({ + "ietf-interfaces": { + "interfaces": { + "interface": [ + { + "name": IFACE_LEFT, + "type": "infix-if-type:veth", + "enabled": True, + "infix-interfaces:veth": {"peer": IFACE_RIGHT}, + "ipv4": { + "address": [{"ip": IP_LEFT, "prefix-length": 24}] + }, + "container-network": {} + }, + { + "name": IFACE_RIGHT, + "type": "infix-if-type:veth", + "enabled": True, + "infix-interfaces:veth": {"peer": IFACE_LEFT}, + "ipv4": { + "address": [{"ip": IP_RIGHT, "prefix-length": 24}] + }, + "container-network": {} + }, + ] + } + }, + "infix-containers": { + "containers": { + "container": [ + { + "name": LEFT, + "image": IMAGE, + "command": "/usr/sbin/httpd -f -v -p 91", + "network": {"interface": [{"name": IFACE_LEFT}]} + }, + { + "name": RIGHT, + "image": IMAGE, + "command": "/usr/sbin/httpd -f -v -p 91", + "network": {"interface": [{"name": IFACE_RIGHT}]} + }, + ] + } + } + }) + + c = infamy.Container(target) + with test.step("Verify both containers have started"): + until(lambda: c.running(LEFT), attempts=60) + until(lambda: c.running(RIGHT), attempts=60) + + with test.step(f"Verify {LEFT} reaches {RIGHT} over the internal VETH pair"): + pid = tgtssh.runsh(f"sudo podman inspect --format '{{{{.State.Pid}}}}' {LEFT}").stdout.strip() + assert pid.isdigit(), f"failed to get pid for container {LEFT}: {pid!r}" + + def reachable(): + return tgtssh.runsh(f"sudo nsenter -t {pid} -n ping -c 2 -w 5 {IP_RIGHT}").returncode == 0 + + until(reachable, attempts=30) + + test.succeed() diff --git a/test/case/containers/internal_link/topology.dot b/test/case/containers/internal_link/topology.dot new file mode 120000 index 00000000..02b78869 --- /dev/null +++ b/test/case/containers/internal_link/topology.dot @@ -0,0 +1 @@ +../../../infamy/topologies/1x1.dot \ No newline at end of file diff --git a/test/case/containers/internal_link/topology.svg b/test/case/containers/internal_link/topology.svg new file mode 100644 index 00000000..6fc6f47a --- /dev/null +++ b/test/case/containers/internal_link/topology.svg @@ -0,0 +1,33 @@ + + + + + + +1x1 + + + +host + +host + +mgmt + + + +target + +mgmt + +target + + + +host:mgmt--target:mgmt + + + + diff --git a/test/case/firewall/basic/topology.svg b/test/case/firewall/basic/topology.svg index a65d5300..35a4d0ca 100644 --- a/test/case/firewall/basic/topology.svg +++ b/test/case/firewall/basic/topology.svg @@ -1,8 +1,7 @@ - +