Skip to content

Conversation

jclong83
Copy link

@jclong83 jclong83 commented Jan 9, 2014

In lib/OpenCloud/Compute/Resource/Server.php the keypair['publicKey'] check (line 641) was incorrectly failing. Keys returned by compute->listKeypairs() do not have a "publicKey" element, but rather "public_key". Correction on line 644 is just to make the print function output match reality as well.

Lines 647-650 incorrectly cause createJson() to add a "keypair" array to the "server" json payload that is sent to nova API to create a server. At least in Rackspace's deployment, this leads to a server being created but no key being placed on the new instance. Example (incorrect at least at Rackspace) json payload made by function as-is:

{"server":{"name":"jamesl1","imageRef":"857d7d36-34f3-409f-8435-693e8797be8b","flavorRef":"2","metadata":{"sdk":"OpenCloud/1.7.0 cURL/7.34.0 PHP/5.5.7-pl0-gentoo"},"networks":[{"uuid":"00000000-0000-0000-0000-000000000000"},{"uuid":"11111111-1111-1111-1111-111111111111"}],"keypair":{"key_name":"jamesl"}}}

The updated line 647 correctly adds "key_name" as a direct element of "server", causing createJson() to make a json payload that is both accepted by Rackspace's nova API and results in the key being placed on the instance. Example (correct at least at Rackspace) json payload made by modified line:

{"server":{"name":"jamesl1","imageRef":"857d7d36-34f3-409f-8435-693e8797be8b","flavorRef":"2","metadata":{"sdk":"OpenCloud/1.7.0 cURL/7.34.0 PHP/5.5.7-pl0-gentoo"},"networks":[{"uuid":"00000000-0000-0000-0000-000000000000"},{"uuid":"11111111-1111-1111-1111-111111111111"}],"key_name":"jamesl"}}

When performing the unit tests, I found that tests/OpenCloud/Tests/Compute/Resource/ServerTest.php also needed to be updated. It also had "publicKey" rather than "public_key" as the keypair key's attribute name. Corrected that and unit tests pass.

Changes to lib/OpenCloud/Common/Collection/PaginatedIterator.php that are included by github in this pull request were not from me.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling ce52902 on jclong83:master into 33d1598 on rackspace:working.

@jamiehannaford
Copy link
Contributor

@jclong83 This is a great catch, thanks for submitting a PR. I had to add a few other conditional statements to cater for backwards compatibility, so I've manually added in your changes along with a few other pieces of code. If it wasn't for this additional stuff, I would have merged this PR. Thanks for your help and feel free to open up more PRs in the future! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants