1078520 - python client

Status: RESOLVED FIXED

(Taskcluster :: General, defect)

Description

[James Lal [:lightsofapollo]]

Its increasingly clear to me that TC would be useful in a wide number of other mozilla services... I think its time to build a python client (which behaves as close as possible to the node client).

We could split this up as a metabug but in order of priority:

- http services
- auth
- bewit
- listener

Whatever the implementation details this should be mostly a autogenerated client so we don't need to manually update interfaces the node client can be used as a reference for how to port this.

Comment 1

[James Lal [:lightsofapollo]]

@jhford: Is this something you could put in your queue? I think jonasfj/mdas would be good reviewers.

Comment 2

[Jonas Finnemann Jensen (:jonasfj)]

I've also been thinking about doing this...someday.
But if jhford is up for it that would be great :)

Documentation for our reference formats is present here:
http://docs.taskcluster.net/tools/references/

I might have changes to how the AMQP listener code looks in taskcluster-client, now that pulse has multi-user support. But other than that, I think it's reasonably safe to implement both the API-client and the WebListener more or less as done in node.js.
IMO, a synchronous API is required for python, asynchronous support would be nice to have (maybe someday).

By the way, I would like to figure out how we can support API end-points that doesn't return JSON, but binary content. This shouldn't block writing a python client, but it's something we'll eventually have to support in both javascript and python clients.

Remark: Porting the WebListener is a nice to have, it's the only way to listen for events without credentials. But it's only useful for CLI tools, etc. as it's unreliable by nature.

Comment 3

[James Lal [:lightsofapollo]]

Some more details we quickly discussed on IRC.

  - client should be sync
  - client must work on 2.7.x

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

Taking this bug, work will happen here: https://github.com/taskcluster/taskcluster-client.py

Comment 5

[Jonas Finnemann Jensen (:jonasfj)]

For details on recommended retry logic, see pseudo code in: bug 1056269

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: First review

Here's the first PR.

To build/test locally, you should be able to do:

make dev-env
make test

Note that there is a weird issue with PyHawk, which is why I'm using my own fork of the library.  The intention is to go back to the upstream PyHawk when it's got the fix I need.

Comment 7

[John Ford [:jhford] CET/CEST Berlin Time]

I can't seem to authenticate with either the Node.js client or the Python client, so I can't really test that everything is working as expected.  I'm using the credentials exactly as I get them from auth.taskcluster.net.

Once I have verified that the auth is working, I can write tests to make sure it doesn't break.

Comment 8

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8515679 [details] [review]
First review

you can't authenticate with node.js client? Try writing a test with `auth.scopes(youClientId)` to get scopes that you have. It doesn't take input... Then put the test on  gist and send to me..
Don't include your credentials :)
(but do use recent version of taskcluster-client I update it regularly).

Btw, auth is now managed through tools.taskcluster.net please check it out...

Anyways, we need to:
 - get auth working.
 - Resolve the config situation, IMO immutable config is the way to go
   (AWS SDK does this too).
 - Make the algorithm for hawk unconfigurable
 - Remove title and description from `_options`
 - Expose slugid
 - update api.json to a recent version

But in general it looks pretty good.

Comment 9

[Jonas Finnemann Jensen (:jonasfj)]

Additional comments the PR, but I think you've already seen some of them.

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #8)
> Comment on attachment 8515679 [details] [review]
> First review
> 
> you can't authenticate with node.js client? Try writing a test with
> `auth.scopes(youClientId)` to get scopes that you have. It doesn't take
> input... Then put the test on  gist and send to me..
> Don't include your credentials :)
> (but do use recent version of taskcluster-client I update it regularly).

Ok. so I know what was going on here.  I went to auth.taskcluster.net to retreive my auth credentials and was using the temporary credentials from there instead of my real jhford ones because I had lost those.  I think what happened is that my clientId and accessToken were saved in the browser and it was just giving me these temporary ones?  I dunno.

After getting my credentials sorted out, both the node and python clients work great!  I'll write tests for this tomorrow.

>>> import taskcluster as t
>>> a = t.Auth()
>>> a.getCredentials(os.environ['TASKCLUSTER_CLIENT_ID'])
{u'scopes': [u'*'], u'expires': u'xxxxx', u'accessToken': u'xxxxx', u'clientId': u'xxxxx'}
>>>

> Btw, auth is now managed through tools.taskcluster.net please check it out...

I thought I had, but I went there now and it turns out that I hadn't.  Looks great!

> Anyways, we need to:
>  - get auth working.

Working!  Need to write some tests now that I know it's working as expected.

>  - Resolve the config situation, IMO immutable config is the way to go
>    (AWS SDK does this too).

Immutable config does not exist in python.  I have changed it to do no more shared module-level config, removed the read-only property and setOption() method.  I'm also caching the hawk ext data where appropriate, but I don't think we can safely cache it in __init__ as long as python doesn't have a const system.  I think in a case like this were we have to weigh integrity against speed, we should err on the integrity side until we have performance data to show that it's a big enough improvement to justify.

I wonder if I could do something wacky in __init__ to create a read only property() and make it actually immutable.  Hmm...

>  - Make the algorithm for hawk unconfigurable

Made

>  - Remove title and description from `_options`

Removed

>  - Expose slugid

Exposed

>  - update api.json to a recent version

Updated

> But in general it looks pretty good.

Thanks!

How should I ask that you look at it again?  Submit a new r? or just flip the flag back to r?

Comment 11

