From ef57eea32037633c8e8ad89b35e2ce2f1d4f4387 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 28 Oct 2026 10:35:46 +0100 Subject: [PATCH] fix(backups): sign mc requests with S3v4 and retry failed S3 uploads --- app/Actions/Database/StartDatabaseImport.php | 2 +- app/Jobs/DatabaseBackupJob.php | 6 +-- app/Jobs/VolumeBackupJob.php | 6 +-- app/Models/S3Storage.php | 33 ++++++++++-- tests/Feature/BackupS3CredentialsArgvTest.php | 4 +- .../BackupS3DestinationRedactionTest.php | 2 + .../StartDatabaseImportS3CredentialsTest.php | 2 +- ...VolumeBackupMultipleS3DestinationsTest.php | 24 +++++++++ tests/Unit/S3StorageTest.php | 54 +++++++++++++++++-- 9 files changed, 115 insertions(+), 18 deletions(-) diff --git a/app/Actions/Database/StartDatabaseImport.php b/app/Actions/Database/StartDatabaseImport.php index d9e491345c..f8c3c861ba 100644 --- a/app/Actions/Database/StartDatabaseImport.php +++ b/app/Actions/Database/StartDatabaseImport.php @@ -318,7 +318,7 @@ class StartDatabaseImport private function setMinioClientAlias(S3Storage $storage, Server $server, string $helper): void { $remoteCommand = ($server->isNonRoot() ? 'sudo ' : '').'docker exec -i '.escapeshellarg($helper) - .' mc alias set'.$storage->minioClientPathOption().' s3temp '.escapeshellarg((string) $storage->endpoint).' >/dev/null'; + .' mc alias set'.$storage->minioClientAliasOptions().' s3temp '.escapeshellarg((string) $storage->endpoint).' >/dev/null'; $timeout = (int) config('constants.ssh.command_timeout'); SshRetryHandler::retry( diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index 07e2fd56e3..2fe350e37d 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -949,10 +949,10 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue instant_remote_process($commands, $this->server, true, false, $this->commandTimeout(), disableMultiplexing: true); $this->setMinioClientAlias($this->server, "backup-of-{$this->backup_log_uuid}", $s3, $resolveOptions); - instant_remote_process( + S3Storage::retryMinioClientUpload(fn () => instant_remote_process( ["docker exec backup-of-{$this->backup_log_uuid} mc cp{$resolveOptions} {$escapedBackupLocation} {$escapedS3Destination}"], $this->server, true, false, $this->commandTimeout(), disableMultiplexing: true, - ); + )); } catch (Throwable $e) { $this->add_to_error_output($e->getMessage()); @@ -970,7 +970,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function setMinioClientAlias(Server $server, string $container, S3Storage $s3, string $resolveOptions): void { $remoteCommand = ($server->isNonRoot() ? 'sudo ' : '').'docker exec -i '.escapeshellarg($container) - .' mc alias set'.$resolveOptions.$s3->minioClientPathOption().' temporary '.escapeshellarg($s3->endpoint).' >/dev/null'; + .' mc alias set'.$resolveOptions.$s3->minioClientAliasOptions().' temporary '.escapeshellarg($s3->endpoint).' >/dev/null'; $timeout = (int) config('constants.ssh.command_timeout'); SshRetryHandler::retry( diff --git a/app/Jobs/VolumeBackupJob.php b/app/Jobs/VolumeBackupJob.php index b7524c7012..42f209d1e5 100644 --- a/app/Jobs/VolumeBackupJob.php +++ b/app/Jobs/VolumeBackupJob.php @@ -449,10 +449,10 @@ class VolumeBackupJob implements ShouldBeEncrypted, ShouldQueue .escapeshellarg($backupLocation.':'.$backupLocation.':ro').' '.escapeshellarg($image), ], $server, timeout: $this->timeout, disableMultiplexing: true); $this->setMinioClientAlias($server, $containerName, $s3, $resolveOptions); - instant_remote_process([ + S3Storage::retryMinioClientUpload(fn () => instant_remote_process([ 'docker exec '.escapeshellarg($containerName).' mc cp'.$resolveOptions.' '.escapeshellarg($backupLocation).' ' .escapeshellarg('temporary/'.$s3->bucket.$backupDirectory.'/'), - ], $server, timeout: $this->timeout, disableMultiplexing: true); + ], $server, timeout: $this->timeout, disableMultiplexing: true)); } finally { instant_remote_process( ['docker rm -f '.escapeshellarg($containerName)], @@ -487,7 +487,7 @@ class VolumeBackupJob implements ShouldBeEncrypted, ShouldQueue private function setMinioClientAlias(Server $server, string $container, S3Storage $s3, string $resolveOptions): void { $remoteCommand = ($server->isNonRoot() ? 'sudo ' : '').'docker exec -i '.escapeshellarg($container) - .' mc alias set'.$resolveOptions.$s3->minioClientPathOption().' temporary '.escapeshellarg($s3->endpoint).' >/dev/null'; + .' mc alias set'.$resolveOptions.$s3->minioClientAliasOptions().' temporary '.escapeshellarg($s3->endpoint).' >/dev/null'; $timeout = (int) config('constants.ssh.command_timeout'); SshRetryHandler::retry( diff --git a/app/Models/S3Storage.php b/app/Models/S3Storage.php index 6bd9f88322..84bf072dc1 100644 --- a/app/Models/S3Storage.php +++ b/app/Models/S3Storage.php @@ -34,6 +34,16 @@ class S3Storage extends BaseModel */ private const PATH_STYLE_REJECTION_ERRORS = ['secondleveldomainforbidden', 'pathstyledomainforbidden', 'virtual hosted style', 'virtual-hosted style', 'virtual-styled']; + /** + * Waits before the second and the third attempt of an upload with the MinIO client. + */ + private const MINIO_CLIENT_UPLOAD_RETRY_DELAYS_MS = [10_000, 30_000]; + + /** + * The exception code of runSshProcess() when a remote command times out. + */ + private const REMOTE_COMMAND_TIMEOUT_EXIT_CODE = 124; + protected $fillable = [ 'team_id', 'name', @@ -349,12 +359,27 @@ class S3Storage extends BaseModel } /** - * The `mc alias set` option that makes the MinIO client use virtual-hosted-style URLs, as filesystem() does. - * With path-style URLs, mc keeps its own `auto` lookup, as before. + * The `mc alias set` options. `--api S3v4` signs as filesystem() does. Without it, mc probes the signature + * version on every run, and a network error during the probe makes it try S3v2, which most providers reject. + * `--path off` uses virtual-hosted-style URLs, as filesystem() does. With path-style URLs, mc keeps its own + * `auto` lookup, as before. */ - public function minioClientPathOption(): string + public function minioClientAliasOptions(): string { - return $this->usesPathStyleEndpoint() ? '' : ' --path off'; + return ' --api S3v4'.($this->usesPathStyleEndpoint() ? '' : ' --path off'); + } + + /** + * Runs an upload of a local file with the MinIO client again when it fails, for example after a short network + * outage. A timeout is not retried, because another full attempt can pass the timeout of the backup job. + */ + public static function retryMinioClientUpload(callable $upload): mixed + { + return retry( + self::MINIO_CLIENT_UPLOAD_RETRY_DELAYS_MS, + $upload, + when: fn (\Throwable $exception): bool => $exception->getCode() !== self::REMOTE_COMMAND_TIMEOUT_EXIT_CODE, + ); } /** diff --git a/tests/Feature/BackupS3CredentialsArgvTest.php b/tests/Feature/BackupS3CredentialsArgvTest.php index 5a2775751b..3ecd360a76 100644 --- a/tests/Feature/BackupS3CredentialsArgvTest.php +++ b/tests/Feature/BackupS3CredentialsArgvTest.php @@ -201,8 +201,8 @@ it('makes mc use virtual-hosted-style URLs when the storage turns path-style URL $aliasProcess = $processes->sole(fn (array $process) => str_contains($process['command'], 'mc alias set')); expect(s3ArgvTestRunAliasProcess($aliasProcess)['argv'])->toContain($expectedAlias); })->with([ - 'path-style' => [true, 'mc alias set temporary https://8.8.8.8'], - 'virtual-hosted-style' => [false, 'mc alias set --path off temporary https://8.8.8.8'], + 'path-style' => [true, 'mc alias set --api S3v4 temporary https://8.8.8.8'], + 'virtual-hosted-style' => [false, 'mc alias set --api S3v4 --path off temporary https://8.8.8.8'], ]); it('passes the S3 keys of a volume backup to mc on stdin, not in a command line', function (string $sshUser, bool $streamToS3) { diff --git a/tests/Feature/BackupS3DestinationRedactionTest.php b/tests/Feature/BackupS3DestinationRedactionTest.php index 92deab9ded..81cf689f99 100644 --- a/tests/Feature/BackupS3DestinationRedactionTest.php +++ b/tests/Feature/BackupS3DestinationRedactionTest.php @@ -24,6 +24,7 @@ use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Notification; use Illuminate\Support\Facades\Process; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Sleep; uses(RefreshDatabase::class); @@ -41,6 +42,7 @@ beforeEach(function () { defined('CURLOPT_RESOLVE') || define('CURLOPT_RESOLVE', 10203); Event::fake([BackupCreated::class]); Notification::fake(); + Sleep::fake(); InstanceSettings::unguarded(fn () => InstanceSettings::query()->firstOrCreate(['id' => 0])); $this->team = Team::factory()->create(); diff --git a/tests/Feature/StartDatabaseImportS3CredentialsTest.php b/tests/Feature/StartDatabaseImportS3CredentialsTest.php index c53b343711..15b0041535 100644 --- a/tests/Feature/StartDatabaseImportS3CredentialsTest.php +++ b/tests/Feature/StartDatabaseImportS3CredentialsTest.php @@ -300,7 +300,7 @@ it('passes the S3 keys of an import to mc on stdin, not in a command line', func $run = importS3ArgvTestRunAliasProcess($aliasProcess); expect($run['mcStdin'])->toBe(IMPORT_S3_ARGV_KEY."\n".IMPORT_S3_ARGV_SECRET."\n") - ->and($run['argv'])->toContain('mc alias set s3temp https://8.8.8.8') + ->and($run['argv'])->toContain('mc alias set --api S3v4 s3temp https://8.8.8.8') ->and($run['argv'])->not->toContain(IMPORT_S3_ARGV_SECRET) ->and($run['argv'])->not->toContain(IMPORT_S3_ARGV_KEY) ->and($processes->contains(fn (array $process) => str_contains($process['command']."\n".$process['input'], "-e 'MC_REGION=ru-central-1'")))->toBeTrue() diff --git a/tests/Feature/VolumeBackupMultipleS3DestinationsTest.php b/tests/Feature/VolumeBackupMultipleS3DestinationsTest.php index 45e5a81200..ae6826f584 100644 --- a/tests/Feature/VolumeBackupMultipleS3DestinationsTest.php +++ b/tests/Feature/VolumeBackupMultipleS3DestinationsTest.php @@ -22,12 +22,14 @@ use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Notification; use Illuminate\Support\Facades\Process; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Sleep; uses(RefreshDatabase::class); beforeEach(function () { Server::flushIdentityMap(); Notification::fake(); + Sleep::fake(); config(['broadcasting.default' => 'null']); defined('CURLOPT_RESOLVE') || define('CURLOPT_RESOLVE', 10203); }); @@ -169,6 +171,28 @@ it('keeps the local archive and cleans the partial upload when one destination f ->and($secondReplica->message)->not->toBeEmpty(); }); +it('uploads again when the first upload to a destination fails', function () { + [$backup] = createMultiDestinationVolumeBackup(); + fakeMultiDestinationS3Disks(); + $firstBucketUploads = 0; + Process::fake(function ($process) use (&$firstBucketUploads) { + $script = remoteProcessScript($process); + if (str_contains($script, 'mc cp') && str_contains($script, 'temporary/first-bucket/') && ++$firstBucketUploads === 1) { + return Process::result(errorOutput: 'dial tcp: connect: network is unreachable', exitCode: 1); + } + + return Process::result(output: str_contains($script, 'du -b') ? '128' : ''); + }); + + (new VolumeBackupJob($backup))->handle(); + + $execution = ScheduledVolumeBackupExecution::query()->sole(); + expect($firstBucketUploads)->toBe(2) + ->and($execution->status)->toBe('success') + ->and($execution->message)->toBeNull() + ->and($execution->s3_uploaded)->toBeTrue(); +}); + it('applies S3 retention to each destination separately', function () { [$backup, $first, $second, $server] = createMultiDestinationVolumeBackup([ 'retention_amount_locally' => 0, diff --git a/tests/Unit/S3StorageTest.php b/tests/Unit/S3StorageTest.php index 2778d0a741..4e06b725aa 100644 --- a/tests/Unit/S3StorageTest.php +++ b/tests/Unit/S3StorageTest.php @@ -3,6 +3,7 @@ use App\Models\S3Storage; use Illuminate\Filesystem\FilesystemAdapter; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Sleep; use Tests\TestCase; uses(TestCase::class); @@ -114,11 +115,11 @@ test('S3Storage uses path-style URLs unless the storage turns them off', functio ]); expect($s3Storage->filesystem())->toBe($disk) - ->and($s3Storage->minioClientPathOption())->toBe($mcOption); + ->and($s3Storage->minioClientAliasOptions())->toBe($mcOption); })->with([ - 'not saved yet' => [null, true, ''], - 'path-style' => [true, true, ''], - 'virtual-hosted-style' => [false, false, ' --path off'], + 'not saved yet' => [null, true, ' --api S3v4'], + 'path-style' => [true, true, ' --api S3v4'], + 'virtual-hosted-style' => [false, false, ' --api S3v4 --path off'], ]); test('S3Storage detects the URL style from the endpoint host', function (?string $endpoint, bool $usesPathStyle) { @@ -227,3 +228,48 @@ test('S3Storage testConnection rejects invalid bucket before building client', f 'newline' => ["lab\nid"], 'underscore' => ['lab_bucket'], ]); + +test('S3Storage retries a failed MinIO client upload and waits longer before each attempt', function () { + Sleep::fake(); + $attempts = 0; + + $result = S3Storage::retryMinioClientUpload(function () use (&$attempts) { + if (++$attempts < 3) { + throw new RuntimeException('dial tcp: connect: network is unreachable', 1); + } + + return 'uploaded'; + }); + + expect($result)->toBe('uploaded')->and($attempts)->toBe(3); + Sleep::assertSequence([Sleep::for(10)->seconds(), Sleep::for(30)->seconds()]); +}); + +test('S3Storage stops retrying a MinIO client upload after three attempts', function () { + Sleep::fake(); + $attempts = 0; + + $upload = function () use (&$attempts) { + $attempts++; + + throw new RuntimeException('Access Denied.', 1); + }; + + expect(fn () => S3Storage::retryMinioClientUpload($upload))->toThrow(RuntimeException::class, 'Access Denied.'); + expect($attempts)->toBe(3); +}); + +test('S3Storage does not retry a MinIO client upload that timed out', function () { + Sleep::fake(); + $attempts = 0; + + $upload = function () use (&$attempts) { + $attempts++; + + throw new RuntimeException('The command on the server timed out after 3600 seconds.', 124); + }; + + expect(fn () => S3Storage::retryMinioClientUpload($upload))->toThrow(RuntimeException::class, 'timed out'); + expect($attempts)->toBe(1); + Sleep::assertNeverSlept(); +});