1465842 - Create ci-configuration/scope-grants.yml

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

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

Attachment 8982271 [details] [diff] added to Bug 1381870

Attachment #8982271 [details] [diff] to bug 1381870 created

    Email sent to:
        [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail] (hide) 

Create Another Attachment to Bug 1381870
Bug 1381870
Move gecko-specific bits of tcadmin into the Gecko tree
NEW Assigned to dustin
(NeedInfo from tomprince
)
Get help with this page
Status
Product:
▸ Taskcluster
Component:
▸ Operations
Reported:
11 months ago
Modified:
just now
Status:
People (Reporter: dustin, Assigned: dustin, NeedInfo)
Assignee:
Dustin J. Mitchell [:dustin] pronoun: he
Reporter:
Dustin J. Mitchell [:dustin] pronoun: he
Triage Owner:
Brian Stack [:bstack]
NeedInfo From:
tomprince
8 minutes ago
CC:
▸ 5 people
Tracking
Version:
unspecified
Target:
---
Depends on:
1393017, 1437964, 1463778
Dependency tree / graph
Details
Whiteboard:
---
Attachments (1 attachment)
bug1381870.patch
just now Dustin J. Mitchell [:dustin] pronoun: he
4.30 KB, patch
	
dustin
: review? tomprince
	Details | Diff | Splinter Review
Attach File
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Description • 11 months ago

Let's configure all of the roles, hooks, etc., using in-tree code (but not running in automation, since automation does not have the scopes to do so)

	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 1 • 10 months ago

The idea is to have some commands like
 ./mach tcadmin check-config
and
 ./mach tcadmin update-config
that would enforce the bits of gecko-specific TC configuration we're currently managing manually.  Roughly, terraform : AWS :: tcadmin : TC, although quite a bit simpler.

The `check-config` command wouldn't require any auth, so I'd like to run that in a cron.yml task, with notifications to taskcluster-notifications.  Then someone with credentials would need to run `update-config` as necessary.

Does that seem reasonable?

Flags: needinfo?([e-mail])
	Greg Arndt [:garndt]
	
Comment 2 • 10 months ago

