Skip to content

Commit a53715b

Browse files
edersonbrilhanteedersonbrilhante
committed
refactor(storage): hide provider selection behind facades
1 parent 7ef38bd commit a53715b

32 files changed

Lines changed: 487 additions & 576 deletions

‎lambdas/functions/control-plane/src/control-plane-providers.ts‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,8 @@
11
import { createControlPlaneProviderRegistry } from '@aws-github-runner/compute-providers/control-plane';
22
import { computeProviderTypes } from '@aws-github-runner/compute-providers/provider-types';
3-
import type { CreateStartRunnerConfig } from '@aws-github-runner/compute-providers/core';
4-
import type { StorageProviderType } from '@aws-github-runner/storage-providers';
53

64
import { createStartRunnerConfig } from './scale-runners/github-runner';
75

8-
const providerRegistries = new Map<StorageProviderType, ReturnType<typeof createControlPlaneProviderRegistry>>();
9-
10-
export function getControlPlaneProviderRegistry(storageProviderType: StorageProviderType) {
11-
let registry = providerRegistries.get(storageProviderType);
12-
if (!registry) {
13-
const boundCreateStartRunnerConfig: CreateStartRunnerConfig = (...args) =>
14-
createStartRunnerConfig(storageProviderType, ...args);
15-
registry = createControlPlaneProviderRegistry(boundCreateStartRunnerConfig);
16-
providerRegistries.set(storageProviderType, registry);
17-
}
18-
return registry;
19-
}
6+
export const controlPlaneProviderRegistry = createControlPlaneProviderRegistry(createStartRunnerConfig);
207

218
export { computeProviderTypes };

‎lambdas/functions/control-plane/src/github/auth.test.ts‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ const ENVIRONMENT = 'dev';
3434
const GITHUB_APP_ID = '1';
3535
const PARAMETER_GITHUB_APP_ID_NAME = `/actions-runner/${ENVIRONMENT}/github_app_id`;
3636
const PARAMETER_GITHUB_APP_KEY_BASE64_NAME = `/actions-runner/${ENVIRONMENT}/github_app_key_base64`;
37-
const storageProviderType = 'aws_ssm' as const;
3837

3938
const mockedGetParameters = vi.mocked(getParameters);
4039

