Re: [pve-devel] [PATCH pve-firewall 10/15] api: add vnet endpoints

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 11/09/2024 um 11:31 schrieb Stefan Hanreich:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  src/PVE/API2/Firewall/Makefile |   1 +
>  src/PVE/API2/Firewall/Rules.pm |  71 ++++++++++++++
>  src/PVE/API2/Firewall/Vnet.pm  | 166 +++++++++++++++++++++++++++++++++
>  src/PVE/Firewall.pm            |  10 ++
>  4 files changed, 248 insertions(+)
>  create mode 100644 src/PVE/API2/Firewall/Vnet.pm
> 
> diff --git a/src/PVE/API2/Firewall/Makefile b/src/PVE/API2/Firewall/Makefile
> index e916755..325c4d3 100644
> --- a/src/PVE/API2/Firewall/Makefile
> +++ b/src/PVE/API2/Firewall/Makefile
> @@ -9,6 +9,7 @@ LIB_SOURCES=			\
>  	Cluster.pm		\
>  	Host.pm			\
>  	VM.pm			\
> +	Vnet.pm			\
>  	Groups.pm
>  
>  all:
> diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
> index ebb51af..906c6d7 100644
> --- a/src/PVE/API2/Firewall/Rules.pm
> +++ b/src/PVE/API2/Firewall/Rules.pm
> @@ -18,6 +18,10 @@ my $api_properties = {
>      },
>  };
>  
> +sub before_method {
> +    my ($class, $method_name, $param) = @_;

a short comment here stating explicitly that/why this is a no-op implementation
by design might be good.

> +}
> +
>  sub lock_config {
>      my ($class, $param, $code) = @_;
>  
> @@ -93,6 +97,7 @@ sub register_get_rules {
>  	},
>  	code => sub {
>  	    my ($param) = @_;

please keep the empty line after above's method param extraction one.

> +	    $class->before_method('get_rules', $param);
>  
>  	    my ($cluster_conf, $fw_conf, $rules) = $class->load_config($param);
>  
> @@ -191,6 +196,7 @@ sub register_get_rule {
>  	},
>  	code => sub {
>  	    my ($param) = @_;

same as above

> +	    $class->before_method('get_rule', $param);
>  
>  	    my ($cluster_conf, $fw_conf, $rules) = $class->load_config($param);
>  
> @@ -231,6 +237,7 @@ sub register_create_rule {
>  	returns => { type => "null" },
>  	code => sub {
>  	    my ($param) = @_;
> +	    $class->before_method('create_rule', $param);

same as above

>  
>  	    $class->lock_config($param, sub {
>  		my ($param) = @_;
> @@ -292,6 +299,7 @@ sub register_update_rule {
>  	returns => { type => "null" },
>  	code => sub {
>  	    my ($param) = @_;

same as above

> +	    $class->before_method('update_rule', $param);
>  
>  	    $class->lock_config($param, sub {
>  		my ($param) = @_;
> @@ -358,6 +366,7 @@ sub register_delete_rule {
>  	returns => { type => "null" },
>  	code => sub {
>  	    my ($param) = @_;

same as above

> +	    $class->before_method('delete_rule', $param);
>  
>  	    $class->lock_config($param, sub {
>  		my ($param) = @_;
> @@ -636,4 +645,66 @@ sub save_rules {
>  
>  __PACKAGE__->register_handlers();
>  
> +package PVE::API2::Firewall::VnetRules;
> +
> +use strict;
> +use warnings;
> +use PVE::JSONSchema qw(get_standard_option);
> +
> +use base qw(PVE::API2::Firewall::RulesBase);
> +
> +__PACKAGE__->additional_parameters({
> +    vnet => get_standard_option('pve-sdn-vnet-id'),
> +});
> +
> +sub before_method {

I'd prefer a _hook suffix for such a method for slightly added clarity.

And FWIW, if all you do now is check privileges, and there's nothing you already
know that's gonna get added here soon, you could just name it after what it does
and avoid being all to generic, i.e. something like

sub assert_privs_for_method

> +    my ($class, $method_name, $param) = @_;
> +
> +    my $privs;

this is a "one of those privs", not "all of those privs" thing FWICT, maybe encode
that also in the name to slightly reduce potential for introducing errors (as in:
unlikely, but too cheap to not do it), like e.g.:

my $requires_one_of_privs:


Alternatively, avoid the variable and call check_vnet_access directly, but no hard
feelings.

> +
> +    if ($method_name eq 'get_rule' || $method_name eq 'get_rules') {
> +	$privs = ['SDN.Audit', 'SDN.Allocate'];
> +    } elsif ($method_name eq 'update_rule'
> +	|| $method_name eq 'create_rule'
> +	|| $method_name eq 'delete_rule'

It's certainly not always better but IMO a regex match would improve readability
slightly, e.g.:

    } elsif ($method_name =~ /^(create|delete|update)_rule$/) {

> +    ) {
> +	$privs = ['SDN.Allocate'];
> +    } else {
> +	die "unknown method: $method_name";
> +    }
> +
> +    PVE::API2::Firewall::Vnet::check_vnet_access($param->{vnet}, $privs);
> +}
> +
> +sub rule_env {
> +    my ($class, $param) = @_;
> +
> +    return 'vnet';
> +}
> +
> +sub lock_config {
> +    my ($class, $param, $code) = @_;
> +
> +    PVE::Firewall::lock_vnetfw_conf($param->{vnet}, 10, $code, $param);
> +}
> +
> +sub load_config {
> +    my ($class, $param) = @_;
> +
> +    my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef, 1);
> +    my $fw_conf = PVE::Firewall::load_vnetfw_conf($cluster_conf, 'vnet', $param->{vnet});
> +    my $rules = $fw_conf->{rules};
> +
> +    return ($cluster_conf, $fw_conf, $rules);
> +}
> +
> +sub save_rules {
> +    my ($class, $param, $fw_conf, $rules) = @_;
> +
> +    $fw_conf->{rules} = $rules;
> +    PVE::Firewall::save_vnetfw_conf($param->{vnet}, $fw_conf);
> +}
> +
> +__PACKAGE__->register_handlers();
> +
>  1;

> +sub check_vnet_access {
> +    my ($vnetid, $privileges) = @_;
> +
> +    my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid, 1);
> +    die "invalid vnet specified" if !$vnet;

nit: could be combined

    my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid, 1) or die "invalid vnet '$vnetid'\n";

for sake of completeness: what's not ok is a conditional definition like e.g:

  my $foo = 'bar' if baz();

As that keeps the value of the last call if the if evaluates to false.
But, as long as the definition itself is not conditional it's fine.

> +
> +    my $zoneid = $vnet->{zone};
> +
> +    my $rpcenv = PVE::RPCEnvironment::get();
> +    my $authuser = $rpcenv->get_user();
> +
> +    $rpcenv->check_any($authuser, "/sdn/zones/$zoneid/$vnetid", $privileges);
> +};

> +    }});
> +
> +my $option_properties = $PVE::Firewall::vnet_option_properties;

might need a clone to avoid modifying the original reference I think

> +
> +my $add_option_properties = sub {
> +    my ($properties) = @_;
> +
> +    foreach my $k (keys %$option_properties) {
> +	$properties->{$k} = $option_properties->{$k};
> +    }
> +
> +    return $properties;
> +};
> +
> +

> +__PACKAGE__->register_method({
> +    name => 'set_options',
> +    path => 'options',
> +    method => 'PUT',
> +    description => "Set Firewall options.",
> +    protected => 1,
> +    permissions => {
> +	description => "Needs SDN.Allocate permissions on '/sdn/zones/<zone>/<vnet>'",

Hmm, I'm wondering might it make sense to add a SDN.Firewall privilege to separate
allowing VNet allocation and allowing to configure a VNet's firewall?
While adding privs is a bit tricky, this one might be dooable later one too though.

But whatever gets chosen should then be also addressed in a commit message with some
background reasoning (if it's already then I might have overlooked, I did not checked
every all too closely yet).

> +	user => 'all',
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => &$add_option_properties({

nit: probably stems from copying this from existing code, but please prefer modern
$code_ref->() calling style, i.e.:

properties => $add_option_properties->({

alternatively the $add_option_properties could be replaced with a locally scoped sub:

my sub add_option_properties {
    ...

> +	    vnet => get_standard_option('pve-sdn-vnet-id'),
> +	    delete => {
> +		type => 'string', format => 'pve-configid-list',
> +		description => "A list of settings you want to delete.",
> +		optional => 1,
> +	    },
> +	    digest => get_standard_option('pve-config-digest'),
> +	}),
> +    },
> +    returns => { type => "null" },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	check_vnet_access($param->{vnet}, ['SDN.Allocate']);
> +
> +	PVE::Firewall::lock_vnetfw_conf($param->{vnet}, 10, sub {
> +	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
> +	    my $vnetfw_conf = PVE::Firewall::load_vnetfw_conf($cluster_conf, 'vnet', $param->{vnet});
> +
> +	    my (undef, $digest) = PVE::Firewall::copy_opject_with_digest($vnetfw_conf->{options});
> +	    PVE::Tools::assert_if_modified($digest, $param->{digest});
> +
> +	    if ($param->{delete}) {
> +		foreach my $opt (PVE::Tools::split_list($param->{delete})) {

nit: we prefer for over foreach for new code:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices

> +		    raise_param_exc({ delete => "no such option '$opt'" })
> +			if !$option_properties->{$opt};
> +		    delete $vnetfw_conf->{options}->{$opt};
> +		}
> +	    }
> +
> +	    if (defined($param->{enable})) {
> +		$param->{enable} = $param->{enable} ? 1 : 0;
> +	    }
> +
> +	    foreach my $k (keys %$option_properties) {

same nit w.r.t. foreach as above

> +		next if !defined($param->{$k});
> +		$vnetfw_conf->{options}->{$k} = $param->{$k};
> +	    }
> +
> +	    PVE::Firewall::save_vnetfw_conf($param->{vnet}, $vnetfw_conf);
> +	});
> +
> +	return undef;
> +    }});
> +
> +1;




_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel