[PATCH nft] tests: validate generated netlink instructions

[PATCH nft] tests: validate generated netlink instructions

[Florian Westphal]

Don't apply -- this patch is intentionally incomplete;
I don't want to spam this list with 350k patch.

If you think this is worthwile to have in nft I'll push the full
changeset (this patch + 64 test files with recorded netlink debug output).

The test script is extended to compare netlink instructions generated
by each of the nft test files with recorded version.

Example: udp dport 80 accept in ip family should look like

ip test-ip4 input
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x00000011 ]
  [ payload load 2b @ transport header + 2 => reg 1 ]
  [ cmp eq reg 1 0x00005000 ]
  [ immediate reg 0 accept ]

This is stored in udp.t.payload.ip

Other suffixes:
.payload.ip6
.payload.inet
.payload ('any')

The test script first looks for 'testname.t.payload.$family', if that
doesn't exist 'testname.t.payload' is used.

This allows for family independent test (e.g. meta).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/regression/nft-test.py | 100 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 95 insertions(+), 5 deletions(-)

Full diffstat with test files:
 65 files changed, 11259 insertions(+), 5 deletions(-)
 An excerpt of the ct.t.payload is included here
 so you can have a look at the format.

diff --git a/tests/regression/nft-test.py b/tests/regression/nft-test.py
index 153f5e8..26fc2ec 100755
--- a/tests/regression/nft-test.py
+++ b/tests/regression/nft-test.py
@@ -423,9 +423,32 @@ def output_clean(pre_output, chain):
         return ""
     return rule
 
+def payload_check(payload_buffer, file, cmd):
+
+    file.seek(0, 0)
+
+    ret = False
+    i = 0
+
+    for lineno, want_line in enumerate(payload_buffer):
+        line = file.readline()
+
+        if want_line == line:
+            i += 1
+            continue
+
+        if want_line.find('[') < 0 and line.find('[') < 0:
+            continue
+        if want_line.find(']') < 0 and line.find(']') < 0:
+            continue
+
+        print_differences_warning(file.name, lineno, want_line.strip(), line.strip(), cmd);
+        return 0
+
+    return i > 0
 
 def rule_add(rule, table_list, chain_list, filename, lineno,
-             force_all_family_option):
+             force_all_family_option, filename_path):
     '''
     Adds a rule
     '''
