From 9809d6b4452ac2f72134d5baf1868522f0161a3b Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 16 Oct 2026 21:07:52 +0200 Subject: [PATCH] fix(service): connect containers to the predefined network before they start Create the containers with --no-start, connect them to the destination network, then start them. A container that crashed before the late connect was restarting, the connect failed silently, and it never joined the network. Closes #11371 Co-Authored-By: Claude Opus 5.5 --- .ai/lessons.md | 2 +- .../Service/DeployServiceApplication.php | 20 ++++--- app/Actions/Service/StartService.php | 37 +++++++++--- .../Security/CommandInjectionSecurityTest.php | 14 ++++- tests/Feature/ServiceComposeStdinTest.php | 57 +++++++++++++++++-- 5 files changed, 108 insertions(+), 22 deletions(-) diff --git a/.ai/lessons.md b/.ai/lessons.md index a28d452c5c..c2137bdb38 100644 --- a/.ai/lessons.md +++ b/.ai/lessons.md @@ -6,7 +6,7 @@ - Test old reports against the current branch because later changes can make the report obsolete. - Use the same regression test before and after the production change so the result shows the behavior difference. - Several dev instances can run from one checkout (see `./scripts/dev urls`). After you add a migration, run it on each running instance of that checkout, or those instances fail on the new code. -- The checkout is shared with other sessions. When the full suite fails, rerun each failing file alone and compare with a clean `git archive HEAD` copy before you connect a failure to your change. +- The checkout is shared with other sessions. When the full suite fails, rerun each failing file alone and compare with a clean `git archive HEAD` copy before you connect a failure to your change. The dev instance of the checkout runs its live files, so a live repro also runs the uncommitted edits of other sessions: run `git diff` on the code under test before you trust the result. - Run seeders and other artisan commands that write SSH keys in a dev container as `www-data` (`docker exec -u www-data`). Root-owned files in `storage/app/ssh` make every SSH call from the web process fail. - `sshd` keeps the login shell of an open multiplexed SSH connection. Call `SshMultiplexingHelper::removeMuxFile()` before a live test of SSH or login-shell behavior. - SSH retries replay the full command batch. Only exit code 255 is an SSH transport failure; other exit codes come from the remote command, so do not retry them. Keep batches safe to run twice (for example `docker rm -f || true` before `docker run --name `). diff --git a/app/Actions/Service/DeployServiceApplication.php b/app/Actions/Service/DeployServiceApplication.php index 4db4834d4c..2a475cd462 100644 --- a/app/Actions/Service/DeployServiceApplication.php +++ b/app/Actions/Service/DeployServiceApplication.php @@ -47,24 +47,30 @@ class DeployServiceApplication $commands->push("docker network inspect {$safeProjectName} >/dev/null 2>&1 || docker network create --attachable {$safeProjectName}"); } + $connectToPredefinedNetwork = (bool) data_get($service, 'connect_to_docker_network'); $upCommand = "docker compose --project-directory {$safeWorkdir} -f {$safeComposeFile} --project-name {$safeProjectName} up -d --no-deps"; if ($forceRebuild) { $upCommand .= ' --build'; } + if ($connectToPredefinedNetwork) { + $upCommand .= ' --no-start'; + } // No stdin for Compose prompts: they would read the next script lines (see StartService). $upCommand .= " {$safeComposeServiceName} < /dev/null"; $commands->push('echo Starting service container.'); $commands->push($upCommand); - $commands->push("docker network connect {$safeProjectName} coolify-proxy >/dev/null 2>&1 || true"); - - if (data_get($service, 'connect_to_docker_network')) { - $network = escapeshellarg($service->destination->network); - $containerName = escapeshellarg("{$composeServiceName}-{$service->uuid}"); - $networkAlias = escapeshellarg("{$composeServiceName}-{$service->uuid}"); - $commands->push("docker network connect --alias {$networkAlias} {$network} {$containerName} >/dev/null 2>&1 || true"); + if ($connectToPredefinedNetwork) { + // Connect to the predefined network before the container starts (see StartService). + $serviceConfig = data_get(parseDockerComposeYaml($service->docker_compose ?? ''), "services.{$composeServiceName}"); + if ($command = StartService::predefinedNetworkConnectCommand($service, $composeServiceName, $serviceConfig)) { + $commands->push($command); + } + $commands->push('docker start '.escapeshellarg("{$composeServiceName}-{$service->uuid}").' >/dev/null'); } + $commands->push("docker network connect {$safeProjectName} coolify-proxy >/dev/null 2>&1 || true"); + return remote_process($commands->toArray(), $service->server, type_uuid: $service->uuid, callEventOnFinish: 'ServiceStartFinished', callEventData: $service->id, queue: deployment_queue()); } } diff --git a/app/Actions/Service/StartService.php b/app/Actions/Service/StartService.php index eeb394e795..8a2496a45d 100644 --- a/app/Actions/Service/StartService.php +++ b/app/Actions/Service/StartService.php @@ -54,22 +54,43 @@ class StartService $commands[] = "docker network inspect $service->uuid >/dev/null 2>&1 || docker network create --attachable $service->uuid"; } $commands[] = 'echo Starting service.'; - $commands[] = "docker compose --project-directory {$workdir} -f {$workdir}/docker-compose.yml --project-name {$service->uuid} up -d --remove-orphans --force-recreate --build < /dev/null"; - $commands[] = "docker network connect $service->uuid coolify-proxy >/dev/null 2>&1 || true"; + $compose = "docker compose --project-directory {$workdir} -f {$workdir}/docker-compose.yml --project-name {$service->uuid}"; if (data_get($service, 'connect_to_docker_network')) { - $compose = data_get($service, 'docker_compose', []); - $safeNetwork = escapeshellarg($service->destination->network); - $serviceNames = data_get(parseDockerComposeYaml($compose), 'services', []); - foreach ($serviceNames as $serviceName => $serviceConfig) { - $containerName = escapeshellarg("{$serviceName}-{$service->uuid}"); - $commands[] = "docker network connect --alias {$containerName} {$safeNetwork} {$containerName} >/dev/null 2>&1 || true"; + // Connect the created containers to the predefined network before they start. A container + // that needs this network during a service_healthy wait never gets it after `up -d`. + $commands[] = "{$compose} up -d --remove-orphans --force-recreate --build --no-start < /dev/null"; + $composeServices = data_get(parseDockerComposeYaml(data_get($service, 'docker_compose', '')), 'services', []); + foreach ($composeServices as $serviceName => $serviceConfig) { + if ($command = self::predefinedNetworkConnectCommand($service, $serviceName, $serviceConfig)) { + $commands[] = $command; + } } + $commands[] = "{$compose} start < /dev/null"; + } else { + $commands[] = "{$compose} up -d --remove-orphans --force-recreate --build < /dev/null"; } + $commands[] = "docker network connect $service->uuid coolify-proxy >/dev/null 2>&1 || true"; $commands = array_merge($commands, $this->logDrainNetworkConnectCommands($service)); return remote_process($commands, $service->server, type_uuid: $service->uuid, callEventOnFinish: 'ServiceStartFinished', callEventData: $service->id, queue: deployment_queue()); } + /** + * Connects a created container to the predefined network of the destination. Containers with + * a network_mode, or that already declare the network in the compose file, are skipped. + */ + public static function predefinedNetworkConnectCommand(Service $service, string $serviceName, mixed $serviceConfig): ?string + { + $network = $service->destination->network; + if (data_get($serviceConfig, 'network_mode') !== null || array_key_exists($network, data_get($serviceConfig, 'networks') ?? [])) { + return null; + } + $containerName = escapeshellarg("{$serviceName}-{$service->uuid}"); + $warning = escapeshellarg("Warning: Could not connect {$serviceName} to the {$network} network."); + + return 'docker network connect --alias '.$containerName.' '.escapeshellarg($network)." {$containerName} || echo {$warning}"; + } + /** * Shows the volume warnings of the last parse (for example an external volume that the * service does not use yet) in the start log. diff --git a/tests/Feature/Security/CommandInjectionSecurityTest.php b/tests/Feature/Security/CommandInjectionSecurityTest.php index 60f5ae8c91..154e278eae 100644 --- a/tests/Feature/Security/CommandInjectionSecurityTest.php +++ b/tests/Feature/Security/CommandInjectionSecurityTest.php @@ -1,8 +1,11 @@ toContain('escapeshellarg($workdir)') ->and($source)->toContain('escapeshellarg($composeFile)') ->and($source)->toContain('escapeshellarg($service->uuid)') - ->and($source)->toContain('escapeshellarg($composeServiceName)') - ->and($source)->toContain('escapeshellarg($service->destination->network)'); + ->and($source)->toContain('escapeshellarg($composeServiceName)'); + }); + + test('predefined network connect command escapes the container name', function () { + $service = (new Service)->forceFill(['uuid' => 'svc-uuid']); + $service->setRelation('destination', (new StandaloneDocker)->forceFill(['network' => 'coolify'])); + + expect(StartService::predefinedNetworkConnectCommand($service, 'app$(id)', [])) + ->toBe("docker network connect --alias 'app$(id)-svc-uuid' 'coolify' 'app$(id)-svc-uuid' || echo 'Warning: Could not connect app$(id) to the coolify network.'"); }); test('service application restart and stop actions escape container names', function (string $path) { diff --git a/tests/Feature/ServiceComposeStdinTest.php b/tests/Feature/ServiceComposeStdinTest.php index f5d48e9a15..8484d723ff 100644 --- a/tests/Feature/ServiceComposeStdinTest.php +++ b/tests/Feature/ServiceComposeStdinTest.php @@ -36,7 +36,7 @@ afterEach(function () { File::deleteDirectory($this->stubDirectory); }); -function serviceComposeStdinService(string $sshUser): Service +function serviceComposeStdinService(string $sshUser, ?string $composeRaw = null): Service { $team = Team::factory()->create(); $server = Server::factory()->create([ @@ -54,7 +54,7 @@ function serviceComposeStdinService(string $sshUser): Service 'destination_id' => $destination->id, 'destination_type' => $destination->getMorphClass(), 'connect_to_docker_network' => true, - 'docker_compose_raw' => "services:\n app:\n image: busybox:latest\n volumes:\n - data:/data\nvolumes:\n data:\n labels:\n example: changed\n", + 'docker_compose_raw' => $composeRaw ?? "services:\n app:\n image: busybox:latest\n volumes:\n - data:/data\nvolumes:\n data:\n labels:\n example: changed\n", ]); } @@ -98,7 +98,7 @@ function serviceComposeCommandLines(Activity $activity): array { return array_values(array_filter( explode("\n", RemoteProcessCommand::read($activity)), - fn (string $line) => str_contains($line, 'docker compose ') && (str_contains($line, ' up -d') || str_contains($line, ' pull')), + fn (string $line) => str_contains($line, 'docker compose ') && (str_contains($line, ' up -d') || str_contains($line, ' pull') || str_contains($line, ' start')), )); } @@ -130,7 +130,7 @@ it('gives service compose up and pull no stdin and never answers the recreate pr $lines = serviceComposeCommandLines(StartService::run($service, pullLatestImages: true)); - expect($lines)->toHaveCount(2); + expect($lines)->toHaveCount(3); foreach ($lines as $line) { expect($line)->toStartWith('sudo docker compose ') ->toEndWith(' < /dev/null') @@ -138,3 +138,52 @@ it('gives service compose up and pull no stdin and never answers the recreate pr ->not->toContain(' -y '); } }); + +/** + * @param list $calls + */ +function serviceComposeCallIndex(array $calls, string ...$needles): int +{ + foreach ($calls as $index => $call) { + if (collect($needles)->every(fn (string $needle) => str_contains($call, $needle))) { + return $index; + } + } + + throw new RuntimeException('No call contains: '.implode(', ', $needles)); +} + +it('connects service containers to the predefined network before compose starts them', function (string $sshUser) { + $compose = "services:\n db:\n image: busybox:latest\n healthcheck:\n test: ['CMD-SHELL', 'true']\n app:\n image: busybox:latest\n depends_on:\n db:\n condition: service_healthy\n"; + $service = serviceComposeStdinService($sshUser, $compose); + + $calls = runServiceScriptLikeSsh(RemoteProcessCommand::read(StartService::run($service)), $this->stubDirectory); + + $create = serviceComposeCallIndex($calls, ' compose ', ' up -d ', ' --no-start'); + $start = serviceComposeCallIndex($calls, ' compose ', " --project-name {$service->uuid} start"); + foreach (['db', 'app'] as $name) { + $connect = serviceComposeCallIndex($calls, "docker network connect --alias {$name}-{$service->uuid} coolify {$name}-{$service->uuid}"); + expect($connect)->toBeGreaterThan($create)->toBeLessThan($start); + } +})->with(['root' => 'root', 'non-root' => 'deploy']); + +it('does not connect containers that use a network mode or already declare the predefined network', function () { + $compose = "services:\n vpn:\n image: busybox:latest\n networks:\n - default\n - coolify\n app:\n image: busybox:latest\n network_mode: service:vpn\nnetworks:\n coolify:\n external: true\n"; + $service = serviceComposeStdinService('root', $compose); + + $calls = runServiceScriptLikeSsh(RemoteProcessCommand::read(StartService::run($service)), $this->stubDirectory); + + expect(collect($calls)->filter(fn (string $call) => str_contains($call, 'network connect --alias'))->all())->toBe([]); +}); + +it('connects a single service container to the predefined network before it starts', function (string $sshUser) { + $service = serviceComposeStdinService($sshUser); + $service->parse(); + + $calls = runServiceScriptLikeSsh(RemoteProcessCommand::read(DeployServiceApplication::run($service->applications()->firstOrFail())), $this->stubDirectory); + + $create = serviceComposeCallIndex($calls, ' compose ', ' up -d --no-deps --no-start app'); + $connect = serviceComposeCallIndex($calls, "docker network connect --alias app-{$service->uuid} coolify app-{$service->uuid}"); + $start = serviceComposeCallIndex($calls, "docker start app-{$service->uuid}"); + expect($connect)->toBeGreaterThan($create)->toBeLessThan($start); +})->with(['root' => 'root', 'non-root' => 'deploy']);