From fa51085578cd5234117ccd0b6d276d37f1c2218a Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Wed, 13 May 2026 18:29:28 -0400 Subject: [PATCH] Address code review findings for backup verification Fix 4 critical issues: - Add filter/priority/deleteMarkerReplication to S3 CRR rule (deploy would fail without) - Add stack dependency so replica deploys before main stack - Fix DB file extension matching (.sqlite3/.sql instead of .db) - Replace nonexistent `forgejo restore` command with actual restore steps in README Fix 4 moderate issues: - Add timeout=10 to Slack webhook urlopen call - Add filter='data' to tarfile.extract for PEP 706 compliance - Add explicit ValueError for unknown handler mode - Use date-scoped S3/GCS prefix instead of unbounded listing --- README.md | 17 ++++++++----- bin/app.ts | 6 +++-- lambda/backup-verification/app.py | 41 +++++++++++++++++++++---------- lib/forgejo-stack.ts | 3 +++ 4 files changed, 46 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index ba8fe71..93226b2 100644 --- a/README.md +++ b/README.md @@ -68,18 +68,23 @@ sudo /usr/local/bin/forgejo-backup.sh ```bash aws s3 cp s3://forgejo-backups-328440206208/archive//forgejo-.tar.gz /tmp/ systemctl stop forgejo -cd /tmp && tar xzf forgejo-.tar.gz -forgejo restore --config /etc/forgejo/app.ini --from /tmp/forgejo-dump-* -chown -R forgejo:forgejo /var/lib/forgejo +mkdir -p /tmp/forgejo-restore && tar -xzf /tmp/forgejo-.tar.gz -C /tmp/forgejo-restore +cd /tmp/forgejo-restore +cp app.ini /etc/forgejo/app.ini +cp gitea-db.sqlite3 /var/lib/forgejo/data/forgejo.db +rm -rf /var/lib/forgejo/data/repositories +cp -a repos /var/lib/forgejo/data/repositories +chown -R forgejo:forgejo /var/lib/forgejo /etc/forgejo/app.ini systemctl start forgejo +rm -rf /tmp/forgejo-restore /tmp/forgejo-.tar.gz ``` ### Restore from GCS (disaster recovery) ```bash -gcloud config set project seahaven-backups +gcloud config set project sea-haven-backups gsutil cp gs://forgejo-backups-offsite-seahaven/archive//forgejo-.tar.gz /tmp/ -# Then follow the same restore steps as S3 +# Then follow the same restore steps as S3 above ``` To test the backup manually: @@ -167,7 +172,7 @@ Run the setup script to create the GCS offsite bucket, service account, and stor ./scripts/gcp-setup.sh ``` -This creates the `seahaven-backups` GCP project with a locked-retention GCS bucket. After running, configure the Storage Transfer job in the GCP Console using the AWS credentials from `forgejo/gcs-transfer-credentials`. +This creates the `sea-haven-backups` GCP project with a locked-retention GCS bucket. After running, configure the Storage Transfer job in the GCP Console using the AWS credentials from `forgejo/gcs-transfer-credentials`. ## Deployment diff --git a/bin/app.ts b/bin/app.ts index 1721cd7..518b551 100644 --- a/bin/app.ts +++ b/bin/app.ts @@ -6,12 +6,14 @@ import { ForgejoReplicaStack } from "../lib/forgejo-replica-stack"; const app = new cdk.App(); -new ForgejoReplicaStack(app, "forgejo-replica", { +const replicaStack = new ForgejoReplicaStack(app, "forgejo-replica", { stackName: "forgejo-replica", env: { account: "328440206208", region: "us-west-2" }, }); -new ForgejoStack(app, "forgejo", { +const forgejoStack = new ForgejoStack(app, "forgejo", { stackName: "forgejo", env: { account: "328440206208", region: "us-east-1" }, }); + +forgejoStack.addDependency(replicaStack); diff --git a/lambda/backup-verification/app.py b/lambda/backup-verification/app.py index bc7a0dc..46e4828 100644 --- a/lambda/backup-verification/app.py +++ b/lambda/backup-verification/app.py @@ -38,10 +38,14 @@ def _check_s3_bucket(client, bucket, label): now = datetime.now(timezone.utc) cutoff = now - timedelta(hours=48) try: - resp = client.list_objects_v2(Bucket=bucket, Prefix="archive/", MaxKeys=1000) - contents = resp.get("Contents", []) + today = now.strftime("%Y-%m-%d") + yesterday = (now - timedelta(days=1)).strftime("%Y-%m-%d") + contents = [] + for date_prefix in [today, yesterday]: + resp = client.list_objects_v2(Bucket=bucket, Prefix=f"archive/{date_prefix}/") + contents.extend(resp.get("Contents", [])) if not contents: - return False, f"{label}: No objects found under archive/" + return False, f"{label}: No objects found under archive/ for last 2 days" latest = max(contents, key=lambda o: o["LastModified"]) if latest["LastModified"] < cutoff: age = (now - latest["LastModified"]).total_seconds() / 3600 @@ -57,10 +61,14 @@ def _check_gcs(): try: client = _get_gcs_client() bucket = client.bucket(GCS_BUCKET) - blobs = list(bucket.list_blobs(prefix="archive/", max_results=1000)) - if not blobs: - return False, "GCS Offsite: No objects found under archive/" now = datetime.now(timezone.utc) + today = now.strftime("%Y-%m-%d") + yesterday = (now - timedelta(days=1)).strftime("%Y-%m-%d") + blobs = [] + for date_prefix in [today, yesterday]: + blobs.extend(list(bucket.list_blobs(prefix=f"archive/{date_prefix}/"))) + if not blobs: + return False, "GCS Offsite: No objects found under archive/ for last 2 days" cutoff = now - timedelta(hours=72) latest = max(blobs, key=lambda b: b.updated) if latest.updated < cutoff: @@ -95,10 +103,14 @@ def _check_ebs_snapshots(): def _restore_test(): results = [] try: - resp = s3.list_objects_v2(Bucket=SOURCE_BUCKET, Prefix="archive/", MaxKeys=1000) - contents = resp.get("Contents", []) + now = datetime.now(timezone.utc) + contents = [] + for days_ago in range(7): + date_prefix = (now - timedelta(days=days_ago)).strftime("%Y-%m-%d") + resp = s3.list_objects_v2(Bucket=SOURCE_BUCKET, Prefix=f"archive/{date_prefix}/") + contents.extend(resp.get("Contents", [])) if not contents: - return [{"pass": False, "msg": "Restore test: No dumps found in source bucket"}] + return [{"pass": False, "msg": "Restore test: No dumps found in source bucket (last 7 days)"}] latest = max(contents, key=lambda o: o["LastModified"]) results.append({"pass": True, "msg": f"Restore test: Using {latest['Key']} ({latest['Size'] / 1_000_000:.1f} MB)"}) @@ -112,10 +124,10 @@ def _restore_test(): names = tf.getnames() results.append({"pass": True, "msg": f"Restore test: Archive OK — {len(names)} entries"}) - db_entries = [n for n in names if n.endswith(".db") or n.endswith("forgejo.db")] + db_entries = [n for n in names if n.endswith(".sqlite3") or n.endswith(".sql")] if db_entries: import sqlite3 as sqlite_mod - tf.extract(db_entries[0], path=tmpdir) + tf.extract(db_entries[0], path=tmpdir, filter="data") db_path = os.path.join(tmpdir, db_entries[0]) conn = sqlite_mod.connect(db_path) result = conn.execute("PRAGMA integrity_check").fetchone() @@ -125,7 +137,7 @@ def _restore_test(): else: results.append({"pass": False, "msg": f"Restore test: SQLite integrity FAILED — {result[0]}"}) else: - results.append({"pass": True, "msg": "Restore test: No .db file found in archive (may use different format)"}) + results.append({"pass": False, "msg": "Restore test: No SQLite DB file found in archive"}) except tarfile.TarError as e: results.append({"pass": False, "msg": f"Restore test: Archive extraction FAILED — {e}"}) except Exception as e: @@ -143,7 +155,7 @@ def _post_slack(blocks): headers={"Content-Type": "application/json"}, method="POST", ) - urllib.request.urlopen(req) + urllib.request.urlopen(req, timeout=10) def handler(event, context): @@ -186,6 +198,9 @@ def handler(event, context): overall = ":white_check_mark: Restore test passed" if all_pass else ":rotating_light: Restore test failed" blocks.append({"type": "section", "text": {"type": "mrkdwn", "text": f"*Overall:* {overall}"}}) + else: + raise ValueError(f"Unknown mode: {mode!r} (expected 'daily' or 'restore-test')") + _post_slack(blocks) return { diff --git a/lib/forgejo-stack.ts b/lib/forgejo-stack.ts index ae29111..be9b46f 100644 --- a/lib/forgejo-stack.ts +++ b/lib/forgejo-stack.ts @@ -122,6 +122,9 @@ export class ForgejoStack extends cdk.Stack { rules: [{ id: "replicate-to-west", status: "Enabled", + priority: 1, + filter: { prefix: "" }, + deleteMarkerReplication: { status: "Disabled" }, destination: { bucket: replicaBucketArn, storageClass: "STANDARD",