diff mbox series

[ovs-dev] northd: avoid snat on reply packets

Message ID 20220401090448.1137665-1-xsimonar@redhat.com
State Superseded
Headers show
Series [ovs-dev] northd: avoid snat on reply packets | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Xavier Simonart April 1, 2022, 9:04 a.m. UTC 
On gateway routers egress packets might be both:
- unDNATted
- SNATted
Reply packets should not be SNATted (they must of course be UnDNATted
if DNAT was applied).

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2061593

Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
---
 northd/northd.c     |   1 +
 tests/ovn-northd.at |  33 +++++++------
 tests/system-ovn.at | 118 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+), 16 deletions(-)
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index 2fb0a93c2..511bd3331 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12919,6 +12919,7 @@  build_lrouter_out_snat_flow(struct hmap *lflows, struct ovn_datapath *od,
             ds_put_format(actions, "ip%s.src=%s; next;",
                           is_v6 ? "6" : "4", nat->external_ip);
         } else {
+            ds_put_format(match, " && (!ct.trk || !ct.rpl)");
             ds_put_format(actions, "ct_snat(%s", nat->external_ip);
 
             if (nat->external_port_range[0]) {
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index daa664982..f462fca86 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -1030,7 +1030,7 @@  AT_CHECK([grep -e "lr_out_snat" drflows | sed 's/table=../table=??/' | sort], [0
 AT_CHECK([grep -e "lr_out_snat" crflows | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(lr_out_snat        ), priority=0    , match=(1), action=(next;)
   table=??(lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
-  table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src == 50.0.0.11 && ip4.dst == $allowed_range), action=(ct_snat(172.16.1.1);)
+  table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src == 50.0.0.11 && ip4.dst == $allowed_range && (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.1);)
 ])
 
 
@@ -1062,7 +1062,7 @@  AT_CHECK([grep -e "lr_out_snat" drflows2 | sed 's/table=../table=??/' | sort], [
 AT_CHECK([grep -e "lr_out_snat" crflows2 | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(lr_out_snat        ), priority=0    , match=(1), action=(next;)
   table=??(lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
-  table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src == 50.0.0.11), action=(ct_snat(172.16.1.1);)
+  table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src == 50.0.0.11 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.1);)
   table=??(lr_out_snat        ), priority=35   , match=(ip && ip4.src == 50.0.0.11 && ip4.dst == $disallowed_range), action=(next;)
 ])
 
@@ -1091,7 +1091,7 @@  AT_CHECK([grep -e "lr_out_snat" drflows3 | sed 's/table=../table=??/' | sort], [
 AT_CHECK([grep -e "lr_out_snat" crflows3 | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(lr_out_snat        ), priority=0    , match=(1), action=(next;)
   table=??(lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
-  table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src == 50.0.0.11 && ip4.dst == $allowed_range), action=(ct_snat(172.16.1.2);)
+  table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src == 50.0.0.11 && ip4.dst == $allowed_range && (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.2);)
 ])
 
 # Stateful FIP with DISALLOWED_IPs
@@ -1120,7 +1120,7 @@  AT_CHECK([grep -e "lr_out_snat" drflows4 | sed 's/table=../table=??/' | sort], [
 AT_CHECK([grep -e "lr_out_snat" crflows4 | sed 's/table=../table=??/' | sort], [0], [dnl
   table=??(lr_out_snat        ), priority=0    , match=(1), action=(next;)
   table=??(lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
-  table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src == 50.0.0.11), action=(ct_snat(172.16.1.2);)
+  table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src == 50.0.0.11 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.2);)
   table=??(lr_out_snat        ), priority=35   , match=(ip && ip4.src == 50.0.0.11 && ip4.dst == $disallowed_range), action=(next;)
 ])
 
@@ -5140,11 +5140,12 @@  AT_CHECK([grep "lr_out_post_undnat" lr0flows | sed 's/table=./table=?/' | sort],
 AT_CHECK([grep "lr_out_snat" lr0flows | sed 's/table=./table=?/' | sort], [0], [dnl
   table=? (lr_out_snat        ), priority=0    , match=(1), action=(next;)
   table=? (lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
-  table=? (lr_out_snat        ), priority=25   , match=(ip && ip4.src == 10.0.0.0/24), action=(ct_snat(172.168.0.10);)
-  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.10), action=(ct_snat(172.168.0.30);)
-  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.3), action=(ct_snat(172.168.0.20);)
+  table=? (lr_out_snat        ), priority=25   , match=(ip && ip4.src == 10.0.0.0/24 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.10);)
+  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.10 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.30);)
+  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.3 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.20);)
 ])
 
