Skip to content
This repository was archived by the owner on Jan 16, 2025. It is now read-only.

Commit cb78738

Browse files
authored
fix(webhook): Avoid jobs are accepted without labels (#3548)
## Descirption When using the mult-runner with an exact match set to `true` worklfows events with an empty array of labels matches the first matcher. This cause not intented runners are created. This change avoids jobs with no labels ar accepted. The exactMatch is some bagage we ccarry from the past. I have aaded a set of unit tests to show how the match is working. We should think about removing the switch.
1 parent a0890af commit cb78738

File tree

4 files changed

+63
-11
lines changed

4 files changed

+63
-11
lines changed

Diff for: lambdas/functions/webhook/jest.config.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@ const config: Config = {
66
...defaultConfig,
77
coverageThreshold: {
88
global: {
9-
statements: 99,
10-
branches: 86,
9+
statements: 99.07,
10+
branches: 93.33,
1111
functions: 100,
12-
lines: 99,
12+
lines: 99.02,
1313
},
1414
},
1515
};

Diff for: lambdas/functions/webhook/src/webhook/handler.test.ts

+56-5
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import checkrun_event from '../../test/resources/github_check_run_event.json';
77
import workflowjob_event from '../../test/resources/github_workflowjob_event.json';
88
import queuesConfig from '../../test/resources/multi_runner_configurations.json';
99
import { sendActionRequest } from '../sqs';
10-
import { handle } from './handler';
10+
import { canRunJob, handle } from './handler';
1111

1212
jest.mock('../sqs');
1313
jest.mock('@terraform-aws-github-runner/aws-ssm-util');
@@ -261,7 +261,7 @@ describe('handler', () => {
261261
event,
262262
);
263263
expect(resp.statusCode).toBe(202);
264-
expect(sendActionRequest).not.toBeCalled;
264+
expect(sendActionRequest).not.toBeCalled();
265265
});
266266

