[pve-devel] [PATCH v3 pve-network 00/12] SDN: Testing VNets as a blackbox and fixing found bugs

[pve-devel] [PATCH v3 pve-network 00/12] SDN: Testing VNets as a blackbox and fixing found bugs

[Stefan Lendl]

This add several tests for SDN VNets.
State setup as well as testing results is done only via the API to test on the
API boundaries and not against the internal state. Internal state and config
files are mocked to avoid requiring access to system files or pmxcfs.

The first 4 commits fix functionality, identified as bugs thanks to the tests.

The next 7 commits extract various functions to allow mocking them in the
tests. The tests are then added as the test_vnets_blackbox test.
The last commit removes the old vnets tests which are not working anyway.

Tests validate the events of a nic joining a VNet or a nic staring on a VNet.
These events are tested with with different subnet configurations.
Mainly for IPv4 and/or IPv6 configurations and odd combinations.
Further descriptions in the commit.

Differences v2 -> v3:
* Fix functionalitiy in VNet and Subnet so all tests pass
  Thanks @s.hanreich for lots of testing
* Update and add more tests
* Make it build in sbuild

Differences v1 -> v2:
* Add tests that expect a failure when no IP can be allocated
* Removed commented out debug stuff


Stefan Hanreich (2):
  sdn: dhcp: only consider subnets that have dhcp-range configured
  sdn: dhcp: rollback allocated ips on failure

Stefan Lendl (10):
  sdn: dhcp: get next free ip for a specific IP version
  sdn: dhcp: request both IPv4 and IPv6 addresses on VM start
  sdn: zones: extract function that reads datacenter config
  dns: dnsmasq: extract function to systemctl command.
  sdn: dnsmasq: extract function that generates the ethers file path
  sdn: dnsmasq: extract function that updates dnsmasq lease via dbus
  sdn: api: extract function that creates the sdn directory.
  debian: blackbox tests depend on libpve-access-control at build
  tests: test VNets functionality as a blackbox
  tests: remove old Vnets tests

 debian/control                            |   1 +
 src/PVE/API2/Network/SDN/Zones.pm         |   6 +-
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm       |  47 +-
 src/PVE/Network/SDN/Subnets.pm            |   2 +-
 src/PVE/Network/SDN/Vnets.pm              |  29 +-
 src/PVE/Network/SDN/Zones/EvpnPlugin.pm   |   3 +-
 src/PVE/Network/SDN/Zones/Plugin.pm       |   5 +
 src/PVE/Network/SDN/Zones/SimplePlugin.pm |   2 +-
 src/test/Makefile                         |   5 +-
 src/test/run_test_vnets.pl                | 343 ---------
 src/test/run_test_vnets_blackbox.pl       | 894 ++++++++++++++++++++++
 11 files changed, 962 insertions(+), 375 deletions(-)
 delete mode 100755 src/test/run_test_vnets.pl
 create mode 100755 src/test/run_test_vnets_blackbox.pl

-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 01/12] sdn: dhcp: get next free ip for a specific IP version

[Stefan Lendl]

Specify the IP version (4|6) for which an IP shall be requested from the IPAM.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/Subnets.pm | 2 +-
 src/PVE/Network/SDN/Vnets.pm   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Network/SDN/Subnets.pm b/src/PVE/Network/SDN/Subnets.pm
index 3b08dcd..e2c8c9c 100644
--- a/src/PVE/Network/SDN/Subnets.pm
+++ b/src/PVE/Network/SDN/Subnets.pm
@@ -206,7 +206,7 @@ sub del_subnet {
 }
 
 sub add_next_free_ip {
-    my ($zone, $subnetid, $subnet, $hostname, $mac, $vmid, $skipdns, $dhcprange) = @_;
+    my ($zone, $subnetid, $subnet, $hostname, $mac, $vmid, $skipdns, $dhcprange, $ipversion) = @_;
 
     my $cidr = undef;
     my $ip = undef;
diff --git a/src/PVE/Network/SDN/Vnets.pm b/src/PVE/Network/SDN/Vnets.pm
index 0dfdfd7..03609b7 100644
--- a/src/PVE/Network/SDN/Vnets.pm
+++ b/src/PVE/Network/SDN/Vnets.pm
@@ -95,7 +95,7 @@ sub get_subnet_from_vnet_ip {
 }
 
 sub add_next_free_cidr {
-    my ($vnetid, $hostname, $mac, $vmid, $skipdns, $dhcprange) = @_;
+    my ($vnetid, $hostname, $mac, $vmid, $skipdns, $dhcprange, $ipversion) = @_;
 
     my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid);
     return if !$vnet;
@@ -109,7 +109,7 @@ sub add_next_free_cidr {
 
     my $ips = {};
 
-    my @ipversions = qw/ 4 6 /;
+    my @ipversions = defined($ipversion) ? ($ipversion) : qw/ 4 6 /;
     for my $ipversion (@ipversions) {
 	my $ip = undef;
 	my $subnetcount = 0;
@@ -125,7 +125,7 @@ sub add_next_free_cidr {
 	    };
 	    die $@ if $@;
 
-            if ($ip) {
+	    if ($ip) {
 		$ips->{$ipversion} = $ip;
 		last;
 	    }
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 02/12] sdn: dhcp: request both IPv4 and IPv6 addresses on VM start

[Stefan Lendl]

If previously an IP was allocated in the IPAM, but a new subnet added
for the other IP version, we need to allocate an IP in the new subnet.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/Vnets.pm | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Network/SDN/Vnets.pm b/src/PVE/Network/SDN/Vnets.pm
index 03609b7..4542b70 100644
--- a/src/PVE/Network/SDN/Vnets.pm
+++ b/src/PVE/Network/SDN/Vnets.pm
@@ -196,12 +196,10 @@ sub add_dhcp_mapping {
     return if !$zone->{ipam} || !$zone->{dhcp};
 
     my ($ip4, $ip6) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
-    if ( ! ($ip4 || $ip6) ) {
-	print "No IP found for MAC: $mac for VMID:$vmid\n";
-	add_next_free_cidr($vnetid, $name, $mac, "$vmid", undef, 1);
-	($ip4, $ip6) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
-	print "got new IP from IPAM: $ip4 $ip6\n";
-    }
+    add_next_free_cidr($vnetid, $name, $mac, "$vmid", undef, 1, 4) if ! $ip4;
+    add_next_free_cidr($vnetid, $name, $mac, "$vmid", undef, 1, 6) if ! $ip6;
+
+    ($ip4, $ip6) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
     PVE::Network::SDN::Dhcp::add_mapping($vnetid, $mac, $ip4, $ip6) if $ip4 || $ip6;
 }
 
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 03/12] sdn: dhcp: only consider subnets that have dhcp-range configured

[Stefan Lendl]

From: Stefan Hanreich <s.hanreich@proxmox.com>

If DHCP is enabled on a zone with subnets, but no subnet has a
dhcp-range configured, then starting a VM will fail because no IP can
be allocated. This patch fixes this by only considering subnets that
have a dhcp-range configured and only failing if there is at least one
subnet with a dhcp-range configured.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
Reviewed-by: Stefan Lendl <s.lendl@proxmox.com>
Tested-by: Stefan Lendl <s.lendl@proxmox.com>
Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Network/SDN/Vnets.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/Network/SDN/Vnets.pm b/src/PVE/Network/SDN/Vnets.pm
index 4542b70..cbf0a07 100644
--- a/src/PVE/Network/SDN/Vnets.pm
+++ b/src/PVE/Network/SDN/Vnets.pm
@@ -118,6 +118,7 @@ sub add_next_free_cidr {
 	    my $network = $subnet->{network};
 
 	    next if Net::IP::ip_get_version($network) != $ipversion || $ips->{$ipversion};
+	    next if !$subnet->{'dhcp-range'};
 	    $subnetcount++;
 
 	    eval {
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 04/12] sdn: dhcp: rollback allocated ips on failure

[Stefan Lendl]

From: Stefan Hanreich <s.hanreich@proxmox.com>

If DHCP is configured for IPv4 and IPv6, failing to obtain an IPv6 IP
does not roll back the allocation made for IPv4. This patch rolls back
any changes made in case of failure, so that IP allocation is actually
atomic.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
Reviewed-by: Stefan Lendl <s.lendl@proxmox.com>
Tested-by: Stefan Lendl <s.lendl@proxmox.com>
Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Network/SDN/Vnets.pm | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Network/SDN/Vnets.pm b/src/PVE/Network/SDN/Vnets.pm
index cbf0a07..45292e3 100644
--- a/src/PVE/Network/SDN/Vnets.pm
+++ b/src/PVE/Network/SDN/Vnets.pm
@@ -131,7 +131,17 @@ sub add_next_free_cidr {
 		last;
 	    }
 	}
-	die "can't find any free ip" if !$ip && $subnetcount > 0;
+
+	if (!$ip && $subnetcount > 0) {
+	    foreach my $version (sort keys %{$ips}) {
+		my $ip = $ips->{$version};
+		my ($subnetid, $subnet) = PVE::Network::SDN::Subnets::find_ip_subnet($ip, $subnets);
+
+		PVE::Network::SDN::Subnets::del_ip($zone, $subnetid, $subnet, $ip, $hostname, $mac, $skipdns);
+	    }
+
+	    die "can't find any free ip in zone $zoneid for IPv$ipversion";
+	}
     }
 }
 
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 05/12] sdn: zones: extract function that reads datacenter config

