On Tue, Aug 11, 2020 at 3:02 PM Dumitru Ceara <[email protected]> wrote:
>
> On 8/11/20 9:34 AM, [email protected] wrote:
> > From: Numan Siddique <[email protected]>
> >
> > If the below SNAT entry is added:
> > ovn-nbctl lr-nat-add snat 172.168.0.120 10.0.0.5
> >
> > And when the logical port with IP - 10.0.0.5, sends a packet destined to
> > outside, gets SNATted to 172.168.0.120 as expected, but for the reverse
> > traffic
> > if the upstream switch sends an ARP request for 172.168.0.120, then OVN
> > doesn't
> > reply and the reply traffic never reaches the logical port.
> >
> > This patch fixes this issue by addding ARP responder flows for SNAT entries.
> >
> > Note that, if 172.168.0.120 happens to be the logical router IP, then it
> > works.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861294
> > Reported-by: Alexander Constantinescu <[email protected]>
> > Signed-off-by: Numan Siddique <[email protected]>
> > ---
>
> Hi Numan,
>
> This patch looks good to me overall. I do think that it would be good to
> have a system test checking that ARP responder works fine for SNAT.
> Except that I have one more minor comment inline.
Thanks Dumitru for the review. I added the system test and submitted v2.
Request to take a look.
Thanks
Numan
>
> Thanks,
> Dumitru
>
> > northd/ovn-northd.8.xml | 18 +++++++++---------
> > northd/ovn-northd.c | 14 ++++++++++++--
> > tests/ovn-northd.at | 8 ++++++++
> > 3 files changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 20fd19450..ee21c825d 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -1952,15 +1952,15 @@ nd_na_router {
> > <li>
> > <p>
> > These flows reply to ARP requests or IPv6 neighbor solicitation
> > - for the virtual IP addresses configured in the router for DNAT
> > - or load balancing.
> > + for the virtual IP addresses configured in the router for NAT
> > + (both DNAT and SNAT) or load balancing.
> > </p>
> >
> > <p>
> > - IPv4: For a configured DNAT IP address or a load balancer
> > - IPv4 VIP <var>A</var>, for each router port <var>P</var> with
> > - Ethernet address <var>E</var>, a priority-90 flow matches
> > - <code>arp.op == 1 && arp.tpa == <var>A</var></code>
> > + IPv4: For a configured NAT (both DNAT and SNAT) IP address or a
> > + load balancer IPv4 VIP <var>A</var>, for each router port
> > + <var>P</var> with Ethernet address <var>E</var>, a priority-90
> > flow
> > + matches <code>arp.op == 1 && arp.tpa ==
> > <var>A</var></code>
> > (ARP request) with the following actions:
> > </p>
> >
> > @@ -1990,9 +1990,9 @@ output;
> > </p>
> >
> > <p>
> > - IPv6: For a configured DNAT IP address or a load balancer
> > - IPv6 VIP <var>A</var>, solicited node address <var>S</var>,
> > - for each router port <var>P</var> with
> > + IPv6: For a configured NAT (both DNAT and SNAT) IP address or a
> > + load balancer IPv6 VIP <var>A</var>, solicited node address
> > + <var>S</var>, for each router port <var>P</var> with
> > Ethernet address <var>E</var>, a priority-90 flow matches
> > <code>inport == <var>P</var> && nd_ns &&
> > ip6.dst == {<var>A</var>, <var>S</var>} &&
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 99eb582c3..3b0272b00 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -8648,6 +8648,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> > * per logical port but DNAT addresses can be handled per datapath
> > * for non gateway router ports.
> > */
> > + struct sset snat_ips = SSET_INITIALIZER(&snat_ips);
> > for (int i = 0; i < od->nbr->n_nat; i++) {
> > struct ovn_nat *nat_entry = &od->nat_entries[i];
> > const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8657,15 +8658,22 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> > continue;
> > }
> >
> > + struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> > + char *ext_addr = nat_entry_is_v6(nat_entry) ?
> > + ext_addrs->ipv6_addrs[0].addr_s :
> > + ext_addrs->ipv4_addrs[0].addr_s;
> > +
> > if (!strcmp(nat->type, "snat")) {
> > - continue;
> > + if (sset_contains(&snat_ips, ext_addr)) {
> > + continue;
> > + }
> > + sset_add(&snat_ips, ext_addr);
> > }
> >
> > /* Priority 91 and 92 flows are added for each gateway router
> > * port to handle the special cases. In case we get the packet
> > * on a regular port, just reply with the port's ETH address.
> > */
> > - struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> > if (nat_entry_is_v6(nat_entry)) {
> > build_lrouter_nd_flow(od, NULL, "nd_na",
> > ext_addrs->ipv6_addrs[0].addr_s,
> > @@ -8679,6 +8687,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> > &nat->header_, lflows);
> > }
> > }
> > + sset_destroy(&snat_ips);
> > +
>
> Nit: no need for the additional newline here.
>
> >
> > /* Drop ARP packets (priority 85). ARP request packets for
> > router's own
> > * IPs are handled with priority-90 flows.
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 7872d5051..8344c7f5e 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -1572,6 +1572,8 @@ ovn-nbctl set logical_router lr options:chassis=ch
> > ovn-nbctl lr-nat-add lr dnat_and_snat 43.43.43.2 42.42.42.2
> > ovn-nbctl lr-nat-add lr dnat 43.43.43.3 42.42.42.3
> > ovn-nbctl lr-nat-add lr dnat_and_snat 43.43.43.4 42.42.42.4 ls-vm
> > 00:00:00:00:00:02
> > +ovn-nbctl lr-nat-add lr snat 43.43.43.150 43.43.43.50
> > +ovn-nbctl lr-nat-add lr snat 43.43.43.150 43.43.43.51
> >
> > ovn-nbctl --wait=sb sync
> >
> > @@ -1594,6 +1596,9 @@ action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
> > # Ingress router port ETH address is used for ARP reply/NA in
> > lr_in_ip_input.
> > AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" |
> > grep "arp\|nd" | sort], [0], [dnl
> > table=3 (lr_in_ip_input ), priority=90 , dnl
> > +match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
> > +action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP
> > reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa;
> > arp.spa = 43.43.43.150; outport = inport; flags.loopback = 1; output;)
> > + table=3 (lr_in_ip_input ), priority=90 , dnl
> > match=(arp.op == 1 && arp.tpa == 43.43.43.2), dnl
> > action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP
> > reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa;
> > arp.spa = 43.43.43.2; outport = inport; flags.loopback = 1; output;)
> > table=3 (lr_in_ip_input ), priority=90 , dnl
> > @@ -1648,6 +1653,9 @@ action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
> > # Priority 90 flows (per router).
> > AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" |
> > grep "arp\|nd" | sort], [0], [dnl
> > table=3 (lr_in_ip_input ), priority=90 , dnl
> > +match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
> > +action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP
> > reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa;
> > arp.spa = 43.43.43.150; outport = inport; flags.loopback = 1; output;)
> > + table=3 (lr_in_ip_input ), priority=90 , dnl
> > match=(arp.op == 1 && arp.tpa == 43.43.43.2), dnl
> > action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP
> > reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa = arp.spa;
> > arp.spa = 43.43.43.2; outport = inport; flags.loopback = 1; output;)
> > table=3 (lr_in_ip_input ), priority=90 , dnl
> >
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