(In reply to Dustin J. Mitchell [:dustin] from comment #1)
> Roughly, terraform : AWS :: tcadmin : TC,
> although quite a bit simpler.

Exactly what came to my mind

> 
> The `check-config` command wouldn't require any auth, so I'd like to run
> that in a cron.yml task, with notifications to taskcluster-notifications. 

Is this so that if the config we currently have differs from the one defined in tree, we'll get a notification?


> Does that seem reasonable?

I love having this defined in-tree/code somewhere!

Flags: needinfo?([e-mail])
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Updated • 8 months ago
Blocks: 1407681
Depends on: 1393017
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 3 • 4 months ago

https://public.etherpad-mozilla.org/p/dustin-in-tree-resource-config

	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Updated • 4 months ago
No longer blocks: 1407681
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Updated • 4 months ago
Depends on: 1437964
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 4 • 3 months ago

I still want to do this, but frankly it's both getting more of my attention than it deserves (there are higher-priority things) and too little to actually complete it.  Perhaps I will revisit if someone else doesn't pick this up.

Assignee: [e-mail] → [e-mail]
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 5 • a month ago

I think the work on actions as hooks (bug 1415868) is going to make this pretty painfully necessary.

Assignee: [e-mail] → [e-mail]
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 6 • 29 days ago

So I've talked to Tom about this a few times and I *think* I understand his concerns.. please correct me if I'm wrong!  The main concern is that it ties the status of the global (across all projects) config to a single project (probably mozilla-central).  It's also weird that that project can't *enforce* the global config, but only complain when it's wrong.

Things are about to get worse.  With the landing of actions as hooks, the behavior of in-tree actions is pretty inter-dependent on the detailed configuration of hook resources.  In particular, as implemented the hook configuration depends on `.taskcluster.yml` and on the actions defined in-tree.

So, we have a question of where to put the configuration (details about projects, etc.), and where to put the code that's responsible for making that configuration real.

There's a secondary question, which Jonas has raised, about how to express the necessary Taskcluster resources based on that configuration.  Jonas would like that to be declarative, but I'm not convinced that's possible other than by essentially listing every resource in a YML file and manually ensuring all of the necessary, complex correspondences are in place through the review process.  JSON-e is not nearly expressive enough (it quickly becomes unreadable), and I doubt Terraform could express the dependencies either, without a whole lot of copy pasting. I don't think we understand the problem well enough to design a new DSL for the purpose.

That means that the configuration and the means of turning that into Taskcluster resources will probably not be very loosely coupled.  So they should probably be in the same repository.

The configuration *definitely* should not be in mozilla-central.

So I think the logical conclusion is that we need to design this such that ci-configuration is self-contained: aside from providing data for other tools, it contains enough code to interpret the configuration and create/update/delete Taskcluster resources to match.  Basically tcadmin, but a more comprehensive implementation.

While I embark on that .. are there other practical options here?

Flags: needinfo?([e-mail])
Flags: needinfo?([e-mail])
	Tom Prince [:tomprince]
	
Comment 7 • 29 days ago

I think that is a reasonable approximation of my concerns with moving `ci-configuration` + `tc-admin` into mozilla-central.

I'm curious what the pain points for keeping the existing split between ci-configuration and tc-admin are? The only one that I've run into so far, it is not possible to do a trial run against a new version of ci-configuration. I've hacked around that by pointing at a local server, but it should be straigtforward to make the tools optionally take a local path, at least in no-op mode.

Flags: needinfo?([e-mail])
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 8 • 28 days ago

One of them is that tc-admin is a mix of Firefox-specific stuff and Taskcluster-specific stuff.  Also, it's a JS app and the rest of the associated mechanics are in Python.

There is an option of having three repos -- mozilla-central, ci-configuration, and ci-admin; with the latter being a Python version of tc-admin.  But as you've noted it's pretty annoying to try to coordinate changes to ci-configuration and ci-admin, so why not combine them?

It's worth noting that we also want to protect these repos against changes, too -- ideally fewer people than "level 3" can modify either the configuration (data) or the code that implements that configuration.

Putting this all in one repo would work well in the sense that any change that lands would have to go through the usual bug + review process, and then the person landing the patch would be responsible for running the result (and supplying the credentials to do so).

	Tom Prince [:tomprince]
	
Comment 9 • 28 days ago

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #8)
> There is an option of having three repos -- mozilla-central,
> ci-configuration, and ci-admin; with the latter being a Python version of
> tc-admin.

I *think* I *slightly* prefer this option, although I'm not sure I can articulate
a good reason for the preference. 

:+1: to switching to python

> But as you've noted it's pretty annoying to try to coordinate
> changes to ci-configuration and ci-admin, so why not combine them?

I noted it in order to downplay. :)

> It's worth noting that we also want to protect these repos against changes,
> too -- ideally fewer people than "level 3" can modify either the
> configuration (data) or the code that implements that configuration.

Yes.

	Jonas Finnemann Jensen (:jonasfj)
	
Comment 10 • 20 days ago

> Jonas would like that to be declarative, but I'm not convinced that's possible other than by essentially listing every resource in a YML

I think that's a fair point. What if use python code to generate the YAML file?
 1) A python script generates a YAML lists all resources, and what namespaces are managed
 2) A different python script takes the declarative output from (1) and creates/updates/deletes resources in TC

That way it's easy to diff the intermediary YAML file and see what the changes are.
It's also easy to inspect the YAML file to see what resources are created?

Note: whether (1) happens with python, json-e, jinja2, or bash-scripting is less important to me.
      As long as the output is easy to read, and the scritps in (1) are pure-functional, ie. they don't call APIs.

Flags: needinfo?([e-mail])
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 11 • 9 days ago

One thing I've been worried about is updating roles like mozilla-group:vpn_sheriffs, as those roles may have other scopes added "manually" that we want to preserve.  I think the way to fix that is to instead manage roles like project:gecko:action-access:vpn_sheriffs and assume:.. that role (manually) in the relevant mozilla-group:.. roles.

