Skip to content

Commit 6a5922a

Browse files
edersonbrilhanteedersonbrilhante
committed
refactor(compute-providers): centralize dynamic label selection
1 parent d5d5841 commit 6a5922a

10 files changed

Lines changed: 195 additions & 135 deletions

File tree

‎lambdas/libs/compute-providers/aws/ec2/src/webhook/dynamic-labels.test.ts‎

Lines changed: 20 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,65 +1,29 @@
11
import { describe, expect, it } from 'vitest';
22

33
import type { RunnerMatcherConfig } from '../../../../contracts';
4-
import { selectEc2DynamicLabelQueue } from './dynamic-labels';
4+
import { ec2DynamicLabelProvider } from './dynamic-labels';
55

6-
describe('selectEc2DynamicLabelQueue', () => {
7-
it('rejects dynamic labels when the queue disables them', () => {
8-
const queue = runnerQueue('dynamic-labels-disabled');
9-
queue.matcherConfig.enableDynamicLabels = false;
10-
11-
expect(
12-
selectEc2DynamicLabelQueue([queue], ['self-hosted', 'linux'], ['ghr-ec2-instance-type:t3.large']),
13-
).toBeUndefined();
14-
});
15-
16-
it('accepts dynamic labels when the queue has no policy', () => {
6+
describe('ec2DynamicLabelProvider', () => {
7+
it('returns no violations when the queue has no policy', () => {
178
const queue = runnerQueue('no-policy');
189

19-
expect(selectEc2DynamicLabelQueue([queue], ['self-hosted', 'linux'], ['ghr-ec2-instance-type:t3.large'])).toEqual({
20-
queue,
21-
labels: ['self-hosted', 'linux', 'ghr-ec2-instance-type:t3.large'],
22-
});
10+
expect(getViolations(queue)).toEqual([]);
2311
});
2412

25-
it('skips a policy-rejected queue and returns the next compliant queue', () => {
13+
it('returns violations for labels rejected by the policy', () => {
2614
const strictQueue = runnerQueue('strict');
2715
strictQueue.matcherConfig.awsDynamicLabelsPolicy = {
2816
restricted_keys: {
2917
'instance-type': { allowed: ['m5.*'] },
3018
},
3119
};
32-
const permissiveQueue = runnerQueue('permissive');
3320

34-
expect(
35-
selectEc2DynamicLabelQueue(
36-
[strictQueue, permissiveQueue],
37-
['self-hosted', 'linux'],
38-
['ghr-ec2-instance-type:t3.large'],
39-
),
40-
).toEqual({
41-
queue: permissiveQueue,
42-
labels: ['self-hosted', 'linux', 'ghr-ec2-instance-type:t3.large'],
43-
});
44-
});
45-
46-
it('returns undefined when no queue accepts the dynamic labels', () => {
47-
const strictQueue = runnerQueue('strict');
48-
strictQueue.matcherConfig.awsDynamicLabelsPolicy = {
49-
restricted_keys: {
50-
'instance-type': { allowed: ['m5.*'] },
21+
expect(getViolations(strictQueue)).toEqual([
22+
{
23+
label: 'ghr-ec2-instance-type:t3.large',
24+
reason: "value 't3.large' not in allowed list",
5125
},
52-
};
53-
const disabledQueue = runnerQueue('disabled');
54-
disabledQueue.matcherConfig.enableDynamicLabels = false;
55-
56-
expect(
57-
selectEc2DynamicLabelQueue(
58-
[strictQueue, disabledQueue],
59-
['self-hosted', 'linux'],
60-
['ghr-ec2-instance-type:t3.large'],
61-
),
62-
).toBeUndefined();
26+
]);
6327
});
6428

6529
it('enforces a legacy EC2 dynamic labels policy when the new key is absent', () => {
@@ -68,9 +32,7 @@ describe('selectEc2DynamicLabelQueue', () => {
6832
blocked_keys: ['instance-type'],
6933
};
7034

71-
expect(
72-
selectEc2DynamicLabelQueue([queue], ['self-hosted', 'linux'], ['ghr-ec2-instance-type:t3.large']),
73-
).toBeUndefined();
35+
expect(getViolations(queue)).toHaveLength(1);
7436
});
7537

7638
it('falls back to the legacy EC2 dynamic labels policy when the new policy is null', () => {
@@ -80,9 +42,7 @@ describe('selectEc2DynamicLabelQueue', () => {
8042
};
8143
queue.matcherConfig.awsDynamicLabelsPolicy = null;
8244

83-
expect(
84-
selectEc2DynamicLabelQueue([queue], ['self-hosted', 'linux'], ['ghr-ec2-instance-type:t3.large']),
85-
).toBeUndefined();
45+
expect(getViolations(queue)).toHaveLength(1);
8646
});
8747

8848
it('prefers a configured AWS dynamic labels policy over the legacy policy', () => {
@@ -94,13 +54,17 @@ describe('selectEc2DynamicLabelQueue', () => {
9454
blocked_keys: [],
9555
};
9656

97-
expect(selectEc2DynamicLabelQueue([queue], ['self-hosted', 'linux'], ['ghr-ec2-instance-type:t3.large'])).toEqual({
98-
queue,
99-
labels: ['self-hosted', 'linux', 'ghr-ec2-instance-type:t3.large'],
100-
});
57+
expect(getViolations(queue)).toEqual([]);
10158
});
10259
});
10360

61+
function getViolations(queue: RunnerMatcherConfig) {
62+
return ec2DynamicLabelProvider.getViolations({
63+
queue,
64+
labels: ['ghr-ec2-instance-type:t3.large'],
65+
});
66+
}
67+
10468
function runnerQueue(id: string): RunnerMatcherConfig {
10569
return {
10670
id,

‎lambdas/libs/compute-providers/aws/ec2/src/webhook/dynamic-labels.ts‎

Lines changed: 2 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
import { createChildLogger } from '@aws-github-runner/aws-powertools-util';
22

3-
import type { DynamicLabelDispatchTarget, DynamicLabelProvider, RunnerMatcherConfig } from '../../../../contracts';
3+
import type { DynamicLabelProvider, RunnerMatcherConfig } from '../../../../contracts';
44
import { violationsAgainstPolicy } from './dynamic-labels-policy';
55

66
const logger = createChildLogger('handler');
77

8-
export type Ec2DynamicLabelDispatchTarget = DynamicLabelDispatchTarget;
9-
108
function resolveEc2DynamicLabelsPolicy(queue: RunnerMatcherConfig) {
119
const hasLegacyEc2DynamicLabelsPolicy = Object.prototype.hasOwnProperty.call(
1210
queue.matcherConfig,
@@ -23,36 +21,6 @@ function resolveEc2DynamicLabelsPolicy(queue: RunnerMatcherConfig) {
2321
return queue.matcherConfig.awsDynamicLabelsPolicy;
2422
}
2523

26-
export function selectEc2DynamicLabelQueue(
27-
matches: RunnerMatcherConfig[],
28-
nonGhrLabels: string[],
29-
sanitizedGhrLabels: string[],
30-
): Ec2DynamicLabelDispatchTarget | undefined {
31-
for (const queue of matches) {
32-
if (!queue.matcherConfig.enableDynamicLabels) {
33-
logger.warn(`Queue ${queue.id} matches non-dynamic labels but does not allow dynamic labels; trying next match`);
34-
continue;
35-
}
36-
37-
const violations = violationsAgainstPolicy(sanitizedGhrLabels, resolveEc2DynamicLabelsPolicy(queue));
38-
if (violations.length === 0) {
39-
return {
40-
queue,
41-
labels: [...nonGhrLabels, ...sanitizedGhrLabels],
42-
};
43-
}
44-
45-
for (const violation of violations) {
46-
logger.warn(
47-
`Queue ${queue.id}: dynamic label '${violation.label}' does not match policy (${violation.reason}); trying next match`,
48-
);
49-
}
50-
}
51-
52-
return undefined;
53-
}
54-
5524
export const ec2DynamicLabelProvider: DynamicLabelProvider = {
56-
selectQueue: ({ queue, nonGhrLabels, sanitizedGhrLabels }) =>
57-
selectEc2DynamicLabelQueue([queue], nonGhrLabels, sanitizedGhrLabels),
25+
getViolations: ({ queue, labels }) => violationsAgainstPolicy(labels, resolveEc2DynamicLabelsPolicy(queue)),
5826
};

‎lambdas/libs/compute-providers/contracts.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,13 @@ export interface DynamicLabelDispatchTarget {
4343
labels: string[];
4444
}
4545

46+
export interface DynamicLabelViolation {
47+
label: string;
48+
reason: string;
49+
}
50+
4651
export interface DynamicLabelProvider {
47-
selectQueue(input: {
48-
queue: RunnerMatcherConfig;
49-
nonGhrLabels: string[];
50-
sanitizedGhrLabels: string[];
51-
}): DynamicLabelDispatchTarget | undefined;
52+
getViolations(input: { queue: RunnerMatcherConfig; labels: string[] }): DynamicLabelViolation[];
5253
}
5354

5455
export interface ControlPlaneProviderCapabilities {

‎lambdas/libs/compute-providers/dynamic-labels.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { expect, it } from 'vitest';
2+
3+
import { dynamicLabelsForOtherProvider } from './dynamic-labels';
4+
import { computeProviderTypes } from './provider-types';
5+
6+
it.each(computeProviderTypes)('returns labels belonging to providers other than %s', (provider) => {
7+
const providerLabels = computeProviderTypes.map((type) => `ghr-${type}-size:large`);
8+
9+
expect(dynamicLabelsForOtherProvider(providerLabels, provider)).toEqual(
10+
providerLabels.filter((label) => !label.startsWith(`ghr-${provider}-`)),
11+
);
12+
});

‎lambdas/libs/compute-providers/dynamic-labels.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import { computeProviderTypes } from './provider-types';
2+
import type { ComputeProviderType } from './provider-types';
3+
4+
export function dynamicLabelsForOtherProvider(labels: string[], provider: ComputeProviderType): string[] {
5+
return labels.filter((label) =>
6+
computeProviderTypes.some((candidate) => candidate !== provider && label.startsWith(`ghr-${candidate}-`)),
7+
);
8+
}

‎lambdas/libs/compute-providers/registry.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,6 @@ it('exposes every configured provider through both capability registries', () =>
3333
unmarkOrphan: expect.any(Function),
3434
terminate: expect.any(Function),
3535
});
36-
expect(webhookProviderRegistry.capability(type, 'dynamicLabels').selectQueue).toEqual(expect.any(Function));
36+
expect(webhookProviderRegistry.capability(type, 'dynamicLabels').getViolations).toEqual(expect.any(Function));
3737
}
3838
});

‎lambdas/libs/compute-providers/templates/provider/provider.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,5 +29,5 @@ it('exposes every compute provider capability from its compute-provider entry po
2929
terminate: expect.any(Function),
3030
});
3131
expect(webhookPlugin.type).toBe(webhookProvider.type);
32-
expect(webhookPlugin.capabilities.dynamicLabels.selectQueue).toEqual(expect.any(Function));
32+
expect(webhookPlugin.capabilities.dynamicLabels.getViolations).toEqual(expect.any(Function));
3333
});

‎lambdas/libs/compute-providers/templates/provider/webhook.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@ import type { ComputeProviderPlugin } from '../../core';
33
import type { DynamicLabelProvider, WebhookProviderCapabilities, WebhookProviderModule } from '../../contracts';
44

55
export const templateDynamicLabelProvider: DynamicLabelProvider = {
6-
selectQueue: (input) => {
6+
getViolations: (input) => {
77
void input;
8-
// Return a dispatch target when this provider accepts the requested dynamic labels.
9-
return undefined;
8+
// Return violations for dynamic labels this provider does not accept.
9+
return [];
1010
},
1111
};
1212

‎lambdas/libs/compute-providers/webhook.test.ts‎

Lines changed: 84 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,106 @@
1-
import { describe, expect, it } from 'vitest';
1+
import { describe, expect, it, vi } from 'vitest';
22

3-
import type { RunnerMatcherConfig } from './contracts';
3+
import type { DynamicLabelProvider, DynamicLabelViolation, RunnerMatcherConfig } from './contracts';
44
import type { ComputeProviderType } from './provider-types';
5-
import { selectDynamicLabelQueue } from './webhook';
5+
import { createDynamicLabelQueueSelector } from './webhook';
66

7-
describe('selectDynamicLabelQueue', () => {
8-
it('defaults queues without a provider to EC2 dynamic label handling', () => {
9-
const queue = runnerQueue('default-ec2');
7+
describe('createDynamicLabelQueueSelector', () => {
8+
it('returns the first queue accepted by its provider', () => {
9+
const queue = runnerQueue('accepted');
10+
const { selectQueue } = selector();
1011

11-
expect(selectDynamicLabelQueue([queue], ['self-hosted', 'linux'], ['ghr-ec2-instance-type:t3.large'])).toEqual({
12+
expect(selectQueue([queue], ['self-hosted', 'linux'], ['ghr-test-size:large'])).toEqual({
1213
queue,
13-
labels: ['self-hosted', 'linux', 'ghr-ec2-instance-type:t3.large'],
14+
labels: ['self-hosted', 'linux', 'ghr-test-size:large'],
1415
});
1516
});
1617

17-
it('normalizes compute provider casing and surrounding whitespace', () => {
18-
const queue = runnerQueue('normalized-ec2');
19-
(queue as unknown as { computeProvider: string }).computeProvider = ' EC2 ';
18+
it('skips queues that disable dynamic labels', () => {
19+
const disabledQueue = runnerQueue('disabled');
20+
disabledQueue.matcherConfig.enableDynamicLabels = false;
21+
const enabledQueue = runnerQueue('enabled');
22+
const { getViolations, selectQueue } = selector();
2023

21-
expect(selectDynamicLabelQueue([queue], ['self-hosted', 'linux'], ['ghr-ec2-instance-type:t3.large'])).toEqual({
22-
queue,
23-
labels: ['self-hosted', 'linux', 'ghr-ec2-instance-type:t3.large'],
24+
expect(selectQueue([disabledQueue, enabledQueue], ['self-hosted'], ['ghr-test-size:large'])).toEqual({
25+
queue: enabledQueue,
26+
labels: ['self-hosted', 'ghr-test-size:large'],
27+
});
28+
expect(getViolations).toHaveBeenCalledOnce();
29+
expect(getViolations).toHaveBeenCalledWith({ queue: enabledQueue, labels: ['ghr-test-size:large'] });
30+
});
31+
32+
it('skips queues whose provider reports violations', () => {
33+
const rejectedQueue = runnerQueue('rejected');
34+
const acceptedQueue = runnerQueue('accepted');
35+
const { selectQueue } = selector({
36+
violationsByQueue: {
37+
rejected: [{ label: 'ghr-test-size:large', reason: 'size is unavailable' }],
38+
},
39+
});
40+
41+
expect(selectQueue([rejectedQueue, acceptedQueue], ['self-hosted'], ['ghr-test-size:large'])).toEqual({
42+
queue: acceptedQueue,
43+
labels: ['self-hosted', 'ghr-test-size:large'],
44+
});
45+
});
46+
47+
it('returns undefined when every provider reports violations', () => {
48+
const queue = runnerQueue('rejected');
49+
const { selectQueue } = selector({
50+
violationsByQueue: {
51+
rejected: [{ label: 'ghr-test-size:large', reason: 'size is unavailable' }],
52+
},
2453
});
54+
55+
expect(selectQueue([queue], ['self-hosted'], ['ghr-test-size:large'])).toBeUndefined();
2556
});
2657

27-
it.each([['unsupported'], [42]])('throws for unsupported compute provider %j', (computeProvider) => {
28-
const queue = runnerQueue('unsupported-provider');
29-
(queue as unknown as { computeProvider: unknown }).computeProvider = computeProvider;
58+
/* TODO: Re-enable this scenario when the MicroVM provider is added.
59+
it('skips EC2 and selects the MicroVM queue for MicroVM override labels', () => {
60+
const ec2Queue = runnerQueue('ec2');
61+
const microvmQueue = runnerQueue('microvm');
62+
const imageVersionLabel = 'ghr-microvm-image-version:3.0';
63+
const { getViolations, selectQueue } = selector({
64+
providerByQueue: { ec2: 'ec2', microvm: 'microvm' },
65+
labelsForOtherProvider: (labels, provider) =>
66+
provider === 'ec2' ? labels.filter((label) => label.startsWith('ghr-microvm-')) : [],
67+
});
3068
31-
expect(() =>
32-
selectDynamicLabelQueue([queue], ['self-hosted', 'linux'], ['ghr-ec2-instance-type:t3.large']),
33-
).toThrow(`Unsupported compute provider type '${String(computeProvider)}'`);
69+
expect(selectQueue([ec2Queue, microvmQueue], ['self-hosted', 'linux'], [imageVersionLabel])).toEqual({
70+
queue: microvmQueue,
71+
labels: ['self-hosted', 'linux', imageVersionLabel],
72+
});
73+
expect(getViolations).toHaveBeenCalledOnce();
74+
expect(getViolations).toHaveBeenCalledWith({ queue: microvmQueue, labels: [imageVersionLabel] });
3475
});
76+
*/
3577
});
3678

37-
function runnerQueue(id: string, computeProvider?: ComputeProviderType): RunnerMatcherConfig {
79+
function selector(options?: {
80+
providerByQueue?: Record<string, ComputeProviderType>;
81+
violationsByQueue?: Record<string, DynamicLabelViolation[]>;
82+
labelsForOtherProvider?: (labels: string[], provider: ComputeProviderType) => string[];
83+
}) {
84+
const getViolations = vi.fn<DynamicLabelProvider['getViolations']>(({ queue }) => {
85+
return options?.violationsByQueue?.[queue.id] ?? [];
86+
});
87+
88+
return {
89+
getViolations,
90+
selectQueue: createDynamicLabelQueueSelector<ComputeProviderType>({
91+
resolveProvider: (queue) => ({
92+
type: options?.providerByQueue?.[queue.id] ?? 'ec2',
93+
dynamicLabels: { getViolations },
94+
}),
95+
dynamicLabelsForOtherProvider: options?.labelsForOtherProvider ?? (() => []),
96+
}),
97+
};
98+
}
99+
100+
function runnerQueue(id: string): RunnerMatcherConfig {
38101
return {
39102
id,
40103
arn: `arn:${id}`,
41-
computeProvider,
42104
matcherConfig: {
43105
labelMatchers: [['self-hosted', 'linux']],
44106
exactMatch: true,

0 commit comments

Comments
 (0)