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): client vpn endpoint #12234

Merged
merged 25 commits into from Mar 22, 2021
Merged

feat(ec2): client vpn endpoint #12234

merged 25 commits into from Mar 22, 2021

Conversation

jogold
Copy link
Contributor

@jogold jogold commented Dec 26, 2020

Add support for client VPN endpoints with the following L2s: ClientVpnEndpoint,
ClientVpnAuthorizationRule and ClientVpnRoute.

Client VPN endpoints can be added to VPCs with the addClientVpnEndpoint()
method.

Both mutual and user-based authentication are supported.

The ClientVpnEndpoint class implements IConnectable.

Use a custom resource to import server and client certificates in ACM
for the integration test.

Close #4206


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

@gitpod-io
Copy link

gitpod-io bot commented Dec 26, 2020

@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Dec 26, 2020
@jogold
Copy link
Contributor Author

jogold commented Dec 29, 2020

To be discussed:

  • mark as experimental?
  • should ClientVpnUserBasedAuthentication.activeDirectory() take a IDirectory? There's no IDirectory for the moment but we can assume that it will come in aws-directoryservice
  • same but different for ClientVpnUserBasedAuthentication.federated() with the federated role that is currently not supported by CF

@jogold
Copy link
Contributor Author

jogold commented Feb 4, 2021

@rix0rrr any feedback here?

/**
* A Lambda function
*/
export interface IFunction {
Copy link
Contributor

Choose a reason for hiding this comment

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

This only works in TypeScript, not in nominally-typed languages.

What you'd normally have to do is define an interface (or abstract class) for integrations and make a concrete class in a separate package combining EC2 types and Lambda types.

However in this case, I believe aws-lambda already depends on aws-ec2. So it's permissible in this case to define an interface here and have aws-lambda.Function implement it directly.

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 now IClientVpnConnectionHandler, and FunctionBase implements it.

/**
* A certificate
*/
export interface ICertificate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some for this, although the dependency is trickier. We don't have a dependency yet and adding one feels unsafe. So definitely looks like it needs an integration package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need a ICertifcate here after all? Would it be here acceptable to reference an ARN (string) directly in ClientVpnEndpointOptions? I mean how likely is it that CF adds support for "imported" certificates in ACM?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what "imported" certificates means, in this case.

I guess just taking an ARN is acceptable for now... I suppose we can deprecate that field once we figure out a good way to do the integration. Everything I can think of is pretty bad, so just taking a string seems like the simplest solution.

Copy link
Contributor Author

@jogold jogold Mar 8, 2021

Choose a reason for hiding this comment

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

Imported means this: https://docs.aws.amazon.com/acm/latest/userguide/import-certificate.html

Certificates automatically issued by ACM cannot be used here for the client VPN endpoint.

Changed for now to serverCertificateArn and clientCertificateArn but another option could be to define IClientVpnCertificate and when CF adds a resource in ACM for imported certificates it will implement it?

I see that CF added support for AWS::IAM::ServerCertificate last week, what we need here is the same but in ACM. See aws-cloudformation/cloudformation-coverage-roadmap#614.

This is also why I'm using a custom resource for the integ test.

}

/** The ID of the Active Directory to be used for authentication */
public abstract readonly directoryId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels more correct to abstract this away one more level and return the entire object, with "whatever" fields need to be in there fore the given auth type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like this?

public abstract readonly directory?: { directoryId: string };
public abstract readonly samlProvider?: { samlProviderArn: string, selfServiceSamlProviderArn?: string };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I don't get it, isn't the ClientVpnUserBasedAuthentication the entire object with the fields set according to the chosen auth?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is. I'm talking about the protocol between ClientVpnUserBasedAuthentication and ClientVpnEndpoint.

I was thinking the protocol didn't need to be more complicated than this:

export abstract class ClientVpnUserBasedAuthentication {
  public abstract render(): any;
}

Then ClientVpnEndpoint doesn't need to care about the individual fields in that output structure anymore, but can just trust that whatever ClientVpnUserBasedAuthentication returns is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, this means

export class ClientVpnActiveDirectoryAuthentication extends ClientVpnUserBasedAuthentication {
  constructor(private readonly directoryId: string) {
    super();
  }

  render() {
    return {
      type: 'directory-service-authentication',
      activeDirectory: {
        directoryId: this.directoryId,
      },
    };
  }
}

and we drop the static methods on ClientVpnUserBasedAuthentication?

Copy link
Contributor

Choose a reason for hiding this comment

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

The static methods still make sense as factory functions. You can also skip exporting the class that way.

@mergify mergify bot dismissed rix0rrr’s stale review February 25, 2021 16:04

Pull request has been modified.

@jogold
Copy link
Contributor Author

jogold commented Mar 4, 2021

Now that CF supports SAML providers, this needs #13393.

/**
* Active Directory authentication
*/
public static activeDirectory(directoryId: string): ClientVpnUserBasedAuthentication {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here again, maybe we can do IClientVpnActiveDirectory instead?

There are currently no L2s in @aws-cdk/aws-directoryservice but when they'll come there will be a dependency on @aws-cdk/aws-ec2

Copy link
Contributor

Choose a reason for hiding this comment

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

We can always add a new factory method for that if and when it becomes necessary. Let's not worry about it too much for now.

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 1a1ca00
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 4fde59a into aws:master Mar 22, 2021
eladb pushed a commit that referenced this pull request Mar 24, 2021
Add support for client VPN endpoints with the following L2s: `ClientVpnEndpoint`,
`ClientVpnAuthorizationRule` and `ClientVpnRoute`.

Client VPN endpoints can be added to VPCs with the `addClientVpnEndpoint()`
method.

Both mutual and user-based authentication are supported.

The `ClientVpnEndpoint` class implements `IConnectable`.

Use a custom resource to import server and client certificates in ACM
for the integration test.

Close #4206

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
Add support for client VPN endpoints with the following L2s: `ClientVpnEndpoint`,
`ClientVpnAuthorizationRule` and `ClientVpnRoute`.

Client VPN endpoints can be added to VPCs with the `addClientVpnEndpoint()`
method.

Both mutual and user-based authentication are supported.

The `ClientVpnEndpoint` class implements `IConnectable`.

Use a custom resource to import server and client certificates in ACM
for the integration test.

Close aws#4206

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ec2: Support for ClientVpnEndpoint resources
3 participants