[ovs-dev,v2,2/2] conntrack: fix expectations nat configuration for ftp+DNAT

When configuring the nat part of an expectation, care must be taken to
look at the master nat action and direction to properly reproduce it.

The FTP passive mode test is switched to DNAT since the alg only mangles
the packet in this case.
Other active mode tests titles have been updated to reflect they are
dealing with SNAT.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/conntrack.c         | 12 ++++++++++--
 tests/system-traffic.at | 48 ++++++++++++++++++++++++------------------------
 2 files changed, 34 insertions(+), 26 deletions(-)

Thanks for the fix David.

On Thu, Dec 20, 2018 at 5:33 AM David Marchand <david.marchand@redhat.com>
wrote:

> When configuring the nat part of an expectation, care must be taken to
> look at the master nat action and direction to properly reproduce it.
>
> The FTP passive mode test is switched to DNAT since the alg only mangles
> the packet in this case.
> Other active mode tests titles have been updated to reflect they are
> dealing with SNAT.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/conntrack.c         | 12 ++++++++++--
>  tests/system-traffic.at | 48
> ++++++++++++++++++++++++------------------------
>  2 files changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 2e4141a..6106d9a 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2713,21 +2713,29 @@ expectation_create(struct conntrack *ct, ovs_be16
> dst_port,
>      if (reply) {
>          src_addr = master_conn->key.src.addr;
>          dst_addr = master_conn->key.dst.addr;
> +        alg_exp_node->nat_rpl_dst = true;
>          if (skip_nat) {
>              alg_nat_repl_addr = dst_addr;
> +        } else if (master_conn->nat_info &&
> +                   master_conn->nat_info->nat_action & NAT_ACTION_DST) {
> +            alg_nat_repl_addr = master_conn->rev_key.src.addr;
> +            alg_exp_node->nat_rpl_dst = false;
>          } else {
>              alg_nat_repl_addr = master_conn->rev_key.dst.addr;
>          }
> -        alg_exp_node->nat_rpl_dst = true;
>      } else {
>          src_addr = master_conn->rev_key.src.addr;
>          dst_addr = master_conn->rev_key.dst.addr;
> +        alg_exp_node->nat_rpl_dst = false;
>          if (skip_nat) {
>              alg_nat_repl_addr = src_addr;
> +        } else if (master_conn->nat_info &&
> +                   master_conn->nat_info->nat_action & NAT_ACTION_DST) {
> +            alg_nat_repl_addr = master_conn->key.dst.addr;
> +            alg_exp_node->nat_rpl_dst = true;
>          } else {
>              alg_nat_repl_addr = master_conn->key.src.addr;
>          }
> -        alg_exp_node->nat_rpl_dst = false;
>      }
>      if (src_ip_wc) {
>          memset(&src_addr, 0, sizeof src_addr);
>


conntrack.c changes are fine.



> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 3734497..9af0483 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -4218,7 +4218,7 @@ dnl
>  dnl Checks the implementation of conntrack with FTP ALGs in combination
> with
>  dnl NAT, using the provided flow table.
>  m4_define([CHECK_FTP_NAT],
> -   [AT_SETUP([conntrack - FTP NAT $1])
> +   [AT_SETUP([conntrack - FTP $1])
>      AT_SKIP_IF([test $HAVE_FTP = no])
>      CHECK_CONNTRACK()
>      CHECK_CONNTRACK_NAT()
> @@ -4257,7 +4257,7 @@ ls
>      OVS_TRAFFIC_VSWITCHD_STOP
>      AT_CLEANUP])
>
> -dnl CHECK_FTP_NAT_PRE_RECIRC(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
> +dnl CHECK_FTP_SNAT_PRE_RECIRC(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
>  dnl
>  dnl Checks the implementation of conntrack with FTP ALGs in combination
> with
>  dnl NAT, with flow tables that implement the NATing as part of handling of
> @@ -4265,8 +4265,8 @@ dnl initial incoming packets - ie, the first flow is
> ct(nat,table=foo).
>  dnl
>  dnl IP_ADDR must specify the NAT address in standard "10.1.1.x" format,
>  dnl and IP_ADDR_AS_HEX must specify the same address as hex, eg
> 0x0a0101xx.
> -m4_define([CHECK_FTP_NAT_PRE_RECIRC], [dnl
> -   CHECK_FTP_NAT([prerecirc $1], [$2], [dnl
> +m4_define([CHECK_FTP_SNAT_PRE_RECIRC], [dnl
> +    CHECK_FTP_NAT([SNAT prerecirc $1], [$2], [dnl
>  dnl track all IP traffic, de-mangle non-NEW connections
>  table=0 in_port=1, ip, action=ct(table=1,nat)
>  table=0 in_port=2, ip, action=ct(table=2,nat)
> @@ -4320,7 +4320,7 @@
> tcp,orig=(src=10.1.1.2,dst=$2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1
>  ])
>
>  dnl Check that ct(nat,table=foo) works without TCP sequence adjustment.
> -CHECK_FTP_NAT_PRE_RECIRC([], [10.1.1.9], [0x0a010109])
> +CHECK_FTP_SNAT_PRE_RECIRC([], [10.1.1.9], [0x0a010109])
>
>  dnl Check that ct(nat,table=foo) works with TCP sequence adjustment.
>  dnl
> @@ -4331,9 +4331,9 @@ dnl of 10.1.1.1 used in the test and 10.1.1.240
> here), the FTP NAT ALG must
>  dnl resize the packet and adjust TCP sequence numbers. This test is kept
>  dnl separate from the above to easier identify issues in this code on
> different
>  dnl kernels.
> -CHECK_FTP_NAT_PRE_RECIRC([seqadj], [10.1.1.240], [0x0a0101f0])
> +CHECK_FTP_SNAT_PRE_RECIRC([seqadj], [10.1.1.240], [0x0a0101f0])
>
> -dnl CHECK_FTP_NAT_POST_RECIRC(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
> +dnl CHECK_FTP_SNAT_POST_RECIRC(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
>  dnl
>  dnl Checks the implementation of conntrack with FTP ALGs in combination
> with
>  dnl NAT, with flow tables that implement the NATing after the first round
> @@ -4342,8 +4342,8 @@ dnl flow will implement the NATing with
> ct(nat..),output:foo.
>  dnl
>  dnl IP_ADDR must specify the NAT address in standard "10.1.1.x" format,
>  dnl and IP_ADDR_AS_HEX must specify the same address as hex, eg
> 0x0a0101xx.
> -m4_define([CHECK_FTP_NAT_POST_RECIRC], [dnl
> -    CHECK_FTP_NAT([postrecirc $1], [$2], [dnl
> +m4_define([CHECK_FTP_SNAT_POST_RECIRC], [dnl
> +    CHECK_FTP_NAT([SNAT postrecirc $1], [$2], [dnl
>  dnl track all IP traffic (this includes a helper call to non-NEW packets.)
>  table=0 ip, action=ct(table=1)
>  dnl
> @@ -4386,7 +4386,7 @@
> tcp,orig=(src=10.1.1.2,dst=$2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1
>  ])
>
>  dnl Check that ct(nat,table=foo) works without TCP sequence adjustment.
> -CHECK_FTP_NAT_POST_RECIRC([], [10.1.1.9], [0x0a010109])
> +CHECK_FTP_SNAT_POST_RECIRC([], [10.1.1.9], [0x0a010109])
>
>  dnl Check that ct(nat,table=foo) works with TCP sequence adjustment.
>  dnl
> @@ -4397,10 +4397,10 @@ dnl of 10.1.1.1 used in the test and 10.1.1.240
> here), the FTP NAT ALG must
>  dnl resize the packet and adjust TCP sequence numbers. This test is kept
>  dnl separate from the above to easier identify issues in this code on
> different
>  dnl kernels.
> -CHECK_FTP_NAT_POST_RECIRC([seqadj], [10.1.1.240], [0x0a0101f0])
> +CHECK_FTP_SNAT_POST_RECIRC([seqadj], [10.1.1.240], [0x0a0101f0])
>
>
> -dnl CHECK_FTP_NAT_ORIG_TUPLE(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
> +dnl CHECK_FTP_SNAT_ORIG_TUPLE(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
>  dnl
>  dnl Checks the implementation of conntrack original direction tuple
> matching
>  dnl with FTP ALGs in combination with NAT, with flow tables that implement
> @@ -4410,8 +4410,8 @@ dnl commiting of NATed and other connections with
> ct(nat..),output:foo.
>  dnl
>  dnl IP_ADDR must specify the NAT address in standard "10.1.1.x" format,
>  dnl and IP_ADDR_AS_HEX must specify the same address as hex, eg
> 0x0a0101xx.
> -m4_define([CHECK_FTP_NAT_ORIG_TUPLE], [dnl
> -    CHECK_FTP_NAT([orig tuple $1], [$2], [dnl
> +m4_define([CHECK_FTP_SNAT_ORIG_TUPLE], [dnl
> +    CHECK_FTP_NAT([SNAT orig tuple $1], [$2], [dnl
>  dnl Store zone in reg4 and packet direction in reg3 (IN=1, OUT=2).
>  dnl NAT is only applied to OUT-direction packets, so that ACL
>  dnl processing can be done with non-NATted headers.
> @@ -4517,13 +4517,13 @@
> tcp,orig=(src=10.1.1.2,dst=$2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1
>
>  dnl Check that ct(nat,table=foo) works without TCP sequence adjustment
> with
>  dnl an ACL table based on matching on conntrack original direction tuple
> only.
> -CHECK_FTP_NAT_ORIG_TUPLE([], [10.1.1.9], [0x0a010109])
> +CHECK_FTP_SNAT_ORIG_TUPLE([], [10.1.1.9], [0x0a010109])
>
>  dnl Check that ct(nat,table=foo) works with TCP sequence adjustment with
>  dnl an ACL table based on matching on conntrack original direction tuple
> only.
> -CHECK_FTP_NAT_ORIG_TUPLE([seqadj], [10.1.1.240], [0x0a0101f0])
> +CHECK_FTP_SNAT_ORIG_TUPLE([seqadj], [10.1.1.240], [0x0a0101f0])
>
> -AT_SETUP([conntrack - IPv4 FTP Passive with NAT])
> +AT_SETUP([conntrack - IPv4 FTP Passive with DNAT])
>  AT_SKIP_IF([test $HAVE_FTP = no])
>  CHECK_CONNTRACK()
>  CHECK_CONNTRACK_NAT()
> @@ -4535,12 +4535,12 @@ ADD_NAMESPACES(at_ns0, at_ns1)
>
>  ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>  NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11])
> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.240 e6:66:c1:22:22:22])
>  NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e6:66:c1:22:22:22])
>
> -ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.240/24")
>  NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address e6:66:c1:22:22:22])
>  NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e6:66:c1:11:11:11])
> -NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.240 e6:66:c1:11:11:11])
>
>  dnl Allow any traffic from ns0->ns1.
>  AT_DATA([flows.txt], [dnl
> @@ -4551,11 +4551,11 @@ dnl
>  dnl Table 1
>  dnl
>  dnl Allow new FTP control connections.
> -table=1 in_port=1 ct_state=+new tcp nw_src=10.1.1.1 tp_dst=21
> action=ct(alg=ftp,commit,nat(src=10.1.1.240)),2
> +table=1 in_port=1 ct_state=+new tcp nw_src=10.1.1.1 tp_dst=21
> action=ct(alg=ftp,commit,nat(dst=10.1.1.240)),2
>  dnl Allow related TCP connections from port 1.
>  table=1 in_port=1 ct_state=+new+rel tcp nw_src=10.1.1.1
> action=ct(commit,nat),2
>  dnl Allow established TCP connections both ways, post-NAT match.
> -table=1 in_port=1 ct_state=+est tcp nw_src=10.1.1.240 action=2
> +table=1 in_port=1 ct_state=+est tcp nw_dst=10.1.1.240 action=2
>  table=1 in_port=2 ct_state=+est tcp nw_dst=10.1.1.1 action=1
>
>  dnl Allow ICMP both ways.
> @@ -4567,7 +4567,7 @@ table=1 priority=0, action=drop
>  AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
>
>  dnl Check that the stacks working to avoid races.
> -OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.2 >/dev/null])
> +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.240 >/dev/null])
>
>  OVS_START_L7([at_ns1], [ftp])
>
> @@ -4576,8 +4576,8 @@ NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 -t 3
> -T 1 --retry-connrefused -v -o
>
>  dnl Discards CLOSE_WAIT and CLOSING
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0],
> [dnl
>
> -tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.240,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
>
> -tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.240,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp
>
> +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.240,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
>
> +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.240,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp
>  ])
>
>  OVS_TRAFFIC_VSWITCHD_STOP
> --
> 1.8.3.1
>

1/ For the tests, I think we should have both SNAT and DNAT for passive
mode, since some special cases exist and we also
    want to test for ALGs generally. Hence I added DNAT passive mode as a
separate test, rather than converting the
    existing SNAT test to DNAT and I changed the name of the existing test
to "SNAT" from just "NAT".

2/ I also think we need a DNAT active mode test, so I added one as well.

3/ There were a few missing "SNAT" and "DNAT"  vs "NAT" changes for the
FTP/TFTP tests aside from the ones you added, so I added those as well.

So, we end up with the following

Let me know if this makes sense.

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 4c52431..33bb4fc 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -4218,7 +4218,7 @@ dnl
 dnl Checks the implementation of conntrack with FTP ALGs in combination
with
 dnl NAT, using the provided flow table.
 m4_define([CHECK_FTP_NAT],
-   [AT_SETUP([conntrack - FTP NAT $1])
+   [AT_SETUP([conntrack - FTP $1])
     AT_SKIP_IF([test $HAVE_FTP = no])
     CHECK_CONNTRACK()
     CHECK_CONNTRACK_NAT()
@@ -4256,8 +4256,8 @@ dnl initial incoming packets - ie, the first flow is
ct(nat,table=foo).
 dnl
 dnl IP_ADDR must specify the NAT address in standard "10.1.1.x" format,
 dnl and IP_ADDR_AS_HEX must specify the same address as hex, eg 0x0a0101xx.
-m4_define([CHECK_FTP_NAT_PRE_RECIRC], [dnl
-   CHECK_FTP_NAT([prerecirc $1], [$2], [dnl
+m4_define([CHECK_FTP_SNAT_PRE_RECIRC], [dnl
+   CHECK_FTP_NAT([SNAT prerecirc $1], [$2], [dnl
 dnl track all IP traffic, de-mangle non-NEW connections
 table=0 in_port=1, ip, action=ct(table=1,nat)
 table=0 in_port=2, ip, action=ct(table=2,nat)
@@ -4311,7 +4311,7 @@
tcp,orig=(src=10.1.1.2,dst=$2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1
 ])

 dnl Check that ct(nat,table=foo) works without TCP sequence adjustment.
-CHECK_FTP_NAT_PRE_RECIRC([], [10.1.1.9], [0x0a010109])
+CHECK_FTP_SNAT_PRE_RECIRC([], [10.1.1.9], [0x0a010109])

 dnl Check that ct(nat,table=foo) works with TCP sequence adjustment.
 dnl
@@ -4322,9 +4322,9 @@ dnl of 10.1.1.1 used in the test and 10.1.1.240
here), the FTP NAT ALG must
 dnl resize the packet and adjust TCP sequence numbers. This test is kept
 dnl separate from the above to easier identify issues in this code on
different
 dnl kernels.
-CHECK_FTP_NAT_PRE_RECIRC([seqadj], [10.1.1.240], [0x0a0101f0])
+CHECK_FTP_SNAT_PRE_RECIRC([seqadj], [10.1.1.240], [0x0a0101f0])

-dnl CHECK_FTP_NAT_POST_RECIRC(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
+dnl CHECK_FTP_SNAT_POST_RECIRC(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
 dnl
 dnl Checks the implementation of conntrack with FTP ALGs in combination
with
 dnl NAT, with flow tables that implement the NATing after the first round
@@ -4333,8 +4333,8 @@ dnl flow will implement the NATing with
ct(nat..),output:foo.
 dnl
 dnl IP_ADDR must specify the NAT address in standard "10.1.1.x" format,
 dnl and IP_ADDR_AS_HEX must specify the same address as hex, eg 0x0a0101xx.
-m4_define([CHECK_FTP_NAT_POST_RECIRC], [dnl
-    CHECK_FTP_NAT([postrecirc $1], [$2], [dnl
+m4_define([CHECK_FTP_SNAT_POST_RECIRC], [dnl
+    CHECK_FTP_NAT([SNAT postrecirc $1], [$2], [dnl
 dnl track all IP traffic (this includes a helper call to non-NEW packets.)
 table=0 ip, action=ct(table=1)
 dnl
@@ -4377,7 +4377,7 @@
tcp,orig=(src=10.1.1.2,dst=$2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1
 ])

 dnl Check that ct(nat,table=foo) works without TCP sequence adjustment.
-CHECK_FTP_NAT_POST_RECIRC([], [10.1.1.9], [0x0a010109])
+CHECK_FTP_SNAT_POST_RECIRC([], [10.1.1.9], [0x0a010109])

 dnl Check that ct(nat,table=foo) works with TCP sequence adjustment.
 dnl
@@ -4388,10 +4388,10 @@ dnl of 10.1.1.1 used in the test and 10.1.1.240
here), the FTP NAT ALG must
 dnl resize the packet and adjust TCP sequence numbers. This test is kept
 dnl separate from the above to easier identify issues in this code on
different
 dnl kernels.
-CHECK_FTP_NAT_POST_RECIRC([seqadj], [10.1.1.240], [0x0a0101f0])
+CHECK_FTP_SNAT_POST_RECIRC([seqadj], [10.1.1.240], [0x0a0101f0])


-dnl CHECK_FTP_NAT_ORIG_TUPLE(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
+dnl CHECK_FTP_SNAT_ORIG_TUPLE(TITLE, IP_ADDR, IP_ADDR_AS_HEX)
 dnl
 dnl Checks the implementation of conntrack original direction tuple
matching
 dnl with FTP ALGs in combination with NAT, with flow tables that implement
@@ -4401,8 +4401,8 @@ dnl commiting of NATed and other connections with
ct(nat..),output:foo.
 dnl
 dnl IP_ADDR must specify the NAT address in standard "10.1.1.x" format,
 dnl and IP_ADDR_AS_HEX must specify the same address as hex, eg 0x0a0101xx.
-m4_define([CHECK_FTP_NAT_ORIG_TUPLE], [dnl
-    CHECK_FTP_NAT([orig tuple $1], [$2], [dnl
+m4_define([CHECK_FTP_SNAT_ORIG_TUPLE], [dnl
+    CHECK_FTP_NAT([SNAT orig tuple $1], [$2], [dnl
 dnl Store zone in reg4 and packet direction in reg3 (IN=1, OUT=2).
 dnl NAT is only applied to OUT-direction packets, so that ACL
 dnl processing can be done with non-NATted headers.
@@ -4508,13 +4508,13 @@
tcp,orig=(src=10.1.1.2,dst=$2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1

 dnl Check that ct(nat,table=foo) works without TCP sequence adjustment with
 dnl an ACL table based on matching on conntrack original direction tuple
only.
-CHECK_FTP_NAT_ORIG_TUPLE([], [10.1.1.9], [0x0a010109])
+CHECK_FTP_SNAT_ORIG_TUPLE([], [10.1.1.9], [0x0a010109])

 dnl Check that ct(nat,table=foo) works with TCP sequence adjustment with
 dnl an ACL table based on matching on conntrack original direction tuple
only.
-CHECK_FTP_NAT_ORIG_TUPLE([seqadj], [10.1.1.240], [0x0a0101f0])
+CHECK_FTP_SNAT_ORIG_TUPLE([seqadj], [10.1.1.240], [0x0a0101f0])

-AT_SETUP([conntrack - IPv4 FTP Passive with NAT])
+AT_SETUP([conntrack - IPv4 FTP Passive with SNAT])
 AT_SKIP_IF([test $HAVE_FTP = no])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()
@@ -4574,6 +4574,126 @@
tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

+AT_SETUP([conntrack - IPv4 FTP Passive with DNAT])
+AT_SKIP_IF([test $HAVE_FTP = no])
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+CHECK_CONNTRACK_ALG()
+
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11])
+NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e6:66:c1:22:22:22])
+NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.240 e6:66:c1:22:22:22])
+
+ADD_VETH(p1, at_ns1, br0, "10.1.1.240/24")
+NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address e6:66:c1:22:22:22])
+NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e6:66:c1:11:11:11])
+
+dnl Allow any traffic from ns0->ns1.
+AT_DATA([flows.txt], [dnl
+dnl track all IPv4 traffic and NAT any established traffic.
+table=0 priority=10 ip, action=ct(nat,table=1)
+table=0 priority=0 action=drop
+dnl
+dnl Table 1
+dnl
+dnl Allow new FTP control connections.
+table=1 in_port=1 ct_state=+new tcp nw_src=10.1.1.1 tp_dst=21
action=ct(alg=ftp,commit,nat(dst=10.1.1.240)),2
+dnl Allow related TCP connections from port 1.
+table=1 in_port=1 ct_state=+new+rel tcp nw_src=10.1.1.1
action=ct(commit,nat),2
+dnl Allow established TCP connections both ways, post-NAT match.
+table=1 in_port=1 ct_state=+est tcp nw_dst=10.1.1.240 action=2
+table=1 in_port=2 ct_state=+est tcp nw_dst=10.1.1.1 action=1
+
+dnl Allow ICMP both ways.
+table=1 priority=100 in_port=1 icmp, action=2
+table=1 priority=100 in_port=2 icmp, action=1
+table=1 priority=0, action=drop
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl Check that the stacks working to avoid races.
+OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.240 >/dev/null])
+
+OVS_START_L7([at_ns1], [ftp])
+
+dnl FTP requests from p0->p1 should work fine.
+NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 -t 3 -T 1 --retry-connrefused
-v -o wget0.log])
+
+dnl Discards CLOSE_WAIT and CLOSING
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
+tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.240,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
+tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.240,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([conntrack - IPv4 FTP Active with DNAT])
+AT_SKIP_IF([test $HAVE_FTP = no])
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+CHECK_CONNTRACK_ALG()
+
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11])
+NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e6:66:c1:22:22:22])
+NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.240 e6:66:c1:22:22:22])
+
+ADD_VETH(p1, at_ns1, br0, "10.1.1.240/24")
+NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address e6:66:c1:22:22:22])
+NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e6:66:c1:11:11:11])
+
+dnl Allow any traffic from ns0->ns1.
+AT_DATA([flows.txt], [dnl
+dnl track all IPv4 traffic and NAT any established traffic.
+table=0 priority=10 ip, action=ct(nat,table=1)
+table=0 priority=0 action=drop
+dnl
+dnl Table 1
+dnl
+dnl Allow new FTP control connections.
+table=1 in_port=1 ct_state=+new tcp nw_src=10.1.1.1 tp_dst=21
action=ct(alg=ftp,commit,nat(dst=10.1.1.240)),2
+dnl Allow related TCP connections from port 1.
+table=1 in_port=2 ct_state=+new+rel tcp nw_src=10.1.1.240
action=ct(commit,nat),1
+dnl Allow established TCP connections both ways, post-NAT match.
+table=1 in_port=1 ct_state=+est tcp nw_dst=10.1.1.240 action=2
+table=1 in_port=2 ct_state=+est tcp nw_dst=10.1.1.1 action=1
+
+dnl Allow ICMP both ways.
+table=1 priority=100 in_port=1 icmp, action=2
+table=1 priority=100 in_port=2 icmp, action=1
+table=1 priority=0, action=drop
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl Check that the stacks working to avoid races.
+OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.240 >/dev/null])
+
+OVS_START_L7([at_ns1], [ftp])
+
+dnl FTP requests from p0->p1 should work fine.
+NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 --no-passive-ftp -t 3 -T 1
--retry-connrefused -v -o wget0.log])
+
+dnl Discards CLOSE_WAIT and CLOSING
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
+tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.240,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp
+tcp,orig=(src=10.1.1.240,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - IPv6 HTTP with SNAT])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()
@@ -4715,7 +4835,7 @@
udp,orig=(src=fc00::1,dst=fc00::2,sport=<cleared>,dport=<cleared>),reply=(src=fc
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

-AT_SETUP([conntrack - IPv6 FTP with NAT])
+AT_SETUP([conntrack - IPv6 FTP with SNAT])
 AT_SKIP_IF([test $HAVE_FTP = no])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()
@@ -4775,7 +4895,7 @@
tcp,orig=(src=fc00::2,dst=fc00::240,sport=<cleared>,dport=<cleared>),reply=(src=
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

-AT_SETUP([conntrack - IPv6 FTP Passive with NAT])
+AT_SETUP([conntrack - IPv6 FTP Passive with SNAT])
 AT_SKIP_IF([test $HAVE_FTP = no])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()
@@ -4836,7 +4956,7 @@
tcp,orig=(src=fc00::1,dst=fc00::2,sport=<cleared>,dport=<cleared>),reply=(src=fc
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

-AT_SETUP([conntrack - IPv6 FTP with NAT - orig tuple])
+AT_SETUP([conntrack - IPv6 FTP with SNAT - orig tuple])
 AT_SKIP_IF([test $HAVE_FTP = no])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()
@@ -4896,7 +5016,7 @@
tcp,orig=(src=fc00::2,dst=fc00::240,sport=<cleared>,dport=<cleared>),reply=(src=
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

-AT_SETUP([conntrack - IPv4 TFTP with NAT])
+AT_SETUP([conntrack - IPv4 TFTP with SNAT])
 AT_SKIP_IF([test $HAVE_TFTP = no])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()
(END)

Hello and happy new year to all :-)

On Tue, Jan 8, 2019 at 5:24 AM Darrell Ball <dlu998@gmail.com> wrote:

> Thanks for the fix David.
>
> On Thu, Dec 20, 2018 at 5:33 AM David Marchand <david.marchand@redhat.com>
> wrote:
>
>> When configuring the nat part of an expectation, care must be taken to
>> look at the master nat action and direction to properly reproduce it.
>>
>> The FTP passive mode test is switched to DNAT since the alg only mangles
>> the packet in this case.
>> Other active mode tests titles have been updated to reflect they are
>> dealing with SNAT.
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> ---
>>  lib/conntrack.c         | 12 ++++++++++--
>>  tests/system-traffic.at | 48
>> ++++++++++++++++++++++++------------------------
>>  2 files changed, 34 insertions(+), 26 deletions(-)
>>
>
>
[snip]


>
> 1/ For the tests, I think we should have both SNAT and DNAT for passive
> mode, since some special cases exist and we also
>     want to test for ALGs generally. Hence I added DNAT passive mode as a
> separate test, rather than converting the
>     existing SNAT test to DNAT and I changed the name of the existing test
> to "SNAT" from just "NAT".
>
> 2/ I also think we need a DNAT active mode test, so I added one as well.
>

Adding those tests make sense when searching for regressions and I suppose
this won't take a lot of time when compared to the overall test duration.


> 3/ There were a few missing "SNAT" and "DNAT"  vs "NAT" changes for the
> FTP/TFTP tests aside from the ones you added, so I added those as well.
>

Well, I don't really mind updating all the tests titles at once but I was
trying to keep the changes focused on the parts that were impacted by the
fix.
So since you prefer, ok for aligning all tests titles.


> So, we end up with the following
>
> Let me know if this makes sense.
>

Ok, let me incorporate and test those changes as part of my v3.
Should I add a Co-Authored tag on it for you ?

On Tue, Jan 8, 2019 at 1:33 AM David Marchand <david.marchand@redhat.com>
wrote:

> Hello and happy new year to all :-)
>
> On Tue, Jan 8, 2019 at 5:24 AM Darrell Ball <dlu998@gmail.com> wrote:
>
>> Thanks for the fix David.
>>
>> On Thu, Dec 20, 2018 at 5:33 AM David Marchand <david.marchand@redhat.com>
>> wrote:
>>
>>> When configuring the nat part of an expectation, care must be taken to
>>> look at the master nat action and direction to properly reproduce it.
>>>
>>> The FTP passive mode test is switched to DNAT since the alg only mangles
>>> the packet in this case.
>>> Other active mode tests titles have been updated to reflect they are
>>> dealing with SNAT.
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>>  lib/conntrack.c         | 12 ++++++++++--
>>>  tests/system-traffic.at | 48
>>> ++++++++++++++++++++++++------------------------
>>>  2 files changed, 34 insertions(+), 26 deletions(-)
>>>
>>
>>
> [snip]
>
>
>>
>> 1/ For the tests, I think we should have both SNAT and DNAT for passive
>> mode, since some special cases exist and we also
>>     want to test for ALGs generally. Hence I added DNAT passive mode as a
>> separate test, rather than converting the
>>     existing SNAT test to DNAT and I changed the name of the existing
>> test to "SNAT" from just "NAT".
>>
>> 2/ I also think we need a DNAT active mode test, so I added one as well.
>>
>
> Adding those tests make sense when searching for regressions and I suppose
> this won't take a lot of time when compared to the overall test duration.
>
>
>> 3/ There were a few missing "SNAT" and "DNAT"  vs "NAT" changes for the
>> FTP/TFTP tests aside from the ones you added, so I added those as well.
>>
>
> Well, I don't really mind updating all the tests titles at once but I was
> trying to keep the changes focused on the parts that were impacted by the
> fix.
> So since you prefer, ok for aligning all tests titles.
>

Thanks
Possibly having the test titles specify "SNAT" and "DNAT" would have made
it obvious that FTP DNAT tests were completely missing and
prompted adding some, which would have had the obvious effect.



>
>
>> So, we end up with the following
>>
>> Let me know if this makes sense.
>>
>
> Ok, let me incorporate and test those changes as part of my v3.
> Should I add a Co-Authored tag on it for you ?
>

I prefer to avoid submitting my changes as a separate patch, but either way
is fine with me.


>
> --
> David Marchand
>