[Jonas Finnemann Jensen (:jonasfj)]

> Immutable config does not exist in python.
One can make immutable objects in python... but let's not go there.
A quick fix would be to rename the property __options then people usually stay away from fiddling with it, if not I guess it's their funeral :)
Anyways, as long as people aren't encouraged to mutate the options I'm happy.

> How should I ask that you look at it again?  Submit a new r? or just flip the flag back to r?
He he... flip it back.
But really, if we have tests (positive and negative) for:
 - clientId/accessToken authentication
 - authorizedScopes
 - temporaryCredentials
     - Limitation of scopes
     - Use of credentials generated by the official taskcluster-client
       (or at least check that you can load a static/expired pair)
     - Use of credentials generated by the python taskcluster-client.py
 - buildUrl (just test it against queue.ping)
 - buildSignedUrl* (you can test it against auth.scopes)
     - using clientId/accessToken
     - using temporary credentials

Then I think I'm happy.

Note, in tests we shouldn't rely on the '*' scope. Perhaps just have a set of credentials with the scope:
  "queue:define-task:dummy-none-provisioner/dummy-worker-*"

Then you can still play with authorizedScopes and show that restricting to
  "queue:define-task:dummy-none-provisioner/dummy-worker-one"
doesn't allow you to call with:
  "queue:define-task:dummy-none-provisioner/dummy-worker-two"

If your tasks are created under provisionerId: "dummy-none-provisioner" and workerType: "dummy-worker-*".
I don't think anybody will care what you put into those tasks.
Then the credentials you use (which you can create on tools.taskcluster.net/auth) can also be used on travis, though they should still be encrypted. See travis docs for encrypted environment variables.

We should be able to test without scope required for auth.scopes (auth:inspect), so buildSignedUrl should not be testable with the credentials we encrypt and give to travis. Though even without the scope to execute the request, we can still test if we can build the URL without an error being thrown.


Note, it might be tempting to steal the setup we have in taskcluster-client tests:
https://github.com/taskcluster/taskcluster-client/blob/master/test/client_test.js#L19-L34
They create the a server on localhost so it easy to test against... You could just start the node server before your tests or in a test hook... everything needed for a mock auth server is in taskcluster-base, so it really just a node script with 10 lines or so...

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8515679 [details] [review]
First review

Looks good, I have no blocking issues. I'm not that familiar with the requirements of TC, but from a python client perspective I think the API should follow PEP8 if possible (ie: lowercase with underscores instead of camelcase). It's a norm for python libs and what most dev expect.

I'm also not a huge fan of using Make as a command dispatcher instead of generating/managing builds, mostly from painful bias from gaia... mach's a better fit here, and would be easier to maintain.

Comment 13

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8515679 [details] [review]
First review

I see my comments addressed, so r+.

Regarding, coding style all taskcluster methods have a name in the JSON reference, you'll see it in the documentation as `signature`. This name is in mixedCase format (pep8 terminology).

Technically we could try to auto-translate it, but let's not go there :)
For this reason think it's okay use mixedCase for methods persistently.
pep8 says:
    "mixedCase is allowed only in contexts where that's already the prevailing style"

Honestly, I don't care as long as methods a documented. Preferably we can make the documentation available somewhere too.

Anyways, feel free to land this on pypi or something.

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 8515679 [details] [review]
First review

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #11)
> But really, if we have tests (positive and negative) for:
>  - clientId/accessToken authentication

test_mock_auth_works
test_mock_auth_expired
test_mock_auth_invalid

>  - authorizedScopes

test_mock_auth_works_with_small_scope
test_mock_auth_bad_scope

>  - temporaryCredentials
>      - Limitation of scopes
>      - Use of credentials generated by the official taskcluster-client
>        (or at least check that you can load a static/expired pair)
>      - Use of credentials generated by the python taskcluster-client.py

I've done a lot to check that the output of creating temporary credentials.  The contents of the credentials are identical.  The access token is identical, but the ordering of the serialized JSON is different.  I've included the scripts that I used to verify this in a commit comment if you'd like to try to replicate.

For now, these tests are marked as expected failures and the readme reflects this.

>  - buildUrl (just test it against queue.ping)

I have tests for buildUrl, but they don't hit the real server.  This function generates a string, I think it's better to just do string equality checks, since that involves the fewest moving bits.  As well, the tests against the mock auth server of the signed URL functionality include tests of this method as the signed method works with 

>  - buildSignedUrl* (you can test it against auth.scopes)
>      - using clientId/accessToken

test_mock_auth_signed_url
test_mock_auth_signed_url_bad_credentials

>      - using temporary credentials

These are busted, I'm not entirely sure why.  See https://github.com/jhford/taskcluster-client.py/commit/cd70ad535a0cc8d597e9cbe38c5409946dee0c5e for more info.

> Note, it might be tempting to steal the setup we have in taskcluster-client
> tests:

Very tempting, I ended up doing that.

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 8515679 [details] [review]
First review

https://pypi.python.org/pypi/taskcluster/0.0.4

https://github.com/taskcluster/taskcluster-client.py/commit/d6d493e13ba7062201f5af2a8c4313bc13ea6223

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

I've filed bug 1097780, which is the only remaining issue.

Comment 17

[James Lal [:lightsofapollo]]

Comment on attachment 8515679 [details] [review]
First review

clear my f? (it was plus anyway)

Comment 18

[Pete Moore [:pmoore][:pete]]

Resetting Version and Target Milestone that accidentally got changed...