* [pve-devel] [PATCH firewall/network 0/2] SDN: Create firewall aliases for SDN subnets @ 2023-11-08 11:35 Stefan Lendl 2023-11-08 11:35 ` [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path Stefan Lendl 2023-11-08 11:35 ` [pve-devel] [PATCH pve-network 2/2] Create a cluster-wide firewall for SDN subnets Stefan Lendl 0 siblings, 2 replies; 7+ messages in thread From: Stefan Lendl @ 2023-11-08 11:35 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path 2023-11-08 11:35 [pve-devel] [PATCH firewall/network 0/2] SDN: Create firewall aliases for SDN subnets Stefan Lendl @ 2023-11-08 11:35 ` Stefan Lendl 2023-11-08 14:31 ` Thomas Lamprecht 2023-11-08 11:35 ` [pve-devel] [PATCH pve-network 2/2] Create a cluster-wide firewall for SDN subnets Stefan Lendl 1 sibling, 1 reply; 7+ messages in thread From: Stefan Lendl @ 2023-11-08 11:35 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path 2023-11-08 11:35 ` [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path Stefan Lendl @ 2023-11-08 14:31 ` Thomas Lamprecht 2023-11-10 13:26 ` Stefan Lendl 0 siblings, 1 reply; 7+ messages in thread From: Thomas Lamprecht @ 2023-11-08 14:31 UTC (permalink / raw) To: Proxmox VE development discussion, Stefan Lendl 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path 2023-11-08 14:31 ` Thomas Lamprecht @ 2023-11-10 13:26 ` Stefan Lendl 2023-11-12 17:44 ` Thomas Lamprecht 0 siblings, 1 reply; 7+ messages in thread From: Stefan Lendl @ 2023-11-10 13:26 UTC (permalink / raw) To: Proxmox VE development discussion 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path 2023-11-10 13:26 ` Stefan Lendl @ 2023-11-12 17:44 ` Thomas Lamprecht 0 siblings, 0 replies; 7+ messages in thread From: Thomas Lamprecht @ 2023-11-12 17:44 UTC (permalink / raw) To: Proxmox VE development discussion, Stefan Lendl 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH pve-network 2/2] Create a cluster-wide firewall for SDN subnets 2023-11-08 11:35 [pve-devel] [PATCH firewall/network 0/2] SDN: Create firewall aliases for SDN subnets Stefan Lendl 2023-11-08 11:35 ` [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path Stefan Lendl @ 2023-11-08 11:35 ` Stefan Lendl 2023-11-08 14:36 ` Thomas Lamprecht 1 sibling, 1 reply; 7+ messages in thread From: Stefan Lendl @ 2023-11-08 11:35 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH pve-network 2/2] Create a cluster-wide firewall for SDN subnets 2023-11-08 11:35 ` [pve-devel] [PATCH pve-network 2/2] Create a cluster-wide firewall for SDN subnets Stefan Lendl @ 2023-11-08 14:36 ` Thomas Lamprecht 0 siblings, 0 replies; 7+ messages in thread From: Thomas Lamprecht @ 2023-11-08 14:36 UTC (permalink / raw) To: Proxmox VE development discussion, Stefan Lendl 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-12 17:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-08 11:35 [pve-devel] [PATCH firewall/network 0/2] SDN: Create firewall aliases for SDN subnets Stefan Lendl 2023-11-08 11:35 ` [pve-devel] [PATCH pve-firewall 1/2] Manually construct guest config path Stefan Lendl 2023-11-08 14:31 ` Thomas Lamprecht 2023-11-10 13:26 ` Stefan Lendl 2023-11-12 17:44 ` Thomas Lamprecht 2023-11-08 11:35 ` [pve-devel] [PATCH pve-network 2/2] Create a cluster-wide firewall for SDN subnets Stefan Lendl 2023-11-08 14:36 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox