From 4d9c30fbb3bd27b21368221f3f3666da2f9e4fd5 Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Fri, 15 May 2026 17:25:25 -0400 Subject: [PATCH] Address cross-review findings for backup verification - Add size guard before downloading dump in restore test (3.5 GB cap) - Use paginator for list_objects_v2 in S3 checks and restore test - Remove unnecessary overrideLogicalId on GcsTransferCredentials secret - Pass explicit { mode: "daily" } to daily EventBridge rule target - Add fallback for SSM parameter fetch in backup script - Export replica bucket ARN/name from replica stack, consume via props --- bin/app.ts | 2 ++ lambda/backup-verification/app.py | 13 +++++++++---- lib/constructs/backup-verification.ts | 6 +++++- lib/forgejo-replica-stack.ts | 8 +++++++- lib/forgejo-stack.ts | 16 +++++++++------- 5 files changed, 32 insertions(+), 13 deletions(-) diff --git a/bin/app.ts b/bin/app.ts index 518b551..b79e622 100644 --- a/bin/app.ts +++ b/bin/app.ts @@ -14,6 +14,8 @@ const replicaStack = new ForgejoReplicaStack(app, "forgejo-replica", { const forgejoStack = new ForgejoStack(app, "forgejo", { stackName: "forgejo", env: { account: "328440206208", region: "us-east-1" }, + replicaBucketArn: replicaStack.replicaBucketArn, + replicaBucketName: replicaStack.replicaBucketName, }); forgejoStack.addDependency(replicaStack); diff --git a/lambda/backup-verification/app.py b/lambda/backup-verification/app.py index a471c0a..9da8adc 100644 --- a/lambda/backup-verification/app.py +++ b/lambda/backup-verification/app.py @@ -41,9 +41,10 @@ def _check_s3_bucket(client, bucket, label): today = now.strftime("%Y-%m-%d") yesterday = (now - timedelta(days=1)).strftime("%Y-%m-%d") contents = [] + paginator = client.get_paginator("list_objects_v2") for date_prefix in [today, yesterday]: - resp = client.list_objects_v2(Bucket=bucket, Prefix=f"archive/{date_prefix}/") - contents.extend(resp.get("Contents", [])) + for page in paginator.paginate(Bucket=bucket, Prefix=f"archive/{date_prefix}/"): + contents.extend(page.get("Contents", [])) if not contents: return False, f"{label}: No objects found under archive/ for last 2 days" latest = max(contents, key=lambda o: o["LastModified"]) @@ -113,13 +114,17 @@ def _restore_test(): try: now = datetime.now(timezone.utc) contents = [] + paginator = s3.get_paginator("list_objects_v2") 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", [])) + for page in paginator.paginate(Bucket=SOURCE_BUCKET, Prefix=f"archive/{date_prefix}/"): + contents.extend(page.get("Contents", [])) if not contents: return [{"pass": False, "msg": "Restore test: No dumps found in source bucket (last 7 days)"}] latest = max(contents, key=lambda o: o["LastModified"]) + max_bytes = 4 * 1024 * 1024 * 1024 - 512 * 1024 * 1024 + if latest["Size"] > max_bytes: + return [{"pass": False, "msg": f"Restore test: Dump too large for ephemeral storage ({latest['Size'] / 1_000_000_000:.1f} GB)"}] results.append({"pass": True, "msg": f"Restore test: Using {latest['Key']} ({latest['Size'] / 1_000_000:.1f} MB)"}) with tempfile.TemporaryDirectory() as tmpdir: diff --git a/lib/constructs/backup-verification.ts b/lib/constructs/backup-verification.ts index 0da7d9e..64106da 100644 --- a/lib/constructs/backup-verification.ts +++ b/lib/constructs/backup-verification.ts @@ -75,7 +75,11 @@ export class BackupVerification extends Construct { new events.Rule(this, "DailyCheck", { ruleName: "forgejo-backup-daily-check", schedule: events.Schedule.cron({ hour: "8", minute: "0" }), - targets: [new events_targets.LambdaFunction(fn)], + targets: [ + new events_targets.LambdaFunction(fn, { + event: events.RuleTargetInput.fromObject({ mode: "daily" }), + }), + ], }); new events.Rule(this, "MonthlyRestoreTest", { diff --git a/lib/forgejo-replica-stack.ts b/lib/forgejo-replica-stack.ts index ca8fa5a..fe848f6 100644 --- a/lib/forgejo-replica-stack.ts +++ b/lib/forgejo-replica-stack.ts @@ -3,10 +3,13 @@ import * as s3 from "aws-cdk-lib/aws-s3"; import { Construct } from "constructs"; export class ForgejoReplicaStack extends cdk.Stack { + public readonly replicaBucketArn: string; + public readonly replicaBucketName: string; + constructor(scope: Construct, id: string, props?: cdk.StackProps) { super(scope, id, props); - new s3.Bucket(this, "ReplicaBucket", { + const replicaBucket = new s3.Bucket(this, "ReplicaBucket", { bucketName: "forgejo-backups-replica-328440206208", encryption: s3.BucketEncryption.S3_MANAGED, blockPublicAccess: s3.BlockPublicAccess.BLOCK_ALL, @@ -33,5 +36,8 @@ export class ForgejoReplicaStack extends cdk.Stack { ], removalPolicy: cdk.RemovalPolicy.RETAIN, }); + + this.replicaBucketArn = replicaBucket.bucketArn; + this.replicaBucketName = replicaBucket.bucketName; } } diff --git a/lib/forgejo-stack.ts b/lib/forgejo-stack.ts index 254033d..c4c20d3 100644 --- a/lib/forgejo-stack.ts +++ b/lib/forgejo-stack.ts @@ -14,8 +14,13 @@ import { BackupVerification } from "./constructs/backup-verification"; const FORGEJO_VERSION = "10.0.1"; +interface ForgejoStackProps extends cdk.StackProps { + replicaBucketArn: string; + replicaBucketName: string; +} + export class ForgejoStack extends cdk.Stack { - constructor(scope: Construct, id: string, props?: cdk.StackProps) { + constructor(scope: Construct, id: string, props: ForgejoStackProps) { super(scope, id, props); const vpc = ec2.Vpc.fromLookup(this, "SeaHavenVpc", { @@ -93,7 +98,7 @@ export class ForgejoStack extends cdk.Stack { backupS3Prefix.grantRead(role); - const replicaBucketArn = "arn:aws:s3:::forgejo-backups-replica-328440206208"; + const replicaBucketArn = props.replicaBucketArn; const replicationRole = new iam.Role(this, "ReplicationRole", { roleName: "forgejo-s3-replication", @@ -162,9 +167,6 @@ export class ForgejoStack extends cdk.Stack { secretAccessKey: gcsTransferKey.secretAccessKey, }, }); - const gcsTransferCredentialsResource = gcsTransferCredentials.node.defaultChild as secretsmanager.CfnSecret; - gcsTransferCredentialsResource.overrideLogicalId("GcsTransferCredentials"); - const userData = ec2.UserData.forLinux(); userData.addCommands( "set -euxo pipefail", @@ -248,7 +250,7 @@ export class ForgejoStack extends cdk.Stack { "#!/bin/bash", "set -euo pipefail", "TIMESTAMP=$(date +%Y-%m-%d)", - "S3_PREFIX=$(aws ssm get-parameter --name /forgejo/backup-s3-prefix --query Parameter.Value --output text --region us-east-1)", + "S3_PREFIX=$(aws ssm get-parameter --name /forgejo/backup-s3-prefix --query Parameter.Value --output text --region us-east-1 || echo 'archive')", "DUMP_DIR=$(mktemp -d)", "chown forgejo:forgejo \"$DUMP_DIR\"", "cd \"$DUMP_DIR\"", @@ -451,7 +453,7 @@ export class ForgejoStack extends cdk.Stack { new BackupVerification(this, "BackupVerification", { sourceBucket: backupBucket, - replicaBucketName: "forgejo-backups-replica-328440206208", + replicaBucketName: props.replicaBucketName, gcsBucket: "forgejo-backups-offsite-seahaven", gcsSaSecretName: "forgejo/gcs-sa-key", slackWebhookSecretName: "forgejo/slack-webhook",