1220738 - Worker should use temporary task credentials for actions on behalf of the task

Status: RESOLVED FIXED

(Taskcluster :: Workers, defect)

Description

[Greg Arndt [:garndt]]

Use temporary credentials issued from the queue for performing actions on behalf of a task.

This includes extending graphs, creating artifacts.

Note: before this is done, tasks currently scheduled need to be reviewed to ensure they have the correct scopes for retrieving/generating artifacts.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

When this is complete, many of the scopes currently in the credentials from the provisioner can go away, too.

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

Worth noting that this *excludes* the credentials used to generate the bewit handed to bitbar (the taskdroid credentials in bug 1118394)

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Greg, is this on the roadmap for next quarter?

Comment 5

[Greg Arndt [:garndt]]

Wander has added it to the q1 goals for 2016 for himself

Comment 6

[Wander Lairson Costa]

Attachment: Use temp credentials for queue operations

Comment 7

[Wander Lairson Costa]

Attachment: Add necessary scopes to tasks. r=garndt.

Up to now, queue operations on behalf we the task used to with worker
scopes. We are going to change this behavior to use the task temporary
credentials. Therefore, we need to add to the tasks the necessary scopes
to run them.

Comment 8

[Wander Lairson Costa]

Not all that useful now, but here is the try push to make sure there is not syntax bustage
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94ca694ad366

Comment 9

[Greg Arndt [:garndt]]

Comment on attachment 8707440 [details] [diff] [review]
Add necessary scopes to tasks. r=garndt.

Looking at the queue, it appears that claim-task, resolve-task, and create-artifact are granted for that task ID and run ID, so they shouldn't need to be added as task scopes.  This should not be necessary now.

Comment 10

[Greg Arndt [:garndt]]

Comment on attachment 8706889 [details] [review]
Use temp credentials for queue operations

Right now there are reclaimer tests failing that should be looked at.  Let me know if you need any help figuring out why those are failing now.

Comment 11

[Wander Lairson Costa]

Attachment: Allow credentials update

Comment 12

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

Comment on attachment 8711078 [details] [review]
Allow credentials update

Thanks Wander! I've added some feedback in the PR. It looks great, but a couple of bits still to do (doc updates, tests, and locking around updates).

An awesome test would be having it run with temp credentials (see the other tests for how we do test setup for that), then create new temp creds, PUT them, and then make a new request, and validate that they used the new temp creds, not the old ones.

Comment 13

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

I thought about this some more Wander, and I wonder if we need this endpoint at all, or if we can't achieve the same by extending the lifetime of the original temporary credentials to be `maxRunTime` of the task. Adding this endpoint comes at quite a cost in terms of implementation, testing, documentation, maintenance, and head-space for people learning how taskcluster works. I think we could potentially drop all of it by just having one set of credentials that expire when the task run expires... I've pinged Jonas in https://github.com/taskcluster/taskcluster-proxy/pull/14/files#r50682350 to get his opinion too, as I could be overlooking something.

Comment 14

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

Of course we'd also have the benefit of losing the same (implementation, testing, documentation, ...) on the worker side that uses this endpoint.

Comment 15

[Greg Arndt [:garndt]]

Comment on attachment 8711078 [details] [review]
Allow credentials update

gave my feedback in the PR, pmoore can formally r +/- it.  I think the big takeaway is to guard around concurrent access to updating/reading the credentials.

Comment 16

[[github robot]]

Commits pushed to master at https://github.com/taskcluster/taskcluster-proxy

https://github.com/taskcluster/taskcluster-proxy/commit/f2a582fae65612733f14186f7e55a9d85bf55145
Bug 1220738: Create endpoint for credentials update. r=pmoore

docker-worker will use task temporary credentials for taskcluster
requests on task behalf.

During task reclaim, the temporary credentials might be updated, and
if using taskcluster-proxy to forward requests, we should update the
credentials in the proxy as well.

We create a "/Credentials" endpoint which receive PUT requests with
a json body containing the new credentials data for update.

