Move log backfill off the public API Gateway endpoint

torchci/lib/jobUtils.ts

@@ -1,6 +1,7 @@
 import { durationDisplay } from "components/common/TimeUtils";
 import dayjs from "dayjs";
 import { jaroWinkler } from "jaro-winkler-typescript";
+import { invokeLogUploader } from "lib/lambda";
 import {
   BasicJobData,
   IssueData,
@@ -262,33 +263,25 @@ export async function backfillMissingLog(
   repo: string,
   job: RecentWorkflowsData
 ): Promise<boolean> {
-  // This creates a mock GitHub workflow_job completion event to reupload the log
-  // to S3 and trigger log classifier. The action is set to backfill to tell the
-  // lambda code that this is a mock event body. Note that backfill is not a GitHub
-  // event actions
-  const body = {
-    action: "backfill",
-    repository: {
-      full_name: `${owner}/${repo}`,
-    },
-    workflow_job: {
+  // Ask gha-log-uploader to re-fetch the log from GitHub and put it back in S3;
+  // the S3 notification on log/ re-runs the classifier once it lands. This is a
+  // direct invoke rather than a POST to the /api/log-uploader/backfill route,
+  // because we are already inside HUD and a loopback request would only add a
+  // hop that can fail on its own.
+  try {
+    await invokeLogUploader({
+      repo: `${owner}/${repo}`,
+      job_id: job.id,
       conclusion: job.conclusion,
-      id: job.id,
-    },
-  };
-  const res = await fetch(
-    "https://jqogootqqe.execute-api.us-east-1.amazonaws.com/default/github-status-test",
-    {
-      method: "POST",
-      headers: {
-        Accept: "application/json",
-        "Content-Type": "application/json",
-        "X-GitHub-Event": "workflow_job",
-      },
-      body: JSON.stringify(body),
-    }
-  );
-  return res.status === 200;
+    });
+    return true;
+  } catch (error) {
+    console.error(
+      `Failed to queue a log backfill for ${owner}/${repo} job ${job.id}`,
+      error
+    );
+    return false;
+  }
 }
 
 export function isFailureFromPrevMergeCommit(

torchci/pages/api/log-uploader/backfill.ts

@@ -0,0 +1,63 @@
+import { invokeLogUploader } from "lib/lambda";
+import type { NextApiRequest, NextApiResponse } from "next";
+
+/**
+ * Re-upload a job's log to S3.
+ *
+ * Replaces the synthetic `action: "backfill"` event that used to be POSTed at
+ * github-status-test's API Gateway. That endpoint was public and unauthenticated;
+ * gha-log-uploader has no public endpoint at all, so this route is the way in for
+ * callers outside HUD (tools/scripts/backfill_events.py, manual ops).
+ *
+ * Code inside HUD should call invokeLogUploader directly rather than looping back
+ * through here -- see backfillMissingLog in lib/jobUtils.
+ */
+interface BackfillRequest {
+  repo?: string;
+  job_id?: number | string;
+  conclusion?: string | null;
+}
+
+export default async function handler(
+  req: NextApiRequest,
+  res: NextApiResponse<{ error: string } | { queued: true }>
+) {
+  if (req.method !== "POST") {
+    return res.status(405).json({ error: "POST only" });
+  }
+
+  const key = process.env.LOG_UPLOADER_BOT_KEY;
+  // An unset key must not turn into an open endpoint.
+  if (!key || req.headers.authorization !== key) {
+    return res.status(403).json({ error: "Forbidden" });
+  }
+
+  const {
+    repo,
+    job_id: rawJobId,
+    conclusion,
+  }: BackfillRequest = req.body ?? {};
+
+  if (typeof repo !== "string" || !repo.includes("/")) {
+    return res.status(400).json({ error: "'repo' must be 'owner/name'" });
+  }
+
+  const jobId = Number(rawJobId);
+  if (!Number.isSafeInteger(jobId) || jobId <= 0) {
+    return res
+      .status(400)
+      .json({ error: "'job_id' must be a positive integer" });
+  }
+
+  try {
+    await invokeLogUploader({ repo, job_id: jobId, conclusion });
+  } catch (error) {
+    console.error(
+      `Failed to queue a log upload for ${repo} job ${jobId}`,
+      error
+    );
+    return res.status(502).json({ error: "Failed to reach the log uploader" });
+  }
+
+  return res.status(200).json({ queued: true });
+}

torchci/test/backfillMissingLog.test.ts

@@ -0,0 +1,37 @@
+import { backfillMissingLog } from "lib/jobUtils";
+import * as lambda from "lib/lambda";
+import { RecentWorkflowsData } from "lib/types";
+
+const job = { id: 999, conclusion: "failure" } as RecentWorkflowsData;
+
+describe("backfillMissingLog", () => {
+  afterEach(() => jest.restoreAllMocks());
+
+  test("queues an upload and reports success", async () => {
+    const invoke = jest.spyOn(lambda, "invokeLogUploader").mockResolvedValue();
+
+    await expect(
+      backfillMissingLog("pytorch", "executorch", job)
+    ).resolves.toBe(true);
+    expect(invoke).toHaveBeenCalledWith({
+      repo: "pytorch/executorch",
+      job_id: 999,
+      conclusion: "failure",
+    });
+  });
+
+  test.each([
+    ["credentials are missing", new lambda.MissingAwsCredentialsError()],
+    ["the Lambda API is unreachable", new Error("TimeoutError")],
+    ["the role cannot invoke", new Error("AccessDeniedException")],
+  ])("returns false rather than throwing when %s", async (_label, error) => {
+    // Dr.CI calls this mid-comment-render for every failed job with no log. An
+    // exception here would take down the whole Dr.CI run over one missing log.
+    jest.spyOn(lambda, "invokeLogUploader").mockRejectedValue(error);
+    jest.spyOn(console, "error").mockImplementation(() => {});
+
+    await expect(backfillMissingLog("pytorch", "pytorch", job)).resolves.toBe(
+      false
+    );
+  });
+});

torchci/test/logUploaderBackfill.test.ts

@@ -0,0 +1,129 @@
+import * as lambda from "lib/lambda";
+import type { NextApiRequest, NextApiResponse } from "next";
+import handler from "pages/api/log-uploader/backfill";
+
+function mockRes() {
+  const res: any = {};
+  res.status = jest.fn().mockReturnValue(res);
+  res.json = jest.fn().mockReturnValue(res);
+  return res as NextApiResponse & {
+    status: jest.Mock;
+    json: jest.Mock;
+  };
+}
+
+function mockReq(overrides: Partial<NextApiRequest> = {}): NextApiRequest {
+  return {
+    method: "POST",
+    headers: { authorization: "secret" },
+    body: { repo: "pytorch/executorch", job_id: 999, conclusion: "failure" },
+    ...overrides,
+  } as NextApiRequest;
+}
+
+describe("/api/log-uploader/backfill", () => {
+  let invoke: jest.SpyInstance;
+
+  beforeEach(() => {
+    invoke = jest.spyOn(lambda, "invokeLogUploader").mockResolvedValue();
+    process.env.LOG_UPLOADER_BOT_KEY = "secret";
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+    delete process.env.LOG_UPLOADER_BOT_KEY;
+  });
+
+  test("queues an upload for an authorized request", async () => {
+    const res = mockRes();
+    await handler(mockReq(), res);
+
+    expect(invoke).toHaveBeenCalledWith({
+      repo: "pytorch/executorch",
+      job_id: 999,
+      conclusion: "failure",
+    });
+    expect(res.status).toHaveBeenCalledWith(200);
+  });
+
+  test("accepts a job_id that arrived as a string", async () => {
+    const res = mockRes();
+    await handler(
+      mockReq({ body: { repo: "pytorch/pytorch", job_id: "42" } }),
+      res
+    );
+
+    expect(invoke).toHaveBeenCalledWith(
+      expect.objectContaining({ job_id: 42 })
+    );
+  });
+
+  test("rejects a request with no credentials", async () => {
+    const res = mockRes();
+    await handler(mockReq({ headers: {} }), res);
+
+    expect(invoke).not.toHaveBeenCalled();
+    expect(res.status).toHaveBeenCalledWith(403);
+  });
+
+  test("rejects a request with the wrong credentials", async () => {
+    const res = mockRes();
+    await handler(mockReq({ headers: { authorization: "guess" } }), res);
+
+    expect(invoke).not.toHaveBeenCalled();
+    expect(res.status).toHaveBeenCalledWith(403);
+  });
+
+  test("an unset key does not become an open endpoint", async () => {
+    // Otherwise a missing env var would silently reproduce the unauthenticated
+    // API Gateway endpoint this route exists to replace.
+    delete process.env.LOG_UPLOADER_BOT_KEY;
+    const res = mockRes();
+    await handler(mockReq({ headers: {} }), res);
+
+    expect(invoke).not.toHaveBeenCalled();
+    expect(res.status).toHaveBeenCalledWith(403);
+  });
+
+  test("rejects anything but POST", async () => {
+    const res = mockRes();
+    await handler(mockReq({ method: "GET" }), res);
+
+    expect(invoke).not.toHaveBeenCalled();
+    expect(res.status).toHaveBeenCalledWith(405);
+  });
+
+  test.each([
+    ["a missing repo", { job_id: 1 }],
+    ["a repo with no owner", { repo: "pytorch", job_id: 1 }],
+    ["a missing job_id", { repo: "pytorch/pytorch" }],
+    ["a non-numeric job_id", { repo: "pytorch/pytorch", job_id: "abc" }],
+    ["a negative job_id", { repo: "pytorch/pytorch", job_id: -1 }],
+  ])("rejects %s", async (_label, body) => {
+    const res = mockRes();
+    await handler(mockReq({ body }), res);
+
+    expect(invoke).not.toHaveBeenCalled();
+    expect(res.status).toHaveBeenCalledWith(400);
+  });
+
+  test("an empty body does not throw", async () => {
+    const res = mockRes();
+    await handler(mockReq({ body: undefined }), res);
+
+    expect(res.status).toHaveBeenCalledWith(400);
+  });
+
+  test.each([
+    ["credentials are missing", new lambda.MissingAwsCredentialsError()],
+    ["the role cannot invoke", new Error("AccessDeniedException")],
+    ["the Lambda API is unreachable", new Error("TimeoutError")],
+  ])("reports 502 rather than throwing when %s", async (_label, error) => {
+    invoke.mockRejectedValue(error);
+    jest.spyOn(console, "error").mockImplementation(() => {});
+    const res = mockRes();
+    await handler(mockReq(), res);
+
+    expect(res.status).toHaveBeenCalledWith(502);
+  });
+});