[Stefan Lendl]

The datacenter_config() functions in SDN::Zones::Plugin is a simple
wrapper that reads datacenter.cfg via cfs.
This allows mocking datacenter.cfg in tests.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/Zones/EvpnPlugin.pm   | 3 ++-
 src/PVE/Network/SDN/Zones/Plugin.pm       | 5 +++++
 src/PVE/Network/SDN/Zones/SimplePlugin.pm | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
index 655a9f0..4843756 100644
--- a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
+++ b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
@@ -3,6 +3,7 @@ package PVE::Network::SDN::Zones::EvpnPlugin;
 use strict;
 use warnings;
 use PVE::Network::SDN::Zones::VxlanPlugin;
+use PVE::Network::SDN::Zones::Plugin;
 use PVE::Exception qw(raise raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools qw($IPV4RE);
@@ -294,7 +295,7 @@ sub on_update_hook {
     }
 
     if (!defined($zone_cfg->{ids}->{$zoneid}->{'mac'})) {
-	my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
+        my $dc = PVE::Network::SDN::Zones::Plugin->datacenter_config();
 	$zone_cfg->{ids}->{$zoneid}->{'mac'} = PVE::Tools::random_ether_addr($dc->{mac_prefix});
     }
 }
diff --git a/src/PVE/Network/SDN/Zones/Plugin.pm b/src/PVE/Network/SDN/Zones/Plugin.pm
index b55b967..247d0b2 100644
--- a/src/PVE/Network/SDN/Zones/Plugin.pm
+++ b/src/PVE/Network/SDN/Zones/Plugin.pm
@@ -356,4 +356,9 @@ sub get_bridge_ifaces {
 
     return @bridge_ifaces;
 }
+
+sub datacenter_config {
+    return PVE::Cluster::cfs_read_file('datacenter.cfg');
+}
+
 1;
diff --git a/src/PVE/Network/SDN/Zones/SimplePlugin.pm b/src/PVE/Network/SDN/Zones/SimplePlugin.pm
index c996bf3..65e9ad4 100644
--- a/src/PVE/Network/SDN/Zones/SimplePlugin.pm
+++ b/src/PVE/Network/SDN/Zones/SimplePlugin.pm
@@ -139,7 +139,7 @@ sub vnet_update_hook {
     raise_param_exc({ tag => "vlan tag is not allowed on simple zone"}) if defined($tag);
 
     if (!defined($vnet->{mac})) {
-        my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
+        my $dc = PVE::Network::SDN::Zones::Plugin::datacenter_config();
         $vnet->{mac} = PVE::Tools::random_ether_addr($dc->{mac_prefix});
     }
 }
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 06/12] dns: dnsmasq: extract function to systemctl command.

[Stefan Lendl]

systemctl_service() is a wrapper around PVE::Tools::run_command to allow
mocking the systemctl interactions in tests.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index 2844943..f9f1c39 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -101,7 +101,7 @@ sub add_ip_mapping {
     }
 
     my $service_name = "dnsmasq\@$dhcpid";
-    PVE::Tools::run_command(['systemctl', 'reload', $service_name]) if $reload;
+    systemctl_service('reload', $service_name) if $reload;
 
     #update lease as ip could still be associated to an old removed mac
     my $bus = Net::DBus->system();
@@ -163,6 +163,12 @@ sub configure_vnet {
     );
 }
 
