Hi Dumitru, thanks for the feedback! I will change the test file and send a new patch as you suggested, thanks.
Em sex., 9 de fev. de 2024 às 06:47, Dumitru Ceara <dce...@redhat.com> escreveu: > Hi Roberto, > > Thanks for the fix! I have a few minor comments though. > > > On 2/5/24 10:01, Ales Musil wrote: > > On Mon, Feb 5, 2024 at 2:02 AM Roberto Bartzen Acosta via dev < > > ovs-dev@openvswitch.org> wrote: > > > >> This commit fixes the prefix filter function as the return condition for > >> IPv6 addresses is disabling the advertisement of all learned prefixes > >> regardless of the match with the blacklist or not. > > Please use shorter lines in the commit message. > > >> > >> Reported-at: https://bugs.launchpad.net/ubuntu/+source/ovn/+bug/2046804 > >> Fixes: 57b347c55168 ("ovn-ic: Route advertisement.") > >> Signed-off-by: Roberto Bartzen Acosta <roberto.aco...@luizalabs.com> > > Should we update the .mailmap file too? The AUTHORS file has you listed > with your gmail address. Anyway, that can be a separate patch or I can > just add a commit when applying this fix. > Sure, go ahead with updating the .mailmap if it's not a problem for you. Thanks > > >> --->> ic/ovn-ic.c | 15 +++++--- > >> tests/ovn-ic.at | 100 ++++++++++++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 109 insertions(+), 6 deletions(-) > >> > >> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > >> index 6f8f5734d..bc9aea057 100644 > >> --- a/ic/ovn-ic.c > >> +++ b/ic/ovn-ic.c > >> @@ -1064,12 +1064,15 @@ prefix_is_black_listed(const struct smap > >> *nb_options, > >> continue; > >> } > >> } else { > >> - struct in6_addr mask = ipv6_create_mask(bl_plen); > >> - for (int i = 0; i < 16 && mask.s6_addr[i] != 0; i++) { > >> - if ((prefix->s6_addr[i] & mask.s6_addr[i]) > >> - != (bl_prefix.s6_addr[i] & mask.s6_addr[i])) { > >> - continue; > >> - } > >> + struct in6_addr mask = ipv6_create_mask(plen); > >> + /* First calculate the difference between bl_prefix and > >> prefix, so > >> + * use the bl mask to ensure prefixes are correctly > validated. > >> + * e.g.: 2005:1734:5678::/50 is a subnet of 2005:1234::/21 > */ > >> + struct in6_addr m_prefixes = ipv6_addr_bitand(prefix, > >> &bl_prefix); > >> + struct in6_addr m_prefix = ipv6_addr_bitand(&m_prefixes, > >> &mask); > >> + struct in6_addr m_bl_prefix = ipv6_addr_bitand(&bl_prefix, > >> &mask); > >> + if (!ipv6_addr_equals(&m_prefix, &m_bl_prefix)) { > >> + continue; > >> } > >> } > >> matched = true; > >> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > >> index d4c436f84..1f9df71e9 100644 > >> --- a/tests/ovn-ic.at > >> +++ b/tests/ovn-ic.at > >> @@ -1274,3 +1274,103 @@ OVN_CLEANUP_IC([az1], [az2]) > >> > >> AT_CLEANUP > >> ]) > >> + > >> +OVN_FOR_EACH_NORTHD([ > >> +AT_SETUP([ovn-ic -- route sync -- IPv6 blacklist filter]) > >> +AT_KEYWORDS([IPv6-route-sync-blacklist]) > >> + > >> +ovn_init_ic_db > >> +ovn-ic-nbctl ts-add ts1 > >> + > >> +for i in 1 2; do > >> + ovn_start az$i > >> + ovn_as az$i > >> + > >> + # Enable route learning at AZ level > >> + ovn-nbctl set nb_global . options:ic-route-learn=true > >> + # Enable route advertising at AZ level > >> + ovn-nbctl set nb_global . options:ic-route-adv=true > >> + # Enable blacklist single filter for IPv6 > >> + ovn-nbctl set nb_global . > >> options:ic-route-blacklist="2003:db8:1::/64,\ > >> + 2004:aaaa::/32,2005:1234::/21" > >> + > >> + OVS_WAIT_UNTIL([ovn-nbctl show | grep ts1]) > >> + > >> + # Create LRP and connect to TS > >> + ovn-nbctl lr-add lr$i > >> + ovn-nbctl lrp-add lr$i lrp-lr$i-ts1 aa:aa:aa:aa:aa:0$i > >> 2001:db8:1::$i/64 > >> + ovn-nbctl lsp-add ts1 lsp-ts1-lr$i \ > >> + -- lsp-set-addresses lsp-ts1-lr$i router \ > >> + -- lsp-set-type lsp-ts1-lr$i router \ > >> + -- lsp-set-options lsp-ts1-lr$i router-port=lrp-lr$i-ts1 > >> + > >> + ovn-nbctl lrp-add lr$i lrp-lr$i-p$i 00:00:00:00:00:0$i > >> 2002:db8:1::$i/64 > > Please prefix all ovn-nbctl commands with "check ". That will fail if > the command errors out. It turns out your test adds some duplicate > ports, in the logs: > > ovn-nbctl: lrp-lr1-p-ext21: a port with this name already exists > ovn-nbctl: lrp-lr2-p-ext22: a port with this name already exists > > >> + > >> + # Create blacklisted LRPs and connect to TS > >> + ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext$i \ > >> + 11:11:11:11:11:1$i 2003:db8:1::$i/64 > >> + > >> + ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext2$i \ > >> + 22:22:22:22:22:2$i 2004:aaaa:bbb::$i/48 > >> + > >> + # filtered by 2005:1234::/21 - (2005:1000: - 2005:17ff:) > >> + ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext3$i \ > >> + 33:33:33:33:33:3$i 2005:1734:5678::$i/50 > >> + > >> + # additional not filtered prefix -> different subnet bits > >> + ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext4$i \ > >> + 33:33:33:33:33:3$i 2005:1834:5678::$i/50 > >> + > > Nit: no need for empty line. > > >> +done > >> + > >> +for i in 1 2; do > >> + OVS_WAIT_UNTIL([ovn_as az$i ovn-nbctl lr-route-list lr$i | grep > >> learned]) > >> +done > >> + > >> +AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr1 | > >> + awk '/learned/{print $1, $2}' ], [0], [dnl > >> +2002:db8:1::/64 2001:db8:1::2 > >> +2005:1834:5678::/50 2001:db8:1::2 > >> +]) > >> + > >> +for i in 1 2; do > >> + ovn_as az$i > >> + > >> + # Drop blacklist > >> + ovn-nbctl remove nb_global . options ic-route-blacklist > >> + > > Nit: no need for an empty line here. > > >> +done > >> + > >> +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr1 | > >> + awk '/learned/{print $1, $2}' | sort ], [0], [dnl > >> +2002:db8:1::/64 2001:db8:1::2 > >> +2003:db8:1::/64 2001:db8:1::2 > >> +2004:aaaa:bbb::/48 2001:db8:1::2 > >> +2005:1734:5678::/50 2001:db8:1::2 > >> +2005:1834:5678::/50 2001:db8:1::2 > >> +]) > >> + > >> +for i in 1 2; do > >> + ovn_as az$i > >> + > >> + ovn-nbctl set nb_global . \ > >> + > options:ic-route-blacklist="2003:db8:1::/64,2004:db8:1::/64" > >> + > >> + # Create an 'extra' blacklisted LRP and connect to TS > >> + ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext2$i \ > >> + 44:44:44:44:44:4$i 2004:db8:1::$i/64 > >> + > > Nit: no need for an empty line here. > > >> +done > >> + > >> +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr1 | > >> + awk '/learned/{print $1, $2}' | sort ], [0], [dnl > >> +2002:db8:1::/64 2001:db8:1::2 > >> +2004:aaaa:bbb::/48 2001:db8:1::2 > >> +2005:1734:5678::/50 2001:db8:1::2 > >> +2005:1834:5678::/50 2001:db8:1::2 > >> +]) > >> + > >> +OVN_CLEANUP_IC([az1], [az2]) > >> + > >> +AT_CLEANUP > >> +]) > >> -- > >> 2.25.1 > >> > > Regards, > Dumitru > > -- _‘Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas’._ * **‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
