[nft PATCH v2] parser_bison: Accept arbitrary user-defined names by quoting

[nft PATCH v2] parser_bison: Accept arbitrary user-defined names by quoting

[Phil Sutter]

Parser already allows to quote user-defined strings in some places to
avoid clashing with defined keywords, but not everywhere. Extend this
support further and add a test case for it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Fix testcase, I forgot to commit adjustments done to it.

Note: This is a reduced variant of "src: Quote user-defined names" sent
      back in January. Discussion was not conclusive regarding whether
      to quote these names on output or not, but I assume allowing for
      users to specify them by adding quotes is a step forward without
      drawbacks.
---
 src/parser_bison.y                            |  3 ++-
 .../shell/testcases/nft-f/0018quoted-names_0  | 20 +++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100755 tests/shell/testcases/nft-f/0018quoted-names_0

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 670e91f544c75..de8b097a4c222 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -1761,7 +1761,7 @@ flowtable_list_expr	:	flowtable_expr_member
 			|	flowtable_list_expr	COMMA	opt_newline
 			;
 
-flowtable_expr_member	:	STRING
+flowtable_expr_member	:	string
 			{
 				$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
 						       current_scope(state),
@@ -1968,6 +1968,7 @@ chain_policy		:	ACCEPT		{ $$ = NF_ACCEPT; }
 			;
 
 identifier		:	STRING
+			|	QUOTED_STRING
 			;
 
 string			:	STRING
diff --git a/tests/shell/testcases/nft-f/0018quoted-names_0 b/tests/shell/testcases/nft-f/0018quoted-names_0
new file mode 100755
index 0000000000000..6526d66b8e8a1
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0018quoted-names_0
@@ -0,0 +1,20 @@
+#!/bin/bash
+
+# Test if keywords are allowed as names if quoted
+
+set -e
+
+RULESET='
+table inet "day" {
+	chain "minute" {}
+	set "hour" { type inet_service; }
+	flowtable "second" { hook ingress priority 0; devices = { "lo" }; }
+	counter "table" { packets 0 bytes 0 }
+	quota "chain" { 10 bytes }
+}'
+
+$NFT -f - <<< "$RULESET"
+
+# XXX: not possible (yet)
+#OUTPUT=$($NFT list ruleset)
+#$NFT -f - <<< "$OUTPUT"
-- 
2.21.0

Re: [nft PATCH v2] parser_bison: Accept arbitrary user-defined names by quoting

[Pablo Neira Ayuso]

On Mon, Jun 24, 2019 at 06:36:08PM +0200, Phil Sutter wrote:
> Parser already allows to quote user-defined strings in some places to
> avoid clashing with defined keywords, but not everywhere. Extend this
> support further and add a test case for it.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Fix testcase, I forgot to commit adjustments done to it.
> 
> Note: This is a reduced variant of "src: Quote user-defined names" sent
>       back in January. Discussion was not conclusive regarding whether
>       to quote these names on output or not, but I assume allowing for
>       users to specify them by adding quotes is a step forward without
>       drawbacks.

So this will fail later on, right?

        nft list ruleset > file.nft
        nft -f file.nft

> ---
>  src/parser_bison.y                            |  3 ++-
>  .../shell/testcases/nft-f/0018quoted-names_0  | 20 +++++++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>  create mode 100755 tests/shell/testcases/nft-f/0018quoted-names_0
> 
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index 670e91f544c75..de8b097a4c222 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -1761,7 +1761,7 @@ flowtable_list_expr	:	flowtable_expr_member
>  			|	flowtable_list_expr	COMMA	opt_newline
>  			;
>  
> -flowtable_expr_member	:	STRING
> +flowtable_expr_member	:	string
>  			{
>  				$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
>  						       current_scope(state),
> @@ -1968,6 +1968,7 @@ chain_policy		:	ACCEPT		{ $$ = NF_ACCEPT; }
>  			;
>  
>  identifier		:	STRING
> +			|	QUOTED_STRING
>  			;
>  
>  string			:	STRING
> diff --git a/tests/shell/testcases/nft-f/0018quoted-names_0 b/tests/shell/testcases/nft-f/0018quoted-names_0
> new file mode 100755
> index 0000000000000..6526d66b8e8a1
> --- /dev/null
> +++ b/tests/shell/testcases/nft-f/0018quoted-names_0
> @@ -0,0 +1,20 @@
> +#!/bin/bash
> +
> +# Test if keywords are allowed as names if quoted
> +
> +set -e
> +
> +RULESET='
> +table inet "day" {
> +	chain "minute" {}
> +	set "hour" { type inet_service; }
> +	flowtable "second" { hook ingress priority 0; devices = { "lo" }; }
> +	counter "table" { packets 0 bytes 0 }
> +	quota "chain" { 10 bytes }
> +}'
> +
> +$NFT -f - <<< "$RULESET"
> +
> +# XXX: not possible (yet)
> +#OUTPUT=$($NFT list ruleset)
> +#$NFT -f - <<< "$OUTPUT"
> -- 
> 2.21.0
> 

Re: [nft PATCH v2] parser_bison: Accept arbitrary user-defined names by quoting

[Phil Sutter]

Hi Pablo,

On Fri, Jun 28, 2019 at 08:00:51PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jun 24, 2019 at 06:36:08PM +0200, Phil Sutter wrote:
> > Parser already allows to quote user-defined strings in some places to
> > avoid clashing with defined keywords, but not everywhere. Extend this
> > support further and add a test case for it.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Changes since v1:
> > - Fix testcase, I forgot to commit adjustments done to it.
> > 
> > Note: This is a reduced variant of "src: Quote user-defined names" sent
> >       back in January. Discussion was not conclusive regarding whether
> >       to quote these names on output or not, but I assume allowing for
> >       users to specify them by adding quotes is a step forward without
> >       drawbacks.
> 
> So this will fail later on, right?
> 
>         nft list ruleset > file.nft
>         nft -f file.nft

Yes, that's right. I sent a complete version which does the necessary
quoting on output in January[1], but discussion wasn't conclusive. You
had a different approach which accepts the quotes as part of the name
but you weren't happy with it, either. I *think* you wanted to search
for ways to solve this from within bison but we never got back to it
anymore.

This simplified patch is merely trying to make things consistent
regarding user-defined names. IIRC, I can already have an interface
named "month", use that in a netdev family chain declaration (quoted)
and 'nft list ruleset' will print it unquoted, so it can't be applied
anymore. Without my patch, it is simply impossible to use certain
recognized keywords as names for tables, chains, etc., even if one
accepted the implications it has.

Cheers, Phil

[1] Message-Id: <20190116184613.31698-1-phil@nwl.cc>

Re: [nft PATCH v2] parser_bison: Accept arbitrary user-defined names by quoting

[Pablo Neira Ayuso]

On Mon, Jul 01, 2019 at 06:11:39PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Fri, Jun 28, 2019 at 08:00:51PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Jun 24, 2019 at 06:36:08PM +0200, Phil Sutter wrote:
> > > Parser already allows to quote user-defined strings in some places to
> > > avoid clashing with defined keywords, but not everywhere. Extend this
> > > support further and add a test case for it.
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > > Changes since v1:
> > > - Fix testcase, I forgot to commit adjustments done to it.
> > > 
> > > Note: This is a reduced variant of "src: Quote user-defined names" sent
> > >       back in January. Discussion was not conclusive regarding whether
> > >       to quote these names on output or not, but I assume allowing for
> > >       users to specify them by adding quotes is a step forward without
> > >       drawbacks.
> > 
> > So this will fail later on, right?
> > 
> >         nft list ruleset > file.nft
> >         nft -f file.nft
> 
> Yes, that's right. I sent a complete version which does the necessary
> quoting on output in January[1], but discussion wasn't conclusive. You
> had a different approach which accepts the quotes as part of the name
> but you weren't happy with it, either. I *think* you wanted to search
> for ways to solve this from within bison but we never got back to it
> anymore.
> 
> This simplified patch is merely trying to make things consistent
> regarding user-defined names. IIRC, I can already have an interface
> named "month", use that in a netdev family chain declaration (quoted)
> and 'nft list ruleset' will print it unquoted, so it can't be applied
> anymore. Without my patch, it is simply impossible to use certain
> recognized keywords as names for tables, chains, etc., even if one
> accepted the implications it has.

I'm not arguing there's something to fix.

I'm telling this is still incomplete.

Would you allocate a bit of time to discuss this during the NFWS?

Re: [nft PATCH v2] parser_bison: Accept arbitrary user-defined names by quoting

[Pablo Neira Ayuso]

On Mon, Jul 01, 2019 at 08:13:41PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jul 01, 2019 at 06:11:39PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Fri, Jun 28, 2019 at 08:00:51PM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Jun 24, 2019 at 06:36:08PM +0200, Phil Sutter wrote:
> > > > Parser already allows to quote user-defined strings in some places to
> > > > avoid clashing with defined keywords, but not everywhere. Extend this
> > > > support further and add a test case for it.
> > > > 
> > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > ---
> > > > Changes since v1:
> > > > - Fix testcase, I forgot to commit adjustments done to it.
> > > > 
> > > > Note: This is a reduced variant of "src: Quote user-defined names" sent
> > > >       back in January. Discussion was not conclusive regarding whether
> > > >       to quote these names on output or not, but I assume allowing for
> > > >       users to specify them by adding quotes is a step forward without
> > > >       drawbacks.
> > > 
> > > So this will fail later on, right?
> > > 
> > >         nft list ruleset > file.nft
> > >         nft -f file.nft
> > 
> > Yes, that's right. I sent a complete version which does the necessary
> > quoting on output in January[1], but discussion wasn't conclusive. You
> > had a different approach which accepts the quotes as part of the name
> > but you weren't happy with it, either. I *think* you wanted to search
> > for ways to solve this from within bison but we never got back to it
> > anymore.
> > 
> > This simplified patch is merely trying to make things consistent
> > regarding user-defined names. IIRC, I can already have an interface
> > named "month", use that in a netdev family chain declaration (quoted)
> > and 'nft list ruleset' will print it unquoted, so it can't be applied
> > anymore. Without my patch, it is simply impossible to use certain
> > recognized keywords as names for tables, chains, etc., even if one
> > accepted the implications it has.
> 
> I'm not arguing there's something to fix.
> 
> I'm telling this is still incomplete.
> 
> Would you allocate a bit of time to discuss this during the NFWS?

I mean, a quick summary of the different options for a complete
solution for this, and we decide there.

Unless you tell me this is very urgent :-)

Re: [nft PATCH v2] parser_bison: Accept arbitrary user-defined names by quoting

[Phil Sutter]

Hi Pablo,

On Mon, Jul 01, 2019 at 08:19:24PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jul 01, 2019 at 08:13:41PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Jul 01, 2019 at 06:11:39PM +0200, Phil Sutter wrote:
> > > Hi Pablo,
> > > 
> > > On Fri, Jun 28, 2019 at 08:00:51PM +0200, Pablo Neira Ayuso wrote:
> > > > On Mon, Jun 24, 2019 at 06:36:08PM +0200, Phil Sutter wrote:
> > > > > Parser already allows to quote user-defined strings in some places to
> > > > > avoid clashing with defined keywords, but not everywhere. Extend this
> > > > > support further and add a test case for it.
> > > > > 
> > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > ---
> > > > > Changes since v1:
> > > > > - Fix testcase, I forgot to commit adjustments done to it.
> > > > > 
> > > > > Note: This is a reduced variant of "src: Quote user-defined names" sent
> > > > >       back in January. Discussion was not conclusive regarding whether
> > > > >       to quote these names on output or not, but I assume allowing for
> > > > >       users to specify them by adding quotes is a step forward without
> > > > >       drawbacks.
> > > > 
> > > > So this will fail later on, right?
> > > > 
> > > >         nft list ruleset > file.nft
> > > >         nft -f file.nft
> > > 
> > > Yes, that's right. I sent a complete version which does the necessary
> > > quoting on output in January[1], but discussion wasn't conclusive. You
> > > had a different approach which accepts the quotes as part of the name
> > > but you weren't happy with it, either. I *think* you wanted to search
> > > for ways to solve this from within bison but we never got back to it
> > > anymore.
> > > 
> > > This simplified patch is merely trying to make things consistent
> > > regarding user-defined names. IIRC, I can already have an interface
> > > named "month", use that in a netdev family chain declaration (quoted)
> > > and 'nft list ruleset' will print it unquoted, so it can't be applied
> > > anymore. Without my patch, it is simply impossible to use certain
> > > recognized keywords as names for tables, chains, etc., even if one
> > > accepted the implications it has.
> > 
> > I'm not arguing there's something to fix.
> > 
> > I'm telling this is still incomplete.
> > 
> > Would you allocate a bit of time to discuss this during the NFWS?
> 
> I mean, a quick summary of the different options for a complete
> solution for this, and we decide there.
> 
> Unless you tell me this is very urgent :-)

That's a great idea! I'll check my inbox for more of those unfinished
topics. :)

Thanks, Phil