Skip to content

Commit 0150fb2

Browse files
edersonbrilhanteedersonbrilhante
committed
refactor(storage): extract webhook secret store
1 parent 54403da commit 0150fb2

15 files changed

Lines changed: 257 additions & 56 deletions

‎lambdas/functions/webhook/package.json‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
},
3030
"dependencies": {
3131
"@aws-github-runner/aws-powertools-util": "*",
32-
"@aws-github-runner/aws-ssm-util": "*",
3332
"@aws-github-runner/compute-providers": "*",
3433
"@aws-github-runner/storage-providers": "*",
3534
"@aws-sdk/client-sqs": "^3.1009.0",

‎lambdas/functions/webhook/src/ConfigLoader.test.ts‎

Lines changed: 33 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,20 @@
1-
import { getParameter } from '@aws-github-runner/aws-ssm-util';
2-
import { getRunnerMatcherConfigStore, type RunnerMatcherConfigStore } from '@aws-github-runner/storage-providers';
1+
import {
2+
getGitHubWebhookSecretStore,
3+
getRunnerMatcherConfigStore,
4+
type GitHubWebhookSecretStore,
5+
type RunnerMatcherConfigStore,
6+
} from '@aws-github-runner/storage-providers';
37
import { ConfigWebhook, ConfigWebhookEventBridge, ConfigDispatcher } from './ConfigLoader';
48

59
import { logger } from '@aws-github-runner/aws-powertools-util';
610
import { RunnerMatcherConfig } from './sqs';
711
import { describe, it, expect, beforeEach, vi } from 'vitest';
812

9-
vi.mock('@aws-github-runner/aws-ssm-util');
1013
vi.mock('@aws-github-runner/storage-providers');
1114

15+
const githubWebhookSecretStore = {
16+
get: vi.fn(),
17+
} satisfies GitHubWebhookSecretStore;
1218
const runnerMatcherConfigStore = {
1319
get: vi.fn(),
1420
} satisfies RunnerMatcherConfigStore;
@@ -20,6 +26,7 @@ describe('ConfigLoader Tests', () => {
2026
ConfigWebhookEventBridge.reset();
2127
ConfigDispatcher.reset();
2228
logger.setLogLevel('DEBUG');
29+
vi.mocked(getGitHubWebhookSecretStore).mockReturnValue(githubWebhookSecretStore);
2330
vi.mocked(getRunnerMatcherConfigStore).mockReturnValue(runnerMatcherConfigStore);
2431

2532
// clear process.env
@@ -31,7 +38,6 @@ describe('ConfigLoader Tests', () => {
3138
describe('Check base object', () => {
3239
function setupConfiguration(): void {
3340
process.env.EVENT_BUS_NAME = 'event-bus';
34-
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = '/path/to/webhook/secret';
3541
const matcherConfig = [
3642
{
3743
id: '1',
@@ -43,7 +49,7 @@ describe('ConfigLoader Tests', () => {
4349
},
4450
];
4551
runnerMatcherConfigStore.get.mockResolvedValue(JSON.stringify(matcherConfig));
46-
vi.mocked(getParameter).mockResolvedValue('secret');
52+
githubWebhookSecretStore.get.mockResolvedValue('secret');
4753
}
4854

4955
it('should return the same instance of ConfigWebhook (singleton)', async () => {
@@ -52,7 +58,7 @@ describe('ConfigLoader Tests', () => {
5258
const config2 = await ConfigWebhook.load();
5359

5460
expect(config1).toBe(config2);
55-
expect(getParameter).toHaveBeenCalledOnce();
61+
expect(githubWebhookSecretStore.get).toHaveBeenCalledOnce();
5662
expect(runnerMatcherConfigStore.get).toHaveBeenCalledOnce();
5763
});
5864

@@ -62,7 +68,7 @@ describe('ConfigLoader Tests', () => {
6268
const config2 = await ConfigWebhookEventBridge.load();
6369

6470
expect(config1).toBe(config2);
65-
expect(getParameter).toHaveBeenCalledTimes(1);
71+
expect(githubWebhookSecretStore.get).toHaveBeenCalledOnce();
6672
expect(runnerMatcherConfigStore.get).not.toHaveBeenCalled();
6773
});
6874

@@ -72,7 +78,7 @@ describe('ConfigLoader Tests', () => {
7278
const config2 = await ConfigDispatcher.load();
7379

7480
expect(config1).toBe(config2);
75-
expect(getParameter).not.toHaveBeenCalled();
81+
expect(githubWebhookSecretStore.get).not.toHaveBeenCalled();
7682
expect(runnerMatcherConfigStore.get).toHaveBeenCalledOnce();
7783
});
7884

@@ -96,7 +102,6 @@ describe('ConfigLoader Tests', () => {
96102
describe('ConfigWebhook', () => {
97103
it('should load config successfully', async () => {
98104
process.env.REPOSITORY_ALLOW_LIST = '["repo1", "repo2"]';
99-
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = '/path/to/webhook/secret';
100105
const matcherConfig = [
101106
{
102107
id: '1',
@@ -108,7 +113,7 @@ describe('ConfigLoader Tests', () => {
108113
},
109114
];
110115
runnerMatcherConfigStore.get.mockResolvedValue(JSON.stringify(matcherConfig));
111-
vi.mocked(getParameter).mockResolvedValue('secret');
116+
githubWebhookSecretStore.get.mockResolvedValue('secret');
112117

113118
const config: ConfigWebhook = await ConfigWebhook.load();
114119

@@ -118,7 +123,6 @@ describe('ConfigLoader Tests', () => {
118123
});
119124

120125
it('should load config successfully', async () => {
121-
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = '/path/to/webhook/secret';
122126
const matcherConfig = [
123127
{
124128
id: '1',
@@ -130,7 +134,7 @@ describe('ConfigLoader Tests', () => {
130134
},
131135
];
132136
runnerMatcherConfigStore.get.mockResolvedValue(JSON.stringify(matcherConfig));
133-
vi.mocked(getParameter).mockResolvedValue('secret');
137+
githubWebhookSecretStore.get.mockResolvedValue('secret');
134138

135139
const config: ConfigWebhook = await ConfigWebhook.load();
136140

@@ -146,22 +150,20 @@ describe('ConfigLoader Tests', () => {
146150
'Failed to load parameter for matcherConfig from path /path/to/matcher/config: Failed to load matcher config',
147151
),
148152
);
149-
vi.mocked(getParameter).mockResolvedValue('');
153+
githubWebhookSecretStore.get.mockResolvedValue('');
150154

151155
await expect(ConfigWebhook.load()).rejects.toThrow(
152156
'Failed to load config: Failed to load parameter for matcherConfig from path /path/to/matcher/config: Failed to load matcher config',
153157
);
154158
});
155159

156160
it('should load combined matcher config returned by the store', async () => {
157-
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = '/path/to/webhook/secret';
158-
159161
const combinedMatcherConfig = [
160162
{ id: '1', arn: 'arn:aws:sqs:queue1', matcherConfig: { labelMatchers: [['a']], exactMatch: true } },
161163
{ id: '2', arn: 'arn:aws:sqs:queue2', matcherConfig: { labelMatchers: [['b']], exactMatch: true } },
162164
];
163165
runnerMatcherConfigStore.get.mockResolvedValue(JSON.stringify(combinedMatcherConfig));
164-
vi.mocked(getParameter).mockResolvedValue('secret');
166+
githubWebhookSecretStore.get.mockResolvedValue('secret');
165167

166168
const config: ConfigWebhook = await ConfigWebhook.load();
167169

@@ -170,13 +172,12 @@ describe('ConfigLoader Tests', () => {
170172
});
171173

172174
it('should propagate an error from the matcher config store', async () => {
173-
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = '/path/to/webhook/secret';
174175
runnerMatcherConfigStore.get.mockRejectedValue(
175176
new Error(
176177
"Failed to load/parse combined matcher config: Expected ',' or ']' after array element in JSON at position 196",
177178
),
178179
);
179-
vi.mocked(getParameter).mockResolvedValue('secret');
180+
githubWebhookSecretStore.get.mockResolvedValue('secret');
180181

181182
await expect(ConfigWebhook.load()).rejects.toThrow(
182183
"Failed to load config: Failed to load/parse combined matcher config: Expected ',' or ']' after array element in JSON at position 196",
@@ -188,14 +189,7 @@ describe('ConfigLoader Tests', () => {
188189
it('should load config successfully', async () => {
189190
process.env.ACCEPT_EVENTS = '["push", "pull_request"]';
190191
process.env.EVENT_BUS_NAME = 'event-bus';
191-
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = '/path/to/webhook/secret';
192-
193-
vi.mocked(getParameter).mockImplementation(async (paramPath: string) => {
194-
if (paramPath === '/path/to/webhook/secret') {
195-
return 'secret';
196-
}
197-
return '';
198-
});
192+
githubWebhookSecretStore.get.mockResolvedValue('secret');
199193

200194
const config: ConfigWebhookEventBridge = await ConfigWebhookEventBridge.load();
201195

@@ -206,13 +200,23 @@ describe('ConfigLoader Tests', () => {
206200
});
207201

208202
it('should throw error if config loading fails', async () => {
209-
vi.mocked(getParameter).mockImplementation(async (paramPath: string) => {
210-
throw new Error(`Parameter ${paramPath} not found`);
203+
githubWebhookSecretStore.get.mockRejectedValue(new Error('Webhook secret store is unavailable'));
204+
205+
await expect(ConfigWebhookEventBridge.load()).rejects.toThrow(
206+
'Failed to load config: Environment variable for eventBusName is not set and no default value provided., Webhook secret store is unavailable',
207+
);
208+
});
209+
210+
it('should report an error selecting the webhook secret store', async () => {
211+
process.env.EVENT_BUS_NAME = 'event-bus';
212+
vi.mocked(getGitHubWebhookSecretStore).mockImplementationOnce(() => {
213+
throw new Error("Unsupported runner config storage provider 'not-registered'");
211214
});
212215

213216
await expect(ConfigWebhookEventBridge.load()).rejects.toThrow(
214-
'Failed to load config: Environment variable for eventBusName is not set and no default value provided., Failed to load parameter for webhookSecret from path undefined: Parameter undefined not found',
217+
"Failed to load config: Unsupported runner config storage provider 'not-registered'",
215218
);
219+
expect(githubWebhookSecretStore.get).not.toHaveBeenCalled();
216220
});
217221
});
218222

‎lambdas/functions/webhook/src/ConfigLoader.ts‎

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
1-
import { getParameter } from '@aws-github-runner/aws-ssm-util';
2-
import { getRunnerMatcherConfigStore } from '@aws-github-runner/storage-providers';
1+
import { getGitHubWebhookSecretStore, getRunnerMatcherConfigStore } from '@aws-github-runner/storage-providers';
32
import { RunnerMatcherConfig } from './sqs';
43
import { logger } from '@aws-github-runner/aws-powertools-util';
54

65
/**
7-
* Base class for loading configuration from environment variables and SSM parameters.
6+
* Base class for loading configuration from environment variables and configuration stores.
87
*
98
* @remarks
109
* To avoid usages or checking values can be undefined we assume that configuration is
@@ -55,16 +54,12 @@ abstract class BaseConfig {
5554
}
5655
}
5756

58-
protected async loadParameter(paramPath: string, propertyName: keyof this): Promise<void> {
59-
logger.debug(`Loading parameter for ${String(propertyName)} from path ${paramPath}`);
60-
await getParameter(paramPath)
61-
.then((value) => {
62-
this.loadProperty(propertyName, value);
63-
})
64-
.catch((error) => {
65-
const errorMessage = `Failed to load parameter for ${String(propertyName)} from path ${paramPath}: ${(error as Error).message}`;
66-
this.configLoadingErrors.push(errorMessage);
67-
});
57+
protected async loadStoredProperty(propertyName: keyof this, getValue: () => Promise<string>): Promise<void> {
58+
try {
59+
this.loadProperty(propertyName, await getValue());
60+
} catch (error) {
61+
this.configLoadingErrors.push((error as Error).message);
62+
}
6863
}
6964

7065
protected loadProperty(propertyName: keyof this, value: string) {
@@ -117,7 +112,7 @@ export class ConfigWebhook extends MatcherAwareConfig {
117112

118113
await Promise.all([
119114
this.loadMatcherConfig(),
120-
this.loadParameter(process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET, 'webhookSecret'),
115+
this.loadStoredProperty('webhookSecret', () => getGitHubWebhookSecretStore().get()),
121116
]);
122117

123118
validateWebhookSecret(this);
@@ -134,7 +129,7 @@ export class ConfigWebhookEventBridge extends BaseConfig {
134129
async loadConfig(): Promise<void> {
135130
this.loadEnvVar(process.env.ACCEPT_EVENTS, 'allowedEvents', []);
136131
this.loadEnvVar(process.env.EVENT_BUS_NAME, 'eventBusName');
137-
await this.loadParameter(process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET, 'webhookSecret');
132+
await this.loadStoredProperty('webhookSecret', () => getGitHubWebhookSecretStore().get());
138133

139134
validateEventBusName(this);
140135
validateWebhookSecret(this);

‎lambdas/functions/webhook/src/lambda.test.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,12 @@ import { WorkflowJobEvent } from '@octokit/webhooks-types';
66
import { dispatchToRunners, eventBridgeWebhook, directWebhook } from './lambda';
77
import { publishForRunners, publishOnEventBridge } from './webhook';
88
import ValidationError from './ValidationError';
9-
import { getParameter } from '@aws-github-runner/aws-ssm-util';
10-
import { getRunnerMatcherConfigStore, type RunnerMatcherConfigStore } from '@aws-github-runner/storage-providers';
9+
import {
10+
getGitHubWebhookSecretStore,
11+
getRunnerMatcherConfigStore,
12+
type GitHubWebhookSecretStore,
13+
type RunnerMatcherConfigStore,
14+
} from '@aws-github-runner/storage-providers';
1115
import { dispatch } from './runners/dispatch';
1216
import { EventWrapper } from './types';
1317
import { describe, it, expect, beforeEach, vi } from 'vitest';
@@ -80,9 +84,11 @@ const context: Context = {
8084

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

89+
const githubWebhookSecretStore = {
90+
get: vi.fn(),
91+
} satisfies GitHubWebhookSecretStore;
8692
const runnerMatcherConfigStore = {
8793
get: vi.fn(),
8894
} satisfies RunnerMatcherConfigStore;
@@ -92,7 +98,8 @@ describe('Test webhook lambda wrapper.', () => {
9298
vi.clearAllMocks();
9399
// The handlers only need non-empty config values because their downstream
94100
// implementations are mocked in this wrapper test.
95-
vi.mocked(getParameter).mockResolvedValue('["abc"]');
101+
vi.mocked(getGitHubWebhookSecretStore).mockReturnValue(githubWebhookSecretStore);
102+
githubWebhookSecretStore.get.mockResolvedValue('["abc"]');
96103
vi.mocked(getRunnerMatcherConfigStore).mockReturnValue(runnerMatcherConfigStore);
97104
runnerMatcherConfigStore.get.mockResolvedValue('["abc"]');
98105
});

‎lambdas/functions/webhook/src/modules.d.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ declare namespace NodeJS {
22
export interface ProcessEnv {
33
ENVIRONMENT: string;
44
EVENT_BUS_NAME: string;
5-
PARAMETER_GITHUB_APP_WEBHOOK_SECRET: string;
65
QUEUE_SELECTION_STRATEGY: string;
76
REPOSITORY_ALLOW_LIST: string;
87
RUNNER_LABELS: string;

‎lambdas/functions/webhook/src/webhook/index.test.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
import { Webhooks } from '@octokit/webhooks';
2-
import { getParameter } from '@aws-github-runner/aws-ssm-util';
3-
import { getRunnerMatcherConfigStore, type RunnerMatcherConfigStore } from '@aws-github-runner/storage-providers';
2+
import {
3+
getGitHubWebhookSecretStore,
4+
getRunnerMatcherConfigStore,
5+
type GitHubWebhookSecretStore,
6+
type RunnerMatcherConfigStore,
7+
} from '@aws-github-runner/storage-providers';
48

59
import nock from 'nock';
610

@@ -16,10 +20,12 @@ import { describe, it, expect, beforeEach, vi } from 'vitest';
1620
vi.mock('../sqs');
1721
vi.mock('../eventbridge');
1822
vi.mock('../runners/dispatch');
19-
vi.mock('@aws-github-runner/aws-ssm-util');
2023
vi.mock('@aws-github-runner/storage-providers');
2124

2225
const GITHUB_APP_WEBHOOK_SECRET = 'TEST_SECRET';
26+
const githubWebhookSecretStore = {
27+
get: vi.fn(),
28+
} satisfies GitHubWebhookSecretStore;
2329
const runnerMatcherConfigStore = {
2430
get: vi.fn(),
2531
} satisfies RunnerMatcherConfigStore;
@@ -290,7 +296,6 @@ describe('Check message size (checkBodySize)', () => {
290296
});
291297

292298
function mockConfigResponse() {
293-
process.env.PARAMETER_GITHUB_APP_WEBHOOK_SECRET = '/path/to/webhook/secret';
294299
const matcherConfig = [
295300
{
296301
id: '1',
@@ -301,7 +306,8 @@ function mockConfigResponse() {
301306
},
302307
},
303308
];
309+
vi.mocked(getGitHubWebhookSecretStore).mockReturnValue(githubWebhookSecretStore);
304310
vi.mocked(getRunnerMatcherConfigStore).mockReturnValue(runnerMatcherConfigStore);
311+
githubWebhookSecretStore.get.mockResolvedValue(GITHUB_APP_WEBHOOK_SECRET);
305312
runnerMatcherConfigStore.get.mockResolvedValue(JSON.stringify(matcherConfig));
306-
vi.mocked(getParameter).mockResolvedValue(GITHUB_APP_WEBHOOK_SECRET);
307313
}

‎lambdas/libs/storage-providers/aws/ssm/environment.d.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ declare global {
66
PARAMETER_GITHUB_APP_ID_NAME?: string;
77
PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME?: string;
88
PARAMETER_GITHUB_APP_KEY_BASE64_NAME?: string;
9+
PARAMETER_GITHUB_APP_WEBHOOK_SECRET?: string;
910
PARAMETER_RUNNER_MATCHER_CONFIG_PATH?: string;
1011
SSM_CONFIG_PATH?: string;
1112
SSM_CLEANUP_CONFIG?: string;

0 commit comments

Comments
 (0)