[PATCH pve-network v4 06/20] sdn: ipam: key the MAC cache by lower-case MAC

[Hannes Laimer <h.laimer@proxmox.com>]

Generated guest MACs are upper case, but the API accepts either
spelling. The cache was keyed by the MAC as its writer spelled it. A
deletion in the other spelling missed the entry, so the cache kept
answering with the deleted address. The cache is now read with every
MAC in lower case and written under the lower-case MAC. An entry
cached before this change under another spelling is read under the
lower-case key and leaves the file with the next write. Two spellings
that disagree on an address are both dropped on the read, since one
of them is stale and the cache cannot tell which. The next lookup
then asks the IPAM. The dnsmasq reservation file is written and read
with its MACs in lower case as well, since its lines from before keep
their writer's spelling.

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

Notes:
    v4:
     * fold the spelling handling into the cache read
     * split the miss and lock fixes into their own commits

 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm |  3 ++
 src/PVE/Network/SDN/Ipams.pm        | 39 ++++++++++----
 src/test/run_test_vnets_blackbox.pl | 82 +++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+), 11 deletions(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index 477b700f..d3b78fe9 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -53,6 +53,7 @@ sub update_lease {
 
 sub add_ip_mapping {
     my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_;
+    $mac = lc($mac);
 
     my $ethers_file = ethers_file($dhcpid);
     my $ethers_tmp_file = "$ethers_file.tmp";
@@ -76,6 +77,8 @@ sub add_ip_mapping {
                 warn "failed to parse MAC from $dhcpid ethers file on line $line_no: '$line'\n";
                 next;
             }
+            # the cache is keyed in lower case, a line from before keeps its writer's spelling
+            $parsed_mac = lc($parsed_mac);
 
             my ($parsed_ip4, $parsed_ip6);
             if ($parsed_ip2) {
diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm
index 3d849d79..0e795a93 100644
--- a/src/PVE/Network/SDN/Ipams.pm
+++ b/src/PVE/Network/SDN/Ipams.pm
@@ -23,17 +23,31 @@ PVE::Network::SDN::Ipams::Plugin->init();
 my $macdb_filename = "sdn/mac-cache.json";
 cfs_register_file($macdb_filename, \&json_reader, \&json_writer);
 
-# an entry without an address was left behind by a lookup that missed, and
-# says nothing about its MAC
+# the file may hold a MAC under more than one spelling, and an entry without
+# an address says nothing about its MAC. Two spellings that disagree on an
+# address are both dropped, since one of them is stale and nothing here says
+# which. 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 };
+}
+
 sub json_reader {
     my ($filename, $data) = @_;
 
-    my $db = defined($data) && length($data) > 0 ? decode_json($data) : {};
-    for my $mac (keys %{ $db->{macs} // {} }) {
-        my $entry = $db->{macs}->{$mac};
-        delete $db->{macs}->{$mac} if !defined($entry->{ip4}) && !defined($entry->{ip6});
-    }
-    return $db;
+    return fold_macdb(defined($data) && length($data) > 0 ? decode_json($data) : {});
 }
 
 sub json_writer {
@@ -54,8 +68,10 @@ sub write_macdb {
     cfs_write_file($macdb_filename, $data);
 }
 
+# the cache is keyed by the MAC in lower case, whatever spelling its writers use
 sub add_cache_mac_ip {
     my ($mac, $ip) = @_;
+    $mac = lc($mac);
 
     cfs_lock_file(
         $macdb_filename,
@@ -75,6 +91,7 @@ sub add_cache_mac_ip {
 
 sub del_cache_mac_ip {
     my ($mac, $ip) = @_;
+    $mac = lc($mac);
 
     cfs_lock_file(
         $macdb_filename,
@@ -142,7 +159,7 @@ sub complete_sdn_vnet {
 sub get_ips_from_mac {
     my ($mac, $zoneid, $zone) = @_;
 
-    my $cached = read_macdb()->{macs}->{$mac};
+    my $cached = read_macdb()->{macs}->{ lc($mac) };
     return ($cached->{ip4}, $cached->{ip6}) if $cached;
 
     my $plugin_config = get_plugin_config($zone);
@@ -157,8 +174,8 @@ sub get_ips_from_mac {
         undef,
         sub {
             my $db = read_macdb();
-            $db->{macs}->{$mac}->{ip4} = $ip4 if defined($ip4);
-            $db->{macs}->{$mac}->{ip6} = $ip6 if defined($ip6);
+            $db->{macs}->{ lc($mac) }->{ip4} = $ip4 if defined($ip4);
+            $db->{macs}->{ lc($mac) }->{ip6} = $ip6 if defined($ip6);
             write_macdb($db);
         },
     );
diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl
index d8e35231..6b89778f 100755
--- a/src/test/run_test_vnets_blackbox.pl
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -471,6 +471,88 @@ sub test_ipam_cache_misses {
 
 run_test(\&test_ipam_cache_misses);
 
+sub test_ipam_cache_case {
+    my $test_name = (split(/::/, (caller(0))[3]))[-1];
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+    my $mac = "da:65:8f:18:9b:6f";
+
+    # an entry written under the config's spelling before the cache was keyed in
+    # lower case is read and released like any other
+    create_zone({
+        type => "simple",
+        dhcp => "dnsmasq",
+        ipam => "pve",
+        zone => $zoneid,
+    });
+    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"],
+    });
+    $test_state->{macdb}->{macs}->{ uc($mac) } = { ip4 => '10.0.0.150' };
+    my ($ip4) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
+    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($mac, '10.0.0.150');
+    ok(
+        !(grep { lc($_) eq $mac } keys $test_state->{macdb}->{macs}->%*),
+        "$test_name: and released",
+    );
+
+    # a write spelled either way moves such an entry to the lower-case key
+    # and keeps what it held
+    $test_state->{macdb}->{macs}->{ uc($mac) } = { ip4 => '10.0.0.150' };
+    PVE::Network::SDN::Ipams::add_cache_mac_ip(uc($mac), 'fd00::150');
+    is_deeply(
+        [grep { lc($_) eq $mac } keys $test_state->{macdb}->{macs}->%*],
+        [$mac],
+        "$test_name: a write leaves one entry for the MAC",
+    );
+    is_deeply(
+        $test_state->{macdb}->{macs}->{$mac},
+        { ip4 => '10.0.0.150', ip6 => 'fd00::150' },
+        "$test_name: holding both addresses",
+    );
+
+    # the lookup's write moves an address-less entry under another spelling as well
+    create_ip({
+        zone => $zoneid,
+        vnet => $vnetid,
+        mac => $mac,
+        ip => "10.0.0.160",
+    });
+    $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => undef, ip6 => undef } };
+    my @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
+    is($ips[0], '10.0.0.160', "$test_name: a lookup past an address-less entry asks the IPAM");
+    is_deeply(
+        [sort keys $test_state->{macdb}->{macs}->%*],
+        [$mac],
+        "$test_name: and its write leaves one entry for the MAC",
+    );
+
+    # 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' },
+    };
+    @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
+    is($ips[0], '10.0.0.160', "$test_name: a clash of spellings counts as a miss");
+    is_deeply(
+        $test_state->{macdb}->{macs},
+        { $mac => { ip4 => '10.0.0.160' } },
+        "$test_name: and its write leaves the IPAM's answer alone",
+    );
+}
+
+run_test(\&test_ipam_cache_case);
+
 sub test_nic_join {
     my ($test_name, $subnets) = @_;
 
-- 
2.47.3