diff mbox series

[ovs-dev,v2,ovn] northd: manage ARP request locally for FIP traffic

Message ID 92f6a2f668708c677a8b10b0ac861bfd712f6a20.1590511504.git.lorenzo.bianconi@redhat.com
State Accepted
Headers show
Series [ovs-dev,v2,ovn] northd: manage ARP request locally for FIP traffic | expand

Commit Message

Lorenzo Bianconi May 26, 2020, 4:46 p.m. UTC
Modify 100-priority logical flows in Gateway Redirect table of
logical router ingress pipeline (table 15) in order to manage ARP
request locally for FIP traffic. In particular set reg1 and eth.src
to NAT external ip and NAT external mac respectively and do not
distribute ARP traffic using FIP

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
changes since v1:
- squash 150 and 100 prio logical flows in S_ROUTER_IN_GW_REDIRECT
- use NS_CHECK_EXEC
- improve comments/documentation
---
 northd/ovn-northd.8.xml | 10 +++++++---
 northd/ovn-northd.c     | 23 ++++++++++++++++-------
 tests/ovn.at            | 28 +++++++++++++++++++++++++---
 tests/system-ovn.at     | 30 ++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 13 deletions(-)

Comments

Han Zhou May 26, 2020, 7:56 p.m. UTC | #1
On Tue, May 26, 2020 at 9:47 AM Lorenzo Bianconi <
lorenzo.bianconi@redhat.com> wrote:
>
> Modify 100-priority logical flows in Gateway Redirect table of
> logical router ingress pipeline (table 15) in order to manage ARP
> request locally for FIP traffic. In particular set reg1 and eth.src
> to NAT external ip and NAT external mac respectively and do not
> distribute ARP traffic using FIP
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> changes since v1:
> - squash 150 and 100 prio logical flows in S_ROUTER_IN_GW_REDIRECT
> - use NS_CHECK_EXEC
> - improve comments/documentation
> ---
>  northd/ovn-northd.8.xml | 10 +++++++---
>  northd/ovn-northd.c     | 23 ++++++++++++++++-------
>  tests/ovn.at            | 28 +++++++++++++++++++++++++---
>  tests/system-ovn.at     | 30 ++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 13 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 9423cbfd1..dc56de273 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2879,9 +2879,13 @@ icmp4 {
>          For each NAT rule in the OVN Northbound database that can
>          be handled in a distributed manner, a priority-100 logical
>          flow with match <code>ip4.src == <var>B</var> &amp;&amp;
> -        outport == <var>GW</var></code>, where <var>GW</var> is
> -        the logical router distributed gateway port, with actions
> -        <code>next;</code>.
> +        outport == <var>GW</var></code> &amp;&amp;
> +        is_chassis_resident(<var>P</var>), where <var>GW</var> is
> +        the logical router distributed gateway port and <var>P</var>
> +        is the NAT logical port. IP traffic matching the above rule
> +        will be managed locally setting <code>reg1</code> to <var>C</var>
> +        and <code>eth.src</code> to <var>D</var>, where <var>C</var> is
NAT
> +        external ip and <var>D</var> is NAT external mac.
>        </li>
>
>        <li>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 7ae5e45da..2279c33ac 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -9181,16 +9181,25 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
>              /* Ingress Gateway Redirect Table: For NAT on a distributed
>               * router, add flows that are specific to a NAT rule.  These
>               * flows indicate the presence of an applicable NAT rule that
> -             * can be applied in a distributed manner. */
> +             * can be applied in a distributed manner.
> +             * In particulr reg1 and eth.src are set to NAT external IP
and
> +             * NAT external mac so the ARP request generated in the
following
> +             * stage is sent out with proper IP/MAC src addresses
> +             */
>              if (distributed) {
>                  ds_clear(&match);
> -                ds_put_format(&match, "ip%s.src == %s && outport == %s",
> -                              is_v6 ? "6" : "4",
> -                              nat->logical_ip,
> -                              od->l3dgw_port->json_key);
> +                ds_clear(&actions);
> +                ds_put_format(&match,
> +                              "ip%s.src == %s && outport == %s && "
> +                              "is_chassis_resident(\"%s\")",
> +                              is_v6 ? "6" : "4", nat->logical_ip,
> +                              od->l3dgw_port->json_key,
nat->logical_port);
> +                ds_put_format(&actions, "eth.src = %s; %sreg1 = %s;
next;",
> +                              nat->external_mac, is_v6 ? "xx" : "",
> +                              nat->external_ip);
>                  ovn_lflow_add_with_hint(lflows, od,
S_ROUTER_IN_GW_REDIRECT,
> -                                        100, ds_cstr(&match), "next;",
> -                                        &nat->header_);
> +                                        100, ds_cstr(&match),
> +                                        ds_cstr(&actions),
&nat->header_);
>              }
>
>              /* Egress Loopback table: For NAT on a distributed router.
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 8fa1a7e1b..15b40ca1e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -14943,9 +14943,14 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
>      set interface hv2-vif1 external-ids:iface-id=sw1-p0 \
>      options:tx_pcap=hv2/vif1-tx.pcap \
>      options:rxq_pcap=hv2/vif1-rx.pcap \
> -    ofport-request=1
> +    ofport-request=2
> +ovs-vsctl -- add-port br-int hv2-vif2 -- \
> +    set interface hv2-vif2 external-ids:iface-id=sw0-p1 \
> +    options:tx_pcap=hv2/vif2-tx.pcap \
> +    options:rxq_pcap=hv2/vif2-rx.pcap \
> +    ofport-request=3
>
> -ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
> +ovn-nbctl create Logical_Router name=lr0
>  ovn-nbctl ls-add sw0
>  ovn-nbctl ls-add sw1
>
> @@ -14954,13 +14959,16 @@ ovn-nbctl lsp-add sw0 rp-sw0 -- set
Logical_Switch_Port rp-sw0 \
>      type=router options:router-port=sw0 \
>      -- lsp-set-addresses rp-sw0 router
>
> -ovn-nbctl lrp-add lr0 sw1 00:00:02:01:02:03 172.16.1.1/24
2002:0:0:0:0:0:0:1/64
> +ovn-nbctl lrp-add lr0 sw1 00:00:02:01:02:03 172.16.1.1/24
2002:0:0:0:0:0:0:1/64 \
> +    -- set Logical_Router_Port sw1 options:redirect-chassis="hv2"
>  ovn-nbctl lsp-add sw1 rp-sw1 -- set Logical_Switch_Port rp-sw1 \
>      type=router options:router-port=sw1 \
>      -- lsp-set-addresses rp-sw1 router
>
>  ovn-nbctl lsp-add sw0 sw0-p0 \
>      -- lsp-set-addresses sw0-p0 "f0:00:00:01:02:03 192.168.1.2 2001::2"
> +ovn-nbctl lsp-add sw0 sw0-p1 \
> +    -- lsp-set-addresses sw0-p1 "f0:00:00:11:02:03 192.168.1.3 2001::3"
>
>  ovn-nbctl lsp-add sw1 sw1-p0 \
>      -- lsp-set-addresses sw1-p0 unknown
> @@ -15006,6 +15014,20 @@ send_na 2 1 $dst_mac $router_mac1 $dst_ip6
$router_ip6
>
>  OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
>
> +# Create FIP on sw0-p0, add a route on logical router pipeline and
> +# ARP request for a unkwon destination is sent using FIP MAC/IP
> +ovn-nbctl lr-nat-add lr0 dnat_and_snat 172.16.1.2 192.168.1.3 sw0-p1
f0:00:00:01:02:04
> +ovn-nbctl lr-route-add lr0 172.16.2.0/24 172.16.1.11
> +
> +dst_ip=$(ip_to_hex 172 16 2 10)
> +fip_ip=$(ip_to_hex 172 16 1 2)
> +src_ip=$(ip_to_hex 192 168 1 3)
> +gw_router=$(ip_to_hex 172 16 1 11)
> +send_icmp_packet 2 2 f00000110203 $router_mac0 $src_ip $dst_ip 0000 $data
> +echo $(get_arp_req f00000010204 $fip_ip $gw_router) >> expected
> +
> +OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
> +
>  OVN_CLEANUP([hv1],[hv2])
>  AT_CLEANUP
>
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 9ae6c6b1f..9dfe6a4ad 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -2747,6 +2747,19 @@ ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24",
"f0:00:00:01:02:05", \
>  ovn-nbctl lsp-add alice alice1 \
>  -- lsp-set-addresses alice1 "f0:00:00:01:02:05 172.16.1.2"
>
> +# Add external network
> +ADD_NAMESPACES(ext-net)
> +AT_CHECK([ip link add alice-ext netns alice1 type veth peer name
ext-veth netns ext-net])
> +NS_CHECK_EXEC([ext-net], [ip link set dev ext-veth up], [0], [])
> +NS_CHECK_EXEC([ext-net], [ip addr add 10.0.0.1/24 dev ext-veth], [0], [])
> +NS_CHECK_EXEC([ext-net], [ip route add default via 10.0.0.2], [0], [])
> +
> +NS_CHECK_EXEC([alice1], [ip link set dev alice-ext up], [0], [])
> +NS_CHECK_EXEC([alice1], [ip addr add 10.0.0.2/24 dev alice-ext], [0], [])
> +NS_CHECK_EXEC([alice1], [sysctl -w net.ipv4.conf.all.forwarding=1],[0],
[dnl
> +net.ipv4.conf.all.forwarding = 1
> +])
> +
>  # Add DNAT rules
>  AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.3 192.168.1.2
foo1 00:00:02:02:03:04])
>  AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4 192.168.1.3
foo2 00:00:02:02:03:05])
> @@ -2754,6 +2767,9 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat
172.16.1.4 192.168.1.3 foo2 00:0
>  # Add a SNAT rule
>  AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.0.0/16])
>
> +# Add default route to ext-net
> +AT_CHECK([ovn-nbctl lr-route-add R1 10.0.0.0/24 172.16.1.2])
> +
>  ovn-nbctl --wait=hv sync
>  OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep
'nat(src=172.16.1.1)'])
>
> @@ -2797,6 +2813,20 @@ sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0],
[dnl
>
 icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>  ])
>
> +# Try to ping external network
> +NS_CHECK_EXEC([ext-net], [tcpdump -n -c 3 -i ext-veth dst 172.16.1.3 and
icmp > ext-net.pcap &])
> +sleep 1
> +AT_CHECK([ovn-nbctl lr-nat-del R1 snat])
> +NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.1 | FORMAT_PING],
\
> +[0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +OVS_WAIT_UNTIL([
> +    total_pkts=$(cat ext-net.pcap | wc -l)
> +    test "${total_pkts}" = "3"
> +])
> +
>  OVS_APP_EXIT_AND_WAIT([ovn-controller])
>
>  as ovn-sb
> --
> 2.26.2
>

Acked-by: Han Zhou <hzhou@ovn.org>
I'd wait for ack from Dumitru before merge it.
Dumitru Ceara May 27, 2020, 10:18 a.m. UTC | #2
On 5/26/20 6:46 PM, Lorenzo Bianconi wrote:
> Modify 100-priority logical flows in Gateway Redirect table of
> logical router ingress pipeline (table 15) in order to manage ARP
> request locally for FIP traffic. In particular set reg1 and eth.src
> to NAT external ip and NAT external mac respectively and do not
> distribute ARP traffic using FIP
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Looks good to me, thanks!

Acked-by: Dumitru Ceara <dceara@redhat.com>
Han Zhou May 27, 2020, 7:50 p.m. UTC | #3
On Wed, May 27, 2020 at 3:18 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 5/26/20 6:46 PM, Lorenzo Bianconi wrote:
> > Modify 100-priority logical flows in Gateway Redirect table of
> > logical router ingress pipeline (table 15) in order to manage ARP
> > request locally for FIP traffic. In particular set reg1 and eth.src
> > to NAT external ip and NAT external mac respectively and do not
> > distribute ARP traffic using FIP
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>
> Looks good to me, thanks!
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>

Thanks Lorenzo and Dumitru. I applied this to master.
diff mbox series

Patch

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9423cbfd1..dc56de273 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2879,9 +2879,13 @@  icmp4 {
         For each NAT rule in the OVN Northbound database that can
         be handled in a distributed manner, a priority-100 logical
         flow with match <code>ip4.src == <var>B</var> &amp;&amp;
-        outport == <var>GW</var></code>, where <var>GW</var> is
-        the logical router distributed gateway port, with actions
-        <code>next;</code>.
+        outport == <var>GW</var></code> &amp;&amp;
+        is_chassis_resident(<var>P</var>), where <var>GW</var> is
+        the logical router distributed gateway port and <var>P</var>
+        is the NAT logical port. IP traffic matching the above rule
+        will be managed locally setting <code>reg1</code> to <var>C</var>
+        and <code>eth.src</code> to <var>D</var>, where <var>C</var> is NAT
+        external ip and <var>D</var> is NAT external mac.
       </li>
 
       <li>
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 7ae5e45da..2279c33ac 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -9181,16 +9181,25 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
             /* Ingress Gateway Redirect Table: For NAT on a distributed
              * router, add flows that are specific to a NAT rule.  These
              * flows indicate the presence of an applicable NAT rule that
-             * can be applied in a distributed manner. */
+             * can be applied in a distributed manner.
+             * In particulr reg1 and eth.src are set to NAT external IP and
+             * NAT external mac so the ARP request generated in the following
+             * stage is sent out with proper IP/MAC src addresses
+             */
             if (distributed) {
                 ds_clear(&match);
-                ds_put_format(&match, "ip%s.src == %s && outport == %s",
-                              is_v6 ? "6" : "4",
-                              nat->logical_ip,
-                              od->l3dgw_port->json_key);
+                ds_clear(&actions);
+                ds_put_format(&match,
+                              "ip%s.src == %s && outport == %s && "
+                              "is_chassis_resident(\"%s\")",
+                              is_v6 ? "6" : "4", nat->logical_ip,
+                              od->l3dgw_port->json_key, nat->logical_port);
+                ds_put_format(&actions, "eth.src = %s; %sreg1 = %s; next;",
+                              nat->external_mac, is_v6 ? "xx" : "",
+                              nat->external_ip);
                 ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT,
-                                        100, ds_cstr(&match), "next;",
-                                        &nat->header_);
+                                        100, ds_cstr(&match),
+                                        ds_cstr(&actions), &nat->header_);
             }
 
             /* Egress Loopback table: For NAT on a distributed router.
diff --git a/tests/ovn.at b/tests/ovn.at
index 8fa1a7e1b..15b40ca1e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -14943,9 +14943,14 @@  ovs-vsctl -- add-port br-int hv2-vif1 -- \
     set interface hv2-vif1 external-ids:iface-id=sw1-p0 \
     options:tx_pcap=hv2/vif1-tx.pcap \
     options:rxq_pcap=hv2/vif1-rx.pcap \
-    ofport-request=1
+    ofport-request=2
+ovs-vsctl -- add-port br-int hv2-vif2 -- \
+    set interface hv2-vif2 external-ids:iface-id=sw0-p1 \
+    options:tx_pcap=hv2/vif2-tx.pcap \
+    options:rxq_pcap=hv2/vif2-rx.pcap \
+    ofport-request=3
 
-ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
+ovn-nbctl create Logical_Router name=lr0
 ovn-nbctl ls-add sw0
 ovn-nbctl ls-add sw1
 
@@ -14954,13 +14959,16 @@  ovn-nbctl lsp-add sw0 rp-sw0 -- set Logical_Switch_Port rp-sw0 \
     type=router options:router-port=sw0 \
     -- lsp-set-addresses rp-sw0 router
 
-ovn-nbctl lrp-add lr0 sw1 00:00:02:01:02:03 172.16.1.1/24 2002:0:0:0:0:0:0:1/64
+ovn-nbctl lrp-add lr0 sw1 00:00:02:01:02:03 172.16.1.1/24 2002:0:0:0:0:0:0:1/64 \
+    -- set Logical_Router_Port sw1 options:redirect-chassis="hv2"
 ovn-nbctl lsp-add sw1 rp-sw1 -- set Logical_Switch_Port rp-sw1 \
     type=router options:router-port=sw1 \
     -- lsp-set-addresses rp-sw1 router
 
 ovn-nbctl lsp-add sw0 sw0-p0 \
     -- lsp-set-addresses sw0-p0 "f0:00:00:01:02:03 192.168.1.2 2001::2"
+ovn-nbctl lsp-add sw0 sw0-p1 \
+    -- lsp-set-addresses sw0-p1 "f0:00:00:11:02:03 192.168.1.3 2001::3"
 
 ovn-nbctl lsp-add sw1 sw1-p0 \
     -- lsp-set-addresses sw1-p0 unknown
@@ -15006,6 +15014,20 @@  send_na 2 1 $dst_mac $router_mac1 $dst_ip6 $router_ip6
 
 OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
 
+# Create FIP on sw0-p0, add a route on logical router pipeline and
+# ARP request for a unkwon destination is sent using FIP MAC/IP
+ovn-nbctl lr-nat-add lr0 dnat_and_snat 172.16.1.2 192.168.1.3 sw0-p1 f0:00:00:01:02:04
+ovn-nbctl lr-route-add lr0 172.16.2.0/24 172.16.1.11
+
+dst_ip=$(ip_to_hex 172 16 2 10)
+fip_ip=$(ip_to_hex 172 16 1 2)
+src_ip=$(ip_to_hex 192 168 1 3)
+gw_router=$(ip_to_hex 172 16 1 11)
+send_icmp_packet 2 2 f00000110203 $router_mac0 $src_ip $dst_ip 0000 $data
+echo $(get_arp_req f00000010204 $fip_ip $gw_router) >> expected
+
+OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
+
 OVN_CLEANUP([hv1],[hv2])
 AT_CLEANUP
 
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 9ae6c6b1f..9dfe6a4ad 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -2747,6 +2747,19 @@  ADD_VETH(alice1, alice1, br-int, "172.16.1.2/24", "f0:00:00:01:02:05", \
 ovn-nbctl lsp-add alice alice1 \
 -- lsp-set-addresses alice1 "f0:00:00:01:02:05 172.16.1.2"
 
+# Add external network
+ADD_NAMESPACES(ext-net)
+AT_CHECK([ip link add alice-ext netns alice1 type veth peer name ext-veth netns ext-net])
+NS_CHECK_EXEC([ext-net], [ip link set dev ext-veth up], [0], [])
+NS_CHECK_EXEC([ext-net], [ip addr add 10.0.0.1/24 dev ext-veth], [0], [])
+NS_CHECK_EXEC([ext-net], [ip route add default via 10.0.0.2], [0], [])
+
+NS_CHECK_EXEC([alice1], [ip link set dev alice-ext up], [0], [])
+NS_CHECK_EXEC([alice1], [ip addr add 10.0.0.2/24 dev alice-ext], [0], [])
+NS_CHECK_EXEC([alice1], [sysctl -w net.ipv4.conf.all.forwarding=1],[0], [dnl
+net.ipv4.conf.all.forwarding = 1
+])
+
 # Add DNAT rules
 AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.3 192.168.1.2 foo1 00:00:02:02:03:04])
 AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4 192.168.1.3 foo2 00:00:02:02:03:05])
@@ -2754,6 +2767,9 @@  AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4 192.168.1.3 foo2 00:0
 # Add a SNAT rule
 AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.0.0/16])
 
+# Add default route to ext-net
+AT_CHECK([ovn-nbctl lr-route-add R1 10.0.0.0/24 172.16.1.2])
+
 ovn-nbctl --wait=hv sync
 OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 'nat(src=172.16.1.1)'])
 
@@ -2797,6 +2813,20 @@  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
 icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
 ])
 
+# Try to ping external network
+NS_CHECK_EXEC([ext-net], [tcpdump -n -c 3 -i ext-veth dst 172.16.1.3 and icmp > ext-net.pcap &])
+sleep 1
+AT_CHECK([ovn-nbctl lr-nat-del R1 snat])
+NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.1 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+OVS_WAIT_UNTIL([
+    total_pkts=$(cat ext-net.pcap | wc -l)
+    test "${total_pkts}" = "3"
+])
+
 OVS_APP_EXIT_AND_WAIT([ovn-controller])
 
 as ovn-sb