[PATCH nft 0/4] json test case fixups

[PATCH nft 0/4] json test case fixups

[Florian Westphal]

json tests fail in several cases because test files were out of data.
This refreshs them.

One bug is genuine: in limit statement, json parser should treat
a missing burst as 5, not 0.

Florian Westphal (4):
  json: fix icmpv6.t test cases
  json: limit: set default burst to 5
  json: ct: add missing rule
  json: icmp: refresh json output

 src/parser_json.c                 |   2 +-
 tests/py/ip/ct.t.json             |  30 ++
 tests/py/ip/icmp.t.json           | 648 ++++++++++++++++++++++++++----
 tests/py/ip6/icmpv6.t.json        |  27 +-
 tests/py/ip6/icmpv6.t.json.output | 586 ++++++++++++++++++++++++++-
 5 files changed, 1196 insertions(+), 97 deletions(-)

-- 
2.26.2

[PATCH 1/4] json: fix icmpv6.t test cases

[Florian Westphal]

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/ip6/icmpv6.t.json        |  27 +-
 tests/py/ip6/icmpv6.t.json.output | 586 +++++++++++++++++++++++++++++-
 2 files changed, 597 insertions(+), 16 deletions(-)

diff --git a/tests/py/ip6/icmpv6.t.json b/tests/py/ip6/icmpv6.t.json
index f6cfbf172f56..ffc4931c4e0c 100644
--- a/tests/py/ip6/icmpv6.t.json
+++ b/tests/py/ip6/icmpv6.t.json
@@ -1185,7 +1185,7 @@
             "left": {
                 "payload": {
                     "field": "max-delay",
-                    "name": "icmpv6"
+                    "protocol": "icmpv6"
                 }
             },
 	    "op": "==",
@@ -1203,7 +1203,7 @@
             "left": {
                 "payload": {
                     "field": "max-delay",
-                    "name": "icmpv6"
+                    "protocol": "icmpv6"
                 }
             },
             "op": "!=",
@@ -1221,7 +1221,7 @@
             "left": {
                 "payload": {
                     "field": "max-delay",
-                    "name": "icmpv6"
+                    "protocol": "icmpv6"
                 }
             },
 	    "op": "==",
@@ -1244,7 +1244,7 @@
             "left": {
                 "payload": {
                     "field": "max-delay",
-                    "name": "icmpv6"
+                    "protocol": "icmpv6"
                 }
             },
             "op": "!=",
@@ -1267,7 +1267,7 @@
             "left": {
                 "payload": {
                     "field": "max-delay",
-                    "name": "icmpv6"
+                    "protocol": "icmpv6"
                 }
             },
 	    "op": "==",
@@ -1287,7 +1287,7 @@
             "left": {
                 "payload": {
                     "field": "max-delay",
-                    "name": "icmpv6"
+                    "protocol": "icmpv6"
                 }
             },
             "op": "!=",
@@ -1300,3 +1300,18 @@
     }
 ]
 
+# icmpv6 type packet-too-big icmpv6 mtu 1280
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "mtu",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": 1280
+        }
+    }
+]
diff --git a/tests/py/ip6/icmpv6.t.json.output b/tests/py/ip6/icmpv6.t.json.output
index 3a1066211f56..7b8f5c1909db 100644
--- a/tests/py/ip6/icmpv6.t.json.output
+++ b/tests/py/ip6/icmpv6.t.json.output
@@ -8,7 +8,7 @@
                     "protocol": "icmpv6"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "mld-listener-done"
         }
     },
@@ -27,7 +27,7 @@
                     "protocol": "icmpv6"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
                     "time-exceeded",
@@ -53,7 +53,7 @@
                     "protocol": "icmpv6"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
                     "time-exceeded",
@@ -103,7 +103,7 @@
                     "protocol": "icmpv6"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "port-unreachable"
         }
     }
@@ -119,9 +119,12 @@
                     "protocol": "icmpv6"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
-                "range": [ "addr-unreachable", 66 ]
+                "range": [
+                    "addr-unreachable",
+                    66
+                ]
             }
         }
     }
