Create abstractions for using cloudlaunch-cli as a library

[machristie]

Create some abstractions for easily using cloudlaunch-cli as a python library for calling the CloudLaunch REST API.

TODO

• .update() and .delete() methods on API response objects
• proxy attributes to response data dictionary
• return iterator for paginated results moved to issue Support pagination in APIEndpoint #9
• Issue: PUT/PATCH doesn't work with openapi, attributes are converted to query parameters (openapi bug?) The issue had to do with the archived form and query parameter of DeploymentsViewSet, see comment below
• unit tests
• implement attribute access for responses
• create wrapper objects for each type of resource
  • Deployment
  • Task
  • Cloud
  • Credential
  • Image
  • Application
  • ...
• fix how update_endpoint is created for sub resources
• apply_data_mappings is confusing

DEFERRED

• configure read only fields and leave them out of update (PUT/PATCH) requests
  • maybe not needed, seems to be related to coreapi PUT/PATCH bug
  • Another approach is to turn off validation in coreapi
• decided to not switch from coreapi to requests for making REST calls at this time. There is an issue with coreapi when an endpoint has a field and a query parameter with the same name, but that is easily worked around for the time being.
• accept APIResource instances as argument to APIEndpoint create, update
  • I think it is more useful to document keyword arguments for calling create. APIResource's are designed to wrap API responses, not to be used in making requests.

[machristie]

@afgane @nuwang

Just want to get some feedback on the API design. I haven't implemented anything yet but I was thinking of an API that mimics the REST API hierarchically. This is a sketch of what I have in mind:

api_client = APIClient(...)
deployment = api_client.deployments.get(1)
aws_cred = api_client.auth.user.credentials.aws.create(
    name="...", access_key="...", secret_key="...", cloud_id="...", name="...", default=True)

I was also thinking it would be nice if results allowed calling sub-routes of the API (hard to put in words). Here's what I mean, you could do the following:

api_client = APIClient(...)
api_client.deployments.tasks.create(deployment_pk=1, action="HEALTH_CHECK")

but you can also create a task for a deployment with a deployment result directly with something like the following:

api_client = APIClient(...)
dep1 = api_client.deployments.get(1)
health_check_task = dep1.tasks.create(action="HEALTH_CHECK")

What do you guys think?

[afgane]

Looks good to me!

[nuwang]

Sounds good to me too. I think it looks pretty close to the cloudbridge API too, so that would make things easier/consistent across the stack. Really like the sub-routes idea.

[machristie]

Okay I figured out the problem with coreapi. The issue is that for deployments viewset archived is both a form parameter and a query parameter. When trying to update a deployment coreapi sees that archived is both a form and a query parameter and I think it is situation of last one wins so it ends up treating it as a query parameter. This then generates a 404.

Probably this is a schema generation issue. It doesn't make much sense to me that filter_fields defined on a DRF ViewSet are included as parameters for non-GET methods.

The only workaround I can think of would be to rename the archived filter_field so it doesn't conflict. The other option is to just use requests library.