Then the input to (2) is basically:

 * a set of "managed" namespaces
 * a set of desired resources

and (2) consists of enumerating the runtime configuration in the managed namespaces and deleting anything unrecognized, together with creating/updating the desired resources.

	Tom Prince [:tomprince]
	
Comment 12 • 9 days ago

> I think the way to fix that is to instead manage roles like project:gecko:action-access:vpn_sheriffs and assume:.. that role (manually) in the relevant mozilla-group:.. roles.

This seems sensible.

> Then the input to (2) is basically: [...]

This seems like the declarative intermediate state that Jonas was asking for (whether it needs to be dumped to disk or passed via a library is something that can be resolve later).

> > There is an option of having three repos -- mozilla-central,
> > ci-configuration, and ci-admin; with the latter being a Python version of
> > tc-admin.
> 
> I *think* I *slightly* prefer this option, although I'm not sure I can
> articulate
> a good reason for the preference. 

Thinking on this more, I think it makes sense to keep *some* code that accesses the configuration separate from the configuration, as long as there is any code that access the configuration that is not in the config repo (such as mozilla-taskcluster).

	Tom Prince [:tomprince]
	
Comment 13 • 9 days ago

> Thinking on this more, I think it makes sense to keep *some* code that accesses the configuration separate from the configuration, as long as there is any code that access the configuration that is not in the config repo (such as mozilla-taskcluster).

That is, all the code touching the config should either be in the same repo as the config, or none of it.

	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Updated • 8 days ago
Depends on: 1463778
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 14 • 6 days ago

Can I get a 30% review on https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/ci-admin?

The README is a good place to start.  Note that I have stuck this in a subdir in the ci-configuration repo for momentary convenience but I intend to land it in build/ci-admin (bug 1463778).

Questions I'm interested in:
 - overall design -- am I don't something fundamentally wrong?
 - complexity -- is this unapproachably complex code?
 - features -- is there some important feature missing (that isn't in the README's TODO)?
 - future -- do you see something we'd want to do soon, that this would not support?

I'm not especially worried about line-by-line Python stuff, but if you feel so inclined please feel free to bring it up.

Flags: needinfo?([e-mail])
Flags: needinfo?([e-mail])
Flags: needinfo?([e-mail])
	Tom Prince [:tomprince]
	
Comment 15 • 6 days ago

This is looking good.

