[PATCH firewall4] fw4: add support for include.d dir

[Jo-Philipp Wich]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fw4-support-automatic-includes.patch
Type: text/x-patch
Size: 2440 bytes
Desc: not available
URL: <http://lists.openwrt.org/pipermail/openwrt-devel/attachments/20220811/d35d1fa1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openwrt.org/pipermail/openwrt-devel/attachments/20220811/d35d1fa1/attachment.sig>