[PATCH pve-network v2 03/11] sdn: ipam: read the MAC cache case-insensitively

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

Generated guest MACs are upper case, but the API accepts either
spelling, and 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.

Lookups now match a MAC in any spelling, and a write or a deletion
reaches its entry under every spelling the file holds. The keys stay
as written, since nodes not yet upgraded look an entry up under the
spelling their caller hands them, and a MAC new to the file is
written in lower case. Two spellings that disagree on an address are
both left out of the lookup, since one of them is stale and the cache
cannot tell which, so the next lookup 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>
---
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm |  3 ++
 src/PVE/Network/SDN/Ipams.pm        | 72 +++++++++++++++++++++--------
 src/test/run_test_vnets_blackbox.pl | 51 +++++++++++++++++++-
 3 files changed, 107 insertions(+), 19 deletions(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index 477b700f2429..d3b78fe9a894 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 ae2c4337ef8d..1476c4236be0 100644
--- a/src/PVE/Network/SDN/Ipams.pm
+++ b/src/PVE/Network/SDN/Ipams.pm
@@ -35,18 +35,58 @@ sub json_writer {
     return encode_json($data);
 }
 
-sub read_macdb {
+# 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());
+}
+
 sub write_macdb {
     my ($data) = @_;
 
     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) = @_;
+
+    my $macs = $db->{macs} //= {};
+    my @keys = grep { lc($_) eq lc($mac) } keys %$macs;
+    $macs->{$_}->{$family} = $ip for (@keys ? @keys : (lc($mac)));
+}
+
 sub add_cache_mac_ip {
     my ($mac, $ip) = @_;
 
@@ -54,12 +94,8 @@ sub add_cache_mac_ip {
         $macdb_filename,
         undef,
         sub {
-            my $db = read_macdb();
-            if (Net::IP::ip_is_ipv4($ip)) {
-                $db->{macs}->{$mac}->{ip4} = $ip;
-            } else {
-                $db->{macs}->{$mac}->{ip6} = $ip;
-            }
+            my $db = read_macdb_file();
+            set_cache_entry($db, $mac, Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6', $ip);
             write_macdb($db);
         },
     );
@@ -73,14 +109,14 @@ sub del_cache_mac_ip {
         $macdb_filename,
         undef,
         sub {
-            my $db = read_macdb();
-            if (Net::IP::ip_is_ipv4($ip)) {
-                delete $db->{macs}->{$mac}->{ip4};
-            } else {
-                delete $db->{macs}->{$mac}->{ip6};
+            my $db = read_macdb_file();
+            my $family = Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6';
+            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});
             }
-            delete $db->{macs}->{$mac}
-                if !defined($db->{macs}->{$mac}->{ip4}) && !defined($db->{macs}->{$mac}->{ip6});
             write_macdb($db);
         },
     );
@@ -137,7 +173,7 @@ sub get_ips_from_mac {
 
     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}->{$mac};
+    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})
@@ -159,9 +195,9 @@ sub get_ips_from_mac {
             return
                 if (PVE::Cluster::cfs_file_version($macdb_filename) // '') ne ($version // '');
 
-            my $db = read_macdb();
-            $db->{macs}->{$mac}->{ip4} = $ip4 if defined($ip4);
-            $db->{macs}->{$mac}->{ip6} = $ip6 if defined($ip6);
+            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);
             write_macdb($db);
         },
     );
diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl
index 0362b7ca2f8d..e9c9bbb6be32 100755
--- a/src/test/run_test_vnets_blackbox.pl
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -193,7 +193,7 @@ $mocked_sdn_ipams->mock(
         my ($cfg) = @_;
         $test_state->{ipam_config} = $cfg;
     },
-    read_macdb => sub {
+    read_macdb_file => sub {
         return PVE::Network::SDN::Ipams::json_reader(undef, encode_json($test_state->{macdb}));
     },
     write_macdb => sub {
@@ -484,6 +484,55 @@ sub test_ipam_cache_lookup {
     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");
+
+    # 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
+    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_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
+    $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => '10.0.0.150' } };
+    PVE::Network::SDN::Ipams::add_cache_mac_ip($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",
+    );
+
+    # 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 } };
+    @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_deeply(
+        $test_state->{macdb}->{macs},
+        { uc($mac) => { ip4 => '10.0.0.50', ip6 => undef } },
+        "$test_name: and fills the entry under its spelling",
+    );
+
+    # 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.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",
+    );
 }
 
 run_test(\&test_ipam_cache_lookup);
-- 
2.47.3