+sub systemctl_service {
+    my ($action, $service) = @_;
+
+    PVE::Tools::run_command(['systemctl', $action, $service]);
+}
+
 sub before_configure {
     my ($class, $dhcpid) = @_;
 
@@ -250,9 +256,9 @@ sub after_configure {
 
     my $service_name = "dnsmasq\@$dhcpid";
 
-    PVE::Tools::run_command(['systemctl', 'reload', 'dbus']);
-    PVE::Tools::run_command(['systemctl', 'enable', $service_name]);
-    PVE::Tools::run_command(['systemctl', 'restart', $service_name]);
+    systemctl_service('reload', 'dbus');
+    systemctl_service('enable', $service_name);
+    systemctl_service('restart', $service_name);
 }
 
 sub before_regenerate {
@@ -260,8 +266,8 @@ sub before_regenerate {
 
     return if !assert_dnsmasq_installed($noerr);
 
-    PVE::Tools::run_command(['systemctl', 'stop', "dnsmasq@*"]);
-    PVE::Tools::run_command(['systemctl', 'disable', 'dnsmasq@']);
+    systemctl_service('stop', "dnsmasq@*");
+    systemctl_service('disable', 'dnsmasq@');
 }
 
 sub after_regenerate {
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 07/12] sdn: dnsmasq: extract function that generates the ethers file path

[Stefan Lendl]

Extracted to a function so it can be mocked in tests.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index f9f1c39..5a227ba 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -33,10 +33,15 @@ my sub assert_dnsmasq_installed {
     return 1;
 }
 
+sub ethers_file {
+    my ($dhcpid) = @_;
+    return "$DNSMASQ_CONFIG_ROOT/$dhcpid/ethers";
+}
+
 sub add_ip_mapping {
     my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_;
 
-    my $ethers_file = "$DNSMASQ_CONFIG_ROOT/$dhcpid/ethers";
+    my $ethers_file = ethers_file($dhcpid);
     my $ethers_tmp_file = "$ethers_file.tmp";
 
     my $reload = undef;
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 08/12] sdn: dnsmasq: extract function that updates dnsmasq lease via dbus

[Stefan Lendl]

Extract the dbus based interactions with dnsmasq so that it can be
mocked in tests.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index 5a227ba..c14f5d7 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -38,6 +38,17 @@ sub ethers_file {
     return "$DNSMASQ_CONFIG_ROOT/$dhcpid/ethers";
 }
 
+sub update_lease {
+    my ($dhcpid, $ip4, $mac) = @_;
+    #update lease as ip could still be associated to an old removed mac
+    my $bus = Net::DBus->system();
+    my $dnsmasq = $bus->get_service("uk.org.thekelleys.dnsmasq.$dhcpid");
+    my $manager = $dnsmasq->get_object("/uk/org/thekelleys/dnsmasq","uk.org.thekelleys.dnsmasq.$dhcpid");
+
+    my @hostname = unpack("C*", "*");
+    $manager->AddDhcpLease($ip4, $mac, \@hostname, undef, 0, 0, 0) if $ip4;
+}
+
 sub add_ip_mapping {
     my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_;
 
@@ -107,16 +118,7 @@ sub add_ip_mapping {
 
     my $service_name = "dnsmasq\@$dhcpid";
     systemctl_service('reload', $service_name) if $reload;
-
-    #update lease as ip could still be associated to an old removed mac
-    my $bus = Net::DBus->system();
-    my $dnsmasq = $bus->get_service("uk.org.thekelleys.dnsmasq.$dhcpid");
-    my $manager = $dnsmasq->get_object("/uk/org/thekelleys/dnsmasq","uk.org.thekelleys.dnsmasq.$dhcpid");
-
-    my @hostname = unpack("C*", "*");
-    $manager->AddDhcpLease($ip4, $mac, \@hostname, undef, 0, 0, 0) if $ip4;
-#    $manager->AddDhcpLease($ip6, $mac, \@hostname, undef, 0, 0, 0) if $ip6;
-
+    update_lease($dhcpid, $ip4, $mac);
 }
 
 sub configure_subnet {
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 09/12] sdn: api: extract function that creates the sdn directory.

[Stefan Lendl]

create_etc_interfaces_sdn_dir creates the /etc/pve/sdn directory.
This allows mocking in tests to prevent system fs access in tests

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/API2/Network/SDN/Zones.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Network/SDN/Zones.pm b/src/PVE/API2/Network/SDN/Zones.pm
index b09c9ad..35e2f7f 100644
--- a/src/PVE/API2/Network/SDN/Zones.pm
+++ b/src/PVE/API2/Network/SDN/Zones.pm
@@ -186,6 +186,10 @@ __PACKAGE__->register_method ({
 	return &$api_sdn_zones_config($cfg, $param->{zone});
     }});
 
+sub create_etc_interfaces_sdn_dir {
+    mkdir("/etc/pve/sdn");
+}
+
 __PACKAGE__->register_method ({
     name => 'create',
     protected => 1,
@@ -207,7 +211,7 @@ __PACKAGE__->register_method ({
 	my $opts = $plugin->check_config($id, $param, 1, 1);
 
 	PVE::Cluster::check_cfs_quorum();
-	mkdir("/etc/pve/sdn");
+	create_etc_interfaces_sdn_dir();
 
 	PVE::Network::SDN::lock_sdn_config(sub {
 	    my $zone_cfg = PVE::Network::SDN::Zones::config();
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 10/12] debian: blackbox tests depend on libpve-access-control at build

[Stefan Lendl]

For mocking RPCEnvironment in sbuild.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 debian/control | 1 +
 1 file changed, 1 insertion(+)

diff --git a/debian/control b/debian/control
index 6f0bbeb..1f97196 100644
--- a/debian/control
+++ b/debian/control
@@ -10,6 +10,7 @@ Build-Depends: debhelper-compat (= 13),
                perl,
                pve-cluster (>= 8.0.5),
                pve-doc-generator (>= 5.3-3),
+               libpve-access-control,
 Standards-Version: 4.6.1
 Homepage: https://www.proxmox.com
 
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 11/12] tests: test VNets functionality as a blackbox

[Stefan Lendl]

Add several tests for Vnets in test_vnets_blackbox. State setup as well
as testing results is done only via the API to test on the API
boundaries not not against the internal state. Internal state is mocked
to avoid requiring access to system files or pmxcfs.

Mocking is done by reading and writing to a hash that holds the entire
state of SDN. The state is reset after every test run.

Testing is done via helper functions: nic_join and nic_start.
When a nic joins a Vnet, currently it always - and only - calls
add_next_free_cidr(). The same is true if a nic starts on Vnet, which
only calles add_dhcp_mapping.

These test functions homogenize the parameter list in contrast to the
current calls to the current functions.  The intention for the functions
is that they can be moved to Vnets.pm to be called from QemuServer and
LXC!

The tests are composed of a test function which can be parameterized. To
call the test function, the run_test function takes the function pointer
and passes the rest of the arguments to the test functions. It also
takes care of resetting the test state.
This allows fine-grained parameterization per-test directly in the code
instead of separated files that require the entire state to be passed
in.

The tests setup the SDN by creating a simple zone and a simple vnet. The
nic_join and nic_start function is called with different subnet
configuration wiht and without a dhcp-range configured and with or
without an already present IP in the IPAM.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/test/Makefile                   |   5 +-
 src/test/run_test_vnets_blackbox.pl | 894 ++++++++++++++++++++++++++++
 2 files changed, 898 insertions(+), 1 deletion(-)
 create mode 100755 src/test/run_test_vnets_blackbox.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index eb59d5f..5a937a4 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -1,6 +1,6 @@
 all: test
 
-test: test_zones test_ipams test_dns test_subnets
+test: test_zones test_ipams test_dns test_subnets test_vnets_blackbox
 
 test_zones: run_test_zones.pl
 	./run_test_zones.pl
@@ -14,4 +14,7 @@ test_dns: run_test_dns.pl
 test_subnets: run_test_subnets.pl
 	./run_test_subnets.pl
 
+test_vnets_blackbox: run_test_vnets_blackbox.pl
+	./run_test_vnets_blackbox.pl
+
 clean:
diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl
new file mode 100755
index 0000000..f7caca2
--- /dev/null
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -0,0 +1,894 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+use File::Slurp;
+use List::Util qw(first all);
+use NetAddr::IP qw(:lower);
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::Tools qw(extract_param file_set_contents);
+
+use PVE::Network::SDN;
+use PVE::Network::SDN::Zones;
+use PVE::Network::SDN::Zones::Plugin;
+use PVE::Network::SDN::Controllers;
+use PVE::Network::SDN::Dns;
+use PVE::Network::SDN::Vnets;
+
+use PVE::RESTEnvironment;
+
+use PVE::API2::Network::SDN::Zones;
+use PVE::API2::Network::SDN::Subnets;
+use PVE::API2::Network::SDN::Vnets;
+use PVE::API2::Network::SDN::Ipams;
+
+my $TMP_ETHERS_FILE = "/tmp/ethers";
+
+my $test_state = undef;
+sub clear_test_state {
+    $test_state = {
+	locks => {},
+	datacenter_config => {},
+	subnets_config => {},
+	controller_config => {},
+	dns_config => {},
+	zones_config => {},
+	vnets_config => {},
+	macdb => {},
+	ipamdb => {},
+	ipam_config => {
+	    'ids' => {
+		'pve' => {
+		    'type' => 'pve'
+		},
+	    }
+	},
+    };
+    PVE::Tools::file_set_contents($TMP_ETHERS_FILE, "\n");
+}
+clear_test_state();
+
+my $mocked_cfs_lock_file = sub {
+    my ($filename, $timeout, $code, @param) = @_;
+
+    die "$filename already locked\n" if ($test_state->{locks}->{$filename});
+
+    $test_state->{locks}->{$filename} = 1;
+
+    my $res = eval { $code->(@param); };
+
+    delete $test_state->{locks}->{$filename};
+
+    return $res;
+};
+
+sub read_sdn_config {
+    my ($file) = @_;
+    # Read structure back in again
+    open my $in, '<', $file or die $!;
+    my $sdn_config;
+    {
+	local $/;    # slurp mode
+	$sdn_config = eval <$in>;
+    }
+    close $in;
+    return $sdn_config;
+}
+
+my $mocked_pve_sdn;
+$mocked_pve_sdn = Test::MockModule->new('PVE::Network::SDN');
+$mocked_pve_sdn->mock(
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+my $mocked_pve_tools = Test::MockModule->new('PVE::Tools');
+$mocked_pve_tools->mock(
+    lock_file => $mocked_cfs_lock_file,
+);
+
+my $mocked_sdn_zones;
+$mocked_sdn_zones = Test::MockModule->new('PVE::Network::SDN::Zones');
+$mocked_sdn_zones->mock(
+    config => sub {
+	return $test_state->{zones_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{zones_config} = $cfg;
+    },
+);
+
+my $mocked_sdn_zones_super_plugin;
+$mocked_sdn_zones_super_plugin = Test::MockModule->new('PVE::Network::SDN::Zones::Plugin');
+$mocked_sdn_zones_super_plugin->mock(
+    datacenter_config => sub {
+	return $test_state->{datacenter_config};
+    },
+);
+
+my $mocked_sdn_vnets;
+$mocked_sdn_vnets = Test::MockModule->new('PVE::Network::SDN::Vnets');
+$mocked_sdn_vnets->mock(
+    config => sub {
+	return $test_state->{vnets_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{vnets_config} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+my $mocked_sdn_subnets;
+$mocked_sdn_subnets = Test::MockModule->new('PVE::Network::SDN::Subnets');
+$mocked_sdn_subnets->mock(
+    config => sub {
+	return $test_state->{subnets_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{subnets_config} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+my $mocked_sdn_controller;
+$mocked_sdn_controller = Test::MockModule->new('PVE::Network::SDN::Controllers');
+$mocked_sdn_controller->mock(
+    config => sub {
+	return $test_state->{controller_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{controller_config} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+
+my $mocked_sdn_dns;
+$mocked_sdn_dns = Test::MockModule->new('PVE::Network::SDN::Dns');
+$mocked_sdn_dns->mock(
+    config => sub {
+	return $test_state->{dns_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{dns_config} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+
+my $mocked_sdn_ipams;
+$mocked_sdn_ipams = Test::MockModule->new('PVE::Network::SDN::Ipams');
+$mocked_sdn_ipams->mock(
+    config => sub {
+	return $test_state->{ipam_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{ipam_config} = $cfg;
+    },
+    read_macdb => sub {
+	return $test_state->{macdb};
+    },
+    write_macdb => sub {
+	my ($cfg) = @_;
+	$test_state->{macdb} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+my $ipam_plugin = PVE::Network::SDN::Ipams::Plugin->lookup("pve"); # NOTE this is hard-coded to pve
+my $mocked_ipam_plugin = Test::MockModule->new($ipam_plugin);
+$mocked_ipam_plugin->mock(
+    read_db => sub {
+	return $test_state->{ipamdb};
+    },
+    write_db => sub {
+	my ($cfg) = @_;
+	$test_state->{ipamdb} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+my $mocked_sdn_dhcp_dnsmasq = Test::MockModule->new('PVE::Network::SDN::Dhcp::Dnsmasq');
+$mocked_sdn_dhcp_dnsmasq->mock(
+    assert_dnsmasq_installed => sub { return 1; },
+    before_configure => sub {},
+    ethers_file => sub { return "/tmp/ethers"; },
+    systemctl_service => sub {},
+    update_lease => sub {},
+);
+
+my $mocked_api_zones = Test::MockModule->new('PVE::API2::Network::SDN::Zones');
+$mocked_api_zones->mock(
+    create_etc_interfaces_sdn_dir => sub {},
+);
+
+my $rpcenv = PVE::RESTEnvironment->init('priv');
+$rpcenv->init_request();
+$rpcenv->set_language("en_US.UTF-8");
+$rpcenv->set_user('root@pam');
+
+my $mocked_rpc_env_obj = Test::MockModule->new('PVE::RESTEnvironment');
+$mocked_rpc_env_obj->mock(
+    check_any => sub { return 1; },
+);
+
+my $mocked_pve_cluster_obj = Test::MockModule->new('PVE::Cluster');
+$mocked_pve_cluster_obj->mock(
+    check_cfs_quorum => sub { return 1; },
+);
+
+# ------- TEST FUNCTIONS --------------
+
+sub nic_join {
+    my ($vnetid, $mac, $hostname, $vmid) = @_;
+    return PVE::Network::SDN::Vnets::add_next_free_cidr($vnetid, $hostname, $mac, "$vmid", undef, 1);
+}
+
+sub nic_leave {
+    my ($vnetid, $mac, $hostname) = @_;
+    return PVE::Network::SDN::Vnets::del_ips_from_mac($vnetid, $mac, $hostname);
+}
+
+sub nic_start {
+    my ($vnetid, $mac, $vmid, $hostname) = @_;
+    return PVE::Network::SDN::Vnets::add_dhcp_mapping($vnetid, $mac, $vmid, $hostname);
+}
+
+
+# ---- API HELPER FUNCTIONS FOR THE TESTS -----
+
+my $t_invalid;
+sub get_zone {
+    my ($id) = @_;
+    return eval { PVE::API2::Network::SDN::Zones->read({zone => $id}); };
+}
+# verify get_zone actually fails if invalid
+$t_invalid = get_zone("invalid");
+die("getting an invalid zone must fail") if (!$@);
+fail("getting an invalid zone must fail") if (defined $t_invalid);
+
+sub create_zone {
+    my ($params) = @_;
+    my $zoneid = $params->{zone};
+    # die if failed!
+    eval { PVE::API2::Network::SDN::Zones->create($params); };
+    die("creating zone failed: $@") if ($@);
+
+    my $zone = get_zone($zoneid);
+    die ("test setup: zone ($zoneid) not defined") if (!defined $zone);
+    return $zone;
+}
+
+sub get_vnet {
+    my ($id) = @_;
+    return eval { PVE::API2::Network::SDN::Vnets->read({vnet => $id}); };
+}
+# verify get_vnet
+$t_invalid = get_vnet("invalid");
+die("getting an invalid vnet must fail") if (!$@);
+fail("getting an invalid vnet must fail") if (defined $t_invalid);
+
+sub create_vnet {
+    my ($params) = @_;
+    my $vnetid = $params->{vnet};
+    PVE::API2::Network::SDN::Vnets->create($params);
+
+    my $vnet = get_vnet($vnetid);
+    die ("test setup: vnet ($vnetid) not defined") if (!defined $vnet);
+    return $vnet;
+}
+
+sub get_subnet {
+    my ($id) = @_;
+    return eval { PVE::API2::Network::SDN::Subnets->read({subnet => $id}); };
+}
+# verify get_subnet
+$t_invalid = get_subnet("invalid");
+die("getting an invalid subnet must fail") if (!$@);
+fail("getting an invalid subnet must fail") if (defined $t_invalid);
+
+sub create_subnet {
+    my ($params) = @_;
+    PVE::API2::Network::SDN::Subnets->create($params);
+}
+
+sub get_ipam_entries {
+    return PVE::API2::Network::SDN::Ipams->ipamindex({ipam => "pve"});
+}
+
+sub create_ip {
+    my ($param) = @_;
+    return PVE::API2::Network::SDN::Ips->ipcreate($param);
+}
+
+sub run_test {
+    my $test = shift;
+    clear_test_state();
+    $test->(@_);
+}
+
+sub get_ips_from_mac {
+    my ($mac) = @_;
+    my $ipam_entries = get_ipam_entries();
+    return grep { $_->{mac} eq $mac if defined $_->{mac} } $ipam_entries->@* if $ipam_entries;
+}
+
+sub get_ip4 {
+    my $ip4 = first { Net::IP::ip_is_ipv4($_->{ip}) } @_;
+    return $ip4->{ip} if defined $ip4;
+}
+
+sub get_ip6 {
+    my $ip6 = first { Net::IP::ip_is_ipv6($_->{ip}) } @_;
+    return $ip6->{ip} if defined $ip6;
+}
+
+
+# -------------- ACTUAL TESTS  -----------------------
+
+sub test_create_vnet_with_gateway {
+    my $test_name = (split(/::/,(caller(0))[3]))[-1];
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    create_subnet({
+	type => "subnet",
+	vnet => $vnetid,
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    });
+
+    my ($p) = first { $_->{gateway} == 1 } get_ipam_entries()->@*;
+    ok ($p, "$test_name: Gateway IP was created in IPAM");
+}
+run_test(\&test_create_vnet_with_gateway);
+
+
+sub test_without_subnet {
+    my $test_name = (split(/::/,(caller(0))[3]))[-1];
+
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    my $hostname = "testhostname";
+    my $mac = "da:65:8f:18:9b:6f";
+    my $vmid = "999";
+
+    eval {
+	nic_join($vnetid, $mac, $hostname, $vmid);
+    };
+
+    if ($@) {
+	fail("$test_name: $@");
+	return;
+    }
+
+    my @ips = get_ips_from_mac($mac);
+    my $num_ips = scalar @ips;
+    is ($num_ips, 0, "$test_name: No IP allocated in IPAM");
+}
+run_test(\&test_without_subnet);
+
+
+sub test_nic_join {
+    my ($test_name, $subnets) = @_;
+
+    die "$test_name: we're expecting an array of subnets" if !$subnets;
+    my $num_subnets = scalar $subnets->@*;
+    die "$test_name: we're expecting an array of subnets. $num_subnets elements found" if ($num_subnets < 1);
+    my $num_dhcp_ranges = scalar grep { $_->{'dhcp-range'} } $subnets->@*;
+
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    foreach my $subnet ($subnets->@*) {
+	$subnet->{type} = "subnet";
+	$subnet->{vnet} = $vnetid;
+	create_subnet($subnet);
+    };
+
+    my $hostname = "testhostname";
+    my $mac = "da:65:8f:18:9b:6f";
+    my $vmid = "999";
+
+    eval {
+	nic_join($vnetid, $mac, $hostname, $vmid);
+    };
+
+    if ($@) {
+	fail("$test_name: $@");
+	return;
+    }
+
+    my @ips = get_ips_from_mac($mac);
+    my $num_ips = scalar @ips;
+    is ($num_ips, $num_dhcp_ranges, "$test_name: Expecting $num_dhcp_ranges IPs, found $num_ips");
+    ok ((all { ($_->{vnet} eq $vnetid && $_->{zone} eq $zoneid) } @ips),
+	"$test_name: all IPs in correct vnet and zone"
+    );
+}
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4 no dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv6 no dhcp",
+    [{
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4+6 no dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4 with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv6 with dhcp",
+    [{
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4+6 with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4 no DHCP, IPv6 with DHCP",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4 with DHCP, IPv6 no DHCP",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    },
+]);
+
+
+sub test_nic_join_full_dhcp_range {
+    my ($test_name, $subnets, $expected_ip4, $expected_ip6) = @_;
+
+    die "$test_name: we're expecting an array of subnets" if !$subnets;
+    my $num_subnets = scalar $subnets->@*;
+    die "$test_name: we're expecting an array of subnets. $num_subnets elements found" if ($num_subnets < 1);
+
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    foreach my $subnet ($subnets->@*) {
+	$subnet->{type} = "subnet";
+	$subnet->{vnet} = $vnetid;
+	create_subnet($subnet);
+    };
+
+    my $hostname = "testhostname";
+    my $mac = "da:65:8f:18:9b:6f";
+    my $vmid = "999";
+
+    eval {
+	nic_join($vnetid, $mac, $hostname, $vmid);
+    };
+
+    if (! $@) {
+	fail ("$test_name: nic_join() is expected to fail because we cannot allocate all IPs");
+    }
+
+    my @ips = get_ips_from_mac($mac);
+    my $num_ips = scalar @ips;
+    is ($num_ips, 0, "$test_name: No IP allocated in IPAM");
+}
+
+run_test(
+    \&test_nic_join_full_dhcp_range,
+    "nic_join IPv4 with DHCP, dhcp-range full",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.100", # the gateway uses the only available IP in the dhcp-range
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.100"],
+    }
+]);
+
+run_test(
+    \&test_nic_join_full_dhcp_range,
+    "nic_join IPv6 with DHCP, dhcp-range full",
+    [{
+	subnet => "8888::/64",
+	gateway => "8888::100", # the gateway uses the only available IP in the dhcp-range
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::100"],
+    },
+]);
+
+run_test(
+    \&test_nic_join_full_dhcp_range,
+    "nic_join IPv4+6 with DHCP, dhcp-range full for both",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.100",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.100"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::100",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::100"],
+    }
+]);
+
+run_test(
+    \&test_nic_join_full_dhcp_range,
+    "nic_join IPv4+6 with DHCP, dhcp-range full for IPv4",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.100", # the gateway uses the only available IP in the dhcp-range
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.100"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::100"],
+    }],
+);
+
+run_test(
+    \&test_nic_join_full_dhcp_range,
+    "nic_join IPv4+6 with DHCP, dhcp-range full for IPv6",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.100"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::100",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::100"],
+    }],
+);
+
+run_test(
+    \&test_nic_join_full_dhcp_range,
+    "nic_join IPv4 no DHCP, dhcp-range full for IPv6",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::100",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::100"],
+    }],
+);
+
+
+# -------------- nic_start
+sub test_nic_start {
+    my ($test_name, $subnets, $current_ip4, $current_ip6, $num_expected_ips) = @_;
+
+    die "$test_name: we're expecting an array of subnets" if !$subnets;
+    my $num_subnets = scalar $subnets->@*;
+    die "$test_name: we're expecting an array of subnets. $num_subnets elements found" if ($num_subnets < 1);
+
+    $num_expected_ips = scalar grep { $_->{'dhcp-range'} } $subnets->@* if !defined $num_expected_ips;
+
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    foreach my $subnet ($subnets->@*) {
+	$subnet->{type} = "subnet";
+	$subnet->{vnet} = $vnetid;
+	create_subnet($subnet);
+    };
+
+    my $hostname = "testhostname";
+    my $mac = "da:65:8f:18:9b:6f";
+    my $vmid = "999";
+
+    if ($current_ip4) {
+	create_ip({
+	    zone => $zoneid,
+	    vnet => $vnetid,
+	    mac => $mac,
+	    ip => $current_ip4,
+	});
+    }
+
+    if ($current_ip6) {
+	create_ip({
+	    zone => $zoneid,
+	    vnet => $vnetid,
+	    mac => $mac,
+	    ip => $current_ip6,
+	});
+    }
+    my @current_ips = get_ips_from_mac($mac);
+    is ( get_ip4(@current_ips), $current_ip4, "$test_name: setup current IPv4: $current_ip4" ) if defined $current_ip4;
+    is ( get_ip6(@current_ips), $current_ip6, "$test_name: setup current IPv6: $current_ip6" ) if defined $current_ip6;
+
+    eval {
+	nic_start($vnetid, $mac, $hostname, $vmid);
+    };
+
+    if ($@) {
+	fail("$test_name: $@");
+	return;
+    }
+
+    my @ips = get_ips_from_mac($mac);
+    my $num_ips = scalar @ips;
+    is ($num_ips, $num_expected_ips, "$test_name: Expecting $num_expected_ips IPs, found $num_ips");
+    ok ((all { ($_->{vnet} eq $vnetid && $_->{zone} eq $zoneid) } @ips),
+	"$test_name: all IPs in correct vnet and zone"
+    );
+
+    is ( get_ip4(@ips), $current_ip4, "$test_name: still current IPv4: $current_ip4" ) if $current_ip4;
+    is ( get_ip6(@ips), $current_ip6, "$test_name: still current IPv6: $current_ip6" ) if $current_ip6;
+}
+
+run_test(
+    \&test_nic_start,
+    "nic_start no IP, IPv4 without dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    },
+]);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IP, IPv4 without dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    }],
+    "10.0.0.99",
+    undef,
+    1
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv6, IPv6 without dhcp",
+    [{
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    }],
+    undef,
+    "8888::99",
+    1
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start no IP, IPv4 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    },
+]);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IP, IPv4 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }],
+    "10.0.0.99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IP, IPv6 subnet with dhcp",
+    [{
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    }],
+    undef,
+    "8888::99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start IP, IPv4+6 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    },
+]);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv4, IPv4+6 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    }],
+    "10.0.0.99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv6, IPv4+6 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    }],
+    undef,
+    "8888::99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv4+6, IPv4+6 subnets with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    }],
+    "10.0.0.99",
+    "8888::99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv4+6, only IPv4 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    }],
+    "10.0.0.99",
+    "8888::99",
+    2
+);
+
+done_testing();
-- 
2.44.0

