From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 0354E1FF09C for ; Mon, 02 Nov 2026 12:07:57 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 69A10217B6; Mon, 02 Nov 2026 12:07:07 +0100 (CET) From: Hannes Laimer To: pve-devel@lists.proxmox.com Subject: [PATCH pve-network 08/10] sdn: ipam: keep the MAC cache per vnet Date: Mon, 2 Nov 2026 12:06:02 +0100 Message-ID: <20261102110604.915693-9-h.laimer@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261102110604.915693-1-h.laimer@proxmox.com> References: <20261102110604.915693-1-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793617581828 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.368 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: VX7MMUT5L5EYPP524CDQVNZSVPTLCWQX X-Message-ID-Hash: VX7MMUT5L5EYPP524CDQVNZSVPTLCWQX X-MailFrom: h.laimer@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: The same MAC can sit on several vnets, but the cache of its addresses was keyed by the MAC alone. A mapping created or edited on one vnet changed the cached answer on every other vnet as well. A guest with that MAC on another vnet then got a reservation for an address outside its own vnet. The cache now holds the addresses per vnet, and a zone's dnsmasq reservations hold a line for each vnet a MAC sits on, each written from the cache of that vnet. A MAC matches in any spelling. Entries from before, keyed by the MAC alone, stay as written, since nodes not yet upgraded still use them. They answer for a vnet only with addresses in its subnets, so a running guest keeps its reservation across the upgrade. A node not yet upgraded does not update the per-vnet entries, so a release checks that the record still belongs to its MAC and leaves it alone otherwise. The API refuses to delete such a mapping. Signed-off-by: Hannes Laimer --- src/PVE/API2/Network/SDN/Ips.pm | 5 +- src/PVE/Network/SDN/Dhcp.pm | 2 +- src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 125 ++++++------ src/PVE/Network/SDN/Dhcp/Plugin.pm | 6 +- src/PVE/Network/SDN/Ipams.pm | 124 ++++++------ src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 11 +- src/PVE/Network/SDN/Ipams/PVEPlugin.pm | 13 +- src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm | 16 +- src/PVE/Network/SDN/Ipams/Plugin.pm | 4 +- src/PVE/Network/SDN/Subnets.pm | 11 +- src/PVE/Network/SDN/Vnets.pm | 12 +- src/test/run_test_ipams.pl | 37 ++++ src/test/run_test_vnets_blackbox.pl | 217 +++++++++++++++++---- 13 files changed, 404 insertions(+), 179 deletions(-) diff --git a/src/PVE/API2/Network/SDN/Ips.pm b/src/PVE/API2/Network/SDN/Ips.pm index b1faee71c6ba..ecab7fc99b6c 100644 --- a/src/PVE/API2/Network/SDN/Ips.pm +++ b/src/PVE/API2/Network/SDN/Ips.pm @@ -62,11 +62,14 @@ __PACKAGE__->register_method({ my $mac = extract_param($param, 'mac'); my $ip = extract_param($param, 'ip'); - eval { PVE::Network::SDN::Vnets::del_ip($vnet, $ip, '', $mac); }; + my $released = eval { PVE::Network::SDN::Vnets::del_ip($vnet, $ip, '', $mac); }; die "$@\n" if $@; PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac); + # the cache entry is cleared, the IPAM record belongs to another MAC + die "IP '$ip' is not mapped to $mac\n" if defined($released) && !$released; + return undef; }, }); diff --git a/src/PVE/Network/SDN/Dhcp.pm b/src/PVE/Network/SDN/Dhcp.pm index 4650270df09f..28e83d11630b 100644 --- a/src/PVE/Network/SDN/Dhcp.pm +++ b/src/PVE/Network/SDN/Dhcp.pm @@ -41,7 +41,7 @@ sub update_mapping { return if defined($zone->{nodes}) && !$zone->{nodes}->{ PVE::INotify::nodename() }; PVE::Network::SDN::Dhcp::Plugin->lookup($zone->{dhcp}) - ->update_ip_mapping($zoneid, $mac); + ->update_ip_mapping($zoneid, $vnetid, $mac); }; # the records are written, so a failed update only warns warn "could not update dhcp mapping for $mac: $@" if $@; diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm index 4767fb5162ad..1acc7c42318b 100644 --- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm +++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm @@ -13,7 +13,6 @@ use Net::DBus; use PVE::Cluster; use PVE::RESTEnvironment qw(log_warn); -use PVE::Network::SDN::Ipams; my $DNSMASQ_CONFIG_ROOT = '/etc/dnsmasq.d'; my $DNSMASQ_DEFAULT_ROOT = '/etc/default'; @@ -54,98 +53,90 @@ sub update_lease { } sub update_ip_mapping { - my ($class, $dhcpid, $mac) = @_; + my ($class, $dhcpid, $vnetid, $mac) = @_; $mac = lc($mac); my $ethers_file = ethers_file($dhcpid); my $ethers_tmp_file = "$ethers_file.tmp"; - my ($ip4, $ip6); - my $reservation = undef; + my $ip4; my $changed = undef; my $rewriteFn = sub { - # the lines are decided from the cache, which this process may hold an - # old read of, and the local lock does not refresh it + # the lines are decided from the cache and the applied config, which + # the local lock does not refresh PVE::Cluster::cfs_update(); - # read under the ethers lock. A snapshot from before it would sweep a - # line another push wrote meanwhile, and an older push would put its - # addresses back - - my $macs = PVE::Network::SDN::Ipams::read_macdb()->{macs}; - - my $entry = $macs->{$mac}; - ($ip4, $ip6) = ($entry->{ip4}, $entry->{ip6}) if $entry; - if ($ip4 || $ip6) { - $reservation = $mac; - $reservation .= ",$ip4" if $ip4; - $reservation .= ",[$ip6]" if $ip6; + # the file holds a line per vnet a MAC sits on, and a line belongs to + # the vnet of the applied subnet that holds its address + my $vnets = PVE::Network::SDN::Zones::get_vnets($dhcpid, 1) // {}; + my $subnets = PVE::Network::SDN::Vnets::get_subnets(undef, 1) // {}; + $subnets = { + map { $_ => $subnets->{$_} } grep { $vnets->{ $subnets->{$_}->{vnet} } } + keys %$subnets + }; + my $vnet_of = sub { + my ($ip) = @_; + my (undef, $subnet) = + eval { PVE::Network::SDN::Subnets::find_ip_subnet($ip, $subnets) }; + return $subnet ? $subnet->{vnet} : undef; + }; + + # read under the ethers lock, a read from before it would undo a push + # that ran meanwhile. The lines of other MACs are checked against the + # cache alone, so under the lock only the changed MAC may ask the IPAM + my $reservation = sub { + my ($vnet, $line_mac, $cached_only) = @_; + my ($line_ip4, $line_ip6) = + PVE::Network::SDN::Vnets::get_ips_from_mac($vnet, $line_mac, 1, $cached_only); + return (undef, undef) if !$line_ip4 && !$line_ip6; + my $line = $line_mac; + $line .= ",$line_ip4" if $line_ip4; + $line .= ",[$line_ip6]" if $line_ip6; # an external IPAM spells addresses its own way, while the file holds one spelling - $reservation = lc($reservation); - } + return (lc($line), $line_ip4); + }; file_set_contents($ethers_file, '') if !-e $ethers_file; open(my $in, '<', $ethers_file) or die "Could not open file '$ethers_file' $!\n"; open(my $out, '>', $ethers_tmp_file) or die "Could not open file '$ethers_tmp_file' $!\n"; - my $kept = undef; - + my %written; while (my $line = <$in>) { chomp($line); next if $line !~ m/\S/; - my ($parsed_mac, $parsed_ip1, $parsed_ip2) = split(/,/, $line); - # a line from before the file was written in lower case keeps its writer's spelling + my ($parsed_mac, $parsed_ip) = split(/,/, $line); $parsed_mac = lc($parsed_mac); - - # the MAC's own reservation is rewritten. An unchanged one stays in place - if ($parsed_mac eq $mac) { - if (defined($reservation) && !$kept && lc($line) eq $reservation) { - $kept = 1; - print $out "$line\n"; - } else { - $changed = 1; - } - next; - } - - my ($parsed_ip4, $parsed_ip6); - if ($parsed_ip2) { - $parsed_ip4 = $parsed_ip1; - $parsed_ip6 = $parsed_ip2; - } elsif (defined($parsed_ip1) && Net::IP::ip_is_ipv4($parsed_ip1)) { - $parsed_ip4 = $parsed_ip1; - } elsif (defined($parsed_ip1)) { - $parsed_ip6 = $parsed_ip1; - } - $parsed_ip6 = $1 if $parsed_ip6 && $parsed_ip6 =~ m/\[(\S+)\]/; - # a line from before addresses were lower-cased may spell one in upper case - $parsed_ip6 = lc($parsed_ip6) if $parsed_ip6; - - # a line that no longer matches the cache is replaced by the cache's - # current one. Dropping it left that guest unknown to dnsmasq until - # its next start - my $current = $macs->{$parsed_mac}; - if ( - !$current - || ($parsed_ip4 && $current->{ip4} && $current->{ip4} ne $parsed_ip4) - || ($parsed_ip6 && $current->{ip6} && lc($current->{ip6}) ne $parsed_ip6) - ) { + $parsed_ip = $1 if defined($parsed_ip) && $parsed_ip =~ m/\[(\S+)\]/; + my $vnet = defined($parsed_ip) ? $vnet_of->($parsed_ip) : undef; + my $key = defined($vnet) ? "$parsed_mac,$vnet" : undef; + + # a line that no longer matches is replaced, since dropping it left + # that guest unknown to dnsmasq until its next start + my ($want) = + eval { $key && !$written{$key} ? $reservation->($vnet, $parsed_mac, 1) : () }; + if ($@) { + warn "could not read the reservation of $parsed_mac: $@"; + print $out "$line\n"; + } elsif (!defined($want)) { $changed = 1; - next if !$current; - my $replacement = $parsed_mac; - $replacement .= ",$current->{ip4}" if $current->{ip4}; - $replacement .= ",[$current->{ip6}]" if $current->{ip6}; - print $out lc($replacement) . "\n"; next; + } elsif (lc($line) ne $want) { + $changed = 1; + print $out "$want\n"; + } else { + print $out "$line\n"; } - - print $out "$line\n"; + $written{$key}++; } - if (defined($reservation) && !$kept) { - print $out "$reservation\n"; + # the changed MAC's own line may have been dropped above, since only + # the cache was asked for it, so it is written from the IPAM's answer + my $want; + ($want, $ip4) = $reservation->($vnetid, $mac); + if (defined($want) && !$written{"$mac,$vnetid"}) { + print $out "$want\n"; $changed = 1; } diff --git a/src/PVE/Network/SDN/Dhcp/Plugin.pm b/src/PVE/Network/SDN/Dhcp/Plugin.pm index a3a5ae24403f..3484df19b6dc 100644 --- a/src/PVE/Network/SDN/Dhcp/Plugin.pm +++ b/src/PVE/Network/SDN/Dhcp/Plugin.pm @@ -22,10 +22,10 @@ sub private { return $defaultData; } -# the MAC's records changed, so make the backend serve the current ones. With no -# record left its reservation goes +# the MAC's records on the vnet changed, so make the backend serve the current +# ones. With no record left its reservation on the vnet goes sub update_ip_mapping { - my ($class, $dhcpid, $mac) = @_; + my ($class, $dhcpid, $vnetid, $mac) = @_; die 'implement in sub class'; } diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm index 9a31eaf39f58..5b2217c45242 100644 --- a/src/PVE/Network/SDN/Ipams.pm +++ b/src/PVE/Network/SDN/Ipams.pm @@ -35,40 +35,10 @@ sub json_writer { return encode_json($data); } -# Writers spell a MAC their own way, the generated ones in upper case and the -# API in either, and nodes not yet upgraded look an entry up under the spelling -# their caller hands them, so the file keeps its keys as written. Readers get a -# view with every MAC in lower case. Two spellings that disagree on an address -# are left out of it, since one of them is stale and nothing here says which, -# and so is an entry without an address. The next lookup then asks the IPAM -my sub fold_macdb { - my ($db) = @_; - - my $macs = {}; - my %clash; - for my $key (sort keys %{ $db->{macs} // {} }) { - my $entry = $db->{macs}->{$key}; - for my $family (grep { defined($entry->{$_}) } qw(ip4 ip6)) { - my $seen = $macs->{ lc($key) }->{$family}; - $clash{ lc($key) } = 1 if defined($seen) && lc($seen) ne lc($entry->{$family}); - $macs->{ lc($key) }->{$family} //= $entry->{$family}; - } - } - delete $macs->{$_} for keys %clash; - return { %$db, macs => $macs }; -} - -# the file as written, which the writers change and write back -sub read_macdb_file { - my () = @_; - - return cfs_read_file($macdb_filename); -} - sub read_macdb { my () = @_; - return fold_macdb(read_macdb_file()); + return cfs_read_file($macdb_filename); } sub write_macdb { @@ -77,25 +47,56 @@ sub write_macdb { cfs_write_file($macdb_filename, $data); } -# a write goes to every spelling the file holds for the MAC, so each reader -# finds it under its own, and a MAC new to the file is written in lower case -my sub set_cache_entry { - my ($db, $mac, $family, $ip) = @_; +# the map under the MAC alone keeps its keys as written, since nodes not yet +# upgraded still use it under the MAC as their caller spelled it. Spellings +# that disagree on an address are both left out, since one of them is stale +my sub old_cache_entry { + my ($db, $mac, $subnets) = @_; - my $macs = $db->{macs} //= {}; - my @keys = grep { lc($_) eq lc($mac) } keys %$macs; - $macs->{$_}->{$family} = $ip for (@keys ? @keys : (lc($mac))); + my $entry = {}; + my $macs = $db->{macs} // {}; + for my $key (grep { lc($_) eq lc($mac) } sort keys %$macs) { + for my $family (qw(ip4 ip6)) { + my $ip = $macs->{$key}->{$family} // next; + return undef if defined($entry->{$family}) && lc($entry->{$family}) ne lc($ip); + $entry->{$family} = $ip; + } + } + for my $family (keys %$entry) { + delete $entry->{$family} + if !eval { PVE::Network::SDN::Subnets::find_ip_subnet($entry->{$family}, $subnets) }; + } + return %$entry ? $entry : undef; +} + +# an entry from before the cache was kept per vnet fills the families a +# vnet's entry does not have. Where the two disagree, a node still writing +# one map may have moved the address since, so the lookup is a miss +my sub cache_entry { + my ($db, $mac, $vnetid, $subnets) = @_; + + my $entry = (($db->{vnets} // {})->{$vnetid} // {})->{ lc($mac) }; + $entry = undef if $entry && !defined($entry->{ip4}) && !defined($entry->{ip6}); + my $old = old_cache_entry($db, $mac, $subnets); + return undef if !$entry && !$old; + for my $family (qw(ip4 ip6)) { + my ($ip, $old_ip) = ($entry->{$family}, $old->{$family}); + return undef if defined($ip) && defined($old_ip) && lc($ip) ne lc($old_ip); + } + return { map { $_ => $entry->{$_} // $old->{$_} } qw(ip4 ip6) }; } sub add_cache_mac_ip { - my ($mac, $ip) = @_; + my ($vnetid, $mac, $ip) = @_; + $mac = lc($mac); + my $family = Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6'; cfs_lock_file( $macdb_filename, undef, sub { - my $db = read_macdb_file(); - set_cache_entry($db, $mac, Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6', $ip); + my $db = read_macdb(); + $db->{vnets}->{$vnetid}->{$mac}->{$family} = $ip; write_macdb($db); }, ); @@ -103,19 +104,29 @@ sub add_cache_mac_ip { } sub del_cache_mac_ip { - my ($mac, $ip) = @_; + my ($vnetid, $mac, $ip) = @_; + $mac = lc($mac); + my $family = Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6'; cfs_lock_file( $macdb_filename, undef, sub { - my $db = read_macdb_file(); - my $family = Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6'; + my $db = read_macdb(); + my $vnet_entries = ($db->{vnets} // {})->{$vnetid}; + if (my $entry = $vnet_entries ? $vnet_entries->{$mac} : undef) { + delete $entry->{$family}; + delete $vnet_entries->{$mac} if !%$entry; + delete $db->{vnets}->{$vnetid} if !%$vnet_entries; + delete $db->{vnets} if !%{ $db->{vnets} }; + } + # the old map must not hand a released address back my $macs = $db->{macs} // {}; - for my $key (grep { lc($_) eq lc($mac) } keys %$macs) { - delete $macs->{$key}->{$family}; - delete $macs->{$key} - if !defined($macs->{$key}->{ip4}) && !defined($macs->{$key}->{ip6}); + for my $key (grep { lc($_) eq $mac } keys %$macs) { + my $old = $macs->{$key}; + next if lc($old->{$family} // '') ne lc($ip); + delete $old->{$family}; + delete $macs->{$key} if !defined($old->{ip4}) && !defined($old->{ip6}); } write_macdb($db); }, @@ -169,15 +180,13 @@ sub complete_sdn_vnet { } sub get_ips_from_mac { - my ($mac, $zoneid, $zone, $subnets) = @_; + my ($vnetid, $mac, $zoneid, $zone, $subnets, $cached_only) = @_; PVE::Cluster::cfs_update(); # the lock refreshes, so the snapshot must too my $version = PVE::Cluster::cfs_file_version($macdb_filename); - my $cached = read_macdb()->{macs}->{ lc($mac) }; - # an entry without an address was left behind by a lookup that missed, and - # says nothing about its MAC - return ($cached->{ip4}, $cached->{ip6}) - if $cached && (defined($cached->{ip4}) || defined($cached->{ip6})); + my $cached = cache_entry(read_macdb(), $mac, $vnetid, $subnets); + return ($cached->{ip4}, $cached->{ip6}) if $cached; + return if $cached_only; my $plugin_config = get_plugin_config($zone); my $plugin = PVE::Network::SDN::Ipams::Plugin->lookup($plugin_config->{type}); @@ -195,9 +204,10 @@ sub get_ips_from_mac { return if (PVE::Cluster::cfs_file_version($macdb_filename) // '') ne ($version // ''); - my $db = read_macdb_file(); - set_cache_entry($db, $mac, 'ip4', $ip4) if defined($ip4); - set_cache_entry($db, $mac, 'ip6', $ip6) if defined($ip6); + my $db = read_macdb(); + my $entry = $db->{vnets}->{$vnetid}->{ lc($mac) } //= {}; + $entry->{ip4} = $ip4 if defined($ip4); + $entry->{ip6} = $ip6 if defined($ip6); write_macdb($db); }, ); diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm index 01e7f347d796..82c9e80b6c6e 100644 --- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm +++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm @@ -360,7 +360,7 @@ sub add_range_next_freeip { } sub del_ip { - my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr) = @_; + my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr, $mac) = @_; return if !$ip; @@ -369,6 +369,15 @@ sub del_ip { warn "could not find id for ip $ip"; return; } + if ($mac) { + my $record = + eval { netbox_api_request($plugin_config, "GET", "/ipam/ip-addresses/$ip_id/") }; + my ($owner) = (($record // {})->{description} // '') =~ m/^mac:(\S+)$/; + if ($owner && lc($owner) ne lc($mac)) { + warn "not releasing IP '$ip' for $mac, it is held by $owner\n"; + return 0; + } + } eval { netbox_api_request($plugin_config, "DELETE", "/ipam/ip-addresses/$ip_id/"); }; if ($@) { diff --git a/src/PVE/Network/SDN/Ipams/PVEPlugin.pm b/src/PVE/Network/SDN/Ipams/PVEPlugin.pm index 43d0a3daaf9a..86e594a4c604 100644 --- a/src/PVE/Network/SDN/Ipams/PVEPlugin.pm +++ b/src/PVE/Network/SDN/Ipams/PVEPlugin.pm @@ -249,11 +249,12 @@ sub add_range_next_freeip { } sub del_ip { - my ($class, $plugin_config, $subnetid, $subnet, $ip) = @_; + my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr, $mac) = @_; my $cidr = $subnet->{cidr}; my $zone = $subnet->{zone}; + my $released = 1; cfs_lock_file( $ipamdb_file, undef, @@ -271,11 +272,18 @@ sub del_ip { warn "IP '$ip' does not exist in IPAM DB\n"; return; } + my $owner = $dbsubnet->{ips}->{$ip}->{mac}; + if ($mac && $owner && lc($owner) ne lc($mac)) { + warn "not releasing IP '$ip' for $mac, it is held by $owner\n"; + $released = 0; + return; + } delete $dbsubnet->{ips}->{$ip}; write_db($db); }, ); die "$@" if $@; + return $released; } sub get_ips_from_mac { @@ -297,7 +305,8 @@ sub get_ips_from_mac { my $ips = $dbzone->{subnets}->{$subnet}->{ips} // next; for my $ip (keys %$ips) { my $ipobject = $ips->{$ip}; - if ($ipobject->{mac} && $ipobject->{mac} eq $mac) { + # a record keeps the MAC as its writer spelled it + if ($ipobject->{mac} && lc($ipobject->{mac}) eq lc($mac)) { if (Net::IP::ip_is_ipv4($ip)) { $ip4 = $ip; } else { diff --git a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm index c3f014d45585..e1b30e293ab3 100644 --- a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm +++ b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm @@ -237,7 +237,7 @@ sub add_range_next_freeip { } sub del_ip { - my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr) = @_; + my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr, $mac) = @_; return if !$ip; @@ -248,6 +248,18 @@ sub del_ip { my $ip_id = get_ip_id($url, $ip, $headers); return if !$ip_id; + if ($mac) { + my $record = eval { + PVE::Network::SDN::api_request( + "GET", "$url/addresses/$ip_id/", $headers, undef, $fingerprint, + ); + }; + my $owner = (($record // {})->{data} // {})->{mac}; + if ($owner && lc($owner) ne lc($mac)) { + warn "not releasing IP '$ip' for $mac, it is held by $owner\n"; + return 0; + } + } eval { PVE::Network::SDN::api_request( @@ -257,6 +269,8 @@ sub del_ip { if ($@) { die "error delete ip $ip: $@" if !$noerr; } + + return 1; } sub get_ips_from_mac { diff --git a/src/PVE/Network/SDN/Ipams/Plugin.pm b/src/PVE/Network/SDN/Ipams/Plugin.pm index 14f8fe9e67fe..6e23d5434d4b 100644 --- a/src/PVE/Network/SDN/Ipams/Plugin.pm +++ b/src/PVE/Network/SDN/Ipams/Plugin.pm @@ -132,8 +132,10 @@ sub add_range_next_freeip { die "please implement inside plugin"; } +# a MAC names the guest the address is released for, a record holding the +# address for another MAC is then left alone and 0 returned sub del_ip { - my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr) = @_; + my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr, $mac) = @_; die "please implement inside plugin"; } diff --git a/src/PVE/Network/SDN/Subnets.pm b/src/PVE/Network/SDN/Subnets.pm index 1f27fac45db1..4d431f1bd27a 100644 --- a/src/PVE/Network/SDN/Subnets.pm +++ b/src/PVE/Network/SDN/Subnets.pm @@ -263,7 +263,7 @@ sub add_next_free_ip { die $@ if $@; - eval { PVE::Network::SDN::Ipams::add_cache_mac_ip($mac, $ip); }; + eval { PVE::Network::SDN::Ipams::add_cache_mac_ip($subnet->{vnet}, $mac, $ip); }; warn $@ if $@; } @@ -322,7 +322,7 @@ sub add_ip { }; die $@ if $@; - eval { PVE::Network::SDN::Ipams::add_cache_mac_ip($mac, $ip) if $mac; }; + eval { PVE::Network::SDN::Ipams::add_cache_mac_ip($subnet->{vnet}, $mac, $ip) if $mac; }; warn $@ if $@; } @@ -410,14 +410,15 @@ sub del_ip { verify_dns_zone($reversednszone, $reversedns); } + my $released; if ($ipamid) { my $ipam_cfg = PVE::Network::SDN::Ipams::config(); my $plugin_config = $ipam_cfg->{ids}->{$ipamid}; my $plugin = PVE::Network::SDN::Ipams::Plugin->lookup($plugin_config->{type}); - $plugin->del_ip($plugin_config, $subnetid, $subnet, $ip); + $released = $plugin->del_ip($plugin_config, $subnetid, $subnet, $ip, undef, $mac); if ($mac) { - eval { PVE::Network::SDN::Ipams::del_cache_mac_ip($mac, $ip) }; + eval { PVE::Network::SDN::Ipams::del_cache_mac_ip($subnet->{vnet}, $mac, $ip) }; warn $@ if $@; } } @@ -431,6 +432,8 @@ sub del_ip { if ($@) { warn $@; } + + return $released; } 1; diff --git a/src/PVE/Network/SDN/Vnets.pm b/src/PVE/Network/SDN/Vnets.pm index b1a2192ac767..d8824218d9ef 100644 --- a/src/PVE/Network/SDN/Vnets.pm +++ b/src/PVE/Network/SDN/Vnets.pm @@ -191,18 +191,20 @@ sub del_ip { } sub get_ips_from_mac { - my ($vnetid, $mac) = @_; + my ($vnetid, $mac, $running, $cached_only) = @_; - my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid); + my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid, $running); return if !$vnet; my $zoneid = $vnet->{zone}; - my $zone = PVE::Network::SDN::Zones::get_zone($zoneid); + my $zone = PVE::Network::SDN::Zones::get_zone($zoneid, $running); return if !$zone->{ipam} || !$zone->{dhcp}; - my $subnets = PVE::Network::SDN::Vnets::get_subnets($vnetid); - return PVE::Network::SDN::Ipams::get_ips_from_mac($mac, $zoneid, $zone, $subnets); + my $subnets = PVE::Network::SDN::Vnets::get_subnets($vnetid, $running); + return PVE::Network::SDN::Ipams::get_ips_from_mac( + $vnetid, $mac, $zoneid, $zone, $subnets, $cached_only, + ); } sub del_ips_from_mac { diff --git a/src/test/run_test_ipams.pl b/src/test/run_test_ipams.pl index f9c44fe1a3b9..0d3fed120d3f 100755 --- a/src/test/run_test_ipams.pl +++ b/src/test/run_test_ipams.pl @@ -219,5 +219,42 @@ foreach my $path (@plugins) { } +# an address released for a MAC stays when the external IPAM records it +# for another MAC +{ + my $subnet = { zone => 'z', cidr => '10.0.0.0/24' }; + my $owner; + my @deletes; + my $sdn_module = Test::MockModule->new("PVE::Network::SDN"); + $sdn_module->mock( + api_request => sub { + my ($method, $url) = @_; + push @deletes, $url if $method eq 'DELETE'; + return { + description => "mac:$owner", + data => { mac => $owner }, + }; + }, + ); + my $netbox = Test::MockModule->new('PVE::Network::SDN::Ipams::NetboxPlugin'); + $netbox->mock(subnet_prefix => sub { return (1, 2); }, get_ip_id => sub { return 3; }); + my $phpipam = Test::MockModule->new('PVE::Network::SDN::Ipams::PhpIpamPlugin'); + $phpipam->mock(subnet_id => sub { return 7; }, get_ip_id => sub { return 3; }); + + for my $plugin ( + 'PVE::Network::SDN::Ipams::NetboxPlugin', + 'PVE::Network::SDN::Ipams::PhpIpamPlugin', + ) { + my $config = { url => 'u', token => 't' }; + @deletes = (); + $owner = 'da:65:8f:18:9b:70'; + $plugin->del_ip($config, 'z-10.0.0.0-24', $subnet, '10.0.0.50', 1, 'DA:65:8F:18:9B:6F'); + is(scalar(@deletes), 0, "$plugin keeps the record of another MAC"); + $owner = 'da:65:8f:18:9b:6f'; + $plugin->del_ip($config, 'z-10.0.0.0-24', $subnet, '10.0.0.50', 1, 'DA:65:8F:18:9B:6F'); + is(scalar(@deletes), 1, "$plugin releases the MAC's own record"); + } +} + done_testing(); diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl index 27b1295ed556..8f703cec9968 100755 --- a/src/test/run_test_vnets_blackbox.pl +++ b/src/test/run_test_vnets_blackbox.pl @@ -194,7 +194,7 @@ $mocked_sdn_ipams->mock( my ($cfg) = @_; $test_state->{ipam_config} = $cfg; }, - read_macdb_file => sub { + read_macdb => sub { return PVE::Network::SDN::Ipams::json_reader(undef, encode_json($test_state->{macdb})); }, write_macdb => sub { @@ -491,74 +491,97 @@ sub test_ipam_cache_lookup { # an entry holding no address, left behind by an earlier miss, does not # hide a record created since create_ip({ zone => $zoneid, vnet => $vnetid, mac => $mac, ip => "10.0.0.50" }); - $test_state->{macdb}->{macs}->{$mac} = { ip4 => undef, ip6 => undef }; + $test_state->{macdb} = { macs => { $mac => { ip4 => undef, ip6 => undef } } }; my @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); is($ips[0], "10.0.0.50", "$test_name: an address-less entry counts as a miss"); - is( - $test_state->{macdb}->{macs}->{$mac}->{ip4}, - "10.0.0.50", - "$test_name: the answer is cached", + is_deeply( + $test_state->{macdb}, + { + macs => { $mac => { ip4 => undef, ip6 => undef } }, + vnets => { $vnetid => { $mac => { ip4 => "10.0.0.50" } } }, + }, + "$test_name: the answer is cached, the old entry left as written", ); # a write to the cache between the miss and the fill, which the file's # version shows, keeps the IPAM's answer out of the cache - delete $test_state->{macdb}->{macs}->{$mac}; + delete $test_state->{macdb}->{vnets}; my $version = 0; $mocked_pve_cluster_obj->mock(cfs_file_version => sub { return $version++ }); @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); $mocked_pve_cluster_obj->unmock('cfs_file_version'); is($ips[0], "10.0.0.50", "$test_name: the lookup still answers"); - ok(!exists $test_state->{macdb}->{macs}->{$mac}, - "$test_name: but leaves a changed cache alone"); + ok(!exists $test_state->{macdb}->{vnets}, "$test_name: but leaves a changed cache alone"); - # an entry written under the config's spelling, as a node not yet upgraded - # writes it, is read and released like any other and keeps its key + # an entry written under the config's spelling before the cache was keyed in + # lower case is read and released like any other my $other = "da:65:8f:18:9b:70"; $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => '10.0.0.150' }, uc($other) => { ip4 => '10.0.0.170' }, }; my ($ip4) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); - is($ip4, '10.0.0.150', "$test_name: an entry is found however it is spelled"); - PVE::Network::SDN::Ipams::del_cache_mac_ip($mac, '10.0.0.150'); + is($ip4, '10.0.0.150', "$test_name: an old entry is found however it is spelled"); + PVE::Network::SDN::Ipams::del_cache_mac_ip($vnetid, $mac, '10.0.0.150'); is_deeply( $test_state->{macdb}->{macs}, { uc($other) => { ip4 => '10.0.0.170' } }, "$test_name: and released, the other entry left as written", ); - # a write in another spelling goes to the spelling the file holds + # a write by this code leaves an old entry as a node not yet upgraded + # wrote it, so that node still finds it under its own spelling $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => '10.0.0.150' } }; - PVE::Network::SDN::Ipams::add_cache_mac_ip($mac, 'fd00::150'); + PVE::Network::SDN::Ipams::add_cache_mac_ip($vnetid, uc($mac), 'fd00::150'); is_deeply( $test_state->{macdb}->{macs}, - { uc($mac) => { ip4 => '10.0.0.150', ip6 => 'fd00::150' } }, - "$test_name: a write keeps the key as written", + { uc($mac) => { ip4 => '10.0.0.150' } }, + "$test_name: a write leaves the old entry as written", + ); + is_deeply( + [PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac)], + ['10.0.0.150', 'fd00::150'], + "$test_name: holding both addresses", ); - # a lookup past an address-less entry asks the IPAM and fills the entry - # under its spelling - $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => undef, ip6 => undef } }; + # a vnet's entry and an old one that disagree are not trusted either, a + # node not yet upgraded may have moved the address since + $test_state->{macdb} = { + macs => { $mac => { ip4 => '10.0.0.151' } }, + vnets => { $vnetid => { $mac => { ip4 => '10.0.0.150', ip6 => 'fd00::150' } } }, + }; @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); - is($ips[0], '10.0.0.50', "$test_name: a lookup past an address-less entry asks the IPAM"); + is( + $ips[0], + '10.0.0.50', + "$test_name: an old entry disagreeing with the vnet's counts as a miss", + ); is_deeply( - $test_state->{macdb}->{macs}, - { uc($mac) => { ip4 => '10.0.0.50', ip6 => undef } }, - "$test_name: and fills the entry under its spelling", + $test_state->{macdb}->{vnets}->{$vnetid}->{$mac}, + { ip4 => '10.0.0.50', ip6 => 'fd00::150' }, + "$test_name: and the answer replaces the vnet's entry", ); # two spellings that disagree are not trusted, the lookup asks the IPAM - $test_state->{macdb}->{macs} = { - uc($mac) => { ip4 => '10.0.0.150' }, - $mac => { ip4 => '10.0.0.151' }, + $test_state->{macdb} = { + macs => { + uc($mac) => { ip4 => '10.0.0.150' }, + $mac => { ip4 => '10.0.0.151' }, + }, }; @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); is($ips[0], '10.0.0.50', "$test_name: a clash of spellings counts as a miss"); - is_deeply( - $test_state->{macdb}->{macs}, - { uc($mac) => { ip4 => '10.0.0.50' }, $mac => { ip4 => '10.0.0.50' } }, - "$test_name: and its write brings both spellings in line", - ); + + # an old entry's address outside the vnet's subnets is another vnet's + $test_state->{macdb} = { macs => { uc($mac) => { ip4 => '10.9.0.150' } } }; + @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); + is($ips[0], '10.0.0.50', "$test_name: an old address of another vnet counts as a miss"); + + # the record keeps the MAC as its writer spelled it, a lookup in another + # spelling finds it + $test_state->{macdb} = {}; + @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, uc($mac)); + is($ips[0], '10.0.0.50', "$test_name: a lookup in another spelling finds the record"); # a mapping whose record is gone is still deleted, and its cache entry # with it @@ -1128,15 +1151,18 @@ sub test_dnsmasq_stale_reservation_replaced { my $stale = "da:65:8f:18:9b:01"; my $gone = "da:65:8f:18:9b:02"; my $added = "da:65:8f:18:9b:03"; + create_dhcp_zone("TESTZONE", "testvnet", "10.0.1.0/24"); $test_state->{macdb} = { - macs => { - $stale => { ip4 => '10.0.1.11' }, - $added => { ip4 => '10.0.1.13' }, + vnets => { + testvnet => { + $stale => { ip4 => '10.0.1.11' }, + $added => { ip4 => '10.0.1.13' }, + }, }, }; PVE::Tools::file_set_contents($TMP_ETHERS_FILE, "$stale,10.0.1.10\n$gone,10.0.1.12\n"); - PVE::Network::SDN::Dhcp::Dnsmasq->update_ip_mapping('TESTZONE', $added); + PVE::Network::SDN::Dhcp::Dnsmasq->update_ip_mapping('TESTZONE', 'testvnet', $added); is( PVE::Tools::file_get_contents($TMP_ETHERS_FILE), @@ -1297,8 +1323,127 @@ sub test_ipam_per_vnet { "$test_name: vnet$n answers with its own address", ); } + + # a mapping edit on one vnet leaves the other's answer alone + PVE::API2::Network::SDN::Ips->ipupdate( + { zone => $zoneid, vnet => 'vnet1', mac => $mac, ip => '10.1.0.51' }); + is_deeply( + [PVE::Network::SDN::Vnets::get_ips_from_mac('vnet2', $mac)], + ['10.2.0.50', undef], + "$test_name: an edit on the first vnet leaves the second's answer alone", + ); + is_deeply( + [PVE::Network::SDN::Vnets::get_ips_from_mac('vnet1', $mac)], + ['10.1.0.51', undef], + "$test_name: and changes the first's", + ); + + # an entry from before the cache was kept per vnet answers for any vnet, + # and a mapping edit elsewhere does not touch it + $test_state->{macdb} = { macs => { $mac => { ip4 => '10.2.0.60' } } }; + create_ip({ zone => $zoneid, vnet => 'vnet1', mac => $mac, ip => '10.1.0.52' }); + is_deeply( + [PVE::Network::SDN::Vnets::get_ips_from_mac('vnet2', $mac)], + ['10.2.0.60', undef], + "$test_name: an old entry still answers for the other vnet", + ); } run_test(\&test_ipam_per_vnet); +sub test_ipam_edit_stale_entry { + my $test_name = (split(/::/, (caller(0))[3]))[-1]; + my $zoneid = "TESTZONE"; + my $vnetid = "testvnet"; + my $mac = "da:65:8f:18:9b:6f"; + my $other = "da:65:8f:18:9b:70"; + + create_dhcp_zone($zoneid, $vnetid, "10.0.0.0/24"); + + # a node not yet upgraded released the MAC's address and handed it to + # another guest, while the vnet's cache entry still holds it + create_ip({ zone => $zoneid, vnet => $vnetid, mac => $other, ip => '10.0.0.50' }); + $test_state->{macdb}->{vnets}->{$vnetid}->{$mac} = { ip4 => '10.0.0.50' }; + + PVE::API2::Network::SDN::Ips->ipupdate( + { zone => $zoneid, vnet => $vnetid, mac => $mac, ip => '10.0.0.60' }); + is( + get_ip4(get_ips_from_mac($other)), + '10.0.0.50', + "$test_name: an edit leaves the other guest's record alone", + ); + is(get_ip4(get_ips_from_mac($mac)), '10.0.0.60', "$test_name: and records the new address"); + is( + $test_state->{macdb}->{vnets}->{$vnetid}->{$mac}->{ip4}, + '10.0.0.60', + "$test_name: which the cache holds now", + ); + + # a deletion of a mapping the IPAM does not hold is refused + $test_state->{macdb}->{vnets}->{$vnetid}->{$mac} = { ip4 => '10.0.0.50' }; + eval { + PVE::API2::Network::SDN::Ips->ipdelete( + { zone => $zoneid, vnet => $vnetid, mac => $mac, ip => '10.0.0.50' }); + }; + like($@, qr/not mapped to/, "$test_name: a deletion for another guest's address is refused"); +} + +run_test(\&test_ipam_edit_stale_entry); + +sub test_dnsmasq_reservation_per_vnet { + my $test_name = (split(/::/, (caller(0))[3]))[-1]; + my $zoneid = "TESTZONE"; + my $mac = "da:65:8f:18:9b:6f"; + + # the same MAC on two vnets of one zone has a reservation on each, and a + # start on one vnet changes that vnet's line only + create_zone({ type => "simple", dhcp => "dnsmasq", ipam => "pve", zone => $zoneid }); + for my $n (1, 2) { + create_dhcp_vnet($zoneid, "vnet$n", "10.$n.0.0/24"); + create_ip({ zone => $zoneid, vnet => "vnet$n", mac => $mac, ip => "10.$n.0.50" }); + } + + my $reservations = sub { + return [sort grep { $_ } split(/\n/, PVE::Tools::file_get_contents($TMP_ETHERS_FILE))]; + }; + + nic_start("vnet$_", $mac, "999", "testhostname") for (1, 2, 1); + is_deeply( + $reservations->(), + ["$mac,10.1.0.50", "$mac,10.2.0.50"], + "$test_name: each vnet keeps its reservation", + ); + + PVE::API2::Network::SDN::Ips->ipupdate( + { zone => $zoneid, vnet => 'vnet1', mac => $mac, ip => '10.1.0.51' }); + nic_start('vnet1', $mac, "999", "testhostname"); + is_deeply( + $reservations->(), + ["$mac,10.1.0.51", "$mac,10.2.0.50"], + "$test_name: an edit replaces the reservation of its vnet only", + ); + + # the other lines are checked against the cache alone, so a line the + # cache does not hold is dropped without the IPAM being asked + delete $test_state->{macdb}->{vnets}->{vnet2}; + nic_start('vnet1', $mac, "999", "testhostname"); + is_deeply( + $reservations->(), + ["$mac,10.1.0.51"], + "$test_name: a line the cache does not hold is dropped", + ); + + # the changed MAC's own line is answered by the IPAM when the cache does + # not hold it + delete $test_state->{macdb}->{vnets}->{vnet1}; + PVE::Network::SDN::Dhcp::Dnsmasq->update_ip_mapping($zoneid, 'vnet1', $mac); + is_deeply( + $reservations->(), + ["$mac,10.1.0.51"], + "$test_name: the changed MAC's line is kept without a cache entry", + ); +} + +run_test(\&test_dnsmasq_reservation_per_vnet); + done_testing(); -- 2.47.3