[ovs-dev] OVN: add selected mac address to MACAM in update_dynamic_addresses
diff mbox series

Message ID 7e807296ff4af8ba0fbe3beaac2b4461a39c36c7.1542367772.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev] OVN: add selected mac address to MACAM in update_dynamic_addresses
Related show

Commit Message

Lorenzo Bianconi Nov. 16, 2018, 11:31 a.m. UTC
Add selected dynamic mac address to MACAM in update_dynamic_addresses
and not just in in ipam_add_port_addresses/ipam_insert_lsp_addresses
since the second approach can produce a duplicated L2 address in a
IPv6-only network if ipv6_prefix is provided after logical port creation.
The issue can be triggered with the following reproducer:

$ovn-nbctl ls-add sw0
$ovn-nbctl lsp-add sw0 sw0-port1
$ovn-nbctl lsp-set-addresses sw0-port1 "dynamic"
$ovn-nbctl lsp-add sw0 sw0-port2
$ovn-nbctl lsp-set-addresses sw0-port2 "dynamic"
$ovs-vsctl add-port br-int p1 -- \
    set Interface p1 external_ids:iface-id=sw0-port1
$ovs-vsctl add-port br-int p2 -- \
    set Interface p2 external_ids:iface-id=sw0-port2
[..]
$ovn-nbctl --wait=sb set Logical-switch sw0 \
    other_config:ipv6_prefix="aef0::"

$ovn-nbctl list logical_switch_port
_uuid               : 1e0e2ed8-20c6-48dc-bfa8-d823e48c9f45
addresses           : [dynamic]
dhcpv4_options      : []
dhcpv6_options      : []
dynamic_addresses   : "0a:00:00:00:00:01 aef0::800:ff:fe00:1"
enabled             : []
external_ids        : {}
name                : "sw0-port1"
options             : {}
parent_name         : []
port_security       : []
tag                 : []
tag_request         : []
type                : ""
up                  : true

_uuid               : cfeab7fb-e20b-41f1-974c-f99e0b5293d7
addresses           : [dynamic]
dhcpv4_options      : []
dhcpv6_options      : []
dynamic_addresses   : "0a:00:00:00:00:01 aef0::800:ff:fe00:1"
enabled             : []
external_ids        : {}
name                : "sw0-port2"
options             : {}
parent_name         : []
port_security       : []
tag                 : []
tag_request         : []
type                : ""
up                  : true

