Re: [ovs-dev] [PATCH ovn v2 2/3] rbac: Restrict IGMP_Group updates to relevant chassis.

2024-02-02 Thread Ales Musil 
On Tue, Jan 30, 2024 at 10:44 PM Mark Michelson  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 
> > ---
> > 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);
> > +

Re: [ovs-dev] [PATCH ovn v2 2/3] rbac: Restrict IGMP_Group updates to relevant chassis.

2024-01-30 Thread Mark Michelson 

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 
---
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

Re: [ovs-dev] [PATCH ovn v2 2/3] rbac: Restrict IGMP_Group updates to relevant chassis.

2024-01-30 Thread 0-day Robot 
Bleep bloop.  Greetings Mark Michelson, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 88 characters long (recommended limit is 79)
#204 FILE: controller/pinctrl.c:5425:
   
pinctrl.igmp_group_has_chassis_name);

Lines checked: 277, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2 2/3] rbac: Restrict IGMP_Group updates to relevant chassis.

2024-01-30 Thread Mark Michelson 
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 
---
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,