[iptables PATCH 0/4] Re-enable NFLOG tests

[iptables PATCH 0/4] Re-enable NFLOG tests

[Jeremy Sowden]

Some of the NFLOG tests were disabled when iptables-nft was changed to
use nft's nflog implementation, because nft doesn't support
`--nflog-range`.  This patch-set builds on Phil's recent work to support
different test results for -legacy and -nft in order to re-enable those
tests.

* Patch 1 renames a variable.
* Patches 2 & 3 add support for a new test result where the iptables
  command succeeds, but dumping the rule does not give the expected
  output.
* Patch 4 re-enables the tests.

Jeremy Sowden (4):
  tests: iptables-test: rename variable
  tests: add `NOMATCH` test result
  tests: support explicit variant test result
  tests: NFLOG: enable `--nflog-range` tests

 extensions/libxt_NFLOG.t | 12 +++----
 iptables-test.py         | 72 ++++++++++++++++++++++++++--------------
 2 files changed, 53 insertions(+), 31 deletions(-)

-- 
2.34.1

[iptables PATCH 1/4] tests: iptables-test: rename variable

[Jeremy Sowden]

"Splitted" hasn't been current since the seventeenth century.  Replace it with
"tokens".

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables-test.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/iptables-test.py b/iptables-test.py
index b49afcab3c40..91c77e3dc0e0 100755
--- a/iptables-test.py
+++ b/iptables-test.py
@@ -107,20 +107,20 @@ def run_test(iptables, rule, rule_save, res, filename, lineno, netns):
             return -1
 
     matching = 0
-    splitted = iptables.split(" ")
-    if len(splitted) == 2:
-        if splitted[1] == '-4':
+    tokens = iptables.split(" ")
+    if len(tokens) == 2:
+        if tokens[1] == '-4':
             command = IPTABLES_SAVE
-        elif splitted[1] == '-6':
+        elif tokens[1] == '-6':
             command = IP6TABLES_SAVE
-    elif len(splitted) == 1:
-        if splitted[0] == IPTABLES:
+    elif len(tokens) == 1:
+        if tokens[0] == IPTABLES:
             command = IPTABLES_SAVE
-        elif splitted[0] == IP6TABLES:
+        elif tokens[0] == IP6TABLES:
             command = IP6TABLES_SAVE
-        elif splitted[0] == ARPTABLES:
+        elif tokens[0] == ARPTABLES:
             command = ARPTABLES_SAVE
-        elif splitted[0] == EBTABLES:
+        elif tokens[0] == EBTABLES:
             command = EBTABLES_SAVE
 
     command = EXECUTABLE + " " + command
@@ -128,7 +128,7 @@ def run_test(iptables, rule, rule_save, res, filename, lineno, netns):
     if netns:
             command = "ip netns exec ____iptables-container-test " + command
 
