| Message ID | 20240130210810.548338-2-mmichels@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2,1/3] rbac: Only allow relevant chassis to update service monitors. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 1/30/24 16:08, Mark Michelson wrote: > 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 <mmichels@redhat.com> > --- > 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); checkpatch.py doesn't like the length of this line. Assuming this series is acked, then the person who merges should also include this addition: diff --git a/controller/pinctrl.c b/controller/pinctrl.c index faa3f9226..8817b0ac7 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -5419,10 +5419,11 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn, local_dp->datapath, chassis); if (!sbrec_ip_mrouter) { - sbrec_ip_mrouter = igmp_mrouter_create(ovnsb_idl_txn, - local_dp->datapath, - chassis, - pinctrl.igmp_group_has_chassis_name); + sbrec_ip_mrouter = + igmp_mrouter_create(ovnsb_idl_txn, + local_dp->datapath, + chassis, + pinctrl.igmp_group_has_chassis_name); } igmp_mrouter_update_ports(sbrec_ip_mrouter, sbrec_datapath_binding_by_key, > } > 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
On Tue, Jan 30, 2024 at 10:44 PM Mark Michelson <mmichels@redhat.com> wrote: > On 1/30/24 16:08, Mark Michelson wrote: > > 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 <mmichels@redhat.com> > > --- > > 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); > > checkpatch.py doesn't like the length of this line. Assuming this series > is acked, then the person who merges should also include this addition: > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index faa3f9226..8817b0ac7 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -5419,10 +5419,11 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn, > local_dp->datapath, > chassis); > if (!sbrec_ip_mrouter) { > - sbrec_ip_mrouter = igmp_mrouter_create(ovnsb_idl_txn, > - local_dp->datapath, > - chassis, > - > pinctrl.igmp_group_has_chassis_name); > + sbrec_ip_mrouter = > + igmp_mrouter_create(ovnsb_idl_txn, > + local_dp->datapath, > + chassis, > + pinctrl.igmp_group_has_chassis_name); > } > igmp_mrouter_update_ports(sbrec_ip_mrouter, > sbrec_datapath_binding_by_key, > > > > } > > 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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > With the diff included it looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
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
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 <mmichels@redhat.com> --- 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(-)