feat: add stateless registration cleanup with multi-App support - #5465
guicaulada wants to merge 2 commits into
Conversation
Dependency ReviewThe following issues were found:
License Issueslambdas/functions/termination-watcher/package.json
OpenSSF Scorecard
Scanned Files
|
|
@guicaulada how does this compare to the recent changes to the termination watcher? #5055 |
|
@guicaulada I think we should open the scope. Create an interface and make the compute provider the responsible for fetch the instances. So the janitor can work with any provider |
guicaulada
changed the title
63a47e3 to
8b038db
Compare
guicaulada
changed the title
|
Accidentally closed, I wasn't aware of the recent changes on the termination watcher, will check if this is still needed and see what are the differences. I opened a few clean-up PRs today with lambda changes that we had implemented locally after we noticed lots of orphaned resources. |
8b038db to
4116b1e
Compare
guicaulada
changed the title
4116b1e to
fd284e3
Compare
|
Thanks both for pointing these out! @Brend-Smits I checked #5055: the termination watcher handles event-driven deregistration and busy-runner retries, while this janitor discovers registrations left behind by missed events, earlier failures, or instances removed before deregistration was enabled. It complements that cleanup. @edersonbrilhante I’ve also moved runner-name interpretation and backing-resource checks behind a compute-provider interface. EC2 is the first implementation; additional providers can supply their own lookup without changing GitHub discovery or confirmation. The Terraform module currently configures EC2. Checks happen per candidate, and failed lookups never establish absence. Cleanup remains stateless and incremental, with a delayed second observation before deletion. Tests also cover a non-EC2 provider through discovery and confirmation. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A critical Lambda permission defect and unresolved discovery, deadline, client-reuse, documentation, and CI issues must be addressed.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds an opt-in scheduled Lambda and Terraform module for stateless cleanup of orphaned GitHub runner registrations across EC2 regions.
Changes:
- Adds scheduled discovery, delayed SQS confirmation, dry-run support, IAM, and documentation.
- Introduces pluggable compute-provider cleanup with EC2 support.
- Adds multi-App authentication, quota handling, fallback, and tests.
| File | Description |
|---|---|
modules/registration-janitor/versions.tf |
Terraform and AWS provider requirements. |
modules/registration-janitor/variables.tf |
Janitor configuration and validation. |
modules/registration-janitor/tests/registration-janitor.tftest.hcl |
Terraform deployment and IAM tests. |
modules/registration-janitor/README.md |
Module usage and behavior documentation. |
modules/registration-janitor/outputs.tf |
Lambda and dead-letter queue outputs. |
modules/registration-janitor/main.tf |
Lambda, queues, scheduling, and IAM resources. |
lambdas/yarn.lock |
Workspace dependency lock updates. |
lambdas/libs/compute-providers/registration-cleanup.ts |
Cleanup-provider interface and factory. |
lambdas/libs/compute-providers/registration-cleanup.test.ts |
Provider contract tests. |
lambdas/libs/compute-providers/providers.config.registration-cleanup.ts |
Bundled provider registration. |
lambdas/libs/compute-providers/package.json |
Cleanup-provider API export. |
lambdas/libs/compute-providers/aws/ec2/registration-cleanup.ts |
EC2 inventory checks. |
lambdas/functions/termination-watcher/src/registration-janitor.ts |
Discovery, confirmation, and deletion workflow. |
lambdas/functions/termination-watcher/src/registration-janitor.test.ts |
Janitor behavior and safety tests. |
lambdas/functions/termination-watcher/src/lambda.ts |
Janitor handler export. |
lambdas/functions/termination-watcher/src/github-app-client.ts |
Multi-App authentication and quota selection. |
lambdas/functions/termination-watcher/src/github-app-client.test.ts |
Authentication and fallback tests. |
lambdas/functions/termination-watcher/package.json |
Compute-provider dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| resource "aws_lambda_permission" "schedule" { | ||
| action = "lambda:InvokeFunction" | ||
| function_name = module.lambda.lambda.function.function_name | ||
| principal = "events.amazonaws.com" | ||
| source_arn = aws_cloudwatch_event_rule.schedule.arn | ||
| } |
There was a problem hiding this comment.
I checked this against the AWS provider documentation: statement_id is optional and Terraform generates an identifier when it is omitted, so its absence does not prevent creation or EventBridge invocation. Leaving this unchanged. See the provider argument reference.
| if (members.some((runner) => runner.id === candidate.runnerId)) break; | ||
| if (members.length < 100) return; | ||
| } | ||
| if (await provider.exists(candidate.resourceId)) return; |
There was a problem hiding this comment.
Fixed in 7246641. Confirmation now checks the remaining time before authentication for each SQS record, each membership page, the provider lookup, the GitHub runner recheck, and deletion. When the guard stops work, the candidate and subsequent unprocessed records are returned as batch failures for retry. Added four regression cases covering those boundaries; all 100 termination-watcher tests pass. This prevents starting a new operation inside the ten-second reserve; it does not cancel requests already in flight.
|
|
||
| Use a nonempty `runner_name_prefix` reserved exclusively for this AWS account in the configured organization and runner groups. Names must equal that prefix followed by an EC2 instance ID. Include **every AWS region** where that prefix is used. Do not use a prefix shared with another AWS account: absence in this account cannot establish that another account's instance is gone. EC2 tags alone cannot prove ownership after an instance disappears. | ||
|
|
||
| Start with the default `dry_run = true` and inspect the candidate logs. Set it to `false` only after verifying the scope. A GitHub App needs organization self-hosted runner read/write permissions. The Lambda uses the termination watcher's existing single-App authentication, including GHES support; it does not implement a second App failover mechanism. |
There was a problem hiding this comment.
Fixed in 7246641. Removed the outdated single-App-only guidance. The rollout section now explicitly describes optional additional Apps through the manifest, quota-based selection, authentication fallback, and GHES support, consistent with the configuration and implementation.
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = concat([ | ||
| { | ||
| Effect = "Allow" | ||
| Action = ["ec2:DescribeInstances"] | ||
| Resource = "*" | ||
| Condition = { | ||
| StringEquals = { "aws:RequestedRegion" = var.config.regions } | ||
| } | ||
| }, | ||
| { | ||
| Effect = "Allow" | ||
| Action = concat(["ssm:GetParameters"], var.config.github_app_parameters.additional_apps_manifest == null ? [] : ["ssm:GetParameter"]) | ||
| Resource = concat([var.config.github_app_parameters.id.arn, var.config.github_app_parameters.key_base64.arn], var.config.github_app_parameters.additional_apps_manifest == null ? [] : [var.config.github_app_parameters.additional_apps_manifest.arn], var.config.github_app_parameters.additional_app_parameter_arns) | ||
| }, | ||
| { | ||
| Effect = "Allow" | ||
| Action = ["sqs:SendMessage", "sqs:ReceiveMessage", "sqs:DeleteMessage", "sqs:GetQueueAttributes"] | ||
| Resource = aws_sqs_queue.confirmation.arn | ||
| } | ||
| ], var.config.github_app_kms_key_arn == null ? [] : [{ | ||
| Effect = "Allow" | ||
| Action = ["kms:Decrypt"] | ||
| Resource = var.config.github_app_kms_key_arn | ||
| }]) | ||
| }) | ||
| } |
There was a problem hiding this comment.
What do you think using data.iam_policy_document instead? 1 thing to think about is how to decouple the ec2 permission and how to support multiple computer provider at the same time.
1 janitor per compute provider or 1 janitor for all computer providers?
There was a problem hiding this comment.
Agreed on aws_iam_policy_document; changed both policies in 7246641. EC2 configuration and its region-restricted read-only inventory permissions now live in compute-ec2.tf, attached separately from the shared credential/SQS cleanup policy.
I went with one janitor deployment per compute provider and ownership scope. Separate deployments can run concurrently with distinct resource prefixes and disjoint runner-name ownership, keeping permissions, confirmation queues, and execution budgets isolated. The runtime provider interface keeps the GitHub cleanup flow shared. EC2 remains the only bundled implementation; adding another provider also requires its Terraform configuration and permissions. This boundary is now documented.
All four mocked Terraform tests pass, including provider-policy separation, region restrictions, optional KMS permissions, and additional-App credential access.
fd284e3 to
7246641
Compare
Missed deregistration events can leave offline GitHub runners behind after their compute resources disappear. The termination watcher handles termination events and busy-runner retries, but cannot discover historical registrations or missed events. Scale-down also cannot discover this backlog because it starts from existing compute instances. This scheduled reconciliation complements both paths.
Add an opt-in
modules/registration-janitormodule deploying a separate scheduled Lambda from the existing termination-watcher archive with GitHub App/GHES authentication. Discovery processes each runner-group page immediately, verifies candidates individually, and queues work before fetching later pages. Candidate errors are isolated; unavailable pages are skipped, with advancement to another group after three consecutive page failures. Retained instances and failed checks do not consume the queued-candidate limit.Scanning is stateless. Every invocation starts from current GitHub inventory; registrations deleted by earlier cleanup are no longer listed. Pagination is local to each invocation. A deadline guard stops starting new work with ten seconds remaining. Busy, retained, or failed registrations may be checked again, and subsequent scans reconcile shifting page boundaries.
Scope is an organization, explicit runner groups, an exact account-exclusive runner-name prefix, and every AWS region using that prefix. Any non-terminated EC2 state protects the registration. Offline, non-busy registrations with absent instances enter SQS for a second observation at least 15 minutes later. These messages carry candidate identity and observation time for deletion safety. Confirmation searches group membership page by page, then rechecks EC2 absence and current GitHub identity/offline/busy state before deletion. Incomplete or failed lookups never establish absence. Confirmation failures retry independently through SQS and its dead-letter queue; duplicate deletion treats 404 as success. Scope changes invalidate candidates, and candidates expire after 24 hours. Dry-run is the default and suppresses queueing and deletion.
The account-exclusive prefix and complete region list are required configuration because tags cannot prove ownership after an instance disappears. This implementation handles organization EC2 runners in one account and multiple regions. Repository runners and cross-account inventory remain outside its scope. Delayed observations reduce races but are not an atomic GitHub/EC2 transaction.
Additional GitHub Apps are supported through the existing credential manifest format. Discovery selects an App for each group page, and confirmation selects one per candidate. Selection prefers observed remaining quota, skips throttled Apps for one minute when another is available, and tries another configured App after authentication failure. App JWT and installation-token authentication use the same credential and resolve access for the requested owner. Terraform passes the optional manifest and grants read access to its configured credential parameters. Single-App configurations remain supported. Existing termination-watcher behavior is unchanged by this PR.
The GitHub cleanup flow uses a compute-provider interface for runner-name interpretation, backing-resource lookup, and inventory scope. EC2 is the first bundled provider; it performs per-candidate lookups across the configured regions. Providers must throw on incomplete or failed lookups rather than report absence. Provider type and normalized scope are included in candidate validation. Additional providers can be registered without changing GitHub discovery or confirmation; this Terraform module currently supplies EC2 configuration and permissions.
Validation: 100 termination-watcher package tests, 315 compute-provider tests, and four mocked Terraform tests pass. A non-EC2 test provider exercises discovery and confirmation without AWS inventory calls. Regressions cover fresh discovery after earlier deletion, queueing before later-page failure, independent failures, candidate-budget fairness, paged membership verification, deadline handling, paired App authentication, quota selection, authentication fallback, and manifest/IAM wiring. Runtime TypeScript, ESLint, Prettier, Terraform validation/formatting, generated documentation, Lambda bundle build, and diff checks pass. Optional package-wide coverage thresholds already fail in existing deregistration/retry code on unchanged main; no thresholds were lowered. TFLint is unavailable locally. No AWS deployment or live registration deletion was performed.
Independent companion PRs: #5463 (incremental scale-down) and #5464 (incremental SSM cleanup).
Compute-provider configuration and IAM are separated from shared cleanup permissions. Deployment uses one janitor per provider and ownership scope. Confirmation checks its time budget before authentication, membership pages, resource lookup, GitHub recheck, and deletion; unfinished records are retried independently.