Fixes: c814545b43ac ("OVN: configure L2 address according to the used IP
address")

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 ovn/northd/ovn-northd.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Mark Michelson Nov. 16, 2018, 8:39 p.m. UTC | #1
Acked-by: Mark Michelson <mmichels@redhat.com>

On 11/16/2018 06:31 AM, Lorenzo Bianconi wrote:
> Add selected dynamic mac address to MACAM in update_dynamic_addresses
> and not just in in ipam_add_port_addresses/ipam_insert_lsp_addresses
> since the second approach can produce a duplicated L2 address in a
> IPv6-only network if ipv6_prefix is provided after logical port creation.
> The issue can be triggered with the following reproducer:
> 
> $ovn-nbctl ls-add sw0
> $ovn-nbctl lsp-add sw0 sw0-port1
> $ovn-nbctl lsp-set-addresses sw0-port1 "dynamic"
> $ovn-nbctl lsp-add sw0 sw0-port2
> $ovn-nbctl lsp-set-addresses sw0-port2 "dynamic"
> $ovs-vsctl add-port br-int p1 -- \
>      set Interface p1 external_ids:iface-id=sw0-port1
> $ovs-vsctl add-port br-int p2 -- \
>      set Interface p2 external_ids:iface-id=sw0-port2
> [..]
> $ovn-nbctl --wait=sb set Logical-switch sw0 \
>      other_config:ipv6_prefix="aef0::"
> 
> $ovn-nbctl list logical_switch_port
> _uuid               : 1e0e2ed8-20c6-48dc-bfa8-d823e48c9f45
> addresses           : [dynamic]
> dhcpv4_options      : []
> dhcpv6_options      : []
> dynamic_addresses   : "0a:00:00:00:00:01 aef0::800:ff:fe00:1"
> enabled             : []
> external_ids        : {}
> name                : "sw0-port1"
> options             : {}
> parent_name         : []
> port_security       : []
> tag                 : []
> tag_request         : []
> type                : ""
> up                  : true
> 
> _uuid               : cfeab7fb-e20b-41f1-974c-f99e0b5293d7
> addresses           : [dynamic]
> dhcpv4_options      : []
> dhcpv6_options      : []
> dynamic_addresses   : "0a:00:00:00:00:01 aef0::800:ff:fe00:1"
> enabled             : []
> external_ids        : {}
> name                : "sw0-port2"
> options             : {}
> parent_name         : []
> port_security       : []
> tag                 : []
> tag_request         : []
> type                : ""
> up                  : true
> 
> Fixes: c814545b43ac ("OVN: configure L2 address according to the used IP
> address")
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   ovn/northd/ovn-northd.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 58bef7de5..4580cd705 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1341,6 +1341,8 @@ update_dynamic_addresses(struct dynamic_address_update *update)
>   
>       struct ds new_addr = DS_EMPTY_INITIALIZER;
>       ds_put_format(&new_addr, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
> +    ipam_insert_mac(&mac, true);
> +
>       if (ip4) {
>           ipam_insert_ip(update->od, ntohl(ip4));
>           ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4));
>
Numan Siddique Nov. 19, 2018, 2:11 p.m. UTC | #2
On Sat, Nov 17, 2018 at 2:10 AM Mark Michelson <mmichels@redhat.com> wrote:

> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> On 11/16/2018 06:31 AM, Lorenzo Bianconi wrote:
> > Add selected dynamic mac address to MACAM in update_dynamic_addresses
> > and not just in in ipam_add_port_addresses/ipam_insert_lsp_addresses
> > since the second approach can produce a duplicated L2 address in a
> > IPv6-only network if ipv6_prefix is provided after logical port creation.
> > The issue can be triggered with the following reproducer:
> >
> > $ovn-nbctl ls-add sw0
> > $ovn-nbctl lsp-add sw0 sw0-port1
> > $ovn-nbctl lsp-set-addresses sw0-port1 "dynamic"
> > $ovn-nbctl lsp-add sw0 sw0-port2
> > $ovn-nbctl lsp-set-addresses sw0-port2 "dynamic"
> > $ovs-vsctl add-port br-int p1 -- \
> >      set Interface p1 external_ids:iface-id=sw0-port1
> > $ovs-vsctl add-port br-int p2 -- \
> >      set Interface p2 external_ids:iface-id=sw0-port2
> > [..]
> > $ovn-nbctl --wait=sb set Logical-switch sw0 \
> >      other_config:ipv6_prefix="aef0::"
> >
> > $ovn-nbctl list logical_switch_port
> > _uuid               : 1e0e2ed8-20c6-48dc-bfa8-d823e48c9f45
> > addresses           : [dynamic]
> > dhcpv4_options      : []
> > dhcpv6_options      : []
> > dynamic_addresses   : "0a:00:00:00:00:01 aef0::800:ff:fe00:1"
> > enabled             : []
> > external_ids        : {}
> > name                : "sw0-port1"
> > options             : {}
> > parent_name         : []
> > port_security       : []
> > tag                 : []
> > tag_request         : []
> > type                : ""
> > up                  : true
> >
> > _uuid               : cfeab7fb-e20b-41f1-974c-f99e0b5293d7
> > addresses           : [dynamic]
> > dhcpv4_options      : []
> > dhcpv6_options      : []
> > dynamic_addresses   : "0a:00:00:00:00:01 aef0::800:ff:fe00:1"
> > enabled             : []
> > external_ids        : {}
> > name                : "sw0-port2"
> > options             : {}
> > parent_name         : []
> > port_security       : []
> > tag                 : []
> > tag_request         : []
> > type                : ""
> > up                  : true
> >
> > Fixes: c814545b43ac ("OVN: configure L2 address according to the used IP
> > address")
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>

Acked-by: Numan Siddique <nusiddiq@redhat.com>

Since you have a reproducer code in the commit message, would it make sense
to
add them as a test case so that we don't regress later ?

Thanks
Numan

> ---
> >   ovn/northd/ovn-northd.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 58bef7de5..4580cd705 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -1341,6 +1341,8 @@ update_dynamic_addresses(struct
> dynamic_address_update *update)
> >
> >       struct ds new_addr = DS_EMPTY_INITIALIZER;
> >       ds_put_format(&new_addr, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
> > +    ipam_insert_mac(&mac, true);
> > +
> >       if (ip4) {
> >           ipam_insert_ip(update->od, ntohl(ip4));
> >           ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4));
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Lorenzo Bianconi Nov. 21, 2018, 9:29 a.m. UTC | #3
> On Sat, Nov 17, 2018 at 2:10 AM Mark Michelson <mmichels@redhat.com> wrote:
> 
> > Acked-by: Mark Michelson <mmichels@redhat.com>
> >
> > On 11/16/2018 06:31 AM, Lorenzo Bianconi wrote:

[...]

> > >
> > > Fixes: c814545b43ac ("OVN: configure L2 address according to the used IP
> > > address")
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >
> 
> Acked-by: Numan Siddique <nusiddiq@redhat.com>
> 
> Since you have a reproducer code in the commit message, would it make sense
> to
> add them as a test case so that we don't regress later ?

Sure, will do in v2. Thanks for the review

Regards,
Lorenzo

> 
> Thanks
> Numan
>

Patch
diff mbox series

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 58bef7de5..4580cd705 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1341,6 +1341,8 @@  update_dynamic_addresses(struct dynamic_address_update *update)
 
     struct ds new_addr = DS_EMPTY_INITIALIZER;
     ds_put_format(&new_addr, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
+    ipam_insert_mac(&mac, true);
+
     if (ip4) {
         ipam_insert_ip(update->od, ntohl(ip4));
         ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4));