Skip to content

Commit fee06ee

Browse files
TorstenDittmannTorstenDittmann
authored
Merge pull request #236 from open-runtimes/fix/docker-build-cache-command
Fix Docker build cache command handling
2 parents 6e9bc15 + 0c105a2 commit fee06ee

3 files changed

Lines changed: 142 additions & 29 deletions

File tree

‎src/Executor/Runner/Docker.php‎

Lines changed: 69 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ public function createRuntime(
256256
/** @var array<string, mixed> $container */
257257
$container = [];
258258
$output = [];
259+
$cacheWarnings = [];
259260
$startTime = \microtime(true);
260261

261262
$runtime = new Runtime(
@@ -330,9 +331,19 @@ public function createRuntime(
330331

331332
$cacheEnabled = $cacheKey !== '' && $command !== '' && $command !== '0';
332333
if ($cacheEnabled) {
333-
$this->restoreBuildCacheArtifact($cacheKey, $tmpCache, $localDevice);
334+
$this->validateBuildCacheKey($cacheKey);
335+
336+
try {
337+
$this->restoreBuildCacheArtifact($cacheKey, $tmpCache, $localDevice);
338+
} catch (Throwable $err) {
339+
$cacheWarnings[] = $this->getBuildCacheWarning('Failed to restore cache artifact: ' . $err->getMessage());
340+
341+
if (!$localDevice->exists($tmpCache)) {
342+
$localDevice->createDirectory($tmpCache);
343+
}
344+
}
345+
334346
$volumes[] = $tmpCache . ':/cache:rw';
335-
$this->prepareBuildCacheScripts($command, $tmpCache);
336347
}
337348

338349
if ($version === 'v5') {
@@ -361,33 +372,15 @@ public function createRuntime(
361372
throw new \Exception('Failed to create runtime');
362373
}
363374

364-
if ($cacheEnabled) {
365-
$command = 'bash /usr/local/server/helpers/build-cache.sh';
366-
}
367-
368375
/**
369376
* Execute any commands if they were provided
370377
*/
371378
if ($command !== '' && $command !== '0') {
372-
if ($version === 'v2') {
373-
$commands = [
374-
'sh',
375-
'-c',
376-
'touch /var/tmp/logs.txt && (' . $command . ') >> /var/tmp/logs.txt 2>&1 && cat /var/tmp/logs.txt'
377-
];
378-
} else {
379-
$commands = [
380-
'bash',
381-
'-c',
382-
'mkdir -p /tmp/logging && touch /tmp/logging/timings.txt && touch /tmp/logging/logs.txt && script --log-out /tmp/logging/logs.txt --flush --log-timing /tmp/logging/timings.txt --return --quiet --command "' . \str_replace('"', '\"', $command) . '"'
383-
];
384-
}
385-
386379
try {
387380
$stdout = '';
388381
$status = $this->orchestration->execute(
389382
name: $runtimeName,
390-
command: $commands,
383+
command: $this->getBuildCommands($command, $version),
391384
output: $stdout,
392385
timeout: $timeout
393386
);
@@ -402,15 +395,25 @@ public function createRuntime(
402395
'timestamp' => Logs::getTimestamp(),
403396
'content' => $stdout
404397
];
405-
} else {
398+
} elseif (!$cacheEnabled) {
406399
$output = Logs::get($runtimeName);
407400
}
408401
} catch (Throwable $err) {
409402
throw new ExecutorException(ExecutorException::RUNTIME_FAILED, $err->getMessage(), null, $err);
410403
}
411404

412405
if ($cacheEnabled) {
413-
$this->storeBuildCacheArtifact($cacheKey, $tmpCache, $localDevice);
406+
try {
407+
$this->storeBuildCacheArtifact($cacheKey, $tmpCache, $localDevice);
408+
} catch (Throwable $err) {
409+
$cacheWarnings[] = $this->getBuildCacheWarning('Failed to store cache artifact: ' . $err->getMessage());
410+
}
411+
412+
if ($version === 'v2') {
413+
$output = \array_merge($output, $cacheWarnings);
414+
} else {
415+
$output = \array_merge($cacheWarnings, Logs::get($runtimeName));
416+
}
414417
}
415418
}
416419

@@ -486,6 +489,10 @@ public function createRuntime(
486489
]];
487490
}
488491

492+
if ($cacheWarnings !== []) {
493+
$output = \array_merge($cacheWarnings, $output);
494+
}
495+
489496
if ($remove) {
490497
\sleep(2); // Allow time to read logs
491498
}
@@ -529,11 +536,46 @@ public function createRuntime(
529536
return $container;
530537
}
531538

532-
private function prepareBuildCacheScripts(string $command, string $tmpCache): void
539+
/**
540+
* @return string[]
541+
*/
542+
private function getBuildCommands(string $command, string $version): array
543+
{
544+
if ($version === 'v2') {
545+
return [
546+
'sh',
547+
'-c',
548+
'touch /var/tmp/logs.txt && (' . $command . ') >> /var/tmp/logs.txt 2>&1 && cat /var/tmp/logs.txt'
549+
];
550+
}
551+
552+
return [
553+
'bash',
554+
'-c',
555+
'mkdir -p /tmp/logging && touch /tmp/logging/timings.txt && touch /tmp/logging/logs.txt && script --log-out /tmp/logging/logs.txt --flush --log-timing /tmp/logging/timings.txt --return --quiet --command "' . \str_replace('"', '\"', $command) . '"'
556+
];
557+
}
558+
559+
/**
560+
* @return array{timestamp: string, content: string}
561+
*/
562+
private function getBuildCacheWarning(string $message): array
563+
{
564+
return [
565+
'timestamp' => Logs::getTimestamp(),
566+
'content' => '[build cache] Warning: ' . $message . "\n"
567+
];
568+
}
569+
570+
private function validateBuildCacheKey(string $cacheKey): void
533571
{
534-
$commandScript = "#!/bin/sh\n" . $command . "\n";
535-
if (\file_put_contents($tmpCache . '/build-command.sh', $commandScript) === false) {
536-
throw new \Exception('Failed to prepare build command script');
572+
if (
573+
\str_contains($cacheKey, "\0") ||
574+
\str_starts_with($cacheKey, '/') ||
575+
\str_contains($cacheKey, '\\') ||
576+
\preg_match('#(^|/)\.\.(?:/|$)#', $cacheKey) === 1
577+
) {
578+
throw new \Exception('Invalid build cache key', 400);
537579
}
538580
}
539581

‎tests/e2e/ExecutorTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -644,7 +644,7 @@ public function testBuildCache(): void
644644
$firstBuildOutput .= $outputItem['content'];
645645
}
646646

647-
$this->assertStringContainsString('[build cache] Saved.', $firstBuildOutput);
647+
$this->assertStringContainsString('Build cache saved.', $firstBuildOutput);
648648

649649
$params = [
650650
'runtimeId' => 'test-build-cache-hit-' . $runtimeId,
@@ -665,7 +665,7 @@ public function testBuildCache(): void
665665
$secondBuildOutput .= $outputItem['content'];
666666
}
667667

668-
$this->assertStringContainsString('[build cache] Hit.', $secondBuildOutput);
668+
$this->assertStringContainsString('Build cache hit.', $secondBuildOutput);
669669

670670
$this->assertNotEmpty($response['body']['path']);
671671

‎tests/unit/Executor/Runner/DockerTest.php‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Tests\Unit\Executor\Runner;
6+
7+
use OpenRuntimes\Executor\Runner\Docker;
8+
use PHPUnit\Framework\TestCase;
9+
use ReflectionClass;
10+
use ReflectionMethod;
11+
12+
final class DockerTest extends TestCase
13+
{
14+
public function testBuildCommandsUseOriginalUserCommand(): void
15+
{
16+
$docker = $this->createDocker();
17+
$commands = $this->invokeGetBuildCommands($docker, 'echo original-user-command', 'v5');
18+
19+
$this->assertSame('bash', $commands[0]);
20+
$this->assertStringContainsString('echo original-user-command', $commands[2]);
21+
$this->assertStringNotContainsString('build-cache.sh', $commands[2]);
22+
$this->assertStringNotContainsString('/cache/build-command.sh', $commands[2]);
23+
}
24+
25+
public function testNoBuildCommandScriptReferenceRemains(): void
26+
{
27+
$docker = $this->createDocker();
28+
29+
foreach (['v2', 'v5'] as $version) {
30+
$commands = $this->invokeGetBuildCommands($docker, 'echo test', $version);
31+
$this->assertStringNotContainsString('/cache/build-command.sh', \implode(' ', $commands));
32+
}
33+
}
34+
35+
public function testNoCacheLifecycleShellCommandsAreGenerated(): void
36+
{
37+
$docker = $this->createDocker();
38+
$commands = $this->invokeGetBuildCommands($docker, 'npm install', 'v5');
39+
$command = \implode(' ', $commands);
40+
41+
$this->assertStringNotContainsString('restore-build-cache.sh', $command);
42+
$this->assertStringNotContainsString('save-build-cache.sh', $command);
43+
$this->assertStringNotContainsString('build-cache-restore.sh', $command);
44+
$this->assertStringNotContainsString('build-cache-save.sh', $command);
45+
}
46+
47+
public function testInvalidBuildCacheKeyIsRejected(): void
48+
{
49+
$this->expectException(\Exception::class);
50+
$this->expectExceptionCode(400);
51+
52+
$method = new ReflectionMethod(Docker::class, 'validateBuildCacheKey');
53+
$method->invoke($this->createDocker(), '..');
54+
}
55+
56+
/**
57+
* @return string[]
58+
*/
59+
private function invokeGetBuildCommands(Docker $docker, string $command, string $version): array
60+
{
61+
$method = new ReflectionMethod(Docker::class, 'getBuildCommands');
62+
63+
return $method->invoke($docker, $command, $version);
64+
}
65+
66+
private function createDocker(): Docker
67+
{
68+
$reflection = new ReflectionClass(Docker::class);
69+
return $reflection->newInstanceWithoutConstructor();
70+
}
71+
}

0 commit comments

Comments
 (0)