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
> 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
