From 97f3db8fdbe937c5dc6d97b22dcb0b85f19ec53c Mon Sep 17 00:00:00 2001 From: Tobias Waldekranz Date: Mon, 10 Feb 2025 16:28:36 +0100 Subject: [PATCH] confd: Always generate a valid interface DAG Fix #938 Before this change, interface dependencies were only setup when interfaces were created. This led to problems in flows like this: 1. configure set interface br0 set interface br0.10 leave 2. configure set interface dummy0 leave 2. configure no interface br0 no interface br0.10 leave Since neither br0 nor br0.10 was created in (2), no dependencies were setup. Because of that, when the deletion scripts from generation (2) are generated when entering (3), dagger is free to execute them in any order. This means that it may choose to remove br0 before br0.10, which will mean that br0.10 is already gone when we try to remove it. Therefore, make sure that we generate the full dependency graph on every iteration. Also, get rid of the need to keep on-disk state for VETH pairs, and use the name as a descriminator for which side to create/delete. --- src/confd/src/dagger.c | 20 ------- src/confd/src/dagger.h | 4 -- src/confd/src/ietf-interfaces.c | 82 +++++++++++++++++++--------- src/confd/src/ietf-interfaces.h | 4 ++ src/confd/src/infix-if-bridge-port.c | 5 -- src/confd/src/infix-if-bridge.c | 27 +++++++++ src/confd/src/infix-if-veth.c | 77 +++++++++++++++++++------- src/confd/src/infix-if-vlan.c | 20 +++++-- 8 files changed, 160 insertions(+), 79 deletions(-) diff --git a/src/confd/src/dagger.c b/src/confd/src/dagger.c index c9fed136..9dfd3e28 100644 --- a/src/confd/src/dagger.c +++ b/src/confd/src/dagger.c @@ -155,26 +155,6 @@ int dagger_evolve_or_abandon(struct dagger *d) return err; } -void dagger_skip_iface(struct dagger *d, const char *ifname) -{ - touchf("%s/%d/skip/%s", d->path, d->next, ifname); -} - -void dagger_skip_current_iface(struct dagger *d, const char *ifname) -{ - touchf("%s/%d/skip/%s", d->path, d->current, ifname); -} - -int dagger_should_skip(struct dagger *d, const char *ifname) -{ - return fexistf("%s/%d/skip/%s", d->path, d->next, ifname); -} - -int dagger_should_skip_current(struct dagger *d, const char *ifname) -{ - return fexistf("%s/%d/skip/%s", d->path, d->current, ifname); -} - int dagger_is_bootstrap(struct dagger *d) { return d->next == 0; diff --git a/src/confd/src/dagger.h b/src/confd/src/dagger.h index 4720657b..d98469b1 100644 --- a/src/confd/src/dagger.h +++ b/src/confd/src/dagger.h @@ -27,10 +27,6 @@ int dagger_abandon(struct dagger *d); int dagger_evolve(struct dagger *d); int dagger_evolve_or_abandon(struct dagger *d); -void dagger_skip_iface(struct dagger *d, const char *ifname); -void dagger_skip_current_iface(struct dagger *d, const char *ifname); -int dagger_should_skip(struct dagger *d, const char *ifname); -int dagger_should_skip_current(struct dagger *d, const char *ifname); int dagger_is_bootstrap(struct dagger *d); int dagger_claim(struct dagger *d, const char *path); diff --git a/src/confd/src/ietf-interfaces.c b/src/confd/src/ietf-interfaces.c index e1de591c..059fd2d5 100644 --- a/src/confd/src/ietf-interfaces.c +++ b/src/confd/src/ietf-interfaces.c @@ -482,40 +482,36 @@ static bool netdag_must_del(struct lyd_node *dif, struct lyd_node *cif) } static int netdag_gen_iface_del(struct dagger *net, struct lyd_node *dif, - struct lyd_node *cif, bool fixed) + struct lyd_node *cif) { const char *ifname = lydx_get_cattr(dif, "name"); FILE *ip; DEBUG_IFACE(dif, ""); - if (dagger_should_skip_current(net, ifname)) - return 0; - - if (iftype_from_iface(cif) == IFT_VETH) { - struct lyd_node *node; - const char *peer; - - node = lydx_get_descendant(lyd_child(dif), "veth", NULL); - if (!node) - return -EINVAL; - - peer = lydx_get_cattr(node, "peer"); - if (!peer) - return -EINVAL; - - dagger_skip_current_iface(net, peer); - } - ip = dagger_fopen_net_exit(net, ifname, NETDAG_EXIT, "exit.ip"); if (!ip) return -EIO; - if (fixed) { + switch (iftype_from_iface(dif)) { + case IFT_ETH: + case IFT_ETHISH: fprintf(ip, "link set dev %s down\n", ifname); fprintf(ip, "addr flush dev %s\n", ifname); - } else { + break; + case IFT_VETH: + if (!veth_is_primary(dif)) + break; + /* fallthrough */ + case IFT_BRIDGE: + case IFT_DUMMY: + case IFT_GRE: + case IFT_GRETAP: + case IFT_VLAN: + case IFT_VXLAN: + case IFT_UNKNOWN: fprintf(ip, "link del dev %s\n", ifname); + break; } fclose(ip); @@ -545,7 +541,7 @@ static sr_error_t netdag_gen_iface(sr_session_ctx_t *session, struct dagger *net (op == LYDX_OP_NONE) ? "mod" : ((op == LYDX_OP_CREATE) ? "add" : "del")); if (op == LYDX_OP_DELETE) { - err = netdag_gen_iface_del(net, dif, cif, fixed); + err = netdag_gen_iface_del(net, dif, cif); err += netdag_gen_ipv4_autoconf(net, cif, dif); goto err; } @@ -559,7 +555,7 @@ static sr_error_t netdag_gen_iface(sr_session_ctx_t *session, struct dagger *net if (op != LYDX_OP_CREATE && netdag_must_del(dif, cif)) { DEBUG_IFACE(dif, "Must delete"); - err = netdag_gen_iface_del(net, dif, cif, fixed); + err = netdag_gen_iface_del(net, dif, cif); if (err) goto err; @@ -636,14 +632,48 @@ err: return err ? SR_ERR_INTERNAL : SR_ERR_OK; } +static int netdag_init_iface(struct lyd_node *cif) +{ + int err; + + err = dagger_add_node(&confd.netdag, lydx_get_cattr(cif, "name")); + if (err) + return err; + + switch (iftype_from_iface(cif)) { + case IFT_BRIDGE: + return bridge_add_deps(cif); + /* case IFT_LAG: */ + /* return lag_add_deps(cif); */ + case IFT_VLAN: + return vlan_add_deps(cif); + case IFT_VETH: + return veth_add_deps(cif); + + case IFT_DUMMY: + case IFT_ETH: + case IFT_ETHISH: + case IFT_GRE: + case IFT_GRETAP: + case IFT_VXLAN: + case IFT_UNKNOWN: + break; + } + + return 0; +} + static sr_error_t netdag_init(sr_session_ctx_t *session, struct dagger *net, struct lyd_node *cifs, struct lyd_node *difs) { - struct lyd_node *iface; + struct lyd_node *cif; + int err; - LYX_LIST_FOR_EACH(cifs, iface, "interface") - if (dagger_add_node(net, lydx_get_cattr(iface, "name"))) + LYX_LIST_FOR_EACH(cifs, cif, "interface") { + err = netdag_init_iface(cif); + if (err) return SR_ERR_INTERNAL; + } net->session = session; return SR_ERR_OK; diff --git a/src/confd/src/ietf-interfaces.h b/src/confd/src/ietf-interfaces.h index 57c4ef5c..f412f3f6 100644 --- a/src/confd/src/ietf-interfaces.h +++ b/src/confd/src/ietf-interfaces.h @@ -97,20 +97,24 @@ int netdag_gen_ip_addrs(struct dagger *net, FILE *ip, const char *proto, /* infix-if-bridge.c */ int bridge_mstpd_gen(struct lyd_node *cifs); int bridge_gen(struct lyd_node *dif, struct lyd_node *cif, FILE *ip, int add); +int bridge_add_deps(struct lyd_node *cif); /* infix-if-bridge-mcd.c */ int bridge_mcd_gen(struct lyd_node *cifs); /* infix-if-bridge-port.c */ int bridge_port_gen(struct lyd_node *dif, struct lyd_node *cif, FILE *ip); /* infix-if-veth.c */ +bool veth_is_primary(struct lyd_node *cif); int ifchange_cand_infer_veth(sr_session_ctx_t *session, const char *path); int netdag_gen_veth(struct dagger *net, struct lyd_node *dif, struct lyd_node *cif, FILE *ip); +int veth_add_deps(struct lyd_node *cif); /* infix-if-vlan.c */ int ifchange_cand_infer_vlan(sr_session_ctx_t *session, const char *path); int netdag_gen_vlan(struct dagger *net, struct lyd_node *dif, struct lyd_node *cif, FILE *ip); +int vlan_add_deps(struct lyd_node *cif); /* infix-if-gre.c */ int gre_gen(struct dagger *net, struct lyd_node *dif, diff --git a/src/confd/src/infix-if-bridge-port.c b/src/confd/src/infix-if-bridge-port.c index 42f77f2a..ffb24d5e 100644 --- a/src/confd/src/infix-if-bridge-port.c +++ b/src/confd/src/infix-if-bridge-port.c @@ -198,7 +198,6 @@ static int gen_link(struct lyd_node *dif, struct lyd_node *cif) const char *brname, *iface; int mrouter; FILE *next; - int err; if (!lydx_get_child(dif, "bridge-port")) return 0; @@ -214,10 +213,6 @@ static int gen_link(struct lyd_node *dif, struct lyd_node *cif) iface = lydx_get_cattr(cif, "name"); - err = dagger_add_dep(&confd.netdag, brname, iface); - if (err) - return ERR_IFACE(cif, err, "Unable to add dep \"%s\" to %s", iface, brname); - next = dagger_fopen_net_init(&confd.netdag, brname, NETDAG_INIT_LOWERS, "add-ports.ip"); if (!next) diff --git a/src/confd/src/infix-if-bridge.c b/src/confd/src/infix-if-bridge.c index a0434c6e..79c538ba 100644 --- a/src/confd/src/infix-if-bridge.c +++ b/src/confd/src/infix-if-bridge.c @@ -535,4 +535,31 @@ out: return err; } +int bridge_add_deps(struct lyd_node *cif) +{ + const char *brname = lydx_get_cattr(cif, "name"); + struct ly_set *brports; + const char *portname; + int err = 0; + uint32_t i; + + brports = lydx_find_xpathf(cif, "../interface[bridge-port/bridge='%s']", brname); + if (!brports) + return ERR_IFACE(cif, -ENOENT, "Unable to fetch bridge ports"); + + + for (i = 0; i < brports->count; i++) { + portname = lydx_get_cattr(brports->dnodes[i], "name"); + + err = dagger_add_dep(&confd.netdag, brname, portname); + if (err) { + ERR_IFACE(cif, err, "Unable to depend on \"%s\"", portname); + break; + } + } + + ly_set_free(brports, NULL); + return err; +} + /* BR */ diff --git a/src/confd/src/infix-if-veth.c b/src/confd/src/infix-if-veth.c index b4d35868..27c8a87b 100644 --- a/src/confd/src/infix-if-veth.c +++ b/src/confd/src/infix-if-veth.c @@ -12,6 +12,34 @@ #include "ietf-interfaces.h" +/* + * While the kernel atomically creates/destroys the pair, in `running` + * the two sides are distinct interfaces. So we need to figure out + * which one is going to create/delete the other - i.e. which side is + * the "primary" + */ +bool veth_is_primary(struct lyd_node *cif) +{ + struct lyd_node *peer, *veth; + 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; + + return strcmp(lydx_get_cattr(cif, "name"), + lydx_get_cattr(veth, "peer")) < 0; +} + int ifchange_cand_infer_veth(sr_session_ctx_t *session, const char *path) { char *ifname, *type, *peer, *xpath, *val; @@ -67,36 +95,47 @@ int netdag_gen_veth(struct dagger *net, struct lyd_node *dif, struct lyd_node *cif, FILE *ip) { const char *ifname = lydx_get_cattr(cif, "name"); + char ifname_args[64] = "", peer_args[64] = ""; + const char *mac, *peer; struct lyd_node *node; - const char *peer; - int err; + + if (!veth_is_primary(cif)) + return 0; node = lydx_get_descendant(lyd_child(cif), "veth", NULL); if (!node) return -EINVAL; peer = lydx_get_cattr(node, "peer"); - if (dagger_should_skip(net, ifname)) { - err = dagger_add_dep(net, ifname, peer); - if (err) - return ERR_IFACE(cif, err, "Unable to add dep \"%s\" to %s", peer, ifname); - } else { - char ifname_args[64] = "", peer_args[64] = ""; - const char *mac; - dagger_skip_iface(net, peer); + mac = get_phys_addr(dif, NULL); + if (mac) + snprintf(ifname_args, sizeof(ifname_args), "address %s", mac); - mac = get_phys_addr(dif, NULL); - if (mac) - snprintf(ifname_args, sizeof(ifname_args), "address %s", mac); + node = lydx_find_by_name(lyd_parent(cif), "interface", peer); + if (node && (mac = get_phys_addr(node, NULL))) + snprintf(peer_args, sizeof(peer_args), "address %s", mac); - node = lydx_find_by_name(lyd_parent(cif), "interface", peer); - if (node && (mac = get_phys_addr(node, NULL))) - snprintf(peer_args, sizeof(peer_args), "address %s", mac); + fprintf(ip, "link add dev %s %s type veth peer %s %s\n", + ifname, ifname_args, peer, peer_args); - fprintf(ip, "link add dev %s %s type veth peer %s %s\n", - ifname, ifname_args, peer, peer_args); - } + return 0; +} + +int veth_add_deps(struct lyd_node *cif) +{ + struct lyd_node *veth = lydx_get_child(cif, "veth"); + const char *peer; + int err; + + if (veth_is_primary(cif)) + return 0; + + peer = lydx_get_cattr(veth, "peer"); + + err = dagger_add_dep(&confd.netdag, lydx_get_cattr(cif, "name"), peer); + if (err) + return ERR_IFACE(cif, err, "Unable to depend on \"%s\"", peer); return 0; } diff --git a/src/confd/src/infix-if-vlan.c b/src/confd/src/infix-if-vlan.c index 25eb0237..ee9e1a49 100644 --- a/src/confd/src/infix-if-vlan.c +++ b/src/confd/src/infix-if-vlan.c @@ -160,11 +160,6 @@ int netdag_gen_vlan(struct dagger *net, struct lyd_node *dif, lower_if = lydx_get_cattr(vlan, "lower-layer-if"); DEBUG("ifname %s lower if %s\n", ifname, lower_if); - err = dagger_add_dep(net, ifname, lower_if); - if (err) - return ERR_IFACE(cif, err, "Unable to add dep \"%s\"", lower_if); - - fprintf(ip, "link add dev %s down link %s type vlan", ifname, lower_if); if (lydx_get_diff(lydx_get_child(vlan, "tag-type"), &typed)) { @@ -190,3 +185,18 @@ int netdag_gen_vlan(struct dagger *net, struct lyd_node *dif, return 0; } + +int vlan_add_deps(struct lyd_node *cif) +{ + struct lyd_node *vlan = lydx_get_child(cif, "vlan"); + const char *lower; + int err; + + lower = lydx_get_cattr(vlan, "lower-layer-if"); + + err = dagger_add_dep(&confd.netdag, lydx_get_cattr(cif, "name"), lower); + if (err) + return ERR_IFACE(cif, err, "Unable to depend on \"%s\"", lower); + + return 0; +}