Skip to content

Fix various bugs and add "floating IP handler" to the OpenStack provider #301

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 11 commits into from
Closed

Fix various bugs and add "floating IP handler" to the OpenStack provider #301

wants to merge 11 commits into from

Conversation

hcs42
Copy link
Contributor

@hcs42 hcs42 commented May 27, 2014

This commit contains:

  • Fixes of typos in docstrings and comments
  • 7 various small bugfixes
  • a small feature (add "floating IP handler" to the OpenStack provider)
  • a small addition to the DUMMY provider

If I should break up this pull request into several pull requests and/or I should write Jira tickets, just let me know.

hcs42 added 5 commits May 27, 2014 16:31
@hcs42
Fix typos in docstrings
e1ef1b8
@hcs42
Fix typo in comment
7e15c16
@hcs42
Bugfix: fix variable name in EC2 driver
949e351 
"id" is a built-in function in Python, which caused this error to slip through
the unit tests.
@hcs42
Fix import key in EC2 driver
959dc73 
The original code was faulty, because the
ex_find_or_import_keypair_by_key_material function returned a key pair object,
and that object was placed in params['KeyName'], which was later expected to
contain only the key name, not the key pair object.

This fix places only the key name in params['KeyName'].
@hcs42
Fix ec2.import_key_pair_from_string for Python 3
c10cdc6 
In Python 3, the original code didn't work, because base64key was a byte
string, and it became a value in the `params` dictionary, but that needs
strings as values.
Kami
Kami reviewed May 27, 2014
View reviewed changes
libcloud/utils/publickey.py
@@ -16,6 +16,10 @@
import base64
import hashlib

from Crypto.Util.py3compat import bchr
Copy link
Member

Choose a reason for hiding this comment

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

paramiko (and pycrypto) is an optional dependency, but get_pubkey_openssh_fingerprint should also work without those libraries so we can't rely on bchr function from pycrypto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it OK if I add a bchr function to our py3 module?

Copy link
Member

Choose a reason for hiding this comment

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

Yep

hcs42 added 6 commits May 28, 2014 18:54
@hcs42
Fix publickey._to_md5_fingerprint for Python 3
26c33b3 
In Python 3, the original code didn't work, because there is no `encode`
function for binaries:

    (Python3.3)>>> [x.encode("hex") for x in b"abc"]
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<stdin>", line 1, in <listcomp>
    AttributeError: 'int' object has no attribute 'encode'

String do have an `encode` method, but it is a different function from the
`encode` in Python 2:

    (Python3.3)>>> [x.encode("hex") for x in "abc"]
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<stdin>", line 1, in <listcomp>
    LookupError: unknown encoding: hex

So I added a "hexadigits" function to our compatibility module (py3) and used
that in _to_md5_fingerprint.
@hcs42
Fix publickey.get_pubkey_ssh2_fingerprint for Python 3
c3d499d 
In Python 3, the original code didn't work, because there we cannot concatenate
a string and a byte string.

The new code produces two binaries in Python 3 and two string in Python 2, so
they can be concatenated in both versions.
@hcs42
ec2 driver: Make cidr_ips argument mandatory
21ec53d 
If the cidr_ips parameter in this function is None, and therefore the
IpPermissions.1.IpRanges.1.CidrIp is not present in the request to the EC2 API,
the request is not performed. (The response does not indicate an error, but the
security rule is not created).

This behaviour is not documented in
http://docs.aws.amazon.com/AWSEC2/latest/APIReference/ApiReference-query-AuthorizeSecurityGroupIngress.html,
but it is reproducable.

The following request didn't work:

    Action=AuthorizeSecurityGroupIngress
    GroupId=sg-a4b44ece
    IpPermissions.1.FromPort=22
    IpPermissions.1.IpProtocol=tcp
    IpPermissions.1.ToPort=22
    AWSAccessKeyId=...
    Signature=...
    SignatureMethod=HmacSHA256
    SignatureVersion=2
    Timestamp=2014-04-16T07%3A43%3A21Z
    Version=2013-10-15

