[pve-devel] [PATCH pve-firewall 19/21] add support for loading sdn firewall configuration

[Max Carrara]

On Wed Jun 26, 2024 at 2:15 PM CEST, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich <s.hanreich at proxmox.com>
> ---
>  src/PVE/Firewall.pm | 43 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 09544ba..95325a0 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -25,6 +25,7 @@ use PVE::Tools qw($IPV4RE $IPV6RE);
>  use PVE::Tools qw(run_command lock_file dir_glob_foreach);
>  
>  use PVE::Firewall::Helpers;
> +use PVE::RS::Firewall::SDN;
>  
>  my $pvefw_conf_dir = "/etc/pve/firewall";
>  my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
> @@ -3644,7 +3645,7 @@ sub lock_clusterfw_conf {
>  }
>  
>  sub load_clusterfw_conf {
> -    my ($filename) = @_;
> +    my ($filename, $load_sdn_config) = @_;

Small thing:

I would suggest using a prototype here and also accept a hash reference
OR a hash as the last parameter, so that the call signature is a little
more readable.

E.g. right now it's:

    load_clusterfw_conf(undef, 1)

VS:

    load_clusterfw_conf(undef, { load_sdn_config => 1 })

Or:

    load_clusterfw_conf(undef, load_sdn_config => 1)

I know we're gonna phase this whole thing out eventually, but little
things like this help a lot in the long run, IMO. It makes it a little
clearer what the subroutine does at call sites.

I'm not sure if these subroutines are used elsewhere (didn't really
bother to check, sorry), so perhaps you could pass `$filename` via the
hash as well, as an optional parameter. Then it's immediately clear what
*everything* stands for, because a sole `undef` "hides" what's actually
passed to the subroutine.

>  
>      $filename = $clusterfw_conf_filename if !defined($filename);
>      my $empty_conf = {
> @@ -3657,12 +3658,50 @@ sub load_clusterfw_conf {
>  	ipset_comments => {},
>      };
>  
> +    if ($load_sdn_config) {
> +	my $sdn_conf = load_sdn_conf();
> +	$empty_conf = { %$empty_conf, %$sdn_conf };
> +    }
> +
>      my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster');
>      $set_global_log_ratelimit->($cluster_conf->{options});
>  
>      return $cluster_conf;
>  }
>  
> +sub load_sdn_conf {
> +    my $rpcenv = PVE::RPCEnvironment::get();
> +    my $authuser = $rpcenv->get_user();
> +
> +    my $guests = PVE::Cluster::get_vmlist();
> +    my $allowed_vms = [];
> +    foreach my $vmid (sort keys %{$guests->{ids}}) {
> +	next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ], 1);
> +	push @$allowed_vms, $vmid;
> +    }
> +
> +    my $vnets = PVE::Network::SDN::Vnets::config(1);
> +    my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> +    my $allowed_vnets = [];
> +    foreach my $vnet (sort keys %{$vnets->{ids}}) {
> +	my $zone = $vnets->{ids}->{$vnet}->{zone};
> +	next if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$vnet", $privs, 1);
> +	push @$allowed_vnets, $vnet;
> +    }
> +
> +    my $sdn_config = {
> +	ipset => {} ,
> +	ipset_comments => {},
> +    };
> +
> +    eval {
> +	$sdn_config = PVE::RS::Firewall::SDN::config($allowed_vnets, $allowed_vms);
> +    };
> +    warn $@ if $@;
> +
> +    return $sdn_config;
> +}
> +
>  sub save_clusterfw_conf {
>      my ($cluster_conf) = @_;
>  
> @@ -4731,7 +4770,7 @@ sub init {
>  sub update {
>      my $code = sub {
>  
> -	my $cluster_conf = load_clusterfw_conf();
> +	my $cluster_conf = load_clusterfw_conf(undef, 1);
>  	my $hostfw_conf = load_hostfw_conf($cluster_conf);
>  
>  	if (!is_enabled_and_not_nftables($cluster_conf, $hostfw_conf)) {