@@ -137,7 +140,7 @@
                     "protocol": "icmpv6"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
                     "policy-fail",
@@ -162,10 +165,15 @@
                     "protocol": "icmpv6"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    { "range": [ "addr-unreachable", 66 ] }
+                    {
+                        "range": [
+                            "addr-unreachable",
+                            66
+                        ]
+                    }
                 ]
             }
         }
@@ -185,7 +193,565 @@
             "op": "!=",
             "right": {
                 "set": [
-                    { "range": [ "addr-unreachable", 66 ] }
+                    {
+                        "range": [
+                            "addr-unreachable",
+                            66
+                        ]
+                    }
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 id 33-45
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "range": [
+                    33,
+                    45
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 id != 33-45
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "!=",
+            "right": {
+                "range": [
+                    33,
+                    45
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 id {33, 55, 67, 88}
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    33,
+                    55,
+                    67,
+                    88
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 id != {33, 55, 67, 88}
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "!=",
+            "right": {
+                "set": [
+                    33,
+                    55,
+                    67,
+                    88
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 id {33-55}
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 id != {33-55}
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "!=",
+            "right": {
+                "set": [
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 sequence 2
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": 2
+        }
+    }
+]
+
+# icmpv6 sequence {3, 4, 5, 6, 7} accept
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    3,
+                    4,
+                    5,
+                    6,
+                    7
+                ]
+            }
+        }
+    },
+    {
+        "accept": null
+    }
+]
+
+# icmpv6 sequence {2, 4}
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    2,
+                    4
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 sequence != {2, 4}
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "!=",
+            "right": {
+                "set": [
+                    2,
+                    4
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 sequence 2-4
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "range": [
+                    2,
+                    4
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 sequence != 2-4
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "!=",
+            "right": {
+                "range": [
+                    2,
+                    4
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 sequence { 2-4}
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    {
+                        "range": [
+                            2,
+                            4
+                        ]
+                    }
+                ]
+            }
+        }
+    }
+]
+
+# icmpv6 sequence != { 2-4}
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "!=",
+            "right": {
+                "set": [
+                    {
+                        "range": [
+                            2,
+                            4
+                        ]
+                    }
                 ]
             }
         }
-- 
2.26.2

Re: [PATCH 1/4] json: fix icmpv6.t test cases

[Phil Sutter]

On Thu, Jan 21, 2021 at 02:55:07PM +0100, Florian Westphal wrote:
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Phil Sutter <phil@nwl.cc>

[PATCH 2/4] json: limit: set default burst to 5

[Florian Westphal]

