-
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(aws-lambda): allow placing Lambda in VPC #598
Changes from 7 commits
6ba0614
cfefb7e
e807777
d4ef24c
5f9e0f6
66111d7
d078fec
66900d3
5cf128f
37bd186
9fe130f
d11ffb0
305da53
30f78dc
4eb363a
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 |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import cloudwatch = require('@aws-cdk/aws-cloudwatch'); | ||
import ec2 = require('@aws-cdk/aws-ec2'); | ||
import events = require('@aws-cdk/aws-events'); | ||
import iam = require('@aws-cdk/aws-iam'); | ||
import logs = require('@aws-cdk/aws-logs'); | ||
|
@@ -22,10 +23,16 @@ export interface FunctionRefProps { | |
* If the role is not specified, any role-related operations will no-op. | ||
*/ | ||
role?: iam.Role; | ||
|
||
/** | ||
* SecurityGroupId of the securityGroup for this Lambda, if configured. | ||
*/ | ||
securityGroupId?: ec2.SecurityGroupId; | ||
} | ||
|
||
export abstract class FunctionRef extends cdk.Construct | ||
implements events.IEventRuleTarget, logs.ILogSubscriptionDestination, s3n.IBucketNotificationDestination { | ||
implements events.IEventRuleTarget, logs.ILogSubscriptionDestination, s3n.IBucketNotificationDestination, | ||
ec2.IConnectable { | ||
|
||
/** | ||
* Creates a Lambda function object which represents a function not defined | ||
|
@@ -134,6 +141,13 @@ export abstract class FunctionRef extends cdk.Construct | |
*/ | ||
protected abstract readonly canCreatePermissions: boolean; | ||
|
||
/** | ||
* Actual connections object for this Lambda | ||
* | ||
* May be unset, in which case this Lambda is not configured use in a VPC. | ||
*/ | ||
protected _connections?: ec2.Connections; | ||
|
||
/** | ||
* Indicates if the policy that allows CloudWatch logs to publish to this lambda has been added. | ||
*/ | ||
|
@@ -170,6 +184,18 @@ export abstract class FunctionRef extends cdk.Construct | |
this.role.addToPolicy(statement); | ||
} | ||
|
||
/** | ||
* Access the Connections object | ||
* | ||
* Will fail if not a VPC-enabled Lambda Function | ||
*/ | ||
public get connections(): ec2.Connections { | ||
if (!this._connections) { | ||
throw new Error('Only VPC-associated Lambda Functions can have their security groups managed.'); | ||
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. Somehow I am bummed out by the fact this seems to expect anything to know up-front whether a function is in-VPC or not... At lease should provide a property to verify this ( 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. Wouldn't you know since you were building the model yourself? 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. The error message should tell the user how to fix the problem? (i.e. how to add the lambda to a VPC) |
||
} | ||
return this._connections; | ||
} | ||
|
||
/** | ||
* Returns a RuleTarget that can be used to trigger this Lambda as a | ||
* result from a CloudWatch event. | ||
|
@@ -261,6 +287,9 @@ export abstract class FunctionRef extends cdk.Construct | |
public export(): FunctionRefProps { | ||
return { | ||
functionArn: new cdk.Output(this, 'FunctionArn', { value: this.functionArn }).makeImportValue(), | ||
securityGroupId: this._connections && this._connections.securityGroup | ||
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. What's the rational of exporting the security group ID of the Lambda? Is it in order to be able to use connections on an imported lambda? Maybe worth explaining in the docs 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. That's exactly it. |
||
? new cdk.Output(this, 'SecurityGroupId', { value: this._connections.securityGroup.securityGroupId }).makeImportValue() | ||
: undefined | ||
}; | ||
} | ||
|
||
|
@@ -322,6 +351,14 @@ class LambdaRefImport extends FunctionRef { | |
this.functionArn = props.functionArn; | ||
this.functionName = this.extractNameFromArn(props.functionArn); | ||
this.role = props.role; | ||
|
||
if (props.securityGroupId) { | ||
this._connections = new ec2.Connections({ | ||
securityGroup: ec2.SecurityGroupRef.import(this, 'SecurityGroup', { | ||
securityGroupId: props.securityGroupId | ||
}) | ||
}); | ||
} | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import ec2 = require('@aws-cdk/aws-ec2'); | ||
import iam = require('@aws-cdk/aws-iam'); | ||
import cdk = require('@aws-cdk/cdk'); | ||
import { Code } from './code'; | ||
|
@@ -89,6 +90,31 @@ export interface FunctionProps { | |
* Both supplied and generated roles can always be changed by calling `addToRolePolicy`. | ||
*/ | ||
role?: iam.Role; | ||
|
||
/** | ||
* VPC network to place Lambda network interfaces | ||
* | ||
* Specify this if the Lambda function needs to access resources in a VPC. | ||
*/ | ||
vpc?: ec2.VpcNetworkRef; | ||
|
||
/** | ||
* Where to place the network interfaces within the VPC. | ||
* | ||
* Only used if 'vpc' is supplied. | ||
* | ||
* @default Private subnets | ||
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. "all private subnets" |
||
*/ | ||
vpcPlacement?: ec2.VpcPlacementStrategy; | ||
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. Uh? I thought Lambdas could only be in private subnets? 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. Why would that be? I mean, it might be true, but since it's just an ENI I'm guessing you can place it anywhere. How would such a restriction even work? If the routing table points to an IGW instead of an NGW, the Lambda will not work? We also still have isolated subnets that need to be selectable. 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. This blog post seems to imply you need to do something different for public and private subnets, thereby implying it could be in a public subnet: https://aws.amazon.com/premiumsupport/knowledge-center/internet-access-lambda-function/ 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. Maybe unsolicited feedback but I've been browsing the PRs to learn some CDK aspects.. While Lambda can technically be in a public subnet, invocations will fail if the function attempts to reach any publicly routable IP (ie 0.0.0.0 -> VPC internet gateway). Therefore, our recommendation is to always add functions in a Private Subnet with NAT Gateway should they need to reach any publicly routable IP. 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. Thanks for the clarification @heitorlessa. I guess I was wrong. That still leaves the opportunity to have For the moment, I'll propose to fix this with docs. Everyone okay with this? 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. Fixed it with a post-selection check. |
||
|
||
/** | ||
* What security group to associate with the Lambda's network interfaces. | ||
* | ||
* Only used if 'vpc' is supplied. | ||
* | ||
* @default A unique security group is created for this Lambda function. | ||
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. I would be a bit more descriptive here: "If the function is placed within a VPC and a security group is not specified, a dedicated security group will be created for this function" |
||
*/ | ||
securityGroup?: ec2.SecurityGroupRef; | ||
} | ||
|
||
/** | ||
|
@@ -140,16 +166,31 @@ export class Function extends FunctionRef { | |
|
||
this.environment = props.environment || { }; | ||
|
||
this.role = props.role || new iam.Role(this, 'ServiceRole', { | ||
assumedBy: new cdk.ServicePrincipal('lambda.amazonaws.com'), | ||
// the arn is in the form of - arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole | ||
managedPolicyArns: [ cdk.Arn.fromComponents({ | ||
const managedPolicyArns = new Array<cdk.Arn>(); | ||
|
||
// the arn is in the form of - arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole | ||
managedPolicyArns.push(cdk.Arn.fromComponents({ | ||
service: "iam", | ||
region: "", // no region for managed policy | ||
account: "aws", // the account for a managed policy is 'aws' | ||
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. Is this also true in other partitions? 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. Gooooooooood question. 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. Yes, it is. It's not the partition identifier, it's an abstraction of the account number. |
||
resource: "policy", | ||
resourceName: "service-role/AWSLambdaBasicExecutionRole" | ||
})); | ||
|
||
if (props.vpc) { | ||
// Policy that will have ENI creation permissions | ||
managedPolicyArns.push(cdk.Arn.fromComponents({ | ||
service: "iam", | ||
region: "", // no region for managed policy | ||
account: "aws", // the account for a managed policy is 'aws' | ||
resource: "policy", | ||
resourceName: "service-role/AWSLambdaBasicExecutionRole", | ||
})], | ||
resourceName: "service-role/AWSLambdaVPCAccessExecutionRole" | ||
})); | ||
} | ||
|
||
this.role = props.role || new iam.Role(this, 'ServiceRole', { | ||
assumedBy: new cdk.ServicePrincipal('lambda.amazonaws.com'), | ||
managedPolicyArns, | ||
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. Maybe 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. Ah, but you can have your own managed policies that you can attach, and they'll have a different account ('12345' vs 'aws'). Let's use ARNs to be safe. Ultimately, this should be just "the object", not "the ARN", but I just wanted to make this copy/pasted code slighly better, not introduce a new modeling of ManagedPolicies as part of this CR. |
||
}); | ||
|
||
for (const statement of (props.initialPolicy || [])) { | ||
|
@@ -166,6 +207,7 @@ export class Function extends FunctionRef { | |
role: this.role.roleArn, | ||
environment: new cdk.Token(() => this.renderEnvironment()), | ||
memorySize: props.memorySize, | ||
vpcConfig: this.addToVpc(props), | ||
}); | ||
|
||
resource.addDependency(this.role); | ||
|
@@ -226,4 +268,29 @@ export class Function extends FunctionRef { | |
variables: this.environment | ||
}; | ||
} | ||
|
||
/** | ||
* If configured, set up the VPC-related properties | ||
* | ||
* Returns the VpcConfig that should be added to the | ||
* Lambda creation properties. | ||
*/ | ||
private addToVpc(props: FunctionProps): cloudformation.FunctionResource.VpcConfigProperty | 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. rename to 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. It's private, and it does more than just render. 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. So maybe |
||
if (!props.vpc) { return undefined; } | ||
|
||
let securityGroup = props.securityGroup; | ||
if (!securityGroup) { | ||
securityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', { | ||
vpc: props.vpc, | ||
description: 'Automatic security group for Lambda Function ' + this.uniqueId, | ||
}); | ||
} | ||
|
||
this._connections = new ec2.Connections({ securityGroup }); | ||
|
||
return { | ||
subnetIds: props.vpc.subnets(props.vpcPlacement).map(s => s.subnetId), | ||
securityGroupIds: [securityGroup.securityGroupId] | ||
}; | ||
} | ||
} |
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.
Do whatever you want with that comment:
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.
This is what my autoformatter made of it, but I guess I'm in board with you