@@ -91,7 +90,7 @@ describe('Test createGithubAppAuth', () => {
9190
it('Throws early when PARAMETER_GITHUB_APP_ID_NAME is not set', async () => {
9291
delete process.env.PARAMETER_GITHUB_APP_ID_NAME;
9392

94-
await expect(createGithubAppAuth(storageProviderType, installationId)).rejects.toThrow(
93+
await expect(createGithubAppAuth(installationId)).rejects.toThrow(
9594
'Environment variable PARAMETER_GITHUB_APP_ID_NAME is not set',
9695
);
9796
expect(mockedGetParameters).not.toHaveBeenCalled();
@@ -100,7 +99,7 @@ describe('Test createGithubAppAuth', () => {
10099
it('Throws early when PARAMETER_GITHUB_APP_KEY_BASE64_NAME is not set', async () => {
101100
delete process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME;
102101

103-
await expect(createGithubAppAuth(storageProviderType, installationId)).rejects.toThrow(
102+
await expect(createGithubAppAuth(installationId)).rejects.toThrow(
104103
'Environment variable PARAMETER_GITHUB_APP_KEY_BASE64_NAME is not set',
105104
);
106105
expect(mockedGetParameters).not.toHaveBeenCalled();
@@ -121,7 +120,7 @@ describe('Test createGithubAppAuth', () => {
121120
mockedCreatAppAuth.mockReturnValue(mockWithHook);
122121

123122
// Act
124-
await createGithubAppAuth(storageProviderType, installationId);
123+
await createGithubAppAuth(installationId);
125124

126125
// Assert
127126
expect(mockedCreatAppAuth).toBeCalledTimes(1);
@@ -156,7 +155,7 @@ describe('Test createGithubAppAuth', () => {
156155
});
157156

158157
// Act
159-
await createGithubAppAuth(storageProviderType, installationId);
158+
await createGithubAppAuth(installationId);
160159

161160
// Generate two JWTs and verify they are different (jti makes them unique)
162161
const jwt1 = await capturedCreateJwt!(1);
@@ -193,7 +192,7 @@ describe('Test createGithubAppAuth', () => {
193192
mockedCreatAppAuth.mockReturnValue(mockWithHook);
194193

195194
// Act
196-
const result = await createGithubAppAuth(storageProviderType, installationId);
195+
const result = await createGithubAppAuth(installationId);
197196

198197
// Assert
199198
expect(getParameters).toBeCalledWith([PARAMETER_GITHUB_APP_ID_NAME, PARAMETER_GITHUB_APP_KEY_BASE64_NAME]);
@@ -217,7 +216,7 @@ describe('Test createGithubAppAuth', () => {
217216
mockedCreatAppAuth.mockReturnValue(mockWithHook);
218217

219218
// Act
220-
const result = await createGithubAppAuth(storageProviderType, installationId);
219+
const result = await createGithubAppAuth(installationId);
221220

222221
// Assert
223222
expect(getParameters).toBeCalledWith([PARAMETER_GITHUB_APP_ID_NAME, PARAMETER_GITHUB_APP_KEY_BASE64_NAME]);
@@ -254,7 +253,7 @@ describe('Test createGithubAppAuth', () => {
254253
});
255254

256255
// Act
257-
const result = await createGithubAppAuth(storageProviderType, installationId, githubServerUrl);
256+
const result = await createGithubAppAuth(installationId, githubServerUrl);
258257

259258
// Assert
260259
expect(getParameters).toBeCalledWith([PARAMETER_GITHUB_APP_ID_NAME, PARAMETER_GITHUB_APP_KEY_BASE64_NAME]);
@@ -292,7 +291,7 @@ describe('Test createGithubAppAuth', () => {
292291
mockedCreatAppAuth.mockReturnValue(mockWithHook);
293292

294293
// Act
295-
const result = await createGithubAppAuth(storageProviderType, installationId, githubServerUrl);
294+
const result = await createGithubAppAuth(installationId, githubServerUrl);
296295

297296
// Assert
298297
expect(getParameters).toBeCalledWith([PARAMETER_GITHUB_APP_ID_NAME, PARAMETER_GITHUB_APP_KEY_BASE64_NAME]);
@@ -354,7 +353,7 @@ describe('Test getStoredInstallationId', () => {
354353
]),
355354
);
356355

357-
const result = await getStoredInstallationId(storageProviderType, 0);
356+
const result = await getStoredInstallationId(0);
358357
expect(result).toBe(12345);
359358
});
360359

@@ -369,8 +368,8 @@ describe('Test getStoredInstallationId', () => {
369368
]),
370369
);
371370

372-
await expect(getAppId(storageProviderType, 0)).resolves.toBe(GITHUB_APP_ID);
373-
await expect(getStoredInstallationId(storageProviderType, 0)).resolves.toBe(12345);
371+
await expect(getAppId(0)).resolves.toBe(GITHUB_APP_ID);
372+
await expect(getStoredInstallationId(0)).resolves.toBe(12345);
374373
expect(mockedGetParameters).toHaveBeenCalledTimes(1);
375374
});
376375

@@ -382,8 +381,8 @@ describe('Test getStoredInstallationId', () => {
382381
]),
383382
);
384383

385-
await expect(getAppId(storageProviderType)).rejects.toThrow('temporary storage failure');
386-
await expect(getAppId(storageProviderType)).resolves.toBe(GITHUB_APP_ID);
384+
await expect(getAppId()).rejects.toThrow('temporary storage failure');
385+
await expect(getAppId()).resolves.toBe(GITHUB_APP_ID);
387386
expect(mockedGetParameters).toHaveBeenCalledTimes(2);
388387
});
389388

@@ -396,7 +395,7 @@ describe('Test getStoredInstallationId', () => {
396395
]),
397396
);
398397

399-
const result = await getStoredInstallationId(storageProviderType, 0);
398+
const result = await getStoredInstallationId(0);
400399
expect(result).toBeUndefined();
401400
});
402401

@@ -409,7 +408,7 @@ describe('Test getStoredInstallationId', () => {
409408
]),
410409
);
411410

412-
const result = await getStoredInstallationId(storageProviderType, 0);
411+
const result = await getStoredInstallationId(0);
413412
expect(result).toBeUndefined();
414413
});
415414

@@ -422,7 +421,7 @@ describe('Test getStoredInstallationId', () => {
422421
]),
423422
);
424423

425-
const result = await getStoredInstallationId(storageProviderType, 99);
424+
const result = await getStoredInstallationId(99);
426425
expect(result).toBeUndefined();
427426
});
428427

