Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lambdas/functions/webhook/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
},
"dependencies": {
"@aws-github-runner/aws-powertools-util": "*",
"@aws-github-runner/aws-ssm-util": "*",
"@aws-github-runner/compute-providers": "*",
"@aws-github-runner/storage-providers": "*",
"@aws-sdk/client-sqs": "^3.1009.0",
"@middy/core": "^6.4.5",
"@octokit/rest": "22.0.1",
Expand Down
205 changes: 65 additions & 140 deletions lambdas/functions/webhook/src/ConfigLoader.test.ts

Large diffs are not rendered by default.

63 changes: 16 additions & 47 deletions lambdas/functions/webhook/src/ConfigLoader.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { getParameter, getParameters } from '@aws-github-runner/aws-ssm-util';
import { getGitHubWebhookSecretStore, getRunnerMatcherConfigStore } from '@aws-github-runner/storage-providers';
import { RunnerMatcherConfig } from './sqs';
import { logger } from '@aws-github-runner/aws-powertools-util';

/**
* Base class for loading configuration from environment variables and SSM parameters.
* Base class for loading configuration from environment variables and configuration stores.
*
* @remarks
* To avoid usages or checking values can be undefined we assume that configuration is
Expand Down Expand Up @@ -54,19 +54,15 @@ abstract class BaseConfig {
}
}

protected async loadParameter(paramPath: string, propertyName: keyof this): Promise<void> {
logger.debug(`Loading parameter for ${String(propertyName)} from path ${paramPath}`);
await getParameter(paramPath)
.then((value) => {
this.loadProperty(propertyName, value);
})
.catch((error) => {
const errorMessage = `Failed to load parameter for ${String(propertyName)} from path ${paramPath}: ${(error as Error).message}`;
this.configLoadingErrors.push(errorMessage);
});
protected async loadStoredProperty(propertyName: keyof this, getValue: () => Promise<string>): Promise<void> {
try {
this.loadProperty(propertyName, await getValue());
} catch (error) {
this.configLoadingErrors.push((error as Error).message);
}
}

private loadProperty(propertyName: keyof this, value: string) {
protected loadProperty(propertyName: keyof this, value: string) {
try {
this[propertyName] = JSON.parse(value) as unknown as this[keyof this];
} catch {
Expand Down Expand Up @@ -96,38 +92,11 @@ abstract class MatcherAwareConfig extends BaseConfig {
// across the matching queues to avoid concentrating load on a single one.
queueSelectionStrategy: QueueSelectionStrategy = 'first';

protected async loadMatcherConfig(paramPathsEnv: string) {
if (!paramPathsEnv || paramPathsEnv === 'undefined' || paramPathsEnv === 'null' || !paramPathsEnv.includes(':')) {
// Single path or invalid string → load directly
await this.loadParameter(paramPathsEnv, 'matcherConfig');
return;
}

const paths = paramPathsEnv
.split(':')
.map((p) => p.trim())
.filter(Boolean);

// Batch fetch all matcher config paths in a single SSM API call
protected async loadMatcherConfig() {
try {
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);
}
this.loadProperty('matcherConfig', await getRunnerMatcherConfigStore().get());
} catch (error) {
this.configLoadingErrors.push(`Failed to load/parse combined matcher config: ${(error as Error).message}`);
this.configLoadingErrors.push((error as Error).message);
}
}
}
Expand All @@ -142,8 +111,8 @@ export class ConfigWebhook extends MatcherAwareConfig {
this.loadEnvVar(process.env.QUEUE_SELECTION_STRATEGY, 'queueSelectionStrategy', 'first');

await Promise.all([
this.loadMatcherConfig(process.env.PARAMETER_RUNNER_MATCHER_CONFIG_PATH),
this.loadParameter(process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET, 'webhookSecret'),
this.loadMatcherConfig(),
this.loadStoredProperty('webhookSecret', () => getGitHubWebhookSecretStore().get()),
]);

validateWebhookSecret(this);
Expand All @@ -160,7 +129,7 @@ export class ConfigWebhookEventBridge extends BaseConfig {
async loadConfig(): Promise<void> {
this.loadEnvVar(process.env.ACCEPT_EVENTS, 'allowedEvents', []);
this.loadEnvVar(process.env.EVENT_BUS_NAME, 'eventBusName');
await this.loadParameter(process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET, 'webhookSecret');
await this.loadStoredProperty('webhookSecret', () => getGitHubWebhookSecretStore().get());

validateEventBusName(this);
validateWebhookSecret(this);
Expand All @@ -174,7 +143,7 @@ export class ConfigDispatcher extends MatcherAwareConfig {
async loadConfig(): Promise<void> {
this.loadEnvVar(process.env.REPOSITORY_ALLOW_LIST, 'repositoryAllowList', []);
this.loadEnvVar(process.env.QUEUE_SELECTION_STRATEGY, 'queueSelectionStrategy', 'first');
await this.loadMatcherConfig(process.env.PARAMETER_RUNNER_MATCHER_CONFIG_PATH);
await this.loadMatcherConfig();

validateRunnerMatcherConfig(this);
validateQueueSelectionStrategy(this);
Expand Down
26 changes: 20 additions & 6 deletions lambdas/functions/webhook/src/lambda.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import { WorkflowJobEvent } from '@octokit/webhooks-types';
import { dispatchToRunners, eventBridgeWebhook, directWebhook } from './lambda';
import { publishForRunners, publishOnEventBridge } from './webhook';
import ValidationError from './ValidationError';
import { getParameter } from '@aws-github-runner/aws-ssm-util';
import {
getGitHubWebhookSecretStore,
getRunnerMatcherConfigStore,
type GitHubWebhookSecretStore,
type RunnerMatcherConfigStore,
} from '@aws-github-runner/storage-providers';
import { dispatch } from './runners/dispatch';
import { EventWrapper } from './types';
import { describe, it, expect, beforeEach, vi } from 'vitest';
Expand Down Expand Up @@ -79,15 +84,24 @@ const context: Context = {

vi.mock('./runners/dispatch');
vi.mock('./webhook');
vi.mock('@aws-github-runner/aws-ssm-util');
vi.mock('@aws-github-runner/storage-providers');

const githubWebhookSecretStore = {
get: vi.fn(),
} satisfies GitHubWebhookSecretStore;
const runnerMatcherConfigStore = {
get: vi.fn(),
} satisfies RunnerMatcherConfigStore;

describe('Test webhook lambda wrapper.', () => {
beforeEach(() => {
// We mock all SSM request to resolve to a non empty array. Since we mock all implemeantions
// relying on the config object that is enough to test the handlers.
const mockedGet = vi.mocked(getParameter);
mockedGet.mockResolvedValue('["abc"]');
vi.clearAllMocks();
// The handlers only need non-empty config values because their downstream
// implementations are mocked in this wrapper test.
vi.mocked(getGitHubWebhookSecretStore).mockReturnValue(githubWebhookSecretStore);
githubWebhookSecretStore.get.mockResolvedValue('["abc"]');
vi.mocked(getRunnerMatcherConfigStore).mockReturnValue(runnerMatcherConfigStore);
runnerMatcherConfigStore.get.mockResolvedValue('["abc"]');
});

describe('Test webhook lambda wrapper.', () => {
Expand Down
2 changes: 0 additions & 2 deletions lambdas/functions/webhook/src/modules.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ declare namespace NodeJS {
export interface ProcessEnv {
ENVIRONMENT: string;
EVENT_BUS_NAME: string;
PARAMETER_GITHUB_APP_WEBHOOK_SECRET: string;
PARAMETER_RUNNER_MATCHER_CONFIG_PATH: string;
QUEUE_SELECTION_STRATEGY: string;
REPOSITORY_ALLOW_LIST: string;
RUNNER_LABELS: string;
Expand Down
26 changes: 10 additions & 16 deletions lambdas/functions/webhook/src/runners/dispatch.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getParameter } from '@aws-github-runner/aws-ssm-util';
import { selectDynamicLabelQueue } from '@aws-github-runner/compute-providers/webhook';
import { getRunnerMatcherConfigStore, type RunnerMatcherConfigStore } from '@aws-github-runner/storage-providers';

import nock from 'nock';
import { WorkflowJobEvent } from '@octokit/webhooks-types';
Expand All @@ -14,12 +14,14 @@ import { logger } from '@aws-github-runner/aws-powertools-util';
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';

vi.mock('../sqs');
vi.mock('@aws-github-runner/aws-ssm-util');
vi.mock('@aws-github-runner/storage-providers');
vi.mock('@aws-github-runner/compute-providers/webhook', () => ({
selectDynamicLabelQueue: vi.fn(),
}));

const GITHUB_APP_WEBHOOK_SECRET = 'TEST_SECRET';
const runnerMatcherConfigStore = {
get: vi.fn(),
} satisfies RunnerMatcherConfigStore;

const cleanEnv = process.env;

Expand All @@ -37,7 +39,6 @@ describe('Dispatcher', () => {
vi.clearAllMocks();
vi.resetAllMocks();

mockSSMResponse();
config = await createConfig(undefined, runnerConfig);
});

Expand Down Expand Up @@ -242,7 +243,7 @@ describe('Dispatcher', () => {
it('rejects an invalid strategy at config load', async () => {
process.env.QUEUE_SELECTION_STRATEGY = 'bogus';
ConfigDispatcher.reset();
mockSSMResponse(twoExactMatches);
mockMatcherConfigResponse(twoExactMatches);
await expect(ConfigDispatcher.load()).rejects.toThrow(/queue selection strategy/i);
});
});
Expand Down Expand Up @@ -394,23 +395,16 @@ describe('Dispatcher', () => {
});
});

function mockSSMResponse(runnerConfigInput?: RunnerConfig) {
process.env.PARAMETER_RUNNER_MATCHER_CONFIG_PATH = '/github-runner/runner-matcher-config';
const mockedGet = vi.mocked(getParameter);
mockedGet.mockImplementation((parameter_name) => {
const value =
parameter_name == '/github-runner/runner-matcher-config'
? JSON.stringify(runnerConfigInput ?? runnerConfig)
: GITHUB_APP_WEBHOOK_SECRET;
return Promise.resolve(value);
});
function mockMatcherConfigResponse(runnerConfigInput?: RunnerConfig) {
vi.mocked(getRunnerMatcherConfigStore).mockReturnValue(runnerMatcherConfigStore);
runnerMatcherConfigStore.get.mockResolvedValue(JSON.stringify(runnerConfigInput ?? runnerConfig));
}

async function createConfig(repositoryAllowList?: string[], runnerConfig?: RunnerConfig): Promise<ConfigDispatcher> {
if (repositoryAllowList) {
process.env.REPOSITORY_ALLOW_LIST = JSON.stringify(repositoryAllowList);
}
ConfigDispatcher.reset();
mockSSMResponse(runnerConfig);
mockMatcherConfigResponse(runnerConfig);
return await ConfigDispatcher.load();
}
34 changes: 19 additions & 15 deletions lambdas/functions/webhook/src/webhook/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { Webhooks } from '@octokit/webhooks';
import { getParameter } from '@aws-github-runner/aws-ssm-util';
import {
getGitHubWebhookSecretStore,
getRunnerMatcherConfigStore,
type GitHubWebhookSecretStore,
type RunnerMatcherConfigStore,
} from '@aws-github-runner/storage-providers';

import nock from 'nock';

Expand All @@ -15,9 +20,15 @@ import { describe, it, expect, beforeEach, vi } from 'vitest';
vi.mock('../sqs');
vi.mock('../eventbridge');
vi.mock('../runners/dispatch');
vi.mock('@aws-github-runner/aws-ssm-util');
vi.mock('@aws-github-runner/storage-providers');

const GITHUB_APP_WEBHOOK_SECRET = 'TEST_SECRET';
const githubWebhookSecretStore = {
get: vi.fn(),
} satisfies GitHubWebhookSecretStore;
const runnerMatcherConfigStore = {
get: vi.fn(),
} satisfies RunnerMatcherConfigStore;

const cleanEnv = process.env;

Expand All @@ -32,7 +43,7 @@ describe('handle GitHub webhook events', () => {
nock.disableNetConnect();
vi.clearAllMocks();

mockSSMResponse();
mockConfigResponse();
});

describe('handle and dispatch webhook events to build queues', () => {
Expand Down Expand Up @@ -284,9 +295,7 @@ describe('Check message size (checkBodySize)', () => {
});
});

function mockSSMResponse() {
process.env.PARAMETER_RUNNER_MATCHER_CONFIG_PATH = '/path/to/matcher/config';
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = '/path/to/webhook/secret';
function mockConfigResponse() {
const matcherConfig = [
{
id: '1',
Expand All @@ -297,13 +306,8 @@ function mockSSMResponse() {
},
},
];
vi.mocked(getParameter).mockImplementation(async (paramPath: string) => {
if (paramPath === '/path/to/matcher/config') {
return JSON.stringify(matcherConfig);
}
if (paramPath === '/path/to/webhook/secret') {
return GITHUB_APP_WEBHOOK_SECRET;
}
throw new Error('Parameter not found');
});
vi.mocked(getGitHubWebhookSecretStore).mockReturnValue(githubWebhookSecretStore);
vi.mocked(getRunnerMatcherConfigStore).mockReturnValue(runnerMatcherConfigStore);
githubWebhookSecretStore.get.mockResolvedValue(GITHUB_APP_WEBHOOK_SECRET);
runnerMatcherConfigStore.get.mockResolvedValue(JSON.stringify(matcherConfig));
}
2 changes: 2 additions & 0 deletions lambdas/libs/storage-providers/aws/ssm/environment.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ export {};
declare global {
namespace NodeJS {
interface ProcessEnv {
PARAMETER_GITHUB_APP_WEBHOOK_SECRET?: string;
PARAMETER_RUNNER_MATCHER_CONFIG_PATH?: string;
SSM_PARAMETER_STORE_TAGS?: string;
SSM_CONFIG_PATH?: string;
SSM_TOKEN_PATH?: string;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { getParameter } from '@aws-github-runner/aws-ssm-util';
import { beforeEach, describe, expect, it, vi } from 'vitest';

import { createAwsSsmGitHubWebhookSecretStore } from './github-webhook-secret-store';

vi.mock('@aws-github-runner/aws-ssm-util', () => ({
getParameter: vi.fn(),
}));

const getParameterMock = vi.mocked(getParameter);
const cleanEnv = process.env;
const webhookSecretParameter = '/actions-runner/test/webhook_secret';

describe('aws_ssm GitHub webhook secret store', () => {
beforeEach(() => {
vi.clearAllMocks();
process.env = { ...cleanEnv };
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = webhookSecretParameter;
});

it('loads the webhook secret parameter', async () => {
getParameterMock.mockResolvedValue('fake-webhook-secret');
const store = createAwsSsmGitHubWebhookSecretStore();

await expect(store.get()).resolves.toBe('fake-webhook-secret');
expect(getParameterMock).toHaveBeenCalledOnce();
expect(getParameterMock).toHaveBeenCalledWith(webhookSecretParameter);
});

it('wraps a parameter read failure with the legacy error message', async () => {
getParameterMock.mockRejectedValue(new Error('access denied'));
const store = createAwsSsmGitHubWebhookSecretStore();

await expect(store.get()).rejects.toThrow(
`Failed to load parameter for webhookSecret from path ${webhookSecretParameter}: access denied`,
);
});

it.each([undefined, '', ' '])('requires a webhook secret parameter path for input %j', (parameterPath) => {
if (parameterPath === undefined) {
delete process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET;
} else {
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = parameterPath;
}

expect(() => createAwsSsmGitHubWebhookSecretStore()).toThrow(
'Environment variable PARAMETER_GITHUB_APP_WEBHOOK_SECRET is not set',
);
expect(getParameterMock).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { getParameter } from '@aws-github-runner/aws-ssm-util';

import type { GitHubWebhookSecretStore } from '../../core';
import type {} from './environment';

export function createAwsSsmGitHubWebhookSecretStore(): GitHubWebhookSecretStore {
const parameterPath = process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET;
if (!parameterPath || parameterPath.trim() === '') {
throw new Error('Environment variable PARAMETER_GITHUB_APP_WEBHOOK_SECRET is not set');
}

return new AwsSsmGitHubWebhookSecretStore(parameterPath);
}

class AwsSsmGitHubWebhookSecretStore implements GitHubWebhookSecretStore {
constructor(private readonly parameterPath: string) {}

async get(): Promise<string> {
try {
return await getParameter(this.parameterPath);
} catch (error) {
throw new Error(
`Failed to load parameter for webhookSecret from path ${this.parameterPath}: ${(error as Error).message}`,
);
}
}
}
Loading