-
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(ec2): client vpn endpoint #12234
Conversation
To be discussed:
|
@rix0rrr any feedback here? |
/** | ||
* A Lambda function | ||
*/ | ||
export interface IFunction { |
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 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.
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 now IClientVpnConnectionHandler
, and FunctionBase
implements it.
/** | ||
* A certificate | ||
*/ | ||
export interface ICertificate { |
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.
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.
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 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?
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.
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.
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.
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; |
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.
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.
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.
You mean something like this?
public abstract readonly directory?: { directoryId: string };
public abstract readonly samlProvider?: { samlProviderArn: string, selfServiceSamlProviderArn?: string };
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.
Actually I don't get it, isn't the ClientVpnUserBasedAuthentication
the entire object with the fields set according to the chosen auth?
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.
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.
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.
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
?
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.
The static methods still make sense as factory functions. You can also skip exporting the class that way.
Pull request has been modified.
Now that CF supports SAML providers, this needs #13393. |
/** | ||
* Active Directory authentication | ||
*/ | ||
public static activeDirectory(directoryId: string): ClientVpnUserBasedAuthentication { |
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.
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
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.
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.
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
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*
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*
Add support for client VPN endpoints with the following L2s:
ClientVpnEndpoint
,ClientVpnAuthorizationRule
andClientVpnRoute
.Client VPN endpoints can be added to VPCs with the
addClientVpnEndpoint()
method.
Both mutual and user-based authentication are supported.
The
ClientVpnEndpoint
class implementsIConnectable
.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