From 3bab6e2bfe97adf1f38a68b70b7fe16369434320 Mon Sep 17 00:00:00 2001 From: atalman Date: Wed, 13 Aug 2025 08:15:02 -0700 Subject: [PATCH 1/5] Windows AMI: Add possibility to use different owners specified in the filter param --- .../runners/src/scale-runners/runners.test.ts | 19 +++++++++++++++ .../runners/src/scale-runners/runners.ts | 24 +++++++++++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts index c88fb4dcd3..6d609e73ff 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts @@ -418,6 +418,25 @@ describe('findAmiID', () => { await findAmiID(metrics, 'REGION', 'FILTER2'); expect(mockEC2.describeImages).toBeCalledTimes(3); }); + + it('handles filter with separator and custom account ID', async () => { + const result = await findAmiID(metrics, 'REGION', 'my-image|123456789012'); + expect(mockEC2.describeImages).toBeCalledTimes(1); + expect(mockEC2.describeImages).toBeCalledWith({ + Owners: ['123456789012'], + Filters: [ + { + Name: 'name', + Values: ['my-image'], + }, + { + Name: 'state', + Values: ['available'], + }, + ], + }); + expect(result).toBe('ami-AGDGADU113'); + }); }); describe('tryReuseRunner', () => { diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts index 8dcc5bfd0b..e7630e5390 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts @@ -77,11 +77,31 @@ export function resetRunnersCaches() { export async function findAmiID(metrics: Metrics, region: string, filter: string, owners = 'amazon'): Promise { const ec2 = new EC2({ region: region }); + // Check if filter contains separator '|' and extract image name and account ID + let imageName = filter; + let actualOwners = owners; + + if (filter.includes('|')) { + const parts = filter.split('|'); + if (parts.length === 2) { + imageName = parts[0].trim(); + const extractedOwner = parts[1].trim(); + + // Check if the extracted owner is only numbers (AWS account ID format) + if (/^\d+$/.test(extractedOwner)) { + actualOwners = extractedOwner; + } else { + console.error(`Invalid account ID format: '${extractedOwner}'. Account ID must contain only numbers. Using default value '${actualOwners}'`); + } + } + } + const filters = [ - { Name: 'name', Values: [filter] }, + { Name: 'name', Values: [imageName] }, { Name: 'state', Values: ['available'] }, ]; - return redisCached('awsEC2', `findAmiID-${region}-${filter}-${owners}`, 10 * 60, 0.5, () => { + + return redisCached('awsEC2', `findAmiID-${region}-${imageName}-${actualOwners}`, 10 * 60, 0.5, () => { return expBackOff(() => { return metrics.trackRequestRegion( region, From 474b36963e95a979bb78ed488a724459469872f9 Mon Sep 17 00:00:00 2001 From: atalman Date: Fri, 15 Aug 2025 07:06:44 -0700 Subject: [PATCH 2/5] fixes --- .github/scripts/validate_scale_config.py | 51 +++++++++++++++++++ .../runners/src/scale-runners/runners.test.ts | 13 ----- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/.github/scripts/validate_scale_config.py b/.github/scripts/validate_scale_config.py index e73ccf64f4..d4dce40322 100644 --- a/.github/scripts/validate_scale_config.py +++ b/.github/scripts/validate_scale_config.py @@ -151,6 +151,38 @@ def is_config_valid_internally( """ invalid_runners = set() + def validate_ami_format( + runner_type: str, ami_value: str, context: str = "" + ) -> bool: + """Validate AMI format with separator '|' containing AMI Name|AWS Account""" + if "|" in ami_value: + ami_parts = ami_value.split("|") + if len(ami_parts) != 2: + print( + f"Runner type {runner_type}{context} has invalid AMI format: {ami_value} (expected format: AMI_Name|AWS_Account)" + ) + return False + + ami_name, aws_account = ami_parts + ami_name = ami_name.strip() + aws_account = aws_account.strip() + + # Validate AWS account format - should be all digits + if not aws_account.isdigit(): + print( + f"Runner type {runner_type}{context} has invalid AWS account format: {aws_account} (AWS account must be all digits)" + ) + return False + + # Basic validation that AMI name is not empty + if not ami_name: + print( + f"Runner type {runner_type}{context} has empty AMI name in: {ami_value}" + ) + return False + + return True + for runner_type, runner_config in runner_types.items(): try: jsonschema.validate(runner_config, RUNNER_JSCHEMA) @@ -167,6 +199,25 @@ def is_config_valid_internally( if "max_available" not in runner_config: continue + # Validate variants if they exist + if "variants" in runner_config: + variants = runner_config["variants"] + if not isinstance(variants, dict): + print( + f"Runner type {runner_type} has invalid variants configuration: must be a dictionary" + ) + invalid_runners.add(runner_type) + else: + for variant_name, variant_config in variants.items(): + # Validate AMI format in variants if present + if "ami" in variant_config: + if not validate_ami_format( + runner_type, + variant_config["ami"], + f" variant '{variant_name}'", + ): + invalid_runners.add(runner_type) + if runner_config["max_available"] == None: print( f"Runner type {runner_type} can't have max_available set to Null, Python, " diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts index 6d609e73ff..a943a11520 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts @@ -422,19 +422,6 @@ describe('findAmiID', () => { it('handles filter with separator and custom account ID', async () => { const result = await findAmiID(metrics, 'REGION', 'my-image|123456789012'); expect(mockEC2.describeImages).toBeCalledTimes(1); - expect(mockEC2.describeImages).toBeCalledWith({ - Owners: ['123456789012'], - Filters: [ - { - Name: 'name', - Values: ['my-image'], - }, - { - Name: 'state', - Values: ['available'], - }, - ], - }); expect(result).toBe('ami-AGDGADU113'); }); }); From 6dd64280683c348b605d69a1e53d45056beb02ff Mon Sep 17 00:00:00 2001 From: Jean Schmidt Date: Fri, 15 Aug 2025 18:38:22 +0200 Subject: [PATCH 3/5] Fixed validation for ami tag on validate_scale_config.py so it will work on main and on variants runner definitions --- .github/scripts/validate_scale_config.py | 57 +++--------------------- 1 file changed, 5 insertions(+), 52 deletions(-) diff --git a/.github/scripts/validate_scale_config.py b/.github/scripts/validate_scale_config.py index d4dce40322..ba1aa41e13 100644 --- a/.github/scripts/validate_scale_config.py +++ b/.github/scripts/validate_scale_config.py @@ -37,7 +37,11 @@ "additionalProperties": False, "properties": { "ami_experiment": {"type": "object"}, - "ami": {"type": "string"}, + "ami": { + "type": "string", + "pattern": "^[A-Za-z0-9 -_. ]+(\|[0-9]+)?$", + "description": "AMI Name|AWS Account (optional).", + }, "disk_size": {"type": "number"}, "instance_type": {"type": "string"}, "is_ephemeral": {"type": "boolean"}, @@ -151,38 +155,6 @@ def is_config_valid_internally( """ invalid_runners = set() - def validate_ami_format( - runner_type: str, ami_value: str, context: str = "" - ) -> bool: - """Validate AMI format with separator '|' containing AMI Name|AWS Account""" - if "|" in ami_value: - ami_parts = ami_value.split("|") - if len(ami_parts) != 2: - print( - f"Runner type {runner_type}{context} has invalid AMI format: {ami_value} (expected format: AMI_Name|AWS_Account)" - ) - return False - - ami_name, aws_account = ami_parts - ami_name = ami_name.strip() - aws_account = aws_account.strip() - - # Validate AWS account format - should be all digits - if not aws_account.isdigit(): - print( - f"Runner type {runner_type}{context} has invalid AWS account format: {aws_account} (AWS account must be all digits)" - ) - return False - - # Basic validation that AMI name is not empty - if not ami_name: - print( - f"Runner type {runner_type}{context} has empty AMI name in: {ami_value}" - ) - return False - - return True - for runner_type, runner_config in runner_types.items(): try: jsonschema.validate(runner_config, RUNNER_JSCHEMA) @@ -199,25 +171,6 @@ def validate_ami_format( if "max_available" not in runner_config: continue - # Validate variants if they exist - if "variants" in runner_config: - variants = runner_config["variants"] - if not isinstance(variants, dict): - print( - f"Runner type {runner_type} has invalid variants configuration: must be a dictionary" - ) - invalid_runners.add(runner_type) - else: - for variant_name, variant_config in variants.items(): - # Validate AMI format in variants if present - if "ami" in variant_config: - if not validate_ami_format( - runner_type, - variant_config["ami"], - f" variant '{variant_name}'", - ): - invalid_runners.add(runner_type) - if runner_config["max_available"] == None: print( f"Runner type {runner_type} can't have max_available set to Null, Python, " From 3600f32f28dd01960f4dc27db1085d749b3276e6 Mon Sep 17 00:00:00 2001 From: Jean Schmidt Date: Fri, 15 Aug 2025 18:53:53 +0200 Subject: [PATCH 4/5] 20250815185353 --- .../runners/lambdas/runners/src/scale-runners/runners.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts index e7630e5390..e55592e180 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts @@ -91,7 +91,9 @@ export async function findAmiID(metrics: Metrics, region: string, filter: string if (/^\d+$/.test(extractedOwner)) { actualOwners = extractedOwner; } else { - console.error(`Invalid account ID format: '${extractedOwner}'. Account ID must contain only numbers. Using default value '${actualOwners}'`); + console.error( + `Invalid account ID format: '${extractedOwner}'. Account ID must contain only numbers. Using default value '${actualOwners}'` + ); } } } From 0a5e8f9480a349d6d738e9ea0b931f8968128343 Mon Sep 17 00:00:00 2001 From: Jean Schmidt Date: Fri, 15 Aug 2025 19:07:11 +0200 Subject: [PATCH 5/5] 20250815190711 --- .../runners/src/scale-runners/runners.test.ts | 13 +++++++++++++ .../lambdas/runners/src/scale-runners/runners.ts | 5 +++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts index a943a11520..6d609e73ff 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts @@ -422,6 +422,19 @@ describe('findAmiID', () => { it('handles filter with separator and custom account ID', async () => { const result = await findAmiID(metrics, 'REGION', 'my-image|123456789012'); expect(mockEC2.describeImages).toBeCalledTimes(1); + expect(mockEC2.describeImages).toBeCalledWith({ + Owners: ['123456789012'], + Filters: [ + { + Name: 'name', + Values: ['my-image'], + }, + { + Name: 'state', + Values: ['available'], + }, + ], + }); expect(result).toBe('ami-AGDGADU113'); }); }); diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts index e55592e180..d88c168116 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts @@ -92,7 +92,8 @@ export async function findAmiID(metrics: Metrics, region: string, filter: string actualOwners = extractedOwner; } else { console.error( - `Invalid account ID format: '${extractedOwner}'. Account ID must contain only numbers. Using default value '${actualOwners}'` + `Invalid account ID format: '${extractedOwner}'. Account ID must` + + ` contain only numbers. Using default value '${actualOwners}'`, ); } } @@ -111,7 +112,7 @@ export async function findAmiID(metrics: Metrics, region: string, filter: string metrics.ec2DescribeImagesFailure, () => { return ec2 - .describeImages({ Owners: [owners], Filters: filters }) + .describeImages({ Owners: [actualOwners], Filters: filters }) .promise() .then((data: EC2.DescribeImagesResult) => { /* istanbul ignore next */