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

Ssh key management for softlayer #321

Closed
wants to merge 23 commits into from
Closed

Ssh key management for softlayer #321

wants to merge 23 commits into from

Conversation

@Itxaka
Copy link
Contributor

Itxaka commented Jun 22, 2014

This pull requests add ssh key management for the SoftLayer driver.

Add methods for:
list_key_pairs()
get_key_pair()
import_key_pair_from_string()
delete_key_pair()
create_key_pair()

It also adds a new property to create node "ex_keyname" which is the name of the key to be associated to the new node.

Added tests for all methods and modified the test_create_node() test to use a sshkey as well.

Itxaka and others added 18 commits Jun 21, 2014
…h_key as a feature for the driver, associate the key to the node on create_node if a key name is used, added list_keypairs method to get all keys on the account, add key_name_to_id method to use the id on creating the node by providing the label
…_key_pair get just one key instead of getting all of them and then check the results
…ve/get_one/get_all/import_from_string. Adds tests for all new methods.
Itxaka
…to variable to Fasle instead of True
@@ -188,6 +194,47 @@ def _get_object_mask(self, service, mask):
return {}


class KeyPair(object):

This comment has been minimized.

@Kami

Kami Jun 22, 2014 Member

Key pair management is now part of the base compute API so you should use the existing libcloud.compute.base.KeyPair class.

@Itxaka
Copy link
Contributor Author

Itxaka commented Jun 22, 2014

Fixed the Keypar stuff that @Kami brougth to my attention. Im guessing that the way of storing extra stuff for the Keypairs is to use the extra field, so that's where I stored the ID of the key :D

@Itxaka
Copy link
Contributor Author

Itxaka commented Jul 14, 2014

Any update if this is gonna be merged @Kami ?

@Kami
Copy link
Member

Kami commented Aug 30, 2014

@Itxaka Really sorry for the delay.

I will review it again now and try to get it merged asap.

@@ -411,6 +422,82 @@ def create_node(self, **kwargs):

return self._to_node(raw_node)

def _to_key_pairs(self, elems):

This comment has been minimized.

@Kami

Kami Aug 30, 2014 Member

Minor style thing - for consistency, please move "private" methods to the end of the class after all the "public" ones.

extra={'id': elem['id']})
return key_pair

def _key_name_to_id(self, key):

This comment has been minimized.

@Kami

Kami Aug 30, 2014 Member

Argument should probably be called name since it represents a key name and not a key object.

return key_pairs

def get_key_pair(self, name):
key_id = self._key_name_to_id(name)

This comment has been minimized.

@Kami

Kami Aug 30, 2014 Member

Minor thing - for consistency and explicitness sake, please use keyword arguments (self._key_name_to_id(name=name)).

if crypto is False:
raise NotImplementedError("create_key_pair needs"
" the pycrypto library")
key = RSA.generate(2048)

This comment has been minimized.

@Kami

Kami Aug 30, 2014 Member

We should probably also allow user to specify key size and default to 4096 bits just to be on the safe side.

For example:

def create_key_pair(self, name, ex_size=4096)
# can we create new dependencies?
def create_key_pair(self, name):
if crypto is False:
raise NotImplementedError("create_key_pair needs"

This comment has been minimized.

@Kami

Kami Aug 30, 2014 Member

Minor style thing - for consistency, please use single quotes around strings.

@Kami
Copy link
Member

Kami commented Aug 30, 2014

There are some minor style things which need to be addressed, but besides that it looks good.

I will get it merged into trunk as soon as those comments are addressed.

Thanks!

@Itxaka
Copy link
Contributor Author

Itxaka commented Aug 30, 2014

@Kami thanks for the comments, will try to fix them as soon as possible.

Regarding the sshkey stuff, I still don't know if getting the local generation for ssh keys before uploading its an ok thing and could be implemented across libcloud.

Thing is, some(all?) of the cloud providers allow you to import an ssh key, but for that we need a method to generate one, upload the public part and return to the user the keypair object (public and private) so I believe it would be good if it could be used across all drivers as a new generic method. For automation purpouses it makes too much sense (we are doing it that way internally, on node creation we generate a new key from scratch for each machine, but as libcloud does not support it we are doing it outside of it)

It requires a new dependecy thougth, pycrypto.

Cheers!

Itxaka Serrano added 3 commits Aug 31, 2014
…d parameters, allow user to specify key size
Itxaka Serrano
Itxaka Serrano
@Itxaka
Copy link
Contributor Author

Itxaka commented Aug 31, 2014

@Kami Everything modified to confirm to the proper guidelines and all tests passing :)

@Kami
Copy link
Member

Kami commented Aug 31, 2014

@Itxaka Thanks.

I'm having troubles applying this patch on top of trunk (I would squash the commits myself). Can you please sync your branch with latest trunk and squash all the commits?

In the worst case, if you are also having troubles, just sync your branch with trunk, do git diff --no-prefix trunk > t.diff, create new branch, git apply t.diff and commit all the changes under a single commit.

@Itxaka
Copy link
Contributor Author

Itxaka commented Aug 31, 2014

Having issues, will recreate the branch and a new pull request.

@Itxaka Itxaka closed this Aug 31, 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

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