[pve-devel] [PATCH v3 pve-network 12/12] tests: remove old Vnets tests

[Stefan Lendl]

The did not work and were primarily testing against internal state.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/test/run_test_vnets.pl | 343 -------------------------------------
 1 file changed, 343 deletions(-)
 delete mode 100755 src/test/run_test_vnets.pl

diff --git a/src/test/run_test_vnets.pl b/src/test/run_test_vnets.pl
deleted file mode 100755
index 65fbfb7..0000000
--- a/src/test/run_test_vnets.pl
+++ /dev/null
@@ -1,343 +0,0 @@
-#!/usr/bin/perl
-
-use strict;
-use warnings;
-
-use lib qw(..);
-use File::Slurp;
-use NetAddr::IP qw(:lower);
-
-use Test::More;
-use Test::MockModule;
-
-use PVE::Network::SDN;
-use PVE::Network::SDN::Zones;
-use PVE::Network::SDN::Controllers;
-use PVE::INotify;
-use JSON;
-
-use Data::Dumper qw(Dumper);
-$Data::Dumper::Sortkeys = 1;
-
-sub read_sdn_config {
-    my ($file) = @_;
-
-    # Read structure back in again
-    open my $in, '<', $file or die $!;
-    my $sdn_config;
-    {
-	local $/; # slurp mode
-	$sdn_config = eval <$in>;
-    }
-    close $in;
-    return $sdn_config;
-}
-
-my @plugins = read_dir('./vnets/', prefix => 1);
-
-foreach my $path (@plugins) {
-
-    my (undef, $testid) = split(/\//, $path);
-
-    print "test: $testid\n";
-    my $sdn_config = read_sdn_config("$path/sdn_config");
-
-    my $pve_sdn_zones;
-    $pve_sdn_zones = Test::MockModule->new('PVE::Network::SDN::Zones');
-    $pve_sdn_zones->mock(
-	config => sub {
-	    return $sdn_config->{zones};
-	},
-    );
-
-    my $pve_sdn_vnets;
-    $pve_sdn_vnets = Test::MockModule->new('PVE::Network::SDN::Vnets');
-    $pve_sdn_vnets->mock(
-	config => sub {
-	    return $sdn_config->{vnets};
-	},
-    );
-
-    my $pve_sdn_subnets;
-    $pve_sdn_subnets = Test::MockModule->new('PVE::Network::SDN::Subnets');
-    $pve_sdn_subnets->mock(
-	config => sub {
-	    return $sdn_config->{subnets};
-	},
-	verify_dns_zone => sub {
-	    return;
-	},
-	add_dns_record => sub {
-	    return;
-	},
-    );
-
-    my $js = JSON->new;
-    $js->canonical(1);
-
-    #test params;
-    #test params;
-    my $subnets = $sdn_config->{subnets}->{ids};
-
-    my $subnetid = (sort keys %{$subnets})[0];
-    my $subnet =
-	PVE::Network::SDN::Subnets::sdn_subnets_config($sdn_config->{subnets}, $subnetid, 1);
-    my $subnet_cidr = $subnet->{cidr};
-    my $iplist = NetAddr::IP->new($subnet_cidr);
-    my $mask = $iplist->masklen();
-    my $ipversion = undef;
-
-    if (Net::IP::ip_is_ipv4($iplist->canon())) {
-	$iplist++; #skip network address for ipv4
-	$ipversion = 4;
-    } else {
-	$ipversion = 6;
-    }
-
-    my $cidr1 = $iplist->canon() . "/$mask";
-    $iplist++;
-    my $cidr2 = $iplist->canon() . "/$mask";
-    my $cidr_outofrange = '8.8.8.8/8';
-
-    my $subnetid2 = (sort keys %{$subnets})[1];
-    my $subnet2 =
-	PVE::Network::SDN::Subnets::sdn_subnets_config($sdn_config->{subnets}, $subnetid2, 1);
-    my $subnet2_cidr = $subnet2->{cidr};
-    my $iplist2 = NetAddr::IP->new($subnet2_cidr);
-    $iplist2++;
-    my $cidr3 = $iplist2->canon() . "/$mask";
-    $iplist2++;
-    my $cidr4 = $iplist2->canon() . "/$mask";
-
-    my $hostname = "myhostname";
-    my $mac = "da:65:8f:18:9b:6f";
-    my $description = "mydescription";
-    my $ipamdb = read_sdn_config("$path/ipam.db");
-
-    my $zone = $sdn_config->{zones}->{ids}->{"myzone"};
-    my $ipam = $zone->{ipam};
-
-    my $plugin;
-    my $sdn_ipam_plugin;
-    if ($ipam) {
-	$plugin = PVE::Network::SDN::Ipams::Plugin->lookup($ipam);
-	$sdn_ipam_plugin = Test::MockModule->new($plugin);
-	$sdn_ipam_plugin->mock(
-	    read_db => sub {
-		return $ipamdb;
-	    },
-	    write_db => sub {
-		my ($cfg) = @_;
-		$ipamdb = $cfg;
-	    },
-	);
-    }
-
-    my $pve_sdn_ipams;
-    $pve_sdn_ipams = Test::MockModule->new('PVE::Network::SDN::Ipams');
-    $pve_sdn_ipams->mock(
-	config => sub {
-	    my $ipam_config = read_sdn_config("$path/ipam_config");
-	    return $ipam_config;
-	},
-    );
-
-    my $vnetid = "myvnet";
-
-    ## add_ip
-    my $test = "add_cidr $cidr1";
-    my $name = "$testid $test";
-    my $result = undef;
-    my $expected = '';
-
-    eval { PVE::Network::SDN::Vnets::add_cidr($vnetid, $cidr1, $hostname, $mac, $description); };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-    ## add_ip
-    $test = "add_already_exist_cidr $cidr1";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = '';
-
-    eval { PVE::Network::SDN::Vnets::add_cidr($vnetid, $cidr1, $hostname, $mac, $description); };
-
-    if ($@) {
-	is(undef, undef, $name);
-    } elsif ($ipam) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-    ## add_ip
-    $test = "add_cidr $cidr2";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = '';
-
-    eval { PVE::Network::SDN::Vnets::add_cidr($vnetid, $cidr2, $hostname, $mac, $description); };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-    ## add_ip
-    $test = "add_ip_out_of_range_subnets $cidr_outofrange";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = '';
-
-    eval {
-	PVE::Network::SDN::Vnets::add_cidr($vnetid, $cidr_outofrange, $hostname, $mac,
-	    $description);
-    };
-
-    if ($@) {
-	is(undef, undef, $name);
-    } else {
-	fail("$name : $@");
-    }
-
-    ## add_ip
-    $test = "add_cidr $cidr4";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = '';
-
-    eval { PVE::Network::SDN::Vnets::add_cidr($vnetid, $cidr4, $hostname, $mac, $description); };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-    $test = "find_next_free_cidr_in_second_subnet ($cidr3)";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = $ipam ? $cidr3 : undef;
-
-    eval {
-	$result =
-	    PVE::Network::SDN::Vnets::add_next_free_cidr($vnetid, $hostname, $mac, $description);
-    };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is($result, $expected, $name);
-    }
-
-    $test = "del_cidr $cidr1";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = undef;
-
-    eval { $result = PVE::Network::SDN::Vnets::del_cidr($vnetid, $cidr1, $hostname); };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-    $test = "del_cidr $cidr3";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = undef;
-
-    eval { $result = PVE::Network::SDN::Vnets::del_cidr($vnetid, $cidr3, $hostname); };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-    $test = "del_cidr not exist $cidr1";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = undef;
-
-    eval { $result = PVE::Network::SDN::Vnets::del_cidr($vnetid, $cidr1, $hostname); };
-
-    if ($@) {
-	is(undef, undef, $name);
-    } elsif ($ipam) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-    $test = "del_cidr outofrange $cidr_outofrange";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = undef;
-
-    eval { $result = PVE::Network::SDN::Vnets::del_cidr($vnetid, $cidr_outofrange, $hostname); };
-
-    if ($@) {
-	is(undef, undef, $name);
-    } else {
-	fail("$name : $@");
-    }
-
-    $test = "find_next_free_cidr_in_first_subnet ($cidr1)";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = $ipam ? $cidr1 : undef;
-
-    eval {
-	$result =
-	    PVE::Network::SDN::Vnets::add_next_free_cidr($vnetid, $hostname, $mac, $description);
-    };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is($result, $expected, $name);
-    }
-
-    $test = "update_cidr $cidr1";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = undef;
-
-    eval {
-	$result = PVE::Network::SDN::Vnets::update_cidr($vnetid, $cidr1, $hostname, $hostname, $mac,
-	    $description);
-    };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-    $test = "update_cidr deleted $cidr3";
-    $name = "$testid $test";
-    $result = undef;
-    $expected = undef;
-
-    eval {
-	$result = PVE::Network::SDN::Vnets::update_cidr($vnetid, $cidr1, $hostname, $hostname, $mac,
-	    $description);
-    };
-
-    if ($@) {
-	fail("$name : $@");
-    } else {
-	is(undef, undef, $name);
-    }
-
-}
-
-done_testing();
-
-- 
2.44.0

