refactor: split webhook accepting and queue dispatching logic (#4160)

## Description

This PR refacting the webhook lambda. It splits the webhook logic in a
part responsible to accept the event and a part to dispatch to the
runner queues. Secondly it refactor and removes duplicate logic in
tests.

This change should allow to split the lambda in two lambda and and use
the AWS EventBridge to accept events, and use the dispacher only for
delegating an event to a runner queueu.

Author: npalm

lambdas/functions/webhook/jest.config.ts

@@ -6,10 +6,10 @@ const config: Config = {
   ...defaultConfig,
   coverageThreshold: {
     global: {
-      statements: 99.13,
-      branches: 96.87,
+      statements: 99.2,
+      branches: 100,
       functions: 100,
-      lines: 99.09,
+      lines: 99.25,
     },
   },
 };

lambdas/functions/webhook/src/ValidatonError.ts renamed to lambdas/functions/webhook/src/ValidationError.ts

@@ -4,7 +4,7 @@ import { mocked } from 'jest-mock';
 
 import { githubWebhook } from './lambda';
 import { handle } from './webhook';
-import ValidationError from './ValidatonError';
+import ValidationError from './ValidationError';
 import { getParameter } from '@aws-github-runner/aws-ssm-util';
 
 const event: APIGatewayEvent = {
@@ -85,28 +85,28 @@ describe('Test scale up lambda wrapper.', () => {
     const mock = mocked(handle);
     mock.mockImplementation(() => {
       return new Promise((resolve) => {
-        resolve({ statusCode: 200 });
+        resolve({ body: 'test', statusCode: 200 });
       });
     });
 
     const result = await githubWebhook(event, context);
-    expect(result).toEqual({ statusCode: 200 });
+    expect(result).toEqual({ body: 'test', statusCode: 200 });
   });
 
   it('An expected error, resolve.', async () => {
     const mock = mocked(handle);
     mock.mockRejectedValue(new ValidationError(400, 'some error'));
 
     const result = await githubWebhook(event, context);
-    expect(result).toMatchObject({ statusCode: 400 });
+    expect(result).toMatchObject({ body: 'some error', statusCode: 400 });
   });
 
   it('Errors are not thrown.', async () => {
     const mock = mocked(handle);
     const logSpy = jest.spyOn(logger, 'error');
     mock.mockRejectedValue(new Error('some error'));
     const result = await githubWebhook(event, context);
-    expect(result).toMatchObject({ statusCode: 500 });
-    expect(logSpy).toBeCalledTimes(1);
+    expect(result).toMatchObject({ body: 'Check the Lambda logs for the error details.', statusCode: 500 });
+    expect(logSpy).toHaveBeenCalledTimes(1);
   });
 });

lambdas/functions/webhook/src/lambda.test.ts

@@ -5,11 +5,11 @@ import { APIGatewayEvent, Context } from 'aws-lambda';
 import { handle } from './webhook';
 import { Config } from './ConfigResolver';
 import { IncomingHttpHeaders } from 'http';
-import ValidationError from './ValidatonError';
+import ValidationError from './ValidationError';
 
 export interface Response {
   statusCode: number;
-  body?: string;
+  body: string;
 }
 
 middy(githubWebhook).use(captureLambdaHandler(tracer));

lambdas/functions/webhook/src/lambda.ts

