Skip to content
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

Jump to bottom

[BE] Upgrade runner lambdas from deprecated v2 client to v3 #5920

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[BE] Upgrade runner lambdas from deprecated v2 client to v3 #5920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion terraform-aws-github-runner/modules/runners/lambdas/runners/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@
"ts-node-dev": "^1.1.6"
},
"dependencies": {
"@aws-sdk/client-cloudwatch": "^3.692.0",
"@aws-sdk/client-ec2": "^3.692.0",
"@aws-sdk/client-kms": "^3.692.0",
"@aws-sdk/client-secrets-manager": "^3.692.0",
"@aws-sdk/client-sqs": "^3.692.0",
"@aws-sdk/client-ssm": "^3.692.0",
"@octokit/auth-app": "^3.0.0",
"@octokit/request-error": "^2.1.0",
"@octokit/rest": "^18.3.5",
Expand All @@ -41,7 +47,6 @@
"@types/node": "^14.14.34",
"@types/uuid": "^9.0.1",
"async-mutex": "^0.4.0",
"aws-sdk": "^2.863.0",
"cron-parser": "^3.3.0",
"generic-pool": "^3.9.0",
"lru-cache": "^6.0.0",
Expand Down
8 changes: 5 additions & 3 deletions terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/gh-auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Config } from './config';
import { Metrics } from './metrics';
import { Octokit } from '@octokit/rest';
import { OctokitOptions } from '@octokit/core/dist-types/types';
import { SecretsManager } from 'aws-sdk';
import { SecretsManager } from '@aws-sdk/client-secrets-manager';
import {
AppAuthentication,
InstallationAccessTokenAuthentication,
Expand Down Expand Up @@ -32,14 +32,16 @@ async function getCredentialsFromSecretsManager(
): Promise<GithubCredentials> {
return await locallyCached('secretCache', secretsManagerSecretsId, 10 * 60, async () => {
try {
const secretsManager = new SecretsManager();
const secretsManager = new SecretsManager({
region: Config.Instance.awsRegion,
});

const data = await expBackOff(() => {
return metrics.trackRequest(
metrics.smGetSecretValueAWSCallSuccess,
metrics.smGetSecretValueAWSCallFailure,
() => {
return secretsManager.getSecretValue({ SecretId: secretsManagerSecretsId }).promise();
return secretsManager.getSecretValue({ SecretId: secretsManagerSecretsId });
Copy link
Contributor

Choose a reason for hiding this comment

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

seems promise is still supported in the latest version.

https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Request.html#promise-property

Is there a reason you migrated from a async call to a sync call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The auto converter removed those. As per the migration guide, it seems like it automatically returns a promise without it needing to be explicitly referenced:
https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/migrating.html

},
);
});
Expand Down
25 changes: 12 additions & 13 deletions terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/kms/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import AWS from 'aws-sdk';
import { KMS } from '@aws-sdk/client-kms';
import { Config } from '../config';
import { expBackOff } from '../utils';
import { KMS } from 'aws-sdk';
import { Metrics } from '../metrics';

let kms: KMS | undefined = undefined;
Expand All @@ -14,27 +14,26 @@ export async function decrypt(
): Promise<string | undefined> {
/* istanbul ignore next */
if (!kms) {
AWS.config.update({
kms = new KMS({
region: Config.Instance.awsRegion,
});

kms = new KMS();
}

// this is so the linter understands that KMS is not undefined at this point :(
const kmsD = kms;

const decripted = await expBackOff(() => {
return metrics.trackRequest(metrics.kmsDecryptAWSCallSuccess, metrics.kmsDecryptAWSCallFailure, () => {
return kmsD
.decrypt({
CiphertextBlob: Buffer.from(encrypted, 'base64'),
KeyId: key,
EncryptionContext: {
['Environment']: environmentName,
},
})
.promise();
return (
kmsD
.decrypt({
CiphertextBlob: Buffer.from(encrypted, 'base64'),
KeyId: key,
EncryptionContext: {
['Environment']: environmentName,
},
})
Copy link
Contributor

Choose a reason for hiding this comment

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

same here about the sync calling.

);
});
});

Expand Down
10 changes: 6 additions & 4 deletions terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { CloudWatch } from 'aws-sdk';
import { CloudWatch, StandardUnit } from '@aws-sdk/client-cloudwatch';
import { Config } from './config';
import { expBackOff, Repo, RunnerInfo, getRepo } from './utils';

Expand All @@ -17,7 +17,7 @@ interface CloudWatchMetric {
MetricName: string;
Dimensions?: Array<CloudWatchMetricDim>;
Timestamp: Date;
Unit: string;
Unit?: StandardUnit | undefined;
Values: Array<number>;
}

Expand Down Expand Up @@ -111,7 +111,9 @@ export class Metrics {
}

protected constructor(lambdaName: string) {
this.cloudwatch = new CloudWatch({ region: Config.Instance.awsRegion });
this.cloudwatch = new CloudWatch({
region: Config.Instance.awsRegion,
});
this.lambdaName = lambdaName;
this.metrics = new Map();
this.metricsDimensions = new Map();
Expand Down Expand Up @@ -220,7 +222,7 @@ export class Metrics {
`NS: ${metricsReq.Namespace}] (${i} of ${awsMetrics.length})`,
);
await expBackOff(async () => {
return await this.cloudwatch.putMetricData(metricsReq).promise();
return await this.cloudwatch.putMetricData(metricsReq);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

please review all removals of the .promise() it might be a big problem

console.info(`Success sending metrics with cloudwatch.putMetricData (${i} of ${awsMetrics.length})`);
} catch (e) {
Expand Down
104 changes: 59 additions & 45 deletions terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
import { EC2, SSM } from 'aws-sdk';
import {
DescribeInstancesCommand,
DescribeImagesCommandOutput,
DescribeInstancesCommandOutput,
EC2,
Image,
Instance,
_InstanceType,
RunInstancesCommandInput,
} from '@aws-sdk/client-ec2';

import { DescribeParametersCommandInput, ParameterMetadata, SSM } from '@aws-sdk/client-ssm';
import { PromiseResult } from 'aws-sdk/lib/request';
import { RunnerInfo, expBackOff, shuffleArrayInPlace } from './utils';

Expand Down Expand Up @@ -41,7 +52,7 @@ export interface RunnerTypeOptional {

export interface RunnerType extends RunnerTypeOptional {
disk_size: number;
instance_type: string;
instance_type: _InstanceType;
is_ephemeral: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

If it starts with a underscore _ it is not a public interface, and we should not be using it. Can't we cast this to a string or something else?

os: string;
runnerTypeName: string;
Expand All @@ -51,11 +62,6 @@ export interface RunnerTypeScaleConfig extends RunnerType {
variants?: Map<string, RunnerTypeOptional>;
}

export interface DescribeInstancesResultRegion {
awsRegion: string;
describeInstanceResult: PromiseResult<EC2.Types.DescribeInstancesResult, AWS.AWSError>;
}

const SHOULD_NOT_TRY_LIST_SSM = 'SHOULD_NOT_TRY_LIST_SSM';

// Keep the cache as long as half of minimum time, this should reduce calls to AWS API
Expand All @@ -66,7 +72,9 @@ export function resetRunnersCaches() {
}

export async function findAmiID(metrics: Metrics, region: string, filter: string, owners = 'amazon'): Promise<string> {
const ec2 = new EC2({ region: region });
const ec2 = new EC2({
region: region,
});
const filters = [
{ Name: 'name', Values: [filter] },
{ Name: 'state', Values: ['available'] },
Expand All @@ -80,8 +88,7 @@ export async function findAmiID(metrics: Metrics, region: string, filter: string
() => {
return ec2
.describeImages({ Owners: [owners], Filters: filters })
.promise()
.then((data: EC2.DescribeImagesResult) => {
.then((data: DescribeImagesCommandOutput) => {
/* istanbul ignore next */
if (data.Images?.length === 0) {
console.error(`No availabe images found for filter '${filter}'`);
Expand All @@ -97,9 +104,9 @@ export async function findAmiID(metrics: Metrics, region: string, filter: string
}

// Shamelessly stolen from https://ajahne.github.io/blog/javascript/aws/2019/05/15/getting-an-ami-id-nodejs.html
function sortByCreationDate(data: EC2.DescribeImagesResult): void {
const images = data.Images as EC2.ImageList;
images.sort(function (a: EC2.Image, b: EC2.Image) {
function sortByCreationDate(data: DescribeImagesCommandOutput): void {
const images = data.Images as Array<Image>;
images.sort(function (a: Image, b: Image) {
const dateA: string = a['CreationDate'] as string;
const dateB: string = b['CreationDate'] as string;
if (dateA < dateB) {
Expand Down Expand Up @@ -158,18 +165,16 @@ export async function listRunners(
awsRegion,
metrics.ec2DescribeInstancesAWSCallSuccess,
metrics.ec2DescribeInstancesAWSCallFailure,
() => {
return new EC2({ region: awsRegion })
.describeInstances({ Filters: ec2Filters })
.promise()
.then((describeInstanceResult): DescribeInstancesResultRegion => {
console.debug(
`[listRunners]: Result for EC2({ region: ${awsRegion} })` +
`.describeInstances({ Filters: ${ec2Filters} }) = ` +
`${describeInstanceResult?.Reservations?.length ?? 'UNDEF'}`,
);
return { describeInstanceResult, awsRegion };
});
async () => {
const ec2Client = new EC2({ region: awsRegion });
const command = new DescribeInstancesCommand({ Filters: ec2Filters });
const describeInstanceResult = await ec2Client.send(command);
console.debug(
`[listRunners]: Result for EC2({ region: ${awsRegion} })` +
Copy link
Contributor

Choose a reason for hiding this comment

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

.promise().then...

`.describeInstances({ Filters: ${ec2Filters} }) = ` +
`${describeInstanceResult?.Reservations?.length ?? 'UNDEF'}`,
);
return { describeInstanceResult, awsRegion };
},
);
});
Expand Down Expand Up @@ -213,15 +218,17 @@ export function getParameterNameForRunner(environment: string, instanceId: strin
export async function listSSMParameters(
metrics: Metrics,
awsRegion: string,
): Promise<Map<string, SSM.ParameterMetadata>> {
let parametersSet: Map<string, SSM.ParameterMetadata> = ssmParametersCache.get(awsRegion) as Map<
): Promise<Map<string, ParameterMetadata>> {
let parametersSet: Map<string, ParameterMetadata> = ssmParametersCache.get(awsRegion) as Map<
string,
SSM.ParameterMetadata
ParameterMetadata
>;

if (parametersSet === undefined) {
parametersSet = new Map<string, SSM.ParameterMetadata>();
const ssm = new SSM({ region: awsRegion });
parametersSet = new Map<string, ParameterMetadata>();
const ssm = new SSM({
region: awsRegion,
});
let nextToken: string | undefined = undefined;

do {
Expand All @@ -232,10 +239,10 @@ export async function listSSMParameters(
metrics.ssmDescribeParametersAWSCallFailure,
() => {
if (nextToken) {
const reqParam: SSM.DescribeParametersRequest = { NextToken: nextToken };
return ssm.describeParameters(reqParam).promise();
const reqParam: DescribeParametersCommandInput = { NextToken: nextToken };
return ssm.describeParameters(reqParam);
}
return ssm.describeParameters().promise();
return ssm.describeParameters();
},
);
});
Expand All @@ -257,14 +264,16 @@ export async function listSSMParameters(

export async function doDeleteSSMParameter(paramName: string, metrics: Metrics, awsRegion: string): Promise<boolean> {
try {
const ssm = new SSM({ region: awsRegion });
const ssm = new SSM({
region: awsRegion,
});
await expBackOff(() => {
return metrics.trackRequestRegion(
awsRegion,
metrics.ssmdeleteParameterAWSCallSuccess,
metrics.ssmdeleteParameterAWSCallFailure,
() => {
return ssm.deleteParameter({ Name: paramName }).promise();
return ssm.deleteParameter({ Name: paramName });
},
);
});
Expand All @@ -282,15 +291,17 @@ export async function doDeleteSSMParameter(paramName: string, metrics: Metrics,

export async function terminateRunner(runner: RunnerInfo, metrics: Metrics): Promise<void> {
try {
const ec2 = new EC2({ region: runner.awsRegion });
const ec2 = new EC2({
region: runner.awsRegion,
});

await expBackOff(() => {
return metrics.trackRequestRegion(
runner.awsRegion,
metrics.ec2TerminateInstancesAWSCallSuccess,
metrics.ec2TerminateInstancesAWSCallFailure,
() => {
return ec2.terminateInstances({ InstanceIds: [runner.instanceId] }).promise();
return ec2.terminateInstances({ InstanceIds: [runner.instanceId] });
},
);
});
Expand Down Expand Up @@ -327,7 +338,7 @@ export async function terminateRunner(runner: RunnerInfo, metrics: Metrics): Pro
}

async function addSSMParameterRunnerConfig(
instances: EC2.InstanceList,
instances: Array<Instance>,
runnerParameters: RunnerInputParameters,
customAmiExperiment: boolean,
ssm: SSM,
Expand All @@ -347,7 +358,7 @@ async function addSSMParameterRunnerConfig(

const createdSSMParams = await Promise.all(
/* istanbul ignore next */
instances.map(async (i: EC2.Instance) => {
instances.map(async (i: Instance) => {
const parameterName = getParameterNameForRunner(runnerParameters.environment, i.InstanceId as string);
return await expBackOff(() => {
return metrics.trackRequestRegion(
Expand All @@ -360,8 +371,7 @@ async function addSSMParameterRunnerConfig(
Name: parameterName,
Value: runnerConfig,
Type: 'SecureString',
})
.promise();
});
return parameterName;
},
);
Expand Down Expand Up @@ -486,8 +496,12 @@ export async function createRunner(runnerParameters: RunnerInputParameters, metr
for (const [awsRegionIdx, awsRegion] of shuffledAwsRegionInstances.entries()) {
const runnerSubnetSequence = await getCreateRunnerSubnetSequence(runnerParameters, awsRegion, metrics);

const ec2 = new EC2({ region: awsRegion });
const ssm = new SSM({ region: awsRegion });
const ec2 = new EC2({
region: awsRegion,
});
const ssm = new SSM({
region: awsRegion,
});

const validSubnets = new Set(
Config.Instance.awsRegionsToVpcIds
Expand All @@ -510,7 +524,7 @@ export async function createRunner(runnerParameters: RunnerInputParameters, metr
metrics.ec2RunInstancesAWSCallSuccess,
metrics.ec2RunInstancesAWSCallFailure,
async () => {
const params: EC2.RunInstancesRequest = {
const params: RunInstancesCommandInput = {
MaxCount: 1,
MinCount: 1,
LaunchTemplate: {
Expand Down Expand Up @@ -548,7 +562,7 @@ export async function createRunner(runnerParameters: RunnerInputParameters, metr
if (customAmi) {
params.ImageId = await findAmiID(metrics, awsRegion, customAmi);
}
return await ec2.runInstances(params).promise();
return await ec2.runInstances(params);
},
);
});
Expand Down
Loading
Loading