https://github.com/taskcluster/taskcluster-proxy/commit/1517a8504676ec8a5135be46c6f44ea195813c06
Merge pull request #14 from walac/master

Bug 1220738: Create endpoint for credentials update. r=pmoore

Comment 17

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

Comment on attachment 8711078 [details] [review]
Allow credentials update

Great work Wander.

Comment 18

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

Wander, you'll need to release a new version of taskcluster-proxy (3.0.7).

You can follow the instructions here:
https://github.com/taskcluster/taskcluster-proxy/#making-a-release

Be aware, docker worker will take `latest` docker tag for the taskcluster-proxy docker image, so keep your eyes open for problems when you roll out. Alternatively you could tag 3.0.7 first, test that in production e.g. on a single worker type, and if all fine, push the docker tag `latest`. However, that is quite a bit of work, for the service that is not heavily used - so maybe run a test like:
https://tools.taskcluster.net/task-inspector/#B4vJQ3wCSu2CGNw8gMyyCA/ and check it is still working... Note, currently secrets service is down (bug 1235387) so you might need to trigger this task again when that is resolved.

You should get back a response like https://bugzilla.mozilla.org/show_bug.cgi?id=1234929#c51 (note: just being green isn't enough, you should check you get the right headers and body).

If you do decide to check on a single worker type first, I'd suggest using a workerType like ami-test, and adding

"taskclusterProxyImage": "taskcluster/taskcluster-proxy:3.0.7"

to the userData in the worker type definition https://tools.taskcluster.net/aws-provisioner/#ami-test/edit and this way you can test it before all other workertype pick it up. If it is good, just push the latest tag to the new version, as `latest` is what everything uses by default.

Comment 19

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

@wcosta, thanks for releasing taskcluster-proxy! :)

Due to some issues with 3.0.7 (https://github.com/taskcluster/taskcluster-proxy/pull/17), this was released as taskcluster-proxy 3.0.8.

Comment 20

[Greg Arndt [:garndt]]

Comment on attachment 8706889 [details] [review]
Use temp credentials for queue operations

r+ once the merge conflicts are resolved.  Things look good and we tested his with a push to try earlier last week.

Comment 21

[[github robot]]

Commits pushed to master at https://github.com/taskcluster/docker-worker

https://github.com/taskcluster/docker-worker/commit/b72c0832dad8f445aeb7a285d46a919e670b1369
Bug 1220738 part 1: Use temp credentials on task behalf. r=garndt

For all task related queue operations, we use a queue object
created with temporary credentials.

https://github.com/taskcluster/docker-worker/commit/4d2c02e7523e9463aea4a06999e1fcd149f1f522
Bug 1220738 part 2: Make scheduler use temporary credentials. r=garndt

Scheduler is currently only used to extend graphs. Instead of using the
worker credentials, use the task temporary credentials for that.

https://github.com/taskcluster/docker-worker/commit/c10ce872a4088a75fa43b6129ecf3f8e5a2d992f
Bug 1220738 part 3: Update taskcluster-proxy temp credentials. r=garndt

When we reclaim a task, the credentials might get updated. When this
happens, we must send the new credentials to the taskcluster proxy, if
this feature is set to "true".

It uses the new taskcluster-proxy "Credentials" endpoint. The
credentials update integration test will fail until
https://github.com/taskcluster/taskcluster-proxy/pull/14 is merged.

We also had to update the unit test reclaim_test because now the Task
class extends EventEmitter.

https://github.com/taskcluster/docker-worker/commit/d717293f519e703003c2a2622df8e48ec605e569
Bug 1220738 part 4: Use temp creds for docker images. r=garndt

https://github.com/taskcluster/docker-worker/commit/4277d2743ccda8c353f5f73f7a927222d2e15c8f
Merge pull request #203 from walac/master

Bug 1220738: Use temp credentials on task behalf. r=garndt

Comment 22

[Greg Arndt [:garndt]]

AMIs are rolling out with these changes soon.

Comment 23

[Greg Arndt [:garndt]]

All worker types have been updated with this AMI as of 4/18.