Addressing npalm code review comments

Author: jgutierrezglez

README.md

@@ -207,9 +207,9 @@ Talk to the forestkeepers in the `runners-channel` on Slack.
 | <a name="input_runner_binaries_syncer_lambda_zip"></a> [runner\_binaries\_syncer\_lambda\_zip](#input\_runner\_binaries\_syncer\_lambda\_zip) | File location of the binaries sync lambda zip file. | `string` | `null` | no |
 | <a name="input_runner_boot_time_in_minutes"></a> [runner\_boot\_time\_in\_minutes](#input\_runner\_boot\_time\_in\_minutes) | The minimum time for an EC2 runner to boot and register as a runner. | `number` | `5` | no |
 | <a name="input_runner_credit_specification"></a> [runner\_credit\_specification](#input\_runner\_credit\_specification) | The credit option for CPU usage of a T instance. Can be unset, "standard" or "unlimited". | `string` | `null` | no |
+| <a name="input_runner_disable_default_labels"></a> [runner\_disable\_default\_labels](#input\_runner\_disable\_default\_labels) | Disable default labels for the runners (os, architecture and `self-hosted`). If enabled, the runner will only have the extra labels provided in `runner_extra_labels`. | `bool` | `true` | no |
 | <a name="input_runner_ec2_tags"></a> [runner\_ec2\_tags](#input\_runner\_ec2\_tags) | Map of tags that will be added to the launch template instance tag specifications. | `map(string)` | `{}` | no |
 | <a name="input_runner_egress_rules"></a> [runner\_egress\_rules](#input\_runner\_egress\_rules) | List of egress rules for the GitHub runner instances. | <pre>list(object({<br/>    cidr_blocks      = list(string)<br/>    ipv6_cidr_blocks = list(string)<br/>    prefix_list_ids  = list(string)<br/>    from_port        = number<br/>    protocol         = string<br/>    security_groups  = list(string)<br/>    self             = bool<br/>    to_port          = number<br/>    description      = string<br/>  }))</pre> | <pre>[<br/>  {<br/>    "cidr_blocks": [<br/>      "0.0.0.0/0"<br/>    ],<br/>    "description": null,<br/>    "from_port": 0,<br/>    "ipv6_cidr_blocks": [<br/>      "::/0"<br/>    ],<br/>    "prefix_list_ids": null,<br/>    "protocol": "-1",<br/>    "security_groups": null,<br/>    "self": null,<br/>    "to_port": 0<br/>  }<br/>]</pre> | no |
-| <a name="input_runner_enable_default_labels"></a> [runner\_enable\_default\_labels](#input\_runner\_enable\_default\_labels) | Enable default labels for the runners (os, architecture and `self-hosted`). If disabled, the runner will only have the extra labels provided in `runner_extra_labels`. In order set runner\_enable\_default\_labels = false, enable\_jit\_config and enable\_ephemeral\_runners must be set to true; otherwise the value of this variable will be considered as true | `bool` | `true` | no |
 | <a name="input_runner_extra_labels"></a> [runner\_extra\_labels](#input\_runner\_extra\_labels) | Extra (custom) labels for the runners (GitHub). Separate each label by a comma. Labels checks on the webhook can be enforced by setting `enable_workflow_job_labels_check`. GitHub read-only labels should not be provided. | `list(string)` | `[]` | no |
 | <a name="input_runner_group_name"></a> [runner\_group\_name](#input\_runner\_group\_name) | Name of the runner group. | `string` | `"Default"` | no |
 | <a name="input_runner_iam_role_managed_policy_arns"></a> [runner\_iam\_role\_managed\_policy\_arns](#input\_runner\_iam\_role\_managed\_policy\_arns) | Attach AWS or customer-managed IAM policies (by ARN) to the runner IAM role | `list(string)` | `[]` | no |

docs/configuration.md

@@ -24,14 +24,16 @@ The module uses the AWS System Manager Parameter Store to store configuration fo
 | `ssm_paths.root/var.prefix?/runners/config/<name>`            | Configuration parameters used by runner start script                                                                                                                                                                            |
 | `ssm_paths.root/var.prefix?/runners/tokens/<ec2-instance-id>` | Either JIT configuration (ephemeral runners) or registration tokens (non ephemeral runners) generated by the control plane (scale-up lambda), and consumed by the start script on the runner to activate / register the runner. |
 | `ssm_paths.root/var.prefix?/webhook/runner-matcher-config`                             | Runner matcher config used by webhook to decide the target for the webhook event.                                                                                                                                                                                                    |
+
 Available configuration parameters:
 
-| Parameter name      | Description                                                 |
-| ------------------- | ----------------------------------------------------------- |
-| `agent_mode`        | Indicates if the agent is running in ephemeral mode or not. |
-| `enable_cloudwatch` | Configuration for the cloudwatch agent to stream logging.   |
-| `run_as`            | The user used for running the GitHub action runner agent.   |
-| `token_path`        | The path where tokens are stored.                           |
+| Parameter name                      | Description                                                                                       |
+|-------------------------------------|---------------------------------------------------------------------------------------------------|
+| `agent_mode`                        | Indicates if the agent is running in ephemeral mode or not.                                       |
+| `disable_default_labels`            | Indicates if the default labels for the runners (os, architecture and `self-hosted`) are disabled |
+| `enable_cloudwatch`                 | Configuration for the cloudwatch agent to stream logging.                                         |
+| `run_as`                            | The user used for running the GitHub action runner agent.                                         |
+| `token_path`                        | The path where tokens are stored.                                                                 |
 
 ## Encryption
 

main.tf

@@ -9,7 +9,7 @@ locals {
   }
 
   default_runner_labels = distinct(concat(["self-hosted", var.runner_os, var.runner_architecture]))
-  runner_labels         = (var.runner_enable_default_labels == true) ? concat(local.default_runner_labels, var.runner_extra_labels) : var.runner_extra_labels
+  runner_labels         = (var.runner_disable_default_labels == false) ? concat(local.default_runner_labels, var.runner_extra_labels) : var.runner_extra_labels
 
   ssm_root_path = var.ssm_paths.use_prefix ? "/${var.ssm_paths.root}/${var.prefix}" : "/${var.ssm_paths.root}"
 }
@@ -200,7 +200,7 @@ module "runners" {
   scale_down_schedule_expression       = var.scale_down_schedule_expression
   minimum_running_time_in_minutes      = var.minimum_running_time_in_minutes
   runner_boot_time_in_minutes          = var.runner_boot_time_in_minutes
-  runner_enable_default_labels         = var.runner_enable_default_labels
+  runner_disable_default_labels        = var.runner_disable_default_labels
   runner_labels                        = local.runner_labels
   runner_as_root                       = var.runner_as_root
   runner_run_as                        = var.runner_run_as

modules/multi-runner/README.md

@@ -61,6 +61,7 @@ variable "multi_runner_config" {
       pool_runner_owner                       = optional(string, null)
       runner_as_root                          = optional(bool, false)
       runner_boot_time_in_minutes             = optional(number, 5)
+      runner_disable_default_labels           = optional(bool, false)
       runner_extra_labels                     = optional(list(string), [])
       runner_group_name                       = optional(string, "Default")
       runner_name_prefix                      = optional(string, "")
@@ -164,6 +165,7 @@ variable "multi_runner_config" {
         runner_additional_security_group_ids: "List of additional security groups IDs to apply to the runner. If added outside the multi_runner_config block, the additional security group(s) will be applied to all runner configs. If added inside the multi_runner_config, the additional security group(s) will be applied to the individual runner."
         runner_as_root: "Run the action runner under the root user. Variable `runner_run_as` will be ignored."
         runner_boot_time_in_minutes: "The minimum time for an EC2 runner to boot and register as a runner."
+        runner_disable_default_labels: "Disable default labels for the runners (os, architecture and `self-hosted`). If enabled, the runner will only have the extra labels provided in `runner_extra_labels`."
         runner_extra_labels: "Extra (custom) labels for the runners (GitHub). Separate each label by a comma. Labels checks on the webhook can be enforced by setting `multi_runner_config.matcherConfig.exactMatch`. GitHub read-only labels should not be provided."
         runner_group_name: "Name of the runner group."
         runner_name_prefix: "Prefix for the GitHub runner name."

modules/multi-runner/variables.tf

@@ -117,7 +117,7 @@ yarn run dist
 | [aws_launch_template.runner](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/launch_template) | resource |
 | [aws_security_group.runner_sg](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group) | resource |
 | [aws_ssm_parameter.cloudwatch_agent_config_runner](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter) | resource |
-| [aws_ssm_parameter.default_labels](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter) | resource |
+| [aws_ssm_parameter.disable_default_labels](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter) | resource |
 | [aws_ssm_parameter.jit_config_enabled](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter) | resource |
 | [aws_ssm_parameter.runner_agent_mode](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter) | resource |
 | [aws_ssm_parameter.runner_config_run_as](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter) | resource |
@@ -200,8 +200,8 @@ yarn run dist
 | <a name="input_runner_architecture"></a> [runner\_architecture](#input\_runner\_architecture) | The platform architecture of the runner instance\_type. | `string` | `"x64"` | no |
 | <a name="input_runner_as_root"></a> [runner\_as\_root](#input\_runner\_as\_root) | Run the action runner under the root user. Variable `runner_run_as` will be ignored. | `bool` | `false` | no |
 | <a name="input_runner_boot_time_in_minutes"></a> [runner\_boot\_time\_in\_minutes](#input\_runner\_boot\_time\_in\_minutes) | The minimum time for an EC2 runner to boot and register as a runner. | `number` | `5` | no |
+| <a name="input_runner_disable_default_labels"></a> [runner\_disable\_default\_labels](#input\_runner\_disable\_default\_labels) | Disable default labels for the runners (os, architecture and `self-hosted`). If enabled, the runner will only have the extra labels provided in `runner_extra_labels`. | `bool` | `false` | no |
 | <a name="input_runner_ec2_tags"></a> [runner\_ec2\_tags](#input\_runner\_ec2\_tags) | Map of tags that will be added to the launch template instance tag specifications. | `map(string)` | `{}` | no |
-| <a name="input_runner_enable_default_labels"></a> [runner\_enable\_default\_labels](#input\_runner\_enable\_default\_labels) | Enable default labels for the runners (os, architecture and `self-hosted`). If disabled, the runner will only have the extra labels provided in `runner_extra_labels`. | `bool` | `true` | no |
 | <a name="input_runner_group_name"></a> [runner\_group\_name](#input\_runner\_group\_name) | Name of the runner group. | `string` | `"Default"` | no |
 | <a name="input_runner_iam_role_managed_policy_arns"></a> [runner\_iam\_role\_managed\_policy\_arns](#input\_runner\_iam\_role\_managed\_policy\_arns) | Attach AWS or customer-managed IAM policies (by ARN) to the runner IAM role | `list(string)` | `[]` | no |
 | <a name="input_runner_labels"></a> [runner\_labels](#input\_runner\_labels) | All the labels for the runners (GitHub) including the default one's(e.g: self-hosted, linux, x64, label1, label2). Separate each label by a comma | `list(string)` | n/a | yes |

modules/runners/README.md

@@ -12,10 +12,10 @@ resource "aws_ssm_parameter" "runner_agent_mode" {
   tags  = local.tags
 }
 
-resource "aws_ssm_parameter" "default_labels" {
-  name  = "${var.ssm_paths.root}/${var.ssm_paths.config}/default_labels"
+resource "aws_ssm_parameter" "disable_default_labels" {
+  name  = "${var.ssm_paths.root}/${var.ssm_paths.config}/disable_default_labels"
   type  = "String"
-  value = var.runner_enable_default_labels
+  value = var.runner_disable_default_labels
   tags  = local.tags
 }
 

modules/runners/runner-config.tf

@@ -141,8 +141,8 @@ echo "Retrieved /$ssm_config_path/enable_cloudwatch parameter - ($enable_cloudwa
 agent_mode=$(echo "$parameters" | jq --arg ssm_config_path "$ssm_config_path" -r '.[] | select(.Name == "'$ssm_config_path'/agent_mode") | .Value')
 echo "Retrieved /$ssm_config_path/agent_mode parameter - ($agent_mode)"
 
-default_labels=$(echo "$parameters" | jq --arg ssm_config_path "$ssm_config_path" -r '.[] | select(.Name == "'$ssm_config_path'/default_labels") | .Value')
-echo "Retrieved /$ssm_config_path/default_labels parameter - ($default_labels)"
+disable_default_labels=$(echo "$parameters" | jq --arg ssm_config_path "$ssm_config_path" -r '.[] | select(.Name == "'$ssm_config_path'/disable_default_labels") | .Value')
+echo "Retrieved /$ssm_config_path/disable_default_labels parameter - ($disable_default_labels)"
 
 enable_jit_config=$(echo "$parameters" | jq --arg ssm_config_path "$ssm_config_path" -r '.[] | select(.Name == "'$ssm_config_path'/enable_jit_config") | .Value')
 echo "Retrieved /$ssm_config_path/enable_jit_config parameter - ($enable_jit_config)"
@@ -219,13 +219,12 @@ echo "Starting the runner as user $run_as"
 # configure the runner if the runner is non ephemeral or jit config is disabled
 if [[ "$enable_jit_config" == "false" || $agent_mode != "ephemeral" ]]; then
   echo "Configure GH Runner as user $run_as"
-  if [[ "$default_labels" == "true" ]]; then
+  if [[ "$disable_default_labels" == "true" ]]; then
       extra_flags="--no-default-labels"
   else
       extra_flags=""
   fi
-  sudo --preserve-env=RUNNER_ALLOW_RUNASROOT -u "$run_as" -- ./config.sh ${extra_flags} --unattended --name "$runner_name_prefix$instance_id" --work "_work" $${config}
-
+  sudo --preserve-env=RUNNER_ALLOW_RUNASROOT -u "$run_as" -- ./config.sh $${extra_flags} --unattended --name "$runner_name_prefix$instance_id" --work "_work" $${config}
 fi
 
 create_xray_success_segment "$SEGMENT"

modules/runners/templates/start-runner.sh

@@ -217,20 +217,15 @@ variable "runner_boot_time_in_minutes" {
   default     = 5
 }
 
-variable "runner_enable_default_labels" {
-  description = "Enable default labels for the runners (os, architecture and `self-hosted`). If disabled, the runner will only have the extra labels provided in `runner_extra_labels`."
+variable "runner_disable_default_labels" {
+  description = "Disable default labels for the runners (os, architecture and `self-hosted`). If enabled, the runner will only have the extra labels provided in `runner_extra_labels`."
   type        = bool
-  default     = true
+  default     = false
 }
 
 variable "runner_labels" {
   description = "All the labels for the runners (GitHub) including the default one's(e.g: self-hosted, linux, x64, label1, label2). Separate each label by a comma"
   type        = list(string)
-
-  validation {
-    condition     = var.runner_labels != null && var.runner_labels != []
-    error_message = "The runner_labels variable must be set."
-  }
 }
 
 variable "runner_group_name" {

modules/runners/variables.tf

@@ -58,8 +58,8 @@ variable "runner_boot_time_in_minutes" {
   default     = 5
 }
 
-variable "runner_enable_default_labels" {
-  description = "Enable default labels for the runners (os, architecture and `self-hosted`). If disabled, the runner will only have the extra labels provided in `runner_extra_labels`. In order set runner_enable_default_labels = false, enable_jit_config and enable_ephemeral_runners must be set to true; otherwise the value of this variable will be considered as true"
+variable "runner_disable_default_labels" {
+  description = "Disable default labels for the runners (os, architecture and `self-hosted`). If enabled, the runner will only have the extra labels provided in `runner_extra_labels`."
   type        = bool
   default     = true
 }