Prevent removal of network with container references.

Sources/APIServer/APIServer.swift

@@ -65,6 +65,7 @@ struct APIServer: AsyncParsableCommand {
             let containersService = try initializeContainerService(pluginLoader: pluginLoader, log: log, routes: &routes)
             let networkService = try await initializeNetworkService(
                 pluginLoader: pluginLoader,
+                containersService: containersService,
                 log: log,
                 routes: &routes
             )
@@ -223,13 +224,15 @@ struct APIServer: AsyncParsableCommand {
 
     private func initializeNetworkService(
         pluginLoader: PluginLoader,
+        containersService: ContainersService,
         log: Logger,
         routes: inout [XPCRoute: XPCServer.RouteHandler]
     ) async throws -> NetworksService {
         let resourceRoot = appRoot.appendingPathComponent("networks")
         let service = try await NetworksService(
             pluginLoader: pluginLoader,
             resourceRoot: resourceRoot,
+            containersService: containersService,
             log: log
         )
 

Sources/APIServer/Networks/NetworksService.swift

@@ -26,23 +26,30 @@ import Foundation
 import Logging
 
 actor NetworksService {
-    private let resourceRoot: URL
-    // FIXME: remove qualifier once we can update Containerization dependency.
-    private let store: ContainerPersistence.FilesystemEntityStore<NetworkConfiguration>
     private let pluginLoader: PluginLoader
+    private let resourceRoot: URL
+    private let containersService: ContainersService
     private let log: Logger
-    private let networkPlugin: Plugin
 
+    private let store: FilesystemEntityStore<NetworkConfiguration>
+    private let networkPlugin: Plugin
     private var networkStates = [String: NetworkState]()
     private var busyNetworks = Set<String>()
 
-    public init(pluginLoader: PluginLoader, resourceRoot: URL, log: Logger) async throws {
-        try FileManager.default.createDirectory(at: resourceRoot, withIntermediateDirectories: true)
-        self.resourceRoot = resourceRoot
-        self.store = try FilesystemEntityStore<NetworkConfiguration>(path: resourceRoot, type: "network", log: log)
+    public init(
+        pluginLoader: PluginLoader,
+        resourceRoot: URL,
+        containersService: ContainersService,
+        log: Logger
+    ) async throws {
         self.pluginLoader = pluginLoader
+        self.resourceRoot = resourceRoot
+        self.containersService = containersService
         self.log = log
 
+        try FileManager.default.createDirectory(at: resourceRoot, withIntermediateDirectories: true)
+        self.store = try FilesystemEntityStore<NetworkConfiguration>(path: resourceRoot, type: "network", log: log)
+
         let networkPlugin =
             pluginLoader
             .findPlugins()
@@ -137,10 +144,12 @@ actor NetworksService {
 
     /// Delete a network.
     public func delete(id: String) async throws {
+        // check actor busy state
         guard !busyNetworks.contains(id) else {
             throw ContainerizationError(.exists, message: "network \(id) has a pending operation")
         }
 
+        // make actor state busy for this network
         busyNetworks.insert(id)
         defer { busyNetworks.remove(id) }
 
@@ -149,6 +158,8 @@ actor NetworksService {
             metadata: [
                 "id": "\(id)"
             ])
+
+        // basic sanity checks on network itself
         if id == ClientNetwork.defaultNetworkName {
             throw ContainerizationError(.invalidArgument, message: "cannot delete system subnet \(ClientNetwork.defaultNetworkName)")
         }
@@ -161,28 +172,61 @@ actor NetworksService {
             throw ContainerizationError(.invalidState, message: "cannot delete subnet \(id) in state \(networkState.state)")
         }
 
-        let client = NetworkClient(id: id)
-        guard try await client.disableAllocator() else {
-            throw ContainerizationError(.invalidState, message: "cannot delete subnet \(id) with containers attached")
-        }
+        // prevent container operations while we atomically check and delete
+        try await containersService.withContainerList { containers in
+            // find all containers that refer to the network
+            var referringContainers = Set<String>()
+            for container in containers {
+                for containerNetworkId in container.configuration.networks {
+                    if containerNetworkId == id {
+                        referringContainers.insert(container.configuration.id)
+                        break
+                    }
+                }
+            }
 
-        defer { networkStates.removeValue(forKey: id) }
-        do {
-            try pluginLoader.deregisterWithLaunchd(plugin: networkPlugin, instanceId: id)
-        } catch {
-            log.error(
-                "failed to deregister network service after failed creation",
-                metadata: [
-                    "id": "\(id)",
-                    "error": "\(error.localizedDescription)",
-                ])
-        }
+            // bail if any referring containers
+            guard referringContainers.isEmpty else {
+                throw ContainerizationError(
+                    .invalidState,
+                    message: "cannot delete subnet \(id) with referring containers: \(referringContainers.joined(separator: ", "))"
+                )
+            }
 
-        do {
-            try await store.delete(id)
-        } catch {
-            throw ContainerizationError(.notFound, message: error.localizedDescription)
+            // disable the allocator so nothing else can attach
+            // TODO: remove this from the network helper later, not necesssary now that withContainerList is here
+            let client = NetworkClient(id: id)
+            guard try await client.disableAllocator() else {
+                throw ContainerizationError(.invalidState, message: "cannot delete subnet \(id) because the IP allocator cannot be disabled with active containers")
+            }
+
+            // start network deletion, this is the last place we'll want to throw
+            do {
+                try self.pluginLoader.deregisterWithLaunchd(plugin: self.networkPlugin, instanceId: id)
+            } catch {
+                self.log.error(
+                    "failed to deregister network service",
+                    metadata: [
+                        "id": "\(id)",
+                        "error": "\(error.localizedDescription)",
+                    ])
+            }
+
+            // deletion is underway, do not throw anything now
+            do {
+                try await self.store.delete(id)
+            } catch {
+                self.log.error(
+                    "failed to delete network from configuration store",
+                    metadata: [
+                        "id": "\(id)",
+                        "error": "\(error.localizedDescription)",
+                    ])
+            }
         }
+
+        // having deleted successfully, remove the runtime state
+        self.networkStates.removeValue(forKey: id)
     }
 
     /// Perform a hostname lookup on all networks.

Tests/CLITests/Subcommands/Networks/TestCLINetwork.swift

@@ -18,6 +18,7 @@
 
 import AsyncHTTPClient
 import ContainerClient
+import ContainerizationError
 import ContainerizationExtras
 import ContainerizationOS
 import Foundation
@@ -76,7 +77,54 @@ class TestCLINetwork: CLITest {
             #expect(success, "Request to \(url) failed after \(Self.retries - retriesRemaining) retries")
             try doStop(name: name)
         } catch {
-            Issue.record("failed to run container \(error)")
+            Issue.record("failed to create and use network \(error)")
+            return
+        }
+    }
+
+    @available(macOS 26, *)
+    @Test func testNetworkDeleteWithContainer() async throws {
+        do {
+            // prep: delete container and network, ignoring if it doesn't exist
+            let name = Test.current!.name.trimmingCharacters(in: ["(", ")"])
+            try? doRemove(name: name)
+            let networkDeleteArgs = ["network", "delete", name]
+            _ = try? run(arguments: networkDeleteArgs)
+
+            // create our network
+            let networkCreateArgs = ["network", "create", name]
+            let networkCreateResult = try run(arguments: networkCreateArgs)
+            if networkCreateResult.status != 0 {
+                throw CLIError.executionFailed("command failed: \(networkCreateResult.error)")
+            }
+
+            // ensure it's deleted
+            defer {
+                _ = try? run(arguments: networkDeleteArgs)
+            }
+
+            // create a container that refers to the network
+            try doCreate(name: name, networks: [name])
+            defer {
+                try? doRemove(name: name)
+            }
+
+            // deleting the network should fail
+            let networkDeleteResult = try run(arguments: networkDeleteArgs)
+            try #require(networkDeleteResult.status != 0)
+
+            // and should fail with a certain message
+            let msg = networkDeleteResult.error
+            #expect(msg.contains("delete failed"))
+            #expect(msg.contains("[\"\(name)\"]"))
+
+            // now get rid of the container and its network reference
+            try? doRemove(name: name)
+
+            // delete should succeed
+            _ = try run(arguments: networkDeleteArgs)
+        } catch {
+            Issue.record("failed to safely delete network \(error)")
             return
         }
     }

Tests/CLITests/Utilities/CLITest.swift

@@ -223,7 +223,13 @@ class CLITest {
         }
     }
 
-    func doCreate(name: String, image: String? = nil, args: [String]? = nil, volumes: [String] = []) throws {
+    func doCreate(
+        name: String,
+        image: String? = nil,
+        args: [String]? = nil,
+        volumes: [String] = [],
+        networks: [String] = []
+    ) throws {
         let image = image ?? alpine
         let args: [String] = args ?? ["sleep", "infinity"]
 
@@ -234,6 +240,11 @@ class CLITest {
             arguments += ["-v", volume]
         }
 
+        // Add networks
+        for network in networks {
+            arguments += ["--network", network]
+        }
+
         arguments += [image] + args
 
         let (_, error, status) = try run(arguments: arguments)