Re: [pve-devel] [PATCH pve-firewall 20/21] api: load sdn ipsets

[Stefan Hanreich <s.hanreich@proxmox.com>]

Seems like I regenerated the patches once, after writing a comment so
I'll leave it here:

This is certainly the minimally invasive way to go about this, but it
has the downside of having to load the cluster configuration twice. Once
for validating all rules properly and once for providing the methods
with a cluster config dictionary that doesn't contain the SDN ipsets, so
it can be saved. This didn't pose too much of an issue in my tests, the
API calls were still quite fast.

Passing the configurations from load_conf certainly is a bit hacky,
since we're passing almost the same config twice - but works quite well
for this use case. We only have to do this for the cluster
configuration, since this is the only place where the cluster
configuration gets saved.

On 6/26/24 14:15, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  src/PVE/API2/Firewall/Cluster.pm |  3 ++-
>  src/PVE/API2/Firewall/Rules.pm   | 18 +++++++++++-------
>  src/PVE/API2/Firewall/VM.pm      |  3 ++-
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/API2/Firewall/Cluster.pm b/src/PVE/API2/Firewall/Cluster.pm
> index 48ad90d..3f48431 100644
> --- a/src/PVE/API2/Firewall/Cluster.pm
> +++ b/src/PVE/API2/Firewall/Cluster.pm
> @@ -214,6 +214,7 @@ __PACKAGE__->register_method({
>      permissions => {
>  	check => ['perm', '/', [ 'Sys.Audit' ]],
>      },
> +    protected => 1,
>      parameters => {
>  	additionalProperties => 0,
>  	properties => {
> @@ -253,7 +254,7 @@ __PACKAGE__->register_method({
>      code => sub {
>  	my ($param) = @_;
>  
> -	my $conf = PVE::Firewall::load_clusterfw_conf();
> +	my $conf = PVE::Firewall::load_clusterfw_conf(undef, 1);
>  
>  	return PVE::Firewall::Helpers::collect_refs($conf, $param->{type}, "dc");
>      }});
> diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
> index 9fcfb20..ebb51af 100644
> --- a/src/PVE/API2/Firewall/Rules.pm
> +++ b/src/PVE/API2/Firewall/Rules.pm
> @@ -72,6 +72,7 @@ sub register_get_rules {
>  	path => '',
>  	method => 'GET',
>  	description => "List rules.",
> +	protected => 1,
>  	permissions => PVE::Firewall::rules_audit_permissions($rule_env),
>  	parameters => {
>  	    additionalProperties => 0,
> @@ -120,6 +121,7 @@ sub register_get_rule {
>  	path => '{pos}',
>  	method => 'GET',
>  	description => "Get single rule data.",
> +	protected => 1,
>  	permissions => PVE::Firewall::rules_audit_permissions($rule_env),
>  	parameters => {
>  	    additionalProperties => 0,
> @@ -412,11 +414,12 @@ sub lock_config {
>  sub load_config {
>      my ($class, $param) = @_;
>  
> +    my $sdn_conf = PVE::Firewall::load_clusterfw_conf(undef, 1);
>      my $fw_conf = PVE::Firewall::load_clusterfw_conf();
> -    my $rules = $fw_conf->{groups}->{$param->{group}};
> +    my $rules = $sdn_conf->{groups}->{$param->{group}};
>      die "no such security group '$param->{group}'\n" if !defined($rules);
>  
> -    return (undef, $fw_conf, $rules);
> +    return ($sdn_conf, $fw_conf, $rules);
>  }
>  
>  sub save_rules {
> @@ -488,10 +491,11 @@ sub lock_config {
>  sub load_config {
>      my ($class, $param) = @_;
>  
> +    my $sdn_conf = PVE::Firewall::load_clusterfw_conf(undef, 1);
>      my $fw_conf = PVE::Firewall::load_clusterfw_conf();
> -    my $rules = $fw_conf->{rules};
> +    my $rules = $sdn_conf->{rules};
>  
> -    return (undef, $fw_conf, $rules);
> +    return ($sdn_conf, $fw_conf, $rules);
>  }
>  
>  sub save_rules {
> @@ -528,7 +532,7 @@ sub lock_config {
>  sub load_config {
>      my ($class, $param) = @_;
>  
> -    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
> +    my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef, 1);
>      my $fw_conf = PVE::Firewall::load_hostfw_conf($cluster_conf);
>      my $rules = $fw_conf->{rules};
>  
> @@ -572,7 +576,7 @@ sub lock_config {
>  sub load_config {
>      my ($class, $param) = @_;
>  
> -    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
> +    my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef, 1);
>      my $fw_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, 'vm', $param->{vmid});
>      my $rules = $fw_conf->{rules};
>  
> @@ -616,7 +620,7 @@ sub lock_config {
>  sub load_config {
>      my ($class, $param) = @_;
>  
> -    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
> +    my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef, 1);
>      my $fw_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, 'ct', $param->{vmid});
>      my $rules = $fw_conf->{rules};
>  
> diff --git a/src/PVE/API2/Firewall/VM.pm b/src/PVE/API2/Firewall/VM.pm
> index 4222103..9800d8c 100644
> --- a/src/PVE/API2/Firewall/VM.pm
> +++ b/src/PVE/API2/Firewall/VM.pm
> @@ -234,6 +234,7 @@ sub register_handlers {
>  	path => 'refs',
>  	method => 'GET',
>  	description => "Lists possible IPSet/Alias reference which are allowed in source/dest properties.",
> +	protected => 1,
>  	permissions => {
>  	    check => ['perm', '/vms/{vmid}', [ 'VM.Audit' ]],
>  	},
> @@ -278,7 +279,7 @@ sub register_handlers {
>  	code => sub {
>  	    my ($param) = @_;
>  
> -	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
> +	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef, 1);
>  	    my $fw_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, $rule_env, $param->{vmid});
>  
>  	    my $dc_refs = PVE::Firewall::Helpers::collect_refs($cluster_conf, $param->{type}, 'dc');


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