fix(pool): ensure pool top up respects var.ami_id_ssm_parameter_name (#3040)

jpalomaki · web-flow · commit c4ab2428c514 · 2023-03-10T18:47:21.000+01:00
* fix(pool): ensure pool topup respects var.ami_id_ssm_parameter_name

* Add missing var

* Fix pool lambda IAM policy

* Fix var passing
diff --git a/modules/runners/README.md b/modules/runners/README.md
@@ -79,10 +79,10 @@ yarn run dist
 | [aws_cloudwatch_log_group.scale_down](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_log_group) | resource |
 | [aws_cloudwatch_log_group.scale_up](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_log_group) | resource |
 | [aws_iam_instance_profile.runner](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_instance_profile) | resource |
+| [aws_iam_policy.ami_id_ssm_parameter_read](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource |
 | [aws_iam_role.runner](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role) | resource |
 | [aws_iam_role.scale_down](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role) | resource |
 | [aws_iam_role.scale_up](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role) | resource |
-| [aws_iam_role_policy.ami_id_ssm_parameter_read](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) | resource |
 | [aws_iam_role_policy.cloudwatch](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) | resource |
 | [aws_iam_role_policy.describe_tags](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) | resource |
 | [aws_iam_role_policy.dist_bucket](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) | resource |
@@ -96,6 +96,7 @@ yarn run dist
 | [aws_iam_role_policy.scale_up_logging](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) | resource |
 | [aws_iam_role_policy.service_linked_role](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) | resource |
 | [aws_iam_role_policy.ssm_parameters](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) | resource |
+| [aws_iam_role_policy_attachment.ami_id_ssm_parameter_read](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
 | [aws_iam_role_policy_attachment.managed_policies](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
 | [aws_iam_role_policy_attachment.scale_down_vpc_execution_role](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
 | [aws_iam_role_policy_attachment.scale_up_vpc_execution_role](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
diff --git a/modules/runners/lambdas/runners/src/pool/pool.ts b/modules/runners/lambdas/runners/src/pool/pool.ts
@@ -32,6 +32,7 @@ export async function adjust(event: PoolEvent): Promise<void> {
   const instanceMaxSpotPrice = process.env.INSTANCE_MAX_SPOT_PRICE;
   const instanceAllocationStrategy = process.env.INSTANCE_ALLOCATION_STRATEGY || 'lowest-price'; // same as AWS default
   const runnerOwner = process.env.RUNNER_OWNER;
+  const amiIdSsmParameterName = process.env.AMI_ID_SSM_PARAMETER_NAME;
 
   let ghesApiUrl = '';
   if (ghesBaseUrl) {
@@ -109,6 +110,7 @@ export async function adjust(event: PoolEvent): Promise<void> {
         ssmTokenPath,
         subnets,
         numberOfRunners: topUp,
+        amiIdSsmParameterName,
       },
       githubInstallationClient,
     );
diff --git a/modules/runners/policies-lambda-common.tf b/modules/runners/policies-lambda-common.tf
@@ -8,3 +8,26 @@ data "aws_iam_policy_document" "lambda_assume_role_policy" {
     }
   }
 }
+
+resource "aws_iam_policy" "ami_id_ssm_parameter_read" {
+  count       = var.ami_id_ssm_parameter_name != null ? 1 : 0
+  name        = "${var.prefix}-ami-id-ssm-parameter-read"
+  path        = local.role_path
+  description = "Allows for reading ${var.prefix} GitHub runner AMI ID from an SSM parameter"
+  policy      = <<-JSON
+    {
+      "Version": "2012-10-17",
+      "Statement": [
+        {
+          "Effect": "Allow",
+          "Action": [
+            "ssm:GetParameter"
+          ],
+          "Resource": [
+            "arn:${var.aws_partition}:ssm:${var.aws_region}:${data.aws_caller_identity.current.account_id}:parameter/${trimprefix(var.ami_id_ssm_parameter_name, "/")}"
+          ]
+        }
+      ]
+    }
+  JSON
+}
diff --git a/modules/runners/pool.tf b/modules/runners/pool.tf
@@ -45,9 +45,11 @@ module "pool" {
       pool_owner                = var.pool_runner_owner
       role                      = aws_iam_role.runner
     }
-    subnet_ids     = var.subnet_ids
-    ssm_token_path = "${var.ssm_paths.root}/${var.ssm_paths.tokens}"
-    tags           = local.tags
+    subnet_ids                           = var.subnet_ids
+    ssm_token_path                       = "${var.ssm_paths.root}/${var.ssm_paths.tokens}"
+    ami_id_ssm_parameter_name            = var.ami_id_ssm_parameter_name
+    ami_id_ssm_parameter_read_policy_arn = var.ami_id_ssm_parameter_name != null ? aws_iam_policy.ami_id_ssm_parameter_read[0].arn : null
+    tags                                 = local.tags
   }
 
   aws_partition = var.aws_partition
diff --git a/modules/runners/pool/main.tf b/modules/runners/pool/main.tf
@@ -37,6 +37,7 @@ resource "aws_lambda_function" "pool" {
       RUNNER_OWNER                         = var.config.runner.pool_owner
       SSM_TOKEN_PATH                       = var.config.ssm_token_path
       SUBNET_IDS                           = join(",", var.config.subnet_ids)
+      AMI_ID_SSM_PARAMETER_NAME            = var.config.ami_id_ssm_parameter_name
     }
   }
 
@@ -138,3 +139,9 @@ resource "aws_lambda_permission" "pool" {
   principal     = "events.amazonaws.com"
   source_arn    = aws_cloudwatch_event_rule.pool[count.index].arn
 }
+
+resource "aws_iam_role_policy_attachment" "ami_id_ssm_parameter_read" {
+  count      = var.config.ami_id_ssm_parameter_name != null ? 1 : 0
+  role       = aws_iam_role.pool.name
+  policy_arn = var.config.ami_id_ssm_parameter_read_policy_arn
+}
diff --git a/modules/runners/pool/variables.tf b/modules/runners/pool/variables.tf
@@ -49,11 +49,13 @@ variable "config" {
       schedule_expression = string
       size                = number
     }))
-    role_permissions_boundary = string
-    kms_key_arn               = string
-    ami_kms_key_arn           = string
-    role_path                 = string
-    ssm_token_path            = string
+    role_permissions_boundary            = string
+    kms_key_arn                          = string
+    ami_kms_key_arn                      = string
+    role_path                            = string
+    ssm_token_path                       = string
+    ami_id_ssm_parameter_name            = string
+    ami_id_ssm_parameter_read_policy_arn = string
   })
 }
 
diff --git a/modules/runners/scale-up.tf b/modules/runners/scale-up.tf
@@ -122,24 +122,8 @@ resource "aws_iam_role_policy_attachment" "scale_up_vpc_execution_role" {
   policy_arn = "arn:${var.aws_partition}:iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole"
 }
 
-resource "aws_iam_role_policy" "ami_id_ssm_parameter_read" {
-  count  = var.ami_id_ssm_parameter_name != null ? 1 : 0
-  name   = "${var.prefix}-ami-id-ssm-parameter-read"
-  role   = aws_iam_role.scale_up.name
-  policy = <<-JSON
-    {
-      "Version": "2012-10-17",
-      "Statement": [
-        {
-          "Effect": "Allow",
-          "Action": [
-            "ssm:GetParameter"
-          ],
-          "Resource": [
-            "arn:${var.aws_partition}:ssm:${var.aws_region}:${data.aws_caller_identity.current.account_id}:parameter/${trimprefix(var.ami_id_ssm_parameter_name, "/")}"
-          ]
-        }
-      ]
-    }
-  JSON
+resource "aws_iam_role_policy_attachment" "ami_id_ssm_parameter_read" {
+  count      = var.ami_id_ssm_parameter_name != null ? 1 : 0
+  role       = aws_iam_role.scale_up.name
+  policy_arn = aws_iam_policy.ami_id_ssm_parameter_read[0].arn
 }

Original file line number	Diff line number	Diff line change
`@@ -45,9 +45,11 @@ module "pool" {`
`45`	`45`	`pool_owner = var.pool_runner_owner`
`46`	`46`	`role = aws_iam_role.runner`
`47`	`47`	`}`
`48`		`- subnet_ids = var.subnet_ids`
`49`		`- ssm_token_path = "${var.ssm_paths.root}/${var.ssm_paths.tokens}"`
`50`		`- tags = local.tags`
	`48`	`+ subnet_ids = var.subnet_ids`
	`49`	`+ ssm_token_path = "${var.ssm_paths.root}/${var.ssm_paths.tokens}"`
	`50`	`+ ami_id_ssm_parameter_name = var.ami_id_ssm_parameter_name`
	`51`	`+ ami_id_ssm_parameter_read_policy_arn = var.ami_id_ssm_parameter_name != null ? aws_iam_policy.ami_id_ssm_parameter_read[0].arn : null`
	`52`	`+ tags = local.tags`
`51`	`53`	`}`
`52`	`54`
`53`	`55`	`aws_partition = var.aws_partition`
Original file line number	Diff line number	Diff line change
`@@ -37,6 +37,7 @@ resource "aws_lambda_function" "pool" {`
`37`	`37`	`RUNNER_OWNER = var.config.runner.pool_owner`
`38`	`38`	`SSM_TOKEN_PATH = var.config.ssm_token_path`
`39`	`39`	`SUBNET_IDS = join(",", var.config.subnet_ids)`
	`40`	`+ AMI_ID_SSM_PARAMETER_NAME = var.config.ami_id_ssm_parameter_name`
`40`	`41`	`}`
`41`	`42`	`}`
`42`	`43`
`@@ -138,3 +139,9 @@ resource "aws_lambda_permission" "pool" {`
`138`	`139`	`principal = "events.amazonaws.com"`
`139`	`140`	`source_arn = aws_cloudwatch_event_rule.pool[count.index].arn`
`140`	`141`	`}`
	`142`	`+`
	`143`	`+resource "aws_iam_role_policy_attachment" "ami_id_ssm_parameter_read" {`
	`144`	`+ count = var.config.ami_id_ssm_parameter_name != null ? 1 : 0`
	`145`	`+ role = aws_iam_role.pool.name`
	`146`	`+ policy_arn = var.config.ami_id_ssm_parameter_read_policy_arn`
	`147`	`+}`