diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d8fc592c..7161fac6f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -435,7 +435,8 @@ jobs: # in the three-leg layout): api 87s, config 85s, auth 62s, server # 55s, purchase 53s, migrations 64s, and the rest 95s together # (testhelpers 32s, cleanup-lambda 15s, database 15s, scheduler 12s, - # analytics 12s, commitmentopts 9s). api is the floor, as one package + # analytics 12s, commitmentopts 9s; cleanup-lambda was deleted in + # #357, so the rest is about 80s now). api is the floor, as one package # is never split; migrations and the rest run side by side within it. include: - shard: api diff --git a/.gitignore b/.gitignore index 57a35f11e..f03e02e45 100644 --- a/.gitignore +++ b/.gitignore @@ -96,7 +96,6 @@ sanity_report*.json # Compiled binaries cudly-final cudly-test -cleanup-lambda /server /lambda diff --git a/.trivyignore b/.trivyignore index 7cf6809d5..7980c9406 100644 --- a/.trivyignore +++ b/.trivyignore @@ -59,21 +59,6 @@ AVD-AWS-0136 # in the private subnets and routes egress through the NAT. AVD-AWS-0164 -# Azure Function app HTTPS enforcement. -# Justification: the cleanup Function in -# terraform/modules/compute/azure/cleanup-function/ is invoked by -# Azure-internal triggers (timers + queue), never directly by external -# HTTP. https_only=true would still be correct hygiene; tracked as a -# follow-up to set explicitly. -AVD-AZU-0004 - -# Azure storage account network rules default allow. -# Justification: the cleanup function's storage account is in the -# same vnet as the function and is firewall-restricted at the Azure -# resource-group level. The trivy ID still trips because the network -# rules block isn't declared in the resource itself. -AVD-AZU-0012 - # Azure Key Vault network ACL default action. # ID: AZU-0013 (surfaced by trivy v0.70.0; v0.69.3 did not report this ID). # Justification: the vault is in terraform/modules/secrets/azure/main.tf and @@ -100,18 +85,6 @@ AZU-0047 # entry looks stale. Drop it once a Trivy config scan without it is clean. GCP-0015 -# GCP Cloud Storage bucket: cleanup Cloud Function source bucket. -# ID: GCP-0001 (surfaced by trivy v0.70.0; v0.69.3 did not report this ID). -# Justification: the only GCS bucket in the GCP deployment is the cleanup -# Cloud Function source bucket (terraform/modules/compute/gcp/cleanup-function/main.tf). -# This bucket is NOT public: it has uniform_bucket_level_access = true and -# public_access_prevention = "enforced". Trivy may still flag the resource if -# it does not recognise the public_access_prevention attribute. -# The GCP frontend is served from Cloud Run (not a GCS bucket), so no frontend -# build artifacts -- including source maps (*.map, produced by hidden-source-map -# in webpack.config.js) -- are uploaded to any GCS bucket. -GCP-0001 - # Go dependency vulnerability (trivy fs) suppressions live in # .trivyignore.yaml, where each entry is scoped to one module by purl. # Keep this file for config-scan IDs only. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2481f2284..11ebb7d4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,14 @@ are the full record until the next release. Lambda or Fargate) instead; see `docs/DEPLOYMENT.md`. The target-account role templates (`cloudformation/stacks/CUDly-CrossAccount` and the federation CloudFormation templates under `iac/federation/`) are unchanged (#1432) +- The standalone cleanup job `cmd/cleanup-lambda` and its three deploy modules + (`terraform/modules/compute/aws/cleanup-lambda`, + `terraform/modules/compute/gcp/cleanup-function` and + `terraform/modules/compute/azure/cleanup-function`). No environment root used + the modules. The job only deleted executions whose `expires_at` had passed, + and no code path sets that column, so it never deleted an execution. The server's + `cleanup` scheduled task (`CleanupOldExecutions`) already covers execution and + session retention; see "Database Cleanup Jobs" in `docs/DEPLOYMENT.md` (#357) ### Fixed diff --git a/cmd/cleanup-lambda/main.go b/cmd/cleanup-lambda/main.go deleted file mode 100644 index e9bc1f29b..000000000 --- a/cmd/cleanup-lambda/main.go +++ /dev/null @@ -1,141 +0,0 @@ -package main - -import ( - "context" - "fmt" - "log" - "time" - - "github.com/LeanerCloud/cloud-commitments-platform/internal/config" - "github.com/LeanerCloud/cloud-commitments-platform/internal/database" - "github.com/aws/aws-lambda-go/lambda" -) - -// CleanupEvent represents the input to the cleanup function. -type CleanupEvent struct { - DryRun bool `json:"dryRun,omitempty"` -} - -// CleanupResult represents the cleanup operation results. -type CleanupResult struct { - SessionsDeleted int64 `json:"sessionsDeleted"` - ExecutionsDeleted int64 `json:"executionsDeleted"` - DryRun bool `json:"dryRun"` - Timestamp int64 `json:"timestamp"` -} - -// expiredExecutionsWhere selects the purchase_executions rows this job may -// delete: the row's own TTL (expires_at) has lapsed AND the row is settled. -// -// The TTL alone is not enough (issue #998). `expires_at` is written once at -// insert by SavePurchaseExecution and never rewritten when the row changes -// status, so a lapsed TTL says nothing about whether an executor, scheduler -// or reaper still needs the row. The status half therefore mirrors the rules -// the rest of the store already enforces: -// -// - status = ANY(TerminalExecutionStatuses): only rows nothing will act on -// again. It is an allowlist on purpose, so a status added later is kept -// until someone classifies it, the same fail-closed choice -// DeleteCloudAccount makes. -// - status <> ALL(HealthScoredExecutionStatuses): failed and canceled rows -// are left to CleanupOldExecutions, whose own branches retain them for -// the plan health score's window (see that function's branch 3). -// -// Both lists come from the config package so this job cannot drift from the -// store. `status` is NOT NULL (migration 000001), so neither array test is -// exposed to three-valued logic. NULL expires_at never matches, so rows that -// never had a TTL (every row current code writes) are untouched. -// -// Under READ COMMITTED a row another transaction is updating is re-checked -// against this whole predicate after that transaction commits, so a row that -// stops being settled while the DELETE waits for it survives. -const expiredExecutionsWhere = `expires_at < $1 AND status = ANY($2) AND status <> ALL($3)` - -// expiredExecutionsArgs are the arguments for expiredExecutionsWhere. -func expiredExecutionsArgs(now time.Time) []any { - return []any{now, config.TerminalExecutionStatuses, config.HealthScoredExecutionStatuses} -} - -func cleanupExpiredRecords(ctx context.Context, event CleanupEvent) (*CleanupResult, error) { - log.Printf("Starting cleanup job (dryRun=%v)", event.DryRun) - - // A new DB connection is opened per invocation (no connection reuse across warm starts). - // This is intentional: the cleanup Lambda runs infrequently and the simpler, stateless - // design is preferred over the shared-connection pattern used in cmd/lambda/main.go. - db, err := database.OpenFromEnv(ctx) - if err != nil { - return nil, err - } - defer db.Close() - - now := time.Now() - result := &CleanupResult{ - DryRun: event.DryRun, - Timestamp: now.Unix(), - } - - if event.DryRun { - if err := dryRunCount(ctx, db, now, result); err != nil { - return nil, err - } - } else { - if err := deleteExpired(ctx, db, now, result); err != nil { - return nil, err - } - } - - log.Printf("Cleanup job completed: %+v", result) - return result, nil -} - -// dryRunCount counts records that would be deleted without actually deleting -// them. It uses the same execution predicate as deleteExpired. -func dryRunCount(ctx context.Context, db *database.Connection, now time.Time, result *CleanupResult) error { - if err := db.QueryRow(ctx, "SELECT COUNT(*) FROM sessions WHERE expires_at < $1", now).Scan(&result.SessionsDeleted); err != nil { - return fmt.Errorf("failed to count expired sessions: %w", err) - } - if err := db.QueryRow(ctx, "SELECT COUNT(*) FROM purchase_executions WHERE "+expiredExecutionsWhere, expiredExecutionsArgs(now)...).Scan(&result.ExecutionsDeleted); err != nil { - return fmt.Errorf("failed to count expired executions: %w", err) - } - log.Printf("DRY RUN: Would delete %d sessions and %d executions", result.SessionsDeleted, result.ExecutionsDeleted) - return nil -} - -// deleteExpired deletes expired sessions and settled expired executions -// (expiredExecutionsWhere) in a single transaction. -func deleteExpired(ctx context.Context, db *database.Connection, now time.Time, result *CleanupResult) (err error) { - tx, err := db.Begin(ctx) - if err != nil { - return fmt.Errorf("failed to begin transaction: %w", err) - } - defer func() { - if err != nil { - if rErr := tx.Rollback(ctx); rErr != nil { - log.Printf("rollback failed: %v", rErr) - } - } - }() - - tag, err := tx.Exec(ctx, "DELETE FROM sessions WHERE expires_at < $1", now) - if err != nil { - return fmt.Errorf("failed to cleanup sessions: %w", err) - } - result.SessionsDeleted = tag.RowsAffected() - log.Printf("Deleted %d expired sessions", result.SessionsDeleted) - - tag, err = tx.Exec(ctx, "DELETE FROM purchase_executions WHERE "+expiredExecutionsWhere, expiredExecutionsArgs(now)...) - if err != nil { - return fmt.Errorf("failed to cleanup executions: %w", err) - } - result.ExecutionsDeleted = tag.RowsAffected() - log.Printf("Deleted %d expired executions", result.ExecutionsDeleted) - - if err = tx.Commit(ctx); err != nil { - return fmt.Errorf("failed to commit cleanup transaction: %w", err) - } - return nil -} - -func main() { - lambda.Start(cleanupExpiredRecords) -} diff --git a/cmd/cleanup-lambda/main_integration_test.go b/cmd/cleanup-lambda/main_integration_test.go deleted file mode 100644 index 4bb7d3fb4..000000000 --- a/cmd/cleanup-lambda/main_integration_test.go +++ /dev/null @@ -1,177 +0,0 @@ -//go:build integration - -package main - -// Real PostgreSQL tests for the cleanup job's purchase_executions predicate -// (issue #998). deleteExpired, dryRunCount and the migrated schema are real; -// nothing is deployed and no Lambda is invoked. - -import ( - "context" - "log" - "os" - "path/filepath" - "regexp" - "runtime" - "sort" - "testing" - "time" - - "github.com/LeanerCloud/cloud-commitments-platform/internal/config" - "github.com/LeanerCloud/cloud-commitments-platform/internal/database" - "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" - "github.com/stretchr/testify/require" -) - -// Every status the purchase_executions CHECK allows is in exactly one of -// these lists, so a new status fails TestCleanupStatusListsCoverSchema until -// someone decides whether a lapsed TTL may delete it. -var ( - // Settled and not counted by the plan health score: a lapsed TTL deletes. - cleanupDeletable = []string{"completed", "partially_completed", "expired", "revocation_requested"} - // Live (an executor, scheduler or reaper may still act on it), or counted - // by the health score and left to CleanupOldExecutions: always kept. - cleanupKept = []string{"pending", "notified", "approved", "running", "scheduled", "paused", "failed", "canceled", config.LegacyStatusCanceled} -) - -// TestMain stops the PostgreSQL container that every test in this package shares -// (issue #982). Each test still gets a fresh database of its own from -// testhelpers.RequireMigratedDatabase; only the container and the migrated -// template it is cloned from outlive a single test. It lives here rather than in -// a file of its own because .gitignore's "cleanup-lambda" (the built binary) -// also matches this directory, so a new file would silently stay untracked. -func TestMain(m *testing.M) { - code := m.Run() - if err := testhelpers.TerminateSharedPostgres(); err != nil { - // Ryuk reaps the container anyway, so a teardown hiccup does not - // overrule the tests' own verdict. - log.Printf("terminate shared postgres container: %v", err) - } - os.Exit(code) -} - -func cleanupTestDB(ctx context.Context, t *testing.T) *database.Connection { - t.Helper() - _, file, _, _ := runtime.Caller(0) - path := filepath.Join(filepath.Dir(file), "..", "..", "internal", "database", "postgres", "migrations") - return testhelpers.RequireMigratedDatabase(ctx, t, path).DB -} - -// insertExecution adds a plan-less execution whose expires_at is offset from -// the database clock (nil offset leaves it NULL) and returns its execution_id. -func insertExecution(ctx context.Context, t *testing.T, db *database.Connection, status string, offset *time.Duration) string { - t.Helper() - var secs *float64 - if offset != nil { - s := offset.Seconds() - secs = &s - } - var id string - // NOW() + NULL is NULL, so a nil offset leaves expires_at unset. - require.NoError(t, db.QueryRow(ctx, `INSERT INTO purchase_executions (status, scheduled_date, expires_at) - VALUES ($1, NOW(), NOW() + make_interval(secs => $2::float8)) RETURNING execution_id::text`, status, secs).Scan(&id), "insert %s row", status) - return id -} - -func remainingExecutions(ctx context.Context, t *testing.T, db *database.Connection) map[string]string { - t.Helper() - rows, err := db.Query(ctx, `SELECT execution_id::text, status FROM purchase_executions`) - require.NoError(t, err) - defer rows.Close() - out := map[string]string{} - for rows.Next() { - var id, status string - require.NoError(t, rows.Scan(&id, &status)) - out[id] = status - } - require.NoError(t, rows.Err()) - return out -} - -func TestCleanupStatusListsCoverSchema(t *testing.T) { - ctx := context.Background() - db := cleanupTestDB(ctx, t) - var def string - require.NoError(t, db.QueryRow(ctx, `SELECT pg_get_constraintdef(oid) FROM pg_constraint - WHERE conrelid = 'purchase_executions'::regclass AND contype = 'c' - AND pg_get_constraintdef(oid) LIKE '%status%'`).Scan(&def)) - matches := regexp.MustCompile(`'([a-z_]+)'`).FindAllStringSubmatch(def, -1) - allowed := make([]string, 0, len(matches)) - for _, m := range matches { - allowed = append(allowed, m[1]) - } - classified := append(append([]string{}, cleanupDeletable...), cleanupKept...) - sort.Strings(allowed) - sort.Strings(classified) - require.Equal(t, allowed, classified, "every allowed status must be classified exactly once") -} - -// A lapsed TTL deletes only settled rows: live rows and the rows the health -// score counts survive, and the dry run counts exactly what the delete removes. -func TestDeleteExpiredKeepsLiveAndHealthScoredRows(t *testing.T) { - ctx := context.Background() - db := cleanupTestDB(ctx, t) - lapsed, future := -time.Hour, time.Hour - - want := map[string]string{} - for _, s := range cleanupKept { - want[insertExecution(ctx, t, db, s, &lapsed)] = s - } - for _, s := range cleanupDeletable { - insertExecution(ctx, t, db, s, &lapsed) - want[insertExecution(ctx, t, db, s, &future)] = s - want[insertExecution(ctx, t, db, s, nil)] = s - } - - dry := &CleanupResult{} - require.NoError(t, dryRunCount(ctx, db, time.Now(), dry)) - require.EqualValues(t, len(cleanupDeletable), dry.ExecutionsDeleted, "dry run counts only settled lapsed rows") - - got := &CleanupResult{} - require.NoError(t, deleteExpired(ctx, db, time.Now(), got)) - require.EqualValues(t, len(cleanupDeletable), got.ExecutionsDeleted) - require.Equal(t, want, remainingExecutions(ctx, t, db), "live, health-scored, unexpired and TTL-less rows survive") -} - -// The DELETE rechecks the status on the committed row: a settled row a writer -// turns live while the DELETE waits for its lock survives. The writer is a -// stand-in (SavePurchaseExecution's upsert rewrites status unconditionally); -// no current flow moves a settled row back to live. -func TestDeleteExpiredRechecksRowAfterConcurrentWriter(t *testing.T) { - for _, writerFirst := range []bool{true, false} { - name := map[bool]string{true: "writer first", false: "cleanup first"}[writerFirst] - t.Run(name, func(t *testing.T) { - ctx := context.Background() - db := cleanupTestDB(ctx, t) - lapsed := -time.Hour - id := insertExecution(ctx, t, db, "completed", &lapsed) - revive := `UPDATE purchase_executions SET status = 'pending' WHERE execution_id = $1` - - if !writerFirst { - res := &CleanupResult{} - require.NoError(t, deleteExpired(ctx, db, time.Now(), res)) - require.EqualValues(t, 1, res.ExecutionsDeleted) - tag, err := db.Exec(ctx, revive, id) - require.NoError(t, err) - require.Zero(t, tag.RowsAffected(), "the settled row was already deleted") - return - } - - writer, err := db.Begin(ctx) - require.NoError(t, err) - t.Cleanup(func() { _ = writer.Rollback(context.Background()) }) - _, err = writer.Exec(ctx, revive, id) - require.NoError(t, err) - - res := &CleanupResult{} - done := make(chan error, 1) - go func() { done <- deleteExpired(ctx, db, time.Now(), res) }() - testhelpers.WaitForLockWait(ctx, t, db, "%DELETE FROM purchase_executions%") - require.NoError(t, writer.Commit(ctx)) - require.NoError(t, testhelpers.Await(t, done)) - - require.Zero(t, res.ExecutionsDeleted) - require.Equal(t, map[string]string{id: "pending"}, remainingExecutions(ctx, t, db), "the row turned live survives") - }) - } -} diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 340184056..7f4c81730 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -106,7 +106,6 @@ terraform/ │ ├── compute/ │ │ ├── aws/lambda/ │ │ ├── aws/fargate/ -│ │ ├── aws/cleanup-lambda/ │ │ ├── azure/container-apps/ │ │ └── gcp/cloud-run/ │ ├── database/ # aws/ (Aurora), azure/ (Flexible Server), gcp/ (Cloud SQL) @@ -605,8 +604,8 @@ The server's `cleanup` task deletes expired sessions and, via `CleanupOldExecuti - **Azure Container Apps (when `enable_scheduled_tasks` is true):** a Logic App calls `/api/scheduled/cleanup` daily (`terraform/modules/compute/azure/container-apps/scheduled-tasks.tf`) - **AWS Lambda (opt-in):** set `enable_cleanup_schedule = true` (default false; `cleanup_schedule` defaults to `rate(1 day)`) in `terraform/environments/aws`. The first run deletes the whole backlog older than 30 days, so export any history you want to keep first. -- **AWS Fargate (opt-in):** the same `enable_cleanup_schedule` / `cleanup_schedule` variables in `terraform/environments/aws` add a `-fargate-sweep-cleanup` EventBridge rule that runs `./cudly --task cleanup` as a one-off ECS task (`terraform/modules/compute/aws/fargate/sweeps.tf`). The same backlog warning applies. (The `terraform/modules/compute/aws/cleanup-lambda/` module is not consumed by any environment.) -- **GCP Cloud Run and GKE (opt-in):** set `enable_cleanup_schedule = true` (default false; `cleanup_schedule` defaults to `0 3 * * *`) in `terraform/environments/gcp`. It runs as a Cloud Scheduler job (`scheduled-tasks.tf` in each module) only when the module's `enable_scheduled_tasks` is true; that environment passes it to Cloud Run but not to GKE. The same backlog warning applies. The `terraform/modules/compute/gcp/cleanup-function/` module is not consumed by any environment. +- **AWS Fargate (opt-in):** the same `enable_cleanup_schedule` / `cleanup_schedule` variables in `terraform/environments/aws` add a `-fargate-sweep-cleanup` EventBridge rule that runs `./cudly --task cleanup` as a one-off ECS task (`terraform/modules/compute/aws/fargate/sweeps.tf`). The same backlog warning applies. +- **GCP Cloud Run and GKE (opt-in):** set `enable_cleanup_schedule = true` (default false; `cleanup_schedule` defaults to `0 3 * * *`) in `terraform/environments/gcp`. It runs as a Cloud Scheduler job (`scheduled-tasks.tf` in each module) only when the module's `enable_scheduled_tasks` is true; that environment passes it to Cloud Run but not to GKE. The same backlog warning applies. - **AKS (opt-in):** set `enable_cleanup_schedule = true` (default false; `cleanup_schedule` defaults to `0 3 * * *`) on the AKS module. Like the module's other scheduled tasks, it runs as a Kubernetes CronJob (`terraform/modules/compute/azure/aks/scheduled-tasks.tf`) only when `deploy_kubernetes_resources` is true. Alternatively, use pg_cron for database-native scheduling. diff --git a/internal/config/purchase_executions_delete_guard_test.go b/internal/config/purchase_executions_delete_guard_test.go new file mode 100644 index 000000000..3fc451865 --- /dev/null +++ b/internal/config/purchase_executions_delete_guard_test.go @@ -0,0 +1,128 @@ +package config + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "regexp" + "runtime" + "strconv" + "strings" + "testing" +) + +// deleteFromPurchaseExecutions matches a DELETE or TRUNCATE of the +// purchase_executions table, schema-qualified and/or double-quoted, and not +// of a table whose name merely starts with it. +var deleteFromPurchaseExecutions = regexp.MustCompile(`(?i)\b(?:DELETE\s+FROM|TRUNCATE(?:\s+TABLE)?)\s+(?:ONLY\s+)?(?:[\w".]+\s*,\s*)*(?:"?\w+"?\.)?"?purchase_executions"?(?:[^\w"]|$)`) + +// TestPurchaseExecutionsDeletedOnlyByConfigStore guards #357. A second +// retention job, cmd/cleanup-lambda, deleted purchase_executions rows with its +// own predicate outside this package. It drifted from CleanupOldExecutions +// twice (no status guard at all until #1173; a TTL column nothing writes, so +// it never deleted a row) and was removed. Retention now lives only in this +// package, where CleanupOldExecutions keeps the live-status and health-score +// guards next to the rest of the execution state machine. +// +// This scans every non-test Go file under internal/ and cmd/ outside +// internal/config for a string literal that deletes from or truncates +// purchase_executions. A new deleter belongs in the store, behind the same +// guards; add it there rather than widening this test. +// +// It is a best-effort tripwire, like TestNoUnscopedRecommendationsDelete: it +// sees one string literal at a time, so SQL assembled from separate pieces +// ("DELETE FROM " + table) gets past it. +func TestPurchaseExecutionsDeletedOnlyByConfigStore(t *testing.T) { + _, self, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("cannot locate this test file") + } + configDir := filepath.Dir(self) + root := filepath.Join(configDir, "..", "..") + fset := token.NewFileSet() + scanned := 0 + for _, dir := range []string{"internal", "cmd"} { + err := filepath.WalkDir(filepath.Join(root, dir), func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() && (d.Name() == "testdata" || path == configDir) { + return filepath.SkipDir + } + if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + f, perr := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if perr != nil { + return perr + } + scanned++ + for _, lit := range purchaseExecutionsDeletes(f) { + t.Errorf("%s: deletes from purchase_executions outside internal/config; put the delete in the store next to CleanupOldExecutions so it keeps the live-status guards", + fset.Position(lit.Pos())) + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", dir, err) + } + } + if scanned == 0 { + t.Fatal("scanned no Go files; the guard is not looking where it should") + } +} + +// TestPurchaseExecutionsDeletesMatcher pins what the scan above flags, so a +// broken matcher cannot turn the guard into a silent pass. +func TestPurchaseExecutionsDeletesMatcher(t *testing.T) { + src := "package p\n" + + "const cleanupLambda = \"DELETE FROM purchase_executions WHERE \"\n" + + "const lower = `delete from purchase_executions where expires_at < $1`\n" + + "const qualified = `DELETE FROM ONLY \"public\".\"purchase_executions\" e WHERE e.status = $1`\n" + + "const truncate = `TRUNCATE TABLE foo, purchase_executions CASCADE`\n" + + "const commentOnly = `SELECT 1 -- DELETE FROM purchase_executions`\n" + + "const read = `SELECT * FROM purchase_executions`\n" + + "const otherTable = `DELETE FROM purchase_executions_archive`\n" + + "const otherQuoted = `DELETE FROM \"purchase_executions_archive\"`\n" + f, err := parser.ParseFile(token.NewFileSet(), "p.go", src, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + flagged := purchaseExecutionsDeletes(f) + got := make([]string, 0, len(flagged)) + for _, lit := range flagged { + got = append(got, lit.Value) + } + want := []string{ + `"DELETE FROM purchase_executions WHERE "`, + "`delete from purchase_executions where expires_at < $1`", + "`DELETE FROM ONLY \"public\".\"purchase_executions\" e WHERE e.status = $1`", + "`TRUNCATE TABLE foo, purchase_executions CASCADE`", + } + if strings.Join(got, "|") != strings.Join(want, "|") { + t.Fatalf("flagged %q, want %q", got, want) + } +} + +// purchaseExecutionsDeletes returns every string literal in f that deletes +// from or truncates purchase_executions outside SQL comments. +func purchaseExecutionsDeletes(f *ast.File) []*ast.BasicLit { + var found []*ast.BasicLit + ast.Inspect(f, func(n ast.Node) bool { + lit, ok := n.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + sql, err := strconv.Unquote(lit.Value) + if err != nil { + sql = lit.Value + } + if deleteFromPurchaseExecutions.MatchString(sqlComment.ReplaceAllString(sql, " ")) { + found = append(found, lit) + } + return true + }) + return found +} diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index cd617adff..e52de3f22 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -2325,8 +2325,9 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha // dead, so without this branch it would accumulate indefinitely. // // The status gate is an ALLOWLIST: status = ANY(TerminalExecutionStatuses) -// AND status <> ALL(HealthScoredExecutionStatuses), the same predicate -// cmd/cleanup-lambda's expiredExecutionsWhere uses (#998, #1173). A lapsed +// AND status <> ALL(HealthScoredExecutionStatuses) (#998, #1173). This +// is the only expires_at retention path since cmd/cleanup-lambda, which +// ran its own copy of the predicate, was deleted (#357). A lapsed // TTL says nothing about whether an executor, scheduler or reaper still // needs the row: `expires_at` is set at insert and never rewritten, so an // `approved` or `running` row stranded by a dead executor (the rows @@ -2380,8 +2381,8 @@ func (s *PostgresStore) CleanupOldExecutions(ctx context.Context, retentionDays // // Branch 3's allowlist and exclusion are parameterized from // TerminalExecutionStatuses and HealthScoredExecutionStatuses instead of - // repeating literal lists, so this branch, cmd/cleanup-lambda and the - // plan health score cannot drift. `status` is NOT NULL (migration + // repeating literal lists, so this branch and the plan health score + // cannot drift. `status` is NOT NULL (migration // 000001), so neither array test is exposed to three-valued logic. // // Under READ COMMITTED a row another transaction is updating is diff --git a/internal/config/store_postgres_retention_integration_test.go b/internal/config/store_postgres_retention_integration_test.go index 49b698a5a..99922d956 100644 --- a/internal/config/store_postgres_retention_integration_test.go +++ b/internal/config/store_postgres_retention_integration_test.go @@ -49,7 +49,9 @@ func TestRampRetention_OldCleanupWritersPreserveHeldRows(t *testing.T) { deleted, err := f.store.CleanupOldExecutions(ctx, 30) require.NoError(t, err) require.EqualValues(t, 3, deleted, "only canceled spellings and expired are eligible and unheld") - // This is the other production writer, cmd/cleanup-lambda/main.go. + // A raw expires_at DELETE, the shape the deleted cmd/cleanup-lambda ran + // (#357) and an operator could still run by hand: the ramp ledger trigger, + // not the caller's predicate, must keep held rows. tag, err := f.store.db.Exec(ctx, `DELETE FROM purchase_executions WHERE expires_at < NOW()`) require.NoError(t, err) require.EqualValues(t, 1, tag.RowsAffected(), "failed/no-submission is unheld but excluded by ordinary health retention") diff --git a/known_issues/33_executions_retention_cleanup.md b/known_issues/33_executions_retention_cleanup.md index b79dcf4a1..213a36874 100644 --- a/known_issues/33_executions_retention_cleanup.md +++ b/known_issues/33_executions_retention_cleanup.md @@ -10,6 +10,10 @@ > "matches what the cleanup-lambda Terraform actually configures" sentence > under "Original problem". Only Azure Container Apps schedules cleanup today. Retention > scheduling is tracked by #712; retention is scheduled via the server `cleanup` task on AWS Lambda, AWS Fargate, GCP Cloud Run, GKE and AKS when opted in. +> +> **Amendment (#357):** the unconsumed `aws/cleanup-lambda`, `gcp/cleanup-function` +> and `azure/cleanup-function` modules and `cmd/cleanup-lambda` were deleted. The +> server `cleanup` task is the only cleanup job. ## Resolution @@ -18,7 +22,7 @@ `TaskCleanupExpiredRecords` scheduled task type and triggered by: - **AWS:** ~~EventBridge rule `cleanup_schedule` in - `terraform/modules/compute/aws/cleanup-lambda/main.tf:98`~~ (unconsumed module, see amendment) + `terraform/modules/compute/aws/cleanup-lambda/main.tf:98`~~ (unconsumed module, see amendment; deleted in #357) - **AWS Lambda (opt-in):** the `enable_cleanup_schedule` / `cleanup_schedule` sweep `-sweep-cleanup` runs the server cleanup task. - **AWS Fargate (opt-in):** the same `enable_cleanup_schedule` / @@ -29,8 +33,7 @@ the server cleanup task.~~ (the CloudFormation install stack was removed in #1432; use the Terraform AWS install) - **Azure Container Apps:** a Logic App calls `/api/scheduled/cleanup` daily - when `enable_scheduled_tasks` is true. (The `azure/cleanup-function` module - is not consumed by any environment.) + when `enable_scheduled_tasks` is true. - **GCP Cloud Run and GKE (opt-in):** `enable_cleanup_schedule` / `cleanup_schedule` add a `-cleanup` Cloud Scheduler job that calls `/api/scheduled/cleanup` (#712), when the module's `enable_scheduled_tasks` diff --git a/known_issues/35_legacy_guard_followups.md b/known_issues/35_legacy_guard_followups.md index 606912ee7..10fe316ca 100644 --- a/known_issues/35_legacy_guard_followups.md +++ b/known_issues/35_legacy_guard_followups.md @@ -14,6 +14,8 @@ **Status:** ✔️ Resolved +**Update (#357):** `cmd/cleanup-lambda` was deleted, so the files and tests named below no longer exist. No production path sets `PurchaseExecution.TTL`, so `expires_at` is NULL on every row current code writes and it never deleted one, and the server `cleanup` task (`CleanupOldExecutions`) carries the same status guard. `TestPurchaseExecutionsDeletedOnlyByConfigStore` keeps any new `purchase_executions` deleter inside `internal/config`. + **Resolved by:** `deleteExpired` and `dryRunCount` now share `expiredExecutionsWhere`: a lapsed `expires_at` deletes a row only when its status is in `TerminalExecutionStatuses` and not in `HealthScoredExecutionStatuses`, so live rows survive and failed or canceled rows stay with `CleanupOldExecutions`. Covered by the real-Postgres tests in `cmd/cleanup-lambda/main_integration_test.go`, including a writer that turns a settled row live while the DELETE waits for its lock. ## MEDIUM: a failed-with-submission row blocks deleting a managed plan forever diff --git a/scripts/check-azure-kv-access-policy.sh b/scripts/check-azure-kv-access-policy.sh index 3023571b5..62f274482 100755 --- a/scripts/check-azure-kv-access-policy.sh +++ b/scripts/check-azure-kv-access-policy.sh @@ -18,8 +18,9 @@ # access-policy write, so `terraform apply` reports success with no warning and # the grant is silently inert. It surfaces only as a runtime 403, which is the # worst possible place to discover it. That is exactly what happened to the -# Azure cleanup-function and the AKS workload identity (issue #1621) while every -# other consumer in the tree already used `azurerm_role_assignment`. +# Azure cleanup-function (since deleted, #357) and the AKS workload identity +# (issue #1621) while every other consumer in the tree already used +# `azurerm_role_assignment`. # # The supported pattern is: # diff --git a/scripts/check-gcp-secret-scope.sh b/scripts/check-gcp-secret-scope.sh index ac5313411..b5bdf69d6 100755 --- a/scripts/check-gcp-secret-scope.sh +++ b/scripts/check-gcp-secret-scope.sh @@ -18,8 +18,9 @@ # } # # This guard exists because compute/gcp/cloud-run was migrated to per-secret -# bindings while its sibling compute/gcp/cleanup-function was left on a -# project-wide grant for as long as it took someone to notice (issue #1614). +# bindings while its sibling compute/gcp/cleanup-function (since deleted, +# #357) was left on a project-wide grant for as long as it took someone to +# notice (issue #1614). # # HOW IT WORKS. The guard does not try to follow a role from where it is written # to the IAM resource that binds it: `locals`, `for_each` over sets and maps, diff --git a/scripts/test-azure-kv-access-policy.sh b/scripts/test-azure-kv-access-policy.sh index e33c55c88..fde42ee09 100755 --- a/scripts/test-azure-kv-access-policy.sh +++ b/scripts/test-azure-kv-access-policy.sh @@ -108,12 +108,10 @@ run_case "unknown flag exits 2" 2 --bogus-flag run_case "tf.json is rejected rather than silently passed" 2 \ "${FIXTURES}/json-encoding.tf.json" -# The regression half of issue #1621: the two modules that carried the inert -# access policy must stay free of one. This is a claim the guard can check -# directly, and it is the reason the guard exists. -run_case "cleanup-function module declares no access policy" 0 \ - "${REPO_ROOT}/terraform/modules/compute/azure/cleanup-function/main.tf" - +# The regression half of issue #1621: the module that carried the inert access +# policy must stay free of one. This is a claim the guard can check directly, +# and it is the reason the guard exists. (The other module that carried it, +# compute/azure/cleanup-function, was deleted in #357.) run_case "aks module declares no access policy" 0 \ "${REPO_ROOT}/terraform/modules/compute/azure/aks/main.tf" diff --git a/scripts/test-gcp-secret-scope.sh b/scripts/test-gcp-secret-scope.sh index 4f7895690..9e2aecf93 100755 --- a/scripts/test-gcp-secret-scope.sh +++ b/scripts/test-gcp-secret-scope.sh @@ -221,11 +221,6 @@ run_role_lines_case "every disallowed or lookalike per-secret role is reported ( run_case "waived per-secret role outside the allowed pair exits 0 (#505)" 0 \ "${FIXTURES}/per-secret-role-waived.tf.fixture" -# The regression half of issue #1614: the module that carried the over-broad -# grant must stay free of one. -run_case "cleanup-function module has no scope-wide Secret Manager grant" 0 \ - "${REPO_ROOT}/terraform/modules/compute/gcp/cleanup-function/main.tf" - WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT diff --git a/scripts/testdata/azure-kv-access-policy/access-policy.tf.fixture b/scripts/testdata/azure-kv-access-policy/access-policy.tf.fixture index f66026e0e..c8a831803 100644 --- a/scripts/testdata/azure-kv-access-policy/access-policy.tf.fixture +++ b/scripts/testdata/azure-kv-access-policy/access-policy.tf.fixture @@ -1,7 +1,7 @@ # Fixture: the #1621 anti-pattern verbatim. Must exit 1. # # This is the exact shape that shipped in -# terraform/modules/compute/azure/cleanup-function/main.tf and +# terraform/modules/compute/azure/cleanup-function/main.tf (since deleted) and # terraform/modules/compute/azure/aks/main.tf: an access policy declared against # a vault that has enable_rbac_authorization = true, which Azure accepts and # then ignores. diff --git a/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf b/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf index eb9ba7a44..37c470541 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf +++ b/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf @@ -5,9 +5,8 @@ # # WHY A BOUNDARY AND NOT A DENY. The deploy role legitimately creates roles on # every apply: twelve of them across terraform/modules (Lambda execution, -# Fargate task and task-execution, four EventBridge invoker roles, fck-nat, VPC -# flow logs, RDS proxy, secret rotation, and the cleanup Lambda's role, which is -# declared but not instantiated from any environment root today). Which of the +# Fargate task and task-execution, five EventBridge invoker roles, fck-nat, VPC +# flow logs, RDS proxy and secret rotation). Which of the # twelve exist in a given environment depends on compute_platform and the # enable_* flags; the count is not the point, the fact that the deploy path # creates roles at all is. Denying iam:CreateRole, diff --git a/terraform/modules/compute/aws/cleanup-lambda/.terraform.lock.hcl b/terraform/modules/compute/aws/cleanup-lambda/.terraform.lock.hcl deleted file mode 100644 index 211b520a1..000000000 --- a/terraform/modules/compute/aws/cleanup-lambda/.terraform.lock.hcl +++ /dev/null @@ -1,28 +0,0 @@ -# This file is maintained automatically by "terraform init". -# Manual edits may be lost in future updates. - -provider "registry.terraform.io/hashicorp/aws" { - version = "5.100.0" - constraints = "~> 5.100" - hashes = [ - "h1:Ijt7pOlB7Tr7maGQIqtsLFbl7pSMIj06TVdkoSBcYOw=", - "h1:edXOJWE4ORX8Fm+dpVpICzMZJat4AX0VRCAy/xkcOc0=", - "h1:hd45qFU5cFuJMpFGdUniU9mVIr5LYVWP1uMeunBpYYs=", - "h1:wOhTPz6apLBuF7/FYZuCoXRK/MLgrNprZ3vXmq83g5k=", - "zh:054b8dd49f0549c9a7cc27d159e45327b7b65cf404da5e5a20da154b90b8a644", - "zh:0b97bf8d5e03d15d83cc40b0530a1f84b459354939ba6f135a0086c20ebbe6b2", - "zh:1589a2266af699cbd5d80737a0fe02e54ec9cf2ca54e7e00ac51c7359056f274", - "zh:6330766f1d85f01ae6ea90d1b214b8b74cc8c1badc4696b165b36ddd4cc15f7b", - "zh:7c8c2e30d8e55291b86fcb64bdf6c25489d538688545eb48fd74ad622e5d3862", - "zh:99b1003bd9bd32ee323544da897148f46a527f622dc3971af63ea3e251596342", - "zh:9b12af85486a96aedd8d7984b0ff811a4b42e3d88dad1a3fb4c0b580d04fa425", - "zh:9f8b909d3ec50ade83c8062290378b1ec553edef6a447c56dadc01a99f4eaa93", - "zh:aaef921ff9aabaf8b1869a86d692ebd24fbd4e12c21205034bb679b9caf883a2", - "zh:ac882313207aba00dd5a76dbd572a0ddc818bb9cbf5c9d61b28fe30efaec951e", - "zh:bb64e8aff37becab373a1a0cc1080990785304141af42ed6aa3dd4913b000421", - "zh:dfe495f6621df5540d9c92ad40b8067376350b005c637ea6efac5dc15028add4", - "zh:f0ddf0eaf052766cfe09dea8200a946519f653c384ab4336e2a4a64fdd6310e9", - "zh:f1b7e684f4c7ae1eed272b6de7d2049bb87a0275cb04dbb7cda6636f600699c9", - "zh:ff461571e3f233699bf690db319dfe46aec75e58726636a0d97dd9ac6e32fb70", - ] -} diff --git a/terraform/modules/compute/aws/cleanup-lambda/main.tf b/terraform/modules/compute/aws/cleanup-lambda/main.tf deleted file mode 100644 index a7781067a..000000000 --- a/terraform/modules/compute/aws/cleanup-lambda/main.tf +++ /dev/null @@ -1,145 +0,0 @@ -# IAM role for cleanup Lambda -resource "aws_iam_role" "cleanup" { - name = "${var.stack_name}-cleanup-lambda" - permissions_boundary = var.permissions_boundary_arn - - assume_role_policy = jsonencode({ - Version = "2012-10-17" - Statement = [{ - Action = "sts:AssumeRole" - Effect = "Allow" - Principal = { - Service = "lambda.amazonaws.com" - } - }] - }) - - tags = var.tags -} - -# Basic Lambda execution policy -resource "aws_iam_role_policy_attachment" "cleanup_basic" { - role = aws_iam_role.cleanup.name - policy_arn = "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole" -} - -# VPC execution policy -resource "aws_iam_role_policy_attachment" "cleanup_vpc" { - role = aws_iam_role.cleanup.name - policy_arn = "arn:aws:iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole" -} - -# Policy to read database password from Secrets Manager -resource "aws_iam_role_policy" "cleanup_secrets" { - name = "${var.stack_name}-cleanup-secrets" - role = aws_iam_role.cleanup.id - - policy = jsonencode({ - Version = "2012-10-17" - Statement = [{ - Effect = "Allow" - Action = [ - "secretsmanager:GetSecretValue" - ] - Resource = var.db_password_secret_arn - }] - }) -} - -# CloudWatch log group -resource "aws_cloudwatch_log_group" "cleanup" { - name = "/aws/lambda/${var.stack_name}-cleanup" - retention_in_days = 90 - tags = var.tags -} - -# Lambda function -resource "aws_lambda_function" "cleanup" { - function_name = "${var.stack_name}-cleanup" - role = aws_iam_role.cleanup.arn - timeout = var.timeout - memory_size = var.memory_size - - # Use container image - package_type = "Image" - image_uri = var.image_uri - - image_config { - command = ["/app/cleanup-lambda"] - } - - environment { - variables = { - DB_HOST = var.db_host - DB_PORT = "5432" - DB_NAME = var.db_name - DB_USER = var.db_username - DB_PASSWORD_SECRET = var.db_password_secret_arn - DB_SSL_MODE = "require" - SECRET_PROVIDER = "aws" - } - } - - vpc_config { - subnet_ids = var.subnet_ids - security_group_ids = var.security_group_ids - } - - tags = var.tags - - depends_on = [ - aws_cloudwatch_log_group.cleanup, - aws_iam_role_policy_attachment.cleanup_basic, - aws_iam_role_policy_attachment.cleanup_vpc, - aws_iam_role_policy.cleanup_secrets - ] -} - -# EventBridge rule to run cleanup on schedule -resource "aws_cloudwatch_event_rule" "cleanup_schedule" { - name = "${var.stack_name}-cleanup" - description = "Trigger cleanup of expired sessions and executions" - schedule_expression = var.schedule_expression - tags = var.tags -} - -# EventBridge target -resource "aws_cloudwatch_event_target" "cleanup" { - rule = aws_cloudwatch_event_rule.cleanup_schedule.name - target_id = "cleanup-lambda" - arn = aws_lambda_function.cleanup.arn - - # Pass empty event (dryRun defaults to false) - input = jsonencode({ - dryRun = false - }) -} - -# Permission for EventBridge to invoke Lambda -resource "aws_lambda_permission" "cleanup_eventbridge" { - statement_id = "AllowEventBridgeInvoke" - action = "lambda:InvokeFunction" - function_name = aws_lambda_function.cleanup.function_name - principal = "events.amazonaws.com" - source_arn = aws_cloudwatch_event_rule.cleanup_schedule.arn -} - -# CloudWatch alarm for cleanup failures -resource "aws_cloudwatch_metric_alarm" "cleanup_errors" { - alarm_name = "${var.stack_name}-cleanup-errors" - comparison_operator = "GreaterThanThreshold" - evaluation_periods = 1 - metric_name = "Errors" - namespace = "AWS/Lambda" - period = 300 - statistic = "Sum" - threshold = 0 - treat_missing_data = "notBreaching" - - dimensions = { - FunctionName = aws_lambda_function.cleanup.function_name - } - - alarm_description = "Cleanup Lambda function errors" - tags = var.tags -} diff --git a/terraform/modules/compute/aws/cleanup-lambda/outputs.tf b/terraform/modules/compute/aws/cleanup-lambda/outputs.tf deleted file mode 100644 index e86d723e0..000000000 --- a/terraform/modules/compute/aws/cleanup-lambda/outputs.tf +++ /dev/null @@ -1,19 +0,0 @@ -output "function_arn" { - description = "ARN of the cleanup Lambda function" - value = aws_lambda_function.cleanup.arn -} - -output "function_name" { - description = "Name of the cleanup Lambda function" - value = aws_lambda_function.cleanup.function_name -} - -output "schedule_expression" { - description = "EventBridge schedule expression" - value = var.schedule_expression -} - -output "log_group_name" { - description = "CloudWatch log group name" - value = aws_cloudwatch_log_group.cleanup.name -} diff --git a/terraform/modules/compute/aws/cleanup-lambda/variables.tf b/terraform/modules/compute/aws/cleanup-lambda/variables.tf deleted file mode 100644 index cbe622e4e..000000000 --- a/terraform/modules/compute/aws/cleanup-lambda/variables.tf +++ /dev/null @@ -1,77 +0,0 @@ -variable "stack_name" { - description = "Name prefix for all resources" - type = string -} - -variable "permissions_boundary_arn" { - description = <<-EOT - ARN of the permissions boundary that every IAM role in this module must - carry. Required, not optional: cudly-terraform-deploy's IAM grants are - conditioned on iam:PermissionsBoundary, so a role created without this - boundary cannot have its inline policies or managed-policy attachments - written and the apply fails with AccessDenied. See - terraform/environments/aws/ci-cd-permissions/policy_boundary.tf. - EOT - type = string -} - -variable "image_uri" { - description = "Docker image URI containing the cleanup Lambda handler" - type = string -} - -variable "db_host" { - description = "Database host (RDS Proxy endpoint recommended)" - type = string -} - -variable "db_name" { - description = "PostgreSQL database name" - type = string - default = "cudly" -} - -variable "db_username" { - description = "PostgreSQL database username" - type = string - default = "cudly" -} - -variable "db_password_secret_arn" { - description = "ARN of the secret containing the database password" - type = string -} - -variable "subnet_ids" { - description = "VPC subnet IDs for Lambda function" - type = list(string) -} - -variable "security_group_ids" { - description = "Security group IDs for Lambda function" - type = list(string) -} - -variable "schedule_expression" { - description = "EventBridge schedule expression (default: daily at 2 AM UTC)" - type = string - default = "cron(0 2 * * ? *)" -} - -variable "timeout" { - description = "Lambda timeout in seconds" - type = number - default = 300 -} - -variable "memory_size" { - description = "Lambda memory size in MB" - type = number - default = 256 -} - -variable "tags" { - description = "Tags to apply to all resources" - type = map(string) - default = {} -} diff --git a/terraform/modules/compute/aws/cleanup-lambda/versions.tf b/terraform/modules/compute/aws/cleanup-lambda/versions.tf deleted file mode 100644 index 2b8d04a6b..000000000 --- a/terraform/modules/compute/aws/cleanup-lambda/versions.tf +++ /dev/null @@ -1,10 +0,0 @@ -terraform { - required_version = ">= 1.6.0" - - required_providers { - aws = { - source = "hashicorp/aws" - version = "~> 5.100" - } - } -} diff --git a/terraform/modules/compute/azure/cleanup-function/.terraform.lock.hcl b/terraform/modules/compute/azure/cleanup-function/.terraform.lock.hcl deleted file mode 100644 index 513bc597b..000000000 --- a/terraform/modules/compute/azure/cleanup-function/.terraform.lock.hcl +++ /dev/null @@ -1,25 +0,0 @@ -# This file is maintained automatically by "terraform init". -# Manual edits may be lost in future updates. - -provider "registry.terraform.io/hashicorp/azurerm" { - version = "3.117.1" - constraints = "~> 3.117" - hashes = [ - "h1:3c9iOEtBMnHrpJLlhbQ0sCZPWhE/2dvEPcL8KkXAh7w=", - "h1:HZXVBv06RTkWWsc73fLX2uITZPKfVLpuYhzbw+GJEOk=", - "h1:MufLWHQWuFt+iX9bthLooPP3dDZloujkzaIZPW5A/p4=", - "h1:j6wnjpHfBcQC4xd3ZYquaIPIIR46xJQs7rxwPdSOZos=", - "zh:0c513676836e3c50d004ece7d2624a8aff6faac14b833b96feeac2e4bc2c1c12", - "zh:50ea01ada95bae2f187db9e926e463f45d860767a85ebc59160414e00e76c35d", - "zh:52c2a9edacc06b3f72153f5ef6daca0761c6292158815961fe37f60bc576a3d7", - "zh:618eed2a06b19b1a025b45b05891846d570a6a1cca4d23f4942f5a99e1f747ae", - "zh:61cde5d3165d7e5ec311d5d89486819cd605c1b2d54611b5c97bd4e97dba2762", - "zh:6a873358d5031fc222f5e05f029d1237f3dce8345c767665f393283dfa2627f6", - "zh:afdd80064b2a04da311856feb4ed45f77ff4df6c356e8c2b10afb51fe7e61c70", - "zh:b09113df7e0e8c8959539bd22bae6c39faeb269ba3c4cd948e742f5cf58c35fb", - "zh:d340db7973109761cfc27d52aa02560363337c908b2c99b3628adc5a70a99d5b", - "zh:d5a577226ebc8c65e8f19384878a86acc4b51ede4b4a82d37c3b331b0efcd4a7", - "zh:e2962b147f9e71732df8dbc74940c10d20906f3c003cbfaa1eb9fabbf601a9f0", - "zh:f569b65999264a9416862bca5cd2a6177d94ccb0424f3a4ef424428912b9cb3c", - ] -} diff --git a/terraform/modules/compute/azure/cleanup-function/main.tf b/terraform/modules/compute/azure/cleanup-function/main.tf deleted file mode 100644 index b09546ca3..000000000 --- a/terraform/modules/compute/azure/cleanup-function/main.tf +++ /dev/null @@ -1,154 +0,0 @@ -# Storage account for Function App -resource "azurerm_storage_account" "cleanup" { - name = var.storage_account_name - resource_group_name = var.resource_group_name - location = var.location - account_tier = "Standard" - account_replication_type = "LRS" - tags = var.tags -} - -# App Service Plan for Linux containers -resource "azurerm_service_plan" "cleanup" { - name = "${var.function_app_name}-plan" - resource_group_name = var.resource_group_name - location = var.location - os_type = "Linux" - sku_name = "B1" # Basic tier for scheduled functions - - tags = var.tags -} - -# Managed identity for Function App -resource "azurerm_user_assigned_identity" "cleanup" { - name = "${var.function_app_name}-identity" - resource_group_name = var.resource_group_name - location = var.location - tags = var.tags -} - -# Grant Key Vault access to the managed identity. -# -# This MUST be an RBAC role assignment, not an azurerm_key_vault_access_policy. -# The vault this module is pointed at (terraform/modules/secrets/azure/main.tf) -# sets enable_rbac_authorization = true, and an RBAC-enabled vault never -# consults its accessPolicies array. Terraform applies an access policy against -# such a vault successfully, so the mistake is invisible at plan and apply time -# and only surfaces as a runtime 403 when the function reads db-password (#1621). -# -# "Key Vault Secrets User" is the exact RBAC equivalent of the access policy this -# replaced: its dataActions are getSecret + readMetadata, which is precisely -# secret_permissions = ["Get", "List"], and its actions list is empty so it -# confers no management-plane rights. -resource "azurerm_role_assignment" "cleanup_kv_secrets_user" { - scope = var.key_vault_id - role_definition_name = "Key Vault Secrets User" - principal_id = azurerm_user_assigned_identity.cleanup.principal_id -} - -# Linux Function App with container -resource "azurerm_linux_function_app" "cleanup" { - name = var.function_app_name - resource_group_name = var.resource_group_name - location = var.location - service_plan_id = azurerm_service_plan.cleanup.id - - storage_account_name = azurerm_storage_account.cleanup.name - storage_account_access_key = azurerm_storage_account.cleanup.primary_access_key - - # Use container image - site_config { - always_on = true - - application_stack { - docker { - registry_url = split("/", var.image_uri)[0] - image_name = join("/", slice(split("/", var.image_uri), 1, length(split("/", var.image_uri)) - 1)) - image_tag = split(":", var.image_uri)[1] - } - } - - # VNet integration: vnet_route_all_enabled is a scalar bool on - # site_config, so set it conditionally rather than wrapping in a - # dynamic block (which expects nested-block content). - vnet_route_all_enabled = var.subnet_id != "" - } - - # App settings (environment variables) - app_settings = { - DB_HOST = var.db_host - DB_PORT = "5432" - DB_NAME = "cudly" - DB_USER = "cudly" - DB_PASSWORD_SECRET = var.db_password_secret_uri - DB_SSL_MODE = "require" - SECRET_PROVIDER = "azure" - AZURE_KEY_VAULT_URL = replace(var.db_password_secret_uri, "/secrets/.*", "") - - # Function runtime settings - FUNCTIONS_WORKER_RUNTIME = "custom" - WEBSITES_ENABLE_APP_SERVICE_STORAGE = "false" - } - - # Managed identity - identity { - type = "UserAssigned" - identity_ids = [azurerm_user_assigned_identity.cleanup.id] - } - - # VNet integration: virtual_network_subnet_id is a scalar string - # attribute on the function-app resource — null when no VNet is - # supplied, otherwise the configured subnet ID. The previous - # dynamic-around-scalar wrapper was rejected by Terraform. - virtual_network_subnet_id = var.subnet_id != "" ? var.subnet_id : null - - tags = var.tags - - # Nothing in this resource references the Key Vault grant, so without this - # edge Terraform is free to create the function app in parallel with it. - # Ordering only, not a propagation wait: the role assignment returns as soon - # as ARM accepts the write, so an invocation immediately after can still 403 - # on db-password until the grant propagates. - depends_on = [azurerm_role_assignment.cleanup_kv_secrets_user] -} - -# Timer trigger function (defined in host.json and function.json) -# Note: Azure Functions require function.json in the container image -# The container should have a structure like: -# /home/site/wwwroot/cleanup/function.json -# /home/site/wwwroot/host.json - -# Logic App workflow to trigger the function (alternative to timer trigger) -resource "azurerm_logic_app_workflow" "cleanup_trigger" { - name = "${var.function_app_name}-trigger" - location = var.location - resource_group_name = var.resource_group_name - tags = var.tags -} - -# Recurrence trigger -resource "azurerm_logic_app_trigger_recurrence" "cleanup" { - name = "cleanup-schedule" - logic_app_id = azurerm_logic_app_workflow.cleanup_trigger.id - frequency = "Day" - interval = 1 - start_time = formatdate("YYYY-MM-DD'T'02:00:00'Z'", timestamp()) - time_zone = "UTC" -} - -# HTTP action to call Function App -resource "azurerm_logic_app_action_http" "cleanup" { - name = "call-cleanup-function" - logic_app_id = azurerm_logic_app_workflow.cleanup_trigger.id - method = "POST" - uri = "https://${azurerm_linux_function_app.cleanup.default_hostname}/api/cleanup" - - headers = { - "Content-Type" = "application/json" - "x-functions-key" = "@listKeys('${azurerm_linux_function_app.cleanup.id}/host/default', '2022-03-01').functionKeys.default" - } - - body = jsonencode({ - dryRun = false - }) -} diff --git a/terraform/modules/compute/azure/cleanup-function/outputs.tf b/terraform/modules/compute/azure/cleanup-function/outputs.tf deleted file mode 100644 index 2118335cf..000000000 --- a/terraform/modules/compute/azure/cleanup-function/outputs.tf +++ /dev/null @@ -1,14 +0,0 @@ -output "function_app_name" { - description = "Name of the Function App" - value = azurerm_linux_function_app.cleanup.name -} - -output "function_app_url" { - description = "Default hostname of the Function App" - value = "https://${azurerm_linux_function_app.cleanup.default_hostname}" -} - -output "identity_principal_id" { - description = "Principal ID of the managed identity" - value = azurerm_user_assigned_identity.cleanup.principal_id -} diff --git a/terraform/modules/compute/azure/cleanup-function/variables.tf b/terraform/modules/compute/azure/cleanup-function/variables.tf deleted file mode 100644 index 0f5f172b2..000000000 --- a/terraform/modules/compute/azure/cleanup-function/variables.tf +++ /dev/null @@ -1,51 +0,0 @@ -variable "resource_group_name" { - description = "Name of the resource group" - type = string -} - -variable "location" { - description = "Azure region" - type = string -} - -variable "function_app_name" { - description = "Name of the Function App" - type = string -} - -variable "storage_account_name" { - description = "Name of the storage account for Function App" - type = string -} - -variable "image_uri" { - description = "Container image URI for the cleanup function" - type = string -} - -variable "db_host" { - description = "Database host (Azure Database for PostgreSQL FQDN)" - type = string -} - -variable "db_password_secret_uri" { - description = "Azure Key Vault secret URI containing the database password" - type = string -} - -variable "key_vault_id" { - description = "ID of the Key Vault containing secrets" - type = string -} - -variable "subnet_id" { - description = "Subnet ID for VNet integration" - type = string - default = "" -} - -variable "tags" { - description = "Tags to apply to all resources" - type = map(string) - default = {} -} diff --git a/terraform/modules/compute/azure/cleanup-function/versions.tf b/terraform/modules/compute/azure/cleanup-function/versions.tf deleted file mode 100644 index 771ddc19c..000000000 --- a/terraform/modules/compute/azure/cleanup-function/versions.tf +++ /dev/null @@ -1,10 +0,0 @@ -terraform { - required_version = ">= 1.10.0" - - required_providers { - azurerm = { - source = "hashicorp/azurerm" - version = "~> 3.117" - } - } -} diff --git a/terraform/modules/compute/gcp/cleanup-function/.terraform.lock.hcl b/terraform/modules/compute/gcp/cleanup-function/.terraform.lock.hcl deleted file mode 100644 index f062f72ba..000000000 --- a/terraform/modules/compute/gcp/cleanup-function/.terraform.lock.hcl +++ /dev/null @@ -1,25 +0,0 @@ -# This file is maintained automatically by "terraform init". -# Manual edits may be lost in future updates. - -provider "registry.terraform.io/hashicorp/google" { - version = "5.45.2" - constraints = "~> 5.45" - hashes = [ - "h1:cbY/dnrOpmkAzgU/yGTjDoRdlY2FEtxiQCruSTCt5PE=", - "h1:iy2Q9VcnMu4z/bH3v/NmI/nEpgYY7bXgJmT/hVTAUS4=", - "h1:k8taQAdfHrv2F/AiGV5BZBZfI+1uaq8g6O8dWzjx42c=", - "h1:y2hf6zus1eKA5vpAfoyYkNBKDBQTVqmx4OVh3iRBaRo=", - "zh:0d09c8f20b556305192cdbe0efa6d333ceebba963a8ba91f9f1714b5a20c4b7a", - "zh:117143fc91be407874568df416b938a6896f94cb873f26bba279cedab646a804", - "zh:16ccf77d18dd2c5ef9c0625f9cf546ebdf3213c0a452f432204c69feed55081e", - "zh:3e555cf22a570a4bd247964671f421ed7517970cd9765ceb46f335edc2c6f392", - "zh:688bd5b05a75124da7ae6e885b2b92bd29f4261808b2b78bd5f51f525c1052ca", - "zh:6db3ef37a05010d82900bfffb3261c59a0c247e0692049cb3eb8c2ef16c9d7bf", - "zh:70316fde75f6a15d72749f66d994ccbdde5f5ed4311b6d06b99850f698c9bbf9", - "zh:84b8e583771a4f2bd514e519d98ed7fd28dce5efe0634e973170e1cfb5556fb4", - "zh:9d4b8ef0a9b6677935c604d94495042e68ff5489932cfd1ec41052e094a279d3", - "zh:a2089dd9bd825c107b148dd12d6b286f71aa37dfd4ca9c35157f2dcba7bc19d8", - "zh:f03d795c0fd9721e59839255ee7ba7414173017dc530b4ce566daf3802a0d6dd", - "zh:f569b65999264a9416862bca5cd2a6177d94ccb0424f3a4ef424428912b9cb3c", - ] -} diff --git a/terraform/modules/compute/gcp/cleanup-function/main.tf b/terraform/modules/compute/gcp/cleanup-function/main.tf deleted file mode 100644 index f8c6a1d38..000000000 --- a/terraform/modules/compute/gcp/cleanup-function/main.tf +++ /dev/null @@ -1,127 +0,0 @@ -# Service account for Cloud Function -resource "google_service_account" "cleanup" { - project = var.project_id - account_id = "${var.function_name}-sa" - display_name = "Service account for ${var.function_name}" -} - -# Secret Manager access: least-privilege, single-secret binding. -# -# This previously granted the cleanup service account project-wide -# `roles/secretmanager.secretAccessor`, which made every secret in the project -# readable by a session-cleanup job. That set includes the AES-256-GCM -# credential-encryption key that decrypts stored customer cloud credentials, the -# JWT and session secrets, and the SendGrid API key. None of them are reachable -# from this function's code path. -# -# The function reads exactly one secret: the database password, wired into -# DB_PASSWORD_SECRET in service_config below and resolved once by -# database.OpenFromEnv (cmd/cleanup-lambda/main.go). Bind only that secret, -# mirroring the same migration already applied to the Cloud Run module in -# compute/gcp/cloud-run/main.tf. -resource "google_secret_manager_secret_iam_member" "cleanup_db_password" { - project = var.project_id - secret_id = var.db_password_secret_id - role = "roles/secretmanager.secretAccessor" - member = "serviceAccount:${google_service_account.cleanup.email}" -} - -# Cloud Function (2nd gen) -resource "google_cloudfunctions2_function" "cleanup" { - name = var.function_name - location = var.region - project = var.project_id - - build_config { - runtime = "go121" - entry_point = "cleanupExpiredRecords" - - # Source archive is uploaded by the build/CI pipeline before this module - # is applied (see var.source_object_name); Terraform only points the - # function at it, it does not own or generate the object. - source { - storage_source { - bucket = google_storage_bucket.function_source.name - object = var.source_object_name - } - } - } - - service_config { - max_instance_count = 1 - timeout_seconds = 300 - service_account_email = google_service_account.cleanup.email - - # Restrict to internal traffic only; external triggers go via Cloud Scheduler with OIDC - ingress_settings = "ALLOW_INTERNAL_ONLY" - - environment_variables = { - DB_HOST = var.db_host - DB_PORT = "5432" - DB_NAME = "cudly" - DB_USER = "cudly" - DB_PASSWORD_SECRET = var.db_password_secret_id - DB_SSL_MODE = "require" - SECRET_PROVIDER = "gcp" - GCP_PROJECT_ID = var.project_id - } - - # VPC connector for private Cloud SQL access - vpc_connector = var.vpc_connector != "" ? var.vpc_connector : null - vpc_connector_egress_settings = var.vpc_connector != "" ? "ALL_TRAFFIC" : null - } - - labels = var.labels -} - -# Storage bucket for function source (required for Cloud Functions) -resource "google_storage_bucket" "function_source" { - project = var.project_id - name = "${var.project_id}-${var.function_name}-source" - location = var.region - force_destroy = false # Prevent accidental data loss; delete bucket contents manually before destroying - - uniform_bucket_level_access = true - public_access_prevention = "enforced" -} - -# Cloud Scheduler job to trigger the function -resource "google_cloud_scheduler_job" "cleanup" { - project = var.project_id - region = var.region - name = "${var.function_name}-schedule" - description = "Trigger cleanup of expired sessions and executions" - schedule = var.schedule - time_zone = "UTC" - attempt_deadline = "320s" - - http_target { - http_method = "POST" - uri = google_cloudfunctions2_function.cleanup.service_config[0].uri - - body = base64encode(jsonencode({ - dryRun = false - })) - - headers = { - "Content-Type" = "application/json" - } - - oidc_token { - service_account_email = google_service_account.cleanup.email - } - } - - retry_config { - retry_count = 1 - } -} - -# Cloud Function invoker permission for Cloud Scheduler -resource "google_cloudfunctions2_function_iam_member" "cleanup_invoker" { - project = google_cloudfunctions2_function.cleanup.project - location = google_cloudfunctions2_function.cleanup.location - cloud_function = google_cloudfunctions2_function.cleanup.name - role = "roles/cloudfunctions.invoker" - member = "serviceAccount:${google_service_account.cleanup.email}" -} diff --git a/terraform/modules/compute/gcp/cleanup-function/outputs.tf b/terraform/modules/compute/gcp/cleanup-function/outputs.tf deleted file mode 100644 index b8eb81589..000000000 --- a/terraform/modules/compute/gcp/cleanup-function/outputs.tf +++ /dev/null @@ -1,19 +0,0 @@ -output "function_uri" { - description = "URI of the Cloud Function" - value = google_cloudfunctions2_function.cleanup.service_config[0].uri -} - -output "function_name" { - description = "Name of the Cloud Function" - value = google_cloudfunctions2_function.cleanup.name -} - -output "schedule" { - description = "Cloud Scheduler schedule" - value = var.schedule -} - -output "service_account_email" { - description = "Service account email for the cleanup function" - value = google_service_account.cleanup.email -} diff --git a/terraform/modules/compute/gcp/cleanup-function/variables.tf b/terraform/modules/compute/gcp/cleanup-function/variables.tf deleted file mode 100644 index 2f6809a77..000000000 --- a/terraform/modules/compute/gcp/cleanup-function/variables.tf +++ /dev/null @@ -1,60 +0,0 @@ -variable "project_id" { - description = "GCP project ID" - type = string -} - -variable "region" { - description = "GCP region" - type = string -} - -variable "function_name" { - description = "Name of the Cloud Function" - type = string - default = "cudly-cleanup" -} - -variable "db_host" { - description = "Database host (Cloud SQL connection name or private IP)" - type = string -} - -variable "db_password_secret_id" { - description = "Secret Manager secret ID containing the database password" - type = string -} - -variable "vpc_connector" { - description = "VPC connector for private Cloud SQL access" - type = string - default = "" -} - -variable "schedule" { - description = "Cloud Scheduler schedule (cron format)" - type = string - default = "0 2 * * *" -} - -variable "labels" { - description = "Labels to apply to all resources" - type = map(string) - default = {} -} - -variable "source_object_name" { - description = <<-EOT - Name of the object already present in the function-source bucket - (google_storage_bucket.function_source) containing the packaged Cloud - Function source archive. Supplied by the build/CI pipeline, which must - upload the object before this module is applied -- the same - externally-supplied-artifact pattern terraform/modules/build establishes - for container images (see its registry_url/custom_image_tag variables). - Embed a content hash or build ID in the object name (e.g. - "cleanup-.zip") so a source change produces a new name and - therefore a new Cloud Function revision; reusing the same name for - changed content is a no-op deploy because Cloud Functions deploys by - object identity, not by content diffing. - EOT - type = string -} diff --git a/terraform/modules/compute/gcp/cleanup-function/versions.tf b/terraform/modules/compute/gcp/cleanup-function/versions.tf deleted file mode 100644 index 1bc3382e3..000000000 --- a/terraform/modules/compute/gcp/cleanup-function/versions.tf +++ /dev/null @@ -1,10 +0,0 @@ -terraform { - required_version = ">= 1.5" - - required_providers { - google = { - source = "hashicorp/google" - version = "~> 5.45" - } - } -} diff --git a/terraform/modules/compute/gcp/cleanup_function_source_test.go b/terraform/modules/compute/gcp/cleanup_function_source_test.go deleted file mode 100644 index 5d12a97de..000000000 --- a/terraform/modules/compute/gcp/cleanup_function_source_test.go +++ /dev/null @@ -1,45 +0,0 @@ -// Regression test for issue #124: cleanup-function's source object pointed at -// a placeholder.zip that was never committed to the repository (a clean -// checkout could not apply the module at all), and its object name embedded -// timestamp(), which changes on every plan and would redeploy the Cloud -// Function from the placeholder archive on every unrelated apply, wiping out -// whatever real source was deployed out-of-band. -package gcp_test - -import ( - "os" - "regexp" - "testing" -) - -func TestCleanupFunctionSourceIsExternallySupplied(t *testing.T) { - mainData, err := os.ReadFile("cleanup-function/main.tf") - if err != nil { - t.Fatalf("reading cleanup-function/main.tf: %v", err) - } - main := string(mainData) - - if regexp.MustCompile(`\btimestamp\s*\(`).MatchString(main) { - t.Error(`cleanup-function/main.tf calls timestamp(); the source object name must be stable across plans (issue #124), not regenerated on every apply`) - } - - if regexp.MustCompile(`resource\s+"google_storage_bucket_object"\s+"function_source"`).MatchString(main) { - t.Error(`cleanup-function/main.tf still defines google_storage_bucket_object.function_source; the module must not own/generate the source archive (it previously pointed at an uncommitted placeholder.zip) -- take the object name as an externally-supplied variable instead`) - } - - if !regexp.MustCompile(`object\s*=\s*var\.source_object_name`).MatchString(main) { - t.Error(`cleanup-function/main.tf's storage_source block must set object = var.source_object_name, so the build/CI pipeline supplies the already-uploaded source archive instead of Terraform requiring a committed placeholder.zip`) - } - - varsData, err := os.ReadFile("cleanup-function/variables.tf") - if err != nil { - t.Fatalf("reading cleanup-function/variables.tf: %v", err) - } - if !regexp.MustCompile(`variable\s+"source_object_name"\s*\{`).MatchString(string(varsData)) { - t.Error(`cleanup-function/variables.tf must declare a source_object_name variable`) - } - - if _, err := os.Stat("cleanup-function/placeholder.zip"); err == nil { - t.Error(`cleanup-function/placeholder.zip exists but should not: the fix for issue #124 removes the module's dependency on a committed placeholder archive`) - } -}