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

LIBCLOUD-470: Add VPC Elastic IP Support #208

Closed
wants to merge 7 commits into from
Closed

Conversation

@cderamus
Copy link
Contributor

cderamus commented Dec 26, 2013

No description provided.

@@ -961,6 +961,61 @@ def _to_network(self, element):

return EC2Network(vpc_id, name, cidr_block, extra=extra)

def _to_addresses(self, response, only_allocated, all_properties):

This comment has been minimized.

Copy link
@Kami

Kami Dec 27, 2013

Member

Please add a docstring which describes what only_allocated and all_properties arguments do.

"""
params = {'Action': 'AllocateAddress'}
params = {'Action': 'AllocateAddress', 'Domain': domain}

This comment has been minimized.

Copy link
@Kami

Kami Dec 27, 2013

Member

Hm, documentation (http://docs.aws.amazon.com/AWSEC2/latest/APIReference/ApiReference-query-AllocateAddress.html) says that the only supported value for Domain query parameter is vpc, but you are also letting standard through.

Unless I'm missing something, you should only include Domain in the params if domain == vpc.

Edit: updated the comment.

:return: A string representation of the association ID which is
required for VPC disassociation. EC2/standard
addresses return None
:rtype: ``NoneType`` or ``str``

This comment has been minimized.

Copy link
@Kami

Kami Dec 27, 2013

Member

Should be None iirc

return self.ex_associate_address_with_node(node=node,
elastic_ip_address=
elastic_ip_address)
if elastic_ip_address is not None:

This comment has been minimized.

Copy link
@Kami

Kami Dec 27, 2013

Member

I've noticed you use two different styles for the same check in the code.

Once you compare it to None (is not None) and the other time you just check if it's truthy (if foo). It would be good if you could use a consistent style.

I usually prefer to be explicit and use is not None check, but a valid value in this case is always truthy (0, empty list, None, etc. are not valid values for this argument) so you can get away with just checking if the argument is truthy.

This comment has been minimized.

Copy link
@cderamus

cderamus Dec 27, 2013

Author Contributor

Noted. I almost always use is not None myself across all of my code and will try to remain consistent for this project.

return association_id

def ex_associate_addresses(self, node, elastic_ip_address=None,
allocation_id=None):
"""

This comment has been minimized.

Copy link
@Kami

Kami Dec 27, 2013

Member

This method won't work in the same way as ex_associate_address_with_node, because it only uses the first argument if user specifies both.

You should do:

return self.ex_associate_address_with_node(node=node,
                                                    elastic_ip_address=
                                                    elastic_ip_address,
                                                    allocation_id=allocation_id)

Or alternatively, if those arguments are mutually exclusive, you should throw.

This comment has been minimized.

Copy link
@cderamus

cderamus Dec 27, 2013

Author Contributor

They are indeed mutually exclusive and Amazon will throw an exception if you try to pass both as parameters. I'll go ahead and throw.

…e passed in

Added docstring for to_addresses method
Only include the domain parameter if it equals vpc
@cderamus
Copy link
Contributor Author

cderamus commented Dec 27, 2013

I went ahead and added the raise condition into ex_associate_address_with_node so it gets thrown should the user use that method or the deprecated ex_associate_address method.

"""
params = {'Action': 'DisassociateAddress'}

params.update({'PublicIp': elastic_ip_address})
if elastic_ip_address:

This comment has been minimized.

Copy link
@Kami

Kami Dec 27, 2013

Member

You should also throw here if both arguments are specified, right?

Please also add a test case for this.

This comment has been minimized.

Copy link
@cderamus

cderamus Dec 27, 2013

Author Contributor

Right you are. I will take care of that later this morning.
On Dec 27, 2013 8:59 AM, "Tomaz Muraus" notifications@github.com wrote:

In libcloud/compute/drivers/ec2.py:

     """
     params = {'Action': 'DisassociateAddress'}
  •    params.update({'PublicIp': elastic_ip_address})
    
  •    if elastic_ip_address:
    

You should also throw here if both arguments are specified, right?

Please also add a test case for this.


Reply to this email directly or view it on GitHubhttps://github.com//pull/208/files#r8567553
.

Added tests for the raise conditions
@Jc2k
Copy link
Contributor

Jc2k commented Dec 28, 2013

My reading of this is that only_allocated shows addresses that are allocated, but not associated. Have I misunderstood? If not, would only_associated be a better name?

@Jc2k
Copy link
Contributor

Jc2k commented Dec 28, 2013

Also I wonder if allocation_id should be encapsulated within an ElasticIP class, so instead of passing elastic_ip_address AND allocation_id, you pass a single object.

(This PR has caused me to look at the various elastic IP API's we have for other providers - bright box, cloudstack, openstack, haven't looked further yet - and i'm going to try and propose a 'standard' API for the feature. Hiding allocation_id would be needed to do that).

@cderamus
Copy link
Contributor Author

cderamus commented Dec 28, 2013

Yes, only associated would likely be a better name for this and certainly comes with less ambiguity.

@cderamus
Copy link
Contributor Author

cderamus commented Dec 28, 2013

Given that virtually all cloud vendors have some form of public/floating/elastic IP addresses I am all for creating a standard API for this with a class (eg: NodeAddress). I do wish Amazon wouldn't require the allocation ID for VPC related actions as it would certainly make this a bit easier.

@cderamus
Copy link
Contributor Author

cderamus commented Jan 9, 2014

Just checking in to see what direction you guys would like to go to support this functionality. If @Jc2k is indeed going to propose promoting this to the base API then I'll go ahead and close this out.

@Kami
Copy link
Member

Kami commented Jan 9, 2014

@cderamus If a standard API promotion is going to take a while, I'm OK with merging this into trunk if you can update the code to use ElasticIP class as suggested by @Jc2k.

@Jc2k
Copy link
Contributor

Jc2k commented Jan 9, 2014

+1. Hope to look at core API soon, but having this merged suits me.

@Kami
Copy link
Member

Kami commented Jan 9, 2014

@cderamus Also, I don't want to rush you, but the whole 0.14.0 release has already been taking longer than usual and I would like to start a voting thread for it soon and it would be great if we could include this change in it.

@cderamus
Copy link
Contributor Author

cderamus commented Jan 9, 2014

@Kami I live for pressure! I'm planning on dedicating a few hours to the project this evening and will bump this to the top of my list. I should have no issues getting this shored up.

@Kami
Copy link
Member

Kami commented Jan 9, 2014

@cderamus heh, great! :)