The tests fail because json printing omits a burst of 5 and
the parser treats that as 'burst 0'.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index f0486b77a225..2d132caf529c 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -1784,7 +1784,7 @@ static struct stmt *json_parse_limit_stmt(struct json_ctx *ctx,
 					  const char *key, json_t *value)
 {
 	struct stmt *stmt;
-	uint64_t rate, burst = 0;
+	uint64_t rate, burst = 5;
 	const char *rate_unit = "packets", *time, *burst_unit = "bytes";
 	int inv = 0;
 
-- 
2.26.2

Re: [PATCH 2/4] json: limit: set default burst to 5

[Phil Sutter]

Hi!

On Thu, Jan 21, 2021 at 02:55:08PM +0100, Florian Westphal wrote:
> The tests fail because json printing omits a burst of 5 and
> the parser treats that as 'burst 0'.

While this patch is correct in that it aligns json and bison parser
behaviours, I think omitting burst value in JSON output is a bug by
itself: We don't care about output length and users are supposed to
parse (and thus filter) the information anyway, so there's no gain from
omitting such info. I'll address this in a separate patch, though.

Thanks, Phil

Re: [PATCH 2/4] json: limit: set default burst to 5

[Pablo Neira Ayuso]

Hi Phil,

On Thu, Jan 21, 2021 at 03:44:14PM +0100, Phil Sutter wrote:
> Hi!
> 
> On Thu, Jan 21, 2021 at 02:55:08PM +0100, Florian Westphal wrote:
> > The tests fail because json printing omits a burst of 5 and
> > the parser treats that as 'burst 0'.
> 
> While this patch is correct in that it aligns json and bison parser
> behaviours, I think omitting burst value in JSON output is a bug by
> itself: We don't care about output length and users are supposed to
> parse (and thus filter) the information anyway, so there's no gain from
> omitting such info. I'll address this in a separate patch, though.

The listing of:

nft list ruleset

is already omitting this. Would you prefer this is also exposed there?

Thanks.

Re: [PATCH 2/4] json: limit: set default burst to 5

[Pablo Neira Ayuso]

On Thu, Jan 21, 2021 at 03:57:59PM +0100, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Thu, Jan 21, 2021 at 03:44:14PM +0100, Phil Sutter wrote:
> > Hi!
> > 
> > On Thu, Jan 21, 2021 at 02:55:08PM +0100, Florian Westphal wrote:
> > > The tests fail because json printing omits a burst of 5 and
> > > the parser treats that as 'burst 0'.
> > 
> > While this patch is correct in that it aligns json and bison parser
> > behaviours, I think omitting burst value in JSON output is a bug by
> > itself: We don't care about output length and users are supposed to
> > parse (and thus filter) the information anyway, so there's no gain from
> > omitting such info. I'll address this in a separate patch, though.
> 
> The listing of:
> 
> nft list ruleset
> 
> is already omitting this. Would you prefer this is also exposed there?

I mean, IIRC for json it makes sense to display every field (not omit
anything), so my question is whether you think the native syntax
should omit this or it's fine as it is.

Re: [PATCH 2/4] json: limit: set default burst to 5

[Phil Sutter]

Hi Pablo,

On Thu, Jan 21, 2021 at 03:59:34PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 21, 2021 at 03:57:59PM +0100, Pablo Neira Ayuso wrote:
> > Hi Phil,
> > 
> > On Thu, Jan 21, 2021 at 03:44:14PM +0100, Phil Sutter wrote:
> > > Hi!
> > > 
> > > On Thu, Jan 21, 2021 at 02:55:08PM +0100, Florian Westphal wrote:
> > > > The tests fail because json printing omits a burst of 5 and
> > > > the parser treats that as 'burst 0'.
> > > 
> > > While this patch is correct in that it aligns json and bison parser
> > > behaviours, I think omitting burst value in JSON output is a bug by
> > > itself: We don't care about output length and users are supposed to
> > > parse (and thus filter) the information anyway, so there's no gain from
> > > omitting such info. I'll address this in a separate patch, though.
> > 
> > The listing of:
> > 
> > nft list ruleset
> > 
> > is already omitting this. Would you prefer this is also exposed there?
> 
> I mean, IIRC for json it makes sense to display every field (not omit
> anything), so my question is whether you think the native syntax
> should omit this or it's fine as it is.

You hit the bull's eye: I have a ticket about this behaviour already,
claiming that having a non-trivial default value and omitting it from
output is not a good idea. In practice, reporter created a limit
statement which doesn't work with default burst value (limit rate 1).

I'm not against omitting the burst, but it must not become a problem
then. So my idea to keep the benefits of both was to implement an "auto
burst value" which adjusts to the rate value. Do you think that's
feasible? Maybe something simple like 'burst = max(1, rate / 10)' (for
packets).

Cheers, Phil

[PATCH 3/4] json: ct: add missing rule

[Florian Westphal]

ERROR: did not find JSON equivalent for rule 'meta mark set ct original ip daddr map { 1.1.1.1 : 0x00000011 }'

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/ip/ct.t.json | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/py/ip/ct.t.json b/tests/py/ip/ct.t.json
index 881cd4c942c1..d942649a550f 100644
--- a/tests/py/ip/ct.t.json
+++ b/tests/py/ip/ct.t.json
@@ -216,3 +216,33 @@
     }
 ]
 
