fix: resolve Docker validation race conditions and sudo prefix bug
- Fix sudo prefix bug: Use word boundary matching to prevent 'do' keyword from matching 'docker' commands - Add ensureProxyNetworksExist() helper to create networks before docker compose up - Ensure networks exist synchronously before dispatching async proxy startup to prevent race conditions - Update comprehensive unit tests for sudo parsing (50 tests passing) This resolves issues where Docker commands failed to execute with sudo on non-root servers and where proxy networks were not created before the proxy container started. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Andras Bacsai
parent
e5c7459284
commit
246e3cd8a2
6 changed files with 191 additions and 32 deletions
|
|
@ -75,6 +75,10 @@ public function handle(Server $server, bool $async = true, bool $force = false,
|
|||
' done',
|
||||
" echo 'Successfully stopped and removed existing coolify-proxy.'",
|
||||
'fi',
|
||||
]);
|
||||
// Ensure required networks exist BEFORE docker compose up (networks are declared as external)
|
||||
$commands = $commands->merge(ensureProxyNetworksExist($server));
|
||||
$commands = $commands->merge([
|
||||
"echo 'Starting coolify-proxy.'",
|
||||
'docker compose up -d --wait --remove-orphans',
|
||||
"echo 'Successfully started coolify-proxy.'",
|
||||
|
|
|
|||
|
|
@ -168,6 +168,9 @@ public function handle(): void
|
|||
if (! $this->server->isBuildServer()) {
|
||||
$proxyShouldRun = CheckProxy::run($this->server, true);
|
||||
if ($proxyShouldRun) {
|
||||
// Ensure networks exist BEFORE dispatching async proxy startup
|
||||
// This prevents race condition where proxy tries to start before networks are created
|
||||
instant_remote_process(ensureProxyNetworksExist($this->server)->toArray(), $this->server, false);
|
||||
StartProxy::dispatch($this->server);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -206,6 +206,9 @@ public function validateDockerVersion()
|
|||
if (! $proxyShouldRun) {
|
||||
return;
|
||||
}
|
||||
// Ensure networks exist BEFORE dispatching async proxy startup
|
||||
// This prevents race condition where proxy tries to start before networks are created
|
||||
instant_remote_process(ensureProxyNetworksExist($this->server)->toArray(), $this->server, false);
|
||||
StartProxy::dispatch($this->server);
|
||||
} else {
|
||||
$requiredDockerVersion = str(config('constants.docker.minimum_required_version'))->before('.');
|
||||
|
|
|
|||
|
|
@ -108,6 +108,37 @@ function connectProxyToNetworks(Server $server)
|
|||
|
||||
return $commands->flatten();
|
||||
}
|
||||
|
||||
/**
|
||||
* Ensures all required networks exist before docker compose up.
|
||||
* This must be called BEFORE docker compose up since the compose file declares networks as external.
|
||||
*
|
||||
* @param Server $server The server to ensure networks on
|
||||
* @return \Illuminate\Support\Collection Commands to create networks if they don't exist
|
||||
*/
|
||||
function ensureProxyNetworksExist(Server $server)
|
||||
{
|
||||
['allNetworks' => $networks] = collectDockerNetworksByServer($server);
|
||||
|
||||
if ($server->isSwarm()) {
|
||||
$commands = $networks->map(function ($network) {
|
||||
return [
|
||||
"echo 'Ensuring network $network exists...'",
|
||||
"docker network ls --format '{{.Name}}' | grep -q '^{$network}$' || docker network create --driver overlay --attachable $network",
|
||||
];
|
||||
});
|
||||
} else {
|
||||
$commands = $networks->map(function ($network) {
|
||||
return [
|
||||
"echo 'Ensuring network $network exists...'",
|
||||
"docker network ls --format '{{.Name}}' | grep -q '^{$network}$' || docker network create --attachable $network",
|
||||
];
|
||||
});
|
||||
}
|
||||
|
||||
return $commands->flatten();
|
||||
}
|
||||
|
||||
function extractCustomProxyCommands(Server $server, string $existing_config): array
|
||||
{
|
||||
$custom_commands = [];
|
||||
|
|
|
|||
|
|
@ -23,38 +23,51 @@ function shouldChangeOwnership(string $path): bool
|
|||
function parseCommandsByLineForSudo(Collection $commands, Server $server): array
|
||||
{
|
||||
$commands = $commands->map(function ($line) {
|
||||
if (
|
||||
! str(trim($line))->startsWith([
|
||||
'cd',
|
||||
'command',
|
||||
'echo',
|
||||
'true',
|
||||
'if',
|
||||
'fi',
|
||||
'for',
|
||||
'do',
|
||||
'done',
|
||||
'while',
|
||||
'until',
|
||||
'case',
|
||||
'esac',
|
||||
'select',
|
||||
'then',
|
||||
'else',
|
||||
'elif',
|
||||
'break',
|
||||
'continue',
|
||||
'#',
|
||||
])
|
||||
) {
|
||||
return "sudo $line";
|
||||
$trimmedLine = trim($line);
|
||||
|
||||
// All bash keywords that should not receive sudo prefix
|
||||
// Using word boundary matching to avoid prefix collisions (e.g., 'do' vs 'docker', 'if' vs 'ifconfig', 'fi' vs 'find')
|
||||
$bashKeywords = [
|
||||
'cd',
|
||||
'command',
|
||||
'echo',
|
||||
'true',
|
||||
'if',
|
||||
'fi',
|
||||
'for',
|
||||
'done',
|
||||
'while',
|
||||
'until',
|
||||
'case',
|
||||
'esac',
|
||||
'select',
|
||||
'then',
|
||||
'else',
|
||||
'elif',
|
||||
'break',
|
||||
'continue',
|
||||
'do',
|
||||
];
|
||||
|
||||
// Special case: comments (no collision risk with '#')
|
||||
if (str_starts_with($trimmedLine, '#')) {
|
||||
return $line;
|
||||
}
|
||||
|
||||
if (str(trim($line))->startsWith('if')) {
|
||||
return str_replace('if', 'if sudo', $line);
|
||||
// Check all keywords with word boundary matching
|
||||
// Match keyword followed by space, semicolon, or end of line
|
||||
foreach ($bashKeywords as $keyword) {
|
||||
if (preg_match('/^'.preg_quote($keyword, '/').'(\s|;|$)/', $trimmedLine)) {
|
||||
// Special handling for 'if' - insert sudo after 'if '
|
||||
if ($keyword === 'if') {
|
||||
return preg_replace('/^(\s*)if\s+/', '$1if sudo ', $line);
|
||||
}
|
||||
|
||||
return $line;
|
||||
}
|
||||
}
|
||||
|
||||
return $line;
|
||||
return "sudo $line";
|
||||
});
|
||||
|
||||
$commands = $commands->map(function ($line) use ($server) {
|
||||
|
|
|
|||
|
|
@ -272,11 +272,9 @@
|
|||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
// Note: 'docker' starts with 'do' which is an excluded keyword, so it doesn't get sudo prefix
|
||||
// This is a known limitation of the startsWith approach. Docker commands still work because
|
||||
// they typically appear in more complex command sequences or are handled separately.
|
||||
// docker commands now correctly get sudo prefix (word boundary fix for 'do' keyword)
|
||||
// The || operator adds sudo to what follows, and subshell adds sudo inside $()
|
||||
expect($result[0])->toBe('docker ps || sudo echo $(sudo date)');
|
||||
expect($result[0])->toBe('sudo docker ps || sudo echo $(sudo date)');
|
||||
});
|
||||
|
||||
test('handles whitespace-only commands gracefully', function () {
|
||||
|
|
@ -516,3 +514,110 @@
|
|||
expect($result[0])->not->toContain('sudo select');
|
||||
expect($result[2])->toBe(' break');
|
||||
});
|
||||
|
||||
// Tests for word boundary matching - ensuring commands are not confused with bash keywords
|
||||
|
||||
test('adds sudo for ifconfig command (not confused with if keyword)', function () {
|
||||
$commands = collect(['ifconfig eth0']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo ifconfig eth0');
|
||||
expect($result[0])->not->toContain('if sudo');
|
||||
});
|
||||
|
||||
test('adds sudo for ifup command (not confused with if keyword)', function () {
|
||||
$commands = collect(['ifup eth0']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo ifup eth0');
|
||||
});
|
||||
|
||||
test('adds sudo for ifdown command (not confused with if keyword)', function () {
|
||||
$commands = collect(['ifdown eth0']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo ifdown eth0');
|
||||
});
|
||||
|
||||
test('adds sudo for find command (not confused with fi keyword)', function () {
|
||||
$commands = collect(['find /var -name "*.log"']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo find /var -name "*.log"');
|
||||
});
|
||||
|
||||
test('adds sudo for file command (not confused with fi keyword)', function () {
|
||||
$commands = collect(['file /tmp/test']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo file /tmp/test');
|
||||
});
|
||||
|
||||
test('adds sudo for finger command (not confused with fi keyword)', function () {
|
||||
$commands = collect(['finger user']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo finger user');
|
||||
});
|
||||
|
||||
test('adds sudo for docker command (not confused with do keyword)', function () {
|
||||
$commands = collect(['docker ps']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo docker ps');
|
||||
});
|
||||
|
||||
test('adds sudo for fortune command (not confused with for keyword)', function () {
|
||||
$commands = collect(['fortune']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo fortune');
|
||||
});
|
||||
|
||||
test('adds sudo for formail command (not confused with for keyword)', function () {
|
||||
$commands = collect(['formail -s procmail']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('sudo formail -s procmail');
|
||||
});
|
||||
|
||||
test('if keyword with word boundary gets sudo inserted correctly', function () {
|
||||
$commands = collect(['if [ -f /tmp/test ]; then']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('if sudo [ -f /tmp/test ]; then');
|
||||
});
|
||||
|
||||
test('fi keyword with word boundary is not given sudo', function () {
|
||||
$commands = collect(['fi']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('fi');
|
||||
});
|
||||
|
||||
test('for keyword with word boundary is not given sudo', function () {
|
||||
$commands = collect(['for i in 1 2 3; do']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('for i in 1 2 3; do');
|
||||
});
|
||||
|
||||
test('do keyword with word boundary is not given sudo', function () {
|
||||
$commands = collect(['do']);
|
||||
|
||||
$result = parseCommandsByLineForSudo($commands, $this->server);
|
||||
|
||||
expect($result[0])->toBe('do');
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue