From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Stefan Hanreich <s.hanreich@proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-firewall 10/15] api: add vnet endpoints
Date: Thu, 26 Sep 2024 08:37:43 +0200 [thread overview]
Message-ID: <a55cfa6e-b332-474a-81df-da982136a6cd@proxmox.com> (raw)
In-Reply-To: <20240911093116.112960-11-s.hanreich@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
next prev parent reply other threads:[~2024-09-26 6:37 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-11 9:31 [pve-devel] [RFC firewall/manager/network/proxmox{-ve-rs, -firewall} 00/15] add forward chain firewalling for hosts and bridges Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH proxmox-ve-rs 01/15] cargo: bump dependencies Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH proxmox-ve-rs 02/15] firewall: add forward direction Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH proxmox-ve-rs 03/15] firewall: add bridge firewall config parser Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH proxmox-ve-rs 04/15] host: add struct representing bridge names Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH proxmox-firewall 05/15] sdn: add support for loading vnet-level firewall config Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH proxmox-firewall 06/15] sdn: create forward firewall rules Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH proxmox-firewall 07/15] use std::mem::take over drain() Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH proxmox-firewall 08/15] cargo: bump dependencies Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH pve-firewall 09/15] sdn: add vnet firewall configuration Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH pve-firewall 10/15] api: add vnet endpoints Stefan Hanreich
2024-09-26 6:37 ` Thomas Lamprecht [this message]
2024-10-04 15:36 ` Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH pve-manager 11/15] firewall: add forward direction to rule panel Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH pve-manager 12/15] firewall: add vnet to firewall options component Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH pve-manager 13/15] firewall: make base_url dynamically configurable in " Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH pve-manager 14/15] sdn: add firewall panel Stefan Hanreich
2024-09-11 9:31 ` [pve-devel] [PATCH pve-network 15/15] firewall: add endpoints for vnet-level firewall Stefan Hanreich
2024-09-11 12:31 ` [pve-devel] [RFC firewall/manager/network/proxmox{-ve-rs, -firewall} 00/15] add forward chain firewalling for hosts and bridges Stefan Hanreich
2024-09-11 15:22 ` Gabriel Goller
2024-10-10 16:00 ` Stefan Hanreich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a55cfa6e-b332-474a-81df-da982136a6cd@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=s.hanreich@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox