Rename network Service entity to Target

[evanphx]

Summary

• Renames the network "Service" entity to "Target" to eliminate naming confusion with app-level services (web, worker processes in AppVersion configs)
• Renames controllers/service package to controllers/target, updating all types (ServiceController → TargetController, etc.)
• Updates all consumers: runner, sandbox controller, ipalloc, CLI config (ServicePrefixes → TargetPrefixes)
• Adds MigrateServiceToTarget() migration to rename stored entities in etcd (attribute IDs, kind refs, endpoints references), using Replace for full entity replacement

What's NOT changed

• nftables chain/set names (service_ip4s, services chain) — internal kernel identifiers
• network.ServiceManager — unrelated DNS/network management concept
• App-level services in AppVersion configs

Test plan

• Migration unit tests pass (basic rename, port component, already-migrated, empty store)
• make lint passes
• make test passes

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request introduces a migration routine to convert network service entities to target entities and systematically renames all related references throughout the codebase. It adds MigrateServiceToTarget() function to handle entity conversion, updates the network schema from "service" to "target" terminology, and refactors controller imports, types, and dependencies across runner, sandbox, and allocator components. All field names such as ServicePrefixes are renamed to TargetPrefixes, and entity constants are updated to reflect target-based identifiers. The changes include corresponding test files to validate the migration logic and schema transformations.

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

controllers/sandbox/sandbox.go (2)

1005-1033: Consider aligning log wording with Target terminology.

This path now operates on Target entities; updating the message text helps avoid confusion with app-level services.

♻️ Suggested wording tweak

- c.Log.Debug("updating services", "id", co.ID, "labels", md.Labels, "services", len(sresp.Values()))
+ c.Log.Debug("updating targets", "id", co.ID, "labels", md.Labels, "services", len(sresp.Values()))
...
- c.Log.Debug("skipping service, labels do not match", "service", srv.ID, "labels", srv.Match, "entity", md.Labels)
+ c.Log.Debug("skipping target, labels do not match", "service", srv.ID, "labels", srv.Match, "entity", md.Labels)

---

1038-1078: Align addEndpoint log/error wording with Target terminology.

This keeps the rename consistent and reduces ambiguity in logs and errors.

♻️ Suggested wording tweak

- c.Log.Debug("adding endpoint to service", "service", srv.ID, "sandbox", sb.ID, "containers", len(sb.Spec.Container))
+ c.Log.Debug("adding endpoint to target", "service", srv.ID, "sandbox", sb.ID, "containers", len(sb.Spec.Container))
...
- c.Log.Debug("skipping port, not in service", "port", p.Port, "service", srv.ID)
+ c.Log.Debug("skipping port, not in target", "port", p.Port, "service", srv.ID)
...
- return fmt.Errorf("failed to update service: %w", err)
+ return fmt.Errorf("failed to update target: %w", err)
...
- c.Log.Debug("updated service", "id", pr.Id(), "service", eps.Target)
+ c.Log.Debug("updated target", "id", pr.Id(), "service", eps.Target)

cli/commands/server.go (1)

651-655: Minor comment inconsistency.

The comment says "Set service prefixes" but the field has been renamed to TargetPrefixes. Consider updating the comment to match.