header: Date header: Server header: Content-Type header: Vary header: Allow header: X-Frame-Options header: Content-Length send: b'PUT /api/v1/deployments
/23/?archived=false HTTP/1.1\r\nHost: localhost:8000\r\nuser-agent: coreapi\r\nAccept-Encoding: gzip, deflate\r\naccept: application/coreapi+json, applica
tion/vnd.coreapi+json, */*\r\nConnection: keep-alive\r\nauthorization: Token c781f362f577fdac7eff4323e2507e33de5e701b\r\nContent-Length: 1903\r\nContent-T
ype: application/json\r\n\r\n'
send: b'{"name": "my-ubuntu-test", "application_version": 5, "target_cloud": "amazon-us-east-n-virginia", "provider_settings": null, "application_config":
 {"config_cloudlaunch": {"instanceType": "t2.small", "firewall": [{"securityGroup": "cloudlaunch-vm", "rules": [{"protocol": "tcp", "from": "80", "to": "8
0", "cidr": "0.0.0.0/0"}, {"protocol": "tcp", "from": "22", "to": "22", "cidr": "0.0.0.0/0"}]}], "vmType": "t2.micro", "rootStorageType": "instance", "pla
cementZone": null, "keyPair": null, "network": null, "subnet": null, "staticIP": null, "customImageID": null, "provider_settings": {"ebsOptimised": null,
"volumeIOPS": null}}}, "added": "2018-03-16T13:11:46.895913-04:00", "updated": "2018-03-26T12:42:56.561832-04:00", "owner": "machristie", "app_version_det
ails": {"version": "16.04", "frontend_component_path": "app/marketplace/plugins/plugins.module#PluginsModule", "frontend_component_name": "ubuntu-config",
 "application": {"slug": "ubuntu", "name": "Ubuntu"}}, "tasks": "http://localhost:8000/api/v1/deployments/23/tasks/", "latest_task": {"id": 211, "url": "h

ttp://localhost:8000/api/v1/deployments/23/tasks/211/", "celery_id": null, "action": "DELETE", "status": "SUCCESS", "result": {"result": true}, "traceback
": null, "added": "2018-03-24T08:55:27.473824-04:00", "updated": "2018-03-24T08:56:42.139419-04:00", "deployment": 23}, "launch_task": {"id": 207, "url":
"http://localhost:8000/api/v1/deployments/23/tasks/207/", "celery_id": null, "action": "LAUNCH", "status": "SUCCESS", "result": {"cloudLaunch": {"keyPair"
: {"id": "cloudlaunch_key_pair", "name": "cloudlaunch_key_pair", "material": null}, "securityGroup": {"id": "sg-ec40da9e", "name": "cloudlaunch-vm"}, "ins
tance": {"id": "i-0ddb33b7a28727776"}, "publicIP": "34.200.216.36"}}, "traceback": null, "added": "2018-03-16T13:11:46.907230-04:00", "updated": "2018-03-
16T14:12:48.543345-04:00", "deployment": 23}, "credentials": 2}'
reply: 'HTTP/1.1 404 Not Found\r\n'
DEBUG:urllib3.connectionpool:http://localhost:8000 "PUT /api/v1/deployments/23/?archived=false HTTP/1.1" 404 23
header: Date header: Server header: Content-Type header: Vary header: Allow header: X-Frame-Options header: Content-Length Traceback (most recent call las
t):
  File "cloudlaunch_cli/api.py", line 308, in <module>
    dep.update()
  File "cloudlaunch_cli/api.py", line 55, in update
    api_response = self.update_endpoint.update(self.id, **data)
  File "cloudlaunch_cli/api.py", line 202, in update
    item = self._client.action(document, self.path + ['update'], params=params, validate=False)
  File "/Users/machrist/SGCI/cloudlaunch/launcher/venv/lib/python3.6/site-packages/coreapi/client.py", line 173, in action
    return transport.transition(link, self.decoders, params=params, link_ancestors=link_ancestors)
  File "/Users/machrist/SGCI/cloudlaunch/launcher/venv/lib/python3.6/site-packages/coreapi/transports/http.py", line 338, in transition
    raise exceptions.ErrorMessage(result)
coreapi.exceptions.ErrorMessage: <Error: Not Found>
    detail: "Not found."

See also

• URL not constructed in PUT/PATCH methods core-api/python-client#137
• Prevent path/param drop when both fields have the same name. core-api/python-client#138

[nuwang]

Nice working tracking that down. Seems ok to rename archived to avoid the conflict? The requests route seems ok too, if coreapi is overcomplicating things for what it offers. I was wondering though, coreapi does generate client side objects right? So if we go down the requests route, would we manually have to write client wrapper classes?

[machristie]

I reorganized the source code and also renamed APIResponse to APIResource.

I also implemented a simple form of attribute access that proxies to the response data dictionary. I think we'll want some sort of mapping for nested type so that those can be converted as well. For example, Deployment has a 'latest_task' attribute which should be converted to the Task wrapper object once we have it.

[machristie]

@afgane @nuwang I've taken a first stab at implementing wrapper resource classes for Deployment and Task. If you guys want to jump in and add more endpoints and resource classes, feel free.

I've started adding some unit tests but I think I will focus next on adding some additional tests for the APIEndpoint wrapper classes.

[nuwang]

@machristie I went through the cloudlaunch-cli code in greater detail and I've gained an appreciation of the design - I think it's a very generic/flexible design and has tremendously shortened the amount of code that needs to be written.

I was also comparing it to cloudbridge while going through the code, because I was hoping we could have some cross-pollination of ideas as well as improve consistency between the two, given that they are very similar in terms of what they do (interfacing with ReSTFUL resources/subresources) and also belong to the same CloudVE suite. So what follows is a kind of comparative critique - I'm just noting things here so we could take it up for discussion perhaps at our next meeting.

1. One thing that caught my attention is the difference in subroute/child-service handling between the two. In CloudBridge, nested services were moved to the resource.py class, because they couldn't be kept in the service.py class, since it creates a circular dependency. Example here: https://github.com/gvlproject/cloudbridge/blob/f005723fb81a82a89f9600755c42330b50abdeba/cloudbridge/cloud/interfaces/resources.py#L970

   I always thought this was a bit ugly in terms of design - what should ideally have been a service, is now within the resources.py class instead. In contrast, in cloudlaunch-cli, that problem has been neatly side-stepped by assigning the sub-resource property dynamically here:

   cloudlaunch-cli/cloudlaunch_cli/api/endpoints.py

   Line 147 in cf18ba1

   setattr(api_response,

   The main downside of this approach is that it can only indirectly be inferred that the tasks property exists on the deployment resource, e.g:

   cloudlaunch-cli/cloudlaunch_cli/api/endpoints.py

   Line 179 in cf18ba1

   subroutes = {

   I take it code completion will still work though?

   My thinking is that cloudbridge should follow this design, so we can neatly separate Resources and Services, but somehow preserve the explicit nature of the fact that the child service exists. Maybe we can have a public property called tasks on the Deployment resource, and assign _tasks dynamically or something?

2. Another thing was the difference in method signatures. The cloudlaunch-cli has generic method signatures, e.g:

   cloudlaunch-cli/cloudlaunch_cli/api/resources.py

   Line 21 in cf18ba1

   def update(self, **kwargs):

   Cloudbridge has more explicit signatures: https://github.com/gvlproject/cloudbridge/blob/f005723fb81a82a89f9600755c42330b50abdeba/cloudbridge/cloud/interfaces/resources.py#L1612

   I prefer the latter approach, since it's easier to code against, but up for debate since we may move to auto-generated code.

3. Instead of deserializing the object through the data parameter in the constructor, I was wondering whether we could do Resource.from_dict to construct the object instead? That way, we could make the Resource's constructer more explicit as in no. 2 above? Also, I found the apply_data_mappings logic a bit hard to grasp initially:

   cloudlaunch-cli/cloudlaunch_cli/api/resources.py

   Line 86 in cf18ba1

   data['launch_task'] = Task(data['launch_task']['id'],

4. The list methods probably needs paging support, with limit and marker perhaps, just like in cloudbridge?

[nuwang]

5. Just noting that CloudBridge has Services and Resources. CloudLaunch-cli has Endpoints and Resources. I feel as if this is fine, because it makes sense in context.

A bit of clarification on the circular import issue. This is where the circular import becomes a problem - when the resource needs import the child service:
https://github.com/gvlproject/cloudbridge/blob/f005723fb81a82a89f9600755c42330b50abdeba/cloudbridge/cloud/providers/aws/resources.py#L566

[machristie]

Thanks for the in depth feedback @nuwang ! I'll provide some responses here but it might be easier to discuss at our next meeting.

1. I'm not sure about code completion, but I like the public property idea.
2. I guess you can make more specific signatures in the subclasses. I think something that would be nice is some form of documentation about what keywords args are allowed, and more specific signatures help with that. Another thought I had, it might be nice if the endpoints took the APIResource instances as input as well. The APIResource subclasses could document all of the fields and that might be nicer for users of the api.
3. I'm not quite getting the from_dict idea, but I agree about apply_data_mappings, that needs work.
4. Yes, it needs paging supports, it's in the TODO list above :)

Also not following the circular import issue, but it sounds interesting.

[machristie]

@nuwang I just committed a change that addresses some of the comments you made in your review.

• no more setattr creating dynamic properties, subroute endpoints are declared now, see Deployment.tasks
• no need to override apply_data_mappings, instead now just supply a data_mappings dict. Here is the data_mappings dict of the Deployment resource
• also now child resources should have a proper update endpoint so they can self update