Preserve route groups during edits

internal/routing/manager.go

@@ -441,7 +441,7 @@ func (m *RoutingManager) listLiveRoutesLocked() []RouteItem {
 					if cleanGw == "" {
 						cleanGw = nhIfName
 					}
-					routeID := fmt.Sprintf("kr-%d-%s-%s", r.Table, cleanDst, cleanGw)
+					routeID := fmt.Sprintf("kr-%d-%s-%s-m%d-p%d-t%d", r.Table, cleanDst, cleanGw, r.Priority, r.Protocol, r.Type)
 
 					liveRoutes = append(liveRoutes, RouteItem{
 						ID:           routeID,
@@ -477,7 +477,7 @@ func (m *RoutingManager) listLiveRoutesLocked() []RouteItem {
 			if cleanGw == "" {
 				cleanGw = ifName
 			}
-			routeID := fmt.Sprintf("kr-%d-%s-%s", r.Table, cleanDst, cleanGw)
+			routeID := fmt.Sprintf("kr-%d-%s-%s-m%d-p%d-t%d", r.Table, cleanDst, cleanGw, r.Priority, r.Protocol, r.Type)
 
 			liveRoutes = append(liveRoutes, RouteItem{
 				ID:           routeID,
@@ -509,17 +509,18 @@ func (m *RoutingManager) ListRoutes() []RouteItem {
 
 	// Merge static routes that might be disabled or have custom metadata
 	matchedLive := make(map[int]bool)
+	kernelRouteCount := len(liveRoutes)
 	for _, sr := range m.staticRoutes {
 		normSrDst := normalizeDst(sr.DstAddress)
 		found := false
-		for i, lr := range liveRoutes {
-			if matchedLive[i] {
+		for i, lr := range liveRoutes[:kernelRouteCount] {
+			if sr.Disabled || !lr.Static || lr.Protocol != "static" || matchedLive[i] {
 				continue
 			}
 			if normalizeDst(lr.DstAddress) != normSrDst {
 				continue
 			}
-			if lr.RoutingTable != sr.RoutingTable {
+			if !sameRouteGroup(lr, sr) {
 				continue
 			}
 			gwMatches := sr.Gateway == "" || lr.Gateway == sr.Gateway || lr.Interface == sr.Gateway || (sr.Interface != "" && lr.Interface == sr.Interface)
@@ -609,6 +610,18 @@ func (m *RoutingManager) ListRoutes() []RouteItem {
 	return liveRoutes
 }
 
+// Route groups are keyed by destination, table and metric, not nexthop.
+func sameRouteGroup(a, b RouteItem) bool {
+	table := func(s string) string {
+		s = strings.TrimSpace(s)
+		if s == "" {
+			return "main"
+		}
+		return s
+	}
+	return normalizeDst(a.DstAddress) == normalizeDst(b.DstAddress) && table(a.RoutingTable) == table(b.RoutingTable) && a.Distance == b.Distance
+}
+
 func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 	if r == nil {
 		return fmt.Errorf("route cannot be nil")
@@ -616,16 +629,34 @@ func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	before := append([]RouteItem(nil), m.staticRoutes...)
-	dirty, applied := false, false
+	var oldRouteToClean *RouteItem
+	var unmanagedOriginal *RouteItem
+	dirty, newAttempted, oldAttempted := false, false, false
 	defer func() {
-		if err != nil && dirty {
-			m.staticRoutes = before
-			if applied {
-				_ = m.syncRouteGroupToKernel(r.DstAddress, r.RoutingTable, r.Distance)
+		if err == nil || !dirty {
+			return
+		}
+		m.staticRoutes = append([]RouteItem(nil), before...)
+		if unmanagedOriginal != nil {
+			m.staticRoutes = append(m.staticRoutes, *unmanagedOriginal)
+		}
+		var failures []error
+		failures = append(failures, err)
+		if newAttempted {
+			if rollbackErr := m.syncRouteGroupToKernel(r.DstAddress, r.RoutingTable, r.Distance); rollbackErr != nil {
+				failures = append(failures, fmt.Errorf("restoring new route group: %w", rollbackErr))
+			}
+		}
+		if oldAttempted && oldRouteToClean != nil && !sameRouteGroup(*oldRouteToClean, *r) {
+			if rollbackErr := m.syncRouteGroupToKernel(oldRouteToClean.DstAddress, oldRouteToClean.RoutingTable, oldRouteToClean.Distance); rollbackErr != nil {
+				failures = append(failures, fmt.Errorf("restoring original route group: %w", rollbackErr))
 			}
-			m.applyAll()
-			_ = m.save()
 		}
+		m.staticRoutes = before
+		if saveErr := m.save(); saveErr != nil {
+			failures = append(failures, fmt.Errorf("restoring saved routes: %w", saveErr))
+		}
+		err = errors.Join(failures...)
 	}()
 
 	if r.DstAddress == "" {
@@ -640,7 +671,6 @@ func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 	if _, _, parseErr := net.ParseCIDR(r.DstAddress); parseErr != nil && net.ParseIP(r.DstAddress) == nil {
 		return fmt.Errorf("invalid destination: %s", r.DstAddress)
 	}
-	dirty = true
 	r.Static = true
 
 	origID := r.ID
@@ -668,7 +698,7 @@ func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 			if exTbl == "" {
 				exTbl = "main"
 			}
-			if normalizeDst(existing.DstAddress) == normOldDst && exTbl == oldTable {
+			if normalizeDst(existing.DstAddress) == normOldDst && exTbl == oldTable && (r.OldDistance == nil || existing.Distance == *r.OldDistance) {
 				oldGwMatches := r.OldGateway == "" ||
 					existing.Gateway == r.OldGateway ||
 					existing.Interface == r.OldGateway ||
@@ -689,7 +719,7 @@ func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 			if exTbl == "" {
 				exTbl = "main"
 			}
-			if normalizeDst(existing.DstAddress) == normNewDst && exTbl == r.RoutingTable {
+			if normalizeDst(existing.DstAddress) == normNewDst && exTbl == r.RoutingTable && existing.Distance == r.Distance {
 				gwMatches := (existing.Gateway != "" && existing.Gateway == r.Gateway) ||
 					(existing.Interface != "" && existing.Interface == r.Interface) ||
 					(existing.Gateway == "" && r.Gateway == "")
@@ -713,7 +743,7 @@ func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 			if exTbl == "" {
 				exTbl = "main"
 			}
-			if normalizeDst(existing.DstAddress) == normOldDst && exTbl == oldTable {
+			if normalizeDst(existing.DstAddress) == normOldDst && exTbl == oldTable && (r.OldDistance == nil || existing.Distance == *r.OldDistance) {
 				foundIndex = i
 				break
 			}
@@ -738,7 +768,6 @@ func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 		r.HealthStatus = "none"
 	}
 
-	var oldRouteToClean *RouteItem
 	if foundIndex >= 0 {
 		oldExisting := m.staticRoutes[foundIndex]
 		r.ID = oldExisting.ID // Preserve persistent ID!
@@ -758,40 +787,32 @@ func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 			r.HealthLossPct = oldExisting.HealthLossPct
 			r.HealthLastCheck = oldExisting.HealthLastCheck
 		}
+		dirty = true
 		m.staticRoutes[foundIndex] = *r
 	} else {
 		if r.ID == "" || strings.HasPrefix(r.ID, "kr-") {
 			r.ID = fmt.Sprintf("route-%d", time.Now().UnixNano())
 		}
-		// If adopting a live kernel route not in staticRoutes, remove it from direct kernel routes
+		// An unmanaged route may be adopted only when it is an identifiable,
+		// single static route. Preserve its actual kernel values for rollback.
 		if strings.HasPrefix(origID, "kr-") {
 			for _, lr := range m.listLiveRoutesLocked() {
 				if lr.ID == origID {
+					if lr.Protocol != "static" || lr.IsMultiPath {
+						return fmt.Errorf("cannot edit a dynamic or multipath kernel route")
+					}
+					original := lr
+					unmanagedOriginal = &original
+					oldRouteToClean = &original
 					break
 				}
 			}
-		}
-		m.staticRoutes = append(m.staticRoutes, *r)
-	}
-
-	if r.OldDstAddress != "" {
-		oldDist := 0
-		if r.OldDistance != nil {
-			oldDist = *r.OldDistance
-		}
-		if normalizeDst(r.OldDstAddress) != normalizeDst(r.DstAddress) ||
-			r.OldRoutingTable != r.RoutingTable ||
-			oldDist != r.Distance ||
-			r.OldGateway != r.Gateway ||
-			r.OldInterface != r.Interface {
-			oldRouteToClean = &RouteItem{
-				DstAddress:   r.OldDstAddress,
-				Gateway:      r.OldGateway,
-				RoutingTable: r.OldRoutingTable,
-				Distance:     oldDist,
-				Interface:    r.OldInterface,
+			if unmanagedOriginal == nil {
+				return fmt.Errorf("kernel route not found: %s", origID)
 			}
 		}
+		dirty = true
+		m.staticRoutes = append(m.staticRoutes, *r)
 	}
 
 	for i := range m.staticRoutes {
@@ -821,17 +842,18 @@ func (m *RoutingManager) SaveRoute(r *RouteItem) (err error) {
 		return saveErr
 	}
 
-	// Clean up old route group if destination, routing table, distance, gateway, or interface changed
-	if oldRouteToClean != nil {
-		_ = m.syncRouteGroupToKernel(oldRouteToClean.DstAddress, oldRouteToClean.RoutingTable, oldRouteToClean.Distance)
-		_ = m.deleteDirectKernelRoute(*oldRouteToClean)
-	}
-
-	// Apply updated/new route group to kernel
+	// Install the new group first to preserve connectivity when the destination
+	// changes. Reconcile the old group only if its key differs.
+	newAttempted = true
 	if err := m.syncRouteGroupToKernel(r.DstAddress, r.RoutingTable, r.Distance); err != nil {
-		return err
+		return fmt.Errorf("applying new route group: %w", err)
+	}
+	if oldRouteToClean != nil && !sameRouteGroup(*oldRouteToClean, *r) && !oldRouteToClean.Disabled {
+		oldAttempted = true
+		if err := m.syncRouteGroupToKernel(oldRouteToClean.DstAddress, oldRouteToClean.RoutingTable, oldRouteToClean.Distance); err != nil {
+			return fmt.Errorf("cleaning previous route group: %w", err)
+		}
 	}
-	applied = true
 
 	return nil
 }
@@ -864,7 +886,7 @@ func (m *RoutingManager) DeleteRoute(id string) error {
 				normLrDst := normalizeDst(lr.DstAddress)
 				for i, sr := range m.staticRoutes {
 					gwMatches := sr.Gateway == "" || lr.Gateway == sr.Gateway || lr.Interface == sr.Gateway || (sr.Interface != "" && lr.Interface == sr.Interface)
-					if normalizeDst(sr.DstAddress) == normLrDst && sr.RoutingTable == lr.RoutingTable && gwMatches {
+					if normalizeDst(sr.DstAddress) == normLrDst && sameRouteGroup(sr, lr) && gwMatches {
 						matchIdx = i
 						break
 					}
@@ -877,10 +899,13 @@ func (m *RoutingManager) DeleteRoute(id string) error {
 							updated = append(updated, r)
 						}
 					}
-					_ = m.deleteDirectKernelRoute(lr)
 					break
 				} else {
-					// Pure kernel route, not managed in static_routes.json
+					// Do not remove routes belonging to other routing daemons or a
+					// single nexthop of a multipath route.
+					if lr.Protocol != "static" || lr.IsMultiPath {
+						return fmt.Errorf("cannot delete a dynamic or multipath kernel route")
+					}
 					return m.deleteDirectKernelRoute(lr)
 				}
 			}
@@ -940,7 +965,7 @@ func (m *RoutingManager) SetRouteState(id string, disabled bool) error {
 			normLrDst := normalizeDst(lr.DstAddress)
 			for i := range m.staticRoutes {
 				gwMatches := m.staticRoutes[i].Gateway == "" || lr.Gateway == m.staticRoutes[i].Gateway || lr.Interface == m.staticRoutes[i].Gateway || (m.staticRoutes[i].Interface != "" && lr.Interface == m.staticRoutes[i].Interface)
-				if normalizeDst(m.staticRoutes[i].DstAddress) == normLrDst && m.staticRoutes[i].RoutingTable == lr.RoutingTable && gwMatches {
+				if normalizeDst(m.staticRoutes[i].DstAddress) == normLrDst && sameRouteGroup(m.staticRoutes[i], lr) && gwMatches {
 					before := append([]RouteItem(nil), m.staticRoutes...)
 					m.staticRoutes[i].Disabled = disabled
 					if !disabled && m.staticRoutes[i].HealthStatus != "offline" {
@@ -954,6 +979,9 @@ func (m *RoutingManager) SetRouteState(id string, disabled bool) error {
 
 			// If pure kernel static route, adopt into staticRoutes as disabled
 			if disabled {
+				if lr.Protocol != "static" || lr.IsMultiPath {
+					return fmt.Errorf("cannot disable a dynamic or multipath kernel route")
+				}
 				lr.Disabled = true
 				lr.Static = true
 				lr.Active = false

internal/routing/route_change_test.go

@@ -110,3 +110,167 @@ func TestIsolatedRouteGroupDeletionKeepsOtherMetric(t *testing.T) {
 		t.Fatalf("unexpected routes: %s", out)
 	}
 }
+
+func TestRouteEditSameGroupDoesNotDeleteReplacement(t *testing.T) {
+	useTempRouteStores(t)
+	bin := t.TempDir()
+	t.Setenv("PATH", bin)
+	logPath := filepath.Join(t.TempDir(), "commands")
+	t.Setenv("DEBTIK_TEST_IP_LOG", logPath)
+	script := "#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$DEBTIK_TEST_IP_LOG\"\nexit 0\n"
+	if err := os.WriteFile(filepath.Join(bin, "ip"), []byte(script), 0755); err != nil {
+		t.Fatal(err)
+	}
+	old := RouteItem{ID: "r1", DstAddress: "198.51.100.0/24", Gateway: "192.0.2.1", Interface: "eth0", RoutingTable: "main", Distance: 10, Active: true}
+	m := &RoutingManager{staticRoutes: []RouteItem{old}}
+	if err := m.save(); err != nil {
+		t.Fatal(err)
+	}
+	next := old
+	next.Gateway = "192.0.2.2"
+	if err := m.SaveRoute(&next); err != nil {
+		t.Fatal(err)
+	}
+	data, err := os.ReadFile(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	commands := strings.Split(strings.TrimSpace(string(data)), "\n")
+	if len(commands) != 1 || !strings.Contains(commands[0], "route replace 198.51.100.0/24 via 192.0.2.2") {
+		t.Fatalf("replacement was deleted or repeated: %s", data)
+	}
+}
+
+func TestRouteEditCleanupFailureRollsBack(t *testing.T) {
+	useTempRouteStores(t)
+	bin := t.TempDir()
+	t.Setenv("PATH", bin)
+	logPath := filepath.Join(t.TempDir(), "commands")
+	t.Setenv("DEBTIK_TEST_IP_LOG", logPath)
+	script := "#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$DEBTIK_TEST_IP_LOG\"\nif [ \"$2\" = del ] && [ \"$3\" = 198.51.100.0/24 ]; then echo 'permission denied' >&2; exit 2; fi\nif [ \"$2\" = del ]; then echo 'RTNETLINK answers: No such process' >&2; exit 2; fi\nexit 0\n"
+	if err := os.WriteFile(filepath.Join(bin, "ip"), []byte(script), 0755); err != nil {
+		t.Fatal(err)
+	}
+	old := RouteItem{ID: "r1", DstAddress: "198.51.100.0/24", Gateway: "192.0.2.1", Interface: "eth0", RoutingTable: "main", Distance: 10, Active: true}
+	m := &RoutingManager{staticRoutes: []RouteItem{old}}
+	if err := m.save(); err != nil {
+		t.Fatal(err)
+	}
+	next := old
+	next.DstAddress = "203.0.113.0/24"
+	err := m.SaveRoute(&next)
+	if err == nil || !strings.Contains(err.Error(), "cleaning previous route group") {
+		t.Fatalf("cleanup failure ignored: %v", err)
+	}
+	if len(m.staticRoutes) != 1 || m.staticRoutes[0].DstAddress != old.DstAddress {
+		t.Fatalf("memory not restored: %+v", m.staticRoutes)
+	}
+	data, err := os.ReadFile(StaticRoutesStorePath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var persisted []RouteItem
+	if err = json.Unmarshal(data, &persisted); err != nil {
+		t.Fatal(err)
+	}
+	if len(persisted) != 1 || persisted[0].DstAddress != old.DstAddress {
+		t.Fatalf("storage not restored: %+v", persisted)
+	}
+	cmds, err := os.ReadFile(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(string(cmds), "route replace 198.51.100.0/24") || !strings.Contains(string(cmds), "route del 203.0.113.0/24") {
+		t.Fatalf("rollback omitted: %s", cmds)
+	}
+}
+
+func TestIsolatedRouteEditKeepsReplacementAndRollsBackFailure(t *testing.T) {
+	if os.Getenv("DEBTIK_TEST_ISOLATED_NETNS") != "1" {
+		t.Skip("requires a dedicated network namespace")
+	}
+	useTempRouteStores(t)
+	ip := func(args ...string) string {
+		t.Helper()
+		out, err := exec.Command("ip", args...).CombinedOutput()
+		if err != nil {
+			t.Fatalf("ip %v: %s: %v", args, out, err)
+		}
+		return string(out)
+	}
+	ip("link", "set", "lo", "up")
+	ip("link", "add", "route-test0", "type", "dummy")
+	ip("link", "set", "route-test0", "up")
+	old := RouteItem{ID: "r1", DstAddress: "198.51.101.0/24", Gateway: "192.0.2.1", Interface: "route-test0", RoutingTable: "main", Distance: 10, Active: true}
+	ip("route", "add", old.DstAddress, "via", old.Gateway, "dev", old.Interface, "onlink", "metric", "10", "proto", "static")
+	m := &RoutingManager{staticRoutes: []RouteItem{old}}
+	if err := m.save(); err != nil {
+		t.Fatal(err)
+	}
+	next := old
+	next.Gateway = "192.0.2.2"
+	if err := m.SaveRoute(&next); err != nil {
+		t.Fatal(err)
+	}
+	output := ip("route", "show", old.DstAddress)
+	if !strings.Contains(output, "via 192.0.2.2") || strings.Contains(output, "via 192.0.2.1") {
+		t.Fatalf("replacement missing: %s", output)
+	}
+	invalid := next
+	invalid.DstAddress = "203.0.114.0/24"
+	invalid.Interface = "does-not-exist"
+	if err := m.SaveRoute(&invalid); err == nil {
+		t.Fatal("expected failure applying invalid device")
+	}
+	output = ip("route", "show", old.DstAddress)
+	if !strings.Contains(output, "via 192.0.2.2") {
+		t.Fatalf("old route lost after failed edit: %s", output)
+	}
+	if output = ip("route", "show", invalid.DstAddress); strings.TrimSpace(output) != "" {
+		t.Fatalf("invalid route left in kernel: %s", output)
+	}
+}
+
+func TestIsolatedRouteIDsAndMetricsRemainDistinct(t *testing.T) {
+	if os.Getenv("DEBTIK_TEST_ISOLATED_NETNS") != "1" {
+		t.Skip("requires a dedicated network namespace")
+	}
+	useTempRouteStores(t)
+	ip := func(args ...string) {
+		t.Helper()
+		if out, err := exec.Command("ip", args...).CombinedOutput(); err != nil {
+			t.Fatalf("ip %v: %s %v", args, out, err)
+		}
+	}
+	ip("link", "set", "lo", "up")
+	ip("link", "add", "route-test1", "type", "dummy")
+	ip("link", "set", "route-test1", "up")
+	ip("route", "add", "198.51.102.0/24", "dev", "route-test1", "metric", "10", "proto", "static")
+	ip("route", "add", "198.51.102.0/24", "dev", "route-test1", "metric", "20", "proto", "static")
+	m := &RoutingManager{}
+	var kernelIDs []string
+	for _, r := range m.listLiveRoutesLocked() {
+		if r.DstAddress == "198.51.102.0/24" {
+			kernelIDs = append(kernelIDs, r.ID)
+		}
+	}
+	if len(kernelIDs) != 2 || kernelIDs[0] == kernelIDs[1] {
+		t.Fatalf("ambiguous kernel IDs: %v", kernelIDs)
+	}
+	m.staticRoutes = []RouteItem{{ID: "r20", DstAddress: "198.51.102.0/24", Interface: "route-test1", RoutingTable: "main", Distance: 20, Active: true}, {ID: "r10", DstAddress: "198.51.102.0/24", Interface: "route-test1", RoutingTable: "main", Distance: 10, Active: true}}
+	seen := map[int]string{}
+	for _, r := range m.ListRoutes() {
+		if r.DstAddress == "198.51.102.0/24" && r.Active {
+			seen[r.Distance] = r.ID
+		}
+	}
+	if seen[10] != "r10" || seen[20] != "r20" {
+		t.Fatalf("wrong route mapping: %v", seen)
+	}
+	m.staticRoutes[1].Disabled = true
+	for _, r := range m.ListRoutes() {
+		if r.ID == "r10" && r.Active {
+			t.Fatal("disabled managed route displayed as active")
+		}
+	}
+}