Re: [pve-devel] [PATCH v4 firewall 1/2] implement fail2ban backend and API

[Oguz Bektas <o.bektas@proxmox.com>]

On Tue, Oct 19, 2021 at 03:43:49PM +0200, Dominik Csapak wrote:
> while the code looks ok IMHO, i have some general questions:
> * does it really make sense to hard depend on fail2ban?
>   could it not also make sense to have it as 'recommends' or 'suggests'?
>   setting enabled to 1 could then check if its installed and
>   raise an error
> 
> * if we do not plan to add more fail2ban options in our config,
>   i would rather see a combined fail2ban option (propertystring?)
>   that would go into the general host firewall options
> 
>   that way we would not have to c&p the whole config parsing/setting api
>   and could have a single new option line in the gui instead
>   of a whole new panel with only 3 options (i think the majority of our
>   users will not use fail2ban)

> 
> does that make sense to you?
> 

well if we hide it like that inside the menu, then surely nobody will
use it ;)

separate panel makes it more visible IMO. it shouldn't be hidden 3
layers deep in the options menu (Host -> Firewall -> Options -> Fail2ban
-> Enable) for such a simple feature, i think a lot more people would
use it if it's placed in a visible location (Host -> Fail2ban ->
Enable). if you really insist on putting it in the firewall options menu
then i'll have to insist for it to be installed & enabled by default :)

i didn't see any good reason to not have it installed and enabled with
sane defaults since it doesn't hurt anything IMHO... that's why i went
with hard dependency instead of 'recommends' or 'suggests'.

also i wanted to have a separate API endpoint which is why i didnt go for
adding it into the options section.
though we can still make it take a property string as option i suppose.

oguz