- Given that this is going to be in a separate repo, there should be a way to specify a local repo to look at, for testing (but probably not applying?), as well as looking at the repository-of-record. (Just based on reading the README)
- https://github.com/Tinche/cattrs might be useful for the json conversion
- I would try to avoid using formatted strings for holding data (e.g. https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/ci-admin/ciadmin/current/hooks.py#l18)
- Note that `attrs` has a bunch of options for only using bits of it, and for constructing attributes that aren't exposed in the constructor (for e.g. https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/ci-admin/ciadmin/resources/resources.py#l84)

Once Bug 1463778 is done, I think it would make sense to get phabricator and lando turned on, and use them from the start.

Flags: needinfo?([e-mail])
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 16 • 6 days ago

(In reply to Tom Prince [:tomprince] from comment #15)
> This is looking good.
> 
> - Given that this is going to be in a separate repo, there should be a way
> to specify a local repo to look at, for testing (but probably not
> applying?), as well as looking at the repository-of-record. (Just based on
> reading the README)

This is possible already - I'll add a note to the README.

> - I would try to avoid using formatted strings for holding data (e.g.
> https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/
> ci-admin/ciadmin/current/hooks.py#l18)

I'm not sure what you mean -- how is that "holding data"?

> - Note that `attrs` has a bunch of options for only using bits of it, and
> for constructing attributes that aren't exposed in the constructor (for e.g.
> https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/
> ci-admin/ciadmin/resources/resources.py#l84)

I'm confused by this, too -- what attributes would be constructed there?

> Once Bug 1463778 is done, I think it would make sense to get phabricator and
> lando turned on, and use them from the start.

Sigh, I suppose you're right.

	Tom Prince [:tomprince]
	
Comment 17 • 6 days ago

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #16)
> > - I would try to avoid using formatted strings for holding data (e.g.
> > https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/
> > ci-admin/ciadmin/current/hooks.py#l18)
> 
> I'm not sure what you mean -- how is that "holding data"?

You got some structured data (that you are looking for a hook, with a given group-id) and are encoding it in specifically formatted string. I haven't looked at all the involved code, but generally it is better to use structured data rather than passing around formatted strings.
 
> > - Note that `attrs` has a bunch of options for only using bits of it, and
> > for constructing attributes that aren't exposed in the constructor (for e.g.
> > https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/
> > ci-admin/ciadmin/resources/resources.py#l84)
> 
> I'm confused by this, too -- what attributes would be constructed there?

Something vaguely like

def make_managed(managed):
    managed = SortedList()
    for m in managed:
        self.manage(m)

resources = attr.ib(type=SortedKeyList, converter=lambda resources: SortedKeyList(resources, key=lambda r: r.id))
managed = attr.ib(converter=make_managed)

def __attr_post_init__(self):
    self._verify()

(typed directly into bugzilla)

	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 18 • 6 days ago

The `id` is a unique identifier for the Taskcluster resource, and we do prefix matching on them.  The prefix matching is always after the "=", but still, I think it's easier to say id.startswith(pattern) than (id.kind == pattern.kind and id.subid.startswith(pattern.subid)) (and also, I'm not sure what to call the subId -- for hooks it's '{hookGroupId}/{hookId}').

Good point on using attrs for Resources, though!  Thanks for clarifying..

	Gregory Szorc [:gps]
	
Comment 19 • 6 days ago

I'm far from a domain expert in what is being done here. I like the idea of the project (making it easier to define/update the TC platform level settings required to power Firefox CI). The code all seems pretty reasonable. Thank you for going hard on Python 3 for new projects!

Taking a step backward, it seems like the problem being solved is generic to other TC deployments and a generic tool for configuring platform-level features should be part of the redeployability project. That's *massive* scope bloat from the targeted solution for Firefox implemented here though and I don't want to encourage you to go down that road. But it would help to know if you envision this code evolving to be generic enough to support other projects, whether this code needs to be a bridge until such a generic tool exists, etc. The answer drastically changes how I'd go about reviewing the code.

For example, if you tell me this is meant as a short-term bridge or a project to be used only by TC admins who know what they're doing, then I think the current approach of everything living inline in Python code is very acceptable. But there are a lot of scenarios where I could find myself saying things like "I think things would be clearer if there were a separation between code and data: something like how taskgraph works."

Regardless of how the data is defined, things in files like ciadmin/generate/project_roles.py could definitely use a lot more inline docs. For each resource, I'd like to know why it is present and what its expected use is for. This information is currently scattered across probably dozens of bugs and random IRC conversations and email threads. It would be great if the code capture more of the history for why things are the way they are. It may be excessive, but I think comments for literally every single item (like individual scopes) would be useful. Obviously code speaks for itself and this project is better than "the config lives in the database." So comprehensive docs feel like follow-up fodder after an initial implementation has landed.

Since this is Python 3, the used `from __future__` lines are no-ops and can be removed.

Thank you for using attrs. It's one of my favorite new Python packages. Time will tell if I abandon it in favor of https://docs.python.org/3.7/library/dataclasses.html. Sadly, you probably don't want to make Python 3.7 a requirement to run this code, as 3.7 isn't released yet :)

In a perfect world, I'd like to think this could live inside mozilla-central and we'd just always use the latest code on mozilla-central. But mozilla-central isn't currently very friendly to projects that aren't Firefox proper. So having this live in a separate repo is a reasonable decision.

Given that TC admins will run this code, the security of the code is kinda important. I'd love to see a pip requirements file with pinned hashes so we have integrity checking on downloaded packages. I've recently started using pip-compile (from https://github.com/jazzband/pip-tools) for managing pip requirements files to make things more turnkey.

I hope this is useful. It's possible I missed the boat completely since I'm coming into this without much context for the full scope of the problem space!

Flags: needinfo?([e-mail])
	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 20 • 4 days ago

Thanks!

I expect at some point the generic version of this will be supplied by something like a Terraform plugin, so yes, this is intended to be Firefox-specific (and that's why it's in Python, not JS).

This problem just isn't very declarative, and trying to make it declarative (I have an etherpad a few comments back where I worked on that) results in something that checks the "declarative" box but very emphatically un-checks the "readable" box, to the point where even I would not know how to modify it to support anything new.  So like taskgraph, I've tried to take a hybrid data-and-code approach that I think will produce a more readable, maintainable product.  On Jonas's advice, I've broken the problem into a data-and-code "generation" phase, and a very declarative apply phase (which is just the no-op `ci-admin diff` so far).

I began writing this (months ago) as a mach command in-tree, but it was an awkward fit with the trains: unlike the rest of Firefox code, this code had a "privileged" version which was the one in mozilla-central, and on any later branch (beta, release, ..) was unused and likely different from reality. I'm not 100% convinced the code doesn't belong in ci-configuration, but tomprince's argument -- that if any code is accessing ci-configuration from outside the repo, then *all* code should access it that way -- seemed sensible so I followed it.

I'll pin the versions once it's ready to go.  Thanks for the pip-tools pointer!

	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 21 • 2 days ago

I'm not going to use cattr.  First:

>>> import cattr
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/dustin/p/ci-admin/sandbox/lib/python3.5/site-packages/cattr/__init__.py", line 2, in <module>
    from .converters import Converter, UnstructureStrategy
  File "/home/dustin/p/ci-admin/sandbox/lib/python3.5/site-packages/cattr/converters.py", line 2, in <module>
    from typing import (Mapping, Sequence, Optional,
ImportError: cannot import name '_Union'

that doesn't inspire confidence!  But more importantly, it doesn't seem to support renaming properties on structure/unstructure.  I'd like to have that for hook metadata (which is in a `metadata` property in JSON, but should be flat in the Python data structure).  It's possible to fix this with a hook, but that hook's body will be identical to the toJSON/fromJSON functions I already have.

	Dustin J. Mitchell [:dustin] pronoun: he
(Assignee) 	
Comment 22 • 19 hours ago

I'm thinking about making a scope-grants.yml in ci-configuration; docs would be something like the below.  Thoughts?  I'm worried about how to express all of this while still taking advantage of parameterized roles.

---

Grants have the general form:

  - grant: <permission(s)>
    to: <subject(s)>

Either can be a list, and will be expanded; that is,

  - grant: [perm1, perm2]
    to: [subj1, subj2]

is equivalent to

  - grant: perm1
    to: subj1
  - grant: perm2
    to: subj1
  - grant: perm1
    to: subj2
  - grant: perm2
    to: subj2

The available permissions are:

  "some-scope"
    -- raw strings are assumed to be scopes

  {trigger-hook: {level: <l>, actionPerm: <ap>}}
    -- trigger the given hook at the given SCM level

  {route: <route>}
    -- queue:route:<route>

  {index: <namespace>}
    -- both queue:route:index.<namespace> and index:insert-task:<namespace>

The available subjects are:

  {mozilla-group: <group>}
    -- users in the Mozilla LDAP group <group> (to work correctly, the role
    `mozilla-group:<group>` must contain scope `assume:project:gecko:group-access:<group>`)
  ## project:gecko:group-access:<group>`

  {project: <proj>, trust_domain: <td>, level: <level>}
    -- all operations on project with alias <proj>.  Use "*" to match all
    projects, in which case '<..>' in the resulting scopes will be
    parameterized to the project alias.  If the trust domain or level are
    omitted, applies to all trust domains or levels.
  ## all of {role_root}:branch:{domain}:level-{level}:<proj>

  {project: <proj>, only: {pushes: true}, trust_domain: <td>, level: <level>}
    -- Like {project: ..}, but only for pushes (not actions or cron tasks)
  ## assume:{role_root}:push:{domain}:level-{level}:{alias}'

  {project: <proj>, only: {action: <action>}, trust_domain: <td>, level: <level>}
    -- Like {project: ..}, but only for the given action (the action name can be
    a * pattern)
  ## ??

  {project: <proj>, only: {cron: <cron>}, trust_domain: <td>, level: <level>}
    -- Like {project: ..}, but only for the given cron task (the cron name can be
    a * pattern, such as nightly-*)
  ## ??

  {level: <l>, trust_domain: <td>}
    -- All projects at level <l>; if trust_domain is specified, applies only
    to projects in that trust domain, otherwise to all
  ## moz-tree:level:<l>:<td>

  {project-feature: <feature>, trust_domain: <td>, level: <l>}
    -- All projects at level <l> and trust_domain <td> with feature <feature>
    If level or trust_domain are omitted, applies to all.  `<..>` in any
    granted scopes will refer to the project alias.
  # {role_root}:feature:{feature}:{domain}:level-{level}:*

Comment 1

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

Jonas, is this something you could work on?

This is a "medium" priority, as gecko's complex role configuration is blocking a few changes that would help make things more secure, and in particular blocking the big change to remove repo scopes from user credentials after actions-as-hooks lands.

Comment 2

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

also, I have no idea why all that junk got copied into comment 0.

Comment 3

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

OK, so I think I have an idea how to implement this.  However, I don't want to screw it up.  So, to test, I'd like to start with a bunch of scopesets and make sure they expand to the same thing before and after.

Which means I need a way to simulate scope expansion locally.

Happily, I already wrote something (in Python!) back when doing the parameterized roles work.  I even still have it on a spare git branch.

Unhappily, its performance doesn't quite match that of taskcluster-auth.  I suspect that one of the more subtle changes we made in the parameterized role support is to blame, but I haven't quite figured it out.

Anyway, with that in place, I'll build a temporary harness to do some before-and-after comparisons (using the "current" and "generated" resources), so I can be sure that the set of grants I invent has exactly the same effect as the current hodge-podge.

Comment 4

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

A bunch of progress in https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-admin/pushloghtml?changeset=7191ec114ed27eb9c6b1ca887b8a84a5e65f9870

Notably:
 * manage moz-tree:* roles
 * simulate scope expansion locally!
 * `ci-admin check`
 * ability to do some basic has-a-scope / doesn't-have-a-scope checks that will make the sec team happy /cc ulfr

This last bit is pretty cool.  I only really added `ci-admin check` so that I could verify my scope-grants.yml doesn't have any effect, but then I needed a better excuse to land it.  Checking scopes is just such an excuse!

Comment 5

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

Attachment: Bug 1465842: refactor ciadmin.util into sub-modules

Comment 6

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

Attachment: Bug 1465842: Add scope utilities

This includes a resolver that emulates taskcluster-auth, and will be used to
make some checks about the result of proposed role changes.

Depends on D2436

Comment 7

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

Attachment: Bug 1465842: generate moz-tree:* roles, too

Depends on D2437

Comment 8

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

Attachment: Bug 1465842: add 'ci-admin check', upgrade requirements

TODO: once .taskcluster.yml is merged, run this as well

Depends on D2438

Comment 9

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

Attachment: Bug 1465842: test for signing scopes

This is never going to be a super-secure check, but will help make sure we
don't do anything boneheaded.

Depends on D2439

Comment 10

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

Attachment: Bug 1465842: test that changes have no effect DO NOT LAND

Depends on D2439

Comment 11

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

Attachment: Bug 1465842: move ciconfig into sub-modules

If only because this avoids naming conflicts between modules defining the data
structure and modules generating resources based on those data structures..

Depends on D2442

Comment 12

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

Attachment: Bug 1465842: implement grants.yml

In the same commit, this removes all other code that sets repo roles or user
roles, as otherwise they try to generate the same resources.  Checks in place
during development (but not to land) ensure that actual scopes do not change.

Depends on D2443

Comment 13

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

Attachment: Bug 1465842: add grants and remove other scope specifications

Comment 14

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

OK, I think I phabricated correctly, more-or-less.  What I'm looking for is a 50% review of these changes.  I'm also hoping to get Brian and Pete more familiar with ci-admin / ci-configuration.

This patch sequence accomplishes a few things:

 * centralizes administration of Firefox-related (taken broadly to include nss and TB) roles
 * makes those roles self-serve (via patch - someone still has to apply them)
 * allows some automated access checks like secops would prefer
 * prepares us to reduce scopes assigned to users (once actions as hooks are landed..)

I have a few things on my TODO list, on which I'd like feedback, too:

* add `ciadmin check` to .taskcluster.yml and run it periodically
* support multiple grant files (so, maybe grants.yml has `grants-from: [nss-grants.yml, action-grants.yml, ..]`)
* support utility roles ({grant: [<scopes>] to: {role: <roleId>}}) and manage 
  - lists of workers
  - sccache access
  - nightly-scopes (currently hand-managed!)
  - etc.
* break out the huge grants at the beginning of grants.yml into more manageable, topical chunks
  -- possibly using utility roles like lists of workers
* use parameterized roles automatically
  -- detect when a scope fits one of a few patterns, and automatically use a parameterized role fitting that pattern

I'm not sure the parameterized roles is necessary.  As written, this is going to put a *lot* of scopes in the various repo roles' `.scopes` property, but the resulting `.expandedScopes` will not really change (that's what the checks verify). The scope resolver in tc-auth basically deals with expanded scopes.  So I'm not sure the new arrangement would be any less efficient, or even any harder to read in the UI.  Thoughts?

I don't intend to land this until I get back on August 19, so take your time :)

Comment 15

[Phabricator Automation]

Comment on attachment 8995316 [details]
Bug 1465842: refactor ciadmin.util into sub-modules

Brian Stack [:bstack] has approved the revision.

https://phabricator.services.mozilla.com/D2436

Comment 16

[Phabricator Automation]

Comment on attachment 8995317 [details]
Bug 1465842: Add scope utilities

Brian Stack [:bstack] has approved the revision.

https://phabricator.services.mozilla.com/D2437

Comment 17

[Brian Stack [:bstack]]

Comment on attachment 8995318 [details]
Bug 1465842: generate moz-tree:* roles, too

Brian Stack [:bstack] has approved the revision.

Comment 18

[Brian Stack [:bstack]]

Comment on attachment 8995319 [details]
Bug 1465842: add 'ci-admin check', upgrade requirements

Brian Stack [:bstack] has approved the revision.

Comment 19

[Brian Stack [:bstack]]

Comment on attachment 8995321 [details]
Bug 1465842: test for signing scopes

Brian Stack [:bstack] has approved the revision.

Comment 20

[Brian Stack [:bstack]]

Comment on attachment 8995325 [details]
Bug 1465842: add grants and remove other scope specifications

Brian Stack [:bstack] has approved the revision.

Comment 21

[Brian Stack [:bstack]]

Comment on attachment 8995323 [details]
Bug 1465842: move ciconfig into sub-modules

Brian Stack [:bstack] has approved the revision.

Comment 22

[Brian Stack [:bstack]]

Comment on attachment 8995324 [details]
Bug 1465842: implement grants.yml

Brian Stack [:bstack] has approved the revision.

Comment 23

[Brian Stack [:bstack]]

I've reviewed all but the DO NOT LAND one. If that needs a review, let me know on monday!

Comment 24

[Tom Prince [:tomprince]]

Comment on attachment 8995316 [details]
Bug 1465842: refactor ciadmin.util into sub-modules

Tom Prince [:tomprince] has approved the revision.

Comment 25

[Tom Prince [:tomprince]]

Comment on attachment 8995317 [details]
Bug 1465842: Add scope utilities

Tom Prince [:tomprince] has approved the revision.

Comment 26

[Tom Prince [:tomprince]]

Comment on attachment 8995323 [details]
Bug 1465842: move ciconfig into sub-modules

Tom Prince [:tomprince] has approved the revision.

Comment 27

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

I talked to Pete today, and he will give this a look.  I also checked in with jhford and wcosta, and I'd love to hear any thoughts either of you have, but I won't block waiting on it (so don't feel pressured to comment).

Comment 28

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

Looking at this now!

Comment 29

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

Comment on attachment 8995316 [details]
Bug 1465842: refactor ciadmin.util into sub-modules

Pete Moore [:pmoore][:pete] has approved the revision.

Comment 30

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

Comment on attachment 8995317 [details]
Bug 1465842: Add scope utilities

Pete Moore [:pmoore][:pete] has approved the revision.

Comment 31

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

Comment on attachment 8995319 [details]
Bug 1465842: add 'ci-admin check', upgrade requirements

Pete Moore [:pmoore][:pete] has approved the revision.

Comment 32

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

Comment on attachment 8995321 [details]
Bug 1465842: test for signing scopes

Pete Moore [:pmoore][:pete] has approved the revision.

Comment 33

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

Comment on attachment 8995322 [details]
Bug 1465842: test that changes have no effect DO NOT LAND

Pete Moore [:pmoore][:pete] has approved the revision.

Comment 34

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

Comment on attachment 8995323 [details]
Bug 1465842: move ciconfig into sub-modules

Pete Moore [:pmoore][:pete] has approved the revision.

Comment 35

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

Comment on attachment 8995324 [details]
Bug 1465842: implement grants.yml

Pete Moore [:pmoore][:pete] has approved the revision.

Comment 36

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

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #14)
> * add `ciadmin check` to .taskcluster.yml and run it periodically

I'll do this once I add .taskcluster.yml (bug 1486431)

> * support multiple grant files (so, maybe grants.yml has `grants-from:
> [nss-grants.yml, action-grants.yml, ..]`)

I'm not sure this is a good idea, since the file contents are fetched remotely as raw files.  Maybe that's a bad idea, but I don't want to revisit the question right now.

> * support utility roles ({grant: [<scopes>] to: {role: <roleId>}}) and
> manage 
>   - lists of workers
>   - sccache access
>   - nightly-scopes (currently hand-managed!)
>   - etc.

Follow-up - bug 1486544.

> * break out the huge grants at the beginning of grants.yml into more
> manageable, topical chunks
>   -- possibly using utility roles like lists of workers

Follow-up work, to be chipped away at as necessary.

> * use parameterized roles automatically
>   -- detect when a scope fits one of a few patterns, and automatically use a
> parameterized role fitting that pattern

I don't think this is a good idea -- it sounds fragile and complex without any real benefit.

Comment 37

[Tom Prince [:tomprince]]

Comment on attachment 8995321 [details]
Bug 1465842: test for signing scopes

Tom Prince [:tomprince] has approved the revision.

Comment 38

[Tom Prince [:tomprince]]

Comment on attachment 8995324 [details]
Bug 1465842: implement grants.yml

Tom Prince [:tomprince] has approved the revision.

Comment 39

[Tom Prince [:tomprince]]

Comment on attachment 8995318 [details]
Bug 1465842: generate moz-tree:* roles, too

Tom Prince [:tomprince] has approved the revision.

Comment 40

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

Comment on attachment 8995325 [details]
Bug 1465842: add grants and remove other scope specifications

Pete Moore [:pmoore][:pete] has approved the revision.

Comment 41

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

OK!  Everything is landed, and I'm now applying the changes.  I'm trying to do so in an order that will not cause anything to fail.

Comment 42

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

Changes landed for try at 16:19UTC

Comment 43

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

..and a try task created successfully:
  https://tools.taskcluster.net/groups/fX75fyZrS_WPmSwxwh1qGw/tasks/fX75fyZrS_WPmSwxwh1qGw/details

It appears that treeherder ingestion is behind, as I don't see a D for this or any other try job since the top of the hour, but that is decidedly unrelated to this work.  And now that I've written that, I see more D's -- so it's just treeherder lag.

Comment 44

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

Attachment: roles.json

listRoles output as of this time (in case I need to roll back :crossed fingers:)

Comment 45

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

I've now seen an autoland task created.  No other errors in the mozilla-taskcluster logs.  So I think this is good!