fix: update return codes, no error code for job that are ignored (#1381)

philips-labs · Nov 9, 2021 · f9f705f · f9f705f
1 parent fcfbc53
commit f9f705f
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 39 deletions.
diff --git a/modules/webhook/lambdas/webhook/src/lambda.ts b/modules/webhook/lambdas/webhook/src/lambda.ts
@@ -2,14 +2,17 @@ import { handle } from './webhook/handler';
 import { APIGatewayEvent, Context, Callback } from 'aws-lambda';
 import { logger } from './webhook/logger';
 
+export interface Response {
+  statusCode: number;
+  body?: string;
+}
+
 export const githubWebhook = async (event: APIGatewayEvent, context: Context, callback: Callback): Promise<void> => {
   logger.setSettings({ requestId: context.awsRequestId });
   logger.debug(JSON.stringify(event));
   try {
-    const statusCode = await handle(event.headers, event.body as string);
-    callback(null, {
-      statusCode: statusCode,
-    });
+    const response = await handle(event.headers, event.body as string);
+    callback(null, response);
   } catch (e) {
     callback(e as Error);
   }

diff --git a/modules/webhook/lambdas/webhook/src/local.ts b/modules/webhook/lambdas/webhook/src/local.ts
@@ -8,7 +8,7 @@ app.use(bodyParser.json());
 
 app.post('/event_handler', (req, res) => {
   handle(req.headers, JSON.stringify(req.body))
-    .then((c) => res.status(c).end())
+    .then((c) => res.status(c.statusCode).end())
     .catch((e) => {
       console.log(e);
       res.status(404);

diff --git a/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts b/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts
@@ -38,12 +38,12 @@ describe('handler', () => {
 
   it('returns 500 if no signature available', async () => {
     const resp = await handle({}, '');
-    expect(resp).toBe(500);
+    expect(resp.statusCode).toBe(500);
   });
 
   it('returns 401 if signature is invalid', async () => {
     const resp = await handle({ 'X-Hub-Signature': 'bbb' }, 'aaaa');
-    expect(resp).toBe(401);
+    expect(resp.statusCode).toBe(401);
   });
 
   describe('Test for workflowjob event: ', () => {
@@ -56,14 +56,14 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
 
     it('does not handle other events', async () => {
       const event = JSON.stringify(workflowjob_event);
       const resp = await handle({ 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'push' }, event);
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(202);
       expect(sendActionRequest).not.toBeCalled();
     });
 
@@ -73,7 +73,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).not.toBeCalled();
     });
 
@@ -83,7 +83,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).not.toBeCalled();
     });
 
@@ -94,7 +94,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(403);
+      expect(resp.statusCode).toBe(403);
       expect(sendActionRequest).not.toBeCalled();
     });
 
@@ -105,7 +105,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
 
@@ -122,7 +122,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
 
@@ -139,7 +139,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
 
@@ -156,7 +156,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
 
@@ -173,7 +173,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
 
@@ -191,7 +191,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
     it('Check not allowed runner label is declined', async () => {
@@ -207,7 +207,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' },
         event,
       );
-      expect(resp).toBe(403);
+      expect(resp.statusCode).toBe(202);
       expect(sendActionRequest).not.toBeCalled();
     });
   });
@@ -219,7 +219,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
 
@@ -229,7 +229,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).not.toBeCalled();
     });
 
@@ -239,7 +239,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).not.toBeCalled();
     });
 
@@ -250,7 +250,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' },
         event,
       );
-      expect(resp).toBe(403);
+      expect(resp.statusCode).toBe(403);
       expect(sendActionRequest).not.toBeCalled();
     });
 
@@ -261,7 +261,7 @@ describe('handler', () => {
         { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' },
         event,
       );
-      expect(resp).toBe(200);
+      expect(resp.statusCode).toBe(201);
       expect(sendActionRequest).toBeCalled();
     });
   });

diff --git a/modules/webhook/lambdas/webhook/src/webhook/handler.ts b/modules/webhook/lambdas/webhook/src/webhook/handler.ts
@@ -4,38 +4,48 @@ import { sendActionRequest } from '../sqs';
 import { CheckRunEvent, WorkflowJobEvent } from '@octokit/webhooks-types';
 import { getParameterValue } from '../ssm';
 import { logger as rootLogger } from './logger';
