Hi, > [...] > > Sorry, but I don't agree with the following reasons. Let me briefly explain > why. > > All files under '/usr/share/firewall4/includes.d' are uci files. I can see > all relevant options.
One problem with your suggested implementation is that you introduce a generic directory (/usr/share/firewall4/includes.d) containing uci partials that do look like a full /etc/config/firewall but actually ignore all section types except `config include` ones, that will lead to confusion. > I can set in the includes my own path. This is > relevant for packages that updates the ruleset on events. If I do not want > to be this rules persistent saved I could use a tmp file in the system > (strongswan). Using my proposal you could achieve the same by placing a symlink to a temporary file path > Also the new include add by you already does have the state file feature. > Which is relevant on reloading the ruleset. Not sure what you mean with that. > In addition, it would make the ruleset.uc file confusing if I added the > same feature again. The ruleset.uc file wouldn't change at all. The implementation would be confined to fw4.uc, like your approach. > Also, I would have to add two sections on include to support temporary > rules, which I would not have to store under > '/usr/share/firewall4/includes.d/' but under '/tmp/firewall4/includes.d/' > for example to support the not persistent feature. ln -s /tmp/strongswan/nftables.d/pre-input.nft \ /usr/share/nftables.d/chain-pre/input/10_strongswan.nft ln -s /tmp/strongswan/nftables.d/pre-output.nft \ /usr/share/nftables.d/chain-pre/output/10_strongswan.nft ln -s /tmp/strongswan/nftables.d/pre-forward.nft \ /usr/share/nftables.d/chain-pre/forward/10_strongswan.nft > We also use the include to add the helpers. Can you elaborate? > Last but not leased, if we now add an other possibility, then I don't > think anyone knows where which file adds which rule! The same applies to /usr/share/firewall4/includes.d/ as well. You do need to be aware of it to know that includes can stem from there. > From my point of view, it makes sense to use the include function from you > with my extension. So I think the include feature is the better and > unified solution. What I do not like about it are several facts: - It introduces uci in a non-standard location - It implies configurability where there isn't any (it is package managed resource files, like init scripts. Technically you can edit init scripts but they're certainly not configuration but package integration code). - It introduces overhead; you do have to read and parse a uci file to extract a path from it which you then write into the ruleset so that nftables can include it - It creates the false impression of being a general-purpose uci partial solution to be able to modularize /etc/config/firewall which it isn't since it'll silently ignore any non-include section type being put there I have attached my proposal as patch for reference. Kind regards, Jo
From 5ab0f61350f02590c5e6c1981bce4531510517de Mon Sep 17 00:00:00 2001 From: Jo-Philipp Wich <j...@mein.io> Date: Thu, 11 Aug 2022 13:48:14 +0200 Subject: [PATCH] fw4: support automatic includes Introduce a new directory tree /usr/share/nftables/includes.d/ which may contain partial nftables files being included into the rendered ruleset. The include position is derived from the file path; - Files in .../includes.d/table-pre/ and .../includes.d/table-post/ are included before and after the `table inet fw4 { ... }` declaration respectively - Files in .../includes.d/ruleset-pre/ and .../includes.d/ruleset-post/ are included before the first chain and after the last chain declaration within the fw4 table respectively - Files in .../includes.d/chain-pre/${chain}/ and .../chain-post/${chain}/ are included before the first and after the last rule within the mentioned chain of the fw4 table respectively Automatic includes can be disabled by setting the `auto_includes` option to `0` in the global defaults section. Signed-off-by: Jo-Philipp Wich <j...@mein.io> --- root/usr/share/ucode/fw4.uc | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc index 2dc44ac..86e3331 100644 --- a/root/usr/share/ucode/fw4.uc +++ b/root/usr/share/ucode/fw4.uc @@ -733,6 +733,19 @@ return { this.cursor.foreach("firewall", "include", i => self.parse_include(i)); + // + // Discover automatic includes + // + + if (this.default_option("auto_includes")) { + for (let position in [ 'ruleset-pre', 'ruleset-post', 'table-pre', 'table-post', 'chain-pre', 'chain-post' ]) + for (let chain in (position in [ 'chain-pre', 'chain-post' ]) ? fs.lsdir(`/usr/share/nftables/includes.d/${position}`) : [ null ]) + for (let path in fs.glob(`/usr/share/nftables/includes.d/${position}/${chain ?? ''}/*.nft`)) + if (fs.access(path)) + this.parse_include({ type: 'nftables', position, chain, path }); + } + + if (use_statefile) { let fd = fs.open(STATEFILE, "w"); @@ -1876,7 +1889,9 @@ return { custom_chains: [ "bool", null, UNSUPPORTED ], disable_ipv6: [ "bool", null, UNSUPPORTED ], flow_offloading: [ "bool", "0" ], - flow_offloading_hw: [ "bool", "0" ] + flow_offloading_hw: [ "bool", "0" ], + + auto_includes: [ "bool", "1" ] }); if (defs.synflood_protect === null) -- 2.35.1
signature.asc
Description: OpenPGP digital signature
_______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel