diff --git a/lambdas/functions/ami-housekeeper/src/ami.test.ts b/lambdas/functions/ami-housekeeper/src/ami.test.ts index 281e97856b..ab3149e00c 100644 --- a/lambdas/functions/ami-housekeeper/src/ami.test.ts +++ b/lambdas/functions/ami-housekeeper/src/ami.test.ts @@ -7,12 +7,8 @@ import { EC2Client, Image, } from '@aws-sdk/client-ec2'; -import { - DescribeParametersCommand, - DescribeParametersCommandOutput, - GetParameterCommand, - SSMClient, -} from '@aws-sdk/client-ssm'; +import { DescribeParametersCommand, DescribeParametersCommandOutput, SSMClient } from '@aws-sdk/client-ssm'; +import { getParameters } from '@aws-github-runner/aws-ssm-util'; import { mockClient } from 'aws-sdk-client-mock'; import 'aws-sdk-client-mock-jest/vitest'; @@ -20,6 +16,8 @@ import { AmiCleanupOptions, amiCleanup, defaultAmiCleanupOptions } from './ami'; import { describe, it, expect, beforeEach, vi } from 'vitest'; import { fail } from 'assert'; +vi.mock('@aws-github-runner/aws-ssm-util'); + process.env.AWS_REGION = 'eu-east-1'; const deleteAmisOlderThenDays = 30; const date31DaysAgo = new Date(new Date().setDate(new Date().getDate() - (deleteAmisOlderThenDays + 1))); @@ -83,22 +81,12 @@ describe("delete AMI's", () => { mockSSMClient.reset(); mockSSMClient.on(DescribeParametersCommand).resolves(ssmParameters); - mockSSMClient.on(GetParameterCommand, { Name: 'ami-id/ami-ssm0001' }).resolves({ - Parameter: { - Name: 'ami-id/ami-ssm0001', - Type: 'String', - Value: 'ami-ssm0001', - Version: 1, - }, - }); - mockSSMClient.on(GetParameterCommand, { Name: 'ami-id/ami-ssm0002' }).resolves({ - Parameter: { - Name: 'ami-id/ami-ssm0002', - Type: 'String', - Value: 'ami-ssm0002', - Version: 1, - }, - }); + vi.mocked(getParameters).mockResolvedValue( + new Map([ + ['ami-id/ami-ssm0001', 'ami-ssm0001'], + ['ami-id/ami-ssm0002', 'ami-ssm0002'], + ]), + ); mockEC2Client.on(DescribeLaunchTemplatesCommand).resolves({ LaunchTemplates: [ @@ -143,13 +131,7 @@ describe("delete AMI's", () => { expect(mockEC2Client).toHaveReceivedCommand(DescribeLaunchTemplatesCommand); expect(mockEC2Client).toHaveReceivedCommand(DescribeLaunchTemplateVersionsCommand); expect(mockSSMClient).toHaveReceivedCommand(DescribeParametersCommand); - expect(mockSSMClient).toHaveReceivedCommandTimes(GetParameterCommand, 2); - expect(mockSSMClient).toHaveReceivedCommandWith(GetParameterCommand, { - Name: 'ami-id/ami-ssm0001', - }); - expect(mockSSMClient).toHaveReceivedCommandWith(GetParameterCommand, { - Name: 'ami-id/ami-ssm0002', - }); + expect(getParameters).toHaveBeenCalledWith(['ami-id/ami-ssm0001', 'ami-id/ami-ssm0002']); }); it('should NOT delete instances in use.', async () => { @@ -485,14 +467,7 @@ describe("delete AMI's", () => { ], }); - mockSSMClient.on(GetParameterCommand, { Name: '/github-runner/config/ami_id' }).resolves({ - Parameter: { - Name: '/github-runner/config/ami_id', - Type: 'String', - Value: 'ami-underscore0001', - Version: 1, - }, - }); + vi.mocked(getParameters).mockResolvedValue(new Map([['/github-runner/config/ami_id', 'ami-underscore0001']])); await amiCleanup({ minimumDaysOld: 0, @@ -501,9 +476,7 @@ describe("delete AMI's", () => { // AMI should not be deleted because it's referenced in SSM expect(mockEC2Client).not.toHaveReceivedCommand(DeregisterImageCommand); - expect(mockSSMClient).toHaveReceivedCommandWith(GetParameterCommand, { - Name: '/github-runner/config/ami_id', - }); + expect(getParameters).toHaveBeenCalledWith(['/github-runner/config/ami_id']); expect(mockSSMClient).not.toHaveReceivedCommand(DescribeParametersCommand); }); @@ -518,14 +491,7 @@ describe("delete AMI's", () => { ], }); - mockSSMClient.on(GetParameterCommand, { Name: '/github-runner/config/ami-id' }).resolves({ - Parameter: { - Name: '/github-runner/config/ami-id', - Type: 'String', - Value: 'ami-hyphen0001', - Version: 1, - }, - }); + vi.mocked(getParameters).mockResolvedValue(new Map([['/github-runner/config/ami-id', 'ami-hyphen0001']])); await amiCleanup({ minimumDaysOld: 0, @@ -534,9 +500,7 @@ describe("delete AMI's", () => { // AMI should not be deleted because it's referenced in SSM expect(mockEC2Client).not.toHaveReceivedCommand(DeregisterImageCommand); - expect(mockSSMClient).toHaveReceivedCommandWith(GetParameterCommand, { - Name: '/github-runner/config/ami-id', - }); + expect(getParameters).toHaveBeenCalledWith(['/github-runner/config/ami-id']); expect(mockSSMClient).not.toHaveReceivedCommand(DescribeParametersCommand); }); @@ -561,14 +525,7 @@ describe("delete AMI's", () => { ], }); - mockSSMClient.on(GetParameterCommand, { Name: '/some/path/ami-id' }).resolves({ - Parameter: { - Name: '/some/path/ami-id', - Type: 'String', - Value: 'ami-wildcard0001', - Version: 1, - }, - }); + vi.mocked(getParameters).mockResolvedValue(new Map([['/some/path/ami-id', 'ami-wildcard0001']])); await amiCleanup({ minimumDaysOld: 0, @@ -580,9 +537,7 @@ describe("delete AMI's", () => { expect(mockSSMClient).toHaveReceivedCommandWith(DescribeParametersCommand, { ParameterFilters: [{ Key: 'Name', Option: 'Contains', Values: ['ami-id'] }], }); - expect(mockSSMClient).toHaveReceivedCommandWith(GetParameterCommand, { - Name: '/some/path/ami-id', - }); + expect(getParameters).toHaveBeenCalledWith(['/some/path/ami-id']); }); it('handles wildcard SSM parameter patterns (*ami_id)', async () => { @@ -606,14 +561,9 @@ describe("delete AMI's", () => { ], }); - mockSSMClient.on(GetParameterCommand, { Name: '/github-runner/config/ami_id' }).resolves({ - Parameter: { - Name: '/github-runner/config/ami_id', - Type: 'String', - Value: 'ami-wildcard-underscore0001', - Version: 1, - }, - }); + vi.mocked(getParameters).mockResolvedValue( + new Map([['/github-runner/config/ami_id', 'ami-wildcard-underscore0001']]), + ); await amiCleanup({ minimumDaysOld: 0, @@ -625,9 +575,7 @@ describe("delete AMI's", () => { expect(mockSSMClient).toHaveReceivedCommandWith(DescribeParametersCommand, { ParameterFilters: [{ Key: 'Name', Option: 'Contains', Values: ['ami_id'] }], }); - expect(mockSSMClient).toHaveReceivedCommandWith(GetParameterCommand, { - Name: '/github-runner/config/ami_id', - }); + expect(getParameters).toHaveBeenCalledWith(['/github-runner/config/ami_id']); }); it('handles mixed explicit names and wildcard patterns', async () => { @@ -649,14 +597,9 @@ describe("delete AMI's", () => { ], }); - mockSSMClient.on(GetParameterCommand, { Name: '/explicit/ami_id' }).resolves({ - Parameter: { - Name: '/explicit/ami_id', - Type: 'String', - Value: 'ami-explicit0001', - Version: 1, - }, - }); + vi.mocked(getParameters) + .mockResolvedValueOnce(new Map([['/explicit/ami_id', 'ami-explicit0001']])) + .mockResolvedValueOnce(new Map([['/discovered/ami-id', 'ami-wildcard0001']])); mockSSMClient.on(DescribeParametersCommand).resolves({ Parameters: [ @@ -668,15 +611,6 @@ describe("delete AMI's", () => { ], }); - mockSSMClient.on(GetParameterCommand, { Name: '/discovered/ami-id' }).resolves({ - Parameter: { - Name: '/discovered/ami-id', - Type: 'String', - Value: 'ami-wildcard0001', - Version: 1, - }, - }); - await amiCleanup({ minimumDaysOld: 0, ssmParameterNames: ['/explicit/ami_id', '*ami-id'], @@ -688,15 +622,11 @@ describe("delete AMI's", () => { ImageId: 'ami-unused0001', }); - expect(mockSSMClient).toHaveReceivedCommandWith(GetParameterCommand, { - Name: '/explicit/ami_id', - }); + expect(getParameters).toHaveBeenCalledWith(['/explicit/ami_id']); expect(mockSSMClient).toHaveReceivedCommandWith(DescribeParametersCommand, { ParameterFilters: [{ Key: 'Name', Option: 'Contains', Values: ['ami-id'] }], }); - expect(mockSSMClient).toHaveReceivedCommandWith(GetParameterCommand, { - Name: '/discovered/ami-id', - }); + expect(getParameters).toHaveBeenCalledWith(['/discovered/ami-id']); }); it('handles SSM parameter fetch failures gracefully', async () => { @@ -710,7 +640,7 @@ describe("delete AMI's", () => { ], }); - mockSSMClient.on(GetParameterCommand, { Name: '/nonexistent/ami_id' }).rejects(new Error('ParameterNotFound')); + vi.mocked(getParameters).mockRejectedValue(new Error('ParameterNotFound')); // Should not throw and should delete the AMI since SSM reference failed await amiCleanup({ @@ -768,7 +698,7 @@ describe("delete AMI's", () => { ImageId: 'ami-no-ssm0001', }); expect(mockSSMClient).not.toHaveReceivedCommand(DescribeParametersCommand); - expect(mockSSMClient).not.toHaveReceivedCommand(GetParameterCommand); + expect(getParameters).not.toHaveBeenCalled(); }); }); }); diff --git a/lambdas/functions/ami-housekeeper/src/ami.ts b/lambdas/functions/ami-housekeeper/src/ami.ts index f61dea921c..4f0c63d045 100644 --- a/lambdas/functions/ami-housekeeper/src/ami.ts +++ b/lambdas/functions/ami-housekeeper/src/ami.ts @@ -8,9 +8,10 @@ import { Filter, Image, } from '@aws-sdk/client-ec2'; -import { GetParameterCommand, SSMClient, DescribeParametersCommand } from '@aws-sdk/client-ssm'; +import { SSMClient, DescribeParametersCommand } from '@aws-sdk/client-ssm'; import { createChildLogger } from '@aws-github-runner/aws-powertools-util'; import { getTracedAWSV3Client } from '@aws-github-runner/aws-powertools-util'; +import { getParameters } from '@aws-github-runner/aws-ssm-util'; const logger = createChildLogger('ami'); @@ -184,22 +185,34 @@ async function deleteSnapshot(options: AmiCleanupOptions, amiDetails: Image, ec2 } /** - * Resolves the value of an SSM parameter by its name. Doesn't fail on errors, - * but warns instead, as this process is best-effort. + * Resolves the values of multiple SSM parameters by their names. + * Delegates batching to the shared `getParameters` utility. + * Doesn't fail on errors, but warns instead, as this process is best-effort. * - * @param name - The SSM parameter name to resolve - * @param ssmClient - Configured SSM client for making API calls - * @returns The parameter value if successful, undefined if parameter doesn't exist or access fails + * @param names - Array of SSM parameter names to resolve + * @returns Array of parameter values in the same order as input (undefined for missing/failed parameters) */ -async function resolveSsmParameterValue(name: string, ssmClient: SSMClient): Promise { +async function resolveSsmParameterValues(names: string[]): Promise<(string | undefined)[]> { + if (names.length === 0) { + return []; + } + try { - const { Parameter: parameter } = await ssmClient.send(new GetParameterCommand({ Name: name })); + const parameterMap = await getParameters(names); - return parameter?.Value; - } catch (error: unknown) { - logger.warn(`Failed to resolve image id from SSM parameter ${name}`, { error }); + // Log warnings for parameters that couldn't be resolved + for (const name of names) { + if (!parameterMap.has(name)) { + logger.warn(`Failed to resolve image id from SSM parameter ${name}: Parameter not found or access denied`); + } + } - return undefined; + // Return values in the same order as input names + return names.map((name) => parameterMap.get(name)); + } catch (error: unknown) { + logger.warn(`Failed to resolve image ids from SSM parameters ${names.join(', ')}`, { error }); + // Mark all parameters as undefined on failure + return names.map(() => undefined); } } @@ -273,11 +286,12 @@ async function getAmisReferedInSSM(options: AmiCleanupOptions): Promise<(string const explicitNames = options.ssmParameterNames.filter((n) => !n.startsWith('*')); const wildcardPatterns = options.ssmParameterNames.filter((n) => n.startsWith('*')); - const explicitValuesPromise = Promise.all(explicitNames.map((name) => resolveSsmParameterValue(name, ssmClient))); + // Batch fetch explicit parameter values in chunks of 10 (AWS API limit) + const explicitValuesPromise = resolveSsmParameterValues(explicitNames); // Handle wildcard patterns by first discovering matching parameters, then // fetching their values - let wildcardValues: Promise<(string | undefined)[]> = Promise.resolve([]); + let wildcardValuesPromise: Promise<(string | undefined)[]> = Promise.resolve([]); if (wildcardPatterns.length > 0) { // Convert wildcard patterns to SSM ParameterFilters using Contains logic // Example: "*ami-id" becomes a filter for parameters containing "ami-id" @@ -287,24 +301,30 @@ async function getAmisReferedInSSM(options: AmiCleanupOptions): Promise<(string Values: [p.replace(/^\*/g, '')], })); - try { - // Discover parameters matching the wildcard patterns - logger.debug('Describing SSM parameter', { filters }); - const ssmParameters = await ssmClient.send(new DescribeParametersCommand({ ParameterFilters: filters })); - - // Fetch the actual values of discovered parameters - wildcardValues = Promise.all( - (ssmParameters.Parameters ?? []).map((param) => resolveSsmParameterValue(param.Name!, ssmClient)), - ); - } catch (e) { - logger.warn('Failed to describe SSM parameters using wildcard patterns', { error: e }); - } + wildcardValuesPromise = (async () => { + try { + // Discover parameters matching the wildcard patterns + logger.debug('Describing SSM parameter', { filters }); + const ssmParameters = await ssmClient.send(new DescribeParametersCommand({ ParameterFilters: filters })); + + // Batch fetch the actual values of discovered parameters + const discoveredNames = (ssmParameters.Parameters ?? []) + .map((param) => param.Name) + .filter((name): name is string => name !== undefined); + + return resolveSsmParameterValues(discoveredNames); + } catch (e) { + logger.warn('Failed to describe SSM parameters using wildcard patterns', { error: e }); + return []; + } + })(); } // Combine results from both explicit and wildcard parameter resolution - const values = await Promise.all([explicitValuesPromise, wildcardValues]); + const [explicitValues, wildcardValues] = await Promise.all([explicitValuesPromise, wildcardValuesPromise]); + const values = [...explicitValues, ...wildcardValues]; logger.debug('Resolved SSM parameter values', { values }); - return values.flat(); + return values; } export { amiCleanup, getAmisNotInUse }; diff --git a/lambdas/functions/control-plane/src/github/auth.test.ts b/lambdas/functions/control-plane/src/github/auth.test.ts index 55026fa322..274496ea20 100644 --- a/lambdas/functions/control-plane/src/github/auth.test.ts +++ b/lambdas/functions/control-plane/src/github/auth.test.ts @@ -2,7 +2,7 @@ import { createAppAuth } from '@octokit/auth-app'; import { StrategyOptions } from '@octokit/auth-app/dist-types/types'; import { request } from '@octokit/request'; import { RequestInterface, RequestParameters } from '@octokit/types'; -import { getParameter } from '@aws-github-runner/aws-ssm-util'; +import { getParameters } from '@aws-github-runner/aws-ssm-util'; import { generateKeyPairSync } from 'node:crypto'; import * as nock from 'nock'; @@ -27,7 +27,7 @@ const GITHUB_APP_ID = '1'; const PARAMETER_GITHUB_APP_ID_NAME = `/actions-runner/${ENVIRONMENT}/github_app_id`; const PARAMETER_GITHUB_APP_KEY_BASE64_NAME = `/actions-runner/${ENVIRONMENT}/github_app_key_base64`; -const mockedGet = vi.mocked(getParameter); +const mockedGetParameters = vi.mocked(getParameters); beforeEach(() => { vi.resetModules(); @@ -78,9 +78,32 @@ describe('Test createGithubAppAuth', () => { process.env.ENVIRONMENT = ENVIRONMENT; }); + it('Throws early when PARAMETER_GITHUB_APP_ID_NAME is not set', async () => { + delete process.env.PARAMETER_GITHUB_APP_ID_NAME; + + await expect(createGithubAppAuth(installationId)).rejects.toThrow( + 'Environment variable PARAMETER_GITHUB_APP_ID_NAME is not set', + ); + expect(mockedGetParameters).not.toHaveBeenCalled(); + }); + + it('Throws early when PARAMETER_GITHUB_APP_KEY_BASE64_NAME is not set', async () => { + delete process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME; + + await expect(createGithubAppAuth(installationId)).rejects.toThrow( + 'Environment variable PARAMETER_GITHUB_APP_KEY_BASE64_NAME is not set', + ); + expect(mockedGetParameters).not.toHaveBeenCalled(); + }); + it('Creates auth object with createJwt callback including jti claim', async () => { // Arrange - mockedGet.mockResolvedValueOnce(GITHUB_APP_ID).mockResolvedValueOnce(b64); + mockedGetParameters.mockResolvedValueOnce( + new Map([ + [PARAMETER_GITHUB_APP_ID_NAME, GITHUB_APP_ID], + [PARAMETER_GITHUB_APP_KEY_BASE64_NAME, b64], + ]), + ); const mockedAuth = vi.fn(); mockedAuth.mockResolvedValue({ token }); @@ -108,7 +131,12 @@ describe('Test createGithubAppAuth', () => { }); const b64Key = Buffer.from(privateKey as string).toString('base64'); - mockedGet.mockResolvedValueOnce(GITHUB_APP_ID).mockResolvedValueOnce(b64Key); + mockedGetParameters.mockResolvedValueOnce( + new Map([ + [PARAMETER_GITHUB_APP_ID_NAME, GITHUB_APP_ID], + [PARAMETER_GITHUB_APP_KEY_BASE64_NAME, b64Key], + ]), + ); let capturedCreateJwt: (appId: string | number, timeDifference?: number) => Promise<{ jwt: string }>; mockedCreatAppAuth.mockImplementation((opts: StrategyOptions) => { @@ -137,9 +165,41 @@ describe('Test createGithubAppAuth', () => { expect(payload).toHaveProperty('iss'); }); + it('Creates auth object with line breaks in SSH key.', async () => { + // Arrange + const b64PrivateKeyWithLineBreaks = Buffer.from(decryptedValue + '\n' + decryptedValue, 'binary').toString( + 'base64', + ); + mockedGetParameters.mockResolvedValueOnce( + new Map([ + [PARAMETER_GITHUB_APP_ID_NAME, GITHUB_APP_ID], + [PARAMETER_GITHUB_APP_KEY_BASE64_NAME, b64PrivateKeyWithLineBreaks], + ]), + ); + + const mockedAuth = vi.fn(); + mockedAuth.mockResolvedValue({ token }); + const mockWithHook = Object.assign(mockedAuth, { hook: vi.fn() }); + mockedCreatAppAuth.mockReturnValue(mockWithHook); + + // Act + const result = await createGithubAppAuth(installationId); + + // Assert + expect(getParameters).toBeCalledWith([PARAMETER_GITHUB_APP_ID_NAME, PARAMETER_GITHUB_APP_KEY_BASE64_NAME]); + expect(mockedCreatAppAuth).toBeCalledTimes(1); + expect(mockedAuth).toBeCalledWith({ type: authType }); + expect(result.token).toBe(token); + }); + it('Creates auth object for public GitHub', async () => { // Arrange - mockedGet.mockResolvedValueOnce(GITHUB_APP_ID).mockResolvedValueOnce(b64); + mockedGetParameters.mockResolvedValueOnce( + new Map([ + [PARAMETER_GITHUB_APP_ID_NAME, GITHUB_APP_ID], + [PARAMETER_GITHUB_APP_KEY_BASE64_NAME, b64], + ]), + ); const mockedAuth = vi.fn(); mockedAuth.mockResolvedValue({ token }); @@ -150,8 +210,7 @@ describe('Test createGithubAppAuth', () => { const result = await createGithubAppAuth(installationId); // Assert - expect(getParameter).toBeCalledWith(PARAMETER_GITHUB_APP_ID_NAME); - expect(getParameter).toBeCalledWith(PARAMETER_GITHUB_APP_KEY_BASE64_NAME); + expect(getParameters).toBeCalledWith([PARAMETER_GITHUB_APP_ID_NAME, PARAMETER_GITHUB_APP_KEY_BASE64_NAME]); expect(mockedCreatAppAuth).toBeCalledTimes(1); const callArgs = mockedCreatAppAuth.mock.calls[0][0] as Record; @@ -171,7 +230,12 @@ describe('Test createGithubAppAuth', () => { () => mockedRequestInterface as RequestInterface, ); - mockedGet.mockResolvedValueOnce(GITHUB_APP_ID).mockResolvedValueOnce(b64); + mockedGetParameters.mockResolvedValueOnce( + new Map([ + [PARAMETER_GITHUB_APP_ID_NAME, GITHUB_APP_ID], + [PARAMETER_GITHUB_APP_KEY_BASE64_NAME, b64], + ]), + ); const mockedAuth = vi.fn(); mockedAuth.mockResolvedValue({ token }); // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -183,8 +247,7 @@ describe('Test createGithubAppAuth', () => { const result = await createGithubAppAuth(installationId, githubServerUrl); // Assert - expect(getParameter).toBeCalledWith(PARAMETER_GITHUB_APP_ID_NAME); - expect(getParameter).toBeCalledWith(PARAMETER_GITHUB_APP_KEY_BASE64_NAME); + expect(getParameters).toBeCalledWith([PARAMETER_GITHUB_APP_ID_NAME, PARAMETER_GITHUB_APP_KEY_BASE64_NAME]); expect(mockedCreatAppAuth).toBeCalledTimes(1); const callArgs = mockedCreatAppAuth.mock.calls[0][0] as Record; @@ -207,7 +270,12 @@ describe('Test createGithubAppAuth', () => { const installationId = undefined; - mockedGet.mockResolvedValueOnce(GITHUB_APP_ID).mockResolvedValueOnce(b64); + mockedGetParameters.mockResolvedValueOnce( + new Map([ + [PARAMETER_GITHUB_APP_ID_NAME, GITHUB_APP_ID], + [PARAMETER_GITHUB_APP_KEY_BASE64_NAME, b64], + ]), + ); const mockedAuth = vi.fn(); mockedAuth.mockResolvedValue({ token }); const mockWithHook = Object.assign(mockedAuth, { hook: vi.fn() }); @@ -217,8 +285,7 @@ describe('Test createGithubAppAuth', () => { const result = await createGithubAppAuth(installationId, githubServerUrl); // Assert - expect(getParameter).toBeCalledWith(PARAMETER_GITHUB_APP_ID_NAME); - expect(getParameter).toBeCalledWith(PARAMETER_GITHUB_APP_KEY_BASE64_NAME); + expect(getParameters).toBeCalledWith([PARAMETER_GITHUB_APP_ID_NAME, PARAMETER_GITHUB_APP_KEY_BASE64_NAME]); expect(mockedCreatAppAuth).toBeCalledTimes(1); const callArgs = mockedCreatAppAuth.mock.calls[0][0] as Record; diff --git a/lambdas/functions/control-plane/src/github/auth.ts b/lambdas/functions/control-plane/src/github/auth.ts index 927765523b..9a572c48a8 100644 --- a/lambdas/functions/control-plane/src/github/auth.ts +++ b/lambdas/functions/control-plane/src/github/auth.ts @@ -22,7 +22,7 @@ import { Octokit } from '@octokit/rest'; import { retry } from '@octokit/plugin-retry'; import { throttling } from '@octokit/plugin-throttling'; import { createChildLogger } from '@aws-github-runner/aws-powertools-util'; -import { getParameter } from '@aws-github-runner/aws-ssm-util'; +import { getParameters } from '@aws-github-runner/aws-ssm-util'; import { EndpointDefaults } from '@octokit/types'; const logger = createChildLogger('gh-auth'); @@ -91,13 +91,32 @@ function signJwt(payload: Record, privateKey: string): string { } async function createAuth(installationId: number | undefined, ghesApiUrl: string): Promise { - const appId = parseInt(await getParameter(process.env.PARAMETER_GITHUB_APP_ID_NAME)); + const appIdParamName = process.env.PARAMETER_GITHUB_APP_ID_NAME; + const appKeyParamName = process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME; + if (!appIdParamName) { + throw new Error('Environment variable PARAMETER_GITHUB_APP_ID_NAME is not set'); + } + if (!appKeyParamName) { + throw new Error('Environment variable PARAMETER_GITHUB_APP_KEY_BASE64_NAME is not set'); + } + + // Batch fetch both App ID and Private Key in a single SSM API call + const paramNames = [appIdParamName, appKeyParamName]; + const params = await getParameters(paramNames); + const appIdValue = params.get(appIdParamName); + const privateKeyBase64 = params.get(appKeyParamName); + if (!appIdValue) { + throw new Error(`Parameter ${appIdParamName} not found`); + } + if (!privateKeyBase64) { + throw new Error(`Parameter ${appKeyParamName} not found`); + } + + const appId = parseInt(appIdValue); // replace literal \n characters with new lines to allow the key to be stored as a // single line variable. This logic should match how the GitHub Terraform provider // processes private keys to retain compatibility between the projects - const privateKey = Buffer.from(await getParameter(process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME), 'base64') - .toString() - .replace('/[\\n]/g', String.fromCharCode(10)); + const privateKey = Buffer.from(privateKeyBase64, 'base64').toString().replace('/[\\n]/g', String.fromCharCode(10)); // Use a custom createJwt callback to include a jti (JWT ID) claim in every token. // Without this, concurrent Lambda invocations generating JWTs within the same second diff --git a/lambdas/functions/control-plane/src/scale-runners/scale-up.ts b/lambdas/functions/control-plane/src/scale-runners/scale-up.ts index 7f797422cc..759be95089 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-up.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-up.ts @@ -132,8 +132,6 @@ async function getGithubRunnerRegistrationToken(githubRunnerConfig: CreateGitHub repo: githubRunnerConfig.runnerOwner.split('/')[1], }); - const appId = parseInt(await getParameter(process.env.PARAMETER_GITHUB_APP_ID_NAME)); - logger.info('App id from SSM', { appId: appId }); return registrationToken.data.token; } diff --git a/lambdas/functions/webhook/src/ConfigLoader.test.ts b/lambdas/functions/webhook/src/ConfigLoader.test.ts index 3e15e3308a..d1a4f304b4 100644 --- a/lambdas/functions/webhook/src/ConfigLoader.test.ts +++ b/lambdas/functions/webhook/src/ConfigLoader.test.ts @@ -1,4 +1,4 @@ -import { getParameter } from '@aws-github-runner/aws-ssm-util'; +import { getParameter, getParameters } from '@aws-github-runner/aws-ssm-util'; import { ConfigWebhook, ConfigWebhookEventBridge, ConfigDispatcher } from './ConfigLoader'; import { logger } from '@aws-github-runner/aws-powertools-util'; @@ -183,9 +183,15 @@ describe('ConfigLoader Tests', () => { { id: '2', arn: 'arn:aws:sqs:queue2', matcherConfig: { labelMatchers: [['b']], exactMatch: true } }, ]; + // Mock getParameters for batch fetching multiple paths + vi.mocked(getParameters).mockResolvedValue( + new Map([ + ['/path/to/matcher/config-1', partialMatcher1], + ['/path/to/matcher/config-2', partialMatcher2], + ]), + ); + vi.mocked(getParameter).mockImplementation(async (paramPath: string) => { - if (paramPath === '/path/to/matcher/config-1') return partialMatcher1; - if (paramPath === '/path/to/matcher/config-2') return partialMatcher2; if (paramPath === '/path/to/webhook/secret') return 'secret'; return ''; }); @@ -205,15 +211,21 @@ describe('ConfigLoader Tests', () => { const partialMatcher2 = ',{"id":"2","arn":"arn:aws:sqs:queue2","matcherConfig":{"labelMatchers":[["b"]],"exactMatch":true}}'; + // Mock getParameters for batch fetching - returns incomplete JSON that will fail to parse + vi.mocked(getParameters).mockResolvedValue( + new Map([ + ['/path/to/matcher/config-1', partialMatcher1], + ['/path/to/matcher/config-2', partialMatcher2], + ]), + ); + vi.mocked(getParameter).mockImplementation(async (paramPath: string) => { - if (paramPath === '/path/to/matcher/config-1') return partialMatcher1; - if (paramPath === '/path/to/matcher/config-2') return partialMatcher2; if (paramPath === '/path/to/webhook/secret') return 'secret'; return ''; }); await expect(ConfigWebhook.load()).rejects.toThrow( - "Failed to load config: Failed to parse combined matcher config: Expected ',' or ']' after array element in JSON at position 196", // eslint-disable-line max-len + "Failed to load config: Failed to load/parse combined matcher config: Expected ',' or ']' after array element in JSON at position 196", // eslint-disable-line max-len ); }); }); @@ -291,11 +303,13 @@ describe('ConfigLoader Tests', () => { { id: '2', arn: 'arn:aws:sqs:queue2', matcherConfig: { labelMatchers: [['y']], exactMatch: true } }, ]; - vi.mocked(getParameter).mockImplementation(async (paramPath: string) => { - if (paramPath === '/path/to/matcher/config-1') return partial1; - if (paramPath === '/path/to/matcher/config-2') return partial2; - return ''; - }); + // Mock getParameters for batch fetching multiple paths + vi.mocked(getParameters).mockResolvedValue( + new Map([ + ['/path/to/matcher/config-1', partial1], + ['/path/to/matcher/config-2', partial2], + ]), + ); const config: ConfigDispatcher = await ConfigDispatcher.load(); diff --git a/lambdas/functions/webhook/src/ConfigLoader.ts b/lambdas/functions/webhook/src/ConfigLoader.ts index 4af58022a4..e77a92b16e 100644 --- a/lambdas/functions/webhook/src/ConfigLoader.ts +++ b/lambdas/functions/webhook/src/ConfigLoader.ts @@ -1,4 +1,4 @@ -import { getParameter } from '@aws-github-runner/aws-ssm-util'; +import { getParameter, getParameters } from '@aws-github-runner/aws-ssm-util'; import { RunnerMatcherConfig } from './sqs'; import { logger } from '@aws-github-runner/aws-powertools-util'; @@ -101,16 +101,27 @@ abstract class MatcherAwareConfig extends BaseConfig { .split(':') .map((p) => p.trim()) .filter(Boolean); - let combinedString = ''; - for (const path of paths) { - await this.loadParameter(path, 'matcherConfig'); - combinedString += this.matcherConfig; - } + // Batch fetch all matcher config paths in a single SSM API call try { - this.matcherConfig = JSON.parse(combinedString); + const params = await getParameters(paths); + let combinedString = ''; + for (const path of paths) { + const value = params.get(path); + if (value) { + combinedString += value; + } else { + this.configLoadingErrors.push( + `Failed to load parameter for matcherConfig from path ${path}: Parameter not found`, + ); + } + } + + if (combinedString) { + this.matcherConfig = JSON.parse(combinedString); + } } catch (error) { - this.configLoadingErrors.push(`Failed to parse combined matcher config: ${(error as Error).message}`); + this.configLoadingErrors.push(`Failed to load/parse combined matcher config: ${(error as Error).message}`); } } } diff --git a/lambdas/libs/aws-ssm-util/src/index.test.ts b/lambdas/libs/aws-ssm-util/src/index.test.ts index 52e4242fdb..51a9d65027 100644 --- a/lambdas/libs/aws-ssm-util/src/index.test.ts +++ b/lambdas/libs/aws-ssm-util/src/index.test.ts @@ -1,6 +1,7 @@ import { GetParameterCommand, GetParameterCommandOutput, + GetParametersCommand, PutParameterCommand, PutParameterCommandOutput, SSMClient, @@ -9,7 +10,7 @@ import 'aws-sdk-client-mock-jest/vitest'; import { mockClient } from 'aws-sdk-client-mock'; import nock from 'nock'; -import { getParameter, putParameter, SSM_ADVANCED_TIER_THRESHOLD } from '.'; +import { getParameter, getParameters, putParameter, SSM_ADVANCED_TIER_THRESHOLD } from '.'; import { describe, it, expect, beforeEach, vi } from 'vitest'; const mockSSMClient = mockClient(SSMClient); @@ -166,3 +167,90 @@ describe('Test getParameter and putParameter', () => { }); }); }); + +describe('Test getParameters (batch)', () => { + beforeEach(() => { + mockSSMClient.reset(); + }); + + it('returns multiple parameters in a single call', async () => { + mockSSMClient.on(GetParametersCommand).resolves({ + Parameters: [ + { Name: '/app/param1', Value: 'value1' }, + { Name: '/app/param2', Value: 'value2' }, + ], + }); + + const result = await getParameters(['/app/param1', '/app/param2']); + + expect(result).toEqual( + new Map([ + ['/app/param1', 'value1'], + ['/app/param2', 'value2'], + ]), + ); + expect(mockSSMClient).toHaveReceivedCommandWith(GetParametersCommand, { + Names: ['/app/param1', '/app/param2'], + WithDecryption: true, + }); + }); + + it('returns empty map for empty input', async () => { + const result = await getParameters([]); + + expect(result).toEqual(new Map()); + expect(mockSSMClient).not.toHaveReceivedCommand(GetParametersCommand); + }); + + it('chunks requests when more than 10 parameters', async () => { + const names = Array.from({ length: 12 }, (_, i) => `/app/param${i}`); + + mockSSMClient + .on(GetParametersCommand, { Names: names.slice(0, 10), WithDecryption: true }) + .resolves({ + Parameters: names.slice(0, 10).map((name) => ({ Name: name, Value: `val-${name}` })), + }) + .on(GetParametersCommand, { Names: names.slice(10), WithDecryption: true }) + .resolves({ + Parameters: names.slice(10).map((name) => ({ Name: name, Value: `val-${name}` })), + }); + + const result = await getParameters(names); + + expect(result.size).toBe(12); + expect(mockSSMClient).toHaveReceivedCommandTimes(GetParametersCommand, 2); + for (const name of names) { + expect(result.get(name)).toBe(`val-${name}`); + } + }); + + it('omits parameters with missing Name or Value', async () => { + mockSSMClient.on(GetParametersCommand).resolves({ + Parameters: [ + { Name: '/app/good', Value: 'value' }, + { Name: '/app/no-value', Value: undefined }, + { Name: undefined, Value: 'orphan' }, + ], + }); + + const result = await getParameters(['/app/good', '/app/no-value']); + + expect(result).toEqual(new Map([['/app/good', 'value']])); + }); + + it('propagates errors from SSM API', async () => { + mockSSMClient.on(GetParametersCommand).rejects(new Error('AccessDenied')); + + await expect(getParameters(['/app/param1'])).rejects.toThrow('AccessDenied'); + }); + + it('handles response with empty Parameters array', async () => { + mockSSMClient.on(GetParametersCommand).resolves({ + Parameters: [], + }); + + const result = await getParameters(['/app/missing']); + + expect(result).toEqual(new Map()); + }); +}); diff --git a/lambdas/libs/aws-ssm-util/src/index.ts b/lambdas/libs/aws-ssm-util/src/index.ts index 0b4925c17d..9173cbb210 100644 --- a/lambdas/libs/aws-ssm-util/src/index.ts +++ b/lambdas/libs/aws-ssm-util/src/index.ts @@ -1,4 +1,4 @@ -import { PutParameterCommand, SSMClient, Tag } from '@aws-sdk/client-ssm'; +import { GetParametersCommand, PutParameterCommand, SSMClient, Tag } from '@aws-sdk/client-ssm'; import { getTracedAWSV3Client } from '@aws-github-runner/aws-powertools-util'; import { SSMProvider } from '@aws-lambda-powertools/parameters/ssm'; @@ -17,6 +17,61 @@ export async function getParameter(parameter_name: string): Promise { return result; } +/** + * Retrieves multiple parameters from AWS Systems Manager Parameter Store. + * + * This function uses the AWS SSM {@link GetParametersCommand} API to fetch the values + * for the provided parameter names. Requests are automatically chunked into batches + * of up to 10 names per call to comply with the AWS GetParameters API limit. + * + * Each successfully retrieved parameter is added to the returned {@link Map}, where: + * - The map key is the full parameter name as stored in Parameter Store. + * - The map value is the decrypted string value of the parameter. + * + * Parameter names that are not found in Parameter Store (or that cannot be returned + * by the API) are silently omitted from the resulting map. They will not appear as + * keys in the returned {@link Map}. + * + * @param parameter_names - An array of parameter names to retrieve from SSM Parameter Store. + * If the array is empty, an empty {@link Map} is returned without calling the AWS API. + * + * @returns A {@link Map} where each key is a parameter name and each value is the + * corresponding decrypted string value for that parameter. Only parameters that + * are successfully retrieved and have both a `Name` and a `Value` are included. + * + * @throws Error Propagates any error thrown by the underlying AWS SDK client, + * such as network errors, AWS service errors (e.g., access denied, throttling), + * or configuration issues (e.g., missing region or credentials). + */ +export async function getParameters(parameter_names: string[]): Promise> { + if (parameter_names.length === 0) { + return new Map(); + } + + const ssmClient = getTracedAWSV3Client(new SSMClient({ region: process.env.AWS_REGION })); + const result = new Map(); + + // AWS SSM GetParameters API has a limit of 10 parameters per call + const chunkSize = 10; + for (let i = 0; i < parameter_names.length; i += chunkSize) { + const chunk = parameter_names.slice(i, i + chunkSize); + const response = await ssmClient.send( + new GetParametersCommand({ + Names: chunk, + WithDecryption: true, + }), + ); + + for (const param of response.Parameters ?? []) { + if (param.Name && param.Value) { + result.set(param.Name, param.Value); + } + } + } + + return result; +} + export const SSM_ADVANCED_TIER_THRESHOLD = 4000; export async function putParameter( diff --git a/modules/runners/job-retry/policies/lambda.json b/modules/runners/job-retry/policies/lambda.json index 591ec04790..f1c9efd569 100644 --- a/modules/runners/job-retry/policies/lambda.json +++ b/modules/runners/job-retry/policies/lambda.json @@ -4,7 +4,8 @@ { "Effect": "Allow", "Action": [ - "ssm:GetParameter" + "ssm:GetParameter", + "ssm:GetParameters" ], "Resource": [ "${github_app_key_base64_arn}", diff --git a/modules/runners/policies/lambda-scale-down.json b/modules/runners/policies/lambda-scale-down.json index d35be746b7..067a747c81 100644 --- a/modules/runners/policies/lambda-scale-down.json +++ b/modules/runners/policies/lambda-scale-down.json @@ -46,7 +46,8 @@ { "Effect": "Allow", "Action": [ - "ssm:GetParameter" + "ssm:GetParameter", + "ssm:GetParameters" ], "Resource": [ "${github_app_key_base64_arn}", diff --git a/modules/runners/policies/lambda-scale-up.json b/modules/runners/policies/lambda-scale-up.json index 3b16e710d5..93faf506a3 100644 --- a/modules/runners/policies/lambda-scale-up.json +++ b/modules/runners/policies/lambda-scale-up.json @@ -30,7 +30,8 @@ { "Effect": "Allow", "Action": [ - "ssm:GetParameter" + "ssm:GetParameter", + "ssm:GetParameters" ], "Resource": [ "${github_app_key_base64_arn}", diff --git a/modules/runners/pool/policies/lambda-pool.json b/modules/runners/pool/policies/lambda-pool.json index b0360a825c..91c9997ce4 100644 --- a/modules/runners/pool/policies/lambda-pool.json +++ b/modules/runners/pool/policies/lambda-pool.json @@ -51,7 +51,8 @@ { "Effect": "Allow", "Action": [ - "ssm:GetParameter" + "ssm:GetParameter", + "ssm:GetParameters" ], "Resource": [ "${github_app_key_base64_arn}", diff --git a/modules/webhook/policies/lambda-ssm.json b/modules/webhook/policies/lambda-ssm.json index 9e33d1ca0a..b1ebca8c8b 100644 --- a/modules/webhook/policies/lambda-ssm.json +++ b/modules/webhook/policies/lambda-ssm.json @@ -3,7 +3,7 @@ "Statement": [ { "Effect": "Allow", - "Action": ["ssm:GetParameter"], + "Action": ["ssm:GetParameter", "ssm:GetParameters"], "Resource": ${resource_arns} } ]