-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
feat(lambda): add cloudwatch lambda insights arm support #17665
Changes from 3 commits
881fc28
0843a06
8e986d6
f3525eb
ad56a5d
0cb160b
a8bf1b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -600,6 +600,8 @@ export class Function extends FunctionBase { | |
private readonly currentVersionOptions?: VersionOptions; | ||
private _currentVersion?: Version; | ||
|
||
private _architecture?: Architecture; | ||
|
||
constructor(scope: Construct, id: string, props: FunctionProps) { | ||
super(scope, id, { | ||
physicalName: props.functionName, | ||
|
@@ -683,7 +685,7 @@ export class Function extends FunctionBase { | |
if (props.architectures && props.architectures.length > 1) { | ||
throw new Error('Only one architecture must be specified.'); | ||
} | ||
const architecture = props.architecture ?? (props.architectures && props.architectures[0]); | ||
this._architecture = props.architecture ?? (props.architectures && props.architectures[0]); | ||
|
||
const resource: CfnFunction = new CfnFunction(this, 'Resource', { | ||
functionName: this.physicalName, | ||
|
@@ -717,7 +719,7 @@ export class Function extends FunctionBase { | |
kmsKeyArn: props.environmentEncryption?.keyArn, | ||
fileSystemConfigs, | ||
codeSigningConfigArn: props.codeSigningConfig?.codeSigningConfigArn, | ||
architectures: architecture ? [architecture.name] : undefined, | ||
architectures: this._architecture ? [this._architecture.name] : undefined, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ruh roh. Looks like we'll be able to have more than one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CloudFormation for some reason accepts a list with a max length of 1. We originally accepted a list, but then deprecated that in favor of a single value #16849. |
||
}); | ||
|
||
resource.node.addDependency(this.role); | ||
|
@@ -935,7 +937,7 @@ Environment variables can be marked for removal when used in Lambda@Edge by sett | |
if (props.runtime !== Runtime.FROM_IMAGE) { | ||
// Layers cannot be added to Lambda container images. The image should have the insights agent installed. | ||
// See https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/Lambda-Insights-Getting-Started-docker.html | ||
this.addLayers(LayerVersion.fromLayerVersionArn(this, 'LambdaInsightsLayer', props.insightsVersion.layerVersionArn)); | ||
this.addLayers(LayerVersion.fromLayerVersionArn(this, 'LambdaInsightsLayer', props.insightsVersion._bind(this, this).arn)); | ||
} | ||
this.role?.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('CloudWatchLambdaInsightsExecutionRolePolicy')); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,18 @@ | ||
import { Aws, CfnMapping, Fn, IResolveContext, Lazy, Stack, Token } from '@aws-cdk/core'; | ||
import { FactName, RegionInfo } from '@aws-cdk/region-info'; | ||
import { Construct } from 'constructs'; | ||
import { Architecture } from './architecture'; | ||
import { IFunction } from './function-base'; | ||
|
||
|
||
// This is the name of the mapping that will be added to the CloudFormation template, if a stack is region agnostic | ||
const DEFAULT_MAPPING_PREFIX = 'LambdaInsightsVersions'; | ||
|
||
interface InsightsBindConfig { | ||
readonly arn: string; | ||
corymhall marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
} | ||
|
||
// To add new versions, update fact-tables.ts `CLOUDWATCH_LAMBDA_INSIGHTS_ARNS` and create a new `public static readonly VERSION_A_B_C_D` | ||
|
||
/** | ||
|
@@ -31,6 +40,11 @@ export abstract class LambdaInsightsVersion { | |
*/ | ||
public static readonly VERSION_1_0_98_0 = LambdaInsightsVersion.fromInsightsVersion('1.0.98.0'); | ||
|
||
/** | ||
* Version 1.0.119.0 | ||
*/ | ||
public static readonly VERSION_1_0_119_0 = LambdaInsightsVersion.fromInsightsVersion('1.0.119.0'); | ||
corymhall marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Use the insights extension associated with the provided ARN. Make sure the ARN is associated | ||
* with same region as your function | ||
|
@@ -40,23 +54,35 @@ export abstract class LambdaInsightsVersion { | |
public static fromInsightVersionArn(arn: string): LambdaInsightsVersion { | ||
class InsightsArn extends LambdaInsightsVersion { | ||
public readonly layerVersionArn = arn; | ||
public _bind(_scope: Construct, _function: IFunction): InsightsBindConfig { | ||
return { arn }; | ||
} | ||
} | ||
return new InsightsArn(); | ||
} | ||
|
||
// Use the verison to build the object. Not meant to be called by the user -- user should use e.g. VERSION_1_0_54_0 | ||
private static fromInsightsVersion(insightsVersion: string): LambdaInsightsVersion { | ||
|
||
// Check if insights version is valid. This should only happen if one of the public static readonly versions are set incorrectly | ||
const versionExists = RegionInfo.regions.some(regionInfo => regionInfo.cloudwatchLambdaInsightsArn(insightsVersion)); | ||
if (!versionExists) { | ||
throw new Error(`Insights version ${insightsVersion} does not exist.`); | ||
} | ||
|
||
class InsightsVersion extends LambdaInsightsVersion { | ||
public readonly layerVersionArn = Lazy.uncachedString({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if it ok to just remove this? It will only be correct if you are using x86. Also not sure under what circumstances you would want to use this property. From what I can tell it is only used internally. |
||
produce: (context) => getVersionArn(context, insightsVersion), | ||
}); | ||
|
||
public _bind(_scope: Construct, _function: IFunction): InsightsBindConfig { | ||
const arch = _function.architecture?.name ?? Architecture.X86_64.name; | ||
// Check if insights version is valid. This should only happen if one of the public static readonly versions are set incorrectly | ||
// or if the version is not available for the Lambda Architecture | ||
const versionExists = RegionInfo.regions.some(regionInfo => regionInfo.cloudwatchLambdaInsightsArn(insightsVersion, arch)); | ||
if (!versionExists) { | ||
throw new Error(`Insights version ${insightsVersion} does not exist.`); | ||
} | ||
return { | ||
arn: Lazy.uncachedString({ | ||
produce: (context) => getVersionArn(context, insightsVersion, arch), | ||
}), | ||
}; | ||
} | ||
} | ||
return new InsightsVersion(); | ||
} | ||
|
@@ -65,6 +91,13 @@ export abstract class LambdaInsightsVersion { | |
* The arn of the Lambda Insights extension | ||
*/ | ||
public readonly layerVersionArn: string = ''; | ||
|
||
/** | ||
* Returns the arn of the Lambda Insights extension based on the | ||
* Lambda architecture | ||
* @internal | ||
*/ | ||
public abstract _bind(_scope: Construct, _function: IFunction): InsightsBindConfig; | ||
} | ||
|
||
/** | ||
|
@@ -73,14 +106,15 @@ export abstract class LambdaInsightsVersion { | |
* | ||
* This function is run on CDK synthesis. | ||
*/ | ||
function getVersionArn(context: IResolveContext, insightsVersion: string): string { | ||
function getVersionArn(context: IResolveContext, insightsVersion: string, architecture?: string): string { | ||
|
||
const scopeStack = Stack.of(context.scope); | ||
const region = scopeStack.region; | ||
const arch = architecture ?? Architecture.X86_64.name; | ||
|
||
// Region is defined, look up the arn, or throw an error if the version isn't supported by a region | ||
if (region !== undefined && !Token.isUnresolved(region)) { | ||
const arn = RegionInfo.get(region).cloudwatchLambdaInsightsArn(insightsVersion); | ||
const arn = RegionInfo.get(region).cloudwatchLambdaInsightsArn(insightsVersion, arch); | ||
if (arn === undefined) { | ||
throw new Error(`Insights version ${insightsVersion} is not supported in region ${region}`); | ||
} | ||
|
@@ -116,19 +150,33 @@ function getVersionArn(context: IResolveContext, insightsVersion: string): strin | |
* -- {'arn': 'arn3'}, | ||
* - us-east-2 | ||
* -- {'arn': 'arn4'} | ||
* LambdaInsightsVersions101190arm64 // a separate mapping version 1.0.119.0 arm64 | ||
* - us-east-1 | ||
* -- {'arn': 'arn3'}, | ||
* - us-east-2 | ||
* -- {'arn': 'arn4'} | ||
*/ | ||
|
||
const mapName = DEFAULT_MAPPING_PREFIX + insightsVersion.split('.').join(''); | ||
let mapName = DEFAULT_MAPPING_PREFIX + insightsVersion.split('.').join(''); | ||
// if the architecture is arm64 then append that to the end of the name | ||
// this is so that we can have a separate mapping for x86 vs arm in scenarios | ||
// where we have Lambda functions with both architectures in the same stack | ||
if (arch === Architecture.ARM_64.name) { | ||
mapName += arch; | ||
} | ||
const mapping: { [k1: string]: { [k2: string]: any } } = {}; | ||
const region2arns = RegionInfo.regionMap(FactName.cloudwatchLambdaInsightsVersion(insightsVersion)); | ||
const region2arns = RegionInfo.regionMap(FactName.cloudwatchLambdaInsightsVersion(insightsVersion, arch)); | ||
for (const [reg, arn] of Object.entries(region2arns)) { | ||
mapping[reg] = { arn }; | ||
} | ||
|
||
// Only create a given mapping once. If another version of insights is used elsewhere, that mapping will also exist | ||
if (!scopeStack.node.tryFindChild(mapName)) { | ||
new CfnMapping(scopeStack, mapName, { mapping }); | ||
// need to call findInMap here if we are going to set lazy=true, otherwise | ||
// we get the informLazyUse info message | ||
const map = new CfnMapping(scopeStack, mapName, { mapping, lazy: true }); | ||
return map.findInMap(Aws.REGION, 'arn'); | ||
} | ||
// The ARN will be looked up at deployment time from the mapping we created | ||
return Fn.findInMap(mapName, Aws.REGION, 'arn'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we default this to
X86
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past it looks like we decided to intentionally not default this. See this comment and this comment