@@ -437,7 +460,23 @@ def rule_add(rule, table_list, chain_list, filename, lineno,
         print_error(reason, filename, lineno)
         return [-1, warning, error, unit_tests]
 
+    payload_expected = []
+
     for table in table_list:
+        try:
+            payload_log = open("%s.payload.%s" % (filename_path, table[0]))
+        except (IOError):
+            payload_log = open("%s.payload" % filename_path)
+
+        if rule[1].strip() == "ok":
+	    try:
+                payload_expected.index(rule[0])
+            except (ValueError):
+                payload_expected = payload_find_expected(payload_log, rule[0])
+
+                if payload_expected == []:
+                    print_error("did not find payload information for rule '%s'" % rule[0], payload_log.name, 1)
+
         for chain in chain_list:
             if len(rule) == 1:
                 reason = "Skipping malformed test. (" + \
@@ -450,7 +489,10 @@ def rule_add(rule, table_list, chain_list, filename, lineno,
             table_info = " " + table[0] + " " + table[1] + " "
             cmd = "nft add rule" + table_info + chain + " " + rule[0]
 
-            ret = execute_cmd(cmd, filename, lineno)
+            payload_log = os.tmpfile();
+
+            cmd = "nft add rule --debug=netlink" + table_info + chain + " " + rule[0]
+            ret = execute_cmd(cmd, filename, lineno, payload_log)
 
             state = rule[1].rstrip()
             if (ret == 0 and state == "fail") or (ret != 0 and state == "ok"):
@@ -470,6 +512,20 @@ def rule_add(rule, table_list, chain_list, filename, lineno,
                 continue
 
             if ret == 0:
+            # Check for matching payload
+                if state == "ok" and not payload_check(payload_expected, payload_log, cmd):
+                    error += 1
+                    gotf = open("%s.payload.got" % filename_path, 'a')
+                    payload_log.seek(0, 0)
+                    gotf.write("# %s\n" % rule[0])
+                    while True:
+                        line = payload_log.readline()
+                        if line == "":
+                            break
+                        gotf.write(line)
+	            gotf.close()
+                    print_warning("Wrote payload for rule %s" % rule[0], gotf.name, 1)
+
             # Check output of nft
                 process = subprocess.Popen(['nft', '-nnn', 'list', 'table'] + table,
                                            shell=False, stdout=subprocess.PIPE,
@@ -536,7 +592,7 @@ def signal_handler(signal, frame):
     signal_received = 1
 
 
-def execute_cmd(cmd, filename, lineno):
+def execute_cmd(cmd, filename, lineno, stdout_log = False):
     '''
     Executes a command, checks for segfaults and returns the command exit
     code.
@@ -549,8 +605,12 @@ def execute_cmd(cmd, filename, lineno):
     print >> log_file, "command: %s" % cmd
     if debug_option:
         print cmd
+
+    if not stdout_log:
+        stdout_log = log_file
+
     ret = subprocess.call(cmd, shell=True, universal_newlines=True,
-                          stderr=subprocess.STDOUT, stdout=log_file,
+                          stderr=log_file, stdout=stdout_log,
                           preexec_fn=preexec)
     log_file.flush()
 
@@ -619,6 +679,36 @@ def set_element_process(element_line, filename, lineno):
     return set_add_elements(set_element, set_name, all_set, rule_state,
                             table_list, filename, lineno)
 
+def payload_find_expected(payload_log, rule):
+    '''
+    Find the netlink payload that should be generated by given rule in payload_log
+
+    :param payload_log: open file handle of the payload data
+    :param rule: nft rule we are going to add
+    '''
+    found = 0
+    pos = 0
+    payload_buffer = []
+
+    while True:
+        line = payload_log.readline()
+	if not line:
+	    break
+
+        if line[0] == "#":  # rule start
+            rule_line = line.strip()[2:]
+
+            if rule_line == rule.strip():
+                found = 1
+                continue
+
+        if found == 1:
+            payload_buffer.append(line)
+            if line.isspace():
+                return payload_buffer
+
+    payload_log.seek(0, 0)
+    return payload_buffer
 
 def run_test_file(filename, force_all_family_option, specific_file):
     '''
@@ -699,7 +789,7 @@ def run_test_file(filename, force_all_family_option, specific_file):
             continue
 
         result = rule_add(rule, table_list, chain_list, filename, lineno,
-                          force_all_family_option)
+                          force_all_family_option, filename_path)
         tests += 1
         ret = result[0]
         warning = result[1]
diff --git a/tests/regression/any/ct.t.payload b/tests/regression/any/ct.t.payload
new file mode 100644
index 0000000..f77c284
--- /dev/null
+++ b/tests/regression/any/ct.t.payload
@@ -0,0 +1,239 @@
+# ct state new,established, related, untracked
+ip test-ip4 output
+  [ ct load state => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x0000004e ) ^ 0x00000000 ]
+  [ cmp neq reg 1 0x00000000 ]
+
+# ct state != related
+ip test-ip4 output
+  [ ct load state => reg 1 ]
+  [ cmp neq reg 1 0x00000004 ]
+
+# ct state {new,established, related, untracked}
+set%d test-ip4 3
+set%d test-ip4 0
+	element 00000008  : 0 [end]	element 00000002  : 0 [end]	element 00000004  : 0 [end]	element 00000040  : 0 [end]
+ip test-ip4 output
+  [ ct load state => reg 1 ]
+  [ lookup reg 1 set set%d ]
+
+# ct state invalid drop
+ip test-ip4 output
+  [ ct load state => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x00000001 ) ^ 0x00000000 ]
+  [ cmp neq reg 1 0x00000000 ]
+  [ immediate reg 0 drop ]
+
+# ct state established accept
+ip test-ip4 output
+  [ ct load state => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x00000002 ) ^ 0x00000000 ]
+  [ cmp neq reg 1 0x00000000 ]
+  [ immediate reg 0 accept ]
+
+# ct state 8
+ip test-ip4 output
+  [ ct load state => reg 1 ]
+  [ bitwise reg 1 = (reg=1 & 0x00000008 ) ^ 0x00000000 ]
+  [ cmp neq reg 1 0x00000000 ]

