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

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: garndt, Assigned: wcosta)

Tracking

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
No longer blocks: 1134342
Depends on: 1134342
When this is complete, many of the scopes currently in the credentials from the provisioner can go away, too.
Duplicate of this bug: 1218784
Worth noting that this *excludes* the credentials used to generate the bewit handed to bitbar (the taskdroid credentials in bug 1118394)
Greg, is this on the roadmap for next quarter?
Flags: needinfo?(garndt)
(Reporter)

Comment 5

3 years ago
Wander has added it to the q1 goals for 2016 for himself
Flags: needinfo?(garndt)
(Assignee)

Updated

3 years ago
Assignee: nobody → wcosta
Status: NEW → ASSIGNED
(Assignee)

Comment 6

3 years ago
Created attachment 8706889 [details] [review]
Use temp credentials for queue operations
Attachment #8706889 - Flags: review?(garndt)
(Assignee)

Comment 7

3 years ago
Created attachment 8707440 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8707440 - Flags: review?(garndt)
(Assignee)

Comment 8

3 years ago
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
Keywords: leave-open
(Reporter)

Comment 9

3 years ago
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.
Attachment #8707440 - Flags: review?(garndt)
(Assignee)

Updated

3 years ago
Attachment #8707440 - Attachment is obsolete: true
(Reporter)

Comment 10

3 years ago
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.
Attachment #8706889 - Flags: review?(garndt) → review-
(Assignee)

Comment 11

3 years ago
Created attachment 8711078 [details] [review]
Allow credentials update
Attachment #8711078 - Flags: review?(pmoore)
Attachment #8711078 - Flags: feedback?(garndt)
(Assignee)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Updated

3 years ago
Attachment #8706889 - Flags: review- → review?(garndt)
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.
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.
Of course we'd also have the benefit of losing the same (implementation, testing, documentation, ...) on the worker side that uses this endpoint.
(Reporter)

Comment 15

3 years ago
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.
Attachment #8711078 - Flags: feedback?(garndt) → feedback+

Comment 16

3 years ago
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 on attachment 8711078 [details] [review]
Allow credentials update

Great work Wander.
Attachment #8711078 - Flags: review?(pmoore) → review+
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.
Flags: needinfo?(wcosta)
@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.
Flags: needinfo?(wcosta)
(Reporter)

Comment 20

3 years ago
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.
Attachment #8706889 - Flags: review?(garndt) → review+

Comment 21

3 years ago
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
(Reporter)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 22

3 years ago
AMIs are rolling out with these changes soon.
(Reporter)

Comment 23

3 years ago
All worker types have been updated with this AMI as of 4/18.
You need to log in before you can comment on or make changes to this bug.