taskcluster / taskcluster-rfcs

Taskcluster team planning
Mozilla Public License 2.0
11 stars 19 forks source link

RFC: projectId #163

Closed djmitche closed 3 years ago

djmitche commented 4 years ago

Thoughts on the open question at the bottom?

petemoore commented 4 years ago

Thoughts on the open question at the bottom?

You raise a good point that current decision tasks would break, since they call queue.createTask without a projectId but do not explicitly include scope queue:create-task:project:none.

Perhaps the cleanest solution is to make projectId optional. I think not specifying a value, or specifying the empty string, should put a task in the general purpose, non-project bucket. Later on we could require all projects to include a projectId, and at some point disallow tasks without a projectId. I think having the projectId as optional (and not including it if it doesn't exist) is a little cleaner than having the special project none. This way, to create a task with a projectId, you would need the scope, and if it doesn't have a projectId, you don't. This might also help to keep existing task shapes the same as they currently are, for any chain-of-trust type operations that might format the json output, sort the properties etc, and then calculate e.g. the sha256 of the json representation (I'm not sure if we have anything that does exactly that, but I can imagine that having the task definitions not mutate when this lands could be beneficial).

This then avoids the dilema about handling queue:create-task:project:none because if a task doesn't have a project, it won't need the scope. It also opens the door to being able to have a task associated to multiple projects (that might also be worth considering in this first proposal, whether there are any use cases of value where a task might belong to multiple projects).

djmitche commented 4 years ago

@petemoore and I talked about this in Zoom. Pete's interest is primarily in avoiding "flag days" where users must change their implementations on a particular day. There are two days to be concerned with: the day this change lands, and a year later when we drop support for the compatibility measures.

Not requiring new scopes for tasks created without a projectId seems the best way to ensure no breakage on the day this change lands -- all scope changes would add a new alternative to {AnyOf: ..}, so anything that succeeded before the change would succeed after.

A year later, either users have had a chance to adapt their use of Taskcluster to define project(s), or the deployment administrators have had time to add queue:create-task:project:none in the necessary places to allow task creation to continue without the compatibility infrastructure.

We discussed pros and cons of "none", null, and "" as the distinguished projectId during the year of compatibility. The string "none" works well in a scope expression, where null and "" do not. Using null requires that anything handling projectIds handle the null case (or forget to do so). In rust that means if let Some(projectId) = .., in Go it means checking for nil or "", and in JS and Python it means probably forgetting that null is a possibility and getting a TypeError at some point. Empty strings have the correct type, but are easy to miss and tend to disappear when interpolated.

I'd like to hear @escapewindow's thoughts on problems caused by adding a new field to the response to queue.task -- would that cause issues with any kind of validation? That could help us decide whether to "hide" the projectId when it's set to the compatibility value ("none" or "" or null), or to always include it in the task definition.

As for tasks associated with multiple projects -- I don't think that's a good idea. It would cause issues for allocation of costs, which is one of the motivations for this proposal (given a task with three projects, which is "billed" for the task?). The case where some subset of tasks should be addressable by multiple groups of people can be handled by assigning smaller-grained projects. For example, if group A should be able to manipulate tasks X and Y, while group B should be able to manipulate tasks Y and Z, that can be represented by giving group A scopes for projects X and Y, and group B scopes for projects Y and Z. Use of * in scopes to refer to multiple projects can help manage this kind of configuration, just as it does for other scoped parameters.

So:

escapewindow commented 3 years ago

I'd like to hear @escapewindow's thoughts on problems caused by adding a new field to the response to queue.task -- would that cause issues with any kind of validation? That could help us decide whether to "hide" the projectId when it's set to the compatibility value ("none" or "" or null), or to always include it in the task definition.

The primary way we verify task definitions is in rebuilding decision/action/cron tasks from .taskcluster.yml and/or actions.json. If we're still able to do that after this change, we should be good.

We also store a copy of the task definition in the CoT artifact. It looks like we don't do anything with this atm, and it's there for auditing. We probably want the projectId to be set the same in both the queue and CoT artifact.

[Edit] there may be a period of time where new tasks have projectId and older tasks don't, or a cutover period where CoT may break if we assume certain behavior. We can adjust the verification logic to allow for a mismatch in that field, specifically, until behavior is consistent.

escapewindow commented 3 years ago

I think this is a good change, and I can't think of anything specific objections. 👍

djmitche commented 3 years ago

[Edit] there may be a period of time where new tasks have projectId and older tasks don't, or a cutover period where CoT may break if we assume certain behavior. We can adjust the verification logic to allow for a mismatch in that field, specifically, until behavior is consistent.

Thanks! I think this is what I needed to know. We could probably land a change to CoT before deploying the TC changes, so that CoT ignores projectId everywhere. Then once all tasks have a non-default projectId, we can modify that so that CoT expects and verifies projectId everywhere.

escapewindow commented 3 years ago

This may be as simple as adding projectId to this tuple.

[Edit] Maybe also here if the rebuilt decision/action/cron definition is different than the runtime one.

djmitche commented 3 years ago

(rebased)

I'm moving this to final-comment period now. Barring objections, we'll consider it decided next Wednesday.