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.

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

Merged
merged 15 commits into from
Sep 9, 2018
Merged
10 changes: 9 additions & 1 deletion packages/@aws-cdk/aws-ec2/lib/connections.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,16 @@ export interface ConnectionsProps {
*
*/
export class Connections {
/**
* Underlying securityGroup for this Connections object, if present
*
* May be empty if this Connections object is not managing a SecurityGroup,
* but simply representing a Connectable peer.
*/
public readonly securityGroup?: SecurityGroupRef;

private readonly securityGroupRule: ISecurityGroupRule;
private readonly securityGroup?: SecurityGroupRef;

private readonly defaultPortRange?: IPortRange;

constructor(props: ConnectionsProps) {
Expand Down
39 changes: 38 additions & 1 deletion packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts
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');
Expand All @@ -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 {
Copy link
Contributor

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:

I'm not a huge fan of this indentation right here. I would prefer some column-aligned. No hard feelings.

Copy link
Contributor Author

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


/**
* Creates a Lambda function object which represents a function not defined
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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.');
Copy link
Contributor

Choose a reason for hiding this comment

The 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 (public get isVpcBound(): boolean or something?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't you know since you were building the model yourself?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
};
}

Expand Down Expand Up @@ -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
})
});
}
}

/**
Expand Down
79 changes: 73 additions & 6 deletions packages/@aws-cdk/aws-lambda/lib/lambda.ts
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';
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

"all private subnets"

*/
vpcPlacement?: ec2.VpcPlacementStrategy;
Copy link
Contributor

Choose a reason for hiding this comment

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

Uh? I thought Lambdas could only be in private subnets?

Copy link
Contributor Author

@rix0rrr rix0rrr Aug 23, 2018

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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/

Choose a reason for hiding this comment

The 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.

Lambda VPC flowchart

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Private (NAT routed) and Isolated (nonrouted) subnets, so we still need a placement argument, but now complicated by the fact that selecting public subnets is an error, and we might not know a priori what the selection is going to match.

For the moment, I'll propose to fix this with docs. Everyone okay with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
}

/**
Expand Down Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this also true in other partitions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gooooooooood question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe role.addManagedPolicy('service-role/xxx') would be nicer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 || [])) {
Expand All @@ -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);
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to renderVpcConfig or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's private, and it does more than just render.

Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe configureVpc

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]
};
}
}
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-lambda/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
"@aws-cdk/assets": "^0.8.2",
"@aws-cdk/aws-cloudwatch": "^0.8.2",
"@aws-cdk/aws-codepipeline-api": "^0.8.2",
"@aws-cdk/aws-ec2": "^0.8.2",
"@aws-cdk/aws-events": "^0.8.2",
"@aws-cdk/aws-iam": "^0.8.2",
"@aws-cdk/aws-logs": "^0.8.2",
Expand Down
Loading