Merge lp:~rackspace-titan/nova/change-password-v1-1 into lp:~hudson-openstack/nova/trunk

Proposed by Mark Washenberger
Status: Merged
Approved by: Matt Dietz
Approved revision: 821
Merged at revision: 929
Proposed branch: lp:~rackspace-titan/nova/change-password-v1-1
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 129 lines (+89/-4)
2 files modified
nova/api/openstack/servers.py (+21/-4)
nova/tests/api/openstack/test_servers.py (+68/-0)
To merge this branch: bzr merge lp:~rackspace-titan/nova/change-password-v1-1
Reviewer Review Type Date Requested Status
Matt Dietz (community) Approve
termie (community) Approve
Brian Waldon (community) Approve
Review via email: mp+54917@code.launchpad.net

Commit message

Add a change password action to /servers in openstack api v1.1, and associated tests.

Description of the change

Add a change password action to /servers in openstack api v1.1, and associated tests.

To post a comment you must log in.
Revision history for this message
Brian Waldon (bcwaldon) wrote :

What is the expected behavior if an empty adminPass is provided?

review: Needs Information
Revision history for this message
termie (termie) wrote :

- please correct the indentation on the other items in the actions dictionary so that they don't have extra whitespace (I know you didn't modify them but you are nearby in the file)
- at line 28 please use parens to encapsulate the multiline statement instead of escaping the newline... for example:

if (foo == really_long_name
    and bar == really_long_name):
    ...do some stuff...

- you've got some extra newlines around 57 and 59

review: Needs Fixing
Revision history for this message
Mark Washenberger (markwash) wrote :

Thanks for the reviews!

Waldon,

Thanks for catching that. The answer now is that if the password string is empty, we return 400. Also, if the password is None or something other than a json string, we return 400.

Termie,

I believe I have addressed your concerns. I wasn't sure exactly what your third note regarding newlines would have me do, but I fiddled with it a little bit anyway :-). Let me know if its up to the standards, and if not, I'm happy to change it.

Thanks again

Revision history for this message
Brian Waldon (bcwaldon) wrote :

Excellent work! One last (non-emergency) comment:

37 if (not 'changePassword' in input_dict

This check doesn't seem necessary within the change_password method. It is already guaranteed to be true.

review: Approve
Revision history for this message
Mark Washenberger (markwash) wrote :

I know that checking 'changePassword' is a bit overly defensive. However, I am copying the standard laid down in _action_resize.

What I would like to see is the action handler not pass the whole input_dict, just the value associated with the particular action. Then there would be no need to check, and no need to worry about referencing keys that might not exist.

Cool?

Revision history for this message
Brian Waldon (bcwaldon) wrote :

> I know that checking 'changePassword' is a bit overly defensive. However, I am
> copying the standard laid down in _action_resize.
>
> What I would like to see is the action handler not pass the whole input_dict,
> just the value associated with the particular action. Then there would be no
> need to check, and no need to worry about referencing keys that might not
> exist.
>
> Cool?

Agreed.

Revision history for this message
termie (termie) wrote :

looks great :) only one little thing which is that it looks like your mock class can live outside of that function, it doesn't seem to have any specific requirements to be defined in the test so seems like you may as well make it a top-level item.

Revision history for this message
termie (termie) wrote :

per discussion seems fine to leave it in there as it seems unlikely to be applicable outside that test

review: Approve
821. By Mark Washenberger

merge lp:nova

Revision history for this message
Matt Dietz (cerberus) wrote :

