With SB RBAC, a chassis may update the status of a BFD row only if the
row's chassis_name is the name of that chassis. ovn-northd takes
chassis_name from the binding of the logical router port, and the value
is wrong in the following cases:
- For a distributed gateway port, the binding of the logical router port
is a patch port, and no chassis claims a patch port. So chassis_name
stays empty. The BFD sessions of the port run on the chassis where
its chassisredirect port is bound.
- For an existing row, ovn-northd writes chassis_name only when the row
already has the new value, so the value never changes. When
ovn-controller claims a gateway router port after ovn-northd created
the row, chassis_name stays empty. When the gateway router moves to
another chassis, chassis_name keeps the name of the old chassis.
- The northd engine node ignores a change of the chassis of a binding.
So the BFD rows are synced again only when an unrelated change
recomputes the bfd_sync node.
The SB database then rejects every status update that ovn-controller
makes for these rows. Each rejection fails the whole SB transaction of
that ovn-controller iteration, with all its other writes. So a route
that uses the session can stay withdrawn while the session is up, or
stay in place while the session is down.
Set chassis_name from the binding that decides where the sessions run:
the binding of the chassisredirect port for a distributed gateway port,
and the binding of the port itself otherwise. Update it when the
chassis of that binding changes, and clear it when the binding has no
chassis, where 4885e337f kept the last name, so that a chassis that
released the port can no longer update the row. The bfd_sync engine
node now has SB Port_Binding as an input. Its handler recomputes the
node when the chassis changes in the binding of a port with BFD
sessions or of its chassisredirect port, including a new binding that
already has a chassis, and ignores all other binding changes. Each
change of chassis_name is an SB BFD update, so ovn-northd then
recomputes its logical flows, as it does for every BFD status update.
ovn-controller writes the status only on a state change of the session
and does not retry a rejected update. So a chassis that claims a port,
for the first time, after a move, or again after it released it, waits
for ovn-northd to set chassis_name to its name, and a state change that
it writes before that is still rejected. The status then stays wrong
until the next state change, unless ovn-controller also corrects the
status once chassis_name names its chassis, which is a separate
ovn-controller change.
Fixes: 4885e337f929 ("rbac: Only allow relevant chassis to update BFD.")
Fixes: 15c9c9f42ad8 ("northd: Add bfd, static_routes, route_policies and
bfd_sync nodes to I-P engine.")
Submitted-at: https://github.com/ovn-org/ovn/pull/334
Assisted-by: Claude Opus 5.5 (claude-opus-5-5), Cursor Grok Bot / Ultimum
harness assistants
Signed-off-by: Premysl Kouril <[email protected]>
---
northd/en-northd.c | 18 ++++++
northd/en-northd.h | 2 +
northd/inc-proc-northd.c | 2 +
northd/northd.c | 66 ++++++++++++++++++++--
northd/northd.h | 3 +
ovn-sb.xml | 11 +++-
tests/ovn-inc-proc-graph-dump.at | 1 +
tests/ovn-northd.at | 95 ++++++++++++++++++++++++++++++++
tests/ovn.at | 73 ++++++++++++++++++++++++
9 files changed, 264 insertions(+), 7 deletions(-)
diff --git a/northd/en-northd.c b/northd/en-northd.c
index 480dc61ca..fbe49b1f9 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -566,6 +566,24 @@ bfd_sync_routes_change_handler(struct engine_node *node,
return EN_HANDLED_UNCHANGED;
}
+enum engine_input_handler_result
+bfd_sync_sb_port_binding_handler(struct engine_node *node, void *data)
+{
+ struct northd_data *northd_data = engine_get_input_data("northd", node);
+ const struct sbrec_port_binding_table *sbrec_port_binding_table =
+ EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
+ struct bfd_sync_data *bfd_sync_data = data;
+
+ /* The en_northd node ignores changes of the chassis of a binding, which
+ * the SB BFD "chassis_name" depends on. */
+ if (!bfd_sync_handle_sb_port_binding_changes(sbrec_port_binding_table,
+ &northd_data->lr_ports,
+ &bfd_sync_data->bfd_ports)) {
+ return EN_UNHANDLED;
+ }
+ return EN_HANDLED_UNCHANGED;
+}
+
enum engine_node_state
en_bfd_sync_run(struct engine_node *node, void *data)
{
diff --git a/northd/en-northd.h b/northd/en-northd.h
index c62631008..5915baed7 100644
--- a/northd/en-northd.h
+++ b/northd/en-northd.h
@@ -58,6 +58,8 @@ bfd_sync_northd_change_handler(struct engine_node *node,
enum engine_input_handler_result
bfd_sync_routes_change_handler(struct engine_node *node,
void *data OVS_UNUSED);
+enum engine_input_handler_result
+bfd_sync_sb_port_binding_handler(struct engine_node *node, void *data);
enum engine_node_state en_bfd_sync_run(struct engine_node *node, void *data);
void en_bfd_sync_cleanup(void *data OVS_UNUSED);
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 53604a20a..8d730a5f0 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -347,6 +347,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(&en_bfd_sync, &en_routes, bfd_sync_routes_change_handler);
engine_add_input(&en_bfd_sync, &en_route_policies, NULL);
engine_add_input(&en_bfd_sync, &en_northd, bfd_sync_northd_change_handler);
+ engine_add_input(&en_bfd_sync, &en_sb_port_binding,
+ bfd_sync_sb_port_binding_handler);
engine_add_input(&en_ecmp_nexthop, &en_global_config, NULL);
engine_add_input(&en_ecmp_nexthop, &en_routes, NULL);
diff --git a/northd/northd.c b/northd/northd.c
index f37040b57..50e08b58b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12004,6 +12004,59 @@ bfd_get_connection_status(const struct nbrec_bfd
*nb_bt,
return bfd_rp ? bfd_rp->status : bfd_sr->status;
}
+/* Returns the chassis that runs the BFD sessions of logical router port
+ * 'op': the chassis in the binding of its chassisredirect port if it has
+ * one, and otherwise in its own binding. Returns NULL if that binding has
+ * no chassis. */
+static const struct sbrec_chassis *
+bfd_port_chassis(const struct ovn_port *op)
+{
+ if (op->cr_port) {
+ return op->cr_port->sb ? op->cr_port->sb->chassis : NULL;
+ }
+ return op->sb->chassis;
+}
+
+/* Returns false if the chassis changed in the Port_Binding of a logical
+ * router port in 'bfd_ports' or of its chassisredirect port, because the SB
+ * BFD "chassis_name" of the port's sessions then needs an update (see
+ * bfd_port_chassis()). A new binding counts as a change if it already has
+ * a chassis, because a chassis can claim a binding before ovn-northd sees
+ * that it was inserted. If a binding that ovn-northd still uses is
+ * deleted, the en_northd node recomputes, and so does this node. */
+bool
+bfd_sync_handle_sb_port_binding_changes(
+ const struct sbrec_port_binding_table *sbrec_port_binding_table,
+ const struct hmap *lr_ports, const struct sset *bfd_ports)
+{
+ const struct sbrec_port_binding *pb;
+ SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) {
+ if (sbrec_port_binding_is_deleted(pb)) {
+ continue;
+ }
+
+ bool chassis_changed = sbrec_port_binding_is_new(pb)
+ ? pb->chassis != NULL
+ : sbrec_port_binding_is_updated(
+ pb, SBREC_PORT_BINDING_COL_CHASSIS);
+ if (!chassis_changed) {
+ continue;
+ }
+
+ const struct ovn_port *op = ovn_port_find(lr_ports, pb->logical_port);
+ if (!op) {
+ continue;
+ }
+ if (bfd_is_port_running(bfd_ports, op->key) ||
+ (op->primary_port &&
+ bfd_is_port_running(bfd_ports, op->primary_port->key))) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
void
bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn,
const struct nbrec_bfd_table *nbrec_bfd_table,
@@ -12062,8 +12115,9 @@ bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn,
sbrec_bfd_set_disc(sb_bt, 1 + random_uint32());
sbrec_bfd_set_src_port(sb_bt, udp_src);
sbrec_bfd_set_status(sb_bt, nb_bt->status);
- if (op->sb->chassis) {
- sbrec_bfd_set_chassis_name(sb_bt, op->sb->chassis->name);
+ const struct sbrec_chassis *chassis = bfd_port_chassis(op);
+ if (chassis) {
+ sbrec_bfd_set_chassis_name(sb_bt, chassis->name);
}
int min_tx = nb_bt->n_min_tx ? nb_bt->min_tx[0] : BFD_DEF_MINTX;
@@ -12084,10 +12138,10 @@ bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn,
}
build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt);
- if (op->sb->chassis && !strcmp(op->sb->chassis->name,
- bfd_e->sb_bt->chassis_name)) {
- sbrec_bfd_set_chassis_name(bfd_e->sb_bt,
- op->sb->chassis->name);
+ const struct sbrec_chassis *chassis = bfd_port_chassis(op);
+ const char *chassis_name = chassis ? chassis->name : "";
+ if (strcmp(bfd_e->sb_bt->chassis_name, chassis_name)) {
+ sbrec_bfd_set_chassis_name(bfd_e->sb_bt, chassis_name);
}
}
diff --git a/northd/northd.h b/northd/northd.h
index 4150157b0..20fda76d8 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -1025,6 +1025,9 @@ void bfd_table_sync(struct ovsdb_idl_txn *, const struct
nbrec_bfd_table *,
const struct hmap *, const struct hmap *,
const struct hmap *, const struct hmap *,
struct sset *);
+bool bfd_sync_handle_sb_port_binding_changes(
+ const struct sbrec_port_binding_table *, const struct hmap *lr_ports,
+ const struct sset *bfd_ports);
void build_bfd_map(const struct nbrec_bfd_table *,
const struct sbrec_bfd_table *, struct hmap *);
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 2096fc3e3..9a0bfe8ea 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -5559,7 +5559,16 @@ tcp.flags = RST;
</column>
<column name="chassis_name">
- The name of the chassis where the logical port is bound.
+ The name of the chassis that would run the BFD session. For a
+ distributed gateway port, this is the chassis where its
+ chassisredirect port is bound. For other ports, it is the chassis
+ where the logical port is bound. The column is empty when that
+ binding has no chassis. <code>ovn-controller</code> runs the
+ session only for a distributed gateway port or an l3gateway port
+ whose binding has a peer, so the chassis named here does not always
+ run it. <code>ovn-northd</code> sets this column. With role-based
+ access control, <code>ovn-controller</code> may update
+ <ref column="status"/> only from the chassis named here.
</column>
<column name="options">
diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ovn-inc-proc-graph-dump.at
index 8348c3467..668991962 100644
--- a/tests/ovn-inc-proc-graph-dump.at
+++ b/tests/ovn-inc-proc-graph-dump.at
@@ -163,6 +163,7 @@ digraph "Incremental-Processing-Engine" {
routes -> bfd_sync [[label="bfd_sync_routes_change_handler"]];
route_policies -> bfd_sync [[label=""]];
northd -> bfd_sync [[label="bfd_sync_northd_change_handler"]];
+ SB_port_binding -> bfd_sync
[[label="bfd_sync_sb_port_binding_handler"]];
SB_learned_route [[style=filled, shape=box, fillcolor=white,
label="SB_learned_route"]];
learned_route_sync [[style=filled, shape=box, fillcolor=white,
label="learned_route_sync"]];
SB_learned_route -> learned_route_sync
[[label="learned_route_sync_sb_learned_route_change_handler"]];
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index f8c144918..9198eaa38 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4769,6 +4769,101 @@ OVN_CLEANUP_NORTHD
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([BFD chassis_name follows the chassis that runs the session])
+AT_KEYWORDS([northd-bfd])
+ovn_start
+
+check ovn-sbctl chassis-add gw1 geneve 127.0.0.1
+check ovn-sbctl chassis-add gw2 geneve 127.0.0.2
+check ovn-sbctl chassis-add hv1 geneve 127.0.0.3
+
+# The BFD sessions of a distributed gateway port run on the chassis where its
+# chassisredirect port is bound. The port's own binding is a patch port that
+# no chassis claims.
+check ovn-nbctl lr-add r0
+check ovn-nbctl ls-add ext
+check ovn-nbctl lrp-add r0 r0-ext 00:00:00:00:00:01 10.0.0.1/24
+check ovn-nbctl lsp-add-router-port ext ext-r0 r0-ext
+check ovn-nbctl lrp-set-gateway-chassis r0-ext gw1
+check ovn-nbctl --bfd lr-route-add r0 0.0.0.0/0 10.0.0.254 r0-ext
+# A distributed gateway port without BFD sessions, and a switch port.
+check ovn-nbctl lr-add r2
+check ovn-nbctl lrp-add r2 r2-ext 00:00:00:00:02:01 10.0.2.1/24
+check ovn-nbctl lsp-add-router-port ext ext-r2 r2-ext
+check ovn-nbctl lrp-set-gateway-chassis r2-ext gw1
+check ovn-nbctl ls-add sw0
+check ovn-nbctl lsp-add sw0 vif1
+check ovn-nbctl --wait=sb sync
+wait_row_count BFD 1 dst_ip=10.0.0.254
+check_column "" BFD chassis_name dst_ip=10.0.0.254
+
+AS_BOX([The chassisredirect port is claimed])
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-sbctl lsp-bind cr-r0-ext gw1
+wait_column gw1 BFD chassis_name dst_ip=10.0.0.254
+check ovn-nbctl --wait=sb sync
+check_engine_stats bfd_sync recompute nocompute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AS_BOX([Other binding changes do not recompute bfd_sync])
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-sbctl lsp-bind vif1 hv1
+check ovn-sbctl lsp-bind cr-r2-ext gw1
+check ovn-sbctl set Port_Binding cr-r0-ext external_ids:foo=bar
+check ovn-sbctl remove Port_Binding cr-r0-ext external_ids foo
+check ovn-nbctl --wait=sb sync
+check_engine_stats bfd_sync norecompute compute
+check_column gw1 BFD chassis_name dst_ip=10.0.0.254
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AS_BOX([The chassisredirect port moves])
+check ovn-sbctl lsp-unbind cr-r0-ext -- lsp-bind cr-r0-ext gw2
+wait_column gw2 BFD chassis_name dst_ip=10.0.0.254
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AS_BOX([The chassisredirect port is released])
+# Like ovn-controller, also set "up" to false.
+check ovn-sbctl lsp-unbind cr-r0-ext -- set Port_Binding cr-r0-ext up=false
+wait_column "" BFD chassis_name dst_ip=10.0.0.254
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AS_BOX([A session created while the chassisredirect port is bound])
+check ovn-sbctl lsp-bind cr-r0-ext gw1
+wait_column gw1 BFD chassis_name dst_ip=10.0.0.254
+check ovn-nbctl --wait=sb --bfd lr-route-add r0 10.1.0.0/16 10.0.0.253 r0-ext
+wait_row_count BFD 1 dst_ip=10.0.0.253 chassis_name=gw1
+# ovn-northd sets chassis_name when it creates the row.
+AT_CHECK([grep 'received request, method="transact"' ovn-sb/ovsdb-server.log |
\
+ grep -c
'"op":"insert","row":{"chassis_name":"gw1"[[^}]]*"dst_ip":"10.0.0.253"'],
+ [0], [1
+])
+
+AS_BOX([The distributed gateway port loses its gateway chassis])
+check ovn-nbctl --wait=sb lrp-del-gateway-chassis r0-ext gw1
+wait_column "" BFD chassis_name dst_ip=10.0.0.254
+wait_column "" BFD chassis_name dst_ip=10.0.0.253
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+# The BFD sessions of a gateway router port run on the chassis where the
+# l3gateway port is bound. ovn-controller claims it after ovn-northd created
+# the BFD row.
+AS_BOX([Gateway router port])
+check ovn-nbctl lr-add r1 -- set Logical_Router r1 options:chassis=gw1
+check ovn-nbctl ls-add ext1
+check ovn-nbctl lrp-add r1 r1-ext 00:00:00:00:01:01 10.0.1.1/24
+check ovn-nbctl lsp-add-router-port ext1 ext1-r1 r1-ext
+check ovn-nbctl --wait=sb --bfd lr-route-add r1 0.0.0.0/0 10.0.1.254 r1-ext
+wait_row_count BFD 1 dst_ip=10.0.1.254
+check_column "" BFD chassis_name dst_ip=10.0.1.254
+check ovn-sbctl lsp-bind r1-ext gw1
+wait_column gw1 BFD chassis_name dst_ip=10.0.1.254
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([ovn -- check CoPP config])
AT_KEYWORDS([northd-CoPP])
diff --git a/tests/ovn.at b/tests/ovn.at
index 136b825f9..ff23a61ce 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -12953,6 +12953,79 @@ ignored_tables=OFTABLE_PHY_TO_LOG
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([BFD status of a distributed gateway port with SB RBAC])
+AT_KEYWORDS([ovn-bfd bfd-rbac])
+# With SSL, ovn-controller connects to the SB database with the RBAC role
+# "ovn-controller" (see ovn_start).
+AT_SKIP_IF([test "$HAVE_OPENSSL" = no])
+CHECK_SCAPY
+ovn_start
+
+net_add underlay
+net_add provider
+sim_add gw1
+as gw1
+check ovs-vsctl add-br br-phys
+ovn_attach underlay br-phys 192.168.0.1
+check ovs-vsctl add-br br-ex
+net_attach provider br-ex
+check ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=phys:br-ex
+
+check ovn-nbctl lr-add r0
+check ovn-nbctl ls-add ext
+check ovn-nbctl lsp-add-localnet-port ext ln-ext phys
+check ovn-nbctl lrp-add r0 r0-ext 00:00:00:00:00:01 10.0.0.1/24
+check ovn-nbctl lsp-add-router-port ext ext-r0 r0-ext
+check ovn-nbctl lrp-set-gateway-chassis r0-ext gw1
+check ovn-nbctl static-mac-binding-add r0-ext 10.0.0.254 00:00:00:00:00:fe
+check ovn-nbctl --bfd lr-route-add r0 0.0.0.0/0 10.0.0.254 r0-ext
+wait_column "$(fetch_column Chassis _uuid name=gw1)" Port_Binding chassis \
+ logical_port=cr-r0-ext
+wait_row_count BFD 1
+wait_column down BFD status dst_ip=10.0.0.254
+# SB RBAC lets gw1 update the row only if chassis_name is gw1.
+wait_column gw1 BFD chassis_name dst_ip=10.0.0.254
+OVN_WAIT_PATCH_PORT_FLOWS([ln-ext], [gw1])
+check ovn-nbctl --wait=hv sync
+
+# Plays the BFD peer 10.0.0.254: sends a control packet in state "init",
+# with 1 s intervals and detection multiplier 3, every 0.3 s. That brings
+# gw1's session up and keeps it up.
+pkt=$(fmt_pkt "Ether(dst='00:00:00:00:00:01', src='00:00:00:00:00:fe')/ \
+ IP(src='10.0.0.254', dst='10.0.0.1', ttl=255)/ \
+ UDP(sport=49152, dport=3784)/ \
+ Raw(load=bytes.fromhex('20800318000000fe' + \
+ '$(printf %08x $(fetch_column BFD disc dst_ip=10.0.0.254))' + \
+ '000f4240000f424000000000'))")
+(while :; do
+ as gw1 ovs-appctl netdev-dummy/receive br-ex_provider $pkt \
+ >/dev/null 2>&1
+ sleep 0.3
+ done) &
+echo $! > bfd-peer.pid
+on_exit 'test -e bfd-peer.pid && kill $(cat bfd-peer.pid)'
+
+AS_BOX([The session comes up])
+wait_column up BFD status dst_ip=10.0.0.254
+OVS_WAIT_UNTIL([ovn-sbctl lflow-list r0 | grep lr_in_ip_routing | \
+ grep -q 'reg0 = 10.0.0.254'])
+
+AS_BOX([The peer stops answering])
+kill $(cat bfd-peer.pid)
+rm bfd-peer.pid
+wait_column down BFD status dst_ip=10.0.0.254
+OVS_WAIT_UNTIL([! ovn-sbctl lflow-list r0 | grep lr_in_ip_routing | \
+ grep -q 'reg0 = 10.0.0.254'])
+
+# The SB database accepted all of gw1's updates.
+AT_CHECK([grep -c 'transaction error' gw1/ovn-controller.log], [1], [0
+])
+
+OVN_CLEANUP([gw1])
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([4 HV, 1 LS, 1 LR, packet test with HA distributed router gateway
port])
ovn_start
--
2.48.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
