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(ec2): ClientVpn support #4233

Draft
wants to merge 28 commits into
base: master
from
Draft

feat(ec2): ClientVpn support #4233

wants to merge 28 commits into from

Conversation

@nmussy
Copy link
Contributor

nmussy commented Sep 25, 2019

Fixes #4206


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Sep 25, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process
@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

readonly authenticationOptions: IClientAuthenticationRequest[];
readonly clientCidrBlock: string;
readonly connectionLogOptions: IConnectionLogOptions;
// TODO replace with serverCertificate object?

This comment has been minimized.

Copy link
@nmussy

nmussy Sep 25, 2019

Author Contributor

Unfortunately, I don't think it's possible:

Dependency cycles detected, you should fix these!
aws-cdk/aws-certificatemanager -> @aws-cdk/aws-ec2 -> @aws-cdk/aws-lambda -> @aws-cdk/aws-certificatemanager

Any way to work around this?

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 26, 2019

Contributor

I'm sorry, where are these dependencies coming from?

On my machine, I'm not seeing any dependency from EC2 to either Lambda or CertificateManager.

This comment has been minimized.

Copy link
@nmussy

nmussy Sep 26, 2019

Author Contributor

I was trying to add @aws-cdk/aws-certificatemanager to EC2 to get access to ICertificates. I haven't committed it because of the previous error

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Is it possible to use both IAM and ACM certificates here? I've been thinking about this for a while. A potential solution might be to declare an iam.ICertificate (or iam.ICertificateSource), and have ACM's ICertificate extend that. Since everything depends on IAM already, we wouldn't have any dependency issues.

This comment has been minimized.

Copy link
@nmussy

nmussy Sep 30, 2019

Author Contributor

I don't think it's possible to use IAM certificates in this case, unless I'm missing something:

Getting started:

The server certificate must be provisioned in AWS Certificate Manager (ACM)

Client Authentication and Authorization:

The server and client certificates must be uploaded to AWS Certificate Manager (ACM).

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Dang.

nmussy added 6 commits Sep 25, 2019
@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 25, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

jogold left a comment

Hey @nmussy,

I'm currently using Client VPN in production and have developed a CDK construct for my use case.

Some feedback on your PR based on my experience:

  • I would expect the construct to create a new logs.LogGroup if the user wants logging but doesn't want to specify group/stream (a retention prop could be exposed)
  • Instead of having to specify a subnet: ISubnet how about using a subnetSelection: ec2.SubnetSelection; like other CDK constructs do?
  • You are missing a security group on your target network associations, without it the client vpn will be using the VPC's default security group. This means that you cannot offer connections on your construct. Specifying a security group for a target network association is currently not supported by CloudFormation, I had to go with the AwsCustomResource, something like this:
this.securityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', {
  vpc: props.vpc,
  allowAllOutbound: false,
});

// Use custom resource to update security group
new custom.AwsCustomResource(this, 'ApplySecurityGroupsToClientVpnTargetNetwork', {
  onUpdate: {
    service: 'EC2',
    action: 'applySecurityGroupsToClientVpnTargetNetwork',
    parameters: {
      ClientVpnEndpointId: clientVpnEndpoint.ref,
      VpcId: props.vpc.vpcId,
      SecurityGroupIds: [this.securityGroup.securityGroupId],
    } as EC2.ApplySecurityGroupsToClientVpnTargetNetworkRequest,
    physicalResourceId: this.securityGroup.securityGroupId,
  },
});
@nmussy

This comment has been minimized.

Copy link
Contributor Author

nmussy commented Sep 26, 2019

Thanks a lot @jogold for the great comments, as always. I'll keep pushing this feature tomorrow, I should have to ready for review soon-ish.

@nmussy

This comment has been minimized.

Copy link
Contributor Author

nmussy commented Sep 27, 2019

Concerning @jogold second comment:

Instead of having to specify a subnet: ISubnet how about using a subnetSelection: ec2.SubnetSelection; like other CDK constructs do?

This would require asking the user for a VPC to vpc.selectSubnets, while it's not strictly required for any ClientVpn construct.
EDIT: The VPC would be used to create the security group his third comment

There's also the issue of having multiple subnets returned by vpc.selectSubnets. I don't know if we should throw and warn the user that their selection is ambiguous, or change the method to a addTargetNetworkAssociations and all the selected subnets.

Thoughts?

Copy link
Contributor

rix0rrr left a comment

