Fix VLAN Leak

The logic was backwards so what was likely to happen was the network
would be deleted, we'd update the allocation, which would then fail due
to a conflict error.  We'd never reconcile again because the network had
gone and thus we had no way of getting the segmentation ID.  This makes
deallocation idempotent so it can be called multiple times, and does it
before the network is deleted so we always have access to the
segmentation ID.

Author: spjmurray

pkg/providers/allocation/vlan/vlan.go

@@ -199,10 +199,16 @@ func (a *Allocator) Free(ctx context.Context, id int) error {
 
 	allocation.Spec.Allocations = slices.DeleteFunc(allocation.Spec.Allocations, callback)
 
-	// Question... would this make more sense to be idempotent?
-	// Logic would dictate that some source of truth has this marked as allocated
-	// but we think it isn't so it's probably a bug somewhere.
-	if len(allocation.Spec.Allocations) != allocationsLength-1 {
+	delta := allocationsLength - len(allocation.Spec.Allocations)
+
+	// Deallocation is idempotent.
+	if delta == 0 {
+		return nil
+	}
+
+	// If we have more than one allocation, a bug has crept in or someone has
+	// manually corrupted the allocations table.
+	if delta > 1 {
 		return fmt.Errorf("%w: vlan id %d not allocated exactly once", ErrAllocation, id)
 	}
 

pkg/providers/allocation/vlan/vlan_test.go

@@ -177,7 +177,7 @@ func TestVLANAllocationIllegalRangeHigh(t *testing.T) {
 	require.ErrorIs(t, err, vlan.ErrAllocation)
 }
 
-// TestVLANFree tests an allocated VLAN can be freed, and not freed multiple times.
+// TestVLANFree tests an allocated VLAN can be freed, and is idempotent.
 func TestVLANFree(t *testing.T) {
 	t.Parallel()
 
@@ -190,7 +190,7 @@ func TestVLANFree(t *testing.T) {
 	require.Equal(t, segmentStart, id)
 
 	require.NoError(t, allocator.Free(t.Context(), id))
-	require.ErrorIs(t, allocator.Free(t.Context(), id), vlan.ErrAllocation)
+	require.NoError(t, allocator.Free(t.Context(), id))
 }
 
 // TestVLANFreeIllegalID tests illegaly VLAN IDs are caught.

pkg/providers/internal/openstack/provider.go

@@ -1774,26 +1774,25 @@ func (p *Provider) DeleteNetwork(ctx context.Context, identity *unikornv1.Identi
 	if openstackNetwork != nil {
 		log.V(1).Info("deleting network")
 
-		if err := networking.DeleteNetwork(ctx, openstackNetwork.ID); err != nil {
-			return err
-		}
-
+		// VLAN deallocation is idempotent, but requires the network to
+		// exist so we can lookup the segmentation ID, so this has to
+		// occur first.
 		if p.region.Spec.Openstack.Network.UseProviderNetworks() {
 			log.V(1).Info("freeing vlan", "id", openstackNetwork.SegmentationID)
 
 			vlanID, err := strconv.Atoi(openstackNetwork.SegmentationID)
 			if err != nil {
-				log.Error(err, "failed to free vlan", "id", vlanID)
-
-				return nil
+				return fmt.Errorf("%w: segmentation ID not parsable", err)
 			}
 
 			if err := p.vlanAllocator.Free(ctx, vlanID); err != nil {
-				log.Error(err, "failed to free vlan", "id", vlanID)
-
-				return nil
+				return fmt.Errorf("%w: failed to free vlan", err)
 			}
 		}
+
+		if err := networking.DeleteNetwork(ctx, openstackNetwork.ID); err != nil {
+			return err
+		}
 	}
 
 	return nil