While this one did (the only difference is adding CidrIp):

    Action=AuthorizeSecurityGroupIngress
    GroupId=sg-a4b44ece
    IpPermissions.1.FromPort=22
    IpPermissions.1.IpProtocol=tcp
    IpPermissions.1.ToPort=22
    IpPermissions.1.IpRanges.1.CidrIp=0.0.0.0/0
    AWSAccessKeyId=...
    Signature=...
    SignatureMethod=HmacSHA256
    SignatureVersion=2
    Timestamp=2014-04-16T07%3A43%3A21Z
    Version=2013-10-15
@hcs42
OpenStack/delete security group: Accept ACCEPTED
002501e 
According to https://wiki.openstack.org/wiki/Os-security-groups#Delete_Security_Group
and my own experiments with the HP cloud, the provider's answer upon success is
not NO_CONTENT (HTTP status code 204) but ACCEPTED (HTTP status code 202).

So I modified to code to accept ACCEPTED. I still kept NO_CONTENT (I would
think it was put there because some provider did return NO_CONTENT).
@hcs42
Add "floating IP" functions to the OpenStack provider
611f284 
The Compute API of the HP Cloud used by Libcloud [1] does not support floating IP
pools, but it does support floating IPs. (To be more precise, one needs to use
floating IPs if he wants to access the instance from outside the cloud.)

So I added floating ip handling methods to the OpenStack_1_1_NodeDriver class.

[1] HP Helion Public Cloud v13.5 Compute Service:
https://docs.hpcloud.com/api/v13/compute/#3.10Extensions
@hcs42
Add import_key_pair_from_string to DummyNodeDriver
f5723fe
@hcs42
Copy link
Contributor Author

hcs42 commented May 28, 2014

I updated three commits:

  • "Fix publickey._to_md5_fingerprint for Python 3": added a unit test.

  • "Fix publickey.get_pubkey_ssh2_fingerprint for Python 3": use our own py3.bchr function.

  • "Add "floating IP" functions to the OpenStack provider": instead of using a handler, I added IP address handler methods directory to the driver class. I used the method name you mentioned, I don't find it's confusing.

    I had to add a "driver" parameter to the OpenStack_1_1_FloatingIpAddress class so that the object can use it to delete itself. I didn't want to use the "pool" object for this, I felt that that would be more confusing. Suggestions about better solutions are welcome.

@asfgit asfgit closed this in 21a0d06 May 28, 2014
@Kami
Copy link
Member

Kami commented May 28, 2014

I've squashed the commits, fixed some lint issues and merged your changes into trunk.

Thanks!

@Kami
Copy link
Member

Kami commented May 29, 2014

@hcs42 Btw, I know this has already been merged, but what do you think about this approach - https://issues.apache.org/jira/browse/LIBCLOUD-567?focusedCommentId=14012135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14012135 ?

@hcs42
Copy link
Contributor Author

hcs42 commented May 29, 2014

Thanks! I posted my opinion in the JIRA ticket.

hcs42 added a commit to esl/elibcloud that referenced this pull request May 29, 2014
@hcs42
Use the modified floating IP handling of Libcloud
4f8047c 
The following PR shows that after I proposed my solution for the problem of
using floating IP addressed with ex_create_floating_ip_handler, in the end we
went for a different solution (ex_create_floating_ip):
apache/libcloud#301

This commit changes elibcloud accordingly.
@zerthimon zerthimon mentioned this pull request May 29, 2014
Closed
dahlia pushed a commit to geofront-auth/geofront that referenced this pull request Jul 4, 2014
Force to upgrade apache-libcloud to 0.15.0
7f33f8a 
apache-libcloud 0.15.0 fixed some Python 3 compatibility bugs which
might critical:

apache/libcloud#301
https://libcloud.readthedocs.org/en/latest/changelog.html#changes-with-apache-libcloud-0-15-0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hcs42 @Kami