-    args = splitted[1:]
+    args = tokens[1:]
     proc = subprocess.Popen(command, shell=True,
                             stdin=subprocess.PIPE,
                             stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-- 
2.34.1

[iptables PATCH 2/4] tests: add `NOMATCH` test result

[Jeremy Sowden]

Currently, there are two supported test results: `OK` and `FAIL`.  It is
expected that either the iptables command fails, or it succeeds and
dumping the rule has the correct output.  However, it is possible that
the command may succeed but the output may not be correct.  Add a
`NOMATCH` result to cover this outcome.

Make a few white-space improvements at the same time.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables-test.py | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/iptables-test.py b/iptables-test.py
index 91c77e3dc0e0..4a587a29c823 100755
--- a/iptables-test.py
+++ b/iptables-test.py
@@ -73,9 +73,9 @@ def run_test(iptables, rule, rule_save, res, filename, lineno, netns):
     Executes an unit test. Returns the output of delete_rule().
 
     Parameters:
-    :param  iptables: string with the iptables command to execute
+    :param iptables: string with the iptables command to execute
     :param rule: string with iptables arguments for the rule to test
-    :param rule_save: string to find the rule in the output of iptables -save
+    :param rule_save: string to find the rule in the output of iptables-save
     :param res: expected result of the rule. Valid values: "OK", "FAIL"
     :param filename: name of the file tested (used for print_error purposes)
     :param lineno: line number being tested (used for print_error purposes)
@@ -92,7 +92,7 @@ def run_test(iptables, rule, rule_save, res, filename, lineno, netns):
     # report failed test
     #
     if ret:
-        if res == "OK":
+        if res != "FAIL":
             reason = "cannot load: " + cmd
             print_error(reason, filename, lineno)
             return -1
@@ -146,10 +146,20 @@ def run_test(iptables, rule, rule_save, res, filename, lineno, netns):
     # find the rule
     matching = out.find(rule_save.encode('utf-8'))
     if matching < 0:
-        reason = "cannot find: " + iptables + " -I " + rule
-        print_error(reason, filename, lineno)
-        delete_rule(iptables, rule, filename, lineno)
-        return -1
+        if res == "OK":
+            reason = "cannot find: " + iptables + " -I " + rule
+            print_error(reason, filename, lineno)
+            delete_rule(iptables, rule, filename, lineno)
+            return -1
+        else:
+            # do not report this error
+            return 0
+    else:
+        if res != "OK":
+            reason = "should not match: " + cmd
+            print_error(reason, filename, lineno)
+            delete_rule(iptables, rule, filename, lineno)
+            return -1
 
     # Test "ip netns del NETNS" path with rules in place
     if netns:
@@ -190,14 +200,18 @@ def variant_res(res, variant):
     result. Therefore map @res to itself if given variant is current, invert it
     otherwise.
 
-    :param res: expected result from test spec ("OK" or "FAIL")
+    :param res: expected result from test spec ("OK", "FAIL" or "NOMATCH")
     :param variant: variant @res is scoped to by test spec ("NFT" or "LEGACY")
     '''
     variant_executable = {
-            "NFT": "xtables-nft-multi",
-            "LEGACY": "xtables-legacy-multi"
+        "NFT": "xtables-nft-multi",
+        "LEGACY": "xtables-legacy-multi"
+    }
+    res_inverse = {
+        "OK": "FAIL",
+        "FAIL": "OK",
+        "NOMATCH": "OK"
     }
-    res_inverse = { "OK": "FAIL", "FAIL": "OK" }
 
     if variant_executable[variant] == EXECUTABLE:
         return res
-- 
2.34.1

[iptables PATCH 3/4] tests: support explicit variant test result

[Jeremy Sowden]

Now that there are more than two test results, add support for
explicitly indicating which result to expect if the variants differ.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables-test.py | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/iptables-test.py b/iptables-test.py
index 4a587a29c823..6acaa82228fa 100755
--- a/iptables-test.py
+++ b/iptables-test.py
@@ -186,22 +186,23 @@ def execute_cmd(cmd, filename, lineno):
     log_file.flush()
 
     # generic check for segfaults
-    if ret  == -11:
+    if ret == -11:
         reason = "command segfaults: " + cmd
         print_error(reason, filename, lineno)
     return ret
 
 
-def variant_res(res, variant):
+def variant_res(res, variant, alt_res=None):
     '''
     Adjust expected result with given variant
 
     If expected result is scoped to a variant, the other one yields a different
-    result. Therefore map @res to itself if given variant is current, invert it
-    otherwise.
+    result. Therefore map @res to itself if given variant is current, use the
+    alternate result, @alt_res, if specified, invert @res otherwise.
 
     :param res: expected result from test spec ("OK", "FAIL" or "NOMATCH")
     :param variant: variant @res is scoped to by test spec ("NFT" or "LEGACY")
+    :param alt_res: optional expected result for the alternate variant.
     '''
     variant_executable = {
         "NFT": "xtables-nft-multi",
@@ -215,6 +216,8 @@ def variant_res(res, variant):
 
     if variant_executable[variant] == EXECUTABLE:
         return res
+    if alt_res is not None:
+        return alt_res
     return res_inverse[res]
 
 
@@ -312,7 +315,12 @@ def run_test_file(filename, netns):
 
             res = item[2].rstrip()
             if len(item) > 3:
-                res = variant_res(res, item[3].rstrip())
+                variant = item[3].rstrip()
+                if len(item) > 4:
+                    alt_res = item[4].rstrip()
+                else:
+                    alt_res = None
+                res = variant_res(res, variant, alt_res)
 
             ret = run_test(iptables, rule, rule_save,
                            res, filename, lineno + 1, netns)
-- 
2.34.1

[iptables PATCH 4/4] tests: NFLOG: enable `--nflog-range` tests

[Jeremy Sowden]

iptables-legacy and iptable-nft have different results for these tests.
Now that it is possible to specify the expected results correctly, we
can enable the tests.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.t | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/extensions/libxt_NFLOG.t b/extensions/libxt_NFLOG.t
index 561ec8c77650..25f332ae16b6 100644
--- a/extensions/libxt_NFLOG.t
+++ b/extensions/libxt_NFLOG.t
@@ -3,12 +3,12 @@
 -j NFLOG --nflog-group 65535;=;OK
 -j NFLOG --nflog-group 65536;;FAIL
 -j NFLOG --nflog-group 0;-j NFLOG;OK
-# `--nflog-range` is broken and only supported by xtables-legacy.  It
-# has been superseded by `--nflog--group`.
-# -j NFLOG --nflog-range 1;=;OK
-# -j NFLOG --nflog-range 4294967295;=;OK
-# -j NFLOG --nflog-range 4294967296;;FAIL
-# -j NFLOG --nflog-range -1;;FAIL
+# `--nflog-range` is broken and only supported by xtables-legacy.
+# It has been superseded by `--nflog--group`.
+-j NFLOG --nflog-range 1;=;OK;LEGACY;NOMATCH
+-j NFLOG --nflog-range 4294967295;=;OK;LEGACY;NOMATCH
+-j NFLOG --nflog-range 4294967296;;FAIL
+-j NFLOG --nflog-range -1;;FAIL
 -j NFLOG --nflog-size 0;=;OK
 -j NFLOG --nflog-size 1;=;OK
 -j NFLOG --nflog-size 4294967295;=;OK
-- 
2.34.1

Re: [iptables PATCH 0/4] Re-enable NFLOG tests

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> Some of the NFLOG tests were disabled when iptables-nft was changed to
> use nft's nflog implementation, because nft doesn't support
> `--nflog-range`.  This patch-set builds on Phil's recent work to support
> different test results for -legacy and -nft in order to re-enable those
> tests.

Series applied, thank you.

Re: [iptables PATCH 2/4] tests: add `NOMATCH` test result

[Phil Sutter]

Hi Jeremy,

On Sat, Feb 12, 2022 at 04:58:30PM +0000, Jeremy Sowden wrote:
> Currently, there are two supported test results: `OK` and `FAIL`.  It is
> expected that either the iptables command fails, or it succeeds and
> dumping the rule has the correct output.  However, it is possible that
> the command may succeed but the output may not be correct.  Add a
> `NOMATCH` result to cover this outcome.

Hmm. Wouldn't it make sense to extend the scope of LEGACY/NFT keywords
to output checks as well instead of introducing a new one? I think we
could cover expected output that way by duplicating the test case with
different expected output instead of marking it as unspecific "may
produce garbage".

Cheers, Phil

Re: [iptables PATCH 2/4] tests: add `NOMATCH` test result

[Jeremy Sowden]

[-- Attachment #1.1: Type: text/plain, Size: 1146 bytes --]

On 2022-02-14, at 11:01:13 +0100, Phil Sutter wrote:
> On Sat, Feb 12, 2022 at 04:58:30PM +0000, Jeremy Sowden wrote:
> > Currently, there are two supported test results: `OK` and `FAIL`.
> > It is expected that either the iptables command fails, or it
> > succeeds and dumping the rule has the correct output.  However, it
> > is possible that the command may succeed but the output may not be
> > correct.  Add a `NOMATCH` result to cover this outcome.
>
> Hmm. Wouldn't it make sense to extend the scope of LEGACY/NFT keywords
> to output checks as well instead of introducing a new one? I think we
> could cover expected output that way by duplicating the test case with
> different expected output instead of marking it as unspecific "may
> produce garbage".

Something like the following?  One reason why I went with the `NOMATCH`
result is that in the two divergent test-cases, there is no -nft output
to match.  We can make that work by just using the empty string as the
alternative output since that will match anything.  I don't think it's
ideal, but it's simpler than overhauling the matching code for what is a
rare corner case.

J.

[-- Attachment #1.2: v2-0001-tests-specify-non-matching-output-instead-of-NOMA.patch --]
[-- Type: text/x-diff, Size: 5398 bytes --]

From a18fa07425f82d71a46846e4f3656ec65155fd0c Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Sun, 20 Feb 2022 12:49:23 +0000
Subject: [iptables PATCH v2] tests: specify non-matching output instead of
 `NOMATCH` test result

Recently, we introduced a new test result `NOMATCH` to cover the case
where the test succeeds for both -legacy and -nft, but the two variants
do not have the same output.  This patch does away with the new result
in favour of specifying the alternative output.  For example, instead of

  -j EXAMPLE-TARGET --example-option;=;OK;LEGACY;NOMATCH

which specifies that the test passes for -legacy with the output
matching the rule, but for -nft it passes with output which does not
match the rule, we specify the test as:

  -j EXAMPLE-TARGET --example-option;=;OK;LEGACY;-j EXAMPLE-TARGET --nft-option

which specifies that the test passes for -legacy with the output
matching the rule, but for -nft it passes with the different output
given in the last field.

In the case of tests which have no output to match, we leave the last
field empty:

  -j EXAMPLE-TARGET --example-option;=;OK;LEGACY;

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.t |  4 ++--
 iptables-test.py         | 50 ++++++++++++++++++++++++++++++----------
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/extensions/libxt_NFLOG.t b/extensions/libxt_NFLOG.t
index 25f332ae16b6..b3241f51b153 100644
--- a/extensions/libxt_NFLOG.t
+++ b/extensions/libxt_NFLOG.t
@@ -5,8 +5,8 @@
 -j NFLOG --nflog-group 0;-j NFLOG;OK
 # `--nflog-range` is broken and only supported by xtables-legacy.
 # It has been superseded by `--nflog--group`.
--j NFLOG --nflog-range 1;=;OK;LEGACY;NOMATCH
--j NFLOG --nflog-range 4294967295;=;OK;LEGACY;NOMATCH
+-j NFLOG --nflog-range 1;=;OK;LEGACY;
+-j NFLOG --nflog-range 4294967295;=;OK;LEGACY;
 -j NFLOG --nflog-range 4294967296;;FAIL
 -j NFLOG --nflog-range -1;;FAIL
 -j NFLOG --nflog-size 0;=;OK
diff --git a/iptables-test.py b/iptables-test.py
index 6acaa82228fa..763e5b449ca5 100755
--- a/iptables-test.py
+++ b/iptables-test.py
@@ -192,17 +192,18 @@ def execute_cmd(cmd, filename, lineno):
     return ret
 
 
-def variant_res(res, variant, alt_res=None):
+def variant_res(res, variant, alt_rule_save):
     '''
     Adjust expected result with given variant
 
     If expected result is scoped to a variant, the other one yields a different
-    result. Therefore map @res to itself if given variant is current, use the
-    alternate result, @alt_res, if specified, invert @res otherwise.
+    result or different output. Therefore map @res to itself if given variant is
+    current, use "OK" if the expected result is "OK" but the other variant yields
+    different output, invert @res otherwise.
 
-    :param res: expected result from test spec ("OK", "FAIL" or "NOMATCH")
+    :param res: expected result from test spec ("OK" or "FAIL")
     :param variant: variant @res is scoped to by test spec ("NFT" or "LEGACY")
-    :param alt_res: optional expected result for the alternate variant.
+    :param alt_rule_save: alternative output if the variants have different output
     '''
     variant_executable = {
         "NFT": "xtables-nft-multi",
@@ -210,17 +211,40 @@ def variant_res(res, variant, alt_res=None):
     }
     res_inverse = {
         "OK": "FAIL",
-        "FAIL": "OK",
-        "NOMATCH": "OK"
+        "FAIL": "OK"
     }
 
     if variant_executable[variant] == EXECUTABLE:
         return res
-    if alt_res is not None:
-        return alt_res
+    if res == "OK" and alt_rule_save is not None:
+        return res
     return res_inverse[res]
 
 
+def variant_rule_save(rule_save, variant, alt_rule_save):
+    '''
+    Adjust expected output with given variant
+
+    If expected output is scoped to a variant, the other one yields different
+    output. Therefore map @rule_save to itself if given variant is current, use the
+    alternate output, @alt_rule_save otherwise.
+
+    :param rule_save: expected output if variant matches test spec
+    :param variant: variant given in test spec
+    :param alt_rule_save: expected output if the variant does not match the
+                          test spec
+    '''
+    variant_executable = {
+        "NFT": "xtables-nft-multi",
+        "LEGACY": "xtables-legacy-multi"
+    }
+
+    if variant_executable[variant] == EXECUTABLE:
+        return rule_save
+    else:
+        return alt_rule_save
+
+
 def run_test_file(filename, netns):
     '''
     Runs a test file
@@ -317,10 +341,12 @@ def run_test_file(filename, netns):
             if len(item) > 3:
                 variant = item[3].rstrip()
                 if len(item) > 4:
-                    alt_res = item[4].rstrip()
+                    alt_rule_save = item[4].rstrip()
                 else:
-                    alt_res = None
-                res = variant_res(res, variant, alt_res)
+                    alt_rule_save = None
+
+                res = variant_res(res, variant, alt_rule_save)
+                rule_save = variant_rule_save(rule_save, variant, alt_rule_save)
 
             ret = run_test(iptables, rule, rule_save,
                            res, filename, lineno + 1, netns)
-- 
2.34.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [iptables PATCH 2/4] tests: add `NOMATCH` test result

[Phil Sutter]

Hi Jeremy,

On Sun, Feb 20, 2022 at 01:10:29PM +0000, Jeremy Sowden wrote:
> On 2022-02-14, at 11:01:13 +0100, Phil Sutter wrote:
> > On Sat, Feb 12, 2022 at 04:58:30PM +0000, Jeremy Sowden wrote:
> > > Currently, there are two supported test results: `OK` and `FAIL`.
> > > It is expected that either the iptables command fails, or it
> > > succeeds and dumping the rule has the correct output.  However, it
> > > is possible that the command may succeed but the output may not be
> > > correct.  Add a `NOMATCH` result to cover this outcome.
> >
> > Hmm. Wouldn't it make sense to extend the scope of LEGACY/NFT keywords
> > to output checks as well instead of introducing a new one? I think we
> > could cover expected output that way by duplicating the test case with
> > different expected output instead of marking it as unspecific "may
> > produce garbage".
> 
> Something like the following?  One reason why I went with the `NOMATCH`
> result is that in the two divergent test-cases, there is no -nft output
> to match.  We can make that work by just using the empty string as the
> alternative output since that will match anything.  I don't think it's
> ideal, but it's simpler than overhauling the matching code for what is a
> rare corner case.

Thanks for compiling the patch. What I had in mind was to merge result
checks of failing rule with output mismatch, but I realize this would
likely turn into a mess.

[...]
> In the case of tests which have no output to match, we leave the last
> field empty:
> 
>   -j EXAMPLE-TARGET --example-option;=;OK;LEGACY;

A non-empty rule leading to empty output is a bug, IMHO.

[...]
> --- a/extensions/libxt_NFLOG.t
> +++ b/extensions/libxt_NFLOG.t
> @@ -5,8 +5,8 @@
>  -j NFLOG --nflog-group 0;-j NFLOG;OK
>  # `--nflog-range` is broken and only supported by xtables-legacy.
>  # It has been superseded by `--nflog--group`.
> --j NFLOG --nflog-range 1;=;OK;LEGACY;NOMATCH
> --j NFLOG --nflog-range 4294967295;=;OK;LEGACY;NOMATCH
> +-j NFLOG --nflog-range 1;=;OK;LEGACY;
> +-j NFLOG --nflog-range 4294967295;=;OK;LEGACY;

The crucial detail here is that an expected output of "-j NFLOG" is
trivial and str::find() won't complain about extra output.

Given that we're discussing corner cases and what I had in mind has its
own downsides, I guess the status quo is fine at least for now. Sorry
for the fuss!

Cheers, Phil