If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo #624

c-mart · 2018-06-12T20:34:13Z

Description

Problem: When creating a floating IP address and associating it to an instance, rtwo chooses an arbitrary external network (the first one that happens to be returned from the API). If there are multiple extrenal networks, rtwo may select a network which is unreachable from the user's private network (no router connects them).

Solution: When using external router topology, determine the correct external network (based on the user's assigned public router) and explicitly specity it when creating a floating IP address.

Co-depends on rtwo #27

Checklist before merging Pull Requests

~~New test(s) included to reproduce the bug/verify the feature~~
Add an entry in the changelog
~~Documentation created/updated (include links)~~
~~If creating/modifying DB models which will contain secrets or sensitive information, PR to clank updating sanitation queries in roles/sanitary-sql-access/templates/sanitize-dump.sh.j2~~
Reviewed and approved by at least one other contributor.
~~New variables supported in Clank~~
~~New variables committed to secrets repos~~

coveralls · 2018-06-12T20:45:36Z

Coverage decreased (-0.02%) to 37.341% when pulling 45aa3c4 on c-mart:handle-multiple-external-networks into 905f8e0 on cyverse:v33.

c-mart · 2018-06-13T22:08:05Z

Tested on atmobeta with only one external network, and with multiple external networks; removing WIP label.

cdosborn · 2018-06-15T16:56:27Z

service/tasks/driver.py

+                    == "External Router Topology":
+                # Determine correct external network based on external gateway
+                # info of the identity's public router
+                public_router_name = identity.credentials['router_name']


Should be identity.get_credential('router_name')

rtwo identity objects don't have a .get_credential

I confused identity with core_identity. The credentials object actually exists in the core_identity model, the rtwo identity's credentials is just a copy, so core_identity.get_credential('router_name'). I do not know why the rtwo Identity class exists.

cdosborn · 2018-06-15T16:57:20Z

service/tasks/driver.py

+                floating_ip = \
+                    network_driver.associate_floating_ip(instance_alias)
+            floating_ip_addr = \
+                floating_ip["floating_ip_address"]


I think we should instead store the external_network in the identity's credentials. add_floating_ip doesn't need to be responsible for looking this up every time we add a floating.
Could be something like:

associate_args = (instance_alias,) if core_identity.provider.cloud_config['network']['topology'] \ == "External Router Topology": external_network_id = identity.get_credential('external_network_id') associate_args = (instance_alias, external_network_id) floating_ip = network_driver.associate_floating_ip(*associate_args)

In that case, what do you think should be responsible for looking up the appropriate external network for an identity's router, and setting that in the identity's credentials? Should that be done when an identity is created? If so, we'd also need some on-demand process to set external networks for existing identities.

What if the above logic was moved to some helper function (e.g. get_external_network) that was called by add_floating_ip -- do you think that would suffice?

I'll take a look at this today.

I'm okay with this change, there are larger fish to fry.

cdosborn · 2018-06-21T20:04:41Z

service/tasks/driver.py

+                    if router['name'] == public_router_name:
+                        public_router = router
+                if not public_router:
+                    raise Exception("Could not find a router matching"


Should include router_name, and username in this exception for the traceback to be more valuable

cdosborn · 2018-06-21T21:51:00Z

service/tasks/driver.py

+                        public_router = router
+                if not public_router:
+                    raise Exception("Could not find a router matching"
+                                    " public_router name {} for user {}"


I made a small change to remove the specific numeric reference inside each {} which is only primarily useful when you want them to appear in a different order than listed, or more than once.

…when adding floating IP and explicitly pass it to rtwo

…ram to associate_floating_ip

cdosborn · 2018-06-22T22:00:01Z

service/tasks/driver.py

+                    == "External Router Topology":
+                # Determine correct external network based on external gateway
+                # info of the identity's public router
+                public_router_name = core_identity.get_credential('router_name')


I meant to say core_identity earlier in my previous comment. If you do a quick search in the codebase the credentials object is never accessed off an rtwo identity, its a copy of the credental set model on the core identity.

cdosborn

I realized just before merging, that rtwo was never upgraded to the necessary version. I tacked on a commit to do this. Can you test in a way that requires the proper version of rtwo to be loaded? Does jtc have more than one external network?

c-mart · 2018-06-25T16:08:44Z

@cdosborn before testing, I manually patched rtwo with the version that accepts an external network ID in associate_floating_ip(). And yes, JTC currently has two external networks/routers.

c-mart · 2018-06-25T17:36:53Z

You can try it out now on JTC if you wish :)

cdosborn · 2018-06-25T19:28:27Z

Right, obviously you tested with the right rtwo version, or this wouldn't have worked. LTGM

c-mart changed the title ~~If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo~~ WIP: If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo Jun 12, 2018

c-mart mentioned this pull request Jun 12, 2018

Allow external network to be passed to associate_floating_ip cyverse/rtwo#27

Merged

5 tasks

c-mart changed the title ~~WIP: If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo~~ If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo Jun 13, 2018

cdosborn self-assigned this Jun 13, 2018

cdosborn suggested changes Jun 15, 2018

View reviewed changes

cdosborn reviewed Jun 21, 2018

View reviewed changes

cdosborn force-pushed the handle-multiple-external-networks branch from c851263 to 970dc4e Compare June 21, 2018 21:23

cdosborn changed the base branch from master to v33 June 21, 2018 21:24

cdosborn reviewed Jun 21, 2018

View reviewed changes

cdosborn force-pushed the handle-multiple-external-networks branch from 076ba45 to 7c6370d Compare June 22, 2018 21:40

cmart added 2 commits June 22, 2018 14:43

If using External Router topology then determine external network ID …

c2d2a07

…when adding floating IP and explicitly pass it to rtwo

changelog entry

25168db

cdosborn force-pushed the handle-multiple-external-networks branch from 7c6370d to 076ba45 Compare June 22, 2018 21:46

Leverage rtwo(0.5.20) with support for passing external_network_id pa…

45aa3c4

…ram to associate_floating_ip

cdosborn force-pushed the handle-multiple-external-networks branch from 076ba45 to 25168db Compare June 22, 2018 21:58

cdosborn reviewed Jun 22, 2018

View reviewed changes

cdosborn approved these changes Jun 22, 2018

View reviewed changes

cdosborn suggested changes Jun 22, 2018

View reviewed changes

cdosborn approved these changes Jun 25, 2018

View reviewed changes

cdosborn merged commit 6bc9953 into cyverse:v33 Jun 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo #624

If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo #624

c-mart commented Jun 12, 2018 •

edited by cdosborn

Loading

coveralls commented Jun 12, 2018 •

edited

Loading

c-mart commented Jun 13, 2018

cdosborn Jun 15, 2018

c-mart Jun 22, 2018

cdosborn Jun 22, 2018

cdosborn Jun 15, 2018

c-mart Jun 18, 2018

cdosborn Jun 21, 2018

cdosborn Jun 21, 2018

cdosborn Jun 21, 2018

cdosborn Jun 21, 2018

cdosborn Jun 22, 2018

cdosborn left a comment •

edited

Loading

c-mart commented Jun 25, 2018

c-mart commented Jun 25, 2018

cdosborn commented Jun 25, 2018

If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo #624

If using External Router topology then determine external network ID when adding floating IP and explicitly pass it to rtwo #624

Conversation

c-mart commented Jun 12, 2018 • edited by cdosborn Loading

Description

Checklist before merging Pull Requests

coveralls commented Jun 12, 2018 • edited Loading

c-mart commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdosborn left a comment • edited Loading

Choose a reason for hiding this comment

c-mart commented Jun 25, 2018

c-mart commented Jun 25, 2018

cdosborn commented Jun 25, 2018

c-mart commented Jun 12, 2018 •

edited by cdosborn

Loading

coveralls commented Jun 12, 2018 •

edited

Loading

cdosborn left a comment •

edited

Loading