+# meta mark set ct original ip daddr map { 1.1.1.1 : 0x00000011 }
+[
+    {
+        "mangle": {
+            "key": {
+                "meta": {
+                    "key": "mark"
+                }
+            },
+            "value": {
+                "map": {
+                    "data": {
+                        "set": [
+                            [
+                                "1.1.1.1",
+                                17
+                            ]
+                        ]
+                    },
+                    "key": {
+                        "ct": {
+                            "dir": "original",
+                            "key": "ip daddr"
+                        }
+                    }
+                }
+            }
+        }
+    }
+]
-- 
2.26.2

Re: [PATCH 3/4] json: ct: add missing rule

[Phil Sutter]

On Thu, Jan 21, 2021 at 02:55:09PM +0100, Florian Westphal wrote:
> ERROR: did not find JSON equivalent for rule 'meta mark set ct original ip daddr map { 1.1.1.1 : 0x00000011 }'
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Fixes: 8b043938e77b1 ("evaluate: disallow ct original {s,d}ddr from
maps")
Acked-by: Phil Sutter <phil@nwl.cc>

[PATCH 4/4] json: icmp: refresh json output

[Florian Westphal]

nft inserts dependencies for icmp header types, but I forgot to
update the json test files to reflect this change.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/ip/icmp.t.json | 648 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 568 insertions(+), 80 deletions(-)

diff --git a/tests/py/ip/icmp.t.json b/tests/py/ip/icmp.t.json
index 965eb10be9ed..480740afb525 100644
--- a/tests/py/ip/icmp.t.json
+++ b/tests/py/ip/icmp.t.json
@@ -8,7 +8,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "echo-reply"
         }
     },
@@ -27,7 +27,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "destination-unreachable"
         }
     },
@@ -46,7 +46,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "source-quench"
         }
     },
@@ -65,7 +65,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "redirect"
         }
     },
@@ -84,7 +84,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "echo-request"
         }
     },
@@ -103,7 +103,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "time-exceeded"
         }
     },
@@ -122,7 +122,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "parameter-problem"
         }
     },
@@ -141,7 +141,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "timestamp-request"
         }
     },
@@ -160,7 +160,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "timestamp-reply"
         }
     },
@@ -179,7 +179,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "info-request"
         }
     },
@@ -198,7 +198,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "info-reply"
         }
     },
@@ -217,7 +217,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "address-mask-request"
         }
     },
@@ -236,7 +236,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "address-mask-reply"
         }
     },
@@ -255,7 +255,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "router-advertisement"
         }
     },
@@ -274,7 +274,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": "router-solicitation"
         }
     },
@@ -293,7 +293,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
                     "echo-reply",
@@ -301,6 +301,8 @@
                     "source-quench",
                     "redirect",
                     "echo-request",
+                    "router-advertisement",
+                    "router-solicitation",
                     "time-exceeded",
                     "parameter-problem",
                     "timestamp-request",
@@ -308,9 +310,7 @@
                     "info-request",
                     "info-reply",
                     "address-mask-request",
-                    "address-mask-reply",
-                    "router-advertisement",
-                    "router-solicitation"
+                    "address-mask-reply"
                 ]
             }
         }
@@ -352,7 +352,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": 111
         }
     },
@@ -390,9 +390,12 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
-                "range": [ 33, 55 ]
+                "range": [
+                    33,
+                    55
+                ]
             }
         }
     }
@@ -410,7 +413,10 @@
             },
             "op": "!=",
             "right": {
-                "range": [ 33, 55 ]
+                "range": [
+                    33,
+                    55
+                ]
             }
         }
     }
@@ -426,10 +432,15 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
@@ -449,7 +460,12 @@
             "op": "!=",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
@@ -466,11 +482,11 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    2,
-                    4,
+                    "prot-unreachable",
+                    "frag-needed",
                     33,
                     54,
                     56
@@ -514,7 +530,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": 12343
         }
     },
@@ -552,9 +568,12 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
-                "range": [ 11, 343 ]
+                "range": [
+                    11,
+                    343
+                ]
             }
         }
     },
@@ -575,7 +594,10 @@
             },
             "op": "!=",
             "right": {
-                "range": [ 11, 343 ]
+                "range": [
+                    11,
+                    343
+                ]
             }
         }
     },
@@ -594,10 +616,15 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    { "range": [ 11, 343 ] }
+                    {
+                        "range": [
+                            11,
+                            343
+                        ]
+                    }
                 ]
             }
         }
@@ -620,7 +647,12 @@
             "op": "!=",
             "right": {
                 "set": [
-                    { "range": [ 11, 343 ] }
+                    {
+                        "range": [
+                            11,
+                            343
+                        ]
+                    }
                 ]
             }
         }
@@ -640,12 +672,12 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    1111,
                     222,
-                    343
+                    343,
+                    1111
                 ]
             }
         }
@@ -668,9 +700,9 @@
             "op": "!=",
             "right": {
                 "set": [
-                    1111,
                     222,
-                    343
+                    343,
+                    1111
                 ]
             }
         }
@@ -682,6 +714,23 @@
 
 # icmp id 1245 log
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -690,7 +739,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": 1245
         }
     },
@@ -701,6 +750,23 @@
 
 # icmp id 22
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -709,7 +775,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": 22
         }
     }
@@ -717,6 +783,23 @@
 
 # icmp id != 233
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -733,6 +816,23 @@
 
 # icmp id 33-45
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -741,9 +841,12 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
-                "range": [ 33, 45 ]
+                "range": [
+                    33,
+                    45
+                ]
             }
         }
     }
@@ -751,6 +854,23 @@
 
 # icmp id != 33-45
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -761,7 +881,10 @@
             },
             "op": "!=",
             "right": {
-                "range": [ 33, 45 ]
+                "range": [
+                    33,
+                    45
+                ]
             }
         }
     }
@@ -769,6 +892,23 @@
 
 # icmp id { 33-55}
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -777,10 +917,15 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
@@ -789,6 +934,23 @@
 
 # icmp id != { 33-55}
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -800,7 +962,12 @@
             "op": "!=",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
@@ -809,6 +976,23 @@
 
 # icmp id { 22, 34, 333}
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -817,7 +1001,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
                     22,
@@ -831,6 +1015,23 @@
 
 # icmp id != { 22, 34, 333}
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -853,6 +1054,23 @@
 
 # icmp sequence 22
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -861,7 +1079,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": 22
         }
     }
@@ -869,6 +1087,23 @@
 
 # icmp sequence != 233
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -885,6 +1120,23 @@
 
 # icmp sequence 33-45
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -893,9 +1145,12 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
-                "range": [ 33, 45 ]
+                "range": [
+                    33,
+                    45
+                ]
             }
         }
     }
@@ -903,6 +1158,23 @@
 
 # icmp sequence != 33-45
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -913,7 +1185,10 @@
             },
             "op": "!=",
             "right": {
-                "range": [ 33, 45 ]
+                "range": [
+                    33,
+                    45
+                ]
             }
         }
     }
@@ -921,6 +1196,23 @@
 
 # icmp sequence { 33, 55, 67, 88}
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -929,7 +1221,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
                     33,
@@ -944,6 +1236,23 @@
 
 # icmp sequence != { 33, 55, 67, 88}
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -967,6 +1276,23 @@
 
 # icmp sequence { 33-55}
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -975,10 +1301,15 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
@@ -987,6 +1318,23 @@
 
 # icmp sequence != { 33-55}
 [
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
     {
         "match": {
             "left": {
@@ -998,10 +1346,105 @@
             "op": "!=",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
+                ]
+            }
+        }
+    }
+]
+
+# icmp id 1 icmp sequence 2
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": 1
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": 2
+        }
+    }
+]
+
+# icmp type { echo-reply, echo-request} icmp id 1 icmp sequence 2
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-reply",
+                    "echo-request"
                 ]
             }
         }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": 1
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "sequence",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": 2
+        }
     }
 ]
 
@@ -1015,7 +1458,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": 33
         }
     }
