From fcf92b751f24605c670f41d6ab6df13265254c7c Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 3 Nov 2026 20:46:44 +0100 Subject: [PATCH] fix(healthcheck): read the Dockerfile HEALTHCHECK only from the built stage Closes #9475 Co-Authored-By: Claude Opus 5.5 --- app/Models/Application.php | 59 +++------ bootstrap/helpers/shared.php | 69 ++++++++++ .../ApplicationHealthcheckRemovalTest.php | 120 ++++++++++++------ 3 files changed, 170 insertions(+), 78 deletions(-) diff --git a/app/Models/Application.php b/app/Models/Application.php index a21d8de6ea..3b7de984ac 100644 --- a/app/Models/Application.php +++ b/app/Models/Application.php @@ -2850,8 +2850,8 @@ class Application extends BaseModel public function parseHealthcheckFromDockerfile($dockerfile, bool $isInit = false) { - $dockerfile = str($dockerfile)->trim()->explode("\n"); - $hasHealthcheck = str($dockerfile)->contains('HEALTHCHECK'); + $healthcheckCommand = dockerfileHealthcheckInstruction($dockerfile, $this->dockerfile_target_build); + $hasHealthcheck = $healthcheckCommand !== null; // Always check if healthcheck was removed, regardless of health_check_enabled setting if (! $hasHealthcheck && $this->custom_healthcheck_found) { @@ -2867,45 +2867,26 @@ class Application extends BaseModel } if ($hasHealthcheck && ($this->isHealthcheckDisabled() || $isInit)) { - $healthcheckCommand = null; - $lines = $dockerfile->toArray(); - foreach ($lines as $line) { - $trimmedLine = trim($line); - if (str_starts_with($trimmedLine, 'HEALTHCHECK')) { - $healthcheckCommand .= trim($trimmedLine, '\\ '); + $interval = str($healthcheckCommand)->match('/--interval=([0-9]+[a-zµ]*)/'); + $timeout = str($healthcheckCommand)->match('/--timeout=([0-9]+[a-zµ]*)/'); + $start_period = str($healthcheckCommand)->match('/--start-period=([0-9]+[a-zµ]*)/'); + $retries = str($healthcheckCommand)->match('/--retries=(\d+)/'); - continue; - } - if (isset($healthcheckCommand) && str_contains($trimmedLine, '\\')) { - $healthcheckCommand .= ' '.trim($trimmedLine, '\\ '); - } - if (isset($healthcheckCommand) && ! str_contains($trimmedLine, '\\') && ! empty($healthcheckCommand)) { - $healthcheckCommand .= ' '.$trimmedLine; - break; - } + if ($interval->isNotEmpty()) { + $this->health_check_interval = parseDockerfileInterval($interval); } - if (str($healthcheckCommand)->isNotEmpty()) { - $interval = str($healthcheckCommand)->match('/--interval=([0-9]+[a-zµ]*)/'); - $timeout = str($healthcheckCommand)->match('/--timeout=([0-9]+[a-zµ]*)/'); - $start_period = str($healthcheckCommand)->match('/--start-period=([0-9]+[a-zµ]*)/'); - $retries = str($healthcheckCommand)->match('/--retries=(\d+)/'); - - if ($interval->isNotEmpty()) { - $this->health_check_interval = parseDockerfileInterval($interval); - } - if ($timeout->isNotEmpty()) { - $this->health_check_timeout = parseDockerfileInterval($timeout); - } - if ($start_period->isNotEmpty()) { - $this->health_check_start_period = parseDockerfileInterval($start_period); - } - if ($retries->isNotEmpty()) { - $this->health_check_retries = $retries->toInteger(); - } - if ($interval || $timeout || $start_period || $retries) { - $this->custom_healthcheck_found = true; - $this->save(); - } + if ($timeout->isNotEmpty()) { + $this->health_check_timeout = parseDockerfileInterval($timeout); + } + if ($start_period->isNotEmpty()) { + $this->health_check_start_period = parseDockerfileInterval($start_period); + } + if ($retries->isNotEmpty()) { + $this->health_check_retries = $retries->toInteger(); + } + if ($interval || $timeout || $start_period || $retries) { + $this->custom_healthcheck_found = true; + $this->save(); } } } diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 6285ea97d5..98298709c2 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -5126,6 +5126,75 @@ function getJobStatus(?string $jobId = null) return $jobFound->first()->status; } +/** + * Find the HEALTHCHECK instruction that applies to the image built from the Dockerfile. + * + * Docker builds the target stage, or the last stage when no target is set. A stage keeps the + * HEALTHCHECK of an earlier stage that it starts FROM, and the last HEALTHCHECK of a stage wins. + * Returns null when no HEALTHCHECK applies or when it is HEALTHCHECK NONE. + */ +function dockerfileHealthcheckInstruction(?string $dockerfile, ?string $target = null): ?string +{ + $instructions = []; + $current = ''; + foreach (preg_split('/\R/', (string) $dockerfile) as $line) { + $line = trim($line); + if ($line === '' || str_starts_with($line, '#')) { + continue; + } + $continues = str_ends_with($line, '\\'); + $current = trim($current.' '.rtrim($line, '\\ ')); + if (! $continues) { + $instructions[] = $current; + $current = ''; + } + } + if ($current !== '') { + $instructions[] = $current; + } + + /** @var array $stages */ + $stages = []; + foreach ($instructions as $instruction) { + if (preg_match('/^FROM\s+(?:--\S+\s+)*(\S+)(?:\s+AS\s+(\S+))?/i', $instruction, $matches)) { + $stages[] = [ + 'name' => isset($matches[2]) ? strtolower($matches[2]) : null, + 'parent' => strtolower($matches[1]), + 'healthcheck' => null, + ]; + } elseif ($stages !== [] && preg_match('/^HEALTHCHECK(\s|$)/i', $instruction)) { + $stages[array_key_last($stages)]['healthcheck'] = $instruction; + } + } + if ($stages === []) { + return null; + } + + $target = filled($target) ? strtolower(trim($target)) : null; + $index = $target === null ? array_key_last($stages) : null; + foreach ($stages as $i => $stage) { + if ($target !== null && $stage['name'] === $target) { + $index = $i; + } + } + + while ($index !== null) { + $stage = $stages[$index]; + if ($stage['healthcheck'] !== null) { + return preg_match('/^HEALTHCHECK\s+NONE$/i', $stage['healthcheck']) ? null : $stage['healthcheck']; + } + $parentIndex = null; + foreach ($stages as $i => $candidate) { + if ($i < $index && $candidate['name'] === $stage['parent']) { + $parentIndex = $i; + } + } + $index = $parentIndex; + } + + return null; +} + function parseDockerfileInterval(string $something) { $value = preg_replace('/[^0-9]/', '', $something); diff --git a/tests/Unit/ApplicationHealthcheckRemovalTest.php b/tests/Unit/ApplicationHealthcheckRemovalTest.php index 2ccb6be01b..9dc5d53f0b 100644 --- a/tests/Unit/ApplicationHealthcheckRemovalTest.php +++ b/tests/Unit/ApplicationHealthcheckRemovalTest.php @@ -1,57 +1,99 @@ makePartial(); + $application->shouldReceive('save')->andReturnTrue(); + $application->forceFill([ + 'health_check_enabled' => false, + 'custom_healthcheck_found' => false, + 'health_check_interval' => 5, + 'health_check_timeout' => 5, + 'health_check_retries' => 10, + 'health_check_start_period' => 5, + 'dockerfile_target_build' => null, + ...$attributes, + ]); - $dockerfile = str("FROM nginx:latest\nCOPY . /app\nEXPOSE 80")->trim()->explode("\n"); + return $application; +} - // The key fix: hasHealthcheck check happens BEFORE the isHealthcheckDisabled check - $hasHealthcheck = str($dockerfile)->contains('HEALTHCHECK'); +const MULTI_STAGE_DOCKERFILE = <<<'DOCKERFILE' +FROM node:20-alpine AS builder +RUN npm ci - // Simulate an application with custom_healthcheck_found = true - $customHealthcheckFound = true; +FROM node:20-alpine AS api +COPY --from=builder /app/dist ./dist +HEALTHCHECK --interval=30s --timeout=5s --start-period=15s --retries=3 \ + CMD node -e "require('http').get('http://localhost:3000/health')" +CMD ["node", "dist/main.js"] - // The fixed logic: This condition should be true when HEALTHCHECK is removed - $shouldReset = ! $hasHealthcheck && $customHealthcheckFound; +# === Stage: worker === +FROM node:20-alpine AS worker +COPY --from=builder /app/dist ./dist +# No HEALTHCHECK - this stage has no HTTP server +CMD ["node", "dist/worker.js"] +DOCKERFILE; - expect($shouldReset)->toBeTrue() - ->and($hasHealthcheck)->toBeFalse() - ->and($customHealthcheckFound)->toBeTrue(); +it('resets the custom healthcheck when HEALTHCHECK is removed from the Dockerfile', function () { + $application = healthcheckApplication(['custom_healthcheck_found' => true, 'health_check_interval' => 30]); + + $application->parseHealthcheckFromDockerfile("FROM nginx:latest\nCOPY . /app\nEXPOSE 80"); + + expect($application->custom_healthcheck_found)->toBeFalse() + ->and($application->health_check_interval)->toBe(5); }); -it('does not reset when HEALTHCHECK exists in dockerfile', function () { - $dockerfile = str("FROM nginx:latest\nHEALTHCHECK --interval=30s CMD curl\nEXPOSE 80")->trim()->explode("\n"); +it('reads the HEALTHCHECK options of a single-stage Dockerfile', function () { + $application = healthcheckApplication(); - $hasHealthcheck = str($dockerfile)->contains('HEALTHCHECK'); - $customHealthcheckFound = true; + $application->parseHealthcheckFromDockerfile("FROM nginx:latest\nHEALTHCHECK --interval=30s --retries=4 \\\n CMD curl -f http://localhost/\nEXPOSE 80"); - // When healthcheck exists, should not reset - $shouldReset = ! $hasHealthcheck && $customHealthcheckFound; - - expect($shouldReset)->toBeFalse() - ->and($hasHealthcheck)->toBeTrue(); + expect($application->custom_healthcheck_found)->toBeTrue() + ->and($application->health_check_interval)->toBe(30) + ->and($application->health_check_retries)->toBe(4); }); -it('does not reset when custom_healthcheck_found is false', function () { - $dockerfile = str("FROM nginx:latest\nCOPY . /app\nEXPOSE 80")->trim()->explode("\n"); +it('ignores a HEALTHCHECK of another stage than the build target', function () { + $application = healthcheckApplication(['dockerfile_target_build' => 'worker']); - $hasHealthcheck = str($dockerfile)->contains('HEALTHCHECK'); - $customHealthcheckFound = false; + $application->parseHealthcheckFromDockerfile(MULTI_STAGE_DOCKERFILE); - // When custom_healthcheck_found is false, no need to reset - $shouldReset = ! $hasHealthcheck && $customHealthcheckFound; - - expect($shouldReset)->toBeFalse() - ->and($customHealthcheckFound)->toBeFalse(); + expect($application->custom_healthcheck_found)->toBeFalse() + ->and($application->health_check_interval)->toBe(5); }); + +it('resets a custom healthcheck found earlier when the build target has no HEALTHCHECK', function () { + $application = healthcheckApplication(['dockerfile_target_build' => 'worker', 'custom_healthcheck_found' => true]); + + $application->parseHealthcheckFromDockerfile(MULTI_STAGE_DOCKERFILE); + + expect($application->custom_healthcheck_found)->toBeFalse(); +}); + +it('reads the HEALTHCHECK of the build target stage', function () { + $application = healthcheckApplication(['dockerfile_target_build' => 'API']); + + $application->parseHealthcheckFromDockerfile(MULTI_STAGE_DOCKERFILE); + + expect($application->custom_healthcheck_found)->toBeTrue() + ->and($application->health_check_interval)->toBe(30) + ->and($application->health_check_start_period)->toBe(15) + ->and($application->health_check_retries)->toBe(3); +}); + +it('finds the HEALTHCHECK that applies to the built image', function (string $dockerfile, ?string $target, ?string $expected) { + expect(dockerfileHealthcheckInstruction($dockerfile, $target))->toBe($expected); +})->with([ + 'last stage when no target is set' => [MULTI_STAGE_DOCKERFILE, null, null], + 'HEALTHCHECK only in a builder stage' => ["FROM node AS build\nHEALTHCHECK CMD true\nFROM nginx\nCOPY --from=build /a /b", null, null], + 'inherited from a parent stage' => ["FROM node AS base\nHEALTHCHECK --interval=10s CMD true\nFROM base AS app\nCMD run", 'app', 'HEALTHCHECK --interval=10s CMD true'], + 'HEALTHCHECK NONE overrides the parent stage' => ["FROM node AS base\nHEALTHCHECK CMD true\nFROM base AS worker\nHEALTHCHECK NONE", 'worker', null], + 'last HEALTHCHECK of a stage wins' => ["FROM node\nHEALTHCHECK --interval=1s CMD a\nHEALTHCHECK --interval=2s CMD b", null, 'HEALTHCHECK --interval=2s CMD b'], + 'FROM with platform flag' => ["FROM --platform=linux/amd64 node AS app\nhealthcheck --retries=2 CMD true", 'app', 'healthcheck --retries=2 CMD true'], + 'target that does not exist' => ["FROM node AS app\nHEALTHCHECK CMD true", 'missing', null], + 'comment mentions HEALTHCHECK' => ["FROM node\n# HEALTHCHECK is not needed\nCMD run", null, null], + 'empty Dockerfile' => ['', null, null], +]);