[pve-devel] [PATCH firewall/network 0/2] SDN: Create firewall aliases for SDN subnets

[pve-devel] [PATCH firewall/network 0/2] SDN: Create firewall aliases for SDN subnets

[Stefan Lendl]

Creates a cluster-way firewall alias when creating an SDN subnet

Including Firewall from pve-network introduces a dependency cycle which the
patch to Firewall.pm elimiates.



pve-firewall:

Stefan Lendl (1):
  Manually construct guest config path

 src/PVE/Firewall.pm | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)


pve-network:

Stefan Lendl (1):
  Create a cluster-wide firewall for SDN subnets

 src/PVE/Network/SDN/Subnets.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)


Summary over all repositories:
  2 files changed, 25 insertions(+), 27 deletions(-)

-- 
murpp v0.4.0

[pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path

[Stefan Lendl]

Remove require QemuConfig from Firewall.pm
We only use it to construct the guest config paths.
Fixes circular include when accessing Firewall::Aliases from
pve-network.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Firewall.pm | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 77cbaf4..4db4b0c 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -28,22 +28,6 @@ use PVE::Firewall::Helpers;
 
 my $pvefw_conf_dir = "/etc/pve/firewall";
 my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
-
-# dynamically include PVE::QemuServer and PVE::LXC
-# to avoid dependency problems
-my $have_qemu_server;
-eval {
-    require PVE::QemuServer;
-    require PVE::QemuConfig;
-    $have_qemu_server = 1;
-};
-
-my $have_lxc;
-eval {
-    require PVE::LXC;
-    $have_lxc = 1;
-};
-
 my $pve_fw_status_dir = "/var/lib/pve-firewall";
 
 mkdir $pve_fw_status_dir; # make sure this exists
@@ -3257,18 +3241,14 @@ sub read_local_vm_config {
 	next if !$d->{node} || $d->{node} ne $nodename;
 	next if !$d->{type};
 	if ($d->{type} eq 'qemu') {
-	    if ($have_qemu_server) {
-		my $cfspath = PVE::QemuConfig->cfs_config_path($vmid);
-		if (my $conf = PVE::Cluster::cfs_read_file($cfspath)) {
-		    $qemu->{$vmid} = $conf;
-		}
+	    my $cfspath = "nodes/$nodename/qemu-server/$vmid.conf";
+	    if (my $conf = PVE::Cluster::cfs_read_file($cfspath)) {
+		$qemu->{$vmid} = $conf;
 	    }
-        } elsif ($d->{type} eq 'lxc') {
-	    if ($have_lxc) {
-		my $cfspath = PVE::LXC::Config->cfs_config_path($vmid);
-		if (my $conf = PVE::Cluster::cfs_read_file($cfspath)) {
-		    $lxc->{$vmid} = $conf;
-		}
+	} elsif ($d->{type} eq 'lxc') {
+	    my $cfspath = "nodes/$nodename/lxc/$vmid.conf";
+	    if (my $conf = PVE::Cluster::cfs_read_file($cfspath)) {
+		$lxc->{$vmid} = $conf;
 	    }
 	}
     }
-- 
2.41.0

Re: [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path

[Thomas Lamprecht]

Am 08/11/2023 um 12:35 schrieb Stefan Lendl:
> Remove require QemuConfig from Firewall.pm
> We only use it to construct the guest config paths.
> Fixes circular include when accessing Firewall::Aliases from
> pve-network.
> 

This won't work as now cfs_read_file only works by luck, if at all, as the
cfs_read_file needs the cfs_register_file that happens in PVE::QemuServer
so that the parser is actually available...

I'd much rather see Firewall be split-up than doing broken hacks and
switching from one of our saner interfaces to manual assembly.

Re: [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path

[Stefan Lendl]

The issue arises because firewall depends on qemu-server but qemu-server
depends on SDN. So if I try to include firewall from SDN, it will not work.

I have looked at Firewall for quite some time now. Some functions in
Firewall.pm depend on QemuServer mainly for the parse_net function. I
tried to extract the functions that depend on QemuServer to another
package but I couldn't get the tests to pass.

Firewall.pm is using several global variables and I couldn't identify
what I missed.

Another option would be to split the SDN module to allow QemuServer to
depend only on a certain part of SDN to notify SDN about nic added to a
VM and VM start. I have not analyzed if it's possible to can split the
dependency cycle.

I don't see a clear path to implement this at this point and I will
focus on supporting Stefan Hanreich next week to finalize other aspects
of SDN for a successful release.

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

> Am 08/11/2023 um 12:35 schrieb Stefan Lendl:
>> Remove require QemuConfig from Firewall.pm
>> We only use it to construct the guest config paths.
>> Fixes circular include when accessing Firewall::Aliases from
>> pve-network.
>>
>
> This won't work as now cfs_read_file only works by luck, if at all, as the
> cfs_read_file needs the cfs_register_file that happens in PVE::QemuServer
> so that the parser is actually available...
>
> I'd much rather see Firewall be split-up than doing broken hacks and
> switching from one of our saner interfaces to manual assembly.

Re: [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path

[Thomas Lamprecht]

Am 10/11/2023 um 14:26 schrieb Stefan Lendl:
> Thomas Lamprecht <t.lamprecht@proxmox.com> writes:
>> This won't work as now cfs_read_file only works by luck, if at all, as the
>> cfs_read_file needs the cfs_register_file that happens in PVE::QemuServer
>> so that the parser is actually available...
>>
>> I'd much rather see Firewall be split-up than doing broken hacks and
>> switching from one of our saner interfaces to manual assembly.
> 
> The issue arises because firewall depends on qemu-server but qemu-server
> depends on SDN. So if I try to include firewall from SDN, it will not work.

Yes, I knew that and read it from your first mail ;-)
My point was, the result to fix that should work and not introduce more
mess to the system.

> 
> I have looked at Firewall for quite some time now. Some functions in
> Firewall.pm depend on QemuServer mainly for the parse_net function. I
> tried to extract the functions that depend on QemuServer to another
> package but I couldn't get the tests to pass.

another package or another module?

> Firewall.pm is using several global variables and I couldn't identify
> what I missed.

Maybe post/push the WIP somewhere so others can help or pick it up (later).

> Another option would be to split the SDN module to allow QemuServer to
> depend only on a certain part of SDN to notify SDN about nic added to a
> VM and VM start. I have not analyzed if it's possible to can split the
> dependency cycle.

A rather different idea would be to avoid that SDN calls into firewall at
all, but like I wrote to my reply to the other patch, rather that the
firewall daemon automatically adds the aliases from the SDN config, after
all such an alias would be useable before the firewall daemon starts the
next round anyway, so from that POV it would be as good as this and avoid
doing direct API calls from some uncontrolled context (can mess with
various (file) permissions).

Ideally those would live in their own namespace, as said in the other
reply, as then one would also avoid odd effects, especially with existing
aliases.

> I don't see a clear path to implement this at this point and I will

Did you tried above option as POC, i.e., the one from my reply from last
Wednesday?

> focus on supporting Stefan Hanreich next week to finalize other aspects
> of SDN for a successful release.
> 

Yeah, I'm *still* missing any patches to the docs, those would be very
high priority as of now, and should be doable on the side, especially
as you tried the various existing features since a few weeks.

ps. as I asked you in the past, *please* avoid top-posting!

[pve-devel] [PATCH pve-network 2/2] Create a cluster-wide firewall for SDN subnets

[Stefan Lendl]

Upon creation of a subnet, we create a cluster-wide firewall alias.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---

Notes:
    Creates the alias directly when the Subnet is created.
    
    Other SDN objects are created upon 'Apply': commit_config().
    Although, IPAM creates the subnet right away as well.
    This should not be an issue but is inconsistent.

 src/PVE/Network/SDN/Subnets.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/PVE/Network/SDN/Subnets.pm b/src/PVE/Network/SDN/Subnets.pm
index 6bb42e5..fe67abd 100644
--- a/src/PVE/Network/SDN/Subnets.pm
+++ b/src/PVE/Network/SDN/Subnets.pm
@@ -6,6 +6,7 @@ use warnings;
 use Net::Subnet qw(subnet_matcher);
 use Net::IP;
 use NetAddr::IP qw(:lower);
+use PVE::API2::Firewall::Aliases;
 
 use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
 use PVE::Network::SDN::Dns;
@@ -161,6 +162,13 @@ sub del_dns_ptr_record {
     $plugin->del_ptr_record($plugin_config, $reversezone, $ip);
 }
 
+sub get_fw_alias_name {
+    my ($subnet) = @_;
+    my $cidr = $subnet->{cidr};
+    $cidr =~ tr/.\//-/;
+    return "$subnet->{zone}_$subnet->{vnet}_$cidr";
+}
+
 sub add_subnet {
     my ($zone, $subnetid, $subnet) = @_;
 
@@ -170,6 +178,13 @@ sub add_subnet {
     my $plugin_config = $ipam_cfg->{ids}->{$ipam};
     my $plugin = PVE::Network::SDN::Ipams::Plugin->lookup($plugin_config->{type});
     $plugin->add_subnet($plugin_config, $subnetid, $subnet);
+
+    my $param = {
+	name => get_fw_alias_name($subnet),
+	cidr => $subnet->{cidr},
+	comment => "Automatically created Alias from SDN => Zone: $subnet->{zone}, VNet: $subnet->{vnet}, Subnet: $subnet->{cidr}"
+    };
+    PVE::API2::Firewall::ClusterAliases->create_alias($param);
 }
 
 sub del_subnet {
@@ -181,6 +196,9 @@ sub del_subnet {
     my $plugin_config = $ipam_cfg->{ids}->{$ipam};
     my $plugin = PVE::Network::SDN::Ipams::Plugin->lookup($plugin_config->{type});
     $plugin->del_subnet($plugin_config, $subnetid, $subnet);
+
+    my $param = { name => get_fw_alias_name($subnet) };
+    PVE::API2::Firewall::ClusterAliases->remove_alias($param);
 }
 
 sub next_free_ip {
-- 
2.41.0

Re: [pve-devel] [PATCH pve-network 2/2] Create a cluster-wide firewall for SDN subnets

[Thomas Lamprecht]

Am 08/11/2023 um 12:35 schrieb Stefan Lendl:
> Upon creation of a subnet, we create a cluster-wide firewall alias.
> 
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
> 
> Notes:
>     Creates the alias directly when the Subnet is created.
>     
>     Other SDN objects are created upon 'Apply': commit_config().
>     Although, IPAM creates the subnet right away as well.
>     This should not be an issue but is inconsistent.
> 
>  src/PVE/Network/SDN/Subnets.pm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/src/PVE/Network/SDN/Subnets.pm b/src/PVE/Network/SDN/Subnets.pm
> index 6bb42e5..fe67abd 100644
> --- a/src/PVE/Network/SDN/Subnets.pm
> +++ b/src/PVE/Network/SDN/Subnets.pm
> @@ -6,6 +6,7 @@ use warnings;
>  use Net::Subnet qw(subnet_matcher);
>  use Net::IP;
>  use NetAddr::IP qw(:lower);
> +use PVE::API2::Firewall::Aliases;

This would need pve-firewall to get added to the dependency list in the
debian/control file, otherwise it will only work by luck but break, e.g.,
bootstrapping.

>  
>  use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
>  use PVE::Network::SDN::Dns;
> @@ -161,6 +162,13 @@ sub del_dns_ptr_record {
>      $plugin->del_ptr_record($plugin_config, $reversezone, $ip);
>  }
>  
> +sub get_fw_alias_name {
> +    my ($subnet) = @_;
> +    my $cidr = $subnet->{cidr};
> +    $cidr =~ tr/.\//-/;
> +    return "$subnet->{zone}_$subnet->{vnet}_$cidr";
> +}

this can easily clash with existing aliases, that then are deleted or addition
fails below.

wouldn't it be nicer if firewall gets the SDN subnets and manages those aliases
in a separate namespaces, i.e., such that they cannot clash with the explicit
aliases from the config?