[PATCH pve-network v2] sdn: push mapping changes from the ipam API to the dhcp backend

[PATCH pve-network v2] sdn: push mapping changes from the ipam API to the dhcp backend

[Hannes Laimer]

Mapping edits through the API only wrote the IPAM record and the
cache, the dhcp backend was never told, so dnsmasq kept serving
stale reservations until a later guest start in the zone happened
to sweep them. The same held for a guest whose NIC moved to another
bridge, the old records were released and new ones allocated without
the backend hearing of either.

Push the MAC's current answer after every record change, removing
the old reservation first, since a re-add alone leaves a stale
address behind when only one of the MAC's two addresses was
deleted. The push is best effort, the record write stays
authoritative. Mapping removal was dispatched to the plugins but
implemented nowhere and never called, add the dnsmasq
implementation for it.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---

v2:
 - one push helper in the dhcp dispatcher instead of the push repeated
   in the ips API handlers
 - the guest allocation and release paths push too, so a bridge change
   on a running guest reaches the backend
 - a guest start pushes once, only for what it allocated
 - the update goes through a plugin hook, remove then add stays the
   default for dnsmasq

 src/PVE/API2/Network/SDN/Ips.pm     |  7 +++++++
 src/PVE/Network/SDN/Dhcp.pm         | 21 +++++++++++++++++++
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 32 ++++++++++++++++++++++++++++-
 src/PVE/Network/SDN/Dhcp/Plugin.pm  | 13 ++++++++++++
 src/PVE/Network/SDN/Vnets.pm        | 17 ++++++++++-----
 5 files changed, 84 insertions(+), 6 deletions(-)

diff --git a/src/PVE/API2/Network/SDN/Ips.pm b/src/PVE/API2/Network/SDN/Ips.pm
index 5ff05e7..d7b682d 100644
--- a/src/PVE/API2/Network/SDN/Ips.pm
+++ b/src/PVE/API2/Network/SDN/Ips.pm
@@ -46,6 +46,8 @@ __PACKAGE__->register_method({
         eval { PVE::Network::SDN::Vnets::del_ip($vnet, $ip, '', $mac); };
         die "$@\n" if $@;
 
+        PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac);
+
         return undef;
     },
 });
@@ -82,6 +84,8 @@ __PACKAGE__->register_method({
 
         PVE::Network::SDN::Vnets::add_ip($vnet, $ip, '', $mac, undef);
 
+        PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac);
+
         return undef;
     },
 });
@@ -132,6 +136,9 @@ __PACKAGE__->register_method({
         }
 
         die "$error\n" if $error;
+
+        PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac);
+
         return undef;
     },
 });
diff --git a/src/PVE/Network/SDN/Dhcp.pm b/src/PVE/Network/SDN/Dhcp.pm
index 65e40d4..a9459b4 100644
--- a/src/PVE/Network/SDN/Dhcp.pm
+++ b/src/PVE/Network/SDN/Dhcp.pm
@@ -55,6 +55,27 @@ sub remove_mapping {
     $dhcp_plugin->del_ip_mapping($zoneid, $mac);
 }
 