-	// Set service prefixes in ServerState
+	// Set target prefixes in ServerState
 	ctx.ServerState.TargetPrefixes = []netip.Prefix{

api/network/migrate.go (4)

61-69: Potential issue: Mutating attribute ID in place.

Lines 62, 65, and 68 modify attr.ID directly on the struct from oldAttrs. While this works because oldAttrs isn't used afterward, it's safer to create a copy to avoid subtle bugs if the code evolves.

🛡️ Suggested defensive fix

 		case oldServiceIp:
-			attr.ID = network_v1alpha.TargetIpId
-			newAttrs = append(newAttrs, attr)
+			newAttrs = append(newAttrs, entity.Attr{ID: network_v1alpha.TargetIpId, Value: attr.Value})
 		case oldServiceMatch:
-			attr.ID = network_v1alpha.TargetMatchId
-			newAttrs = append(newAttrs, attr)
+			newAttrs = append(newAttrs, entity.Attr{ID: network_v1alpha.TargetMatchId, Value: attr.Value})
 		case oldServicePort:
-			attr.ID = network_v1alpha.TargetPortId
-			newAttrs = append(newAttrs, attr)
+			newAttrs = append(newAttrs, entity.Attr{ID: network_v1alpha.TargetPortId, Value: attr.Value})

---

70-77: Same mutation concern for sub-component paths.

Line 74 also mutates attr.ID in place before appending.

🛡️ Suggested defensive fix

 		default:
 			// Rename sub-component builder paths: service.port.* → target.port.*
 			if strings.HasPrefix(string(attr.ID), oldServicePortPrefix) {
 				newID := "dev.miren.network/target.port" + strings.TrimPrefix(string(attr.ID), oldServicePortPrefix)
-				attr.ID = entity.Id(newID)
+				newAttrs = append(newAttrs, entity.Attr{ID: entity.Id(newID), Value: attr.Value})
+				continue
 			}
 			newAttrs = append(newAttrs, attr)

---

121-126: Same mutation pattern in endpoints migration.

Consider using the same defensive copy approach here.

🛡️ Suggested fix

 		for _, attr := range oldAttrs {
 			if attr.ID == oldEndpointsService {
-				attr.ID = network_v1alpha.EndpointsTargetId
+				newAttrs = append(newAttrs, entity.Attr{ID: network_v1alpha.EndpointsTargetId, Value: attr.Value})
+				continue
 			}
 			newAttrs = append(newAttrs, attr)
 		}

---

80-85: Migration silently continues on individual entity failures.

The migration logs errors but continues processing, which is appropriate for resilience. However, the function returns nil even when some entities failed to migrate. Consider returning an aggregate error or at least a warning if any entities failed.

💡 Optional: Track and report failures

 func migrateServiceEntities(ctx context.Context, log *slog.Logger, eac *entityserver_v1alpha.EntityAccessClient) error {
 	// ... existing code ...
 	migratedCount := 0
+	failedCount := 0

 	for _, e := range resp.Values() {
 		// ... existing code ...
 		if _, err := eac.Replace(ctx, newAttrs, e.Revision()); err != nil {
 			log.Error("failed to migrate service entity to target", "error", err, "id", e.Id())
+			failedCount++
 			continue
 		}
 		migratedCount++
 	}

 	if migratedCount > 0 {
-		log.Info("migrated service entities to target", "count", migratedCount)
+		log.Info("migrated service entities to target", "count", migratedCount, "failed", failedCount)
 	}

Also applies to: 128-131

controllers/target/target_test.go (1)

64-64: Test function name inconsistency.

The test function is named TestServiceController but it's testing the TargetController. Consider renaming for consistency.

-func TestServiceController(t *testing.T) {
+func TestTargetController(t *testing.T) {

controllers/target/target.go (2)

398-402: Log message inconsistency.

The log message on line 402 still says "Initializing service controller" but this is now the TargetController.

 func (s *TargetController) Init(ctx context.Context) error {
 	s.chainEndpoints = make(map[string][]string)
 	s.routablePrefixes = append([]netip.Prefix{s.IPv4Routable}, s.TargetPrefixes...)

-	s.Log.Info("Initializing service controller")
+	s.Log.Info("Initializing target controller")

---

142-172: Clarification: Chain names intentionally unchanged.

The serviceChain, service_ip4s, service_ip6s naming is intentionally preserved as these are nftables kernel identifiers. This aligns with the PR description stating that "nftables chain/set names remain as kernel identifiers."

Consider adding a brief comment to clarify this intentional choice for future maintainers:

+// serviceChain returns the nftables chain name for a target IP:port.
+// Note: The "service_" prefix is retained for nftables compatibility.
 func (s *TargetController) serviceChain(ip netip.Addr, port uint16) string {

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[phinze]

Talked this through live, we agreed to pick another word just to leave deploy target open as a phrase to use elsewhere. Re-request review when you get around to it!