+
 # Set lb force snat logical router.
 check ovn-nbctl --wait=sb set logical_router lr0 options:lb_force_snat_ip="router_ip"
 check ovn-nbctl --wait=sb sync
@@ -5201,9 +5202,9 @@  AT_CHECK([grep "lr_out_snat" lr0flows | sed 's/table=./table=?/' | sort], [0], [
   table=? (lr_out_snat        ), priority=110  , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), action=(ct_snat(172.168.0.10);)
   table=? (lr_out_snat        ), priority=110  , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);)
   table=? (lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
-  table=? (lr_out_snat        ), priority=25   , match=(ip && ip4.src == 10.0.0.0/24), action=(ct_snat(172.168.0.10);)
-  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.10), action=(ct_snat(172.168.0.30);)
-  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.3), action=(ct_snat(172.168.0.20);)
+  table=? (lr_out_snat        ), priority=25   , match=(ip && ip4.src == 10.0.0.0/24 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.10);)
+  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.10 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.30);)
+  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.3 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.20);)
 ])
 
 # Add a LB VIP same as router ip.
@@ -5266,9 +5267,9 @@  AT_CHECK([grep "lr_out_snat" lr0flows | sed 's/table=./table=?/' | sort], [0], [
   table=? (lr_out_snat        ), priority=110  , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-public"), action=(ct_snat(172.168.0.10);)
   table=? (lr_out_snat        ), priority=110  , match=(flags.force_snat_for_lb == 1 && ip4 && outport == "lr0-sw0"), action=(ct_snat(10.0.0.1);)
   table=? (lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
-  table=? (lr_out_snat        ), priority=25   , match=(ip && ip4.src == 10.0.0.0/24), action=(ct_snat(172.168.0.10);)
-  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.10), action=(ct_snat(172.168.0.30);)
-  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.3), action=(ct_snat(172.168.0.20);)
+  table=? (lr_out_snat        ), priority=25   , match=(ip && ip4.src == 10.0.0.0/24 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.10);)
+  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.10 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.30);)
+  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.3 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.20);)
 ])
 
 # Add IPv6 router port and LB.