@@ -448,11 +447,11 @@ describe('Test getStoredInstallationId', () => {
448447
);
449448

450449
// Primary app (index 0) has no stored installation ID
451-
const result0 = await getStoredInstallationId(storageProviderType, 0);
450+
const result0 = await getStoredInstallationId(0);
452451
expect(result0).toBeUndefined();
453452

454453
// Additional app (index 1) has stored installation ID
455-
const result1 = await getStoredInstallationId(storageProviderType, 1);
454+
const result1 = await getStoredInstallationId(1);
456455
expect(result1).toBe(67890);
457456
});
458457
});

‎lambdas/functions/control-plane/src/github/auth.ts‎

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ import { Octokit } from '@octokit/rest';
2222
import { retry } from '@octokit/plugin-retry';
2323
import { throttling } from '@octokit/plugin-throttling';
2424
import { createChildLogger } from '@aws-github-runner/aws-powertools-util';
25-
import { controlPlaneStorageProviderRegistry } from '@aws-github-runner/storage-providers/control-plane';
26-
import type { GitHubAppCredential, StorageProviderType } from '@aws-github-runner/storage-providers';
25+
import { getControlPlaneStorageProvider } from '@aws-github-runner/storage-providers/control-plane';
26+
import type { GitHubAppCredential } from '@aws-github-runner/storage-providers';
2727
import { EndpointDefaults } from '@octokit/types';
2828

2929
const logger = createChildLogger('gh-auth');
@@ -70,46 +70,38 @@ export function onSecondaryRateLimit(
7070
return retryCount < MAX_SECONDARY_RATE_LIMIT_RETRIES;
7171
}
7272

73-
const appCredentialsPromises = new Map<StorageProviderType, Promise<GitHubAppCredential[]>>();
73+
let appCredentialsPromise: Promise<GitHubAppCredential[]> | undefined;
7474

