feat(runners): Namespace Application tag (#2182)

mcaulifn · npalm · web-flow · commit a1a47a4a18fe · 2022-11-30T22:25:09.000+01:00
Tag `Application` on runner instances will be replaced by `ghr:Application`

Co-authored-by: Niek Palm &lt;npalm@users.noreply.github.com&gt;
diff --git a/modules/runners/lambdas/runners/src/aws/runners.test.ts b/modules/runners/lambdas/runners/src/aws/runners.test.ts
@@ -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({
@@ -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'] },
       ],
     });
   });
@@ -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);
   });
@@ -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);
   });
@@ -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 },
         ],
diff --git a/modules/runners/lambdas/runners/src/aws/runners.ts b/modules/runners/lambdas/runners/src/aws/runners.ts
@@ -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 = [
-    { 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;
@@ -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 },
             ],
diff --git a/modules/runners/policies/lambda-scale-down.json b/modules/runners/policies/lambda-scale-down.json
@@ -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 ~}
-        }
-    ]
+    }
+  ]
 }
