Merge lp:~xtoddx/nova/disable_creds into lp:~hudson-openstack/nova/trunk

Proposed by Todd Willey
Status: Work in progress
Proposed branch: lp:~xtoddx/nova/disable_creds
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 56 lines (+27/-0)
2 files modified
nova/adminclient.py (+21/-0)
nova/api/ec2/admin.py (+6/-0)
To merge this branch: bzr merge lp:~xtoddx/nova/disable_creds
Reviewer Review Type Date Requested Status
Christopher MacGown (community) Needs Fixing
termie (community) Approve
Thierry Carrez (community) ffe Needs Information
John Dewey (community) Needs Fixing
Review via email: mp+54453@code.launchpad.net

Description of the change

In the case of compromised vpn credentials, we need to be able to terminate the affected vpn and revoke the credentials. This adds that functionality to the ec2 admin api.

To post a comment you must log in.
Revision history for this message
John Dewey (retr0h) wrote :

I do not see tests for adminclient.py, but maybe now is a good time to add them?

I have a hard time approving a feature that lacks tests, even if those before us didn't add them.

review: Needs Fixing
Revision history for this message
Thierry Carrez (ttx) wrote :

We just hit feature freeze, if you want this into Cactus rather than wait for Diablo, could you provide a quick benefit vs. risk of regression rationale, then request a specific FFe review from me ?

Revision history for this message
Thierry Carrez (ttx) :
review: Needs Information (ffe)
Revision history for this message
termie (termie) wrote :

diffline 33: please put StatusResponse on the next line, indented with the other args.

should we be internationalizing the response in the disable project creds call?

also looks like you'll need a newline before the StatusResponse class def.

Other than that, looking good. Other folks's comments still applicable, though I think tests will need to be in smoketests if you write them.

review: Needs Fixing
lp:~xtoddx/nova/disable_creds updated
847. By Todd Willey

merge trunk

848. By Todd Willey

Fix style issues and add i18n.

Revision history for this message
Todd Willey (xtoddx) wrote :

Merged trunk and fixed style.

Decided against i18n as clients may be different language than api server.

lp:~xtoddx/nova/disable_creds updated
849. By Todd Willey

Revert i18n on server-side.

Revision history for this message
termie (termie) wrote :

style-wise looks good, smoketests would be great

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

Looks good to me, though I agree that tests would be lovely.

Revision history for this message
Christopher MacGown (0x44) wrote :

Per the new testing policy decided at the Diablo summit, this needs tests before it can be merged.

review: Needs Fixing
Revision history for this message
Christopher MacGown (0x44) wrote :

> Per the new testing policy decided at the Diablo summit, this needs tests
> before it can be merged.

Otherwise, it looks good.

Unmerged revisions

849. By Todd Willey

Revert i18n on server-side.

848. By Todd Willey

Fix style issues and add i18n.

847. By Todd Willey

merge trunk

846. By Todd Willey

Admin api methods for disabling credentials & terminating vpns.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'nova/adminclient.py'
--- nova/adminclient.py 2011-03-16 04:04:38 +0000
+++ nova/adminclient.py 2011-03-30 00:01:11 +0000
@@ -265,6 +265,21 @@
265 setattr(self, name, str(value))265 setattr(self, name, str(value))
266266
267267
268class StatusResponse(object):
269 def __init__(self, connection=None):
270 self.connection = connection
271 self.status = None
272
273 def __repr__(self):
274 return 'Status:%s' % self.status
275
276 def startElement(self, name, attrs, connection):
277 return None
278
279 def endElement(self, name, value, connection):
280 setattr(self, name, str(value))
281
282
268class NovaAdminClient(object):283class NovaAdminClient(object):
269284
270 def __init__(285 def __init__(
@@ -471,3 +486,9 @@
471 """Grabs the list of all users."""486 """Grabs the list of all users."""
472 return self.apiconn.get_list('DescribeInstanceTypes', {},487 return self.apiconn.get_list('DescribeInstanceTypes', {},
473 [('item', InstanceType)])488 [('item', InstanceType)])
489
490 def disable_project_credentials(self, project):
491 """Revoke project credentials and kill the cloudpipe/vpn instance."""
492 return self.apiconn.get_object('DisableProjectCredentials',
493 {'Project': project},
494 StatusResponse)
474495
=== modified file 'nova/api/ec2/admin.py'
--- nova/api/ec2/admin.py 2011-03-24 18:02:04 +0000
+++ nova/api/ec2/admin.py 2011-03-30 00:01:11 +0000
@@ -23,6 +23,7 @@
23import base6423import base64
24import datetime24import datetime
2525
26from nova import crypto
26from nova import db27from nova import db
27from nova import exception28from nova import exception
28from nova import flags29from nova import flags
@@ -329,3 +330,8 @@
329 def describe_host(self, _context, name, **_kwargs):330 def describe_host(self, _context, name, **_kwargs):
330 """Returns status info for single node."""331 """Returns status info for single node."""
331 return host_dict(db.host_get(name))332 return host_dict(db.host_get(name))
333
334 def disable_project_credentials(self, context, project):
335 """Revoke credentials and stop the vpn instance."""
336 crypto.revoke_certs_by_project(project)
337 return {'status': 'OK', 'message': 'Credentials Revoked'}