diff --git a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts index b028a4c098..19b19a1cbd 100644 --- a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts +++ b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts @@ -9,6 +9,19 @@ vi.mock('@aws-github-runner/aws-ssm-util', () => ({ })); const getParametersMock = vi.mocked(getParameters); +const loggerMock = vi.hoisted(() => ({ + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), +})); + +vi.mock('@aws-github-runner/aws-powertools-util', () => ({ + createChildLogger: vi.fn(() => ({ + ...loggerMock, + appendPersistentKeys: vi.fn(), + })), +})); describe('aws_ssm GitHub App credentials store', () => { beforeEach(() => { @@ -62,4 +75,31 @@ describe('aws_ssm GitHub App credentials store', () => { process.env.PARAMETER_GITHUB_APP_ID_NAME = 'id-0:id-1'; expect(() => createAwsSsmGitHubAppCredentialsStore()).toThrow('parameter count mismatch'); }); + + it('logs safe context when a credential parameter is missing', async () => { + getParametersMock.mockResolvedValue(new Map([['app-key', Buffer.from('private-key').toString('base64')]])); + + await expect(createAwsSsmGitHubAppCredentialsStore().get()).rejects.toThrow('Parameter app-id not found'); + + expect(loggerMock.error).toHaveBeenCalledWith('GitHub App credential parameter is missing', { + credentialField: 'appId', + appIndex: 0, + parameterName: 'app-id', + }); + expect(JSON.stringify(loggerMock.error.mock.calls)).not.toContain('private-key'); + }); + + it('logs only error names when the provider lookup fails', async () => { + const error = Object.assign(new Error('private-key-secret'), { name: 'InternalServerException' }); + getParametersMock.mockRejectedValue(error); + + await expect(createAwsSsmGitHubAppCredentialsStore().get()).rejects.toBe(error); + + expect(loggerMock.error).toHaveBeenCalledWith('Failed to read GitHub App credential parameters', { + parameterCount: 2, + appCount: 1, + errorNames: ['InternalServerException'], + }); + expect(JSON.stringify(loggerMock.error.mock.calls)).not.toContain('private-key-secret'); + }); }); diff --git a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts index 0b9ce7bc5a..803020ba4c 100644 --- a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts +++ b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts @@ -1,6 +1,9 @@ import { getParameters } from '@aws-github-runner/aws-ssm-util'; import type { GitHubAppCredential, GitHubAppCredentialsStore } from '../../core'; +import { createAwsSsmStorageLogger, getErrorNames } from './logger'; + +const logger = createAwsSsmStorageLogger('github-app-credentials-store'); interface AwsSsmGitHubAppCredentialsEnvironment { PARAMETER_GITHUB_APP_ID_NAME?: string; @@ -33,19 +36,47 @@ class AwsSsmGitHubAppCredentialsStore implements GitHubAppCredentialsStore { ) {} async get(): Promise { - const parameters = await getParameters([ + const parameterNames = [ ...this.idParameters, ...this.keyParameters, ...this.installationIdParameters.filter(Boolean), - ]); - return this.idParameters.map((idParameter, index) => { + ]; + logger.debug('Reading GitHub App credential parameters', { + parameterCount: parameterNames.length, + appCount: this.idParameters.length, + }); + + let parameters: Map; + try { + parameters = await getParameters(parameterNames); + } catch (error) { + logger.error('Failed to read GitHub App credential parameters', { + parameterCount: parameterNames.length, + appCount: this.idParameters.length, + errorNames: getErrorNames(error), + }); + throw error; + } + + const credentials = this.idParameters.map((idParameter, index) => { const appIdValue = parameters.get(idParameter); if (!appIdValue) { + logger.error('GitHub App credential parameter is missing', { + credentialField: 'appId', + appIndex: index, + parameterName: idParameter, + }); throw new Error(`Parameter ${idParameter} not found`); } - const privateKeyBase64 = parameters.get(this.keyParameters[index]); + const keyParameter = this.keyParameters[index]; + const privateKeyBase64 = parameters.get(keyParameter); if (!privateKeyBase64) { - throw new Error(`Parameter ${this.keyParameters[index]} not found`); + logger.error('GitHub App credential parameter is missing', { + credentialField: 'privateKey', + appIndex: index, + parameterName: keyParameter, + }); + throw new Error(`Parameter ${keyParameter} not found`); } const installationIdParameter = this.installationIdParameters[index]; const installationIdValue = installationIdParameter ? parameters.get(installationIdParameter) : undefined; @@ -55,6 +86,12 @@ class AwsSsmGitHubAppCredentialsStore implements GitHubAppCredentialsStore { installationId: installationIdValue ? Number.parseInt(installationIdValue, 10) : undefined, }; }); + + logger.debug('Loaded GitHub App credential parameters', { + appCount: credentials.length, + installationIdCount: credentials.filter(({ installationId }) => installationId !== undefined).length, + }); + return credentials; } } diff --git a/lambdas/libs/storage-providers/aws/ssm/logger.test.ts b/lambdas/libs/storage-providers/aws/ssm/logger.test.ts new file mode 100644 index 0000000000..9fcd42af98 --- /dev/null +++ b/lambdas/libs/storage-providers/aws/ssm/logger.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { runnerConfigStorageProvider } from '../../provider'; +import { createAwsSsmStorageLogger, getErrorNames } from './logger'; + +const loggerMock = vi.hoisted(() => ({ + appendPersistentKeys: vi.fn(), +})); +const createChildLoggerMock = vi.hoisted(() => vi.fn(() => loggerMock)); + +vi.mock('@aws-github-runner/aws-powertools-util', () => ({ + createChildLogger: createChildLoggerMock, +})); + +describe('AWS SSM storage logger', () => { + it('adds the canonical storage provider while preserving the adapter module', () => { + expect(createAwsSsmStorageLogger('runner-config-store')).toBe(loggerMock); + expect(createChildLoggerMock).toHaveBeenCalledWith('runner-config-store'); + expect(loggerMock.appendPersistentKeys).toHaveBeenCalledWith({ + storageProvider: runnerConfigStorageProvider.awsSsm, + }); + }); + + it('returns bounded error names from a cause chain', () => { + const cause = Object.assign(new Error('missing'), { name: 'ParameterNotFound' }); + const error = Object.assign(new Error('wrapped'), { name: 'GetParameterError', cause }); + Object.assign(cause, { cause: error }); + + expect(getErrorNames(error)).toEqual(['GetParameterError', 'ParameterNotFound']); + }); +}); diff --git a/lambdas/libs/storage-providers/aws/ssm/logger.ts b/lambdas/libs/storage-providers/aws/ssm/logger.ts new file mode 100644 index 0000000000..0e3633eb71 --- /dev/null +++ b/lambdas/libs/storage-providers/aws/ssm/logger.ts @@ -0,0 +1,27 @@ +import { createChildLogger } from '@aws-github-runner/aws-powertools-util'; + +import { runnerConfigStorageProvider } from '../../provider'; + +export function createAwsSsmStorageLogger(module: string) { + const logger = createChildLogger(module); + logger.appendPersistentKeys({ + storageProvider: runnerConfigStorageProvider.awsSsm, + }); + return logger; +} + +export function getErrorNames(error: unknown): string[] { + const names: string[] = []; + const seen = new Set(); + let current: unknown = error; + + while (current !== null && typeof current === 'object' && !seen.has(current)) { + seen.add(current); + if ('name' in current && typeof current.name === 'string') { + names.push(current.name); + } + current = 'cause' in current ? current.cause : undefined; + } + + return names; +} diff --git a/lambdas/libs/storage-providers/aws/ssm/runner-config-consumer.test.ts b/lambdas/libs/storage-providers/aws/ssm/runner-config-consumer.test.ts index 2a5d2217ea..14dbc670c3 100644 --- a/lambdas/libs/storage-providers/aws/ssm/runner-config-consumer.test.ts +++ b/lambdas/libs/storage-providers/aws/ssm/runner-config-consumer.test.ts @@ -1,5 +1,5 @@ import { DeleteParameterCommand, GetParameterCommand, type SSMClient } from '@aws-sdk/client-ssm'; -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { AwsSdkSsmRunnerConfigApi, @@ -7,6 +7,20 @@ import { type AwsSsmRunnerConfigApi, } from './runner-config-consumer'; +const loggerMock = vi.hoisted(() => ({ + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), +})); + +vi.mock('@aws-github-runner/aws-powertools-util', () => ({ + createChildLogger: vi.fn(() => ({ + ...loggerMock, + appendPersistentKeys: vi.fn(), + })), +})); + function namedError(name: string, message = 'provider detail'): Error { const error = new Error(message); error.name = name; @@ -38,6 +52,10 @@ describe('AWS SDK SSM runner config API', () => { }); describe('SSM runner config consumer', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + afterEach(() => { vi.useRealTimers(); }); @@ -49,7 +67,7 @@ describe('SSM runner config consumer', () => { .mockResolvedValueOnce('encoded-jit'); const deleteParameter = vi.fn().mockResolvedValue(undefined); const consumer = createAwsSsmRunnerConfigConsumer( - { RUNNER_CONFIG_STORAGE_PROVIDER: 'aws_ssm', SSM_TOKEN_PATH: '/runner/tokens' }, + { SSM_TOKEN_PATH: '/runner/tokens' }, { api: { getParameter, deleteParameter }, callTimeoutMs: 100, @@ -80,7 +98,7 @@ describe('SSM runner config consumer', () => { .mockResolvedValueOnce(undefined), }; const consumer = createAwsSsmRunnerConfigConsumer( - { RUNNER_CONFIG_STORAGE_PROVIDER: 'aws_ssm', SSM_TOKEN_PATH: '/runner/tokens' }, + { SSM_TOKEN_PATH: '/runner/tokens' }, { api, callTimeoutMs: 100, configTimeoutMs: 2_000, deleteAttempts: 2, pollIntervalMs: 1 }, ); @@ -95,31 +113,42 @@ describe('SSM runner config consumer', () => { }); it('fails closed when another reader deletes the SSM parameter first', async () => { + const deleteError = namedError('ParameterNotFound'); const api: AwsSsmRunnerConfigApi = { getParameter: vi.fn().mockResolvedValue('encoded-jit'), - deleteParameter: vi.fn().mockRejectedValue(namedError('ParameterNotFound')), + deleteParameter: vi.fn().mockRejectedValue(deleteError), }; const consumer = createAwsSsmRunnerConfigConsumer( - { RUNNER_CONFIG_STORAGE_PROVIDER: 'aws_ssm', SSM_TOKEN_PATH: '/runner/tokens' }, + { SSM_TOKEN_PATH: '/runner/tokens' }, { api, callTimeoutMs: 100, configTimeoutMs: 100, deleteAttempts: 3, pollIntervalMs: 1 }, ); - await expect( - consumer.consume('runner-123', { - deadlineMs: Date.now() + 1_000, - signal: new AbortController().signal, - }), - ).rejects.toThrow('runner configuration could not be deleted from SSM'); + const pending = consumer.consume('runner-123', { + deadlineMs: Date.now() + 1_000, + signal: new AbortController().signal, + }); + + await expect(pending).rejects.toMatchObject({ + message: 'runner configuration could not be deleted from SSM', + cause: deleteError, + }); expect(api.deleteParameter).toHaveBeenCalledOnce(); + expect(loggerMock.error).toHaveBeenCalledWith('Failed to delete consumed runner configuration', { + runnerId: 'runner-123', + parameterName: '/runner/tokens/runner-123', + deleteAttempts: 1, + errorNames: ['ParameterNotFound'], + }); }); it('sanitizes non-retryable provider failures', async () => { + const providerError = namedError('AccessDeniedException', 'encoded-jit-secret'); const api: AwsSsmRunnerConfigApi = { - getParameter: vi.fn().mockRejectedValue(namedError('AccessDeniedException', 'encoded-jit-secret')), + getParameter: vi.fn().mockRejectedValue(providerError), deleteParameter: vi.fn(), }; const consumer = createAwsSsmRunnerConfigConsumer( - { RUNNER_CONFIG_STORAGE_PROVIDER: 'aws_ssm', SSM_TOKEN_PATH: '/runner/tokens' }, + { SSM_TOKEN_PATH: '/runner/tokens' }, { api, callTimeoutMs: 100, configTimeoutMs: 100, pollIntervalMs: 1 }, ); @@ -127,9 +156,18 @@ describe('SSM runner config consumer', () => { deadlineMs: Date.now() + 1_000, signal: new AbortController().signal, }); - await expect(pending).rejects.toThrow('failed to read runner configuration from SSM'); - await expect(pending).rejects.not.toThrow('encoded-jit-secret'); + await expect(pending).rejects.toMatchObject({ + message: 'failed to read runner configuration from SSM', + cause: providerError, + }); expect(api.deleteParameter).not.toHaveBeenCalled(); + expect(loggerMock.error).toHaveBeenCalledWith('Failed to read runner configuration', { + runnerId: 'runner-123', + parameterName: '/runner/tokens/runner-123', + pollAttempt: 1, + errorNames: ['AccessDeniedException'], + }); + expect(JSON.stringify(loggerMock.error.mock.calls)).not.toContain('encoded-jit-secret'); }); it('rejects an empty SSM parameter value without attempting deletion', async () => { @@ -138,7 +176,7 @@ describe('SSM runner config consumer', () => { deleteParameter: vi.fn(), }; const consumer = createAwsSsmRunnerConfigConsumer( - { RUNNER_CONFIG_STORAGE_PROVIDER: 'aws_ssm', SSM_TOKEN_PATH: '/runner/tokens' }, + { SSM_TOKEN_PATH: '/runner/tokens' }, { api, callTimeoutMs: 100, configTimeoutMs: 100, pollIntervalMs: 1 }, ); @@ -157,7 +195,7 @@ describe('SSM runner config consumer', () => { deleteParameter: vi.fn(), }; const consumer = createAwsSsmRunnerConfigConsumer( - { RUNNER_CONFIG_STORAGE_PROVIDER: 'aws_ssm', SSM_TOKEN_PATH: `/${'x'.repeat(890)}` }, + { SSM_TOKEN_PATH: `/${'x'.repeat(890)}` }, { api, callTimeoutMs: 100, configTimeoutMs: 100, pollIntervalMs: 1 }, ); @@ -177,7 +215,7 @@ describe('SSM runner config consumer', () => { }; const controller = new AbortController(); const consumer = createAwsSsmRunnerConfigConsumer( - { RUNNER_CONFIG_STORAGE_PROVIDER: 'aws_ssm', SSM_TOKEN_PATH: '/runner/tokens' }, + { SSM_TOKEN_PATH: '/runner/tokens' }, { api, callTimeoutMs: 10_000, configTimeoutMs: 10_000, pollIntervalMs: 1 }, ); const pending = consumer.consume('runner-123', { @@ -202,7 +240,7 @@ describe('SSM runner config consumer', () => { }), }; const consumer = createAwsSsmRunnerConfigConsumer( - { RUNNER_CONFIG_STORAGE_PROVIDER: 'aws_ssm', SSM_TOKEN_PATH: '/runner/tokens' }, + { SSM_TOKEN_PATH: '/runner/tokens' }, { api, callTimeoutMs: 100, configTimeoutMs: 1_000, pollIntervalMs: 99 }, ); diff --git a/lambdas/libs/storage-providers/aws/ssm/runner-config-consumer.ts b/lambdas/libs/storage-providers/aws/ssm/runner-config-consumer.ts index da26a25402..dd5e096fae 100644 --- a/lambdas/libs/storage-providers/aws/ssm/runner-config-consumer.ts +++ b/lambdas/libs/storage-providers/aws/ssm/runner-config-consumer.ts @@ -13,8 +13,10 @@ import { withCallDeadline, type RunnerConfigPollingOptions, } from './runner-config-consumer-common'; +import { createAwsSsmStorageLogger, getErrorNames } from './logger'; const DEFAULT_DELETE_ATTEMPTS = 3; +const logger = createAwsSsmStorageLogger('runner-config-consumer'); export interface AwsSsmRunnerConfigApi { getParameter(name: string, signal: AbortSignal): Promise; @@ -92,9 +94,19 @@ class AwsSsmRunnerConfigConsumer implements RunnerConfigConsumer { const deleteReserveMs = Math.min(this.callTimeoutMs, Math.max(1, Math.floor(remainingMs / 2))); const pollDeadline = Math.min(startedAt + this.configTimeoutMs, options.deadlineMs - deleteReserveMs); let runnerConfig: string | undefined; + let pollAttempt = 0; + + logger.debug('Waiting for runner configuration', { + runnerId, + parameterName, + callTimeoutMs: this.callTimeoutMs, + pollDeadline, + pollIntervalMs: this.pollIntervalMs, + }); while (Date.now() < pollDeadline) { throwIfCancelled(options.signal); + pollAttempt += 1; try { runnerConfig = await this.read(parameterName, pollDeadline, options.signal); if (runnerConfig !== undefined) { @@ -105,11 +117,23 @@ class AwsSsmRunnerConfigConsumer implements RunnerConfigConsumer { } } catch (error) { if (options.signal.aborted) { - throw new Error('runner configuration consumption was cancelled'); + throw new Error('runner configuration consumption was cancelled', { cause: error }); } if (!isSsmNotFound(error) && !isRetryableProviderError(error)) { - throw new Error('failed to read runner configuration from SSM'); + logger.error('Failed to read runner configuration', { + runnerId, + parameterName, + pollAttempt, + errorNames: getErrorNames(error), + }); + throw new Error('failed to read runner configuration from SSM', { cause: error }); } + logger.debug('Runner configuration is not available; polling will continue', { + runnerId, + parameterName, + pollAttempt, + errorNames: getErrorNames(error), + }); } const remaining = pollDeadline - Date.now(); @@ -119,10 +143,21 @@ class AwsSsmRunnerConfigConsumer implements RunnerConfigConsumer { } if (runnerConfig === undefined) { + logger.warn('Runner configuration did not become available before the deadline', { + runnerId, + parameterName, + pollAttempts: pollAttempt, + pollDeadline, + }); throw new Error('runner configuration did not become available before the deadline'); } - await this.delete(parameterName, options); + logger.debug('Runner configuration became available', { + runnerId, + parameterName, + pollAttempts: pollAttempt, + }); + await this.delete(parameterName, runnerId, options); return runnerConfig; } @@ -132,16 +167,25 @@ class AwsSsmRunnerConfigConsumer implements RunnerConfigConsumer { ); } - private async delete(name: string, options: RunnerConfigConsumeOptions): Promise { + private async delete(name: string, runnerId: string, options: RunnerConfigConsumeOptions): Promise { + let lastError: unknown; + let attemptedDeletes = 0; for (let attempt = 1; attempt <= this.deleteAttempts; attempt += 1) { + attemptedDeletes = attempt; try { await withCallDeadline(options.signal, options.deadlineMs, this.callTimeoutMs, (callSignal) => this.api.deleteParameter(name, callSignal), ); + logger.debug('Deleted consumed runner configuration', { + runnerId, + parameterName: name, + deleteAttempt: attempt, + }); return; } catch (error) { + lastError = error; if (options.signal.aborted) { - throw new Error('runner configuration consumption was cancelled'); + throw new Error('runner configuration consumption was cancelled', { cause: error }); } if (!isRetryableProviderError(error) || attempt === this.deleteAttempts) { break; @@ -151,10 +195,22 @@ class AwsSsmRunnerConfigConsumer implements RunnerConfigConsumer { if (remaining <= 0) { break; } + logger.debug('Runner configuration deletion failed; retrying', { + runnerId, + parameterName: name, + deleteAttempt: attempt, + errorNames: getErrorNames(error), + }); await delay(Math.min(2 ** (attempt - 1) * 1_000, 5_000, remaining), options.signal); } } - throw new Error('runner configuration could not be deleted from SSM'); + logger.error('Failed to delete consumed runner configuration', { + runnerId, + parameterName: name, + deleteAttempts: attemptedDeletes, + errorNames: getErrorNames(lastError), + }); + throw new Error('runner configuration could not be deleted from SSM', { cause: lastError }); } } diff --git a/lambdas/libs/storage-providers/aws/ssm/runner-config-housekeeper.ts b/lambdas/libs/storage-providers/aws/ssm/runner-config-housekeeper.ts index a7bfdccbb0..8bd657b22a 100644 --- a/lambdas/libs/storage-providers/aws/ssm/runner-config-housekeeper.ts +++ b/lambdas/libs/storage-providers/aws/ssm/runner-config-housekeeper.ts @@ -1,7 +1,15 @@ -import { DeleteParameterCommand, GetParametersByPathCommand, SSMClient } from '@aws-sdk/client-ssm'; -import { getTracedAWSV3Client, logger } from '@aws-github-runner/aws-powertools-util'; +import { + DeleteParameterCommand, + GetParametersByPathCommand, + SSMClient, + type GetParametersByPathCommandOutput, +} from '@aws-sdk/client-ssm'; +import { getTracedAWSV3Client } from '@aws-github-runner/aws-powertools-util'; import type { RunnerConfigHousekeeper } from '../../core'; +import { createAwsSsmStorageLogger, getErrorNames } from './logger'; + +const logger = createAwsSsmStorageLogger('runner-config-housekeeper'); export interface SSMCleanupOptions { dryRun: boolean; @@ -14,38 +22,62 @@ export function createAwsSsmRunnerConfigHousekeeper(options?: SSMCleanupOptions) } export async function cleanSSMTokens(options: SSMCleanupOptions): Promise { - logger.info(`Cleaning tokens / JIT config older then ${options.minimumDaysOld} days, dryRun: ${options.dryRun}`); - logger.debug('Cleaning with options', { options }); validateOptions(options); + logger.info('Cleaning expired runner configurations', { + minimumDaysOld: options.minimumDaysOld, + dryRun: options.dryRun, + tokenPath: options.tokenPath, + }); const client = getTracedAWSV3Client(new SSMClient({ region: process.env.AWS_REGION })); - const parameters = await client.send(new GetParametersByPathCommand({ Path: options.tokenPath })); - while (parameters.NextToken) { - const nextParameters = await client.send( - new GetParametersByPathCommand({ Path: options.tokenPath, NextToken: parameters.NextToken }), - ); - parameters.Parameters?.push(...(nextParameters.Parameters ?? [])); - parameters.NextToken = nextParameters.NextToken; + let parameters: GetParametersByPathCommandOutput; + try { + parameters = await client.send(new GetParametersByPathCommand({ Path: options.tokenPath })); + while (parameters.NextToken) { + const nextParameters = await client.send( + new GetParametersByPathCommand({ Path: options.tokenPath, NextToken: parameters.NextToken }), + ); + parameters.Parameters?.push(...(nextParameters.Parameters ?? [])); + parameters.NextToken = nextParameters.NextToken; + } + } catch (error) { + logger.error('Failed to list runner configurations', { + tokenPath: options.tokenPath, + errorNames: getErrorNames(error), + }); + throw error; } - logger.info(`Found #${parameters.Parameters?.length} parameters in path ${options.tokenPath}`); + logger.info('Found runner configurations', { + tokenPath: options.tokenPath, + parameterCount: parameters.Parameters?.length ?? 0, + }); const minimumDate = new Date(); minimumDate.setDate(minimumDate.getDate() - options.minimumDaysOld); for (const parameter of parameters.Parameters ?? []) { if (parameter.LastModifiedDate && new Date(parameter.LastModifiedDate) < minimumDate) { - logger.info(`Deleting parameter ${parameter.Name} with last modified date ${parameter.LastModifiedDate}`); + logger.info('Deleting expired runner configuration', { + parameterName: parameter.Name, + lastModifiedDate: parameter.LastModifiedDate, + dryRun: options.dryRun, + }); try { if (!options.dryRun) { await new Promise((resolve) => setTimeout(resolve, 50)); await client.send(new DeleteParameterCommand({ Name: parameter.Name })); } } catch (error) { - logger.warn(`Failed to delete parameter ${parameter.Name} with error ${(error as Error).message}`); - logger.debug('Failed to delete parameter', { error }); + logger.warn('Failed to delete expired runner configuration', { + parameterName: parameter.Name, + errorNames: getErrorNames(error), + }); } } else { - logger.debug(`Skipping parameter ${parameter.Name} with last modified date ${parameter.LastModifiedDate}`); + logger.debug('Skipping runner configuration that is not expired', { + parameterName: parameter.Name, + lastModifiedDate: parameter.LastModifiedDate, + }); } } } diff --git a/lambdas/libs/storage-providers/aws/ssm/runner-config-store.test.ts b/lambdas/libs/storage-providers/aws/ssm/runner-config-store.test.ts index a4bb80aa56..9d3e948b9d 100644 --- a/lambdas/libs/storage-providers/aws/ssm/runner-config-store.test.ts +++ b/lambdas/libs/storage-providers/aws/ssm/runner-config-store.test.ts @@ -9,6 +9,19 @@ vi.mock('@aws-github-runner/aws-ssm-util', () => ({ const putParameterMock = vi.mocked(putParameter); const cleanEnv = process.env; +const loggerMock = vi.hoisted(() => ({ + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), +})); + +vi.mock('@aws-github-runner/aws-powertools-util', () => ({ + createChildLogger: vi.fn(() => ({ + ...loggerMock, + appendPersistentKeys: vi.fn(), + })), +})); describe('aws_ssm runner config store', () => { beforeEach(() => { @@ -77,6 +90,22 @@ describe('aws_ssm runner config store', () => { expect(putParameterMock).toHaveBeenCalledWith('/runner/tokens/runner-1', 'jit-config', true, { tags: [] }); }); + + it('logs safe context when a runner configuration write fails', async () => { + const error = Object.assign(new Error('encoded-jit-secret'), { name: 'ThrottlingException' }); + putParameterMock.mockRejectedValue(error); + + await expect( + createAwsSsmRunnerConfigStore().create({ runnerId: 'runner-1', value: 'encoded-jit-secret' }), + ).rejects.toBe(error); + + expect(loggerMock.error).toHaveBeenCalledWith('Failed to write runner configuration', { + runnerId: 'runner-1', + parameterName: '/runner/tokens/runner-1', + errorNames: ['ThrottlingException'], + }); + expect(JSON.stringify(loggerMock.error.mock.calls)).not.toContain('encoded-jit-secret'); + }); }); function setTokenPath(tokenPath: string | undefined): void { diff --git a/lambdas/libs/storage-providers/aws/ssm/runner-config-store.ts b/lambdas/libs/storage-providers/aws/ssm/runner-config-store.ts index 80941822a5..584b654ba9 100644 --- a/lambdas/libs/storage-providers/aws/ssm/runner-config-store.ts +++ b/lambdas/libs/storage-providers/aws/ssm/runner-config-store.ts @@ -2,8 +2,11 @@ import { putParameter } from '@aws-github-runner/aws-ssm-util'; import type { RunnerConfigMetadata, RunnerConfigRecord, RunnerConfigStore } from '../../core'; import type {} from './environment'; +import { createAwsSsmStorageLogger, getErrorNames } from './logger'; import { loadSsmParameterStoreTagsFromEnvironment } from './parameter-store-tags'; +const logger = createAwsSsmStorageLogger('runner-config-store'); + export interface AwsSsmRunnerConfigStoreConfig { tokenPath: string; parameterStoreTags: ReadonlyArray>; @@ -35,11 +38,31 @@ class AwsSsmRunnerConfigStore implements RunnerConfigStore { constructor(private readonly config: AwsSsmRunnerConfigStoreConfig) {} async create(record: RunnerConfigRecord, options: { metadata?: RunnerConfigMetadata[] } = {}): Promise { - await putParameter(`${this.config.tokenPath}/${record.runnerId}`, record.value, true, { - tags: [ - ...(options.metadata ?? []).map(({ key, value }) => ({ Key: key, Value: value })), - ...this.config.parameterStoreTags, - ], + const parameterName = `${this.config.tokenPath}/${record.runnerId}`; + logger.debug('Writing runner configuration', { + runnerId: record.runnerId, + parameterName, + }); + + try { + await putParameter(parameterName, record.value, true, { + tags: [ + ...(options.metadata ?? []).map(({ key, value }) => ({ Key: key, Value: value })), + ...this.config.parameterStoreTags, + ], + }); + } catch (error) { + logger.error('Failed to write runner configuration', { + runnerId: record.runnerId, + parameterName, + errorNames: getErrorNames(error), + }); + throw error; + } + + logger.debug('Stored runner configuration', { + runnerId: record.runnerId, + parameterName, }); } } diff --git a/lambdas/libs/storage-providers/aws/ssm/runner-group-cache-store.test.ts b/lambdas/libs/storage-providers/aws/ssm/runner-group-cache-store.test.ts index cfb84e2d89..4fc13926d9 100644 --- a/lambdas/libs/storage-providers/aws/ssm/runner-group-cache-store.test.ts +++ b/lambdas/libs/storage-providers/aws/ssm/runner-group-cache-store.test.ts @@ -11,6 +11,19 @@ vi.mock('@aws-github-runner/aws-ssm-util', () => ({ const getParameterMock = vi.mocked(getParameter); const putParameterMock = vi.mocked(putParameter); +const loggerMock = vi.hoisted(() => ({ + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), +})); + +vi.mock('@aws-github-runner/aws-powertools-util', () => ({ + createChildLogger: vi.fn(() => ({ + ...loggerMock, + appendPersistentKeys: vi.fn(), + })), +})); describe('aws_ssm runner group cache store', () => { beforeEach(() => { @@ -46,6 +59,14 @@ describe('aws_ssm runner group cache store', () => { ); await expect(createAwsSsmRunnerGroupCacheStore().get('Default')).resolves.toBeUndefined(); + expect(loggerMock.info).toHaveBeenCalledWith( + 'Runner group cache miss; caller will resolve the ID from GitHub', + expect.objectContaining({ + runnerGroupName: 'Default', + parameterName: '/runner/config/runner-group/Default', + errorNames: ['GetParameterError', 'ParameterNotFound'], + }), + ); }); it('propagates access and service errors', async () => { diff --git a/lambdas/libs/storage-providers/aws/ssm/runner-group-cache-store.ts b/lambdas/libs/storage-providers/aws/ssm/runner-group-cache-store.ts index c82fc8a182..37995d1e5d 100644 --- a/lambdas/libs/storage-providers/aws/ssm/runner-group-cache-store.ts +++ b/lambdas/libs/storage-providers/aws/ssm/runner-group-cache-store.ts @@ -2,8 +2,11 @@ import { getParameter, putParameter } from '@aws-github-runner/aws-ssm-util'; import type { RunnerGroupCacheRecord, RunnerGroupCacheStore } from '../../core'; import type {} from './environment'; +import { createAwsSsmStorageLogger, getErrorNames } from './logger'; import { loadSsmParameterStoreTagsFromEnvironment } from './parameter-store-tags'; +const logger = createAwsSsmStorageLogger('runner-group-cache-store'); + export interface AwsSsmRunnerGroupCacheStoreConfig { configPath: string; parameterStoreTags: ReadonlyArray>; @@ -33,25 +36,54 @@ class AwsSsmRunnerGroupCacheStore implements RunnerGroupCacheStore { constructor(private readonly config: AwsSsmRunnerGroupCacheStoreConfig) {} async get(runnerGroupName: string): Promise { + const parameterName = this.parameterName(runnerGroupName); + logger.debug('Reading runner group ID from cache', { + runnerGroupName, + parameterName, + }); + try { - const value = await getParameter(this.parameterName(runnerGroupName)); + const value = await getParameter(parameterName); const runnerGroupId = Number.parseInt(value, 10); if (Number.isNaN(runnerGroupId)) { throw new Error(`Cached runner group ID for ${runnerGroupName} is invalid`); } + + logger.debug('Runner group cache hit', { + runnerGroupName, + parameterName, + runnerGroupId, + }); return runnerGroupId; } catch (error) { if (isParameterNotFoundError(error)) { + logger.info('Runner group cache miss; caller will resolve the ID from GitHub', { + runnerGroupName, + parameterName, + errorNames: getErrorNames(error), + }); return undefined; } + + logger.error('Runner group cache lookup failed', { + runnerGroupName, + parameterName, + errorNames: getErrorNames(error), + }); throw error; } } async create(record: RunnerGroupCacheRecord): Promise { - await putParameter(this.parameterName(record.runnerGroupName), record.runnerGroupId.toString(), false, { + const parameterName = this.parameterName(record.runnerGroupName); + await putParameter(parameterName, record.runnerGroupId.toString(), false, { tags: [...this.config.parameterStoreTags], }); + logger.info('Stored runner group ID in cache', { + runnerGroupName: record.runnerGroupName, + parameterName, + runnerGroupId: record.runnerGroupId, + }); } private parameterName(runnerGroupName: string): string { @@ -60,16 +92,5 @@ class AwsSsmRunnerGroupCacheStore implements RunnerGroupCacheStore { } function isParameterNotFoundError(error: unknown): boolean { - const seen = new Set(); - let current: unknown = error; - - while (current !== null && typeof current === 'object' && !seen.has(current)) { - seen.add(current); - if ('name' in current && current.name === 'ParameterNotFound') { - return true; - } - current = 'cause' in current ? current.cause : undefined; - } - - return false; + return getErrorNames(error).includes('ParameterNotFound'); }