diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 3cfde9765..0fc65c61d 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -599,16 +599,16 @@ Each cloud provider has lifecycle policies configured via Terraform (`terraform/ ### Database Cleanup Jobs -The server's `cleanup` task deletes expired sessions and, via `CleanupOldExecutions(30)`, purchase executions that are completed with `scheduled_date` older than 30 days, canceled with `updated_at` older than 30 days, or in any status other than failed or canceled with `expires_at` older than 30 days. Failed executions are kept. Azure Container Apps schedules it by default; AWS Lambda and CloudFormation can schedule it on request: +The server's `cleanup` task deletes expired sessions and, via `CleanupOldExecutions(30)`, purchase executions that are completed with `scheduled_date` older than 30 days, canceled with `updated_at` older than 30 days, or in any status other than failed or canceled with `expires_at` older than 30 days. Failed executions are kept. Azure Container Apps schedules it by default; AWS Lambda, AWS Fargate and CloudFormation can schedule it on request: - **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. - **CloudFormation (opt-in):** set the `EnableCleanupSchedule` stack parameter to `true` (default `false`; `CleanupSchedule` defaults to `rate(1 day)`) on `cloudformation/stacks/CUDly`. The same backlog warning applies. -- **AWS Fargate:** nothing schedules it yet. The `terraform/modules/compute/aws/cleanup-lambda/` 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. (The `terraform/modules/compute/aws/cleanup-lambda/` module is not consumed by any environment.) - **GCP:** nothing schedules it. The `terraform/modules/compute/gcp/cleanup-function/` module is not consumed by any environment. - **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. -Scheduling for AWS Fargate, CloudFormation and GCP is tracked by #712. +Scheduling for GCP is tracked by #712. Alternatively, use pg_cron for database-native scheduling. @@ -618,8 +618,9 @@ CUDly learns that an RI listed on the AWS Reserved Instance Marketplace sold, cl - **AWS Lambda:** hourly by default. `enable_finalize_revocations_schedule` (default `true`) and `finalize_revocations_schedule` (default `rate(1 hour)`) in `terraform/environments/aws`. - **CloudFormation:** hourly by default. The `EnableFinalizeRevocationsSchedule` (default `true`) and `FinalizeRevocationsSchedule` (default `rate(1 hour)`) stack parameters on `cloudformation/stacks/CUDly`. +- **AWS Fargate:** hourly by default, as a one-off ECS task. The same `enable_finalize_revocations_schedule` (default `true`) and `finalize_revocations_schedule` (default `rate(1 hour)`) variables in `terraform/environments/aws`. - **AKS:** hourly by default, as a Kubernetes CronJob, when `deploy_kubernetes_resources` is true. `enable_finalize_revocations_schedule` (default `true`) and `finalize_revocations_schedule` (default `30 * * * *`) on the AKS module. -- **AWS Fargate, GCP Cloud Run, GKE and Azure Container Apps:** nothing schedules `finalize_revocations`, so the poll does not run and listings stay as the list and cancel endpoints recorded them. Until #712 wires the task there, call `POST /api/scheduled/finalize_revocations` (authenticated like the other `/api/scheduled/` tasks) from your own scheduler, about hourly. +- **GCP Cloud Run, GKE and Azure Container Apps:** nothing schedules `finalize_revocations`, so the poll does not run and listings stay as the list and cancel endpoints recorded them. Until #712 wires the task there, call `POST /api/scheduled/finalize_revocations` (authenticated like the other `/api/scheduled/` tasks) from your own scheduler, about hourly. --- diff --git a/known_issues/33_executions_retention_cleanup.md b/known_issues/33_executions_retention_cleanup.md index caa2d4f02..a68f7c7bf 100644 --- a/known_issues/33_executions_retention_cleanup.md +++ b/known_issues/33_executions_retention_cleanup.md @@ -9,7 +9,7 @@ > schedules cleanup on AWS. The same holds for the Terraform behind the > "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 and CloudFormation when opted in. +> scheduling is tracked by #712; retention is scheduled via the server `cleanup` task on AWS Lambda, AWS Fargate and CloudFormation when opted in. ## Resolution @@ -21,6 +21,9 @@ `terraform/modules/compute/aws/cleanup-lambda/main.tf:98`~~ (unconsumed module, see amendment) - **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` / + `cleanup_schedule` variables add a `-fargate-sweep-cleanup` rule + that runs `./cudly --task cleanup` as a one-off ECS task. - **CloudFormation (opt-in):** the `EnableCleanupSchedule` / `CleanupSchedule` stack parameters add a `-Cleanup` rule that runs the server cleanup task. @@ -34,7 +37,7 @@ The `CleanupOldExecutions` Postgres implementation in `internal/config/store_postgres.go` runs the expected `DELETE FROM purchase_executions WHERE …`, so the table is bounded -only where that task is scheduled (Azure Container Apps by default, AWS Lambda and CloudFormation when opted in, see amendment). +only where that task is scheduled (Azure Container Apps by default, AWS Lambda, AWS Fargate and CloudFormation when opted in, see amendment). ## Original problem (historical context) diff --git a/terraform/environments/aws/compute.tf b/terraform/environments/aws/compute.tf index 27b090787..150462e9b 100644 --- a/terraform/environments/aws/compute.tf +++ b/terraform/environments/aws/compute.tf @@ -232,6 +232,20 @@ module "compute_fargate" { enable_fire_scheduled_purchases_schedule = var.enable_fire_scheduled_purchases_schedule fire_scheduled_purchases_schedule = var.fire_scheduled_purchases_schedule + # Sweeps (sweeps.tf); process, notifications and cleanup are opt-in + enable_process_scheduled_purchases_schedule = var.enable_process_scheduled_purchases_schedule + process_scheduled_purchases_schedule = var.process_scheduled_purchases_schedule + enable_send_notifications_schedule = var.enable_send_notifications_schedule + send_notifications_schedule = var.send_notifications_schedule + enable_finalize_revocations_schedule = var.enable_finalize_revocations_schedule + finalize_revocations_schedule = var.finalize_revocations_schedule + enable_reap_stuck_purchases_schedule = var.enable_reap_stuck_purchases_schedule + reap_stuck_purchases_schedule = var.reap_stuck_purchases_schedule + enable_analytics_collect_schedule = var.enable_analytics_collect_schedule + analytics_collect_schedule = var.analytics_collect_schedule + enable_cleanup_schedule = var.enable_cleanup_schedule + cleanup_schedule = var.cleanup_schedule + # ECS Exec for debugging enable_execute_command = var.fargate_enable_execute_command diff --git a/terraform/modules/compute/aws/fargate/sweeps.tf b/terraform/modules/compute/aws/fargate/sweeps.tf new file mode 100644 index 000000000..149a89249 --- /dev/null +++ b/terraform/modules/compute/aws/fargate/sweeps.tf @@ -0,0 +1,137 @@ +# Additional scheduled sweeps (#712). Each enabled entry becomes an +# EventBridge rule whose target runs a one-off copy of the app task with +# `./cudly --task `, the same entry point the recommendations, RI +# exchange, ladder and fire targets in main.tf use. Task names, schedule +# variables and enable defaults match the Lambda module: reap_stuck_purchases, +# analytics_collect and finalize_revocations are on by default; +# process_scheduled_purchases, send_notifications and cleanup are opt-in +# (default off), so no deployment starts proposing purchases, sending +# notification emails or deleting execution history unless it asks to. +# +# `rule` is the rule-name suffix: rule names are +# "-fargate-sweep-" (longest suffix "-sweep-analytics"). +# Retries stay off, as on the other ECS targets in main.tf; a missed run is +# picked up by the next tick, and an overlapping run is a no-op under the +# server's per-task advisory lock. +locals { + sweep_tasks = { + reap_stuck_purchases = { rule = "reap", enabled = var.enable_reap_stuck_purchases_schedule, schedule = var.reap_stuck_purchases_schedule } + analytics_collect = { rule = "analytics", enabled = var.enable_analytics_collect_schedule, schedule = var.analytics_collect_schedule } + process_scheduled_purchases = { rule = "process", enabled = var.enable_process_scheduled_purchases_schedule, schedule = var.process_scheduled_purchases_schedule } + send_notifications = { rule = "notify", enabled = var.enable_send_notifications_schedule, schedule = var.send_notifications_schedule } + finalize_revocations = { rule = "finalize", enabled = var.enable_finalize_revocations_schedule, schedule = var.finalize_revocations_schedule } + cleanup = { rule = "cleanup", enabled = var.enable_cleanup_schedule, schedule = var.cleanup_schedule } + } + active_sweeps = { for k, v in local.sweep_tasks : k => v if v.enabled } +} + +resource "aws_cloudwatch_event_rule" "sweep" { + for_each = local.active_sweeps + + name = "${local.name_prefix}-sweep-${each.value.rule}" + description = "Trigger the ${each.key} scheduled task" + schedule_expression = each.value.schedule + + tags = local.common_tags +} + +resource "aws_cloudwatch_event_target" "sweep" { + for_each = local.active_sweeps + + rule = aws_cloudwatch_event_rule.sweep[each.key].name + target_id = "ecs-task" + arn = aws_ecs_cluster.main.arn + role_arn = aws_iam_role.eventbridge_sweeps[0].arn + + ecs_target { + task_count = 1 + task_definition_arn = aws_ecs_task_definition.main.arn + launch_type = "FARGATE" + platform_version = "LATEST" + + network_configuration { + subnets = var.private_subnet_ids + security_groups = [aws_security_group.ecs_tasks.id] + assign_public_ip = false + } + } + + retry_policy { + maximum_retry_attempts = 0 + maximum_event_age_in_seconds = 3600 + } + + input = jsonencode({ + containerOverrides = [{ + name = "app" + command = ["./cudly", "--task", each.key] + }] + }) +} + +# One invoker role for all sweeps: every sweep runs the same task definition +# in the same cluster, so per-task roles would carry identical policies. Same +# scoping as the other invoker roles in main.tf (#292): RunTask only on the +# module's task definition in the module's cluster, PassRole only for the +# task and task-execution roles and only to ECS tasks. +resource "aws_iam_role" "eventbridge_sweeps" { + count = length(local.active_sweeps) > 0 ? 1 : 0 + + name = "${local.name_prefix}-eb-sweeps" + permissions_boundary = var.permissions_boundary_arn + + assume_role_policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Effect = "Allow" + Principal = { + Service = "events.amazonaws.com" + } + Action = "sts:AssumeRole" + } + ] + }) + + tags = local.common_tags +} + +resource "aws_iam_role_policy" "eventbridge_sweeps" { + count = length(local.active_sweeps) > 0 ? 1 : 0 + + name = "ecs-run-task" + role = aws_iam_role.eventbridge_sweeps[0].id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Effect = "Allow" + Action = [ + "ecs:RunTask" + ] + Resource = aws_ecs_task_definition.main.arn + Condition = { + ArnEquals = { + "ecs:cluster" = aws_ecs_cluster.main.arn + } + } + }, + { + Effect = "Allow" + Action = [ + "iam:PassRole" + ] + Resource = [ + aws_iam_role.task_execution.arn, + aws_iam_role.task.arn + ] + Condition = { + StringEquals = { + "iam:PassedToService" = "ecs-tasks.amazonaws.com" + } + } + } + ] + }) +} diff --git a/terraform/modules/compute/aws/fargate/tests/eventbridge_invoker_iam.tftest.hcl b/terraform/modules/compute/aws/fargate/tests/eventbridge_invoker_iam.tftest.hcl index 3162857ab..25e807667 100644 --- a/terraform/modules/compute/aws/fargate/tests/eventbridge_invoker_iam.tftest.hcl +++ b/terraform/modules/compute/aws/fargate/tests/eventbridge_invoker_iam.tftest.hcl @@ -1,4 +1,4 @@ -# The four EventBridge invoker roles (#292). Each may run exactly the module's +# The five EventBridge invoker roles (#292, plus the shared sweeps role of #712). Each may run exactly the module's # task definition, only in the module's cluster, and may pass the task and # task-execution roles only to ECS tasks. Without the two conditions an actor # who can create EventBridge targets in the account can run the task definition @@ -10,7 +10,7 @@ # overrides give those resources distinct, well-formed ARNs: the provider # validates ARN-typed arguments, and two roles sharing one mock ARN would let # a PassRole statement naming only one of them pass. Roles not overridden -# (the four invoker roles) share the default, which nothing asserts on. +# (the five invoker roles) share the default, which nothing asserts on. mock_provider "aws" { mock_resource "aws_iam_role" { defaults = { arn = "arn:aws:iam::123456789012:role/cudly-prod-abcd1234-fargate-mock" } @@ -136,4 +136,21 @@ run "invoker_roles_are_scoped" { ] == [[toset([aws_iam_role.task_execution.arn, aws_iam_role.task.arn]), "ecs-tasks.amazonaws.com"]] error_message = "eventbridge_fire_scheduled_purchases: iam:PassRole must be one statement on the task and task-execution roles, conditioned on StringEquals iam:PassedToService = ecs-tasks.amazonaws.com." } + + # sweeps (one role for every sweep in sweeps.tf; reap, analytics_collect + # and finalize_revocations are on by default, so the role exists here) + assert { + condition = [ + for s in jsondecode(aws_iam_role_policy.eventbridge_sweeps[0].policy).Statement : + [s.Resource, try(s.Condition.ArnEquals["ecs:cluster"], null)] if contains(s.Action, "ecs:RunTask") + ] == [[aws_ecs_task_definition.main.arn, aws_ecs_cluster.main.arn]] + error_message = "eventbridge_sweeps: ecs:RunTask must be one statement on the module's task definition, conditioned on ArnEquals ecs:cluster = the module's cluster." + } + assert { + condition = [ + for s in jsondecode(aws_iam_role_policy.eventbridge_sweeps[0].policy).Statement : + [toset(s.Resource), try(s.Condition.StringEquals["iam:PassedToService"], null)] if contains(s.Action, "iam:PassRole") + ] == [[toset([aws_iam_role.task_execution.arn, aws_iam_role.task.arn]), "ecs-tasks.amazonaws.com"]] + error_message = "eventbridge_sweeps: iam:PassRole must be one statement on the task and task-execution roles, conditioned on StringEquals iam:PassedToService = ecs-tasks.amazonaws.com." + } } diff --git a/terraform/modules/compute/aws/fargate/tests/sweeps.tftest.hcl b/terraform/modules/compute/aws/fargate/tests/sweeps.tftest.hcl new file mode 100644 index 000000000..3d9564308 --- /dev/null +++ b/terraform/modules/compute/aws/fargate/tests/sweeps.tftest.hcl @@ -0,0 +1,112 @@ +# Module wiring for the Fargate sweep rules (#712). Defaults and opt-in policy +# match the Lambda module's sweeps.tftest.hcl. Rule -> target key cross-wiring +# is checked by the Go test in terraform/modules/compute; invoker-role scoping +# by eventbridge_invoker_iam.tftest.hcl. +mock_provider "aws" {} + +variables { + # Stay within the module's existing 32-character ALB name limit. + # Sweep rule names are checked separately against their 64-character limit. + stack_name = "cudly-prod-abcd1234" + enable_cross_account_sts = false + permissions_boundary_arn = "arn:aws:iam::123456789012:policy/cudly-boundary" + environment = "production" + region = "us-east-1" + image_uri = "123456789012.dkr.ecr.us-east-1.amazonaws.com/cudly:test" + database_host = "db.example.com" + database_name = "cudly" + database_username = "cudly" + database_password_secret_arn = "arn:aws:secretsmanager:us-east-1:123456789012:secret:db" + admin_email = "admin@example.com" + vpc_id = "vpc-00000000" + private_subnet_ids = ["subnet-00000001", "subnet-00000002"] + public_subnet_ids = ["subnet-00000003", "subnet-00000004"] + alb_security_group_id = "sg-00000000" +} + +run "defaults" { + command = plan + + assert { + condition = toset(keys(aws_cloudwatch_event_rule.sweep)) == toset(["analytics_collect", "finalize_revocations", "reap_stuck_purchases"]) + error_message = "Only reap_stuck_purchases, analytics_collect and finalize_revocations are default-on sweeps; process, send_notifications and cleanup are opt-in." + } + assert { + condition = length(aws_cloudwatch_event_target.sweep) == 3 && length(aws_iam_role.eventbridge_sweeps) == 1 && length(aws_iam_role_policy.eventbridge_sweeps) == 1 + error_message = "Each active sweep needs a target, and the sweeps need their invoker role and policy." + } + assert { + condition = length(aws_cloudwatch_event_rule.fire_scheduled_purchases) == 0 && length(aws_cloudwatch_event_rule.ladder_run) == 0 && length(aws_cloudwatch_event_rule.ri_exchange) == 0 + error_message = "fire, ladder_run and ri_exchange stay opt-in." + } + assert { + condition = alltrue([ + for k, r in aws_cloudwatch_event_rule.sweep : + length(r.name) <= 64 && startswith(r.name, "cudly-") && endswith(r.name, "-sweep-${local.sweep_tasks[k].rule}") + ]) + error_message = "Rule names must be <= 64 chars, keep the cudly- prefix and end with -sweep-." + } + assert { + condition = aws_cloudwatch_event_rule.sweep["reap_stuck_purchases"].schedule_expression == "rate(5 minutes)" && aws_cloudwatch_event_rule.sweep["analytics_collect"].schedule_expression == "rate(1 day)" && aws_cloudwatch_event_rule.sweep["finalize_revocations"].schedule_expression == "rate(1 hour)" + error_message = "Default schedules must match the Lambda module." + } + assert { + condition = alltrue([ + for k, t in aws_cloudwatch_event_target.sweep : + t.input == jsonencode({ containerOverrides = [{ name = "app", command = ["./cudly", "--task", k] }] }) + ]) + error_message = "Each target must run ./cudly --task in the app container." + } +} + +run "opt_in" { + command = plan + + variables { + enable_process_scheduled_purchases_schedule = true + enable_send_notifications_schedule = true + enable_cleanup_schedule = true + } + + assert { + condition = toset(keys(aws_cloudwatch_event_rule.sweep)) == toset(["analytics_collect", "cleanup", "finalize_revocations", "process_scheduled_purchases", "reap_stuck_purchases", "send_notifications"]) + error_message = "Opting in must add process, send_notifications and cleanup." + } + assert { + condition = length(aws_cloudwatch_event_target.sweep) == 6 && length(aws_iam_role.eventbridge_sweeps) == 1 + error_message = "Every opted-in sweep needs a target; all share one invoker role." + } + assert { + condition = length(distinct([for r in aws_cloudwatch_event_rule.sweep : r.name])) == 6 + error_message = "Sweep rule names must be distinct." + } + assert { + condition = alltrue([ + for k, r in aws_cloudwatch_event_rule.sweep : + length(r.name) <= 64 && r.schedule_expression == local.sweep_tasks[k].schedule + ]) + error_message = "Rule names must be <= 64 chars and schedules must come from the variables." + } + assert { + condition = alltrue([ + for k, t in aws_cloudwatch_event_target.sweep : + t.input == jsonencode({ containerOverrides = [{ name = "app", command = ["./cudly", "--task", k] }] }) + ]) + error_message = "Each target must run ./cudly --task in the app container." + } +} + +run "all_off" { + command = plan + + variables { + enable_reap_stuck_purchases_schedule = false + enable_analytics_collect_schedule = false + enable_finalize_revocations_schedule = false + } + + assert { + condition = length(aws_cloudwatch_event_rule.sweep) == 0 && length(aws_iam_role.eventbridge_sweeps) == 0 && length(aws_iam_role_policy.eventbridge_sweeps) == 0 + error_message = "With every sweep off, no rule and no invoker role may be created." + } +} diff --git a/terraform/modules/compute/aws/fargate/variables.tf b/terraform/modules/compute/aws/fargate/variables.tf index be66b35de..3d89dc4d5 100644 --- a/terraform/modules/compute/aws/fargate/variables.tf +++ b/terraform/modules/compute/aws/fargate/variables.tf @@ -237,6 +237,80 @@ variable "fire_scheduled_purchases_schedule" { default = "rate(15 minutes)" } +# Sweeps in sweeps.tf. Names, defaults and opt-in policy match the Lambda module. + +variable "enable_reap_stuck_purchases_schedule" { + description = "Enable scheduled sweep of purchase_executions stuck in approved/running. Issue #678 backstop for executor crashes." + type = bool + default = true +} + +variable "reap_stuck_purchases_schedule" { + description = "EventBridge schedule for the stuck-purchase reaper. Should be more frequent than PURCHASE_APPROVED_REAP_AFTER (default 10m) so a stuck row is reaped within ~1 threshold-window." + type = string + default = "rate(5 minutes)" +} + +variable "enable_analytics_collect_schedule" { + description = "Enable the scheduled savings-snapshot analytics collection (issues #1023/#1033): snapshot, partition maintenance, retention and view refresh." + type = bool + default = true +} + +variable "analytics_collect_schedule" { + description = "EventBridge schedule for the analytics_collect task. rate() starts from deployment time; use cron() for fixed clock times." + type = string + default = "rate(1 day)" +} + +variable "enable_process_scheduled_purchases_schedule" { + description = "Enable the scheduled process_scheduled_purchases sweep (proposes purchases that are due). Opt-in: default false so no purchase proposal runs unless an operator asks for it." + type = bool + default = false +} + +variable "process_scheduled_purchases_schedule" { + description = "EventBridge schedule for the process_scheduled_purchases sweep." + type = string + default = "rate(1 hour)" +} + +variable "enable_send_notifications_schedule" { + description = "Enable the scheduled send_notifications sweep. Opt-in: default false; enable it together with process_scheduled_purchases." + type = bool + default = false +} + +variable "send_notifications_schedule" { + description = "EventBridge schedule for the send_notifications sweep." + type = string + default = "rate(1 day)" +} + +variable "enable_finalize_revocations_schedule" { + description = "Enable the scheduled finalize_revocations sweep (idempotent audit repair of revoked purchases, plus the RI Marketplace listing poll, which has no schedule of its own: disabling this also stops recording listing sales and proceeds)." + type = bool + default = true +} + +variable "finalize_revocations_schedule" { + description = "EventBridge schedule for the finalize_revocations sweep." + type = string + default = "rate(1 hour)" +} + +variable "enable_cleanup_schedule" { + description = "Enable the scheduled server cleanup sweep. Opt-in: default false. It deletes expired sessions and, via CleanupOldExecutions(30), purchase_executions that are completed with scheduled_date older than 30 days, canceled with updated_at older than 30 days, or in a status outside failed/canceled with expires_at older than 30 days. The first run deletes the whole backlog, so export any history you want to keep first." + type = bool + default = false +} + +variable "cleanup_schedule" { + description = "EventBridge schedule for the cleanup sweep." + type = string + default = "rate(1 day)" +} + variable "task_timeout" { description = "Timeout in seconds for one-off scheduled tasks" type = number diff --git a/terraform/modules/compute/scheduled_task_coverage_test.go b/terraform/modules/compute/scheduled_task_coverage_test.go index ce30b57dc..8fb0e6f4f 100644 --- a/terraform/modules/compute/scheduled_task_coverage_test.go +++ b/terraform/modules/compute/scheduled_task_coverage_test.go @@ -47,11 +47,9 @@ var ( // knownGaps documents what is not wired yet, per platform and dimension. Each // PR that wires a task removes it here; the test fails when actual != documented. -// aws/cleanup-lambda and gcp/cleanup-function run cleanup outside the server -// task and are not scanned, so cleanup shows as a gap on AWS and GCP. +// gcp/cleanup-function runs cleanup outside the server task and is not +// scanned, so cleanup shows as a gap on GCP. var knownGaps = map[string]map[string][]string{ - "aws-fargate": {dimTask: {"analytics_collect", "cleanup", "finalize_revocations", - "process_scheduled_purchases", "reap_stuck_purchases", "send_notifications"}}, "gcp-cloud-run": {dimTask: sevenGCP}, "gcp-gke": {dimTask: sevenGCP}, "azure-container-apps": {dimTask: {"analytics_collect", "finalize_revocations", "fire_scheduled_purchases", @@ -59,16 +57,17 @@ var knownGaps = map[string]map[string][]string{ } // aws platforms are also checked against terraform/environments/aws: envBlock -// is the module block in compute.tf, needsPermission whether sweeps.tf must -// hold an aws_lambda_permission (Lambda only). cronJob platforms schedule -// their sweep_tasks through a Kubernetes CronJob in scheduled-tasks.tf. +// is the module block in compute.tf, lambda whether sweeps.tf targets a +// Lambda (action key plus aws_lambda_permission) rather than ECS RunTask. +// cronJob platforms schedule their sweep_tasks through a Kubernetes CronJob +// in scheduled-tasks.tf. type platform struct { - name, dir, envBlock string - aws, needsPermission, cronJob bool + name, dir, envBlock string + aws, lambda, cronJob bool } var platforms = []platform{ - {name: "aws-lambda", dir: "aws/lambda", aws: true, envBlock: "compute_lambda", needsPermission: true}, + {name: "aws-lambda", dir: "aws/lambda", aws: true, envBlock: "compute_lambda", lambda: true}, {name: "aws-fargate", dir: "aws/fargate", aws: true, envBlock: "compute_fargate"}, {name: "cloudformation", dir: "../../../cloudformation/stacks/CUDly"}, {name: "gcp-cloud-run", dir: "gcp/cloud-run"}, @@ -322,7 +321,7 @@ func TestRequiredTasksWiredPerPlatform(t *testing.T) { t.Errorf("%s: %s (missing now: %v)", p.name, prob, missing) } if p.aws { - sweeps, err := sweepProblems(p.dir, p.needsPermission) + sweeps, err := sweepProblems(p.dir, p.lambda) if err != nil { t.Errorf("%s: %v", p.name, err) } diff --git a/terraform/modules/compute/scheduled_task_env_test.go b/terraform/modules/compute/scheduled_task_env_test.go index 1836272fb..dfc415382 100644 --- a/terraform/modules/compute/scheduled_task_env_test.go +++ b/terraform/modules/compute/scheduled_task_env_test.go @@ -54,6 +54,26 @@ func envDefaultProblem(task, def string) string { return "" } +// moduleDefaultProblem checks the module's own enable default against the +// opt-in policy, so the module cannot switch an opt-in task on for a caller +// that does not pass the variable. An undeclared variable is reported by the +// declaration check instead. +func moduleDefaultProblem(modVars, task string) string { + enable := taskVars[task].enable + body, ok := blockOnce(modVars, `variable\s+"`+enable+`"\s*\{`) + if !ok { + return "" + } + m := defaultRe.FindStringSubmatch(body) + if m == nil { + return "module variable " + enable + " has no default" + } + if prob := envDefaultProblem(task, m[1]); prob != "" { + return "module variables.tf: " + prob + } + return "" +} + // blockOnce returns the body of the single block matching header; zero or // several matches report not-found so a duplicate cannot be silently ignored. func blockOnce(src, header string) (string, bool) { @@ -83,6 +103,9 @@ func envProblems(modVars, envVars, compute, blockName string, wired map[string]b continue } tv := taskVars[task] + if prob := moduleDefaultProblem(modVars, task); prob != "" { + problems = append(problems, prob) + } for _, v := range []string{tv.enable, tv.schedule} { if !strings.Contains(modVars, `variable "`+v+`"`) { problems = append(problems, "module variables.tf does not declare "+v) @@ -124,9 +147,11 @@ func tfvarsProblems(name, src string) []string { } // sweepProblems cross-checks dir/sweeps.tf: each sweep target must point at -// the rule with the same key, and (Lambda) so must its invoke permission. An +// the rule with the same key and name the task by its key. A Lambda target +// sends `action = each.key` and needs an invoke permission on the same rule; +// a Fargate (ECS) target runs `--task each.key` under an invoker role. An // absent file is fine; an existing one must have the blocks. -func sweepProblems(dir string, needPermission bool) ([]string, error) { +func sweepProblems(dir string, lambda bool) ([]string, error) { raw, err := os.ReadFile(filepath.Join(dir, "sweeps.tf")) if os.IsNotExist(err) { return nil, nil @@ -136,7 +161,13 @@ func sweepProblems(dir string, needPermission bool) ([]string, error) { src := stripComments(string(raw)) var problems []string forEach := regexp.MustCompile(`(?m)^\s*for_each\s*=\s*local\.active_sweeps\s*$`) - action := regexp.MustCompile(`action\s*=\s*each\.key\b`) + taskRefs := []*regexp.Regexp{regexp.MustCompile(`action\s*=\s*each\.key\b`)} + if !lambda { + taskRefs = []*regexp.Regexp{ + regexp.MustCompile(`"--task",\s*each\.key\b`), + regexp.MustCompile(`(?m)^\s*role_arn\s*=\s*aws_iam_role\.\w+\[0\]\.arn\s*$`), + } + } check := func(typ, attr, ref string, extra ...*regexp.Regexp) { body, ok := blockOnce(src, `resource\s+"`+typ+`"\s+"sweep"\s*\{`) if !ok { @@ -160,8 +191,8 @@ func sweepProblems(dir string, needPermission bool) ([]string, error) { } } check("aws_cloudwatch_event_rule", "", "") - check("aws_cloudwatch_event_target", "rule", "name", action) - if needPermission { + check("aws_cloudwatch_event_target", "rule", "name", taskRefs...) + if lambda { check("aws_lambda_permission", "source_arn", "arn") } return problems, nil @@ -225,7 +256,7 @@ func TestNoTfvarsDisablesDefaultOnTask(t *testing.T) { // text that satisfy envProblems for one task with the given enable default. func envFixture(task, def string) (mod, env, compute string) { tv := taskVars[task] - mod = fmt.Sprintf("variable %q {}\nvariable %q {}\n", tv.enable, tv.schedule) + mod = fmt.Sprintf("variable %q {\n default = %s\n}\nvariable %q {}\n", tv.enable, def, tv.schedule) env = fmt.Sprintf("variable %q {\n default = %s\n}\nvariable %q {\n default = \"x\"\n}\n", tv.enable, def, tv.schedule) compute = fmt.Sprintf("module \"m\" {\n %[1]s = var.%[1]s\n %[2]s = var.%[2]s\n}\n", tv.enable, tv.schedule) return mod, env, compute @@ -245,6 +276,12 @@ func TestEnvChecksNegatives(t *testing.T) { "opt-in task opt-in": {fire, "false", same, false, ""}, "opt-in task defaulting true": {fire, "true", same, true, ""}, "default-on task defaulting off": {cr, "false", same, true, ""}, + "opt-in task on in the module only": {fire, "false", func(m, e, c string) (string, string, string) { + return strings.Replace(m, "default = false", "default = true", 1), e, c + }, true, ""}, + "module enable var without a default": {cr, "true", func(m, e, c string) (string, string, string) { + return strings.Replace(m, " default = true\n", "", 1), e, c + }, true, ""}, "pass-through not name-identical": {cr, "true", func(m, e, c string) (string, string, string) { return m, e, strings.Replace(c, "= var.recommendation_schedule", "= var.other", 1) }, true, ""}, @@ -308,6 +345,7 @@ func TestSweepCrossWiring(t *testing.T) { const rule = "resource \"aws_cloudwatch_event_rule\" \"sweep\" {\n for_each = local.active_sweeps\n}\n" const target = "resource \"aws_cloudwatch_event_target\" \"sweep\" {\n for_each = local.active_sweeps\n rule%s= aws_cloudwatch_event_rule.sweep[%s].name\n input = jsonencode({ action = each.key })\n}\n" const perm = "resource \"aws_lambda_permission\" \"sweep\" {\n for_each = local.active_sweeps\n source_arn%s= aws_cloudwatch_event_rule.sweep[%s].arn\n}\n" + const ecsTarget = "resource \"aws_cloudwatch_event_target\" \"sweep\" {\n for_each = local.active_sweeps\n rule = aws_cloudwatch_event_rule.sweep[each.key].name\n role_arn = aws_iam_role.eventbridge_sweeps[0].arn\n input = jsonencode({ containerOverrides = [{ command = [\"./cudly\", \"--task\", each.key] }] })\n}\n" good, goodPerm := fmt.Sprintf(target, " ", "each.key"), fmt.Sprintf(perm, " ", "each.key") for name, c := range map[string]struct { src string @@ -316,7 +354,11 @@ func TestSweepCrossWiring(t *testing.T) { }{ "aligned": {rule + fmt.Sprintf(target, " ", "each.key") + fmt.Sprintf(perm, " ", "each.key"), true, false}, "unaligned": {rule + good + goodPerm, true, false}, - "fargate needs no perm": {rule + good, false, false}, + "fargate needs no perm": {rule + ecsTarget, false, false}, + "fargate task not key": {rule + strings.Replace(ecsTarget, `"--task", each.key`, `"--task", "cleanup"`, 1), false, true}, + "fargate no invoker role": {rule + strings.Replace(ecsTarget, "role_arn", "# role_arn", 1), false, true}, + "fargate lambda target": {rule + good, false, true}, + "lambda ecs target": {rule + ecsTarget + goodPerm, true, true}, "wrong key": {rule + fmt.Sprintf(target, " ", "each.value.rule") + goodPerm, true, true}, "wrong resource type": {rule + strings.Replace(good, "event_rule", "event_target", 1) + goodPerm, true, true}, "missing permission": {rule + good, true, true},