Skip to content

fix(runners): Namespace Application tag #2182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Nov 30, 2022
Merged

fix(runners): Namespace Application tag #2182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ac26958
fix(tag): Namespace `Application` tag
mcaulifn Jun 22, 2022
2d805c8
Update tests
mcaulifn Jun 22, 2022
c10dc91
Merge branch 'develop' into mcaulifn/app-tag
mcaulifn Jun 23, 2022
95abc52
Reverting format
mcaulifn Jun 24, 2022
707012f
Support legacy `Application` tag key
mcaulifn Jul 5, 2022
1bfafc5
Merge branch 'develop' into mcaulifn/app-tag
mcaulifn Sep 6, 2022
321cd35
Merge branch 'develop' into mcaulifn/app-tag
mcaulifn Sep 15, 2022
af34741
Merge branch 'develop' into mcaulifn/app-tag
npalm Oct 14, 2022
66bcc58
Merge branch 'develop' into mcaulifn/app-tag
mcaulifn Nov 14, 2022
8baf486
Adding legacy StringEquals
mcaulifn Nov 18, 2022
1eb3612
Reverting test
mcaulifn Nov 18, 2022
069d87f
Merge branch 'develop' into mcaulifn/app-tag
mcaulifn Nov 18, 2022
5e7f145
Removing new lines
mcaulifn Nov 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 66 additions & 34 deletions modules/runners/lambdas/runners/src/aws/runners.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,41 +15,53 @@ const ORG_NAME = 'SomeAwesomeCoder';
const REPO_NAME = `${ORG_NAME}/some-amazing-library`;
const ENVIRONMENT = 'unit-test-environment';