…PC_Elastic_IP_Support

Conflicts:
	libcloud/compute/drivers/ec2.py
@cderamus
Copy link
Contributor Author

cderamus commented Jan 10, 2014

Sorry about the bad commit message on this one guys. I went ahead and added a new ElasticIp class into the driver and put all VPC specific attributes into the extra dict. As I was making the change I couldn't help but think that the all_properties boolean is sort of silly now that we're using a class.

@cderamus
Copy link
Contributor Author

cderamus commented Jan 10, 2014

I'll be submitting a follow-up with some additional changes momentarily.

…class for both method arguments and return objects. This will make things much easier when promoting ElasticIp/FloatingIp to the base API in the future. All address tests were also updated to reflect this change.
@cderamus
Copy link
Contributor Author

cderamus commented Jan 10, 2014

@Jc2k I believe this is what you wanted. Leveraging the ElasticIp instance required a new keyword argument 'domain' which is only used for VPC actions. If anything other than vpc is passed in the class will throw. Thanks for the suggestions as this was definitely a better approach.

"""
params = {'Action': 'ReleaseAddress'}

params.update({'PublicIp': elastic_ip_address})
if domain is not None and domain is not 'vpc':

This comment has been minimized.

Copy link
@Kami

Kami Jan 10, 2014

Member

Second check won't work everywhere.

is compares identifies (e.g. if two variables point to the same objects) which means you should only use it for immutable types (True, False, None, ...) otherwise you might get an unexpected result.

When you are comparing values you should use == or != in this case.

This is also the reason why PyPy tests are currently failing.

This comment has been minimized.

Copy link
@cderamus

cderamus Jan 10, 2014

Author Contributor

Aye, will make that change now. Thanks!

dict to make promotion to the base API easier.
"""

def __init__(self, ip, domain, instance_id, extra=None):

This comment has been minimized.

Copy link
@Kami

Kami Jan 10, 2014

Member

Can you please add some docstrings which document what each attribute represents.

This comment has been minimized.

Copy link
@cderamus

cderamus Jan 10, 2014

Author Contributor

Absolutely.

return None

# If all properties are not requested, only send back the IP
if not all_properties:

This comment has been minimized.

Copy link
@Kami

Kami Jan 10, 2014

Member

It looks like now that we use ElasticIP abstraction there is not much more sense in having all_properties arguments anymore so we can get rid of it.

This comment has been minimized.

Copy link
@cderamus

cderamus Jan 10, 2014

Author Contributor

Fantastic that's what I was thinking as well.

…he migration to the ElasticIp abstraction. Docstring information was added for the ElasticIp class and the unit tests were updated accordingly. The domain conditional test which checks to validate that the value is 'vpc' was also updated.
% (self.id, self.name))


class ElasticIp(object):

This comment has been minimized.

Copy link
@Kami

Kami Jan 10, 2014

Member

Hm, I just checked the code and it seems like there is an even split between Ip and IP.

I prefer IP (ElasticIP) over Ip since it's an abbreviation for Internet Protocol. What do other ppl think?

kami /w/lc/libcloud/libcloud (git:trunk)$ grep -r IP . | grep class
./test/loadbalancer/test_rackspace.py:class RackspaceLBWithVIPMockHttp(MockHttpTestCase):
./common/gandi.py:class IPAddress(BaseObject):
./compute/drivers/cloudstack.py:class CloudStackIPForwardingRule(object):

kami /w/lc/libcloud/libcloud (git:trunk)$ grep -r Ip . | grep class
./common/gogrid.py:class GoGridIpAddress(object):
./compute/drivers/openstack.py:class OpenStack_1_0_SharedIpGroup(object):
./compute/drivers/openstack.py:class OpenStack_1_0_NodeIpAddresses(object):
./compute/drivers/openstack.py:class OpenStack_1_1_FloatingIpPool(object):
./compute/drivers/openstack.py:class OpenStack_1_1_FloatingIpAddress(object):
@cderamus
Copy link
Contributor Author

cderamus commented Jan 10, 2014

+1 for IP

@Kami
Copy link
Member

Kami commented Jan 10, 2014

@cderamus Can you please make this change, sync your branch and squash the commits?

I'm having problems (conflicts) doing this myself.

@cderamus
Copy link
Contributor Author

cderamus commented Jan 10, 2014

Sure I'll take care of that shortly.

… including the addition of a new ElasticIP abstraction that will make future promotion to the base API much easier.
@cderamus
Copy link
Contributor Author

cderamus commented Jan 11, 2014

@Kami I am also having issues when attempting to squash the commits. Want me to just close this PR and open up a new one with a clean fork? That may be easier at this point.

@Kami
Copy link
Member

Kami commented Jan 11, 2014

@cderamus Yeah, if it makes it less painful than resolving the conflicts, then go for it.

@cderamus cderamus closed this Jan 11, 2014
@cderamus cderamus deleted the DivvyCloud:LIBCLOUD-470_Add_VPC_Elastic_IP_Support branch Jan 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.