+# re-apply a MAC's mapping on this node from the current records, best
+# effort, the record write stays authoritative
+sub update_mapping {
+    my ($vnetid, $mac) = @_;
+
+    eval {
+        my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid);
+        return if !$vnet;
+
+        my $zoneid = $vnet->{zone};
+        my $zone = PVE::Network::SDN::Zones::get_zone($zoneid);
+        return if !$zone->{ipam} || !$zone->{dhcp};
+
+        my ($ip4, $ip6) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
+        my $macdb = PVE::Network::SDN::Ipams::read_macdb();
+        my $dhcp_plugin = PVE::Network::SDN::Dhcp::Plugin->lookup($zone->{dhcp});
+        $dhcp_plugin->update_ip_mapping($zoneid, $macdb, $mac, $ip4, $ip6);
+    };
+    warn "could not update dhcp mapping for $mac: $@" if $@;
+}
+
 sub regenerate_config {
     my ($reload) = @_;
 
diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index 477b700..4677330 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -6,7 +6,7 @@ use warnings;
 use base qw(PVE::Network::SDN::Dhcp::Plugin);
 
 use Net::IP qw(:PROC);
-use PVE::Tools qw(file_set_contents run_command lock_file);
+use PVE::Tools qw(file_get_contents file_set_contents run_command lock_file);
 
 use File::Copy;
 use Net::DBus;
@@ -136,6 +136,36 @@ sub add_ip_mapping {
     update_lease($dhcpid, $ip4, $mac);
 }
 
+sub del_ip_mapping {
+    my ($class, $dhcpid, $mac) = @_;
+
+    my $ethers_file = ethers_file($dhcpid);
+
+    my $reload = undef;
+
+    my $removeFn = sub {
+        my @lines = split(/\n/, file_get_contents($ethers_file));
+        my @remaining = grep {
+            my ($parsed_mac) = split(/,/, $_);
+            !defined($parsed_mac) || $parsed_mac ne $mac;
+        } @lines;
+
+        return if scalar(@remaining) == scalar(@lines);
+
+        file_set_contents($ethers_file, join("\n", @remaining) . "\n", 0644);
+        $reload = 1;
+    };
+
+    PVE::Tools::lock_file($ethers_file, 10, $removeFn);
+
+    if ($@) {
+        warn "Unable to remove $mac from the dnsmasq configuration: $@\n";
+        return;
+    }
+
+    systemctl_service('reload', "dnsmasq\@$dhcpid") if $reload;
+}
+
 sub configure_subnet {
     my ($class, $config, $dhcpid, $vnetid, $subnet_config) = @_;
 
diff --git a/src/PVE/Network/SDN/Dhcp/Plugin.pm b/src/PVE/Network/SDN/Dhcp/Plugin.pm
index b5d32fa..659c938 100644
--- a/src/PVE/Network/SDN/Dhcp/Plugin.pm
+++ b/src/PVE/Network/SDN/Dhcp/Plugin.pm
@@ -27,6 +27,19 @@ sub add_ip_mapping {
     die 'implement in sub class';
 }
 
+sub del_ip_mapping {
+    my ($class, $dhcpid, $mac) = @_;
+    die 'implement in sub class';
+}
+
+# the MAC's records changed, make the backend serve the current ones
+sub update_ip_mapping {
+    my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_;
+
+    $class->del_ip_mapping($dhcpid, $mac);
+    $class->add_ip_mapping($dhcpid, $macdb, $mac, $ip4, $ip6) if $ip4 || $ip6;
+}
+
 sub configure_range {
     my ($class, $config, $dhcpid, $vnetid, $subnet_config, $range_config) = @_;
     die 'implement in sub class';
diff --git a/src/PVE/Network/SDN/Vnets.pm b/src/PVE/Network/SDN/Vnets.pm
index c327a4b..daf6654 100644
--- a/src/PVE/Network/SDN/Vnets.pm
+++ b/src/PVE/Network/SDN/Vnets.pm
@@ -154,6 +154,8 @@ sub add_next_free_cidr {
             die "can't find any free ip in zone $zoneid for IPv$ipversion";
         }
     }
+
+    PVE::Network::SDN::Dhcp::update_mapping($vnetid, $mac) if %$ips;
 }
 
 sub add_ip {
@@ -211,6 +213,8 @@ sub del_ips_from_mac {
     PVE::Network::SDN::Vnets::del_ip($vnetid, $ip4, $hostname, $mac) if $ip4;
     PVE::Network::SDN::Vnets::del_ip($vnetid, $ip6, $hostname, $mac) if $ip6;
 
+    PVE::Network::SDN::Dhcp::update_mapping($vnetid, $mac) if $ip4 || $ip6;
+
     return ($ip4, $ip6);
 }
 
@@ -224,12 +228,15 @@ sub add_dhcp_mapping {
 
     return if !$zone->{ipam} || !$zone->{dhcp};
 
+    # an allocation pushes the mapping itself, a guest whose records exist
+    # already has to be pushed here
     my ($ip4, $ip6) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
-    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;
+    if (!$ip4 || !$ip6) {
+        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;
+    } else {
+        PVE::Network::SDN::Dhcp::update_mapping($vnetid, $mac);
+    }
 }
 
 1;
-- 
2.47.3