feat: serve uploads through storage, and move all 3,852 files to MinIO
Serving went straight to disk with FileResponse, so object storage was
effectively write-only: bytes went to the bucket and were still read from the
volume. `/uploads/{path}` now tries the local file first, then the object,
keeping the existing authorisation and path-confinement checks in front of both.
That is what makes the volume removable at all.
Migration (scripts/migrate_uploads_to_s3.py)
Every file is copied and read back with a SHA-256 comparison before anything is
deleted, and deletion is a separate opt-in flag that refuses to run if a single
file failed to verify. 3,852 files, 853.7 MB, all verified, then removed from the
volume — which now holds 0 files.
A bug this caught in its own first run: verification used `storage_service.load`,
which falls back to the volume, so it compared each local file against itself and
reported 3,852 perfect matches against an empty bucket. `s3_object` reads
strictly from S3 with no fallback, and verification uses that. The fallback is
right for serving and wrong for verifying, and the two now have separate calls.
Proven before deleting: a file removed from the volume still served correctly and
byte-identically from the bucket.
Backups, corrected: borgmatic already covers /var/lib/docker/volumes, so
quiz_minio_data is backed up nightly with 7/4/6 retention — my earlier claim that
MinIO was outside the backup routine was wrong, based on db-backup alone.
Existing archives still hold the old uploads volume, so there is no window in
which these files exist in only one place.
Tests: 131 backend green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017acfNLsJpnkvH3sCZSjMJM
This commit is contained in:
parent
db2df87fc6
commit
d071e5cdc5
3 changed files with 152 additions and 4 deletions
|
|
@ -1,9 +1,12 @@
|
|||
"""Native browser media authentication only; API authentication stays bearer-only."""
|
||||
import mimetypes
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException, Request
|
||||
from fastapi.responses import FileResponse
|
||||
from fastapi.responses import FileResponse, Response
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from app.database import get_db
|
||||
from app.services import storage_service
|
||||
from app.utils.auth import get_current_user
|
||||
from app.utils.upload_access import (
|
||||
LEGACY_LMS_PREFIXES, local_upload_path, upload_file, references,
|
||||
|
|
@ -41,8 +44,19 @@ def _read_upload(path, request, attempt_id, db, headers):
|
|||
if not can_read_upload(db, path, user, questions, cards, attempt_id):
|
||||
raise HTTPException(404, "File not found")
|
||||
# Known legacy LMS directories retain their policy; this is not a whole-LMS audit.
|
||||
if not target.is_file():
|
||||
raise HTTPException(404, "File not found")
|
||||
if target.suffix.lower() == ".svg":
|
||||
headers["Content-Security-Policy"] = "sandbox; default-src 'none'; style-src 'unsafe-inline'"
|
||||
return FileResponse(target, headers=headers)
|
||||
|
||||
# Serving must go through the storage service, or object storage would be
|
||||
# write-only: the bytes would be in the bucket and still read from disk.
|
||||
# The path is already authorised and confined to the upload root above.
|
||||
if target.is_file():
|
||||
return FileResponse(target, headers=headers)
|
||||
data = storage_service.s3_object(path)
|
||||
if data is None:
|
||||
raise HTTPException(404, "File not found")
|
||||
media_type = mimetypes.guess_type(path)[0] or "application/octet-stream"
|
||||
if request.method == "HEAD":
|
||||
return Response(status_code=200, headers={**headers, "Content-Length": str(len(data))},
|
||||
media_type=media_type)
|
||||
return Response(content=data, media_type=media_type, headers=headers)
|
||||
|
|
|
|||
|
|
@ -138,6 +138,20 @@ def presigned_url(key: str, expires: int = 3600) -> str | None:
|
|||
return None
|
||||
|
||||
|
||||
def s3_object(key: str) -> bytes | None:
|
||||
"""Read strictly from S3, with no fallback.
|
||||
|
||||
`load` falls back to the volume, which is right for serving but useless for
|
||||
verifying a migration: it would compare a local file against itself.
|
||||
"""
|
||||
if not using_s3():
|
||||
return None
|
||||
try:
|
||||
return _s3().get_object(Bucket=settings.S3_BUCKET, Key=key)["Body"].read()
|
||||
except Exception:
|
||||
return None
|
||||
|
||||
|
||||
def size_of(key: str) -> int | None:
|
||||
if using_s3():
|
||||
try:
|
||||
|
|
|
|||
120
backend/scripts/migrate_uploads_to_s3.py
Normal file
120
backend/scripts/migrate_uploads_to_s3.py
Normal file
|
|
@ -0,0 +1,120 @@
|
|||
"""Copy the existing uploads volume into object storage.
|
||||
|
||||
Verification order matters: every file is copied and read back *before* anything
|
||||
is deleted, and deletion only happens when explicitly asked for. A migration that
|
||||
removes the source before proving the destination is readable has no way back.
|
||||
|
||||
docker compose exec backend python -m scripts.migrate_uploads_to_s3
|
||||
docker compose exec backend python -m scripts.migrate_uploads_to_s3 --apply
|
||||
docker compose exec backend python -m scripts.migrate_uploads_to_s3 --verify
|
||||
docker compose exec backend python -m scripts.migrate_uploads_to_s3 --apply --delete-local
|
||||
"""
|
||||
import hashlib
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
from app.config import settings
|
||||
from app.services import storage_service
|
||||
|
||||
|
||||
def walk(root: Path):
|
||||
"""Every file under the uploads directory, keyed by its relative path."""
|
||||
for path in sorted(root.rglob("*")):
|
||||
if path.is_file() and not path.name.startswith("."):
|
||||
yield path.relative_to(root).as_posix(), path
|
||||
|
||||
|
||||
def human(size: int) -> str:
|
||||
for unit in ("B", "KB", "MB", "GB"):
|
||||
if size < 1024 or unit == "GB":
|
||||
return f"{size:.0f} {unit}" if unit == "B" else f"{size:.1f} {unit}"
|
||||
size /= 1024
|
||||
return f"{size:.1f} GB"
|
||||
|
||||
|
||||
def main():
|
||||
apply_changes = "--apply" in sys.argv
|
||||
verify_only = "--verify" in sys.argv
|
||||
delete_local = "--delete-local" in sys.argv
|
||||
|
||||
if not storage_service.using_s3():
|
||||
print("STORAGE_BACKEND is not 's3'; nothing to migrate into.")
|
||||
return 1
|
||||
|
||||
root = Path(settings.UPLOAD_DIR)
|
||||
if not root.is_dir():
|
||||
print(f"No uploads directory at {root}")
|
||||
return 1
|
||||
|
||||
storage_service.ensure_bucket()
|
||||
files = list(walk(root))
|
||||
total_bytes = sum(path.stat().st_size for _key, path in files)
|
||||
print(f" files on the volume : {len(files)} ({human(total_bytes)})")
|
||||
|
||||
copied, already, failed, verified, mismatched = 0, 0, [], 0, []
|
||||
for key, path in files:
|
||||
data = path.read_bytes()
|
||||
digest = hashlib.sha256(data).hexdigest()
|
||||
|
||||
# Strictly S3: the fallback in `load` would compare a file to itself.
|
||||
if verify_only or storage_service.s3_object(key) is not None:
|
||||
stored = storage_service.s3_object(key)
|
||||
if stored is None:
|
||||
failed.append(key)
|
||||
elif hashlib.sha256(stored).hexdigest() != digest:
|
||||
mismatched.append(key)
|
||||
else:
|
||||
verified += 1
|
||||
already += 1
|
||||
continue
|
||||
|
||||
if not apply_changes:
|
||||
copied += 1
|
||||
continue
|
||||
|
||||
try:
|
||||
storage_service.save(key, data)
|
||||
# Read back before trusting it; a write that cannot be read is not a copy.
|
||||
stored = storage_service.s3_object(key)
|
||||
if stored is None or hashlib.sha256(stored).hexdigest() != digest:
|
||||
mismatched.append(key)
|
||||
else:
|
||||
copied += 1
|
||||
verified += 1
|
||||
except Exception as error:
|
||||
failed.append(f"{key}: {error}")
|
||||
|
||||
print(f" copied : {copied}")
|
||||
print(f" already in storage : {already}")
|
||||
print(f" read back and match : {verified}")
|
||||
if mismatched:
|
||||
print(f" MISMATCHED : {len(mismatched)}")
|
||||
for key in mismatched[:10]:
|
||||
print(f" {key}")
|
||||
if failed:
|
||||
print(f" FAILED : {len(failed)}")
|
||||
for key in failed[:10]:
|
||||
print(f" {key}")
|
||||
|
||||
if delete_local:
|
||||
if mismatched or failed:
|
||||
print("\n Refusing to delete: not every file verified.")
|
||||
return 1
|
||||
if not apply_changes:
|
||||
print("\n --delete-local needs --apply.")
|
||||
return 1
|
||||
removed = 0
|
||||
for key, path in files:
|
||||
# Belt and braces: prove the object reads before removing the source.
|
||||
if storage_service.s3_object(key) is not None:
|
||||
path.unlink()
|
||||
removed += 1
|
||||
print(f" local files removed : {removed}")
|
||||
|
||||
if not apply_changes and not verify_only:
|
||||
print("\n Re-run with --apply to copy, then --verify to check.")
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
Loading…
Reference in a new issue