Tests look good. Seems good to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/api/openstack/servers.py'
2--- nova/api/openstack/servers.py 2011-03-30 15:10:37 +0000
3+++ nova/api/openstack/servers.py 2011-03-31 19:53:30 +0000
4@@ -288,11 +288,12 @@
5 resize a server"""
6
7 actions = {
8- 'reboot': self._action_reboot,
9- 'resize': self._action_resize,
10+ 'changePassword': self._action_change_password,
11+ 'reboot': self._action_reboot,
12+ 'resize': self._action_resize,
13 'confirmResize': self._action_confirm_resize,
14- 'revertResize': self._action_revert_resize,
15- 'rebuild': self._action_rebuild,
16+ 'revertResize': self._action_revert_resize,
17+ 'rebuild': self._action_rebuild,
18 }
19
20 input_dict = self._deserialize(req.body, req.get_content_type())
21@@ -301,6 +302,9 @@
22 return actions[key](input_dict, req, id)
23 return faults.Fault(exc.HTTPNotImplemented())
24
25+ def _action_change_password(self, input_dict, req, id):
26+ return exc.HTTPNotImplemented()
27+
28 def _action_confirm_resize(self, input_dict, req, id):
29 try:
30 self.compute_api.confirm_resize(req.environ['nova.context'], id)
31@@ -629,6 +633,19 @@
32 def _get_addresses_view_builder(self, req):
33 return nova.api.openstack.views.addresses.ViewBuilderV11(req)
34
35+ def _action_change_password(self, input_dict, req, id):
36+ context = req.environ['nova.context']
37+ if (not 'changePassword' in input_dict
38+ or not 'adminPass' in input_dict['changePassword']):
39+ msg = _("No adminPass was specified")
40+ return exc.HTTPBadRequest(msg)
41+ password = input_dict['changePassword']['adminPass']
42+ if not isinstance(password, basestring) or password == '':
43+ msg = _("Invalid adminPass")
44+ return exc.HTTPBadRequest(msg)
45+ self.compute_api.set_admin_password(context, id, password)
46+ return exc.HTTPAccepted()
47+
48 def _limit_items(self, items, req):
49 return common.limited_by_marker(items, req)
50
51
52=== modified file 'nova/tests/api/openstack/test_servers.py'
53--- nova/tests/api/openstack/test_servers.py 2011-03-30 15:10:37 +0000
54+++ nova/tests/api/openstack/test_servers.py 2011-03-31 19:53:30 +0000
55@@ -764,6 +764,74 @@
56 res = req.get_response(fakes.wsgi_app())
57 self.assertEqual(res.status_int, 404)
58
59+ def test_server_change_password(self):
60+ body = {'changePassword': {'adminPass': '1234pass'}}
61+ req = webob.Request.blank('/v1.0/servers/1/action')
62+ req.method = 'POST'
63+ req.content_type = 'application/json'
64+ req.body = json.dumps(body)
65+ res = req.get_response(fakes.wsgi_app())
66+ self.assertEqual(res.status_int, 501)
67+
68+ def test_server_change_password_v1_1(self):
69+
70+ class MockSetAdminPassword(object):
71+ def __init__(self):
72+ self.instance_id = None
73+ self.password = None
74+
75+ def __call__(self, context, instance_id, password):
76+ self.instance_id = instance_id
77+ self.password = password
78+
79+ mock_method = MockSetAdminPassword()
80+ self.stubs.Set(nova.compute.api.API, 'set_admin_password', mock_method)
81+ body = {'changePassword': {'adminPass': '1234pass'}}
82+ req = webob.Request.blank('/v1.1/servers/1/action')
83+ req.method = 'POST'
84+ req.content_type = 'application/json'
85+ req.body = json.dumps(body)
86+ res = req.get_response(fakes.wsgi_app())
87+ self.assertEqual(res.status_int, 202)
88+ self.assertEqual(mock_method.instance_id, '1')
89+ self.assertEqual(mock_method.password, '1234pass')
90+
91+ def test_server_change_password_bad_request_v1_1(self):
92+ body = {'changePassword': {'pass': '12345'}}
93+ req = webob.Request.blank('/v1.1/servers/1/action')
94+ req.method = 'POST'
95+ req.content_type = 'application/json'
96+ req.body = json.dumps(body)
97+ res = req.get_response(fakes.wsgi_app())
98+ self.assertEqual(res.status_int, 400)
99+
100+ def test_server_change_password_empty_string_v1_1(self):
101+ body = {'changePassword': {'adminPass': ''}}
102+ req = webob.Request.blank('/v1.1/servers/1/action')
103+ req.method = 'POST'
104+ req.content_type = 'application/json'
105+ req.body = json.dumps(body)
106+ res = req.get_response(fakes.wsgi_app())
107+ self.assertEqual(res.status_int, 400)
108+
109+ def test_server_change_password_none_v1_1(self):
110+ body = {'changePassword': {'adminPass': None}}
111+ req = webob.Request.blank('/v1.1/servers/1/action')
112+ req.method = 'POST'
113+ req.content_type = 'application/json'
114+ req.body = json.dumps(body)
115+ res = req.get_response(fakes.wsgi_app())
116+ self.assertEqual(res.status_int, 400)
117+
118+ def test_server_change_password_not_a_string_v1_1(self):
119+ body = {'changePassword': {'adminPass': 1234}}
120+ req = webob.Request.blank('/v1.1/servers/1/action')
121+ req.method = 'POST'
122+ req.content_type = 'application/json'
123+ req.body = json.dumps(body)
124+ res = req.get_response(fakes.wsgi_app())
125+ self.assertEqual(res.status_int, 400)
126+
127 def test_server_reboot(self):
128 body = dict(server=dict(
129 name='server_test', imageId=2, flavorId=2, metadata={},