On Tue, Jan 30, 2024 at 10:44 PM Mark Michelson <mmich...@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 <mmich...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
With the diff included it looks good to me, thanks.
Acked-by: Ales Musil <amu...@redhat.com>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
