diff --git a/app/Modules/Platform/Settings/ExternalStorageSettings.php b/app/Modules/Platform/Settings/ExternalStorageSettings.php index 8aaf248b..f73d7795 100644 --- a/app/Modules/Platform/Settings/ExternalStorageSettings.php +++ b/app/Modules/Platform/Settings/ExternalStorageSettings.php @@ -75,7 +75,38 @@ class ExternalStorageSettings extends Model public static function current(): self { - return static::query()->firstOrNew([]); + $settings = static::query()->firstOrNew([]); + + // Column defaults — the $attributes array above — apply to a NEW + // model, never to one hydrated from a row. So a row written by an + // older release, before one of these columns existed, reads that + // column as null however sensible its default is. + // + // That matters here more than it would anywhere else, because + // PlatformServiceProvider::boot() reads these settings on every + // process boot — and boot happens BEFORE `artisan migrate` runs. + // For the length of an upgrade the code is new and the schema is + // still old, and every artisan command in that window, including + // the one that would run the migrations, boots through here. + // + // A null `provider` made the match in isConfigured() throw + // UnhandledMatchError, which the official image's readiness probe + // reported to the operator as "database unreachable" — on a + // perfectly reachable database, in a container that then + // restart-looped without ever reaching the migration that would + // have fixed it (#1770, upgrading from 2.0/2.1 with external + // storage configured). + // + // Applying the defaults to a hydrated row closes that window for + // every column that has one, rather than for the single column + // where it was found. Inert on any install whose schema is current. + foreach ((new self)->getAttributes() as $column => $default) { + if (! array_key_exists($column, $settings->getAttributes())) { + $settings->setAttribute($column, $default); + } + } + + return $settings; } /** @@ -93,6 +124,11 @@ class ExternalStorageSettings extends Model // authenticate with different things entirely: S3 wants a key and // a secret, GCS wants a service account key file. // + // The match is deliberately left total rather than given a default + // arm: current() guarantees a provider even on a row older than the + // column, and a default arm here would quietly swallow a genuinely + // unhandled case instead of naming it. + // // Unless S3 is being asked to authenticate as the machine it is // running on, in which case there is no credential to fill in at // all and demanding one would leave the disk permanently diff --git a/docker/production/entrypoint.sh b/docker/production/entrypoint.sh index 18e75512..ef7d9890 100755 --- a/docker/production/entrypoint.sh +++ b/docker/production/entrypoint.sh @@ -58,12 +58,26 @@ fi # Wait for the database. `migrate` against a database still starting up is # the single most common first-run failure, and a bare failure here would # restart-loop the container with a stack trace instead of a clear message. +# +# The probe boots the whole application, so it fails for two quite different +# reasons: the database really is not there yet, or it is there and the +# application could not start. Both used to be reported as the first one, +# which sent an operator off checking credentials that were never wrong +# (#1770). The last failure is kept and printed, so whichever it was is on +# screen instead of being guessed at. if [ "$1" = "supervisord" ] || [ "$1" = "/usr/bin/supervisord" ]; then i=0 - until su-exec www-data php artisan db:show --quiet >/dev/null 2>&1; do + until probe_error=$(su-exec www-data php artisan db:show --quiet 2>&1); do i=$((i + 1)) if [ "$i" -ge 60 ]; then - echo "projectsend: database unreachable after 60s — check DB_HOST, DB_DATABASE and credentials" >&2 + echo "projectsend: gave up waiting for the database after 60s." >&2 + echo "projectsend: the last attempt failed with:" >&2 + printf '%s\n' "$probe_error" | tail -n 20 >&2 + echo "projectsend:" >&2 + echo "projectsend: if that names the database host, the connection or the credentials," >&2 + echo "projectsend: check DB_HOST, DB_DATABASE, DB_USERNAME and DB_PASSWORD." >&2 + echo "projectsend: if it is an application error, the database is fine and this is a" >&2 + echo "projectsend: bug — please report it at https://github.com/projectsend/projectsend/issues" >&2 exit 1 fi [ "$i" = 1 ] && echo "projectsend: waiting for the database..." diff --git a/tests/Feature/Platform/ExternalStorageSettingsTest.php b/tests/Feature/Platform/ExternalStorageSettingsTest.php index 44b7e62c..469152c1 100644 --- a/tests/Feature/Platform/ExternalStorageSettingsTest.php +++ b/tests/Feature/Platform/ExternalStorageSettingsTest.php @@ -8,9 +8,12 @@ use App\Modules\Files\Storage\ResolvingUploadDisk; use App\Modules\Platform\Capabilities\Edition; use App\Modules\Platform\Settings\ExternalStorageConfigApplier; use App\Modules\Platform\Settings\ExternalStorageSettings; +use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Crypt; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Event; +use Illuminate\Support\Facades\Schema; use Illuminate\Support\Facades\Storage; beforeEach(function () { @@ -239,6 +242,43 @@ test('new uploads go to the external disk with no stored credentials when the se expect($event->disk)->toBe('files_external'); }); +test('booting against a row older than the provider column does not throw', function () { + // #1770. The exact shape of a 2.0/2.1 install with S3 configured, + // upgrading to 2.2 or later: the code is new, the schema is still old, + // and boot happens before `artisan migrate` gets a chance to run. + // + // The failure this pins was not a wrong answer but a fatal one — an + // UnhandledMatchError on a null provider, thrown during + // PlatformServiceProvider::boot(), which took down every artisan + // command including the one that would have added the column. The + // official image reported it as "database unreachable" and + // restart-looped. + legacyStorageRow(); + + app(ExternalStorageConfigApplier::class)->flush(); + app(ExternalStorageConfigApplier::class)->apply(); + + // Read as S3, which is the only thing a row predating the column could + // have been — and is what the migration's own default says. + expect(config('filesystems.disks.files_external.bucket'))->toBe('my-bucket') + ->and(config('filesystems.disks.files_external.key'))->toBe('AKIAEXAMPLE') + ->and(config('filesystems.disks.files_external.driver'))->toBe('s3'); +}); + +test('an upload still reaches the external disk while the schema is mid-upgrade', function () { + // The other half: not throwing is not the same as behaving. An + // installation that was storing files in a bucket before the upgrade + // must not quietly start writing them to local disk during it. + legacyStorageRow(); + + app(ExternalStorageConfigApplier::class)->flush(); + + $event = new ResolvingUploadDisk($this->admin); + Event::dispatch($event); + + expect($event->disk)->toBe('files_external'); +}); + test('staff can save external storage settings', function () { config()->set('projectsend.edition', Edition::Community); @@ -417,6 +457,29 @@ test('a value cached while running as Community cannot leak into Cloud without a expect($event->disk)->toBe('files'); }); +/** + * An external storage row exactly as 2.0/2.1 wrote it: S3 credentials in + * place, and none of the columns added since. + * + * Written with the query builder and then stripped back, rather than + * hand-rolled SQL, so it stays honest if the table changes shape again. + */ +function legacyStorageRow(): void +{ + DB::table('external_storage_settings')->insert([ + 'active' => true, + 'key' => 'AKIAEXAMPLE', + 'secret' => Crypt::encryptString('shh'), + 'bucket' => 'my-bucket', + 'region' => 'us-east-1', + 'use_path_style' => false, + ]); + + Schema::table('external_storage_settings', function (Blueprint $table) { + $table->dropColumn(['provider', 'key_file', 'use_instance_role']); + }); +} + /** * @param array $overrides * @return array