Skip to content

Commit 7abaed3

Browse files
committed
Move log backfill off the public API Gateway endpoint
**Impact:** `backfillMissingLog`, which Dr.CI calls when a failed job has no log **Risk:** medium -- changes a live Dr.CI path ## What `backfillMissingLog` now invokes the `gha-log-uploader` lambda directly instead of POSTing a synthetic `action: "backfill"` event at `jqogootqqe.execute-api.us-east-1.amazonaws.com`. Adds `POST /api/log-uploader/backfill` for callers outside HUD, authenticated with a shared secret in `LOG_UPLOADER_BOT_KEY`, matching the `DRCI_BOT_KEY` and `FLAKY_TEST_BOT_KEY` routes. ghstack-source-id: 0ab1991 Pull-Request: #8595
1 parent 3ba83eb commit 7abaed3

4 files changed

Lines changed: 248 additions & 26 deletions

File tree

torchci/lib/jobUtils.ts

Lines changed: 19 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { durationDisplay } from "components/common/TimeUtils";
22
import dayjs from "dayjs";
33
import { jaroWinkler } from "jaro-winkler-typescript";
4+
import { invokeLogUploader } from "lib/lambda";
45
import {
56
BasicJobData,
67
IssueData,
@@ -262,33 +263,25 @@ export async function backfillMissingLog(
262263
repo: string,
263264
job: RecentWorkflowsData
264265
): Promise<boolean> {
265-
// This creates a mock GitHub workflow_job completion event to reupload the log
266-
// to S3 and trigger log classifier. The action is set to backfill to tell the
267-
// lambda code that this is a mock event body. Note that backfill is not a GitHub
268-
// event actions
269-
const body = {
270-
action: "backfill",
271-
repository: {
272-
full_name: `${owner}/${repo}`,
273-
},
274-
workflow_job: {
266+
// Ask gha-log-uploader to re-fetch the log from GitHub and put it back in S3;
267+
// the S3 notification on log/ re-runs the classifier once it lands. This is a
268+
// direct invoke rather than a POST to the /api/log-uploader/backfill route,
269+
// because we are already inside HUD and a loopback request would only add a
270+
// hop that can fail on its own.
271+
try {
272+
await invokeLogUploader({
273+
repo: `${owner}/${repo}`,
274+
job_id: job.id,
275275
conclusion: job.conclusion,
276-
id: job.id,
277-
},
278-
};
279-
const res = await fetch(
280-
"https://jqogootqqe.execute-api.us-east-1.amazonaws.com/default/github-status-test",
281-
{
282-
method: "POST",
283-
headers: {
284-
Accept: "application/json",
285-
"Content-Type": "application/json",
286-
"X-GitHub-Event": "workflow_job",
287-
},
288-
body: JSON.stringify(body),
289-
}
290-
);
291-
return res.status === 200;
276+
});
277+
return true;
278+
} catch (error) {
279+
console.error(
280+
`Failed to queue a log backfill for ${owner}/${repo} job ${job.id}`,
281+
error
282+
);
283+
return false;
284+
}
292285
}
293286

294287
export function isFailureFromPrevMergeCommit(
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { invokeLogUploader } from "lib/lambda";
2+
import type { NextApiRequest, NextApiResponse } from "next";
3+
4+
/**
5+
* Re-upload a job's log to S3.
6+
*
7+
* Replaces the synthetic `action: "backfill"` event that used to be POSTed at
8+
* github-status-test's API Gateway. That endpoint was public and unauthenticated;
9+
* gha-log-uploader has no public endpoint at all, so this route is the way in for
10+
* callers outside HUD (tools/scripts/backfill_events.py, manual ops).
11+
*
12+
* Code inside HUD should call invokeLogUploader directly rather than looping back
13+
* through here -- see backfillMissingLog in lib/jobUtils.
14+
*/
15+
interface BackfillRequest {
16+
repo?: string;
17+
job_id?: number | string;
18+
conclusion?: string | null;
19+
}
20+
21+
export default async function handler(
22+
req: NextApiRequest,
23+
res: NextApiResponse<{ error: string } | { queued: true }>
24+
) {
25+
if (req.method !== "POST") {
26+
return res.status(405).json({ error: "POST only" });
27+
}
28+
29+
const key = process.env.LOG_UPLOADER_BOT_KEY;
30+
// An unset key must not turn into an open endpoint.
31+
if (!key || req.headers.authorization !== key) {
32+
return res.status(403).json({ error: "Forbidden" });
33+
}
34+
35+
const {
36+
repo,
37+
job_id: rawJobId,
38+
conclusion,
39+
}: BackfillRequest = req.body ?? {};
40+
41+
if (typeof repo !== "string" || !repo.includes("/")) {
42+
return res.status(400).json({ error: "'repo' must be 'owner/name'" });
43+
}
44+
45+
const jobId = Number(rawJobId);
46+
if (!Number.isSafeInteger(jobId) || jobId <= 0) {
47+
return res
48+
.status(400)
49+
.json({ error: "'job_id' must be a positive integer" });
50+
}
51+
52+
try {
53+
await invokeLogUploader({ repo, job_id: jobId, conclusion });
54+
} catch (error) {
55+
console.error(
56+
`Failed to queue a log upload for ${repo} job ${jobId}`,
57+
error
58+
);
59+
return res.status(502).json({ error: "Failed to reach the log uploader" });
60+
}
61+
62+
return res.status(200).json({ queued: true });
63+
}
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import { backfillMissingLog } from "lib/jobUtils";
2+
import * as lambda from "lib/lambda";
3+
import { RecentWorkflowsData } from "lib/types";
4+
5+
const job = { id: 999, conclusion: "failure" } as RecentWorkflowsData;
6+
7+
describe("backfillMissingLog", () => {
8+
afterEach(() => jest.restoreAllMocks());
9+
10+
test("queues an upload and reports success", async () => {
11+
const invoke = jest.spyOn(lambda, "invokeLogUploader").mockResolvedValue();
12+
13+
await expect(
14+
backfillMissingLog("pytorch", "executorch", job)
15+
).resolves.toBe(true);
16+
expect(invoke).toHaveBeenCalledWith({
17+
repo: "pytorch/executorch",
18+
job_id: 999,
19+
conclusion: "failure",
20+
});
21+
});
22+
23+
test.each([
24+
["credentials are missing", new lambda.MissingAwsCredentialsError()],
25+
["the Lambda API is unreachable", new Error("TimeoutError")],
26+
["the role cannot invoke", new Error("AccessDeniedException")],
27+
])("returns false rather than throwing when %s", async (_label, error) => {
28+
// Dr.CI calls this mid-comment-render for every failed job with no log. An
29+
// exception here would take down the whole Dr.CI run over one missing log.
30+
jest.spyOn(lambda, "invokeLogUploader").mockRejectedValue(error);
31+
jest.spyOn(console, "error").mockImplementation(() => {});
32+
33+
await expect(backfillMissingLog("pytorch", "pytorch", job)).resolves.toBe(
34+
false
35+
);
36+
});
37+
});
Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
import * as lambda from "lib/lambda";
2+
import type { NextApiRequest, NextApiResponse } from "next";
3+
import handler from "pages/api/log-uploader/backfill";
4+
5+
function mockRes() {
6+
const res: any = {};
7+
res.status = jest.fn().mockReturnValue(res);
8+
res.json = jest.fn().mockReturnValue(res);
9+
return res as NextApiResponse & {
10+
status: jest.Mock;
11+
json: jest.Mock;
12+
};
13+
}
14+
15+
function mockReq(overrides: Partial<NextApiRequest> = {}): NextApiRequest {
16+
return {
17+
method: "POST",
18+
headers: { authorization: "secret" },
19+
body: { repo: "pytorch/executorch", job_id: 999, conclusion: "failure" },
20+
...overrides,
21+
} as NextApiRequest;
22+
}
23+
24+
describe("/api/log-uploader/backfill", () => {
25+
let invoke: jest.SpyInstance;
26+
27+
beforeEach(() => {
28+
invoke = jest.spyOn(lambda, "invokeLogUploader").mockResolvedValue();
29+
process.env.LOG_UPLOADER_BOT_KEY = "secret";
30+
});
31+
32+
afterEach(() => {
33+
jest.restoreAllMocks();
34+
delete process.env.LOG_UPLOADER_BOT_KEY;
35+
});
36+
37+
test("queues an upload for an authorized request", async () => {
38+
const res = mockRes();
39+
await handler(mockReq(), res);
40+
41+
expect(invoke).toHaveBeenCalledWith({
42+
repo: "pytorch/executorch",
43+
job_id: 999,
44+
conclusion: "failure",
45+
});
46+
expect(res.status).toHaveBeenCalledWith(200);
47+
});
48+
49+
test("accepts a job_id that arrived as a string", async () => {
50+
const res = mockRes();
51+
await handler(
52+
mockReq({ body: { repo: "pytorch/pytorch", job_id: "42" } }),
53+
res
54+
);
55+
56+
expect(invoke).toHaveBeenCalledWith(
57+
expect.objectContaining({ job_id: 42 })
58+
);
59+
});
60+
61+
test("rejects a request with no credentials", async () => {
62+
const res = mockRes();
63+
await handler(mockReq({ headers: {} }), res);
64+
65+
expect(invoke).not.toHaveBeenCalled();
66+
expect(res.status).toHaveBeenCalledWith(403);
67+
});
68+
69+
test("rejects a request with the wrong credentials", async () => {
70+
const res = mockRes();
71+
await handler(mockReq({ headers: { authorization: "guess" } }), res);
72+
73+
expect(invoke).not.toHaveBeenCalled();
74+
expect(res.status).toHaveBeenCalledWith(403);
75+
});
76+
77+
test("an unset key does not become an open endpoint", async () => {
78+
// Otherwise a missing env var would silently reproduce the unauthenticated
79+
// API Gateway endpoint this route exists to replace.
80+
delete process.env.LOG_UPLOADER_BOT_KEY;
81+
const res = mockRes();
82+
await handler(mockReq({ headers: {} }), res);
83+
84+
expect(invoke).not.toHaveBeenCalled();
85+
expect(res.status).toHaveBeenCalledWith(403);
86+
});
87+
88+
test("rejects anything but POST", async () => {
89+
const res = mockRes();
90+
await handler(mockReq({ method: "GET" }), res);
91+
92+
expect(invoke).not.toHaveBeenCalled();
93+
expect(res.status).toHaveBeenCalledWith(405);
94+
});
95+
96+
test.each([
97+
["a missing repo", { job_id: 1 }],
98+
["a repo with no owner", { repo: "pytorch", job_id: 1 }],
99+
["a missing job_id", { repo: "pytorch/pytorch" }],
100+
["a non-numeric job_id", { repo: "pytorch/pytorch", job_id: "abc" }],
101+
["a negative job_id", { repo: "pytorch/pytorch", job_id: -1 }],
102+
])("rejects %s", async (_label, body) => {
103+
const res = mockRes();
104+
await handler(mockReq({ body }), res);
105+
106+
expect(invoke).not.toHaveBeenCalled();
107+
expect(res.status).toHaveBeenCalledWith(400);
108+
});
109+
110+
test("an empty body does not throw", async () => {
111+
const res = mockRes();
112+
await handler(mockReq({ body: undefined }), res);
113+
114+
expect(res.status).toHaveBeenCalledWith(400);
115+
});
116+
117+
test.each([
118+
["credentials are missing", new lambda.MissingAwsCredentialsError()],
119+
["the role cannot invoke", new Error("AccessDeniedException")],
120+
["the Lambda API is unreachable", new Error("TimeoutError")],
121+
])("reports 502 rather than throwing when %s", async (_label, error) => {
122+
invoke.mockRejectedValue(error);
123+
jest.spyOn(console, "error").mockImplementation(() => {});
124+
const res = mockRes();
125+
await handler(mockReq(), res);
126+
127+
expect(res.status).toHaveBeenCalledWith(502);
128+
});
129+
});

0 commit comments

Comments
 (0)