Re: [pve-devel] [PATCH v3 pve-network 00/12] SDN: Testing VNets as a blackbox and fixing found bugs

[Stefan Hanreich]

Already mentioned in the commits, but gave this another spin today on my
test VM and everything seems to work as intended:

* Rollbacking on failure
* Allocating an IPv6 IP on start, if there is already an IPv4 allocated
* Ignoring subnets without any configured DHCP-Range

Also, the pre-existing DHCP functionality also still seems to work as
intended.

[pve-devel] applied: [PATCH v3 pve-network 00/12] SDN: Testing VNets as a blackbox and fixing found bugs

[Thomas Lamprecht]

Am 05/04/2024 um 15:17 schrieb Stefan Lendl:
> This add several tests for SDN VNets.
> State setup as well as testing results is done only via the API to test on the
> API boundaries and not against the internal state. Internal state and config
> files are mocked to avoid requiring access to system files or pmxcfs.
> 
> The first 4 commits fix functionality, identified as bugs thanks to the tests.
> 
> The next 7 commits extract various functions to allow mocking them in the
> tests. The tests are then added as the test_vnets_blackbox test.
> The last commit removes the old vnets tests which are not working anyway.
> 
> Tests validate the events of a nic joining a VNet or a nic staring on a VNet.
> These events are tested with with different subnet configurations.
> Mainly for IPv4 and/or IPv6 configurations and odd combinations.
> Further descriptions in the commit.
> 
> Differences v2 -> v3:
> * Fix functionalitiy in VNet and Subnet so all tests pass
>   Thanks @s.hanreich for lots of testing
> * Update and add more tests
> * Make it build in sbuild
> 
> Differences v1 -> v2:
> * Add tests that expect a failure when no IP can be allocated
> * Removed commented out debug stuff
> 
> 
> Stefan Hanreich (2):
>   sdn: dhcp: only consider subnets that have dhcp-range configured
>   sdn: dhcp: rollback allocated ips on failure
> 
> Stefan Lendl (10):
>   sdn: dhcp: get next free ip for a specific IP version
>   sdn: dhcp: request both IPv4 and IPv6 addresses on VM start
>   sdn: zones: extract function that reads datacenter config
>   dns: dnsmasq: extract function to systemctl command.
>   sdn: dnsmasq: extract function that generates the ethers file path
>   sdn: dnsmasq: extract function that updates dnsmasq lease via dbus
>   sdn: api: extract function that creates the sdn directory.
>   debian: blackbox tests depend on libpve-access-control at build
>   tests: test VNets functionality as a blackbox
>   tests: remove old Vnets tests
> 
>  debian/control                            |   1 +
>  src/PVE/API2/Network/SDN/Zones.pm         |   6 +-
>  src/PVE/Network/SDN/Dhcp/Dnsmasq.pm       |  47 +-
>  src/PVE/Network/SDN/Subnets.pm            |   2 +-
>  src/PVE/Network/SDN/Vnets.pm              |  29 +-
>  src/PVE/Network/SDN/Zones/EvpnPlugin.pm   |   3 +-
>  src/PVE/Network/SDN/Zones/Plugin.pm       |   5 +
>  src/PVE/Network/SDN/Zones/SimplePlugin.pm |   2 +-
>  src/test/Makefile                         |   5 +-
>  src/test/run_test_vnets.pl                | 343 ---------
>  src/test/run_test_vnets_blackbox.pl       | 894 ++++++++++++++++++++++
>  11 files changed, 962 insertions(+), 375 deletions(-)
>  delete mode 100755 src/test/run_test_vnets.pl
>  create mode 100755 src/test/run_test_vnets_blackbox.pl
> 


applied series, thanks!