You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR removes some features and modules not required for our infra use case and makes use of more granular modules.
Scale up and down based on GitHub events
Scale down to zero when no jobs are running
Runners are created on-demand and terminated after use (ephemeral runners)
Runners are created on spot instances
use custom AMI, define the instance types and subnets to use.
OS support: Linux (x64/arm64) and Windows
CI workflow for deployment
Type
enhancement
Description
Added comprehensive variables for AWS configuration, GitHub app parameters, runner settings, and lambda functions.
Configured AWS resources including SQS queues, SSM parameters, webhook module, runners module, and logging.
Setup AWS launch template, security groups, IAM roles, and policies for runners.
Introduced configuration for runner binaries syncer lambda function and trigger.
Defined lambda function for managing runner pools with IAM roles, policies, and cloudwatch event rules.
Added variables and configuration for termination watcher lambda function.
Changes walkthrough
Relevant files
Configuration changes
variables.tf
Add Variables for Configuring AWS, GitHub App, Runners, and Lambdas
github-runners/terraform/autoscaling/variables.tf
Added a comprehensive list of variables for configuring AWS region, VPC, subnets, tags, GitHub app parameters, runner settings, and more.
Introduced variables for configuring ephemeral runners, including enabling ephemeral runners, job queued check, and managed runner security group.
Added variables for configuring lambda functions, including memory size, timeout, and S3 bucket details for lambda functions.
Included variables for AMI configuration, instance types, and runner OS support.
5, due to the extensive number of files and changes involved in this PR, including multiple Terraform modules, Packer configurations, and Lambda function implementations. The complexity and interdependencies between these components significantly increase the review effort required to ensure correctness, security, and adherence to best practices.
🧪 Relevant tests
No
🔍 Possible issues
Possible Security Concern: The PR introduces numerous AWS resources and Lambda functions. Without proper IAM policies and security group configurations, there might be potential security risks, such as overly permissive access or unintended exposure of resources.
Possible Performance Issue: The use of ephemeral runners and on-demand scaling based on GitHub events can lead to unpredictable costs and performance implications under high load or misconfiguration.
Dependency Management: The PR includes external dependencies (e.g., GitHub Actions runner binaries, Docker, AWS CLI). Changes in these dependencies could impact the stability and functionality of the setup.
🔒 Security concerns
- IAM Policies: Ensure that IAM roles and policies assigned to Lambda functions and EC2 instances follow the principle of least privilege to minimize security risks.
Public Exposure: Verify that security groups and network access controls are correctly configured to prevent unauthorized access to resources.
Consider adding lifecycle hooks to the aws_launch_template resource to manage updates and replacements more gracefully. This can help in ensuring that instances are only replaced according to the defined policies, minimizing disruptions. [important]
Implement error handling and retry mechanisms in the Lambda function responsible for syncing runner binaries. This will improve the robustness of the setup by handling transient errors that may occur during the download or update process. [important]
Optimize the Packer build by using a more specific source AMI filter to reduce build times and potential costs. Narrowing down the search to a more recent base AMI can also ensure that the runners are built on a more secure and updated OS. [medium]
Add monitoring and alerting for the Lambda function that adjusts the runner pool size. Monitoring metrics such as invocation errors, duration, and throttles can help in identifying issues early and maintaining the desired state of runner pools. [medium]
Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.
The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
Use dynamic blocks for tags to improve code maintainability.
Consider using a dynamic block for tags to improve maintainability and readability. This approach allows you to easily add or remove tags without modifying the structure of each resource definition.
Change associate_public_ip_address variable type to boolean for clarity.
For the associate_public_ip_address variable, it's more appropriate to use a boolean type instead of a string since the description suggests a true/false value. This change improves clarity and enforces correct input values.
variable "associate_public_ip_address" {
description = "If using a non-default VPC, there is no public IP address assigned to the EC2 instance. If you specified a public subnet, you probably want to set this to true. Otherwise the EC2 instance won't have access to the internet"
- type = string+ type = bool
default = null
}
Use multiple subnets for Lambda functions for higher availability.
To ensure high availability and fault tolerance, consider specifying multiple subnet IDs for the Lambda functions by using a list of subnets rather than a single subnet. This allows the Lambda functions to be deployed across multiple Availability Zones.
Add a condition to ensure s3_bucket, s3_key, and s3_object_version are only set when necessary.
To ensure that the lambda function only uses the S3 bucket when necessary, consider adding a condition to check both var.lambda_s3_bucket and var.lambda_s3_key are not null before setting s3_bucket, s3_key, and s3_object_version. This will make the configuration more robust and avoid potential misconfigurations.
To ensure the scalability and flexibility of your Terraform configuration, consider parameterizing the instance_types to allow different instance types for different environments or use cases.
Avoid hardcoding AWS region for security and flexibility.
To avoid potential security risks, consider removing the hardcoded AWS region and instead use a variable. This will also increase the flexibility of your Terraform configuration.
Use a more specific version constraint for the amazon plugin.
It's recommended to use a more specific version constraint for the amazon plugin to avoid potential compatibility issues with future versions. Instead of ">= 0.0.2", consider specifying a range that caps the major version, such as "~> 0.0.2" which allows patch updates but prevents major version changes.
amazon = {
- version = ">= 0.0.2"+ version = "~> 0.0.2"
source = "github.com/hashicorp/amazon"
}
Use the null coalescing operator for default values to improve code readability.
For better maintainability and readability, consider using Terraform's null coalescing operator instead of ternary operators for default values. This operator simplifies the expression and makes the code cleaner.
Use a local variable for webhook_secret to reduce duplication.
For better maintainability and to avoid duplication, consider using a local variable or a module output for common values such as webhook_secret instead of repeating the random_id.random.hex in multiple places.
Move variable definitions to a separate file for better maintainability.
To improve the maintainability and readability of the code, consider using a separate variables file for defining variables. This helps in separating the configuration from the template logic and makes the template cleaner.
-variable "runner_version" {- description = "The version (no v prefix) of the runner software to install https://github.com/actions/runner/releases. The latest release will be fetched from GitHub if not provided."- default = null-}+# Variables should be defined in a separate file, e.g., variables.tf
Use a variable for Terraform required version.
To improve maintainability and avoid hardcoding, consider using a Terraform variable for the required_version of Terraform in versions.tf. This allows for easier updates and version management.
Use a separate file for IAM policies and roles to improve maintainability.
To improve the maintainability of your Terraform code, consider using a separate file for IAM policies and roles. This approach makes it easier to manage and review IAM policies, especially as they grow in complexity.
+# In a separate file, e.g., iam.tf
resource "aws_iam_role" "ami_housekeeper" {
name = "${var.prefix}-ami-housekeeper-role"
assume_role_policy = data.aws_iam_policy_document.lambda_assume_role_policy.json
path = local.role_path
permissions_boundary = var.role_permissions_boundary
tags = var.tags
}
Security
Review security practices when using public IP addresses.
For better security practices, avoid using public IP addresses if not necessary. If associate_public_ip_address is set to true, ensure that the instances are secured and only necessary ports are exposed.
-associate_public_ip_address = var.associate_public_ip_address+# Ensure security measures are in place if using public IP addresses
Dynamically fetch AMI names using AWS SSM Parameter Store for enhanced security.
To ensure the EC2 instances are using the latest and most secure AMIs, consider dynamically fetching the source_ami_filter name using AWS SSM Parameter Store instead of hardcoding the AMI name pattern.
Restrict SQS queue policy actions to only necessary ones.
Consider using a more restrictive condition for the SQS queue policy to limit the actions to only what's necessary, rather than allowing all SQS actions ("sqs:*"). This follows the principle of least privilege, enhancing the security of your Terraform configuration.
Enable server-side encryption on SQS queues by default.
For better security, consider enabling server-side encryption on the SQS queues by default, unless there's a specific reason to disable it. This can be done by setting sqs_managed_sse_enabled to true in the SQS resource configurations.
Specify specific ARNs in IAM policy resources instead of using a wildcard.
It's recommended to avoid using a broad wildcard ("*") for IAM policy resources when possible. Specify the ARNs of the resources that the policy should apply to, enhancing the security by following the principle of least privilege.
Specify a more restrictive assume_role_policy for enhanced security.
To enhance security, consider specifying a more restrictive assume_role_policy for the aws_iam_role resource. The current policy allows any Lambda function to assume this role. You should restrict it to only the Lambda functions that need it.
Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.
When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DaMandal0rian
force-pushed
the
ephemeral-runners
branch
from
User description
Terraform module Self-Hosted Scalable GitHub Actions runners on AWS based on the official Philips AWS Module. https://github.com/philips-labs/terraform-aws-github-runner under MIT License.
This PR removes some features and modules not required for our infra use case and makes use of more granular modules.
Scale up and down based on GitHub events
Scale down to zero when no jobs are running
Runners are created on-demand and terminated after use (ephemeral runners)
Runners are created on spot instances
use custom AMI, define the instance types and subnets to use.
OS support: Linux (x64/arm64) and Windows
CI workflow for deployment
Type
enhancement
Description
Changes walkthrough
variables.tf
Add Variables for Configuring AWS, GitHub App, Runners, and Lambdasgithub-runners/terraform/autoscaling/variables.tf
VPC, subnets, tags, GitHub app parameters, runner settings, and more.
enabling ephemeral runners, job queued check, and managed runner
security group.
size, timeout, and S3 bucket details for lambda functions.
OS support.
main.tf
Configure AWS Resources, Runners, and Lambda Functionsgithub-runners/terraform/autoscaling/main.tf
module, and runners module.
main.tf
Setup AWS Launch Template and Security Groups for Runnersgithub-runners/terraform/autoscaling/modules/runners/main.tf
instance type, security groups, and user data.
variables.tf
Add Variables for Runner Binaries Syncer Lambda Configurationgithub-runners/terraform/autoscaling/modules/runner-binaries-syncer/variables.tf
function.
details, and logging.
runner-binaries-syncer.tf
Configure Runner Binaries Syncer Lambda and Triggergithub-runners/terraform/autoscaling/modules/runner-binaries-syncer/runner-binaries-syncer.tf
policies, and cloudwatch event rule.
deployment.
main.tf
Setup Lambda Function for Runner Pool Managementgithub-runners/terraform/autoscaling/modules/runners/pool/main.tf
on schedule.
variables.tf
Add Variables for Termination Watcher Lambda Configurationgithub-runners/terraform/autoscaling/modules/termination-watcher/variables.tf
function.
and environment variables.