Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
On 1/22/24 09:35, Ales Musil wrote:
On Mon, Jan 22, 2024 at 9:09 AM Felix Huettner via dev
mailto:ovs-dev@openvswitch.org>> wrote:
On Fri, Jan 19, 2024 at 04:33:28PM -0500, Mark Michelson wrote:
> With this change, a chassis may only update MAC Binding records
that it
> has created. We achieve this by adding a "chassis_name" column to the
> MAC_Binding table, and having the chassis insert its name into this
> column when creating a new MAC_Binding. The "chassis_name" is now
part
> of the rbac_auth structure for the MAC_Binding table.
Hi Mark,
i am concerned that this will negatively impact MAC_Bindings for LRPs
with multiple gateway chassis.
Suppose a MAC_Binding is first learned by an LRP currently residing on
chassis1. The LRP then failovers to chassis2 and chassis1 is
potentially even
removed completely. In this case the ovn-controller on chassis2 would no
longer be allowed to update the timestamp column. This would break the
arp refresh mechanism.
In this case the MAC_Binding would need to expire first, causing northd
to removed it. Afterwards chassis2 would be allowed to insert a new
record with its own chassis name.
I honestly did not try out this case so i am not fully sure if this
issue realy exists or if i have a missunderstanding somewhere.
Thanks
Felix
Hi Mark and Felix,
I personally don't see the ability to not refresh as an issue, the MAC
binding would age out and the node could create a new one. However, it
will still produce errors when the remote chassis tries to update the
timestamp of MAC binding owned by someone else.
There is another issue that I'm more concerned about and that's in case
the aging is not enabled at all. After failover the MAC binding might
not be updated at all. Similar issue applies to MAM bindings distributed
across many chassis. One will own it and only that chassis can update
MAC address when anything changes which it might never do.
To solve that we would need duplicates per chassis, basically the same
MAC binding row, but with different "owners". This goes in hand with
having OF only for MAC bindings owned by current chassis and nothing
else. Does that make sense?
All the above unfortunately applies also to FDB.
Thanks,
Ales
I spent some time today trying to fix the problem in your first
paragraph: chassis may attempt to update timestamps on mac bindings they
do not own. I tried to fix this by making the chassis name part of the
lookup criteria for SB MAC bindings.
This turned out to be a problem. I needed to change the index we use for
MAC binding lookups to use the logical port, IP address, and chassis
name. However, if the chassis name column isn't present, then I need the
index to behave like it currently does and use only the logical port and
IP address. IDL indexes must be created prior to the first call to
ovsdb_idl_run(). This means that if ovn-controller connects to an old SB
DB that doesn't have the chassis name column in the MAC Binding table,
then it can never create an index that includes the chassis name. Even
if the SB DB were updated to have the chassis name, we couldn't look up
MAC bindings properly unless we also restart ovn-controller so that we
create the proper index.
That doesn't even touch on the issue you've brought up about old MAC
bindings never aging out if the binding moves to a new chassis.
And finally, if I factor in Han's reply, I think the appropriate action
at this time is to drop this patch from the series. I will upload a v2
tomorrow that only has patches 2-4.
> ---
> controller/pinctrl.c | 51
> northd/ovn-northd.c | 2 +-
> ovn-sb.ovsschema | 7 +++---
> ovn-sb.xml | 3 +++
> 4 files changed, 45 insertions(+), 18 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 4992eab08..a00cdceea 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 mac_binding_has_chassis_name;
> };
>
> static struct pinctrl pinctrl;
> @@ -204,7 +205,8 @@ static void run_put_mac_bindings(
> struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> struct ovsdb_idl_index *sbrec_port_binding_by_key,
> - struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> + const struct sbrec_chassis *chassis)
> OVS_REQUIRES(pinctrl_mutex);
> static void wait_put_mac_bindings(struct ovsdb_idl_txn
*ovnsb_idl_txn);
> static void send_mac_binding_buffered_pkts(struct rconn
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
On Mon, Jan 22, 2024 at 6:36 AM Ales Musil wrote:
>
> On Mon, Jan 22, 2024 at 9:09 AM Felix Huettner via dev <
> ovs-dev@openvswitch.org> wrote:
>
> > On Fri, Jan 19, 2024 at 04:33:28PM -0500, Mark Michelson wrote:
> > > With this change, a chassis may only update MAC Binding records that
it
> > > has created. We achieve this by adding a "chassis_name" column to the
> > > MAC_Binding table, and having the chassis insert its name into this
> > > column when creating a new MAC_Binding. The "chassis_name" is now part
> > > of the rbac_auth structure for the MAC_Binding table.
> >
> > Hi Mark,
> >
> > i am concerned that this will negatively impact MAC_Bindings for LRPs
> > with multiple gateway chassis.
> >
> > Suppose a MAC_Binding is first learned by an LRP currently residing on
> > chassis1. The LRP then failovers to chassis2 and chassis1 is potentially
> > even
> > removed completely. In this case the ovn-controller on chassis2 would no
> > longer be allowed to update the timestamp column. This would break the
> > arp refresh mechanism.
> >
> > In this case the MAC_Binding would need to expire first, causing northd
> > to removed it. Afterwards chassis2 would be allowed to insert a new
> > record with its own chassis name.
> >
> > I honestly did not try out this case so i am not fully sure if this
> > issue realy exists or if i have a missunderstanding somewhere.
> >
> > Thanks
> > Felix
> >
> >
> Hi Mark and Felix,
>
> I personally don't see the ability to not refresh as an issue, the MAC
> binding would age out and the node could create a new one. However, it
will
> still produce errors when the remote chassis tries to update the timestamp
> of MAC binding owned by someone else.
>
> There is another issue that I'm more concerned about and that's in case
the
> aging is not enabled at all. After failover the MAC binding might not be
> updated at all. Similar issue applies to MAM bindings distributed across
> many chassis. One will own it and only that chassis can update MAC address
> when anything changes which it might never do.
This is indeed a fundamental problem. Even if aging is configured it is
still a problem. Let's say aging is set as 5 min, then when the IP is moved
to a different chassis it will not work within 5 min, which is unacceptable.
In fact the below test case fails with this patch:
81. ovn.at:5232: testing IP relocation using GARP request --
parallelization=yes -- ovn_monitor_all=yes ...
>
> To solve that we would need duplicates per chassis, basically the same MAC
> binding row, but with different "owners". This goes in hand with having OF
> only for MAC bindings owned by current chassis and nothing else. Does that
> make sense?
>
If each chassis has OF only for MAC bindings owned by itself, there is no
point to have MAC binding table in SB DB, right?
But it doesn't work since the MAC binding table works as ARP cache/Neigbor
table for a distributed router, so we will need a central place to share
this information. Otherwise, when a IP moves from one chassis to another,
how would other chassis know? (the same scenario as the test case 81 above,
and similarly there will be problem for DGP failover)
However I don't have a good solution either. Need to think more about it.
Thanks,
Han
> All the above unfortunately applies also to FDB.
>
> Thanks,
> Ales
>
>
> > > ---
> > > controller/pinctrl.c | 51
> > > northd/ovn-northd.c | 2 +-
> > > ovn-sb.ovsschema | 7 +++---
> > > ovn-sb.xml | 3 +++
> > > 4 files changed, 45 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > > index 4992eab08..a00cdceea 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 mac_binding_has_chassis_name;
> > > };
> > >
> > > static struct pinctrl pinctrl;
> > > @@ -204,7 +205,8 @@ static void run_put_mac_bindings(
> > > struct ovsdb_idl_txn *ovnsb_idl_txn,
> > > struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> > > struct ovsdb_idl_index *sbrec_port_binding_by_key,
> > > -struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> > > +struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> > > +const struct sbrec_chassis *chassis)
> > > OVS_REQUIRES(pinctrl_mutex);
> > > static void wait_put_mac_bindings(struct ovsdb_idl_txn
*ovnsb_idl_txn);
> > > static void send_mac_binding_buffered_pkts(struct rconn *swconn)
> > > @@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl,
const
> > char *br_int_name)
> > > notify_pinctrl_handler();
> > > }
> > >
> > > +bool mac_binding_has_chassis_name =
> > > +sbrec_server_has_mac_binding_table_col_chassis_name(idl);
> > > +if (mac_binding_has_chassis_name !=
> >
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
On Mon, Jan 22, 2024 at 9:09 AM Felix Huettner via dev <
ovs-dev@openvswitch.org> wrote:
> On Fri, Jan 19, 2024 at 04:33:28PM -0500, Mark Michelson wrote:
> > With this change, a chassis may only update MAC Binding records that it
> > has created. We achieve this by adding a "chassis_name" column to the
> > MAC_Binding table, and having the chassis insert its name into this
> > column when creating a new MAC_Binding. The "chassis_name" is now part
> > of the rbac_auth structure for the MAC_Binding table.
>
> Hi Mark,
>
> i am concerned that this will negatively impact MAC_Bindings for LRPs
> with multiple gateway chassis.
>
> Suppose a MAC_Binding is first learned by an LRP currently residing on
> chassis1. The LRP then failovers to chassis2 and chassis1 is potentially
> even
> removed completely. In this case the ovn-controller on chassis2 would no
> longer be allowed to update the timestamp column. This would break the
> arp refresh mechanism.
>
> In this case the MAC_Binding would need to expire first, causing northd
> to removed it. Afterwards chassis2 would be allowed to insert a new
> record with its own chassis name.
>
> I honestly did not try out this case so i am not fully sure if this
> issue realy exists or if i have a missunderstanding somewhere.
>
> Thanks
> Felix
>
>
Hi Mark and Felix,
I personally don't see the ability to not refresh as an issue, the MAC
binding would age out and the node could create a new one. However, it will
still produce errors when the remote chassis tries to update the timestamp
of MAC binding owned by someone else.
There is another issue that I'm more concerned about and that's in case the
aging is not enabled at all. After failover the MAC binding might not be
updated at all. Similar issue applies to MAM bindings distributed across
many chassis. One will own it and only that chassis can update MAC address
when anything changes which it might never do.
To solve that we would need duplicates per chassis, basically the same MAC
binding row, but with different "owners". This goes in hand with having OF
only for MAC bindings owned by current chassis and nothing else. Does that
make sense?
All the above unfortunately applies also to FDB.
Thanks,
Ales
> > ---
> > controller/pinctrl.c | 51
> > northd/ovn-northd.c | 2 +-
> > ovn-sb.ovsschema | 7 +++---
> > ovn-sb.xml | 3 +++
> > 4 files changed, 45 insertions(+), 18 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index 4992eab08..a00cdceea 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 mac_binding_has_chassis_name;
> > };
> >
> > static struct pinctrl pinctrl;
> > @@ -204,7 +205,8 @@ static void run_put_mac_bindings(
> > struct ovsdb_idl_txn *ovnsb_idl_txn,
> > struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> > struct ovsdb_idl_index *sbrec_port_binding_by_key,
> > -struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> > +struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> > +const struct sbrec_chassis *chassis)
> > OVS_REQUIRES(pinctrl_mutex);
> > static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
> > static void send_mac_binding_buffered_pkts(struct rconn *swconn)
> > @@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const
> char *br_int_name)
> > notify_pinctrl_handler();
> > }
> >
> > +bool mac_binding_has_chassis_name =
> > +sbrec_server_has_mac_binding_table_col_chassis_name(idl);
> > +if (mac_binding_has_chassis_name !=
> pinctrl.mac_binding_has_chassis_name) {
> > +pinctrl.mac_binding_has_chassis_name =
> mac_binding_has_chassis_name;
> > +notify_pinctrl_handler();
> > +}
> > +
> > ovs_mutex_unlock(_mutex);
> > }
> >
> > @@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > ovs_mutex_lock(_mutex);
> > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> > sbrec_port_binding_by_key,
> > - sbrec_mac_binding_by_lport_ip);
> > + sbrec_mac_binding_by_lport_ip,
> > + chassis);
> > run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> > sbrec_port_binding_by_key, chassis);
> > send_garp_rarp_prepare(ovnsb_idl_txn,
> sbrec_port_binding_by_datapath,
> > @@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> >const char *logical_port,
> >const struct sbrec_datapath_binding *dp,
> >struct eth_addr ea, const char *ip,
> > - bool update_only)
> > + bool
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
On Fri, Jan 19, 2024 at 04:33:28PM -0500, Mark Michelson wrote:
> With this change, a chassis may only update MAC Binding records that it
> has created. We achieve this by adding a "chassis_name" column to the
> MAC_Binding table, and having the chassis insert its name into this
> column when creating a new MAC_Binding. The "chassis_name" is now part
> of the rbac_auth structure for the MAC_Binding table.
Hi Mark,
i am concerned that this will negatively impact MAC_Bindings for LRPs
with multiple gateway chassis.
Suppose a MAC_Binding is first learned by an LRP currently residing on
chassis1. The LRP then failovers to chassis2 and chassis1 is potentially even
removed completely. In this case the ovn-controller on chassis2 would no
longer be allowed to update the timestamp column. This would break the
arp refresh mechanism.
In this case the MAC_Binding would need to expire first, causing northd
to removed it. Afterwards chassis2 would be allowed to insert a new
record with its own chassis name.
I honestly did not try out this case so i am not fully sure if this
issue realy exists or if i have a missunderstanding somewhere.
Thanks
Felix
> ---
> controller/pinctrl.c | 51
> northd/ovn-northd.c | 2 +-
> ovn-sb.ovsschema | 7 +++---
> ovn-sb.xml | 3 +++
> 4 files changed, 45 insertions(+), 18 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 4992eab08..a00cdceea 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 mac_binding_has_chassis_name;
> };
>
> static struct pinctrl pinctrl;
> @@ -204,7 +205,8 @@ static void run_put_mac_bindings(
> struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> struct ovsdb_idl_index *sbrec_port_binding_by_key,
> -struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> +struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> +const struct sbrec_chassis *chassis)
> OVS_REQUIRES(pinctrl_mutex);
> static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
> static void send_mac_binding_buffered_pkts(struct rconn *swconn)
> @@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
> *br_int_name)
> notify_pinctrl_handler();
> }
>
> +bool mac_binding_has_chassis_name =
> +sbrec_server_has_mac_binding_table_col_chassis_name(idl);
> +if (mac_binding_has_chassis_name !=
> pinctrl.mac_binding_has_chassis_name) {
> +pinctrl.mac_binding_has_chassis_name = mac_binding_has_chassis_name;
> +notify_pinctrl_handler();
> +}
> +
> ovs_mutex_unlock(_mutex);
> }
>
> @@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> ovs_mutex_lock(_mutex);
> run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_key,
> - sbrec_mac_binding_by_lport_ip);
> + sbrec_mac_binding_by_lport_ip,
> + chassis);
> run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_key, chassis);
> send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
> @@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>const char *logical_port,
>const struct sbrec_datapath_binding *dp,
>struct eth_addr ea, const char *ip,
> - bool update_only)
> + bool update_only,
> + const struct sbrec_chassis *chassis)
> {
> /* Convert ethernet argument to string form for database. */
> char mac_string[ETH_ADDR_STRLEN + 1];
> @@ -4302,6 +4313,9 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> sbrec_mac_binding_set_logical_port(b, logical_port);
> sbrec_mac_binding_set_ip(b, ip);
> sbrec_mac_binding_set_datapath(b, dp);
> +if (pinctrl.mac_binding_has_chassis_name) {
> +sbrec_mac_binding_set_chassis_name(b, chassis->name);
> +}
> }
>
> if (strcmp(b->mac, mac_string)) {
> @@ -4323,7 +4337,8 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
>struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
>const struct hmap *local_datapaths,
>const struct sbrec_port_binding *in_pb,
> - struct eth_addr ea, ovs_be32 ip)
> + struct eth_addr ea, ovs_be32 ip,
> + const struct sbrec_chassis *chassis)
> {
> if (!ovnsb_idl_txn) {
> return;
> @@ -4351,7 +4366,7 @@ send_garp_locally(struct ovsdb_idl_txn
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
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: ERROR: Author Mark Michelson needs to sign off. WARNING: Line is 80 characters long (recommended limit is 79) #225 FILE: ovn-sb.ovsschema:289: "refTable": "Datapath_Binding"}}}, Lines checked: 247, Warnings: 1, Errors: 1 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
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
Whoops, I forgot to add
Reported-at: https://issues.redhat.com/browse/FDP-155
to this series. I can add this in v2. I'll wait to post a v2 until I get
some feedback on this series.
On 1/19/24 16:33, Mark Michelson wrote:
With this change, a chassis may only update MAC Binding records that it
has created. We achieve this by adding a "chassis_name" column to the
MAC_Binding table, and having the chassis insert its name into this
column when creating a new MAC_Binding. The "chassis_name" is now part
of the rbac_auth structure for the MAC_Binding table.
---
controller/pinctrl.c | 51
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema | 7 +++---
ovn-sb.xml | 3 +++
4 files changed, 45 insertions(+), 18 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 4992eab08..a00cdceea 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 mac_binding_has_chassis_name;
};
static struct pinctrl pinctrl;
@@ -204,7 +205,8 @@ static void run_put_mac_bindings(
struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
-struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
+struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+const struct sbrec_chassis *chassis)
OVS_REQUIRES(pinctrl_mutex);
static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
static void send_mac_binding_buffered_pkts(struct rconn *swconn)
@@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
*br_int_name)
notify_pinctrl_handler();
}
+bool mac_binding_has_chassis_name =
+sbrec_server_has_mac_binding_table_col_chassis_name(idl);
+if (mac_binding_has_chassis_name != pinctrl.mac_binding_has_chassis_name) {
+pinctrl.mac_binding_has_chassis_name = mac_binding_has_chassis_name;
+notify_pinctrl_handler();
+}
+
ovs_mutex_unlock(_mutex);
}
@@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
ovs_mutex_lock(_mutex);
run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key,
- sbrec_mac_binding_by_lport_ip);
+ sbrec_mac_binding_by_lport_ip,
+ chassis);
run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key, chassis);
send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
@@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
const char *logical_port,
const struct sbrec_datapath_binding *dp,
struct eth_addr ea, const char *ip,
- bool update_only)
+ bool update_only,
+ const struct sbrec_chassis *chassis)
{
/* Convert ethernet argument to string form for database. */
char mac_string[ETH_ADDR_STRLEN + 1];
@@ -4302,6 +4313,9 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
sbrec_mac_binding_set_logical_port(b, logical_port);
sbrec_mac_binding_set_ip(b, ip);
sbrec_mac_binding_set_datapath(b, dp);
+if (pinctrl.mac_binding_has_chassis_name) {
+sbrec_mac_binding_set_chassis_name(b, chassis->name);
+}
}
if (strcmp(b->mac, mac_string)) {
@@ -4323,7 +4337,8 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
const struct hmap *local_datapaths,
const struct sbrec_port_binding *in_pb,
- struct eth_addr ea, ovs_be32 ip)
+ struct eth_addr ea, ovs_be32 ip,
+ const struct sbrec_chassis *chassis)
{
if (!ovnsb_idl_txn) {
return;
@@ -4351,7 +4366,7 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
ip_format_masked(ip, OVS_BE32_MAX, _s);
mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
remote->logical_port, remote->datapath,
- ea, ds_cstr(_s), update_only);
+ ea, ds_cstr(_s), update_only, chassis);
ds_destroy(_s);
}
}
@@ -4361,7 +4376,8 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
-const struct
[ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
With this change, a chassis may only update MAC Binding records that it
has created. We achieve this by adding a "chassis_name" column to the
MAC_Binding table, and having the chassis insert its name into this
column when creating a new MAC_Binding. The "chassis_name" is now part
of the rbac_auth structure for the MAC_Binding table.
---
controller/pinctrl.c | 51
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema | 7 +++---
ovn-sb.xml | 3 +++
4 files changed, 45 insertions(+), 18 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 4992eab08..a00cdceea 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 mac_binding_has_chassis_name;
};
static struct pinctrl pinctrl;
@@ -204,7 +205,8 @@ static void run_put_mac_bindings(
struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
-struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
+struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+const struct sbrec_chassis *chassis)
OVS_REQUIRES(pinctrl_mutex);
static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
static void send_mac_binding_buffered_pkts(struct rconn *swconn)
@@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
*br_int_name)
notify_pinctrl_handler();
}
+bool mac_binding_has_chassis_name =
+sbrec_server_has_mac_binding_table_col_chassis_name(idl);
+if (mac_binding_has_chassis_name != pinctrl.mac_binding_has_chassis_name) {
+pinctrl.mac_binding_has_chassis_name = mac_binding_has_chassis_name;
+notify_pinctrl_handler();
+}
+
ovs_mutex_unlock(_mutex);
}
@@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
ovs_mutex_lock(_mutex);
run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key,
- sbrec_mac_binding_by_lport_ip);
+ sbrec_mac_binding_by_lport_ip,
+ chassis);
run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key, chassis);
send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
@@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
const char *logical_port,
const struct sbrec_datapath_binding *dp,
struct eth_addr ea, const char *ip,
- bool update_only)
+ bool update_only,
+ const struct sbrec_chassis *chassis)
{
/* Convert ethernet argument to string form for database. */
char mac_string[ETH_ADDR_STRLEN + 1];
@@ -4302,6 +4313,9 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
sbrec_mac_binding_set_logical_port(b, logical_port);
sbrec_mac_binding_set_ip(b, ip);
sbrec_mac_binding_set_datapath(b, dp);
+if (pinctrl.mac_binding_has_chassis_name) {
+sbrec_mac_binding_set_chassis_name(b, chassis->name);
+}
}
if (strcmp(b->mac, mac_string)) {
@@ -4323,7 +4337,8 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
const struct hmap *local_datapaths,
const struct sbrec_port_binding *in_pb,
- struct eth_addr ea, ovs_be32 ip)
+ struct eth_addr ea, ovs_be32 ip,
+ const struct sbrec_chassis *chassis)
{
if (!ovnsb_idl_txn) {
return;
@@ -4351,7 +4366,7 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
ip_format_masked(ip, OVS_BE32_MAX, _s);
mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
remote->logical_port, remote->datapath,
- ea, ds_cstr(_s), update_only);
+ ea, ds_cstr(_s), update_only, chassis);
ds_destroy(_s);
}
}
@@ -4361,7 +4376,8 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
-const struct mac_binding *mb)
+const struct mac_binding *mb,
+const struct sbrec_chassis *chassis)
{
/* Convert logical datapath and logical port key into lport. */
const struct sbrec_port_binding *pb = lport_lookup_by_key(
@@ -4384,7 +4400,7 @@
