From dd30790d51159faf340dce0fdca38161c0cdd160 Mon Sep 17 00:00:00 2001
From: Josh Siviour <mrk@mulgadc.com>
Date: Tue, 28 Apr 2026 10:20:51 +1000
Subject: [PATCH] fix: reconcile external IPAM pool on config drift
 (mulga-siv-24) (#167)

initPool's first guard inverted: !errors.Is(err, ErrKeyNotFound) was
true when err==nil, so initPool returned immediately when a record
existed and never reached the drift-reconcile or already-initialized
branches. As a result, range_start/range_end/source changes in
spinifex.toml never propagated and nextAvailableExternalIP failed
with "invalid IP range:  - " on dhcp -> static transitions.

Restructure to: bail on hard errors, reconcile or no-op when record
exists, fall through to create when ErrKeyNotFound.

Verified on dev cluster: dhcp -> static toml change now reconciles
KV bounds on restart and RunInstances allocates from the new range.

Stale Allocated entries and stale gateway_ip on source change remain
out of scope for this hotfix; tracked under mulga-siv-24 follow-ups.
---
 spinifex/handlers/ec2/vpc/external_ipam.go | 57 +++++++++++-----------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/spinifex/handlers/ec2/vpc/external_ipam.go b/spinifex/handlers/ec2/vpc/external_ipam.go
index e043e0679..e96a9d721 100644
--- a/spinifex/handlers/ec2/vpc/external_ipam.go
+++ b/spinifex/handlers/ec2/vpc/external_ipam.go
@@ -117,37 +117,38 @@ func (m *ExternalIPAM) initPools() error {
 func (m *ExternalIPAM) initPool(pool ExternalPoolConfig) error {
 	chk, revision, err := m.getRecord(pool.Name)
 
-	// Catch all other errors
-	if !errors.Is(err, nats.ErrKeyNotFound) {
+	if err != nil && !errors.Is(err, nats.ErrKeyNotFound) {
 		return err
-	} else if errors.Is(err, nats.ErrKeyNotFound) {
-		// If key not found, proceed to creation
-		slog.Info("external IPAM pool not found, creating", "pool", pool.Name)
-	} else if chk.RangeStart != pool.RangeStart || chk.RangeEnd != pool.RangeEnd || chk.Source != pool.Source {
-		// Validate our pool has not changed, e.g editing spinifex.toml and specifying a new start/end/source
-		slog.Info("external IPAM pool start/end range changed, updating KV", "pool", pool.Name, "RangeStart", pool.RangeStart, "RangeEnd", pool.RangeEnd)
-
-		// Update record with the new start/end
-		chk.RangeStart = pool.RangeStart
-		chk.RangeEnd = pool.RangeEnd
-		chk.Source = pool.Source
-
-		data, err := json.Marshal(chk)
-		if err != nil {
-			return fmt.Errorf("marshal external IPAM record: %w", err)
-		}
-
-		if _, err := m.kv.Update(pool.Name, data, revision); err != nil {
-			slog.Warn("external IPAM update failed", "pool", pool.Name)
-			return err
-		}
-
-		return nil
-	} else if err == nil {
-		slog.Info("external IPAM pool already initialized", "pool", pool.Name)
-		return nil // Already exists
 	}
 
+	if err == nil {
+		if chk.RangeStart != pool.RangeStart || chk.RangeEnd != pool.RangeEnd || chk.Source != pool.Source {
+			slog.Info("external IPAM pool config drift, reconciling KV",
+				"pool", pool.Name,
+				"old_range", chk.RangeStart+"-"+chk.RangeEnd, "new_range", pool.RangeStart+"-"+pool.RangeEnd,
+				"old_source", chk.Source, "new_source", pool.Source)
+
+			chk.RangeStart = pool.RangeStart
+			chk.RangeEnd = pool.RangeEnd
+			chk.Source = pool.Source
+
+			data, err := json.Marshal(chk)
+			if err != nil {
+				return fmt.Errorf("marshal external IPAM record: %w", err)
+			}
+
+			if _, err := m.kv.Update(pool.Name, data, revision); err != nil {
+				slog.Warn("external IPAM update failed", "pool", pool.Name, "err", err)
+				return err
+			}
+			return nil
+		}
+		slog.Debug("external IPAM pool already initialized", "pool", pool.Name)
+		return nil
+	}
+
+	slog.Info("external IPAM pool not found, creating", "pool", pool.Name)
+
 	// TODO: Confirm gateway IP defined, should error, given we route traffic to the upstream DHCP gateway
 	gwIP := pool.GatewayIP
 	if gwIP == "" && !pool.IsDHCP() {