Skip to content

Commit 48b5401

Browse files
committed
feat(lambda): move to a single github client with conditional requests
We're currently creating fresh clients for each lambda invocation. This is not entirely optimal, because we'll potentially request the same data from the API multiple times. Move instead to a single central client. This is created at the module scope and then passed down into the handlers. The client also uses `toad-cache` to cache previously-seen responses and send `If-None-Match` and `If-Modified-Since` headers to the API to allow it to avoid sending us back data we already have in our hands. When this happens we get a "304 Not Modified" response, we use our copy, and the call doesn't consume the rate limit.
1 parent cbebee6 commit 48b5401

File tree

13 files changed

+2171
-2156
lines changed

13 files changed

+2171
-2156
lines changed

lambdas/functions/control-plane/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
"@octokit/rest": "20.1.2",
5252
"@octokit/types": "^13.8.0",
5353
"cron-parser": "^4.9.0",
54+
"toad-cache": "^3.7.0",
5455
"typescript": "^5.7.3"
5556
},
5657
"nx": {

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

Lines changed: 0 additions & 87 deletions
This file was deleted.

lambdas/functions/control-plane/src/github/auth.test.ts renamed to lambdas/functions/control-plane/src/github/client.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { mocked } from 'jest-mock';
77
import { MockProxy, mock } from 'jest-mock-extended';
88
import nock from 'nock';
99

10-
import { createGithubAppAuth, createOctokitClient } from './auth';
10+
import { createGithubAppAuth, createOctokitClient } from './client';
1111

1212
jest.mock('@aws-github-runner/aws-ssm-util');
1313
jest.mock('@octokit/auth-app');
Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
import { createAppAuth } from '@octokit/auth-app';
2+
import { StrategyOptions } from '@octokit/auth-app/dist-types/types';
3+
import { OctokitOptions } from '@octokit/core/dist-types/types';
4+
import { Octokit } from '@octokit/rest';
5+
import { throttling } from '@octokit/plugin-throttling';
6+
import { createChildLogger } from '@aws-github-runner/aws-powertools-util';
7+
import { getParameter } from '@aws-github-runner/aws-ssm-util';
8+
import { EndpointDefaults, type OctokitResponse } from '@octokit/types';
9+
import { RequestError } from '@octokit/request-error';
10+
import { Lru } from 'toad-cache';
11+
import type { ActionRequestMessage } from '../scale-runners/scale-up';
12+
13+
const logger = createChildLogger('gh-auth');
14+
15+
interface CacheEntry {
16+
etag?: string;
17+
lastModified?: string;
18+
[key: string]: unknown;
19+
}
20+
21+
22+
// Cache for conditional requests
23+
// Using 15000 entries with 5 minute TTL
24+
const cache = new Lru<CacheEntry>(15000, 5 * 60 * 1000);
25+
26+
async function appAuthCommonParameters(): Promise<StrategyOptions> {
27+
const appId = parseInt(await getParameter(process.env.PARAMETER_GITHUB_APP_ID_NAME));
28+
29+
return {
30+
appId,
31+
privateKey: Buffer.from(
32+
await getParameter(process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME),
33+
'base64',
34+
// replace literal \n characters with new lines to allow the key to be stored as a
35+
// single line variable. This logic should match how the GitHub Terraform provider
36+
// processes private keys to retain compatibility between the projects
37+
)
38+
.toString()
39+
.replace('/[\\n]/g', String.fromCharCode(10)),
40+
};
41+
}
42+
43+
/**
44+
* Handler run before requests are sent. Looks up the URL in the cache, and adds
45+
* headers for conditional retrieval if there is an entry.
46+
*/
47+
async function beforeRequestHandler(octokit: Octokit, options: Required<EndpointDefaults>): Promise<void> {
48+
const { method } = options;
49+
50+
if (method !== 'GET') {
51+
return;
52+
}
53+
54+
const { url } = octokit.request.endpoint.parse(options);
55+
const cacheKey = url;
56+
const cacheEntry = cache.get(cacheKey);
57+
58+
if (cacheEntry === undefined) {
59+
logger.info('Cache miss', { url });
60+
return;
61+
}
62+
63+
const { etag, lastModified } = cacheEntry;
64+
65+
if (etag !== undefined) {
66+
options.headers['If-None-Match'] = etag;
67+
}
68+
69+
if (lastModified !== undefined) {
70+
options.headers['If-Modified-Since'] = lastModified;
71+
}
72+
73+
logger.info('Cache hit', { url, etag, lastModified });
74+
}
75+
76+
/**
77+
* Handler run after requests are sent. Caches the response if it has an ETag or
78+
* Last-Modified header, so that it can be returned by future conditional
79+
* requests if requested again.
80+
*/
81+
async function afterRequestHandler(octokit: Octokit, response: OctokitResponse<any, number>, options: Required<EndpointDefaults>): Promise<void> {
82+
const { status } = response;
83+
const { url } = octokit.request.endpoint.parse(options);
84+
logger.info(`Response received`, { status, url });
85+
86+
const cacheKey = url;
87+
const eTag = response.headers.etag;
88+
const lastModified = response.headers['last-modified'];
89+
90+
if (eTag === undefined && lastModified === undefined) {
91+
return;
92+
}
93+
94+
logger.info('Caching response', { url, eTag, lastModified });
95+
96+
cache.set(cacheKey, {
97+
...(eTag !== undefined ? { etag: eTag } : {}),
98+
...(lastModified !== undefined ? { lastModified } : {}),
99+
...response,
100+
});
101+
}
102+
103+
/**
104+
* Handler run if a request fails. This handler is called for any non-2xx
105+
* response. We will get "304 Not Modified" responses when the conditional
106+
* request is satisfied, and we should return the cached data in that case.
107+
*/
108+
async function errorRequestHandler(octokit: Octokit, error: Error, options: Required<EndpointDefaults>): Promise<CacheEntry> {
109+
if (!(error instanceof RequestError)) {
110+
throw error;
111+
}
112+
113+
const { status } = error;
114+
115+
if (status != 304) {
116+
throw error;
117+
}
118+
119+
const { url } = octokit.request.endpoint.parse(options);
120+
121+
const entry = cache.get(url);
122+
123+
if (entry === undefined) {
124+
throw new Error(`Received 304 Not Modified response for ${url}, but it wasn't found in the cache.`);
125+
}
126+
127+
return entry;
128+
}
129+
130+
export async function createAppAuthClient(ghesApiUrl: string = ''): Promise<Octokit> {
131+
const CustomOctokit = Octokit.plugin(throttling);
132+
133+
const octokit = new CustomOctokit({
134+
authStrategy: createAppAuth,
135+
auth: await appAuthCommonParameters(),
136+
baseUrl: ghesApiUrl || undefined,
137+
previews: ghesApiUrl ? ['antiope'] : undefined,
138+
throttle: {
139+
onRateLimit: (retryAfter: number, options: Required<EndpointDefaults>) => {
140+
logger.warn(`GitHub rate limit: Request quota exhausted for request ${options.method} ${options.url}.`, {
141+
retryAfter,
142+
});
143+
},
144+
onSecondaryRateLimit: (retryAfter: number, options: Required<EndpointDefaults>) => {
145+
logger.warn(`GitHub rate limit: SecondaryRateLimit detected for request ${options.method} ${options.url}`, {
146+
retryAfter,
147+
});
148+
},
149+
},
150+
userAgent: process.env.USER_AGENT || 'github-aws-runners',
151+
});
152+
153+
octokit.hook.before('request', async (options) => beforeRequestHandler(octokit, options));
154+
octokit.hook.after('request', async (response, options) => afterRequestHandler(octokit, response, options));
155+
octokit.hook.error('request', async (error, options) => errorRequestHandler(octokit, error, options));
156+
157+
return octokit;
158+
}
159+
160+
async function getInstallationId(
161+
appClient: Octokit,
162+
enableOrgLevel: boolean,
163+
payload: ActionRequestMessage,
164+
): Promise<number> {
165+
if (payload.installationId !== 0) {
166+
return payload.installationId;
167+
}
168+
169+
return (
170+
enableOrgLevel
171+
? await appClient.apps.getOrgInstallation({
172+
org: payload.repositoryOwner,
173+
})
174+
: await appClient.apps.getRepoInstallation({
175+
owner: payload.repositoryOwner,
176+
repo: payload.repositoryName,
177+
})
178+
).data.id;
179+
}
180+
181+
export async function createAppInstallationClient(appOctokit: Octokit, enableOrgLevel: boolean, payload: ActionRequestMessage): Promise<Octokit> {
182+
const installationId = await getInstallationId(appOctokit, enableOrgLevel, payload);
183+
184+
return appOctokit.auth({
185+
type: 'installation',
186+
installationId,
187+
factory: ({ octokitOptions, ...auth }: { octokitOptions: OctokitOptions }) =>
188+
new Octokit({
189+
...octokitOptions,
190+
auth: auth,
191+
}),
192+
}) as Promise<Octokit>;
193+
}
194+
195+
export function getGitHubEnterpriseApiUrl() {
196+
const ghesBaseUrl = process.env.GHES_URL;
197+
let ghesApiUrl = '';
198+
if (ghesBaseUrl) {
199+
const url = new URL(ghesBaseUrl);
200+
const domain = url.hostname;
201+
if (domain.endsWith('.ghe.com')) {
202+
// Data residency: Prepend 'api.'
203+
ghesApiUrl = `https://api.${domain}`;
204+
} else {
205+
// GitHub Enterprise Server: Append '/api/v3'
206+
ghesApiUrl = `${ghesBaseUrl}/api/v3`;
207+
}
208+
}
209+
logger.debug(`Github Enterprise URLs: api_url - ${ghesApiUrl}; base_url - ${ghesBaseUrl}`);
210+
return { ghesApiUrl, ghesBaseUrl };
211+
}

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

Lines changed: 0 additions & 46 deletions
This file was deleted.

lambdas/functions/control-plane/src/lambda.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ import { scaleDown } from './scale-runners/scale-down';
99
import { scaleUp } from './scale-runners/scale-up';
1010
import { SSMCleanupOptions, cleanSSMTokens } from './scale-runners/ssm-housekeeper';
1111
import { checkAndRetryJob } from './scale-runners/job-retry';
12+
import { createAppAuthClient, getGitHubEnterpriseApiUrl } from './github/client';
13+
14+
const { ghesApiUrl } = getGitHubEnterpriseApiUrl();
15+
// TODO: needs to be ESM for top-level await, or we create this lazily.
16+
const ghAppClient = await createAppAuthClient(ghesApiUrl);
1217

1318
export async function scaleUpHandler(event: SQSEvent, context: Context): Promise<void> {
1419
setContext(context, 'lambda.ts');
@@ -20,7 +25,7 @@ export async function scaleUpHandler(event: SQSEvent, context: Context): Promise
2025
}
2126

2227
try {
23-
await scaleUp(event.Records[0].eventSource, JSON.parse(event.Records[0].body));
28+
await scaleUp(ghAppClient, event.Records[0].eventSource, JSON.parse(event.Records[0].body));
2429
} catch (e) {
2530
if (e instanceof ScaleError) {
2631
throw e;

lambdas/functions/control-plane/src/pool/pool.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import moment from 'moment-timezone';
44
import nock from 'nock';
55

66
import { listEC2Runners } from '../aws/runners';
7-
import * as ghAuth from '../github/auth';
7+
import * as ghAuth from '../github/client';
88
import { createRunners, getGitHubEnterpriseApiUrl } from '../scale-runners/scale-up';
99
import { adjust } from './pool';
1010

0 commit comments

Comments
 (0)