diff --git a/src/compose_file.cpp b/src/compose_file.cpp index d503e98..634c5ce 100644 --- a/src/compose_file.cpp +++ b/src/compose_file.cpp @@ -372,6 +372,85 @@ std::optional parse_volume_entry(std::string_view text, cons return mount; } +enum class DfsColor { white, gray, black }; + +DfsColor& color_of(std::vector>& colors, const std::string& name) { + for (auto& entry : colors) { + if (entry.first == name) { + return entry.second; + } + } + static DfsColor unreachable = DfsColor::black; // every name is seeded into `colors` up front -- never reached + return unreachable; +} + +const ComposeService* find_service(const std::vector& services, const std::string& name) { + for (const auto& service : services) { + if (service.name == name) { + return &service; + } + } + return nullptr; +} + +// Recursive step of find_dependency_cycle()'s three-color DFS: returns the +// cycle path if the subtree rooted at `name` contains one (a depends_on +// edge reaching back to a `gray` -- currently-on-the-stack -- node), else +// nullopt. `stack` mirrors the recursion path so the actual cycle can be +// read off it once a back-edge is found, rather than just reporting "a +// cycle exists somewhere." +std::optional> dfs_find_cycle(const std::vector& services, + std::vector>& colors, + std::vector& stack, const std::string& name) { + color_of(colors, name) = DfsColor::gray; + stack.push_back(name); + + const ComposeService* service = find_service(services, name); + for (const auto& dep : service->depends_on) { + DfsColor dep_color = color_of(colors, dep); + if (dep_color == DfsColor::gray) { + auto start = std::find(stack.begin(), stack.end(), dep); + std::vector cycle(start, stack.end()); + cycle.push_back(dep); + return cycle; + } + if (dep_color == DfsColor::white) { + if (auto found = dfs_find_cycle(services, colors, stack, dep)) { + return found; + } + } + } + + stack.pop_back(); + color_of(colors, name) = DfsColor::black; + return std::nullopt; +} + +// Finds a depends_on cycle among `services` (A -> B -> ... -> A), if any -- +// not just the direct self-reference load_compose_file() already rejects +// per-edge before this ever runs, but any longer one, which would make a +// future orchestrator's own startup ordering impossible. Assumes every +// depends_on name already refers to a real, declared service (the +// per-edge existence check that runs first guarantees this). Returns the +// actual cycle path (e.g. {"a", "b", "c", "a"}) if one exists. +std::optional> find_dependency_cycle(const std::vector& services) { + std::vector> colors; + colors.reserve(services.size()); + for (const auto& service : services) { + colors.emplace_back(service.name, DfsColor::white); + } + + std::vector stack; + for (const auto& service : services) { + if (color_of(colors, service.name) == DfsColor::white) { + if (auto found = dfs_find_cycle(services, colors, stack, service.name)) { + return found; + } + } + } + return std::nullopt; +} + } // namespace std::optional load_compose_file(const std::filesystem::path& path) { @@ -542,11 +621,29 @@ std::optional load_compose_file(const std::filesystem::path& path) if (!result.services[i].container_name) { continue; } - for (size_t j = i + 1; j < result.services.size(); ++j) { - if (result.services[j].container_name && - *result.services[j].container_name == *result.services[i].container_name) { + const std::string& container_name = *result.services[i].container_name; + for (size_t j = 0; j < result.services.size(); ++j) { + if (j == i) { + continue; + } + // Two services both setting the same explicit container_name -- + // checked with j > i so the pair is only reported once, not + // once as (i, j) and again as (j, i). + if (j > i && result.services[j].container_name && *result.services[j].container_name == container_name) { spdlog::error("compose file {}: services '{}' and '{}' both use container_name '{}'", path.string(), - result.services[i].name, result.services[j].name, *result.services[i].container_name); + result.services[i].name, result.services[j].name, container_name); + return std::nullopt; + } + // A service's own explicit container_name colliding with + // *another* service's implicit identity (its own name, when it + // has no container_name of its own) -- inherently one-directional + // (service[i]'s container_name vs. service[j]'s name), so no + // j > i guard is needed here the way the symmetric check above + // needs one. + if (result.services[j].name == container_name) { + spdlog::error( + "compose file {}: service '{}' has container_name '{}', colliding with service '{}'s own name", + path.string(), result.services[i].name, container_name, result.services[j].name); return std::nullopt; } } @@ -677,5 +774,51 @@ std::optional load_compose_file(const std::filesystem::path& path) } } + // Not just the direct self-reference already rejected above -- a longer + // cycle (A -> B -> C -> A) would make a future orchestrator's own + // startup ordering impossible. + if (auto cycle = find_dependency_cycle(result.services)) { + std::string cycle_text; + for (size_t i = 0; i < cycle->size(); ++i) { + if (i > 0) { + cycle_text += " -> "; + } + cycle_text += (*cycle)[i]; + } + spdlog::error("compose file {}: depends_on cycle: {}", path.string(), cycle_text); + return std::nullopt; + } + + // A (host_port, protocol) pair names one real, host-wide socket -- two + // services (or two entries within the same one) both publishing it + // would only ever leave one of them actually reachable, even though + // both DNAT rules would get added by a future orchestrator. + struct PublishedPort { + int host_port; + PortForwardProtocol protocol; + std::string service_name; + }; + std::vector published_ports; + for (const auto& service : result.services) { + for (const auto& port : service.ports) { + auto colliding = + std::find_if(published_ports.begin(), published_ports.end(), [&](const PublishedPort& p) { + return p.host_port == port.host_port && p.protocol == port.protocol; + }); + if (colliding != published_ports.end()) { + const char* proto = port.protocol == PortForwardProtocol::udp ? "udp" : "tcp"; + if (colliding->service_name == service.name) { + spdlog::error("compose file {}: service '{}' publishes host port {}/{} twice", path.string(), + service.name, port.host_port, proto); + } else { + spdlog::error("compose file {}: services '{}' and '{}' both publish host port {}/{}", + path.string(), colliding->service_name, service.name, port.host_port, proto); + } + return std::nullopt; + } + published_ports.push_back({port.host_port, port.protocol, service.name}); + } + } + return result; } diff --git a/tests/unit/test_compose_file.cpp b/tests/unit/test_compose_file.cpp index 1553ff9..c9ed6f4 100644 --- a/tests/unit/test_compose_file.cpp +++ b/tests/unit/test_compose_file.cpp @@ -110,6 +110,19 @@ TEST_CASE("compose file: container_name -- optional, and duplicates across servi " image: busybox:latest\n" " container_name: shared\n"); CHECK_FALSE(load_compose_file(colliding).has_value()); + + // A service's own explicit container_name colliding with a *different* + // service's implicit identity (its own name, since it has no + // container_name of its own) -- also an error, not just two explicit + // container_names matching each other. + auto colliding_with_name = write_compose(scratch.path(), + "services:\n" + " web:\n" + " image: busybox:latest\n" + " worker:\n" + " image: busybox:latest\n" + " container_name: web\n"); + CHECK_FALSE(load_compose_file(colliding_with_name).has_value()); } TEST_CASE("compose file: command -- list form used as-is, scalar form wrapped in sh -c", "[unit]") { @@ -243,6 +256,53 @@ TEST_CASE("compose file: depends_on -- long and short forms, condition handling, CHECK_FALSE(load_compose_file(undeclared).has_value()); } +TEST_CASE("compose file: depends_on cycles are rejected, acyclic graphs are not", "[unit]") { + ScratchXdgDirs scratch; + + // A 2-service direct cycle (a -> b -> a) -- distinct from the + // already-tested direct self-reference (a -> a). + auto two_cycle = write_compose(scratch.path(), + "services:\n" + " a:\n" + " image: busybox:latest\n" + " depends_on: [\"b\"]\n" + " b:\n" + " image: busybox:latest\n" + " depends_on: [\"a\"]\n"); + CHECK_FALSE(load_compose_file(two_cycle).has_value()); + + // A longer, 3-service cycle (a -> b -> c -> a). + auto three_cycle = write_compose(scratch.path(), + "services:\n" + " a:\n" + " image: busybox:latest\n" + " depends_on: [\"b\"]\n" + " b:\n" + " image: busybox:latest\n" + " depends_on: [\"c\"]\n" + " c:\n" + " image: busybox:latest\n" + " depends_on: [\"a\"]\n"); + CHECK_FALSE(load_compose_file(three_cycle).has_value()); + + // A valid, acyclic multi-service graph (a diamond: d depends on both b + // and c, both of which depend on a) must still load successfully. + auto acyclic = write_compose(scratch.path(), + "services:\n" + " a:\n" + " image: busybox:latest\n" + " b:\n" + " image: busybox:latest\n" + " depends_on: [\"a\"]\n" + " c:\n" + " image: busybox:latest\n" + " depends_on: [\"a\"]\n" + " d:\n" + " image: busybox:latest\n" + " depends_on: [\"b\", \"c\"]\n"); + CHECK(load_compose_file(acyclic).has_value()); +} + TEST_CASE("compose file: stop_grace_period parses durations, rejects malformed ones", "[unit]") { ScratchXdgDirs scratch; @@ -386,6 +446,43 @@ TEST_CASE("compose file: ports -- list and scalar forms reuse parse_port_forward CHECK_FALSE(load_compose_file(bad).has_value()); } +TEST_CASE("compose file: duplicate host-port/protocol across (or within) services is an error", "[unit]") { + ScratchXdgDirs scratch; + + auto across_services = write_compose(scratch.path(), + "services:\n" + " web:\n" + " image: busybox:latest\n" + " ports:\n" + " - \"8080:80\"\n" + " web2:\n" + " image: busybox:latest\n" + " ports:\n" + " - \"8080:81\"\n"); + CHECK_FALSE(load_compose_file(across_services).has_value()); + + auto within_one_service = write_compose(scratch.path(), + "services:\n" + " web:\n" + " image: busybox:latest\n" + " ports:\n" + " - \"8080:80\"\n" + " - \"8080:81\"\n"); + CHECK_FALSE(load_compose_file(within_one_service).has_value()); + + // Same host port, different protocols -- must still succeed, matching + // port_forward.h's own existing "same port pair once per protocol" + // precedent (e.g. a DNS-like service forwarding both tcp and udp). + auto different_protocols = write_compose(scratch.path(), + "services:\n" + " web:\n" + " image: busybox:latest\n" + " ports:\n" + " - \"53:53/tcp\"\n" + " - \"53:53/udp\"\n"); + CHECK(load_compose_file(different_protocols).has_value()); +} + TEST_CASE("compose file: service volumes -- bind mounts (absolute-resolved, ro), named volume references", "[unit]") { ScratchXdgDirs scratch;