Skip to content

Commit eaebfc3

Browse files
ZainRizvijeanschmidt
ZainRizvi
and
jeanschmidt
authored
[autoscalers] Only use auth to download github files if needed (#7064)
This enables autoscalers to use a scale-config that's located in an organization other than the one they're located in, as long as that scale-config is located in a public repo (which all our scale configs currently are). Bug it fixes: The old code would create a github client to download the scale-config.yml file, but `createGitHubClientForRunnerOrg` will fail if you try to try to create a client for an org your app doesn't have access to. Using a full blown git client for a public file also seems unnecessary. This version uses a normal http request to pull the raw file. So authentication doesn't matter. (Aside: I considered keeping the old flow as a backup path for if we ever want the scale config to live in a public repo, but if and when that day comes I'd rather we add the logic afresh than leave dead, unused code around in the script.) Testing: Verified the getRunnerTypes functionality locally to ensure it worked end-to-end without mocks. --------- Co-authored-by: Jean Schmidt <4520845+jeanschmidt@users.noreply.github.com>
1 parent af17e0f commit eaebfc3

7 files changed

Lines changed: 134 additions & 60 deletions

File tree

‎terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/gh-runners.test.ts‎

Lines changed: 53 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import { Config } from './config';
2222
import { Octokit } from '@octokit/rest';
2323
import { mocked } from 'ts-jest/utils';
2424
import { locallyCached, redisCached } from './cache';
25-
import nock from 'nock';
2625

2726
const mockEC2 = {
2827
describeInstances: jest.fn(),
@@ -59,7 +58,6 @@ beforeEach(() => {
5958
jest.resetModules();
6059
jest.clearAllMocks();
6160
jest.restoreAllMocks();
62-
nock.disableNetConnect();
6361

6462
jest.spyOn(metrics, 'sendMetrics').mockImplementation(async () => {
6563
return;
@@ -860,54 +858,44 @@ runner_types:
860858
]);
861859

862860
it('gets the contents, twice', async () => {
863-
const repo = { owner: 'owner', repo: 'repo' };
864-
const token1 = 'token1';
865-
const token2 = 'token2';
866-
const repoId = 'mockReturnValueOnce1';
861+
const fileRepo = { owner: 'owner', repo: 'repo' };
862+
const authRepo = { owner: 'auth-owner', repo: 'auth-repo' };
867863
const mockCreateGithubAuth = mocked(createGithubAuth);
868864
const mockCreateOctoClient = mocked(createOctoClient);
869865
const getRepoInstallation = jest.fn().mockResolvedValue({
870-
data: { id: repoId },
866+
data: { id: 'mockReturnValueOnce1' },
871867
});
868+
const scaleConfigBase64 = Buffer.from(scaleConfigYaml).toString('base64');
872869
const mockedOctokit = {
873870
apps: { getRepoInstallation: getRepoInstallation },
874871
repos: {
875-
getContent: jest.fn().mockResolvedValue({
876-
data: { content: Buffer.from(scaleConfigYaml).toString('base64') },
872+
getContent: jest.fn().mockResolvedValueOnce({
877873
status: 200,
874+
data: { content: scaleConfigBase64 },
878875
}),
879876
},
880877
};
881878

882-
mockCreateGithubAuth.mockResolvedValueOnce(token1);
879+
mockCreateGithubAuth.mockResolvedValueOnce('token1');
883880
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);
884-
mockCreateGithubAuth.mockResolvedValueOnce(token2);
881+
mockCreateGithubAuth.mockResolvedValueOnce('token2');
885882
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);
886883

887884
await resetGHRunnersCaches();
888-
expect(await getRunnerTypes(repo, metrics)).toEqual(getRunnerTypeResponse);
889-
expect(await getRunnerTypes(repo, metrics)).toEqual(getRunnerTypeResponse);
890-
891-
expect(mockCreateGithubAuth).toBeCalledTimes(2);
892-
expect(mockCreateGithubAuth).toBeCalledWith(undefined, 'app', Config.Instance.ghesUrlApi, metrics);
893-
expect(mockCreateGithubAuth).toBeCalledWith(repoId, 'installation', Config.Instance.ghesUrlApi, metrics);
894-
895-
expect(mockCreateOctoClient).toBeCalledTimes(2);
896-
expect(mockCreateOctoClient).toBeCalledWith(token1, Config.Instance.ghesUrlApi);
897-
expect(mockCreateOctoClient).toBeCalledWith(token2, Config.Instance.ghesUrlApi);
898-
899-
expect(getRepoInstallation).toBeCalledTimes(1);
900-
expect(getRepoInstallation).toBeCalledWith({ ...repo });
885+
expect(await getRunnerTypes(fileRepo, authRepo, metrics)).toEqual(getRunnerTypeResponse);
886+
expect(await getRunnerTypes(fileRepo, authRepo, metrics)).toEqual(getRunnerTypeResponse);
901887

888+
// Verify the API was called only once due to caching
902889
expect(mockedOctokit.repos.getContent).toBeCalledTimes(1);
903890
expect(mockedOctokit.repos.getContent).toBeCalledWith({
904-
...repo,
891+
...fileRepo,
905892
path: Config.Instance.scaleConfigRepoPath,
906893
});
907894
});
908895

909896
it('return is not 200', async () => {
910-
const repo = { owner: 'owner', repo: 'repo' };
897+
const fileRepo = { owner: 'owner', repo: 'repo_fail' };
898+
const authRepo = { owner: 'auth-owner', repo: 'auth-repo' };
911899
const mockCreateGithubAuth = mocked(createGithubAuth);
912900
const mockCreateOctoClient = mocked(createOctoClient);
913901
const getRepoInstallation = jest.fn().mockResolvedValue({
@@ -916,9 +904,9 @@ runner_types:
916904
const mockedOctokit = {
917905
apps: { getRepoInstallation: getRepoInstallation },
918906
repos: {
919-
getContent: jest.fn().mockResolvedValue({
920-
data: { content: Buffer.from(scaleConfigYaml).toString('base64') },
907+
getContent: jest.fn().mockResolvedValueOnce({
921908
status: 500,
909+
data: {},
922910
}),
923911
},
924912
};
@@ -929,7 +917,43 @@ runner_types:
929917
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);
930918

931919
await resetGHRunnersCaches();
932-
await expect(getRunnerTypes(repo, metrics)).rejects.toThrow(Error);
920+
await expect(getRunnerTypes(fileRepo, authRepo, metrics)).rejects.toThrow(Error);
921+
});
922+
923+
it('gets the contents using org client when enableOrganizationRunners is true', async () => {
924+
const config = {
925+
enableOrganizationRunners: true,
926+
};
927+
jest.spyOn(Config, 'Instance', 'get').mockImplementation(() => config as unknown as Config);
928+
929+
const fileRepo = { owner: 'owner', repo: 'repo' };
930+
const authRepo = { owner: 'auth-owner', repo: 'auth-repo' };
931+
const mockCreateGithubAuth = mocked(createGithubAuth);
932+
const mockCreateOctoClient = mocked(createOctoClient);
933+
const getOrgInstallation = jest.fn().mockResolvedValue({
934+
data: { id: 'mockReturnValueOnce1' },
935+
});
936+
const scaleConfigBase64 = Buffer.from(scaleConfigYaml).toString('base64');
937+
const mockedOctokit = {
938+
apps: { getOrgInstallation: getOrgInstallation },
939+
repos: {
940+
getContent: jest.fn().mockResolvedValueOnce({
941+
status: 200,
942+
data: { content: scaleConfigBase64 },
943+
}),
944+
},
945+
};
946+
947+
mockCreateGithubAuth.mockResolvedValueOnce('token1');
948+
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);
949+
mockCreateGithubAuth.mockResolvedValueOnce('token2');
950+
mockCreateOctoClient.mockReturnValueOnce(mockedOctokit as unknown as Octokit);
951+
952+
await resetGHRunnersCaches();
953+
expect(await getRunnerTypes(fileRepo, authRepo, metrics)).toEqual(getRunnerTypeResponse);
954+
955+
// Verify org client was created
956+
expect(getOrgInstallation).toBeCalledWith({ org: authRepo.owner });
933957
});
934958
});
935959

‎terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/gh-runners.ts‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -307,30 +307,33 @@ export async function getRunnerOrg(org: string, runnerID: string, metrics: Metri
307307
}
308308
}
309309

310+
/**
311+
Get runner types from scale-config.yml
312+
*/
310313
export async function getRunnerTypes(
311-
repo: Repo,
314+
filerepo: Repo,
315+
authrepo: Repo,
312316
metrics: Metrics,
313317
filepath = Config.Instance.scaleConfigRepoPath,
314318
): Promise<Map<string, RunnerType>> {
315319
const alphaNumericStr = /^[a-zA-Z0-9.-]+$/;
316320

317-
return await redisCached('ghRunners', `getRunnerTypes-${repo.owner}.${repo.repo}`, 10 * 60, 0.5, async () => {
321+
return await redisCached('ghRunners', `getRunnerTypes-${filerepo.owner}.${filerepo.repo}`, 10 * 60, 0.5, async () => {
318322
let status = 'noRun';
319323
try {
320-
status = 'doRun';
321-
/* istanbul ignore next */
322324
const githubAppClient = Config.Instance.enableOrganizationRunners
323-
? await createGitHubClientForRunnerOrg(repo.owner, metrics)
324-
: await createGitHubClientForRunnerRepo(repo, metrics);
325+
? await createGitHubClientForRunnerOrg(authrepo.owner, metrics)
326+
: await createGitHubClientForRunnerRepo(authrepo, metrics);
325327

326328
console.debug(
327-
`[getRunnerTypes]: Fetching runner types from ${filepath} for https://github.com/${repo.owner}/${repo.repo}/`,
329+
`[getRunnerTypes]: Fetching runner types from ${filepath} for ` +
330+
`https://github.com/${filerepo.owner}/${filerepo.repo}/`,
328331
);
329332

330333
const response = await expBackOff(() => {
331334
return metrics.trackRequest(metrics.reposGetContentGHCallSuccess, metrics.reposGetContentGHCallFailure, () => {
332335
return githubAppClient.repos.getContent({
333-
...repo,
336+
...filerepo,
334337
path: filepath,
335338
});
336339
});
@@ -340,7 +343,8 @@ export async function getRunnerTypes(
340343
const { content }: { content?: string } = { ...(response?.data || {}) } as { content?: string };
341344
if (response?.status != 200 || !content) {
342345
throw Error(
343-
`Issue (${response.status}) retrieving '${filepath}' for https://github.com/${repo.owner}/${repo.repo}/`,
346+
`Issue (${response.status}) retrieving '${filepath}' for ` +
347+
`https://github.com/${filerepo.owner}/${filerepo.repo}/`,
344348
);
345349
}
346350

@@ -440,7 +444,7 @@ export async function getRunnerTypes(
440444
return filteredResult;
441445
} catch (e) {
442446
console.error(
443-
`[getRunnerTypes]: Error for path '${filepath}' for https://github.com/${repo.owner}/${repo.repo}/`,
447+
`[getRunnerTypes]: Error for path '${filepath}' for https://github.com/${filerepo.owner}/${filerepo.repo}/`,
444448
);
445449
console.error(`[getRunnerTypes]: ${e}`);
446450
throw e;

‎terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts‎

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ describe('scale-down', () => {
156156
describe('org', () => {
157157
const environment = 'environment';
158158
const scaleConfigRepo = 'test-infra';
159-
const theOrg = ' a-owner';
159+
const theOrg = 'a-owner';
160160
const dateRef = moment(new Date());
161161
const runnerTypes = new Map([
162162
['ignore-no-org-no-repo', { is_ephemeral: false } as RunnerType],
@@ -455,7 +455,11 @@ describe('scale-down', () => {
455455
expect(mockedListGithubRunnersOrg).toBeCalledWith(theOrg, metrics);
456456

457457
expect(mockedGetRunnerTypes).toBeCalledTimes(9);
458-
expect(mockedGetRunnerTypes).toBeCalledWith({ owner: theOrg, repo: scaleConfigRepo }, metrics);
458+
expect(mockedGetRunnerTypes).toBeCalledWith(
459+
{ owner: theOrg, repo: scaleConfigRepo },
460+
{ owner: theOrg, repo: '' },
461+
metrics,
462+
);
459463

460464
expect(mockedRemoveGithubRunnerOrg).toBeCalledTimes(5);
461465
{
@@ -797,7 +801,7 @@ describe('scale-down', () => {
797801
expect(mockedListGithubRunnersRepo).toBeCalledWith(repo, metrics);
798802

799803
expect(mockedGetRunnerTypes).toBeCalledTimes(9);
800-
expect(mockedGetRunnerTypes).toBeCalledWith(repo, metrics);
804+
expect(mockedGetRunnerTypes).toBeCalledWith(repo, repo, metrics);
801805

802806
expect(mockedRemoveGithubRunnerRepo).toBeCalledTimes(5);
803807
{
@@ -1253,10 +1257,19 @@ describe('scale-down', () => {
12531257

12541258
mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, {} as RunnerType]]));
12551259

1256-
expect(await isEphemeralRunner({ runnerType: runnerType, org: owner } as RunnerInfo, metrics)).toEqual(false);
1260+
expect(
1261+
await isEphemeralRunner(
1262+
{ runnerType: runnerType, org: owner, repo: `${owner}/${scaleConfigRepo}` } as RunnerInfo,
1263+
metrics,
1264+
),
1265+
).toEqual(false);
12571266

12581267
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1259-
expect(mockedGetRunnerTypes).toBeCalledWith({ owner: owner, repo: scaleConfigRepo }, metrics);
1268+
expect(mockedGetRunnerTypes).toBeCalledWith(
1269+
{ owner: owner, repo: scaleConfigRepo },
1270+
{ owner: owner, repo: scaleConfigRepo },
1271+
metrics,
1272+
);
12601273
});
12611274

12621275
it('org in runner, is_ephemeral === false', async () => {
@@ -1265,10 +1278,19 @@ describe('scale-down', () => {
12651278

12661279
mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, { is_ephemeral: false } as RunnerType]]));
12671280

1268-
expect(await isEphemeralRunner({ runnerType: runnerType, org: owner } as RunnerInfo, metrics)).toEqual(false);
1281+
expect(
1282+
await isEphemeralRunner(
1283+
{ runnerType: runnerType, org: owner, repo: `${owner}/${scaleConfigRepo}` } as RunnerInfo,
1284+
metrics,
1285+
),
1286+
).toEqual(false);
12691287

12701288
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1271-
expect(mockedGetRunnerTypes).toBeCalledWith({ owner: owner, repo: scaleConfigRepo }, metrics);
1289+
expect(mockedGetRunnerTypes).toBeCalledWith(
1290+
{ owner: owner, repo: scaleConfigRepo },
1291+
{ owner: owner, repo: scaleConfigRepo },
1292+
metrics,
1293+
);
12721294
});
12731295

12741296
it('org not in runner, is_ephemeral === true', async () => {
@@ -1282,7 +1304,11 @@ describe('scale-down', () => {
12821304
).toEqual(true);
12831305

12841306
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1285-
expect(mockedGetRunnerTypes).toBeCalledWith({ owner: owner, repo: scaleConfigRepo }, metrics);
1307+
expect(mockedGetRunnerTypes).toBeCalledWith(
1308+
{ owner: owner, repo: scaleConfigRepo },
1309+
{ owner: owner, repo: 'a-repo' },
1310+
metrics,
1311+
);
12861312
});
12871313
});
12881314

@@ -1316,7 +1342,7 @@ describe('scale-down', () => {
13161342
).toEqual(false);
13171343

13181344
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1319-
expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, metrics);
1345+
expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, runnerRepo, metrics);
13201346
});
13211347

13221348
it('is_ephemeral === true', async () => {
@@ -1329,7 +1355,7 @@ describe('scale-down', () => {
13291355
).toEqual(true);
13301356

13311357
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1332-
expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, metrics);
1358+
expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, runnerRepo, metrics);
13331359
});
13341360

13351361
it('is_ephemeral === false', async () => {
@@ -1342,7 +1368,7 @@ describe('scale-down', () => {
13421368
).toEqual(false);
13431369

13441370
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1345-
expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, metrics);
1371+
expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, runnerRepo, metrics);
13461372
});
13471373
});
13481374

@@ -1374,7 +1400,7 @@ describe('scale-down', () => {
13741400
).toEqual(false);
13751401

13761402
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1377-
expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, metrics);
1403+
expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, runnerRepo, metrics);
13781404
});
13791405

13801406
it('is_ephemeral === true', async () => {
@@ -1387,7 +1413,7 @@ describe('scale-down', () => {
13871413
).toEqual(true);
13881414

13891415
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1390-
expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, metrics);
1416+
expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, runnerRepo, metrics);
13911417
});
13921418

13931419
it('is_ephemeral === false', async () => {
@@ -1400,7 +1426,7 @@ describe('scale-down', () => {
14001426
).toEqual(false);
14011427

14021428
expect(mockedGetRunnerTypes).toBeCalledTimes(1);
1403-
expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, metrics);
1429+
expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, runnerRepo, metrics);
14041430
});
14051431
});
14061432
});

‎terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,11 @@ export async function isEphemeralRunner(ec2runner: RunnerInfo, metrics: ScaleDow
268268
return false;
269269
}
270270

271-
const runnerTypes = await getRunnerTypes(backwardCompatibleGetRepoForgetRunnerTypes(ec2runner), metrics);
271+
const runnerTypes = await getRunnerTypes(
272+
backwardCompatibleGetRepoForgetRunnerTypes(ec2runner),
273+
ec2runner.repo ? getRepo(ec2runner.repo as string) : { owner: ec2runner.org as string, repo: '' },
274+
metrics,
275+
);
272276
return runnerTypes.get(ec2runner.runnerType)?.is_ephemeral ?? false;
273277
}
274278

@@ -278,7 +282,11 @@ export async function minRunners(ec2runner: RunnerInfo, metrics: ScaleDownMetric
278282
return Config.Instance.minAvailableRunners;
279283
}
280284

281-
const runnerTypes = await getRunnerTypes(backwardCompatibleGetRepoForgetRunnerTypes(ec2runner), metrics);
285+
const runnerTypes = await getRunnerTypes(
286+
backwardCompatibleGetRepoForgetRunnerTypes(ec2runner),
287+
ec2runner.repo ? getRepo(ec2runner.repo as string) : { owner: ec2runner.org as string, repo: '' },
288+
metrics,
289+
);
282290
return runnerTypes.get(ec2runner.runnerType)?.min_available ?? Config.Instance.minAvailableRunners;
283291
}
284292

‎terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up-chron.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,12 @@ export async function scaleUpChron(metrics: ScaleUpChronMetrics): Promise<void>
1414
// 3. For each runner queued for longer than the minimum delay, try to scale it up
1515

1616
try {
17+
const repo = getRepo(Config.Instance.scaleConfigOrg, Config.Instance.scaleConfigRepo);
1718
const validRunnerTypes = await getRunnerTypes(
18-
getRepo(Config.Instance.scaleConfigOrg, Config.Instance.scaleConfigRepo),
19+
// For scaleUpChron, we don't have a situation where the auth repo is different from the config repo
20+
// so we can just pass the same repo for both parameters
21+
repo,
22+
repo,
1923
metrics,
2024
Config.Instance.scaleConfigRepoPath,
2125
);

0 commit comments

Comments
 (0)