@@ -0,0 +1,255 @@
+import { getParameter } from '@aws-github-runner/aws-ssm-util';
+import { mocked } from 'jest-mock';
+import nock from 'nock';
+import { WorkflowJobEvent } from '@octokit/webhooks-types';
+
+import workFlowJobEvent from '../../test/resources/github_workflowjob_event.json';
+import runnerConfig from '../../test/resources/multi_runner_configurations.json';
+
+import { RunnerConfig, sendActionRequest } from '../sqs';
+import { canRunJob, dispatch } from './dispatch';
+import { Config } from '../ConfigResolver';
+
+jest.mock('../sqs');
+jest.mock('@aws-github-runner/aws-ssm-util');
+
+const sendWebhookEventToWorkflowJobQueueMock = jest.mocked(sendActionRequest);
+const GITHUB_APP_WEBHOOK_SECRET = 'TEST_SECRET';
+
+const cleanEnv = process.env;
+
+describe('Dispatcher', () => {
+  let originalError: Console['error'];
+  let config: Config;
+
+  beforeEach(async () => {
+    process.env = { ...cleanEnv };
+
+    nock.disableNetConnect();
+    originalError = console.error;
+    console.error = jest.fn();
+    jest.clearAllMocks();
+    jest.resetAllMocks();
+
+    mockSSMResponse();
+    config = await createConfig(undefined, runnerConfig);
+  });
+
+  afterEach(() => {
+    console.error = originalError;
+  });
+
+  describe('handle work flow job events ', () => {
+    it('should not handle "workflow_job" events with actions other than queued (action = started)', async () => {
+      const event = { ...workFlowJobEvent, action: 'started' } as unknown as WorkflowJobEvent;
+      const resp = await dispatch(event, 'workflow_job', config);
+      expect(resp.statusCode).toBe(201);
+      expect(sendActionRequest).not.toHaveBeenCalled();
+    });
+
+    it('should not handle workflow_job events from unlisted repositories', async () => {
+      const event = workFlowJobEvent as unknown as WorkflowJobEvent;
+      config = await createConfig(['NotCodertocat/Hello-World']);
+      await expect(dispatch(event, 'push', config)).rejects.toMatchObject({
+        statusCode: 403,
+      });
+      expect(sendActionRequest).not.toHaveBeenCalled();
+      expect(sendWebhookEventToWorkflowJobQueueMock).not.toHaveBeenCalled();
+    });
+
+    it('should handle workflow_job events without installation id', async () => {
+      config = await createConfig(['philips-labs/terraform-aws-github-runner']);
+      const event = { ...workFlowJobEvent, installation: null } as unknown as WorkflowJobEvent;
+      const resp = await dispatch(event, 'workflow_job', config);
+      expect(resp.statusCode).toBe(201);
+      expect(sendActionRequest).toHaveBeenCalled();
+      expect(sendWebhookEventToWorkflowJobQueueMock).toHaveBeenCalled();
+    });
+
+    it('should handle workflow_job events from allow listed repositories', async () => {
+      config = await createConfig(['philips-labs/terraform-aws-github-runner']);
+      const event = workFlowJobEvent as unknown as WorkflowJobEvent;
+      const resp = await dispatch(event, 'workflow_job', config);
+      expect(resp.statusCode).toBe(201);
+      expect(sendActionRequest).toHaveBeenCalled();
+      expect(sendWebhookEventToWorkflowJobQueueMock).toHaveBeenCalled();
+    });
+
+    it('should match labels', async () => {
+      config = await createConfig(undefined, [
+        {
+          ...runnerConfig[0],
+          matcherConfig: {
+            labelMatchers: [['self-hosted', 'test']],
+            exactMatch: true,
+          },
+        },
+        runnerConfig[1],
+      ]);
+
+      const event = {
+        ...workFlowJobEvent,
+        workflow_job: {
+          ...workFlowJobEvent.workflow_job,
+          labels: ['self-hosted', 'Test'],
+        },
+      } as unknown as WorkflowJobEvent;
+      const resp = await dispatch(event, 'workflow_job', config);
+      expect(resp.statusCode).toBe(201);
+      expect(sendActionRequest).toHaveBeenCalledWith({
+        id: event.workflow_job.id,
+        repositoryName: event.repository.name,
+        repositoryOwner: event.repository.owner.login,
+        eventType: 'workflow_job',
+        installationId: 0,
+        queueId: runnerConfig[0].id,
+        queueFifo: false,
+        repoOwnerType: 'Organization',
+      });
+      expect(sendWebhookEventToWorkflowJobQueueMock).toHaveBeenCalled();
+    });
+
+    it('should sort matcher with exact first.', async () => {
+      config = await createConfig(undefined, [
+        {
+          ...runnerConfig[0],
+          matcherConfig: {
+            labelMatchers: [['self-hosted', 'match', 'not-select']],
+            exactMatch: false,
+          },
+        },
+        {
+          ...runnerConfig[0],
+          matcherConfig: {
+            labelMatchers: [['self-hosted', 'no-match']],
+            exactMatch: true,
+          },
+        },
+        {
+          ...runnerConfig[0],
+          id: 'match',
+          matcherConfig: {
+            labelMatchers: [['self-hosted', 'match']],
+            exactMatch: true,
+          },
+        },
+        runnerConfig[1],
+      ]);
+
+      const event = {
+        ...workFlowJobEvent,
+        workflow_job: {
+          ...workFlowJobEvent.workflow_job,
+          labels: ['self-hosted', 'match'],
+        },
+      } as unknown as WorkflowJobEvent;
+      const resp = await dispatch(event, 'workflow_job', config);
+      expect(resp.statusCode).toBe(201);
+      expect(sendActionRequest).toHaveBeenCalledWith({
+        id: event.workflow_job.id,
+        repositoryName: event.repository.name,
+        repositoryOwner: event.repository.owner.login,
+        eventType: 'workflow_job',
+        installationId: 0,
+        queueId: 'match',
+        queueFifo: false,
+        repoOwnerType: 'Organization',
+      });
+      expect(sendWebhookEventToWorkflowJobQueueMock).toHaveBeenCalled();
+    });
+
+    it('should not accept jobs where not all labels are supported (single matcher).', async () => {
+      config = await createConfig(undefined, [
+        {
+          ...runnerConfig[0],
+          matcherConfig: {
+            labelMatchers: [['self-hosted', 'x64', 'linux']],
+            exactMatch: true,
+          },
+        },
+      ]);
+
+      const event = {
+        ...workFlowJobEvent,
+        workflow_job: {
+          ...workFlowJobEvent.workflow_job,
+          labels: ['self-hosted', 'linux', 'x64', 'on-demand'],
+        },
+      } as unknown as WorkflowJobEvent;
+      const resp = await dispatch(event, 'workflow_job', config);
+      expect(resp.statusCode).toBe(202);
+      expect(sendActionRequest).not.toHaveBeenCalled();
+      expect(sendWebhookEventToWorkflowJobQueueMock).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('decides can run job based on label and config (canRunJob)', () => {
+    it('should accept job with an exact match and identical labels.', () => {
+      const workflowLabels = ['self-hosted', 'linux', 'x64', 'ubuntu-latest'];
+      const runnerLabels = [['self-hosted', 'linux', 'x64', 'ubuntu-latest']];
+      expect(canRunJob(workflowLabels, runnerLabels, true)).toBe(true);
+    });
+
+    it('should accept job with an exact match and identical labels, ignoring cases.', () => {
+      const workflowLabels = ['self-Hosted', 'Linux', 'X64', 'ubuntu-Latest'];
+      const runnerLabels = [['self-hosted', 'linux', 'x64', 'ubuntu-latest']];
+      expect(canRunJob(workflowLabels, runnerLabels, true)).toBe(true);
+    });
+
+    it('should accept job with an exact match and runner supports requested capabilities.', () => {
+      const workflowLabels = ['self-hosted', 'linux', 'x64'];
+      const runnerLabels = [['self-hosted', 'linux', 'x64', 'ubuntu-latest']];
+      expect(canRunJob(workflowLabels, runnerLabels, true)).toBe(true);
+    });
+
+    it('should NOT accept job with an exact match and runner not matching requested capabilities.', () => {
+      const workflowLabels = ['self-hosted', 'linux', 'x64', 'ubuntu-latest'];
+      const runnerLabels = [['self-hosted', 'linux', 'x64']];
+      expect(canRunJob(workflowLabels, runnerLabels, true)).toBe(false);
+    });
+
+    it('should accept job with for a non exact match. Any label that matches will accept the job.', () => {
+      const workflowLabels = ['self-hosted', 'linux', 'x64', 'ubuntu-latest', 'gpu'];
+      const runnerLabels = [['gpu']];
+      expect(canRunJob(workflowLabels, runnerLabels, false)).toBe(true);
+    });
+
+    it('should NOT accept job with for an exact match. Not all requested capabilites are supported.', () => {
+      const workflowLabels = ['self-hosted', 'linux', 'x64', 'ubuntu-latest', 'gpu'];
+      const runnerLabels = [['gpu']];
+      expect(canRunJob(workflowLabels, runnerLabels, true)).toBe(false);
+    });
+
+    it('should not accept jobs not providing labels if exact match is.', () => {
+      const workflowLabels: string[] = [];
+      const runnerLabels = [['self-hosted', 'linux', 'x64']];
+      expect(canRunJob(workflowLabels, runnerLabels, true)).toBe(false);
+    });
+
+    it('should accept jobs not providing labels and exact match is set to false.', () => {
+      const workflowLabels: string[] = [];
+      const runnerLabels = [['self-hosted', 'linux', 'x64']];
+      expect(canRunJob(workflowLabels, runnerLabels, false)).toBe(true);
+    });
+  });
+});
+
+function mockSSMResponse(runnerConfigInput?: RunnerConfig) {
+  const mockedGet = mocked(getParameter);
+  mockedGet.mockImplementation((parameter_name) => {
+    const value =
+      parameter_name == '/github-runner/runner-matcher-config'
+        ? JSON.stringify(runnerConfigInput ?? runnerConfig)
+        : GITHUB_APP_WEBHOOK_SECRET;
+    return Promise.resolve(value);
+  });
+}
+
+async function createConfig(repositoryAllowList?: string[], runnerConfig?: RunnerConfig): Promise<Config> {
+  if (repositoryAllowList) {
+    process.env.REPOSITORY_ALLOW_LIST = JSON.stringify(repositoryAllowList);
+  }
+  Config.reset();
+  mockSSMResponse(runnerConfig);
+  return await Config.load();
+}