On Wed Jun 26, 2024 at 2:15 PM CEST, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich <s.hanre...@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)) {
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