75-
async function getAppCredentials(storageProviderType: StorageProviderType): Promise<GitHubAppCredential[]> {
76-
let credentialsPromise = appCredentialsPromises.get(storageProviderType);
75+
async function getAppCredentials(): Promise<GitHubAppCredential[]> {
76+
let credentialsPromise = appCredentialsPromise;
7777
if (!credentialsPromise) {
78-
const credentialsReader = controlPlaneStorageProviderRegistry.capability(
79-
storageProviderType,
80-
'githubAppCredentialsReader',
81-
)();
78+
const credentialsReader = getControlPlaneStorageProvider().config.githubAppCredentials();
8279
credentialsPromise = credentialsReader.read().then((credentials) => {
8380
logger.info(`Loaded ${credentials.length} GitHub App credential(s)`);
8481
return credentials;
8582
});
86-
appCredentialsPromises.set(storageProviderType, credentialsPromise);
83+
appCredentialsPromise = credentialsPromise;
8784
}
8885

8986
try {
9087
return await credentialsPromise;
9188
} catch (error) {
92-
if (appCredentialsPromises.get(storageProviderType) === credentialsPromise) {
93-
appCredentialsPromises.delete(storageProviderType);
94-
}
89+
if (appCredentialsPromise === credentialsPromise) appCredentialsPromise = undefined;
9590
throw error;
9691
}
9792
}
9893

9994
export function resetAppCredentialsCache(): void {
100-
appCredentialsPromises.clear();
95+
appCredentialsPromise = undefined;
10196
}
10297

103-
export async function getStoredInstallationId(
104-
storageProviderType: StorageProviderType,
105-
appIndex: number,
106-
): Promise<number | undefined> {
107-
const credentials = await getAppCredentials(storageProviderType);
98+
export async function getStoredInstallationId(appIndex: number): Promise<number | undefined> {
99+
const credentials = await getAppCredentials();
108100
return credentials[appIndex]?.installationId;
109101
}
110102

111-
export async function getAppId(storageProviderType: StorageProviderType, appIndex = 0): Promise<string> {
112-
const credential = (await getAppCredentials(storageProviderType))[appIndex];
103+
export async function getAppId(appIndex = 0): Promise<string> {
104+
const credential = (await getAppCredentials())[appIndex];
113105
if (!credential) {
114106
throw new Error(`GitHub App credential at index ${appIndex} not found`);
115107
}
@@ -148,12 +140,11 @@ export async function createOctokitClient(token: string, ghesApiUrl = ''): Promi
148140
}
149141

150142
export async function createGithubAppAuth(
151-
storageProviderType: StorageProviderType,
152143
installationId: number | undefined,
153144
ghesApiUrl = '',
154145
appIndex?: number,
155146
): Promise<AppAuthentication & { appIndex: number }> {
156-
const credentials = await getAppCredentials(storageProviderType);
147+
const credentials = await getAppCredentials();
157148
const idx = appIndex ?? Math.floor(Math.random() * credentials.length);
158149
const selected = credentials[idx];
159150
if (!selected) {
@@ -165,12 +156,11 @@ export async function createGithubAppAuth(
165156
}
166157

167158
export async function createGithubInstallationAuth(
168-
storageProviderType: StorageProviderType,
169159
installationId: number | undefined,
170160
ghesApiUrl = '',
171161
appIndex?: number,
172162
): Promise<InstallationAccessTokenAuthentication> {
173-
const credentials = await getAppCredentials(storageProviderType);
163+
const credentials = await getAppCredentials();
174164
const idx = appIndex ?? Math.floor(Math.random() * credentials.length);
175165
const selected = credentials[idx];
176166
if (!selected) {

‎lambdas/functions/control-plane/src/github/octokit.test.ts‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,9 @@ const mockOctokit = {
1212
};
1313

1414
vi.mock('../github/auth', async () => ({
15-
createGithubInstallationAuth: vi
16-
.fn()
17-
.mockImplementation(async (_storageProviderType: string, installationId: number) => {
18-
return { token: 'token', type: 'installation', installationId: installationId };
19-
}),
15+
createGithubInstallationAuth: vi.fn().mockImplementation(async (installationId: number) => {
16+
return { token: 'token', type: 'installation', installationId: installationId };
17+
}),
2018
createOctokitClient: vi.fn().mockImplementation(() => new Octokit()),
2119
createGithubAppAuth: vi.fn().mockResolvedValue({ token: 'token', appIndex: 0 }),
2220
getStoredInstallationId: vi.fn().mockResolvedValue(undefined),
@@ -28,9 +26,6 @@ vi.mock('@octokit/rest', async () => ({
2826
}),
2927
}));
3028

31-
// We've already mocked '../github/auth' above
32-
const storageProviderType = 'aws_ssm' as const;
33-
3429
describe('Test getOctokit', () => {
3530
const data: Array<{
3631
description: string;
@@ -75,7 +70,7 @@ describe('Test getOctokit', () => {
7570
mockOctokit.apps.getOrgInstallation.mockRejectedValue(new Error('Error'));
7671
}
7772

78-
await expect(getOctokit(storageProviderType, '', input.orgLevelRunner, payload)).resolves.toBeDefined();
73+
await expect(getOctokit('', input.orgLevelRunner, payload)).resolves.toBeDefined();
7974

8075
if (output.callOrgInstallation) {
8176
expect(mockOctokit.apps.getOrgInstallation).toHaveBeenCalled();
@@ -110,7 +105,7 @@ describe('Test getOctokit installation ID resolution (Fix B: index-0 payload reu
110105
(getStoredInstallationId as Mock).mockResolvedValue(undefined);
111106
const payload = { ...basePayload, installationId: 5 } as ActionRequestMessage;
112107

113-
await expect(getOctokit(storageProviderType, '', true, payload)).resolves.toBeDefined();
108+
await expect(getOctokit('', true, payload)).resolves.toBeDefined();
114109

115110
// Primary app must NOT do an API lookup — it reuses the webhook payload installationId
116111
expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled();
@@ -123,7 +118,7 @@ describe('Test getOctokit installation ID resolution (Fix B: index-0 payload reu
123118
(getStoredInstallationId as Mock).mockResolvedValue(undefined);
124119
const payload = { ...basePayload, installationId: 5 } as ActionRequestMessage;
125120

126-
await expect(getOctokit(storageProviderType, '', true, payload)).resolves.toBeDefined();
121+
await expect(getOctokit('', true, payload)).resolves.toBeDefined();
127122

128123
// Additional app must do an API lookup (it cannot reuse the webhook payload)
129124
expect(mockOctokit.apps.getOrgInstallation).toHaveBeenCalled();
@@ -135,7 +130,7 @@ describe('Test getOctokit installation ID resolution (Fix B: index-0 payload reu
135130
(getStoredInstallationId as Mock).mockResolvedValue(77);
136131
const payload = { ...basePayload, installationId: 5 } as ActionRequestMessage;
137132

138-
await expect(getOctokit(storageProviderType, '', true, payload)).resolves.toBeDefined();
133+
await expect(getOctokit('', true, payload)).resolves.toBeDefined();
139134

140135
// Stored id wins: no API lookup needed
141136
expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled();
@@ -165,10 +160,10 @@ describe('Test getOctokit stale installation fallback', () => {
165160
.mockResolvedValueOnce({ token: 'fresh-token', type: 'installation', installationId: 99 });
166161

167162
const payload = { ...basePayload, installationId: 5 } as ActionRequestMessage;
168-
await expect(getOctokit(storageProviderType, '', true, payload)).resolves.toBeDefined();
163+
await expect(getOctokit('', true, payload)).resolves.toBeDefined();
169164

170-
expect(createGithubInstallationAuth).toHaveBeenNthCalledWith(1, storageProviderType, 5, '', 0);
171-
expect(createGithubInstallationAuth).toHaveBeenNthCalledWith(2, storageProviderType, 99, '', 0);
165+
expect(createGithubInstallationAuth).toHaveBeenNthCalledWith(1, 5, '', 0);
166+
expect(createGithubInstallationAuth).toHaveBeenNthCalledWith(2, 99, '', 0);
172167
expect(mockOctokit.apps.getOrgInstallation).toHaveBeenCalledWith({ org: 'owner' });
173168
});
174169

@@ -178,7 +173,7 @@ describe('Test getOctokit stale installation fallback', () => {
178173
(createGithubInstallationAuth as Mock).mockRejectedValueOnce(notFound);
179174

180175
const payload = { ...basePayload, installationId: 5 } as ActionRequestMessage;
181-
await expect(getOctokit(storageProviderType, '', true, payload)).rejects.toThrow('Not Found');
176+
await expect(getOctokit('', true, payload)).rejects.toThrow('Not Found');
182177
expect(createGithubInstallationAuth).toHaveBeenCalledTimes(1);
183178
});
184179

@@ -187,7 +182,7 @@ describe('Test getOctokit stale installation fallback', () => {
187182
(createGithubInstallationAuth as Mock).mockRejectedValueOnce(serverError);
188183

189184
const payload = { ...basePayload, installationId: 5 } as ActionRequestMessage;
190-
await expect(getOctokit(storageProviderType, '', true, payload)).rejects.toThrow('Server Error');
185+
await expect(getOctokit('', true, payload)).rejects.toThrow('Server Error');
191186
expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled();
192187
expect(createGithubInstallationAuth).toHaveBeenCalledTimes(1);
193188
});

0 commit comments

Comments
 (0)