describe('list instances', () => {
const mockDescribeInstances = { promise: jest.fn() };
beforeEach(() => {
jest.clearAllMocks();
mockEC2.describeInstances.mockImplementation(() => mockDescribeInstances);
const mockRunningInstances: AWS.EC2.DescribeInstancesResult = {
Reservations: [
const mockDescribeInstances = { promise: jest.fn() };
mockEC2.describeInstances.mockImplementation(() => mockDescribeInstances);
const mockRunningInstances: AWS.EC2.DescribeInstancesResult = {
Reservations: [
{
Instances: [
{
Instances: [
{
LaunchTime: new Date('2020-10-10T14:48:00.000+09:00'),
InstanceId: 'i-1234',
Tags: [
{ Key: 'Application', Value: 'github-action-runner' },
{ Key: 'Type', Value: 'Org' },
{ Key: 'Owner', Value: 'CoderToCat' },
],
},
{
LaunchTime: new Date('2020-10-11T14:48:00.000+09:00'),
InstanceId: 'i-5678',
Tags: [
{ Key: 'Owner', Value: REPO_NAME },
{ Key: 'Type', Value: 'Repo' },
{ Key: 'Application', Value: 'github-action-runner' },
],
},
LaunchTime: new Date('2020-10-10T14:48:00.000+09:00'),
InstanceId: 'i-1234',
Tags: [
{ Key: 'ghr:Application', Value: 'github-action-runner' },
{ Key: 'Type', Value: 'Org' },
{ Key: 'Owner', Value: 'CoderToCat' },
],
},
],
};
mockDescribeInstances.promise.mockReturnValue(mockRunningInstances);
},
],
};
const mockRunningInstancesLegacy: AWS.EC2.DescribeInstancesResult = {
Reservations: [
{
Instances: [
{
LaunchTime: new Date('2020-10-11T14:48:00.000+09:00'),
InstanceId: 'i-5678',
Tags: [
{ Key: 'Owner', Value: REPO_NAME },
{ Key: 'Type', Value: 'Repo' },
{ Key: 'Application', Value: 'github-action-runner' },
],
},
],
},
],
};

describe('list instances', () => {
beforeEach(() => {
jest.resetModules();
jest.clearAllMocks();
});

it('returns a list of instances', async () => {
mockDescribeInstances.promise
.mockReturnValueOnce(mockRunningInstances)
.mockReturnValueOnce(mockRunningInstancesLegacy);
const resp = await listEC2Runners();
expect(resp.length).toBe(2);
expect(resp).toContainEqual({
Expand All @@ -67,41 +79,61 @@ describe('list instances', () => {
});

it('calls EC2 describe instances', async () => {
mockDescribeInstances.promise
.mockReturnValueOnce(mockRunningInstances)
.mockReturnValueOnce(mockRunningInstancesLegacy);
await listEC2Runners();
expect(mockEC2.describeInstances).toBeCalled();
});

it('filters instances on repo name', async () => {
mockDescribeInstances.promise
.mockReturnValueOnce(mockRunningInstances)
.mockReturnValueOnce(mockRunningInstancesLegacy);
await listEC2Runners({ runnerType: 'Repo', runnerOwner: REPO_NAME, environment: undefined });
expect(mockEC2.describeInstances).toBeCalledWith({
Filters: [
{ Name: 'tag:Application', Values: ['github-action-runner'] },
{ Name: 'instance-state-name', Values: ['running', 'pending'] },
{ Name: 'tag:Type', Values: ['Repo'] },
{ Name: 'tag:Owner', Values: [REPO_NAME] },
{ Name: 'tag:ghr:Application', Values: ['github-action-runner'] },
],
});
expect(mockEC2.describeInstances).toBeCalledWith({
Filters: [
{ Name: 'instance-state-name', Values: ['running', 'pending'] },
{ Name: 'tag:Type', Values: ['Repo'] },
{ Name: 'tag:Owner', Values: [REPO_NAME] },
{ Name: 'tag:Application', Values: ['github-action-runner'] },
],
});
});

it('filters instances on org name', async () => {
mockDescribeInstances.promise
.mockReturnValueOnce(mockRunningInstances)
.mockReturnValueOnce(mockRunningInstancesLegacy);
await listEC2Runners({ runnerType: 'Org', runnerOwner: ORG_NAME, environment: undefined });
expect(mockEC2.describeInstances).toBeCalledWith({
Filters: [
{ Name: 'tag:Application', Values: ['github-action-runner'] },
{ Name: 'instance-state-name', Values: ['running', 'pending'] },
{ Name: 'tag:Type', Values: ['Org'] },
{ Name: 'tag:Owner', Values: [ORG_NAME] },
{ Name: 'tag:ghr:Application', Values: ['github-action-runner'] },
],
});
});

it('filters instances on environment', async () => {
mockDescribeInstances.promise
.mockReturnValueOnce(mockRunningInstances)
.mockReturnValueOnce(mockRunningInstancesLegacy);
await listEC2Runners({ environment: ENVIRONMENT });
expect(mockEC2.describeInstances).toBeCalledWith({
Filters: [
{ Name: 'tag:Application', Values: ['github-action-runner'] },
{ Name: 'instance-state-name', Values: ['running', 'pending'] },
{ Name: 'tag:ghr:environment', Values: [ENVIRONMENT] },
{ Name: 'tag:ghr:Application', Values: ['github-action-runner'] },
],
});
});
Expand All @@ -123,7 +155,7 @@ describe('list instances', () => {
},
],
};
mockDescribeInstances.promise.mockReturnValue(noInstances);
mockDescribeInstances.promise.mockReturnValueOnce(noInstances).mockReturnValueOnce(noInstances);
const resp = await listEC2Runners();
expect(resp.length).toBe(0);
});
Expand All @@ -142,7 +174,7 @@ describe('list instances', () => {
},
],
};
mockDescribeInstances.promise.mockReturnValue(noInstances);
mockDescribeInstances.promise.mockReturnValueOnce(noInstances).mockReturnValue({});
const resp = await listEC2Runners();
expect(resp.length).toBe(1);
});
Expand Down Expand Up @@ -459,7 +491,7 @@ function expectedCreateFleetRequest(expectedValues: ExpectedFleetRequestValues):
{
ResourceType: 'instance',
Tags: [
{ Key: 'Application', Value: 'github-action-runner' },
{ Key: 'ghr:Application', Value: 'github-action-runner' },
{ Key: 'Type', Value: expectedValues.type },
{ Key: 'Owner', Value: REPO_NAME },
],
Expand Down
43 changes: 33 additions & 10 deletions modules/runners/lambdas/runners/src/aws/runners.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,24 +45,47 @@ export interface RunnerInputParameters {
amiIdSsmParameterName?: string;
}

interface Ec2Filter {
Name: string;
Values: string[];
}

export async function listEC2Runners(filters: ListRunnerFilters | undefined = undefined): Promise<RunnerList[]> {
const ec2Statuses = filters?.statuses ? filters.statuses : ['running', 'pending'];
const ec2 = new EC2();
const ec2Filters = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it is really needed, but back in the days we have implemented here hardcoded that if not set a filter. A filter got enforced. I think this was done to avoid the scale down can terminate anything that is not owned.

Don't like this hidden magic and we should got cleaned it at some moment. I also updated the tested to see the impact when we remove the depecrated tag Application. In that case you will find that the test for instance we no tags 'Instances with no tags.' failes. Which is related to these default.s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which test did you update? I'll replicate and resolve.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test Instances with no tags. on L134 in on this branch. Sorry for the late reply

Copy link
Contributor Author

@mcaulifn mcaulifn Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Instances with no tags.' should be changed to expect 0 and now it is failing for me.

Despite mockReturnValueOnce being used throughout, and that instance definition not being used in the test in question, I am getting the two instances used at the top of the test. This appears to be an issue with the test rather than with the code. I'm still trying to track down how it is getting injected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this bug is being hit. I'll try to work up a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not been able to find a work-around for the testing bug. @npalm any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check the tests again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least I know what the problem is. When I remove the support vor the tag Application in (runner.ts). Merge the two mocks back in a single mock like before, returning 2 objects. The test "Instances with no tags." starts failing. Running the test in isolation is not causing a problem. So somehow the mocks are not resetting well.

Was not able to find a solution, but this eems the issue.

{ Name: 'tag:Application', Values: ['github-action-runner'] },
{ Name: 'instance-state-name', Values: ec2Statuses },
];
const ec2Filters = constructFilters(filters);
const runners: RunnerList[] = [];
for (const filter of ec2Filters) {
runners.push(...(await getRunners(filter)));
}
return runners;
}

function constructFilters(filters?: ListRunnerFilters): Ec2Filter[][] {
const ec2Statuses = filters?.statuses ? filters.statuses : ['running', 'pending'];
const ec2Filters: Ec2Filter[][] = [];
const ec2FiltersBase = [{ Name: 'instance-state-name', Values: ec2Statuses }];
if (filters) {
if (filters.environment !== undefined) {
ec2Filters.push({ Name: 'tag:ghr:environment', Values: [filters.environment] });
ec2FiltersBase.push({ Name: 'tag:ghr:environment', Values: [filters.environment] });
}
if (filters.runnerType && filters.runnerOwner) {
ec2Filters.push({ Name: `tag:Type`, Values: [filters.runnerType] });
ec2Filters.push({ Name: `tag:Owner`, Values: [filters.runnerOwner] });
ec2FiltersBase.push({ Name: `tag:Type`, Values: [filters.runnerType] });
ec2FiltersBase.push({ Name: `tag:Owner`, Values: [filters.runnerOwner] });
}
}

// ***Deprecation Notice***
// Support for legacy `Application` tag keys
// will be removed in next major release.
for (const key of ['tag:ghr:Application', 'tag:Application']) {
const filter = [...ec2FiltersBase];
filter.push({ Name: key, Values: ['github-action-runner'] });
ec2Filters.push(filter);
}
return ec2Filters;
}

async function getRunners(ec2Filters: Ec2Filter[]): Promise<RunnerList[]> {
const ec2 = new EC2();
const runners: RunnerList[] = [];
let nextToken;
let hasNext = true;
Expand Down Expand Up @@ -191,7 +214,7 @@ export async function createRunner(runnerParameters: RunnerInputParameters): Pro
{
ResourceType: 'instance',
Tags: [
{ Key: 'Application', Value: 'github-action-runner' },
{ Key: 'ghr:Application', Value: 'github-action-runner' },
{ Key: 'Type', Value: runnerParameters.runnerType },
{ Key: 'Owner', Value: runnerParameters.runnerOwner },
],
Expand Down
102 changes: 58 additions & 44 deletions modules/runners/policies/lambda-scale-down.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,62 @@
{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": [
"ec2:DescribeInstances",
"ec2:DescribeTags"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"ec2:TerminateInstances"
],
"Resource": [
"*"
],
"Condition": {
"StringEquals": {
"ec2:ResourceTag/Application": "github-action-runner"
}
}
},
{
"Effect": "Allow",
"Action": [
"ssm:GetParameter"
],
"Resource": [
"${github_app_key_base64_arn}",
"${github_app_id_arn}"
]
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": [
"ec2:DescribeInstances",
"ec2:DescribeTags"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"ec2:TerminateInstances"
],
"Resource": [
"*"
],
"Condition": {
"StringEquals": {
"ec2:ResourceTag/ghr:Application": "github-action-runner"
}
}
},
{
"Effect": "Allow",
"Action": [
"ec2:TerminateInstances"
],
"Resource": [
"*"
],
"Condition": {
"StringEquals": {
"ec2:ResourceTag/Application": "github-action-runner"
}
}
},
{
"Effect": "Allow",
"Action": [
"ssm:GetParameter"
],
"Resource": [
"${github_app_key_base64_arn}",
"${github_app_id_arn}"
]
%{ if kms_key_arn != "" ~}
},
{
"Effect": "Allow",
"Action": [
"kms:Decrypt"
],
"Resource": "${kms_key_arn}"
},
{
"Effect": "Allow",
"Action": [
"kms:Decrypt"
],
"Resource": "${kms_key_arn}"
%{ endif ~}
}
]
}
]
}