diff --git a/README.md b/README.md index ad0eea35..f6500c84 100644 --- a/README.md +++ b/README.md @@ -247,6 +247,10 @@ The class has the following methods: * `resources.filter(pattern)` - return a new Resources object containing only resources matching the given regular expression string * `resources.map(functor)` - return a new Resources object, with fuctor applied to each resource. This is typically used in modifiers. +Resources must be unique -- tc-admin cannot manage multiple hooks with the same name, for example. +However, some resource kinds support merging, where adding a resource with the same identity as one that already exists "merges" it into the existing resource. +See the description of roles, below. + The remaining classes represent individual resources. Each has an `id` formed from its kind and the unique identifier for the resource in the Taskcluster. For example, `Hook=release-hooks/beta-release`. @@ -282,7 +286,7 @@ The items in `bindings` are instances of `Binding(exchange=.., routingKeyPattern ```python from tcadmin.resources import Role -hook = Role( +role = Role( roleId=.., description=.., scopes=(.., ..)) @@ -290,6 +294,19 @@ hook = Role( As with hooks, `scopes` must be a tuple (not a list) of strings. +Roles can be merged if their descriptions match. +The resulting role contains the union of the scopes of the input roles. +This functionality makes management of roles easier in cases where different parts of the generation process may add scopes to the same role. + +For example: + +```python +resources.add(Role(roleId="my-role", description="My Role", scopes=["scope1"])) +resources.add(Role(roleId="my-role", description="My Role", scopes=["scope2"])) +``` + +This will result in a single Role with scopes `["scope1", "scope2"]`. + ### WorkerPool ```python diff --git a/tcadmin/resources/resources.py b/tcadmin/resources/resources.py index ac9f7308..cf3c9469 100644 --- a/tcadmin/resources/resources.py +++ b/tcadmin/resources/resources.py @@ -9,7 +9,6 @@ import blessings import functools import textwrap -import itertools from memoized import memoized from sortedcontainers import SortedKeyList @@ -51,6 +50,15 @@ def to_api(self): "Construct a payload for Taskcluster API methods" raise NotImplementedError + def merge(self, other): + """ + Construct a 'merged' resource, given another resource with the same kind and id. + + Generally, resources can be merged if all fields either match or can be combined + in some way, such as taking the union of scopesets + """ + raise RuntimeError("Cannot merge resources of kind {}".format(self.kind)) + @property def kind(self): "The kind of this instance" @@ -101,21 +109,37 @@ class Resources: ) def __attrs_post_init__(self): + self._by_id = {r.id: r for r in self.resources} + if len(self._by_id) != len(self.resources): + raise RuntimeError("duplicate resources passed to Resources constructor") self._verify() - def add(self, resource): + def add(self, resource, _skip_verify=False): "Add the given resource to the collection" if not self.is_managed(resource.id): raise RuntimeError("unmanaged resource: " + resource.id) + + # if the resource already exists, try to merge it and remove the + # previous version from the list of resources (SortedKeyList does not + # support in-place replacement of items) + if resource.id in self._by_id: + existing = self._by_id[resource.id] + resource = self._by_id[resource.id].merge(resource) + idx = self.resources.index(existing) + del self.resources[idx] + + self._by_id[resource.id] = resource self.resources.add(resource) - self._verify() + + if not _skip_verify: + self._verify() def update(self, resources): "Add the given resources to the collection" for resource in resources: if not self.is_managed(resource.id): raise RuntimeError("unmanaged resource: " + resource.id) - self.resources.update(resources) + self.add(resource, _skip_verify=True) self._verify() def manage(self, pattern): @@ -141,15 +165,6 @@ def map(self, functor): def _verify(self): "Verify that this set of resources is legal (all managed, no duplicates)" - # search for duplicates, taking advantage of sorting - pairs = zip( - itertools.chain([None], (r1.id for r1 in self)), (r2.id for r2 in self) - ) - dupes = [a for (a, b) in pairs if a == b] - if dupes: - unique_dupes = sorted(set(dupes)) - raise RuntimeError("duplicate resources: " + ", ".join(unique_dupes)) - unmanaged = sorted([r.id for r in self if not self.is_managed(r.id)]) if unmanaged: raise RuntimeError("unmanaged resources: " + ", ".join(unmanaged)) diff --git a/tcadmin/resources/role.py b/tcadmin/resources/role.py index 0781cce2..1f9fa816 100644 --- a/tcadmin/resources/role.py +++ b/tcadmin/resources/role.py @@ -8,6 +8,7 @@ from .resources import Resource from .util import description_converter, scopes_converter, list_formatter +from ..util.scopes import normalizeScopes @attr.s @@ -30,3 +31,12 @@ def from_api(cls, api_result): def to_api(self): "Construct a payload for use with auth.createRole or auth.updateRole" return {"description": self.description, "scopes": self.scopes} + + def merge(self, other): + assert self.roleId == other.roleId + if self.description != other.description: + raise RuntimeError( + "Descriptions for {} to be merged differ".format(self.id) + ) + scopes = normalizeScopes(self.scopes + other.scopes) + return Role(roleId=self.roleId, description=self.description, scopes=scopes) diff --git a/tcadmin/tests/test_resources_resources.py b/tcadmin/tests/test_resources_resources.py index ce73d588..72a4628c 100644 --- a/tcadmin/tests/test_resources_resources.py +++ b/tcadmin/tests/test_resources_resources.py @@ -18,6 +18,17 @@ class Thing(Resource): value = attr.ib(type=str) +@attr.s +class MergeableThing(Resource): + thingId = attr.ib(type=str) + value = attr.ib(type=str) + + def merge(self, other): + return MergeableThing( + thingId=self.thingId, value=self.value + "|" + other.value + ) + + @attr.s class ListThing(Resource): listThingId = attr.ib(type=str) @@ -136,6 +147,14 @@ def test_resources_filter(): assert [r.thingId for r in coll] == ["abc", "abd"] +def test_resources_merge(): + "Resources merge when added" + coll = Resources([], [".*"]) + coll.add(MergeableThing(thingId="a", value="artichoke")) + coll.add(MergeableThing(thingId="a", value="aardvark")) + assert coll.resources[0] == MergeableThing(thingId="a", value="artichoke|aardvark") + + def test_resources_to_json(): "Resources.to_json produces the expected data structure" rsrcs = Resources( @@ -184,11 +203,19 @@ def test_resources_manages(): assert not rsrcs.managed.matches("Thing=y") -def test_resources_verify_duplicates_prohibited(): +def test_resources_verify_duplicates_prohibited_constructor(): "Duplicate resources are not allowed" with pytest.raises(RuntimeError) as exc: Resources([Thing("x", "1"), Thing("x", "1")], [".*"]) - assert "duplicate resources: Thing=x" in str(exc.value) + assert "duplicate resources" in str(exc.value) + + +def test_resources_verify_duplicates_prohibited(): + "Duplicate resources are not allowed" + rsrcs = Resources([Thing("x", "1")], [".*"]) + with pytest.raises(RuntimeError) as exc: + rsrcs.add(Thing("x", "1")) + assert "Cannot merge resources of kind Thing" in str(exc.value) def test_resources_verify_unmanaged_prohibited(): diff --git a/tcadmin/tests/test_resources_role.py b/tcadmin/tests/test_resources_role.py index 8e06054f..455c313f 100644 --- a/tcadmin/tests/test_resources_role.py +++ b/tcadmin/tests/test_resources_role.py @@ -4,6 +4,7 @@ # v. 2.0. If a copy of the MPL was not distributed with this file, You can # obtain one at http://mozilla.org/MPL/2.0/. +import pytest import textwrap from tcadmin.resources.resources import Resource @@ -51,3 +52,32 @@ def test_role_from_api(): assert role.roleId == "my:role-id" assert role.description == api_result["description"] assert role.scopes == ("scope-a", "scope-b") + + +def test_role_merge_simple(): + "Roles with matching descriptions can be merged" + r1 = Role(roleId="role", description="test", scopes=["a"]) + r2 = Role(roleId="role", description="test", scopes=["b"]) + merged = r1.merge(r2) + assert merged.roleId == "role" + assert merged.description.endswith("test") + assert merged.scopes == ("a", "b") + + +def test_role_merge_normalized(): + "Scopes are normalized when merging" + r1 = Role(roleId="role", description="test", scopes=["a", "b*"]) + r2 = Role(roleId="role", description="test", scopes=["a", "bcdef", "c*"]) + merged = r1.merge(r2) + assert merged.roleId == "role" + assert merged.description.endswith("test") + assert merged.scopes == ("a", "b*", "c*") + + +def test_role_merge_different_descr(): + "Descriptions must match to merge" + r1 = Role(roleId="role", description="test1", scopes=["a"]) + r2 = Role(roleId="role", description="test2", scopes=["b"]) + with pytest.raises(RuntimeError) as exc: + r1.merge(r2) + assert "Descriptions for Role=role to be merged differ" in str(exc.value) diff --git a/tcadmin/util/scopes.py b/tcadmin/util/scopes.py index b755c6b7..28c8570a 100644 --- a/tcadmin/util/scopes.py +++ b/tcadmin/util/scopes.py @@ -6,8 +6,6 @@ import re -from ..resources import Role - class Resolver: """ @@ -23,6 +21,8 @@ def __init__(self, roles): def from_resources(cls, resources): """Construct an instance from a Resources instance, ignoring any non-Role resources""" + from ..resources import Role + roles = {} for resource in resources: if isinstance(resource, Role):