Add GCE Cloud - Issue #78 #82
Conversation
nuwang
left a comment
nuwang
left a comment
There was a problem hiding this comment.
@machristie Thanks, it all looks great. I just have some minor suggestions.
| config) | ||
| elif isinstance(cloud, models.GCE): | ||
| config = { | ||
| 'gce_client_email': cred_dict['client_email'], |
There was a problem hiding this comment.
@machristie Looking at this, it looks like gce_client_email and gce_project_name are stored redundantly in gce_service_creds_dict. Should we just change the gce provider interface to only use the gce_service_creds_dict?
There was a problem hiding this comment.
I agree, this was also my question in issue 45. If you're fine with it I can go ahead and make the change.
There was a problem hiding this comment.
I've made this change.
| user_data['secret_key'] = ec2_creds.secret | ||
|
|
||
| if hasattr(cloud, 'azure'): | ||
| elif hasattr(cloud, 'azure'): |
There was a problem hiding this comment.
@machristie Thanks for catching this Marcus. I've made a mistake in merging this in. This version of cloudman only supports EC2 clouds, so the original error message is correct. Would you mind deleting the else clauses for both azure and gce and restoring the original error message?
| else: | ||
| return {} | ||
| elif isinstance(cloud, models.GCE): | ||
| gce_credentials_json = request.META.get('HTTP_CL_GCE_CREDENTIALS_JSON') |
There was a problem hiding this comment.
@machristie So I was looking at this a bit. Since an HTTP header can't have any carriage returns or line feeds, I was wondering whether this might pose a problem. However, it looks like JSON data in a header is ok, provided that the json encoder makes sure there are no CR/LFs, which is probably doable. I guess we can test this once the corresponding CloudLaunch UI changes are made.
There was a problem hiding this comment.
Ah good catch, I wasn't thinking about CR/LFs. I think you're right and this will work as long as the JSON doesn't have CR/LFs.
The other approach would be to split out the fields into separate headers. I considered doing this because I read somewhere that web servers tend to support headers with values only up to 8KB. But the credentials file I have is only 2,330 bytes, so it should fit comfortably.
Eventually I decided to just treat the credentials as a kind of opaque blob for the sake of simplicity. But I'd be curious to know if you have an opinion on whether I should have separate headers for each field or not.
There was a problem hiding this comment.
I think it makes sense to leave it as a single header, since this would correspond directly to the gce_service_creds_dict on the cloudbridge side. On the UI side, we could have an upload field so that the user can upload his/her credentials file, which would be subsequently sent across through this header if it's being used as temporary creds, or saved in the "credentials" database field, if it's going to be persistent.
There was a problem hiding this comment.
I guess we'll just leave this like it is until further testing from the UI.
|
@machristie Thanks for the changes. |
2 participants
CloudLaunch changes to incorporate GCE as provided by cloudbridge.
I've done some testing with this but need to do some more testing. But I thought I would go ahead and create the pull request since I think this is ready for an initial review.
This is for issue #78