267267
it('Check webhook does not accept jobs where the job labels are spread across label matchers.', async () => {
@@ -289,7 +289,7 @@ describe('handler', () => {
289289
event,
290290
);
291291
expect(resp.statusCode).toBe(202);
292-
expect(sendActionRequest).not.toBeCalled;
292+
expect(sendActionRequest).not.toBeCalled();
293293
});
294294

295295
it('Check webhook does not accept jobs where not all labels are supported by the runner.', async () => {
@@ -321,7 +321,7 @@ describe('handler', () => {
321321
event,
322322
);
323323
expect(resp.statusCode).toBe(202);
324-
expect(sendActionRequest).not.toBeCalled;
324+
expect(sendActionRequest).not.toBeCalled();
325325
});
326326

327327
it('Check webhook will accept jobs with a single acceptable label.', async () => {
@@ -385,7 +385,7 @@ describe('handler', () => {
385385
event,
386386
);
387387
expect(resp.statusCode).toBe(202);
388-
expect(sendActionRequest).not.toBeCalled;
388+
expect(sendActionRequest).not.toBeCalled();
389389
});
390390
it('Check webhook will accept jobs for specific labels if workflow labels are specific', async () => {
391391
process.env.RUNNER_CONFIG = JSON.stringify([
@@ -520,3 +520,54 @@ describe('handler', () => {
520520
});
521521
});
522522
});
523+
524+
describe('canRunJob', () => {
525+
it('should accept job with an exact match and identical labels.', () => {
526+
const workflowLabels = ['self-hosted', 'linux', 'x64', 'ubuntu-latest'];
527+
const runnerLabels = [['self-hosted', 'linux', 'x64', 'ubuntu-latest']];
528+
const exactMatch = true;
529+
expect(canRunJob(workflowLabels, runnerLabels, exactMatch)).toBe(true);
530+
});
531+
532+
it('should accept job with an exact match and runner supports requested capabilites.', () => {
533+
const workflowLabels = ['self-hosted', 'linux', 'x64'];
534+
const runnerLabels = [['self-hosted', 'linux', 'x64', 'ubuntu-latest']];
535+
const exactMatch = true;
536+
expect(canRunJob(workflowLabels, runnerLabels, exactMatch)).toBe(true);
537+
});
538+
539+
it('should NOT accept job with an exact match and runner not matching requested capabilites.', () => {
540+
const workflowLabels = ['self-hosted', 'linux', 'x64', 'ubuntu-latest'];
541+
const runnerLabels = [['self-hosted', 'linux', 'x64']];
542+
const exactMatch = true;
543+
expect(canRunJob(workflowLabels, runnerLabels, exactMatch)).toBe(false);
544+
});
545+
546+
it('should accept job with for a non exact match. Any label that matches will accept the job.', () => {
547+
const workflowLabels = ['self-hosted', 'linux', 'x64', 'ubuntu-latest', 'gpu'];
548+
const runnerLabels = [['gpu']];
549+
const exactMatch = false;
550+
expect(canRunJob(workflowLabels, runnerLabels, exactMatch)).toBe(true);
551+
});
552+
553+
it('should NOT accept job with for an exact match. Not all requested capabilites are supported.', () => {
554+
const workflowLabels = ['self-hosted', 'linux', 'x64', 'ubuntu-latest', 'gpu'];
555+
const runnerLabels = [['gpu']];
556+
const exactMatch = true;
557+
expect(canRunJob(workflowLabels, runnerLabels, exactMatch)).toBe(false);
558+
});
559+
560+
it('Should not accecpt jobs not providing labels if exact match is.', () => {
561+
const workflowLabels: string[] = [];
562+
const runnerLabels = [['self-hosted', 'linux', 'x64']];
563+
const exactMatch = true;
564+
expect(canRunJob(workflowLabels, runnerLabels, exactMatch)).toBe(false);
565+
});
566+
567+
it('Should accept jobs not providing labels and exact match is set to false.', () => {
568+
const workflowLabels: string[] = [];
569+
const runnerLabels = [['self-hosted', 'linux', 'x64']];
570+
const exactMatch = false;
571+
expect(canRunJob(workflowLabels, runnerLabels, exactMatch)).toBe(true);
572+
});
573+
});

Diff for: lambdas/functions/webhook/src/webhook/handler.ts

+3-2
Original file line numberDiff line numberDiff line change
@@ -158,17 +158,18 @@ function isRepoNotAllowed(repoFullName: string, repositoryWhiteList: string[]):
158158
return repositoryWhiteList.length > 0 && !repositoryWhiteList.includes(repoFullName);
159159
}
160160

161-
function canRunJob(
161+
export function canRunJob(
162162
workflowJobLabels: string[],
163163
runnerLabelsMatchers: string[][],
164164
workflowLabelCheckAll: boolean,
165165
): boolean {
166166
runnerLabelsMatchers = runnerLabelsMatchers.map((runnerLabel) => {
167167
return runnerLabel.map((label) => label.toLowerCase());
168168
});
169-
const match = workflowLabelCheckAll
169+
const matchLabels = workflowLabelCheckAll
170170
? runnerLabelsMatchers.some((rl) => workflowJobLabels.every((wl) => rl.includes(wl.toLowerCase())))
171171
: runnerLabelsMatchers.some((rl) => workflowJobLabels.some((wl) => rl.includes(wl.toLowerCase())));
172+
const match = workflowJobLabels.length === 0 ? !matchLabels : matchLabels;
172173

173174
logger.debug(
174175
`Received workflow job event with labels: '${JSON.stringify(workflowJobLabels)}'. The event does ${

Diff for: modules/webhook/variables.tf

+1-1
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ variable "tags" {
3434
}
3535

3636
variable "runner_config" {
37-
description = "SQS queue to publish accepted build events based on the runner type."
37+
description = "SQS queue to publish accepted build events based on the runner type. When exact match is disabled the webhook accecpts the event if one of the workflow job labels is part of the matcher."
3838
type = map(object({
3939
arn = string
4040
id = string

0 commit comments

Comments
 (0)