[PATCH pve-network v4 03/13] 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. The cache stored each 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 every spelling of it in the cache file. The file keeps its
keys as written, since nodes not yet upgraded look a MAC up by its
exact spelling. A MAC new to the file is written in lower case.

If two spellings disagree on an address, the cache cannot tell which
is stale. A lookup then ignores them, asks the IPAM and writes its
answer under both.

Adding a dnsmasq reservation checks every line of its file against
the cache and drops the ones that do not match. The check ignores case
as well, since an existing line keeps its 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..dc70b9a99a2e 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 read 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 927ab16adba9..be4c581e414e 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 => sub {
+    read_macdb_file => sub {
         return PVE::Network::SDN::Ipams::json_reader(undef, encode_json($test_state->{macdb}));
     },
     write_macdb => sub {
@@ -498,6 +498,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