feat(ecs): vpc link for api gatway and load balanced services #1541
Conversation
Overview
========
The primary purpose of this work is to fill in the gaps in
implementation for deploying a VPC link between API Gateway, and an
ECS service. My goal was to allow setting up a {proxy+} API which would
forward to a Fargate service in a private VPC.
This has been tagged as 'ecs', but also involves changes to api gateway.
Since VPC links require an NLB, the LoadBalanced{Fargate|Ecs}Service classes
have been modified to support selecting either an ALB or an NLB.
Changes
=======
On the APIGW side, `IntegrationOptions` now accepts an optional connetion
type enum, as well as a VpcLink. `VpcLink` itself is a new construct
which accepts an array of Network Load Balancers. I also added the missing
`requestParameters` prop for `Method`, to allow properly setting up a proxy
path variable.
For ECS, in my use case I wanted to use the LoadBalanced*Service constructs, however
they only supported ALB. I have pulled all of the ELBv2 related setup
into the new `LoadBalancedService` base class, and also created a base
props interface `LoadBalancedServiceProps`. This deals with the common setup
between the Fargate and ECS services, and allows the selection of ALB or NLB.
As a side-effect of this refactoring, you can also now pass a Certificate to
`LoadBalancedEcsService`.
There is a new `Method` test for the VPC link props, as well as new tests
for both `VpcLink` and `LoadBalancedFargateService`.
| @@ -173,6 +179,8 @@ export class Method extends cdk.Construct { | |||
| requestTemplates: options.requestTemplates, | |||
| passthroughBehavior: options.passthroughBehavior, | |||
| integrationResponses: options.integrationResponses, | |||
| connectionType: options.connectionType, | |||
| connectionId: options.vpcLink ? options.vpcLink.vpcLinkId : undefined, | |||
There was a problem hiding this comment.
What if connectiontype === internet and a vpcLink is supplied. Is that ok?
There was a problem hiding this comment.
The docs for ConnectionId state:
The ID of the VpcLink used for the integration when connectionType=VPC_LINK and undefined, otherwise.
I took that to mean that it would just be ignored. Probably best to throw an error though to alert the user, since that's probably not what they wanted to happen.
There was a problem hiding this comment.
That's what I was getting at. Thanks.
| /** | ||
| * The name used to label and identify the VPC link. | ||
| */ | ||
| name: string; |
There was a problem hiding this comment.
Please make this optional.
If CloudFormation doesn't allow an optional name, you can always default inside the construct to this.uniqueId.
| * The network load balancers of the VPC targeted by the VPC link. | ||
| * The network load balancers must be owned by the same AWS account of the API owner. | ||
| */ | ||
| targets: elbv2.NetworkLoadBalancer[]; |
There was a problem hiding this comment.
This should take elbv2.INetworkLoadBalancer[].
| */ | ||
| export interface VpcLinkProps { | ||
| /** | ||
| * The name used to label and identify the VPC link. |
There was a problem hiding this comment.
@default automatically generated name
| name: string; | ||
|
|
||
| /** | ||
| * The description of the VPC link. |
| }); | ||
| } | ||
|
|
||
| public get vpcLinkId() { |
There was a problem hiding this comment.
This works, but we generally just declare a string property on the construct and assign it in the constructor.
| @@ -132,6 +90,7 @@ export class LoadBalancedFargateService extends cdk.Construct { | |||
|
|
|||
| const container = taskDefinition.addContainer('web', { | |||
| image: props.image, | |||
| environment: props.containerEnvironment, | |||
There was a problem hiding this comment.
This was recently also added on master, so you need to merge from master and this change is no longer necessary.
| /** | ||
| * Base class for load-balanced Fargate and ECS service | ||
| */ | ||
| export abstract class LoadBalancedService extends cdk.Construct { |
There was a problem hiding this comment.
Please rename to LoadBalancedServiceBase to make it clear from the name that it's not meant to be instantiated. I know, it's abstract already, but more clarity never hurts.
| } | ||
|
|
||
| const targetProps = { | ||
| port: 80 |
There was a problem hiding this comment.
Shouldn't this use containerPort?
There was a problem hiding this comment.
I thought so too, but apparently when used with ECS the port number for the target group is ignored.
I can confirm that it works fine as-is, I use 8000 for my container port.
…ion type is set to 'internet'
Thanks @rix0rrr! I've committed the requested changes, save for setting the LB target port to the container port. That was hard-coded to 80 prior to my changes, and as I said, I don't think it's actually used in conjunction with ECS anyway. |
Interesting. Well okay, if it works then it works :). |
Overview
The primary purpose of this work is to fill in the gaps in
implementation for deploying a VPC link between API Gateway, and an
ECS service. My goal was to allow setting up a {proxy+} API which would
forward to a Fargate service in a private VPC.
This has been tagged as 'ecs', but also involves changes to api gateway.
Since VPC links require an NLB, the LoadBalanced{Fargate|Ecs}Service classes
have been modified to support selecting either an ALB or an NLB.
Closes #1500.
Changes
On the APIGW side,
IntegrationOptionsnow accepts an optional connetiontype enum, as well as a VpcLink.
VpcLinkitself is a new constructwhich accepts an array of Network Load Balancers. I also added the missing
requestParametersprop forMethod, to allow properly setting up a proxypath variable.
For ECS, in my use case I wanted to use the LoadBalanced*Service constructs, however
they only supported ALB. I have pulled all of the ELBv2 related setup
into the new
LoadBalancedServicebase class, and also created a baseprops interface
LoadBalancedServiceProps. This deals with the common setupbetween the Fargate and ECS services, and allows the selection of ALB or NLB.
As a side-effect of this refactoring, you can also now pass a Certificate to
LoadBalancedEcsService.There is a new
Methodtest for the VPC link props, as well as new testsfor both
VpcLinkandLoadBalancedFargateService.Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.