+import { Response } from '../lambda';
 
 const logger = rootLogger.getChildLogger();
 
-export async function handle(headers: IncomingHttpHeaders, body: string): Promise<number> {
+export async function handle(headers: IncomingHttpHeaders, body: string): Promise<Response> {
   // ensure header keys lower case since github headers can contain capitals.
   for (const key in headers) {
     headers[key.toLowerCase()] = headers[key];
   }
 
   const githubEvent = headers['x-github-event'] as string;
 
-  let status = await verifySignature(githubEvent, headers['x-hub-signature'] as string, body);
-  if (status != 200) {
-    return status;
+  let response: Response = {
+    statusCode: await verifySignature(githubEvent, headers['x-hub-signature'] as string, body),
+  };
+
+  if (response.statusCode != 200) {
+    return response;
   }
   const payload = JSON.parse(body);
   logger.info(`Received Github event ${githubEvent} from ${payload.repository.full_name}`);
 
   if (isRepoNotAllowed(payload.repository.full_name)) {
-    console.error(`Received event from unauthorized repository ${payload.repository.full_name}`);
-    return 403;
+    console.warn(`Received event from unauthorized repository ${payload.repository.full_name}`);
+    return {
+      statusCode: 403,
+    };
   }
 
   if (githubEvent == 'workflow_job') {
-    status = await handleWorkflowJob(payload as WorkflowJobEvent, githubEvent);
+    response = await handleWorkflowJob(payload as WorkflowJobEvent, githubEvent);
   } else if (githubEvent == 'check_run') {
-    status = await handleCheckRun(payload as CheckRunEvent, githubEvent);
+    response = await handleCheckRun(payload as CheckRunEvent, githubEvent);
   } else {
     logger.warn(`Ignoring unsupported event ${githubEvent}`);
+    response = {
+      statusCode: 202,
+      body: `Ignoring unsupported event ${githubEvent}`,
+    };
   }
 
-  return status;
+  return response;
 }
 
 async function verifySignature(githubEvent: string, signature: string, body: string): Promise<number> {
@@ -56,12 +66,15 @@ async function verifySignature(githubEvent: string, signature: string, body: str
   return 200;
 }
 
-async function handleWorkflowJob(body: WorkflowJobEvent, githubEvent: string): Promise<number> {
+async function handleWorkflowJob(body: WorkflowJobEvent, githubEvent: string): Promise<Response> {
   const disableCheckWorkflowJobLabelsEnv = process.env.DISABLE_CHECK_WORKFLOW_JOB_LABELS || 'false';
   const disableCheckWorkflowJobLabels = JSON.parse(disableCheckWorkflowJobLabelsEnv) as boolean;
   if (!disableCheckWorkflowJobLabels && !canRunJob(body)) {
-    logger.error(`Received event contains runner labels '${body.workflow_job.labels}' that are not accepted.`);
-    return 403;
+    logger.warn(`Received event contains runner labels '${body.workflow_job.labels}' that are not accepted.`);
+    return {
+      statusCode: 202,
+      body: `Received event contains runner labels '${body.workflow_job.labels}' that are not accepted.`,
+    };
   }
 
   let installationId = body.installation?.id;
@@ -78,10 +91,10 @@ async function handleWorkflowJob(body: WorkflowJobEvent, githubEvent: string): P
     });
   }
   console.info(`Successfully queued job for ${body.repository.full_name}`);
-  return 200;
+  return { statusCode: 201 };
 }
 
-async function handleCheckRun(body: CheckRunEvent, githubEvent: string): Promise<number> {
+async function handleCheckRun(body: CheckRunEvent, githubEvent: string): Promise<Response> {
   let installationId = body.installation?.id;
   if (installationId == null) {
     installationId = 0;
@@ -96,7 +109,7 @@ async function handleCheckRun(body: CheckRunEvent, githubEvent: string): Promise
     });
   }
   console.info(`Successfully queued job for ${body.repository.full_name}`);
-  return 200;
+  return { statusCode: 201 };
 }
 
 function isRepoNotAllowed(repo_full_name: string): boolean {