@@ -1031,9 +1474,12 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
-                "range": [ 22, 33 ]
+                "range": [
+                    22,
+                    33
+                ]
             }
         }
     }
@@ -1049,10 +1495,15 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    { "range": [ 22, 33 ] }
+                    {
+                        "range": [
+                            22,
+                            33
+                        ]
+                    }
                 ]
             }
         }
@@ -1072,7 +1523,12 @@
             "op": "!=",
             "right": {
                 "set": [
-                    { "range": [ 22, 33 ] }
+                    {
+                        "range": [
+                            22,
+                            33
+                        ]
+                    }
                 ]
             }
         }
@@ -1089,7 +1545,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": 22
         }
     }
@@ -1121,9 +1577,12 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
-                "range": [ 33, 45 ]
+                "range": [
+                    33,
+                    45
+                ]
             }
         }
     }
@@ -1141,7 +1600,10 @@
             },
             "op": "!=",
             "right": {
-                "range": [ 33, 45 ]
+                "range": [
+                    33,
+                    45
+                ]
             }
         }
     }
@@ -1157,7 +1619,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
                     33,
@@ -1203,10 +1665,15 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
@@ -1226,7 +1693,12 @@
             "op": "!=",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
@@ -1243,7 +1715,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": 22
         }
     }
@@ -1275,9 +1747,12 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
-                "range": [ 33, 45 ]
+                "range": [
+                    33,
+                    45
+                ]
             }
         }
     }
@@ -1295,7 +1770,10 @@
             },
             "op": "!=",
             "right": {
-                "range": [ 33, 45 ]
+                "range": [
+                    33,
+                    45
+                ]
             }
         }
     }
@@ -1311,7 +1789,7 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
                     33,
@@ -1357,10 +1835,15 @@
                     "protocol": "icmp"
                 }
             },
-	    "op": "==",
+            "op": "==",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
@@ -1380,7 +1863,12 @@
             "op": "!=",
             "right": {
                 "set": [
-                    { "range": [ 33, 55 ] }
+                    {
+                        "range": [
+                            33,
+                            55
+                        ]
+                    }
                 ]
             }
         }
-- 
2.26.2

Re: [PATCH 4/4] json: icmp: refresh json output

[Phil Sutter]

Hi,

On Thu, Jan 21, 2021 at 02:55:10PM +0100, Florian Westphal wrote:
> nft inserts dependencies for icmp header types, but I forgot to
> update the json test files to reflect this change.

For asymmetric JSON output, there are *.t.json.output files. Please add
the missing dependency expressions there.

In general, *.t.json files should contain JSON equivalents for rules as
they are *input* into nft. So we want them to be as close to the
introductory standard syntax comment as possible. This patch "breaks" a
few cases, e.g.:

[...]
> @@ -301,6 +301,8 @@
>                      "source-quench",
>                      "redirect",
>                      "echo-request",
> +                    "router-advertisement",
> +                    "router-solicitation",
>                      "time-exceeded",
>                      "parameter-problem",
>                      "timestamp-request",
> @@ -308,9 +310,7 @@
>                      "info-request",
>                      "info-reply",
>                      "address-mask-request",
> -                    "address-mask-reply",
> -                    "router-advertisement",
> -                    "router-solicitation"
> +                    "address-mask-reply"
>                  ]
>              }
>          }

Input is indeed sorted as prior to this patch. The reordered output is
found in icmp.t.json.output. The benefit from being picky here is minor,
but here's a better example:

[...]
> @@ -466,11 +482,11 @@
>                      "protocol": "icmp"
>                  }
>              },
> -	    "op": "==",
> +            "op": "==",
>              "right": {
>                  "set": [
> -                    2,
> -                    4,
> +                    "prot-unreachable",
> +                    "frag-needed",
>                      33,
>                      54,
>                      56

We test that icmp code values 2 and 4 are accepted. Standard syntax test
covers the asymmetric output containing the names. JSON should do the
same. OTOH, names as input is tested in the negated form of the same
test which follows this one.

Thanks, Phil