fix(vpcpeering): Do not insert peering with duplicate name

When we try to insert a peering with a name that already exists for
another peering in a VpcPeeringTable, this results in an error, but it
also results in the peering being inserted anyway, as we only detect the
name collision when inserting (and we do not implement rollback).

As a consequence, we keep an incorrect peering entry in the table. This
can cause unexpected collisions on prefixes when trying to add
subsequent, legitimate peerings to the table, for example.

Instead of inserting unconditionally, let's check first for the
existence of the peering name in the table.

Fixes: e45c0ef (feat(mgmt): define external model)
Signed-off-by: Quentin Monnet <[email protected]>

Author: qmonnet

mgmt/src/models/external/overlay/vpcpeering.rs

@@ -234,8 +234,16 @@ impl VpcPeeringTable {
     /// Add a [`VpcPeering`] to a [`VpcPeeringTable`]
     pub fn add(&mut self, peering: VpcPeering) -> ConfigResult {
         peering.validate()?;
-        if let Some(peering) = self.0.insert(peering.name.to_owned(), peering) {
-            Err(ConfigError::DuplicateVpcPeeringId(peering.name.clone()))
+
+        // First look for an existing entry, to avoid inserting a duplicate peering
+        if self.0.contains_key(&peering.name) {
+            return Err(ConfigError::DuplicateVpcPeeringId(peering.name.clone()));
+        }
+
+        if self.0.insert(peering.name.to_owned(), peering).is_some() {
+            // We should have prevented this case by checking for duplicates just above.
+            // This should never happen, unless we have another thread modifying the table.
+            unreachable!()
         } else {
             Ok(())
         }