@@ -5346,9 +5347,9 @@  AT_CHECK([grep "lr_out_snat" lr0flows | sed 's/table=./table=?/' | sort], [0], [
   table=? (lr_out_snat        ), priority=110  , match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-public"), action=(ct_snat(def0::10);)
   table=? (lr_out_snat        ), priority=110  , match=(flags.force_snat_for_lb == 1 && ip6 && outport == "lr0-sw0"), action=(ct_snat(aef0::1);)
   table=? (lr_out_snat        ), priority=120  , match=(nd_ns), action=(next;)
-  table=? (lr_out_snat        ), priority=25   , match=(ip && ip4.src == 10.0.0.0/24), action=(ct_snat(172.168.0.10);)
-  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.10), action=(ct_snat(172.168.0.30);)
-  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.3), action=(ct_snat(172.168.0.20);)
+  table=? (lr_out_snat        ), priority=25   , match=(ip && ip4.src == 10.0.0.0/24 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.10);)
+  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.10 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.30);)
+  table=? (lr_out_snat        ), priority=33   , match=(ip && ip4.src == 10.0.0.3 && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.20);)
 ])
 
 check ovn-nbctl lrp-del lr0-sw0
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 5d90be1c9..787c23c26 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -7992,3 +7992,121 @@  OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
 
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([East-West traffic with gateway router if DNAT configured])
+AT_KEYWORDS([ovnnat])
+
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+        -- set Open_vSwitch . external-ids:system-id=hv1 \
+        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+# Logical network:
+# One LR - R1  has two switches: sw0 and sw1
+#    sw0 -- R1 -- sw1
+# Logical port 'sw01' in switch 'sw0'.
+# Logical port 'sw11' in switch 'sw1'.
+# nc server running in sw01
+# nc client running on sw11
+
+check ovn-nbctl lr-add R1
+check ovn-nbctl ls-add sw0
+check ovn-nbctl ls-add sw1
+
+check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
+check ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24
+check ovn-nbctl set logical_router R1 options:chassis=hv1
+
+check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
+    type=router options:router-port=rp-sw0 \
+    -- lsp-set-addresses sw0-rp router
+check ovn-nbctl lsp-add sw1 sw1-rp -- set Logical_Switch_Port sw1-rp \
+    type=router options:router-port=rp-sw1 \
+    -- lsp-set-addresses sw1-rp router
+
+ADD_NAMESPACES(sw01)
+ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
+       "192.168.1.1")
+check ovn-nbctl lsp-add sw0 sw01 \
+    -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2"
+
+ADD_NAMESPACES(sw11)
+ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \
+       "192.168.2.1")
+check ovn-nbctl lsp-add sw1 sw11 \
+    -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2"
+
+NETNS_DAEMONIZE([sw01], [nc -k -l 8000], [nc-sw01.pid])
+
+test_ping() {
+    NS_CHECK_EXEC([$1],  [ping -q -c 1 $2 -w 2 | FORMAT_PING], \
+[0], [dnl
+1 packets transmitted, 1 received, 0% packet loss, time 0ms
+])
+}
+
+# Only SNAT
+check ovn-nbctl --wait=hv lr-nat-add R1 snat 172.16.1.21 192.168.2.0/24
+
+NS_CHECK_EXEC([sw11], [nc 192.168.1.2 8000 <<< h])
+test_ping sw11 192.168.1.2
+
+# Ensure nat has been hit
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep -v "n_packets=0" | grep 'nat(src=172.16.1.21)'])
+# Ensure conntrack entry is present
+OVS_WAIT_FOR_OUTPUT([
+    ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.2.2) | \
+      sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+icmp,orig=(src=192.168.2.2,dst=192.168.1.2,id=<cleared>,type=8,code=0),reply=(src=192.168.1.2,dst=192.168.2.2,id=<cleared>,type=0,code=0),zone=<cleared>
+tcp,orig=(src=192.168.2.2,dst=192.168.1.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=192.168.2.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+])
+
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
+# SNAT and DNAT. using Logical IP
+ovn-nbctl --wait=hv lr-nat-add R1 dnat_and_snat 172.16.1.2 192.168.1.2
+NS_CHECK_EXEC([sw11], [nc 192.168.1.2 8000 <<< h ])
+test_ping sw11 192.168.1.2
+
+# Ensure conntrack entry is present
+OVS_WAIT_FOR_OUTPUT([
+    ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.2.2) | \
+      sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+icmp,orig=(src=192.168.2.2,dst=192.168.1.2,id=<cleared>,type=8,code=0),reply=(src=192.168.1.2,dst=192.168.2.2,id=<cleared>,type=0,code=0),zone=<cleared>
+tcp,orig=(src=192.168.2.2,dst=192.168.1.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=192.168.2.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+])
+
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
+# SNAT and DNAT. using floating IP
+NS_CHECK_EXEC([sw11], [nc 172.16.1.2 8000 <<< h ])
+test_ping sw11 172.16.1.2
+
+# Ensure conntrack entry is present
+OVS_WAIT_FOR_OUTPUT([
+    ovs-appctl dpctl/dump-conntrack | FORMAT_CT(192.168.2.2) | \
+      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=192.168.1.2,dst=192.168.2.2,id=<cleared>,type=0,code=0),zone=<cleared>
+tcp,orig=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=192.168.2.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+])
+
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
+OVS_APP_EXIT_AND_WAIT([ovn-controller])
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+
+AT_CLEANUP
+])