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

@@ -313,6 +313,11 @@ impl VpcPeeringTable {
         peering.validate()?;
         self.validate_peering_addition(&peering)?;
 
+        // First look for an existing entry, to avoid inserting a duplicate peering
+        if self.0.contains_key(&peering.name) {
+            return Err(ApiError::DuplicateVpcPeeringId(peering.name.clone()));
+        }
+
         if let Some(peering) = self.0.insert(peering.name.to_owned(), peering) {
             Err(ConfigError::DuplicateVpcPeeringId(peering.name.clone()))
         } else {
@@ -772,18 +777,13 @@ mod tests {
                     .as_range(prefix_v4("6.0.0.0/16")),
             )
             .expect("Failed to add expose");
-        /*
-        // This test does insert the peering in the table even though we get an error, which makes
-        // the subsequent test fail because of overlapping prefixes. Will be fixed in the next
-        // commit.
         let peering3 = VpcPeering::new("test_peering1", manifest3.clone(), manifest4.clone());
         assert_eq!(
             table.add(peering3),
             Err(ConfigError::DuplicateVpcPeeringId(
                 "test_peering1".to_string()
             ))
         );
-        */
 
         let peering4 = VpcPeering::new("test_peering4", manifest3.clone(), manifest4.clone());
         assert_eq!(table.add(peering4), Ok(()));