[ Rest omitted, they all look pretty much the same ...]

-- 
2.0.5

Re: [PATCH nft] tests: validate generated netlink instructions

[Pablo Neira Ayuso]

On Fri, Jul 17, 2015 at 03:31:39AM +0200, Florian Westphal wrote:
> Don't apply -- this patch is intentionally incomplete;
> I don't want to spam this list with 350k patch.
> 
> If you think this is worthwile to have in nft I'll push the full
> changeset (this patch + 64 test files with recorded netlink debug output).

I like the idea of having more regression tests.

We can probably kill the old .t files and move the 'regression' to
'tests'.

It would be good to add tests for the 'nft -f' path in some automated
way at some point.

Re: [PATCH nft] tests: validate generated netlink instructions

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jul 17, 2015 at 03:31:39AM +0200, Florian Westphal wrote:
> > Don't apply -- this patch is intentionally incomplete;
> > I don't want to spam this list with 350k patch.
> > 
> > If you think this is worthwile to have in nft I'll push the full
> > changeset (this patch + 64 test files with recorded netlink debug output).
> 
> I like the idea of having more regression tests.
> 
> We can probably kill the old .t files and move the 'regression' to
> 'tests'.

Do you mean:
rm tests/*; mv tests/regression/* tests/ ?

Otherwise I'm inclined to just push what I have and let you wrangle
things further if needed :-)

Re: [PATCH nft] tests: validate generated netlink instructions

[Pablo Neira Ayuso]

On Mon, Jul 20, 2015 at 05:10:46PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Jul 17, 2015 at 03:31:39AM +0200, Florian Westphal wrote:
> > > Don't apply -- this patch is intentionally incomplete;
> > > I don't want to spam this list with 350k patch.
> > > 
> > > If you think this is worthwile to have in nft I'll push the full
> > > changeset (this patch + 64 test files with recorded netlink debug output).
> > 
> > I like the idea of having more regression tests.
> > 
> > We can probably kill the old .t files and move the 'regression' to
> > 'tests'.
> 
> Do you mean:
> rm tests/*; mv tests/regression/* tests/ ?
> 
> Otherwise I'm inclined to just push what I have and let you wrangle
> things further if needed :-)

Sure, no problem :)

Re: [PATCH nft] tests: validate generated netlink instructions

[Pablo Neira Ayuso]

On Mon, Jul 20, 2015 at 05:10:46PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Jul 17, 2015 at 03:31:39AM +0200, Florian Westphal wrote:
> > > Don't apply -- this patch is intentionally incomplete;
> > > I don't want to spam this list with 350k patch.
> > > 
> > > If you think this is worthwile to have in nft I'll push the full
> > > changeset (this patch + 64 test files with recorded netlink debug output).
> > 
> > I like the idea of having more regression tests.
> > 
> > We can probably kill the old .t files and move the 'regression' to
> > 'tests'.
> 
> Do you mean:
> rm tests/*; mv tests/regression/* tests/ ?
> 
> Otherwise I'm inclined to just push what I have and let you wrangle
> things further if needed :-)

BTW, I just remembered another bug we've got in the output.

I think we have to convert to BE the content that is printed from the
registers, otherwise we'll have different outputs in LE and BE.

Re: [PATCH nft] tests: validate generated netlink instructions

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Jul 20, 2015 at 05:10:46PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Fri, Jul 17, 2015 at 03:31:39AM +0200, Florian Westphal wrote:
> > > > Don't apply -- this patch is intentionally incomplete;
> > > > I don't want to spam this list with 350k patch.
> > > > 
> > > > If you think this is worthwile to have in nft I'll push the full
> > > > changeset (this patch + 64 test files with recorded netlink debug output).
> > > 
> > > I like the idea of having more regression tests.
> > > 
> > > We can probably kill the old .t files and move the 'regression' to
> > > 'tests'.
> > 
> > Do you mean:
> > rm tests/*; mv tests/regression/* tests/ ?
> > 
> > Otherwise I'm inclined to just push what I have and let you wrangle
> > things further if needed :-)
> 
> BTW, I just remembered another bug we've got in the output.
> 
> I think we have to convert to BE the content that is printed from the
> registers, otherwise we'll have different outputs in LE and BE.

Right.  Another solution would be to keep two sets of each payload in
tests/, one for LE and one for BE platforms, and use the correct one
arcording to whatever arch the script runs on.

Re: [PATCH nft] tests: validate generated netlink instructions

[Pablo Neira Ayuso]

Hi Florian,

I found a problem in your change to validate the netlink instructions
from the python infrastructure that we have for nft.

The set elements are not always displayed in the same order depending
on the hash seed, so we get bogus warnings in that case.

I think the fix for the test infrastructure will require something a
bit more complicated that a simple string comparison as we'll need to
interpret the set element part.

Probably it would be good to wrap the netlink instruction generation
code under some option until this is resolved, instead of having it
enabled by default.

Let me know if you come up with any better idea. Thanks!

Re: [PATCH nft] tests: validate generated netlink instructions

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I found a problem in your change to validate the netlink instructions
> from the python infrastructure that we have for nft.
> 
> The set elements are not always displayed in the same order depending
> on the hash seed, so we get bogus warnings in that case.

Did that change recently?
I run the tests quite extensively at the moment and I did not see
failures in the set parts yet.

> I think the fix for the test infrastructure will require something a
> bit more complicated that a simple string comparison as we'll need to
> interpret the set element part.
> 
> Probably it would be good to wrap the netlink instruction generation
> code under some option until this is resolved, instead of having it
> enabled by default.
> 
> Let me know if you come up with any better idea. Thanks!

I'm currently in the process of finalizing a first draft of vlan
matching, i think i have patches ready next week.

This will also make "nft add rule bridge filter input ip version 4"
work since it adds support for sub-byte sized header elements.

I plan to work on the test suite again after I get v1 out (add BE support
so we can also check nft on s390 etc).

I haven't thought about it yet, first plan was to record separate traces
for LE and BE architectures, think thats better than trying to normalize
the endianess in the output (might also mask errors...).

I'll try to figure out a way to cure the set part.

One way would be accomondate the test parser to recognize the set data
and sort those into some common order (doesn't matter as long as both ondisk
and observed output are in the same sort order).

I don't mind if you add a quick patch that disables the payload
comparision for now, we can reenable it later by default once BE + set
works correctly.

Re: [PATCH nft] tests: validate generated netlink instructions

[Pablo Neira Ayuso]

On Wed, Aug 12, 2015 at 07:46:24PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I found a problem in your change to validate the netlink instructions
> > from the python infrastructure that we have for nft.
> > 
> > The set elements are not always displayed in the same order depending
> > on the hash seed, so we get bogus warnings in that case.
> 
> Did that change recently?
> I run the tests quite extensively at the moment and I did not see
> failures in the set parts yet.

I remember Ana had this problem, currently the script is placing the
elements in a python set to make sure the comparison doesn't break.

> > I think the fix for the test infrastructure will require something a
> > bit more complicated that a simple string comparison as we'll need to
> > interpret the set element part.
> > 
> > Probably it would be good to wrap the netlink instruction generation
> > code under some option until this is resolved, instead of having it
> > enabled by default.
> > 
> > Let me know if you come up with any better idea. Thanks!
> 
> I'm currently in the process of finalizing a first draft of vlan
> matching, i think i have patches ready next week.

Good to know, thanks.

> This will also make "nft add rule bridge filter input ip version 4"
> work since it adds support for sub-byte sized header elements.

Are you using bitwise for that?

> I plan to work on the test suite again after I get v1 out (add BE support
> so we can also check nft on s390 etc).
> 
> I haven't thought about it yet, first plan was to record separate traces
> for LE and BE architectures, think thats better than trying to normalize
> the endianess in the output (might also mask errors...).

My concern is that this might replicate the number of files to
maintain.

> I'll try to figure out a way to cure the set part.

Thanks.

[...]
> I don't mind if you add a quick patch that disables the payload
> comparision for now, we can reenable it later by default once BE + set
> works correctly.

I'm currently disabling this manually here, I can wait a while until
this is fixed.

Re: [PATCH nft] tests: validate generated netlink instructions

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > This will also make "nft add rule bridge filter input ip version 4"
> > work since it adds support for sub-byte sized header elements.
> 
> Are you using bitwise for that?

yes: input ip version 4
 [ payload load 1b @ network header + 0 => reg 1 ]
 [ bitwise reg 1 = (reg=1 & 0x000000f0 ) ^ 0x00000000 ]
 [ cmp eq reg 1 0x00000040 ]

relational_binop_postprocess() is responsible to zap such implicit binops
again when listing a table.

> > I plan to work on the test suite again after I get v1 out (add BE support
> > so we can also check nft on s390 etc).
> > 
> > I haven't thought about it yet, first plan was to record separate traces
> > for LE and BE architectures, think thats better than trying to normalize
> > the endianess in the output (might also mask errors...).
> 
> My concern is that this might replicate the number of files to
> maintain.

Yes, thats true, when adding new rule to test suite one would need to
run nft on both LE and BE system to get the generated instructions for
both...

Not sure if there is a better solution though.

Re: [PATCH nft] tests: validate generated netlink instructions

[Patrick McHardy]

On 12.08, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I found a problem in your change to validate the netlink instructions
> > from the python infrastructure that we have for nft.
> > 
> > The set elements are not always displayed in the same order depending
> > on the hash seed, so we get bogus warnings in that case.
> 
> Did that change recently?
> I run the tests quite extensively at the moment and I did not see
> failures in the set parts yet.
> 
> > I think the fix for the test infrastructure will require something a
> > bit more complicated that a simple string comparison as we'll need to
> > interpret the set element part.
> > 
> > Probably it would be good to wrap the netlink instruction generation
> > code under some option until this is resolved, instead of having it
> > enabled by default.
> > 
> > Let me know if you come up with any better idea. Thanks!
> 
> I'm currently in the process of finalizing a first draft of vlan
> matching, i think i have patches ready next week.
> 
> This will also make "nft add rule bridge filter input ip version 4"
> work since it adds support for sub-byte sized header elements.

I also have patches for this, but some corner cases and not working correctly
yet. I'm looking forward to your patches.

> I plan to work on the test suite again after I get v1 out (add BE support
> so we can also check nft on s390 etc).
> 
> I haven't thought about it yet, first plan was to record separate traces
> for LE and BE architectures, think thats better than trying to normalize
> the endianess in the output (might also mask errors...).
> 
> I'll try to figure out a way to cure the set part.
> 
> One way would be accomondate the test parser to recognize the set data
> and sort those into some common order (doesn't matter as long as both ondisk
> and observed output are in the same sort order).
> 
> I don't mind if you add a quick patch that disables the payload
> comparision for now, we can reenable it later by default once BE + set
> works correctly.

I actually think we should sort it in nft since we might also see duplicate
netlink messages and should eleminate them from the output since they will
cause errors on reload.

Re: [PATCH nft] tests: validate generated netlink instructions

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> I also have patches for this, but some corner cases and not working correctly
> yet. I'm looking forward to your patches.

Any example?

I'm curious to see if my patches handles those :-)

Re: [PATCH nft] tests: validate generated netlink instructions

[Patrick McHardy]

On 16.08, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > I also have patches for this, but some corner cases and not working correctly
> > yet. I'm looking forward to your patches.
> 
> Any example?
> 
> I'm curious to see if my patches handles those :-)

Unfortunately not right now, new notebook and the data is not fully synced yet :)

My general approach is to generate shift expressions for data that is not byte
aligned, then have the shifts transfered to the constant side during bitop eval.

The cases which do not work fully correctly are those where the length of the
constant side needs to be increased to contain the shifted value. It will
incorrectly accept too large values for the field in question. F.i.
ip totlength is 4 bit wide and needs to be shifted by 4 bit, so my simple
approach of adding 4 to expr->len will allow values up to 255 from the user.

I'll send my patches over tommorrow so you can see if there's anything you
can use :)

Re: [PATCH nft] tests: validate generated netlink instructions

[Pablo Neira Ayuso]

On Sun, Aug 16, 2015 at 07:14:40PM +0100, Patrick McHardy wrote:
[...]
> I actually think we should sort it in nft since we might also see duplicate
> netlink messages and should eleminate them from the output since they will
> cause errors on reload.

Right, we need that --sort option so the user always get the same
output when saving the ruleset. The removal of duplicates is also an
issue given the way netlink dumps things.

But the issue with tests is related to --debug=netlink output, so you
suggest we address both sorting and duplication removal from libnftnl, right?

Thanks!