RBAC did not restrict which chassis could update IGMP_Groups. With this
change, we add a new "chassis_name" column to IGMP_Group.
This may seem odd since there is already a "chassis" column in
IGMP_Group. But RBAC specifically works by string matching based on the
certificate common name. Therefore, we need to have a chassis_name
string column instead of a chassis UUID column.
Getting RBAC to function properly required me to fix an existing bug as
well. igmp_group_cleanup() did not ensure that only local IGMP group
records were deleted. This presumably meant that when one ovn-controller
in a cluster was shut down, it would delete ALL IGMP_Group records in
the southbound DB, not just the local ones.
Signed-off-by: Mark Michelson <[email protected]>
---
v1 -> v2:
* Rebased on top of current main
* Fixed igmp_group_cleanup() to only delete local records.
---
controller/ip-mcast.c | 26 +++++++++++++++++++-------
controller/ip-mcast.h | 9 ++++++---
controller/ovn-controller.c | 3 ++-
controller/pinctrl.c | 16 +++++++++++++---
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema | 7 ++++---
ovn-sb.xml | 5 +++++
tests/ovn.at | 2 +-
8 files changed, 51 insertions(+), 19 deletions(-)
diff --git a/controller/ip-mcast.c b/controller/ip-mcast.c
index a870fb29e..b457c7e69 100644
--- a/controller/ip-mcast.c
+++ b/controller/ip-mcast.c
@@ -38,7 +38,8 @@ static struct sbrec_igmp_group *
igmp_group_create_(struct ovsdb_idl_txn *idl_txn,
const char *addr_str,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis);
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name);
struct ovsdb_idl_index *
igmp_group_index_create(struct ovsdb_idl *idl)
@@ -86,7 +87,8 @@ struct sbrec_igmp_group *
igmp_group_create(struct ovsdb_idl_txn *idl_txn,
const struct in6_addr *address,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis)
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name)
{
char addr_str[INET6_ADDRSTRLEN];
@@ -94,16 +96,18 @@ igmp_group_create(struct ovsdb_idl_txn *idl_txn,
return NULL;
}
- return igmp_group_create_(idl_txn, addr_str, datapath, chassis);
+ return igmp_group_create_(idl_txn, addr_str, datapath, chassis,
+ igmp_group_has_chassis_name);
}
struct sbrec_igmp_group *
igmp_mrouter_create(struct ovsdb_idl_txn *idl_txn,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis)
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name)
{
return igmp_group_create_(idl_txn, OVN_IGMP_GROUP_MROUTERS, datapath,
- chassis);
+ chassis, igmp_group_has_chassis_name);
}
void
@@ -211,7 +215,8 @@ igmp_group_delete(const struct sbrec_igmp_group *g)
bool
igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
- struct ovsdb_idl_index *igmp_groups)
+ struct ovsdb_idl_index *igmp_groups,
+ const struct sbrec_chassis *chassis)
{
const struct sbrec_igmp_group *g;
@@ -220,6 +225,9 @@ igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
}
SBREC_IGMP_GROUP_FOR_EACH_BYINDEX (g, igmp_groups) {
+ if (chassis != g->chassis) {
+ continue;
+ }
igmp_group_delete(g);
}
@@ -249,13 +257,17 @@ static struct sbrec_igmp_group *
igmp_group_create_(struct ovsdb_idl_txn *idl_txn,
const char *addr_str,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis)
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name)
{
struct sbrec_igmp_group *g = sbrec_igmp_group_insert(idl_txn);
sbrec_igmp_group_set_address(g, addr_str);
sbrec_igmp_group_set_datapath(g, datapath);
sbrec_igmp_group_set_chassis(g, chassis);
+ if (igmp_group_has_chassis_name) {
+ sbrec_igmp_group_set_chassis_name(g, chassis->name);
+ }
return g;
}
diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h
index 326f39db1..eebada968 100644
--- a/controller/ip-mcast.h
+++ b/controller/ip-mcast.h
@@ -39,11 +39,13 @@ struct sbrec_igmp_group *igmp_group_create(
struct ovsdb_idl_txn *idl_txn,
const struct in6_addr *address,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis);
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name);
struct sbrec_igmp_group *igmp_mrouter_create(
struct ovsdb_idl_txn *idl_txn,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis);
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name);
void igmp_group_update_ports(const struct sbrec_igmp_group *g,
struct ovsdb_idl_index *datapaths,
@@ -61,6 +63,7 @@ igmp_mrouter_update_ports(const struct sbrec_igmp_group *g,
void igmp_group_delete(const struct sbrec_igmp_group *g);
bool igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
- struct ovsdb_idl_index *igmp_groups);
+ struct ovsdb_idl_index *igmp_groups,
+ const struct sbrec_chassis *chassis);
#endif /* controller/ip-mcast.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 54e742dfe..7e7bc71b3 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -6136,7 +6136,8 @@ loop_done:
done = chassis_cleanup(ovs_idl_txn, ovnsb_idl_txn, ovs_table,
chassis, chassis_private) && done;
done = encaps_cleanup(ovs_idl_txn, br_int) && done;
- done = igmp_group_cleanup(ovnsb_idl_txn, sbrec_igmp_group) && done;
+ done = igmp_group_cleanup(ovnsb_idl_txn, sbrec_igmp_group, chassis)
+ && done;
if (done) {
poll_immediate_wake();
}
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index bd3bd3d81..faa3f9226 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -180,6 +180,7 @@ struct pinctrl {
bool mac_binding_can_timestamp;
bool fdb_can_timestamp;
bool dns_supports_ovn_owned;
+ bool igmp_group_has_chassis_name;
};
static struct pinctrl pinctrl;
@@ -3591,6 +3592,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
*br_int_name)
notify_pinctrl_handler();
}
+ bool igmp_group_has_chassis_name =
+ sbrec_server_has_igmp_group_table_col_chassis_name(idl);
+ if (igmp_group_has_chassis_name != pinctrl.igmp_group_has_chassis_name) {
+ pinctrl.igmp_group_has_chassis_name = igmp_group_has_chassis_name;
+ notify_pinctrl_handler();
+ }
+
ovs_mutex_unlock(&pinctrl_mutex);
}
@@ -5396,8 +5404,9 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn,
sbrec_igmp = igmp_group_lookup(sbrec_igmp_groups, &mc_group->addr,
local_dp->datapath, chassis);
if (!sbrec_igmp) {
- sbrec_igmp = igmp_group_create(ovnsb_idl_txn, &mc_group->addr,
- local_dp->datapath, chassis);
+ sbrec_igmp = igmp_group_create(
+ ovnsb_idl_txn, &mc_group->addr, local_dp->datapath,
+ chassis, pinctrl.igmp_group_has_chassis_name);
}
igmp_group_update_ports(sbrec_igmp, sbrec_datapath_binding_by_key,
@@ -5412,7 +5421,8 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn,
if (!sbrec_ip_mrouter) {
sbrec_ip_mrouter = igmp_mrouter_create(ovnsb_idl_txn,
local_dp->datapath,
- chassis);
+ chassis,
+
pinctrl.igmp_group_has_chassis_name);
}
igmp_mrouter_update_ports(sbrec_ip_mrouter,
sbrec_datapath_binding_by_key,
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index c32a11cbd..90a6d62b1 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -118,7 +118,7 @@ static const char *rbac_svc_monitor_auth[] =
static const char *rbac_svc_monitor_auth_update[] =
{"status"};
static const char *rbac_igmp_group_auth[] =
- {""};
+ {"chassis_name"};
static const char *rbac_igmp_group_update[] =
{"address", "chassis", "datapath", "ports"};
static const char *rbac_bfd_auth[] =
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index 1d2b3028d..b42f18b04 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
{
"name": "OVN_Southbound",
- "version": "20.31.0",
- "cksum": "2473562445 31224",
+ "version": "20.32.0",
+ "cksum": "1262133774 31276",
"tables": {
"SB_Global": {
"columns": {
@@ -493,7 +493,8 @@
"ports": {"type": {"key": {"type": "uuid",
"refTable": "Port_Binding",
"refType": "weak"},
- "min": 0, "max": "unlimited"}}},
+ "min": 0, "max": "unlimited"}},
+ "chassis_name": {"type": "string"}},
"indexes": [["address", "datapath", "chassis"]],
"isRoot": true},
"Service_Monitor": {
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 1f3b318e0..2de7228e7 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -4767,6 +4767,11 @@ tcp.flags = RST;
<column name="ports">
The destination port bindings for this IGMP group.
</column>
+
+ <column name="chassis_name">
+ The chassis that inserted this record. This column is used for RBAC
+ purposes only.
+ </column>
</table>
<table name="Service_Monitor">
diff --git a/tests/ovn.at b/tests/ovn.at
index 28c6b6c34..b6130d069 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -22951,7 +22951,7 @@ wait_row_count Chassis 1 name=hv3
other_config:ovn-monitor-all='"true"'
# Inject a fake IGMP_Group entry.
dp=$(fetch_column Datapath_Binding _uuid external_ids:name=sw2)
ch=$(fetch_column Chassis _uuid name=hv3)
-ovn-sbctl create IGMP_Group address=239.0.1.42 datapath=$dp chassis=$ch
+ovn-sbctl create IGMP_Group address=239.0.1.42 datapath=$dp chassis=$ch
chassis_name=hv3
ovn-nbctl --wait=hv sync
wait_row_count IGMP_Group 2 address=239.0.1.68
