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. >> --->> 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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
