| Message ID | 20241115234041.1323259-2-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | northd: Fix DHCPv6 flows that result in extra datapath matches. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Sat, Nov 16, 2024 at 12:41 AM Ilya Maximets <i.maximets@ovn.org> wrote: > These packets are for the multicast IPv6 destination, they should have > a multicast match, so they do not contribute into unwildcarding IPv6 > addresses for non-multicast traffic. > > Also, tests were using incorrect destination MAC addresses. They should > be multicast, since the IPv6 destination is multicast. Not broadcast > and not unicast. > > Fixes: 52dae175841e ("ovn-northd: Add logical flows to support DHCPv6") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > Hi Ilya, thank you for the patch. I have one comment down below. northd/northd.c | 4 ++-- > tests/ovn.at | 6 +++--- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index aed1d425f..31fda8bb6 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -9194,8 +9194,8 @@ build_dhcpv6_options_flows(struct ovn_port *op, > ds_clear(&match); > ds_put_format( > &match, "inport == %s && eth.src == %s" > - " && ip6.dst == ff02::1:2 && udp.src == 546 &&" > - " udp.dst == 547", > + " && ip6.mcast && ip6.dst == ff02::1:2" > + " && udp.src == 546 && udp.dst == 547", > inport->json_key, lsp_addrs->ea_s); > > if (is_external) { > diff --git a/tests/ovn.at b/tests/ovn.at > index 39cc79459..63c28f78e 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -7478,7 +7478,7 @@ test_dhcpv6() { > local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 > boot_file=$6 > local fqdn=$7 > > - local req_scapy="Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}')/ \ > + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ > IPv6(dst='ff02::1:2', src='${src_lla}')/ \ > UDP(sport=546, dport=547)/ \ > DHCP6(msgtype=${msg_code}, trid=0x010203)/ \ > @@ -7541,7 +7541,7 @@ test_dhcpv6_release() { > local inport=$1 src_mac=$2 src_lla=$3 offer_ip=$4 > > local ip_scapy="DHCP6OptIAAddress(addr='${offer_ip}', > preflft=0xffffffff, validlft=0xffffffff)" > - local req_scapy="Ether(dst='00:00:00:10:00:01', src='${src_mac}')/ \ > + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ > IPv6(dst='ff02::1:2', src='${src_lla}')/ \ > UDP(sport=546, dport=547)/ \ > DHCP6(msgtype=8, trid=0x010203)/ \ > @@ -20047,7 +20047,7 @@ test_dhcp() { > test_dhcpv6() { > local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 > local req_pkt_in_expected=$6 > - local request=ffffffffffff${src_mac}86dd00000000002a1101${src_lla} > + local request=333300010002${src_mac}86dd00000000002a1101${src_lla} > We should rewrite the hardcoded packets to use scapy whenever we have the opportunity. > # dst ip ff02::1:2 > request=${request}ff020000000000000000000000010002 > # udp header and dhcpv6 header > -- > 2.47.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
On 11/19/24 09:05, Ales Musil wrote: > On Sat, Nov 16, 2024 at 12:41 AM Ilya Maximets <i.maximets@ovn.org> wrote: > >> These packets are for the multicast IPv6 destination, they should have >> a multicast match, so they do not contribute into unwildcarding IPv6 >> addresses for non-multicast traffic. >> >> Also, tests were using incorrect destination MAC addresses. They should >> be multicast, since the IPv6 destination is multicast. Not broadcast >> and not unicast. >> >> Fixes: 52dae175841e ("ovn-northd: Add logical flows to support DHCPv6") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> > > > Hi Ilya, > > thank you for the patch. I have one comment down below. > > northd/northd.c | 4 ++-- >> tests/ovn.at | 6 +++--- >> 2 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index aed1d425f..31fda8bb6 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -9194,8 +9194,8 @@ build_dhcpv6_options_flows(struct ovn_port *op, >> ds_clear(&match); >> ds_put_format( >> &match, "inport == %s && eth.src == %s" >> - " && ip6.dst == ff02::1:2 && udp.src == 546 &&" >> - " udp.dst == 547", >> + " && ip6.mcast && ip6.dst == ff02::1:2" >> + " && udp.src == 546 && udp.dst == 547", >> inport->json_key, lsp_addrs->ea_s); >> >> if (is_external) { >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 39cc79459..63c28f78e 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -7478,7 +7478,7 @@ test_dhcpv6() { >> local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 >> boot_file=$6 >> local fqdn=$7 >> >> - local req_scapy="Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}')/ \ >> + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ >> IPv6(dst='ff02::1:2', src='${src_lla}')/ \ >> UDP(sport=546, dport=547)/ \ >> DHCP6(msgtype=${msg_code}, trid=0x010203)/ \ >> @@ -7541,7 +7541,7 @@ test_dhcpv6_release() { >> local inport=$1 src_mac=$2 src_lla=$3 offer_ip=$4 >> >> local ip_scapy="DHCP6OptIAAddress(addr='${offer_ip}', >> preflft=0xffffffff, validlft=0xffffffff)" >> - local req_scapy="Ether(dst='00:00:00:10:00:01', src='${src_mac}')/ \ >> + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ >> IPv6(dst='ff02::1:2', src='${src_lla}')/ \ >> UDP(sport=546, dport=547)/ \ >> DHCP6(msgtype=8, trid=0x010203)/ \ >> @@ -20047,7 +20047,7 @@ test_dhcp() { >> test_dhcpv6() { >> local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 >> local req_pkt_in_expected=$6 >> - local request=ffffffffffff${src_mac}86dd00000000002a1101${src_lla} >> + local request=333300010002${src_mac}86dd00000000002a1101${src_lla} >> > > We should rewrite the hardcoded packets to use scapy whenever we have the > opportunity. > Thanks Ilya and Ales! I tried to tinker with this a bit and ended up with the following incremental diff. It changes this version of test_dhcpv6() to match what we have in the other test that uses dhcpv6 ("dhcpv6 : 1 HV, 2 LS, 5 LSPs"). Ideally we have a single unified way of generating dhcpv6 packets and we should also do the same thing for dhcpv4 but I didn't do that here. Let me know if you agree with squashing the incremental in before applying. If not, I can apply the patch as-is and we can cleanup the tests in a follow up series. Regards, Dumitru diff --git a/tests/ovn.at b/tests/ovn.at index 63c28f78e6..04014623be 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -19758,6 +19758,7 @@ AT_CLEANUP OVN_FOR_EACH_NORTHD([ AT_SETUP([external logical port]) +AT_SKIP_IF([test $HAVE_SCAPY = no]) ovn_start net_add n1 @@ -20046,49 +20047,41 @@ test_dhcp() { # from the "ovs-ofctl monitor br-int resume" test_dhcpv6() { local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 - local req_pkt_in_expected=$6 - local request=333300010002${src_mac}86dd00000000002a1101${src_lla} - # dst ip ff02::1:2 - request=${request}ff020000000000000000000000010002 - # udp header and dhcpv6 header - request=${request}02220223002affff${msg_code}010203 - # Client identifier - request=${request}0001000a00030001${src_mac} - # IA-NA (Identity Association for Non Temporary Address) - request=${request}0003000c0102030400000e1000001518 - shift; shift; shift; shift; shift; - - local server_mac=000000100001 - local server_lla=fe80000000000000020000fffe100001 - local reply_code=07 - if test $msg_code = 01; then - reply_code=02 + + local request="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ + IPv6(dst='ff02::1:2', src='${src_lla}')/ \ + UDP(sport=546, dport=547)/ \ + DHCP6(msgtype=${msg_code}, trid=0x010203)/ \ + DHCP6OptClientId(duid=DUID_LL(lladdr='${src_mac}'))" + + # Add IA-NA (Identity Association for Non Temporary Address) if msg_code + # is not 11 (information request packet) + if test $msg_code != 11; then + request="${request}/DHCP6OptIA_NA(iaid=0x01020304, T1=3600, T2=5400)" fi - local msg_len=54 - if test $offer_ip = 1; then - msg_len=28 + + local reply_code=7 + if test $msg_code = 1; then + reply_code=2 fi - local reply=${src_mac}${server_mac}86dd0000000000${msg_len}1101 - reply=${reply}${server_lla}${src_lla} - - # udp header and dhcpv6 header - reply=${reply}0223022200${msg_len}ffff${reply_code}010203 - # Client identifier - reply=${reply}0001000a00030001${src_mac} - # IA-NA + + local reply="Ether(dst='${src_mac}', src='00:00:00:10:00:01')/ \ + IPv6(dst='${src_lla}', src='fe80::0200:00ff:fe10:0001')/ \ + UDP(sport=547, dport=546)/ \ + DHCP6(msgtype=${reply_code}, trid=0x010203)/ \ + DHCP6OptClientId(duid=DUID_LL(lladdr='${src_mac}'))" if test $offer_ip != 1; then - reply=${reply}0003002801020304ffffffffffffffff00050018${offer_ip} - reply=${reply}ffffffffffffffff + local ip_scapy="DHCP6OptIAAddress(addr='${offer_ip}', \ + preflft=0xffffffff, validlft=0xffffffff)" + reply="${reply}/DHCP6OptIA_NA(iaid=0x01020304, T1=0xffffffff, \ + T2=0xffffffff, + ianaopts=[[${ip_scapy}]])" fi # Server identifier - reply=${reply}0002000a00030001${server_mac} - - echo $reply | trim_zeros >> ext${inport}_v6.expected - # The inport also receives the request packet since it is connected - # to the br-phys. - #echo $request >> ext${inport}_v6.expected + reply="${reply}/DHCP6OptServerId(duid=DUID_LL(lladdr='00:00:00:10:00:01'))" - as hv1 ovs-appctl netdev-dummy/receive hv${inport}-ext${inport} $request + echo $(fmt_pkt "${reply}") | trim_zeros >> ext${inport}_v6.expected + as hv1 ovs-appctl netdev-dummy/receive hv${inport}-ext${inport} $(fmt_pkt "${request}") } AT_CAPTURE_FILE([ofctl_monitor0_hv1.log]) @@ -20133,10 +20126,11 @@ rm -f ext1_v4.expected rm -f ext1_v4.packets # Send DHCPv6 request -src_mac=f00000000003 -src_lla=fe80000000000000f20000fffe000003 -offer_ip=ae700000000000000000000000000006 -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip +src_mac="f0:00:00:00:00:03" +src_lla="fe80::f200:00ff:fe00:0003" +offer_ip="ae70::6" +msg_type=1 +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip # NXT_RESUMEs should be 2 in hv1. OVS_WAIT_UNTIL([test 2 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) @@ -20218,10 +20212,11 @@ reset_pcap_file hv1-ext1 hv1/ext1 rm -f ext1_v4.expected # Send DHCPv6 request again -src_mac=f00000000003 -src_lla=fe80000000000000f20000fffe000003 -offer_ip=ae700000000000000000000000000006 -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip 1 +src_mac="f0:00:00:00:00:03" +src_lla="fe80::f200:00ff:fe00:0003" +offer_ip="ae70::6" +msg_type=1 +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip # NXT_RESUMEs should be 2 in hv1. OVS_WAIT_UNTIL([test 2 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) @@ -20250,6 +20245,7 @@ reset_pcap_file br-phys hv2/br-phys # From ls1-lp_ext1, send ARP request for the router ip. The ARP # response should come from the router pipeline of hv2. +src_mac=f00000000003 ext1_mac=f00000000003 router_mac=a01000000001 ext1_ip=`ip_to_hex 10 0 0 6` @@ -20320,10 +20316,11 @@ rm -f ext1_v4.expected rm -f ext1_v4.packets # Send DHCPv6 request -src_mac=f00000000003 -src_lla=fe80000000000000f20000fffe000003 -offer_ip=ae700000000000000000000000000006 -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip +src_mac="f0:00:00:00:00:03" +src_lla="fe80::f200:00ff:fe00:0003" +offer_ip="ae70::6" +msg_type=1 +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip # NXT_RESUMEs should be 4 in hv1. OVS_WAIT_UNTIL([test 4 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) @@ -20394,10 +20391,11 @@ rm -f ext1_v4.expected rm -f ext1_v4.packets # Send DHCPv6 request -src_mac=f00000000003 -src_lla=fe80000000000000f20000fffe000003 -offer_ip=ae700000000000000000000000000006 -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip +src_mac="f0:00:00:00:00:03" +src_lla="fe80::f200:00ff:fe00:0003" +offer_ip="ae70::6" +msg_type=1 +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip # NXT_RESUMEs should be 4 in hv1. OVS_WAIT_UNTIL([test 4 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`])
On 11/19/24 23:36, Dumitru Ceara wrote: > On 11/19/24 09:05, Ales Musil wrote: >> On Sat, Nov 16, 2024 at 12:41 AM Ilya Maximets <i.maximets@ovn.org> wrote: >> >>> These packets are for the multicast IPv6 destination, they should have >>> a multicast match, so they do not contribute into unwildcarding IPv6 >>> addresses for non-multicast traffic. >>> >>> Also, tests were using incorrect destination MAC addresses. They should >>> be multicast, since the IPv6 destination is multicast. Not broadcast >>> and not unicast. >>> >>> Fixes: 52dae175841e ("ovn-northd: Add logical flows to support DHCPv6") >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> --- >>> >> >> >> Hi Ilya, >> >> thank you for the patch. I have one comment down below. >> >> northd/northd.c | 4 ++-- >>> tests/ovn.at | 6 +++--- >>> 2 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/northd/northd.c b/northd/northd.c >>> index aed1d425f..31fda8bb6 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -9194,8 +9194,8 @@ build_dhcpv6_options_flows(struct ovn_port *op, >>> ds_clear(&match); >>> ds_put_format( >>> &match, "inport == %s && eth.src == %s" >>> - " && ip6.dst == ff02::1:2 && udp.src == 546 &&" >>> - " udp.dst == 547", >>> + " && ip6.mcast && ip6.dst == ff02::1:2" >>> + " && udp.src == 546 && udp.dst == 547", >>> inport->json_key, lsp_addrs->ea_s); >>> >>> if (is_external) { >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index 39cc79459..63c28f78e 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -7478,7 +7478,7 @@ test_dhcpv6() { >>> local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 >>> boot_file=$6 >>> local fqdn=$7 >>> >>> - local req_scapy="Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}')/ \ >>> + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ >>> IPv6(dst='ff02::1:2', src='${src_lla}')/ \ >>> UDP(sport=546, dport=547)/ \ >>> DHCP6(msgtype=${msg_code}, trid=0x010203)/ \ >>> @@ -7541,7 +7541,7 @@ test_dhcpv6_release() { >>> local inport=$1 src_mac=$2 src_lla=$3 offer_ip=$4 >>> >>> local ip_scapy="DHCP6OptIAAddress(addr='${offer_ip}', >>> preflft=0xffffffff, validlft=0xffffffff)" >>> - local req_scapy="Ether(dst='00:00:00:10:00:01', src='${src_mac}')/ \ >>> + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ >>> IPv6(dst='ff02::1:2', src='${src_lla}')/ \ >>> UDP(sport=546, dport=547)/ \ >>> DHCP6(msgtype=8, trid=0x010203)/ \ >>> @@ -20047,7 +20047,7 @@ test_dhcp() { >>> test_dhcpv6() { >>> local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 >>> local req_pkt_in_expected=$6 >>> - local request=ffffffffffff${src_mac}86dd00000000002a1101${src_lla} >>> + local request=333300010002${src_mac}86dd00000000002a1101${src_lla} >>> >> >> We should rewrite the hardcoded packets to use scapy whenever we have the >> opportunity. >> > > Thanks Ilya and Ales! > > I tried to tinker with this a bit and ended up with the > following incremental diff. It changes this version of > test_dhcpv6() to match what we have in the other test that > uses dhcpv6 ("dhcpv6 : 1 HV, 2 LS, 5 LSPs"). Ideally we > have a single unified way of generating dhcpv6 packets > and we should also do the same thing for dhcpv4 but I didn't > do that here. > > Let me know if you agree with squashing the incremental in > before applying. If not, I can apply the patch as-is and > we can cleanup the tests in a follow up series. In general, I'm not a fan of adding extra changes to fixes, it makes them harder to understand and sometimes harder to backport. The incremental change seems fine (I didn't test it myself), feel free to quash it in, if you think it is needed. Best regards, Ilya Maximets. > > Regards, > Dumitru > > diff --git a/tests/ovn.at b/tests/ovn.at > index 63c28f78e6..04014623be 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -19758,6 +19758,7 @@ AT_CLEANUP > > OVN_FOR_EACH_NORTHD([ > AT_SETUP([external logical port]) > +AT_SKIP_IF([test $HAVE_SCAPY = no]) > ovn_start > > net_add n1 > @@ -20046,49 +20047,41 @@ test_dhcp() { > # from the "ovs-ofctl monitor br-int resume" > test_dhcpv6() { > local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 > - local req_pkt_in_expected=$6 > - local request=333300010002${src_mac}86dd00000000002a1101${src_lla} > - # dst ip ff02::1:2 > - request=${request}ff020000000000000000000000010002 > - # udp header and dhcpv6 header > - request=${request}02220223002affff${msg_code}010203 > - # Client identifier > - request=${request}0001000a00030001${src_mac} > - # IA-NA (Identity Association for Non Temporary Address) > - request=${request}0003000c0102030400000e1000001518 > - shift; shift; shift; shift; shift; > - > - local server_mac=000000100001 > - local server_lla=fe80000000000000020000fffe100001 > - local reply_code=07 > - if test $msg_code = 01; then > - reply_code=02 > + > + local request="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ > + IPv6(dst='ff02::1:2', src='${src_lla}')/ \ > + UDP(sport=546, dport=547)/ \ > + DHCP6(msgtype=${msg_code}, trid=0x010203)/ \ > + DHCP6OptClientId(duid=DUID_LL(lladdr='${src_mac}'))" > + > + # Add IA-NA (Identity Association for Non Temporary Address) if msg_code > + # is not 11 (information request packet) > + if test $msg_code != 11; then > + request="${request}/DHCP6OptIA_NA(iaid=0x01020304, T1=3600, T2=5400)" > fi > - local msg_len=54 > - if test $offer_ip = 1; then > - msg_len=28 > + > + local reply_code=7 > + if test $msg_code = 1; then > + reply_code=2 > fi > - local reply=${src_mac}${server_mac}86dd0000000000${msg_len}1101 > - reply=${reply}${server_lla}${src_lla} > - > - # udp header and dhcpv6 header > - reply=${reply}0223022200${msg_len}ffff${reply_code}010203 > - # Client identifier > - reply=${reply}0001000a00030001${src_mac} > - # IA-NA > + > + local reply="Ether(dst='${src_mac}', src='00:00:00:10:00:01')/ \ > + IPv6(dst='${src_lla}', src='fe80::0200:00ff:fe10:0001')/ \ > + UDP(sport=547, dport=546)/ \ > + DHCP6(msgtype=${reply_code}, trid=0x010203)/ \ > + DHCP6OptClientId(duid=DUID_LL(lladdr='${src_mac}'))" > if test $offer_ip != 1; then > - reply=${reply}0003002801020304ffffffffffffffff00050018${offer_ip} > - reply=${reply}ffffffffffffffff > + local ip_scapy="DHCP6OptIAAddress(addr='${offer_ip}', \ > + preflft=0xffffffff, validlft=0xffffffff)" > + reply="${reply}/DHCP6OptIA_NA(iaid=0x01020304, T1=0xffffffff, \ > + T2=0xffffffff, > + ianaopts=[[${ip_scapy}]])" > fi > # Server identifier > - reply=${reply}0002000a00030001${server_mac} > - > - echo $reply | trim_zeros >> ext${inport}_v6.expected > - # The inport also receives the request packet since it is connected > - # to the br-phys. > - #echo $request >> ext${inport}_v6.expected > + reply="${reply}/DHCP6OptServerId(duid=DUID_LL(lladdr='00:00:00:10:00:01'))" > > - as hv1 ovs-appctl netdev-dummy/receive hv${inport}-ext${inport} $request > + echo $(fmt_pkt "${reply}") | trim_zeros >> ext${inport}_v6.expected > + as hv1 ovs-appctl netdev-dummy/receive hv${inport}-ext${inport} $(fmt_pkt "${request}") > } > > AT_CAPTURE_FILE([ofctl_monitor0_hv1.log]) > @@ -20133,10 +20126,11 @@ rm -f ext1_v4.expected > rm -f ext1_v4.packets > > # Send DHCPv6 request > -src_mac=f00000000003 > -src_lla=fe80000000000000f20000fffe000003 > -offer_ip=ae700000000000000000000000000006 > -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip > +src_mac="f0:00:00:00:00:03" > +src_lla="fe80::f200:00ff:fe00:0003" > +offer_ip="ae70::6" > +msg_type=1 > +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip > > # NXT_RESUMEs should be 2 in hv1. > OVS_WAIT_UNTIL([test 2 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) > @@ -20218,10 +20212,11 @@ reset_pcap_file hv1-ext1 hv1/ext1 > rm -f ext1_v4.expected > > # Send DHCPv6 request again > -src_mac=f00000000003 > -src_lla=fe80000000000000f20000fffe000003 > -offer_ip=ae700000000000000000000000000006 > -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip 1 > +src_mac="f0:00:00:00:00:03" > +src_lla="fe80::f200:00ff:fe00:0003" > +offer_ip="ae70::6" > +msg_type=1 > +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip > > # NXT_RESUMEs should be 2 in hv1. > OVS_WAIT_UNTIL([test 2 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) > @@ -20250,6 +20245,7 @@ reset_pcap_file br-phys hv2/br-phys > > # From ls1-lp_ext1, send ARP request for the router ip. The ARP > # response should come from the router pipeline of hv2. > +src_mac=f00000000003 > ext1_mac=f00000000003 > router_mac=a01000000001 > ext1_ip=`ip_to_hex 10 0 0 6` > @@ -20320,10 +20316,11 @@ rm -f ext1_v4.expected > rm -f ext1_v4.packets > > # Send DHCPv6 request > -src_mac=f00000000003 > -src_lla=fe80000000000000f20000fffe000003 > -offer_ip=ae700000000000000000000000000006 > -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip > +src_mac="f0:00:00:00:00:03" > +src_lla="fe80::f200:00ff:fe00:0003" > +offer_ip="ae70::6" > +msg_type=1 > +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip > > # NXT_RESUMEs should be 4 in hv1. > OVS_WAIT_UNTIL([test 4 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) > @@ -20394,10 +20391,11 @@ rm -f ext1_v4.expected > rm -f ext1_v4.packets > > # Send DHCPv6 request > -src_mac=f00000000003 > -src_lla=fe80000000000000f20000fffe000003 > -offer_ip=ae700000000000000000000000000006 > -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip > +src_mac="f0:00:00:00:00:03" > +src_lla="fe80::f200:00ff:fe00:0003" > +offer_ip="ae70::6" > +msg_type=1 > +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip > > # NXT_RESUMEs should be 4 in hv1. > OVS_WAIT_UNTIL([test 4 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) >
On 11/21/24 00:26, Ilya Maximets wrote: > On 11/19/24 23:36, Dumitru Ceara wrote: >> On 11/19/24 09:05, Ales Musil wrote: >>> On Sat, Nov 16, 2024 at 12:41 AM Ilya Maximets <i.maximets@ovn.org> wrote: >>> >>>> These packets are for the multicast IPv6 destination, they should have >>>> a multicast match, so they do not contribute into unwildcarding IPv6 >>>> addresses for non-multicast traffic. >>>> >>>> Also, tests were using incorrect destination MAC addresses. They should >>>> be multicast, since the IPv6 destination is multicast. Not broadcast >>>> and not unicast. >>>> >>>> Fixes: 52dae175841e ("ovn-northd: Add logical flows to support DHCPv6") >>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>> --- >>>> >>> >>> >>> Hi Ilya, >>> >>> thank you for the patch. I have one comment down below. >>> >>> northd/northd.c | 4 ++-- >>>> tests/ovn.at | 6 +++--- >>>> 2 files changed, 5 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/northd/northd.c b/northd/northd.c >>>> index aed1d425f..31fda8bb6 100644 >>>> --- a/northd/northd.c >>>> +++ b/northd/northd.c >>>> @@ -9194,8 +9194,8 @@ build_dhcpv6_options_flows(struct ovn_port *op, >>>> ds_clear(&match); >>>> ds_put_format( >>>> &match, "inport == %s && eth.src == %s" >>>> - " && ip6.dst == ff02::1:2 && udp.src == 546 &&" >>>> - " udp.dst == 547", >>>> + " && ip6.mcast && ip6.dst == ff02::1:2" >>>> + " && udp.src == 546 && udp.dst == 547", >>>> inport->json_key, lsp_addrs->ea_s); >>>> >>>> if (is_external) { >>>> diff --git a/tests/ovn.at b/tests/ovn.at >>>> index 39cc79459..63c28f78e 100644 >>>> --- a/tests/ovn.at >>>> +++ b/tests/ovn.at >>>> @@ -7478,7 +7478,7 @@ test_dhcpv6() { >>>> local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 >>>> boot_file=$6 >>>> local fqdn=$7 >>>> >>>> - local req_scapy="Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}')/ \ >>>> + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ >>>> IPv6(dst='ff02::1:2', src='${src_lla}')/ \ >>>> UDP(sport=546, dport=547)/ \ >>>> DHCP6(msgtype=${msg_code}, trid=0x010203)/ \ >>>> @@ -7541,7 +7541,7 @@ test_dhcpv6_release() { >>>> local inport=$1 src_mac=$2 src_lla=$3 offer_ip=$4 >>>> >>>> local ip_scapy="DHCP6OptIAAddress(addr='${offer_ip}', >>>> preflft=0xffffffff, validlft=0xffffffff)" >>>> - local req_scapy="Ether(dst='00:00:00:10:00:01', src='${src_mac}')/ \ >>>> + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ >>>> IPv6(dst='ff02::1:2', src='${src_lla}')/ \ >>>> UDP(sport=546, dport=547)/ \ >>>> DHCP6(msgtype=8, trid=0x010203)/ \ >>>> @@ -20047,7 +20047,7 @@ test_dhcp() { >>>> test_dhcpv6() { >>>> local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 >>>> local req_pkt_in_expected=$6 >>>> - local request=ffffffffffff${src_mac}86dd00000000002a1101${src_lla} >>>> + local request=333300010002${src_mac}86dd00000000002a1101${src_lla} >>>> >>> >>> We should rewrite the hardcoded packets to use scapy whenever we have the >>> opportunity. >>> >> >> Thanks Ilya and Ales! >> >> I tried to tinker with this a bit and ended up with the >> following incremental diff. It changes this version of >> test_dhcpv6() to match what we have in the other test that >> uses dhcpv6 ("dhcpv6 : 1 HV, 2 LS, 5 LSPs"). Ideally we >> have a single unified way of generating dhcpv6 packets >> and we should also do the same thing for dhcpv4 but I didn't >> do that here. >> >> Let me know if you agree with squashing the incremental in >> before applying. If not, I can apply the patch as-is and >> we can cleanup the tests in a follow up series. > > In general, I'm not a fan of adding extra changes to fixes, it makes > them harder to understand and sometimes harder to backport. > > The incremental change seems fine (I didn't test it myself), feel free > to quash it in, if you think it is needed. > OK, I decided to apply the patch as is. Let's cleanup tests as a follow up. Applied to main, 24.09 and 24.03. Regards, Dumitru > Best regards, Ilya Maximets. > >> >> Regards, >> Dumitru >> >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 63c28f78e6..04014623be 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -19758,6 +19758,7 @@ AT_CLEANUP >> >> OVN_FOR_EACH_NORTHD([ >> AT_SETUP([external logical port]) >> +AT_SKIP_IF([test $HAVE_SCAPY = no]) >> ovn_start >> >> net_add n1 >> @@ -20046,49 +20047,41 @@ test_dhcp() { >> # from the "ovs-ofctl monitor br-int resume" >> test_dhcpv6() { >> local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 >> - local req_pkt_in_expected=$6 >> - local request=333300010002${src_mac}86dd00000000002a1101${src_lla} >> - # dst ip ff02::1:2 >> - request=${request}ff020000000000000000000000010002 >> - # udp header and dhcpv6 header >> - request=${request}02220223002affff${msg_code}010203 >> - # Client identifier >> - request=${request}0001000a00030001${src_mac} >> - # IA-NA (Identity Association for Non Temporary Address) >> - request=${request}0003000c0102030400000e1000001518 >> - shift; shift; shift; shift; shift; >> - >> - local server_mac=000000100001 >> - local server_lla=fe80000000000000020000fffe100001 >> - local reply_code=07 >> - if test $msg_code = 01; then >> - reply_code=02 >> + >> + local request="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ >> + IPv6(dst='ff02::1:2', src='${src_lla}')/ \ >> + UDP(sport=546, dport=547)/ \ >> + DHCP6(msgtype=${msg_code}, trid=0x010203)/ \ >> + DHCP6OptClientId(duid=DUID_LL(lladdr='${src_mac}'))" >> + >> + # Add IA-NA (Identity Association for Non Temporary Address) if msg_code >> + # is not 11 (information request packet) >> + if test $msg_code != 11; then >> + request="${request}/DHCP6OptIA_NA(iaid=0x01020304, T1=3600, T2=5400)" >> fi >> - local msg_len=54 >> - if test $offer_ip = 1; then >> - msg_len=28 >> + >> + local reply_code=7 >> + if test $msg_code = 1; then >> + reply_code=2 >> fi >> - local reply=${src_mac}${server_mac}86dd0000000000${msg_len}1101 >> - reply=${reply}${server_lla}${src_lla} >> - >> - # udp header and dhcpv6 header >> - reply=${reply}0223022200${msg_len}ffff${reply_code}010203 >> - # Client identifier >> - reply=${reply}0001000a00030001${src_mac} >> - # IA-NA >> + >> + local reply="Ether(dst='${src_mac}', src='00:00:00:10:00:01')/ \ >> + IPv6(dst='${src_lla}', src='fe80::0200:00ff:fe10:0001')/ \ >> + UDP(sport=547, dport=546)/ \ >> + DHCP6(msgtype=${reply_code}, trid=0x010203)/ \ >> + DHCP6OptClientId(duid=DUID_LL(lladdr='${src_mac}'))" >> if test $offer_ip != 1; then >> - reply=${reply}0003002801020304ffffffffffffffff00050018${offer_ip} >> - reply=${reply}ffffffffffffffff >> + local ip_scapy="DHCP6OptIAAddress(addr='${offer_ip}', \ >> + preflft=0xffffffff, validlft=0xffffffff)" >> + reply="${reply}/DHCP6OptIA_NA(iaid=0x01020304, T1=0xffffffff, \ >> + T2=0xffffffff, >> + ianaopts=[[${ip_scapy}]])" >> fi >> # Server identifier >> - reply=${reply}0002000a00030001${server_mac} >> - >> - echo $reply | trim_zeros >> ext${inport}_v6.expected >> - # The inport also receives the request packet since it is connected >> - # to the br-phys. >> - #echo $request >> ext${inport}_v6.expected >> + reply="${reply}/DHCP6OptServerId(duid=DUID_LL(lladdr='00:00:00:10:00:01'))" >> >> - as hv1 ovs-appctl netdev-dummy/receive hv${inport}-ext${inport} $request >> + echo $(fmt_pkt "${reply}") | trim_zeros >> ext${inport}_v6.expected >> + as hv1 ovs-appctl netdev-dummy/receive hv${inport}-ext${inport} $(fmt_pkt "${request}") >> } >> >> AT_CAPTURE_FILE([ofctl_monitor0_hv1.log]) >> @@ -20133,10 +20126,11 @@ rm -f ext1_v4.expected >> rm -f ext1_v4.packets >> >> # Send DHCPv6 request >> -src_mac=f00000000003 >> -src_lla=fe80000000000000f20000fffe000003 >> -offer_ip=ae700000000000000000000000000006 >> -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip >> +src_mac="f0:00:00:00:00:03" >> +src_lla="fe80::f200:00ff:fe00:0003" >> +offer_ip="ae70::6" >> +msg_type=1 >> +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip >> >> # NXT_RESUMEs should be 2 in hv1. >> OVS_WAIT_UNTIL([test 2 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) >> @@ -20218,10 +20212,11 @@ reset_pcap_file hv1-ext1 hv1/ext1 >> rm -f ext1_v4.expected >> >> # Send DHCPv6 request again >> -src_mac=f00000000003 >> -src_lla=fe80000000000000f20000fffe000003 >> -offer_ip=ae700000000000000000000000000006 >> -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip 1 >> +src_mac="f0:00:00:00:00:03" >> +src_lla="fe80::f200:00ff:fe00:0003" >> +offer_ip="ae70::6" >> +msg_type=1 >> +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip >> >> # NXT_RESUMEs should be 2 in hv1. >> OVS_WAIT_UNTIL([test 2 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) >> @@ -20250,6 +20245,7 @@ reset_pcap_file br-phys hv2/br-phys >> >> # From ls1-lp_ext1, send ARP request for the router ip. The ARP >> # response should come from the router pipeline of hv2. >> +src_mac=f00000000003 >> ext1_mac=f00000000003 >> router_mac=a01000000001 >> ext1_ip=`ip_to_hex 10 0 0 6` >> @@ -20320,10 +20316,11 @@ rm -f ext1_v4.expected >> rm -f ext1_v4.packets >> >> # Send DHCPv6 request >> -src_mac=f00000000003 >> -src_lla=fe80000000000000f20000fffe000003 >> -offer_ip=ae700000000000000000000000000006 >> -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip >> +src_mac="f0:00:00:00:00:03" >> +src_lla="fe80::f200:00ff:fe00:0003" >> +offer_ip="ae70::6" >> +msg_type=1 >> +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip >> >> # NXT_RESUMEs should be 4 in hv1. >> OVS_WAIT_UNTIL([test 4 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) >> @@ -20394,10 +20391,11 @@ rm -f ext1_v4.expected >> rm -f ext1_v4.packets >> >> # Send DHCPv6 request >> -src_mac=f00000000003 >> -src_lla=fe80000000000000f20000fffe000003 >> -offer_ip=ae700000000000000000000000000006 >> -test_dhcpv6 1 $src_mac $src_lla 01 $offer_ip >> +src_mac="f0:00:00:00:00:03" >> +src_lla="fe80::f200:00ff:fe00:0003" >> +offer_ip="ae70::6" >> +msg_type=1 >> +test_dhcpv6 1 $src_mac $src_lla $msg_type $offer_ip >> >> # NXT_RESUMEs should be 4 in hv1. >> OVS_WAIT_UNTIL([test 4 = `cat ofctl_monitor0_hv1.log | grep -c NXT_RESUME`]) >> >
diff --git a/northd/northd.c b/northd/northd.c index aed1d425f..31fda8bb6 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -9194,8 +9194,8 @@ build_dhcpv6_options_flows(struct ovn_port *op, ds_clear(&match); ds_put_format( &match, "inport == %s && eth.src == %s" - " && ip6.dst == ff02::1:2 && udp.src == 546 &&" - " udp.dst == 547", + " && ip6.mcast && ip6.dst == ff02::1:2" + " && udp.src == 546 && udp.dst == 547", inport->json_key, lsp_addrs->ea_s); if (is_external) { diff --git a/tests/ovn.at b/tests/ovn.at index 39cc79459..63c28f78e 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -7478,7 +7478,7 @@ test_dhcpv6() { local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 boot_file=$6 local fqdn=$7 - local req_scapy="Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}')/ \ + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ IPv6(dst='ff02::1:2', src='${src_lla}')/ \ UDP(sport=546, dport=547)/ \ DHCP6(msgtype=${msg_code}, trid=0x010203)/ \ @@ -7541,7 +7541,7 @@ test_dhcpv6_release() { local inport=$1 src_mac=$2 src_lla=$3 offer_ip=$4 local ip_scapy="DHCP6OptIAAddress(addr='${offer_ip}', preflft=0xffffffff, validlft=0xffffffff)" - local req_scapy="Ether(dst='00:00:00:10:00:01', src='${src_mac}')/ \ + local req_scapy="Ether(dst='33:33:00:01:00:02', src='${src_mac}')/ \ IPv6(dst='ff02::1:2', src='${src_lla}')/ \ UDP(sport=546, dport=547)/ \ DHCP6(msgtype=8, trid=0x010203)/ \ @@ -20047,7 +20047,7 @@ test_dhcp() { test_dhcpv6() { local inport=$1 src_mac=$2 src_lla=$3 msg_code=$4 offer_ip=$5 local req_pkt_in_expected=$6 - local request=ffffffffffff${src_mac}86dd00000000002a1101${src_lla} + local request=333300010002${src_mac}86dd00000000002a1101${src_lla} # dst ip ff02::1:2 request=${request}ff020000000000000000000000010002 # udp header and dhcpv6 header
These packets are for the multicast IPv6 destination, they should have a multicast match, so they do not contribute into unwildcarding IPv6 addresses for non-multicast traffic. Also, tests were using incorrect destination MAC addresses. They should be multicast, since the IPv6 destination is multicast. Not broadcast and not unicast. Fixes: 52dae175841e ("ovn-northd: Add logical flows to support DHCPv6") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- northd/northd.c | 4 ++-- tests/ovn.at | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)