There's also the issue of having multiple subnets returned by vpc.selectSubnets. I don't know if we should throw and warn the user that their selection is ambiguous, or change the method to a addTargetNetworkAssociations and all the selected subnets.

Right now there is no way to select a single subnet, it would always be a list of subnets. On the other hand, this feature has been requested so at some point SubnetSelection will grow a way to pick out individual subnets.

Is there a problem with adding the routes to multiple subnets?

Unless you have a pressing need right now to select individual subnets, I suggest you lift along with the rising tide of SubnetSelection. If you do have a pressing need, I still think you should probably implement the SubnetSelection variant, maybe add a special method to apply to a single specific ISubnet, and mark that one @experimental.

In fact, I think I would like it if you marked all of the new classes and interfaces @experimental, so we can give the new API a couple of releases to bake and a number of people to try it out before we commit to it.

import {CIDR_VALIDATION_REGEXES} from "./peer";
import {ISubnet} from './vpc';

interface IClientAuthenticationRequestOptions {

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Docstring on the type declaration itself as well please. This comment applies to all interfaces and classes.

/**
* Information about the Active Directory to be used
*/
readonly activeDirectory?: {

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

This isn't going to work in jsii (and if it does, it shouldn't).

Either inline the directoryId into the parent struct, or define it as an IActiveDirectory with a class that implements it.

/**
* Information about the authentication certificates to be used
*/
readonly mutualAuthentication?: {

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Same here.

/**
* The name of the CloudWatch Logs log group
*/
readonly cloudwatchLogGroup?: string;

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Tell us using a @default declaration what happens if you don't supply this.

Feels like this should be an ILogGroup, and the same for ILogStream, no?

*/
readonly cloudwatchLogStream?: string;
/**
* Indicates whether connection logging is enabled

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Would a default of true be sensible here?

}
}

interface IConnectionLogOptions {

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

This seems to be an intra-framework communication struct (as opposed to something that users are expected to fill in). We tend to call these XxxConfig.

This comment has been minimized.

Copy link
@nmussy

nmussy Sep 30, 2019

Author Contributor

It is. Do your previous comments about the default values still apply, seen as they're required by CloudFormation (either cloudwatchLogGroup or cloudwatchLogStream)?

* The id of the Client VPN Endpoint
*/
readonly clientVpnEndpointId: string;
readonly routes: ClientVpnRoute[];

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Docstrings please.

this.clientVpnEndpointId = clientVpnEndpoint.ref;
}

public addRoute(options: ClientVpnRouteOptions): ClientVpnRoute {

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Does it make sense to add routes?: ClientVpnRouteOptions[] to the construction props and call this method on them in the constructor?

This comment has been minimized.

Copy link
@nmussy

nmussy Sep 30, 2019

Author Contributor

Yeah, I'll add it for all 3 addX methods

}

export interface ClientVpnRouteProps extends ClientVpnRouteOptions {
readonly clientVpnEndpoint: IClientVpnEndpoint;

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

Docs?

subnet,
});

this.targetNetworkAssociations.push(association);

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

This is being pushed both here and in the constructed class.

I'm not sure that I'm a fan. What's the use case for maintaining and publishing these objects through the public array?

This comment has been minimized.

Copy link
@nmussy

nmussy Sep 30, 2019

Author Contributor

I was keeping track of them for the this.targetNetworkAssociations.length, but it doesn't feel great I agree. I didn't know what else to hook onto for the id generation.

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Sep 30, 2019

Contributor

You can just keep the count, instead of the objects themselves.

@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Sep 30, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@nmussy

This comment has been minimized.

Copy link
Contributor Author

nmussy commented Sep 30, 2019

I was looking into implementing @jogold's AwsCustomResource to associate the security group, but it causes another cyclic dependency:

lerna ERR! ECYCLE Dependency cycles detected, you should fix these!
lerna ERR! ECYCLE @aws-cdk/aws-cloudformation -> @aws-cdk/custom-resources -> @aws-cdk/aws-ec2 -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation

An issue was recently opened on the roadmap (#199) to implement the missing method.

@aws-cdk-automation

This comment has been minimized.

Copy link
Collaborator

aws-cdk-automation commented Oct 1, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@jogold

This comment has been minimized.

Copy link
Contributor

jogold commented Oct 3, 2019

I suggest you lift along with the rising tide of SubnetSelection.

We can maybe combine a SubnetSelection with a maxAZs prop... the price per client VPN endpoint association can be costly

@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process
5 similar comments
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Dec 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.