support creating routes for multiple subnets #15
Conversation
Guslington
commented
Dec 10, 2021
•
Guslington
commented
- When creating routes (dns and static) change the default behavior of creating a route with single subnet when multiple subnets have been assigned to the vpn to creating routes for all subnets assigned to the vpn.
- reduce the amount of api calls to the AWS API to retrieve the vpn endpoint id when the initializer will do this by exposing the endpoint_id attribute
- automatically change routes to create routes all assigned subnets when updating
Guslington
force-pushed
the
feature/ha-route-populator
branch
from
732e2d9 to
5475bb4
Compare
| end | ||
|
|
||
| def get_groups_for_route(endpoint, cidr) | ||
| def get_auth_rules(dns_route=nil) |
There was a problem hiding this comment.
is dns route the correct variable here and in the ensuing loop?
There was a problem hiding this comment.
dns_route variable is used here because the value is the dns endpoint that is used to generate the routes. This function searches for authorisation rules with the value of dns_route in the description.
| @@ -42,9 +42,8 @@ def revoke_certificate | |||
|
|
|||
| def apply_rekocation_list | |||
There was a problem hiding this comment.
I'm assuming this should be revocation?
There was a problem hiding this comment.
lol v and k and not even close on the keyboard, i'll resolve this in a separate PR
| @@ -163,27 +163,31 @@ def deploy_vpn | |||
| end | |||
| end | |||
|
|
|||
| def get_routes | |||
| @vpn = CfnVpn::ClientVpn.new(@name, @options['region']) | |||
There was a problem hiding this comment.
Is this supposed to exist? And if so what does it do? I can see @vpn gets used below in other code, not sure of the connection to this?
There was a problem hiding this comment.
This is a Thor Actions implementation include Thor::Actions https://github.com/rails/thor/wiki/Actions
Thor doco is not great but it's basically executing each of these methods in the class from top to bottom. The method name is poorly worded it should be def setup. What it's doing is setting an instance of the ClientVpn class which can then be consumed further down the line.
| @@ -29,20 +29,19 @@ def set_directory | |||
| @build_dir = "#{CfnVpn.cfnvpn_path}/#{@name}" | |||
| end | |||
|
|
|||
| def get_endpoint | |||
| def setup | |||
There was a problem hiding this comment.
same thing here, is this a thor thing?
| }) | ||
| return resp.client_configuration | ||
| end | ||
|
|
||
| def get_rekove_list(endpoint_id) | ||
| def get_rekove_list() |
There was a problem hiding this comment.
will fix this name in a separate PR. This repo needs a going over with my new found spell checker
| } | ||
| # to aide in the migration from single to HA routes if the vpn is HA | ||
| if route[:subnets] | ||
| target_subnets = route[:subnets] |
There was a problem hiding this comment.
This comes across a little confusing with the naming 'target_subnets', at first I thought it was a duplication of the changes above but it appears to be different functionality. Maybe worth changing the naming or am I being pedantic?
There was a problem hiding this comment.
it's the same thing but implemented in a different way.
dns_routesare auto generated by the lambda route populatorcidr_routesare static routes which can be created through cloudformation
Client vpn routes are created by routing a cidr to what AWS refer to as a "Target Subnet" which is just a subnet in a vpc
EC2_ClientVpnRoute(...) {
DestinationCidrBlock ...
TargetVpcSubnetId ...
}
