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

Add support for updating AWS routing table entries #37

Merged
merged 2 commits into from
Aug 29, 2016
Merged

Add support for updating AWS routing table entries #37

merged 2 commits into from
Aug 29, 2016

Conversation

geofffranks
Copy link
Contributor

The advertised_instance_routes cloud property on resource pools/vm
types has been added to allow manifests to connect routes on AWS
Routing Tables with specific instances, if you need to route traffic
through a BOSH deployed VM (like in the case of a site-to-site VPN).

If using this, it is recommended to only have a single instance of a
single job of that resource pool, to avoid confusion. If there are
multiple instances/jobs in the resource pool, the last one processed
will be the node assigned to the route.

Datastructure of advertised_instance_routes is as follows:

cloud_properties:
  advertised_instance_routes:
  - table_id:    rt-abcdef123
    destination: 10.0.0.0/16
  - table_id:    rt 12345abcd
    destination: 10.0.0.0/16

This allows the manifest to specify that multiple routing tables
are updated to send the destination route(s) to it. Once removed
from the manifest, the routes will remain, but once the VM is recreated,
the route will become a black hole in the AWS routing table, and cease
to work.

The `advertised_instance_routes` cloud property on resource pools/vm
types has been added to allow manifests to connect routes on AWS
Routing Tables with specific instances, if you need to route traffic
through a BOSH deployed VM (like in the case of a site-to-site VPN).

If using this, it is recommended to only have a single instance of a
single job of that resource pool, to avoid confusion. If there are
multiple instances/jobs in the resource pool, the last one processed
will be the node assigned to the route.

Datastructure of `advertised_instance_routes` is as follows:

```
cloud_properties:
  advertised_instance_routes:
  - table_id:    rt-abcdef123
    destination: 10.0.0.0/16
  - table_id:    rt 12345abcd
    destination: 10.0.0.0/16
```

This allows the manifest to specify that multiple routing tables
are updated to send the destination route(s) to it. Once removed
from the manifest, the routes will remain, but once the VM is recreated,
the route will become a black hole in the AWS routing table, and cease
to work.
@cfdreddbot
Copy link

Hey geofffranks!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@logger.debug("Sending traffic for '#{definition["destination"]}' to '#{@aws_instance.id}' in '#{definition["table_id"]}'")

existing = false
table.routes.each do |route|
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs say routes is a normal array, so could probably shorten this to: existing = table.routes.any ...

@dpb587
Copy link
Contributor

dpb587 commented Aug 29, 2016

Oh, that's an interesting idea. Hadn't considered this sort of thing as a CPI property before, just as a co-located release sort of thing. Wonder what CPI team thinks of this approach.

@cppforlife
Copy link
Contributor

@dpb587 api already talks to the iaas so might as well have it here instead of connecting from somewhere else. it's logged, diffed via cloud properties, audited through events, etc.

@ljfranklin
Copy link
Contributor

@geofffranks neat feature! Looks good, my only feedback is seems a little messy to not delete the route when we delete the VM. Do you see any issues with calling delete_route during delete_vm for any routes that have route.instance == instance_to_delete, or is there some reason why a black-hole route is desirable?

@dpb587
Copy link
Contributor

dpb587 commented Aug 29, 2016

Cool - I'll look forward to using it!

@cppforlife
Copy link
Contributor

@ljfranklin black holes are not bad. just means not configured. i think we should make as little api calls as possible. cc @geofffranks

@geofffranks
Copy link
Contributor Author

I can add the delete feature, and clean up the check for existing routes.

Thanks for the quick feedback!

On Aug 29, 2016, at 1:13 PM, Lyle Franklin notifications@github.com wrote:

@geofffranks https://github.com/geofffranks neat feature! Looks good, my only feedback is seems a little messy to not delete the route when we delete the VM. Do you see any issues with calling delete_route during delete_vm for any routes that have route.instance == instance_to_delete, or is there some reason why a black-hole route is desirable?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #37 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AG2SUtf0ezGyDXIIbijHXhN2RY6WGI6Pks5qkxM6gaJpZM4Jvo6w.

@geofffranks
Copy link
Contributor Author

Just saw @cppforlife 's note about maybe not deleting. Let me know which way you want to go.

@ljfranklin
Copy link
Contributor

@cppforlife @geofffranks If I'm relying on BOSH to create a route for me, my expectation would be for BOSH to also remove that route when that VM goes away. I could imagine an initial setup with a fallback route 0.0.0.0 that goes to a Gateway. You run bosh deploy vpn-box.yml with advertised_instance_routes, all traffic to the given CIDR magically starts getting routed through the VPN box. You later run bosh delete deployment vpn-box.yml and everything magically starts going back through the Gateway. Pretty slick IMO.

@ljfranklin
Copy link
Contributor

After a convo on slack, the consensus is to leave the black hole routes as we don't have access to the cloud_properties during the delete_vm call. Seems sketchy to delete routes that may have been created out-of-band. Story to merge PR is here.

@ljfranklin ljfranklin merged commit fb57651 into cloudfoundry:master Aug 29, 2016
@cppforlife
Copy link
Contributor

@geofffranks Build failed on integration tests due to rubygems flakiness. Restarted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants