1463522 - Include hash of .taskcluster.yml in action hookId

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Comment 1

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

Attachment: Bug 1463522 - only read .taskcluster.yml once;

PyYAML is not fast, so the fewer times we parse the same file, the better

Review commit: https://reviewboard.mozilla.org/r/245836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245836/

Comment 2

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

Attachment: Bug 1463522 - include .taskcluster.yml hash in hookId;

Review commit: https://reviewboard.mozilla.org/r/245838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245838/

Comment 3

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

Oops, I didn't add any context here.  The idea is in bug 1415868 comment 77.  Basically, if we put a hash in the hookId, then changes to .taskcluster.yml can ride the trains.

Comment 4

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

https://github.com/taskcluster/taskcluster-admin/pull/21

Comment 5

[Tom Prince [:tomprince]]

Comment on attachment 8979693 [details]
Bug 1463522 - only read .taskcluster.yml once;

https://reviewboard.mozilla.org/r/245836/#review252666

Does the memoization make an appreciable difference?

::: taskcluster/taskgraph/actions/registry.py:39
(Diff revision 1)
>          return False
>      return True
>  
>  
> +@memoize
> +def read_taskcluster_yml_raw(template):

Given the behavior, this name doesn't seem appropriate (not generic enough).

::: taskcluster/taskgraph/actions/registry.py:46
(Diff revision 1)
> +        return f.read()
> +
> +
> +@memoize
> +def read_taskcluster_yml(template):
> +    return yaml.safe_load(read_taskcluster_yml_raw(template))

There is already `taskgraph.util.yaml.load_yaml`.

Comment 6

[Tom Prince [:tomprince]]

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

https://reviewboard.mozilla.org/r/245838/#review252652

This seems like a reasonable approach.

I'm not convinced that having a proliferation of hooks is better than having a generic hook that calls into the tree to generate the real action, like happens for cron. But I'm not going to object to this approach.

::: taskcluster/taskgraph/actions/registry.py:55
(Diff revision 1)
> +@memoize
> +def hash_taskcluster_yml(template):
> +    """Generate a short hash identifier for the content of .taskcluster.yml"""
> +    h = hashlib.sha256()
> +    h.update(read_taskcluster_yml_raw(template))
> +    return h.hexdigest()[:10]

Maybe this could use `taskgraph.util.hash.hash_path`?

Do we expect people to be interacting with these hooks directly? That is, is there a reason to trunctate the hash?

::: taskcluster/taskgraph/actions/registry.py:225
(Diff revision 1)
>                  trustDomain = graph_config['trust-domain']
>                  level = parameters['level']
> +                tcyml_hash = hash_taskcluster_yml(graph_config.taskcluster_yml)
> +
> +                # the tcyml_hash is prefixed with `_` in the hookId, so users will be granted
> +                # hooks:trigger-hook:project-gecko/in-tree-action-3-myaction_*; if another

This looks like it is using all of `/`, `:`, `-`, and `_` as separators. That seems excesive.

Maybe something like
`hooks:trigger-hook:project-<trust-domain>/in-tree-action/level-3/<action>/<hash>`?

Comment 7

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

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

https://reviewboard.mozilla.org/r/245838/#review253010

::: taskcluster/taskgraph/actions/registry.py:55
(Diff revision 1)
> +@memoize
> +def hash_taskcluster_yml(template):
> +    """Generate a short hash identifier for the content of .taskcluster.yml"""
> +    h = hashlib.sha256()
> +    h.update(read_taskcluster_yml_raw(template))
> +    return h.hexdigest()[:10]

The hashes are part of the hookId, which is limited to 64 characters.  It's only necessary to ensure uniqueness, so 10 characters is probably sufficient.

Comment 8

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

Regarding the one generic hook -- the key here is that the hooks service is the "escalation point" and defines exactly the task that can be executed (using the hook task template) and limits the allowed inputs (using the triggerSchema).  If we were to make one generic hook, then it would have all the privileges, and would be responsible for a litany of security checks: verifying the caller has permission to run this hook, verifying that the hook and the target taskgraph are at the same level, verifying that the inputs are suitable, and then starting the hook task with only the appropriate scopes.  The result would, I think, leave the foxsec team wondering why I wasted so much time accomplishing no measurable increment in security.

Comment 9

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

Attachment: Bug 1463522 - include .taskcluster.yml hash in hookId;

Review commit: https://reviewboard.mozilla.org/r/246920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246920/

Comment 10

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

Comment on attachment 8979693 [details]
Bug 1463522 - only read .taskcluster.yml once;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245836/diff/1-2/

Comment 11

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

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245838/diff/1-2/

Comment 12

[Code Review Bot [:reviewbot]]

Comment on attachment 8979693 [details]
Bug 1463522 - only read .taskcluster.yml once;

https://reviewboard.mozilla.org/r/245836/#review253016


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/actions/registry.py:18
(Diff revision 2)
>  from slugid import nice as slugid
>  from types import FunctionType
>  from collections import namedtuple
>  from taskgraph import create
>  from taskgraph.config import load_graph_config
> -from taskgraph.util import taskcluster
> +from taskgraph.util import taskcluster, yaml

Error: Redefinition of unused 'yaml' from line 12 [flake8: F811]

Comment 13

[Code Review Bot [:reviewbot]]

Comment on attachment 8980754 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

https://reviewboard.mozilla.org/r/246920/#review253018


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/actions/registry.py:19
(Diff revision 1)
>  from slugid import nice as slugid
>  from types import FunctionType
>  from collections import namedtuple
>  from taskgraph import create
>  from taskgraph.config import load_graph_config
>  from taskgraph.util import taskcluster, yaml

Error: Redefinition of unused 'yaml' from line 12 [flake8: F811]

::: taskcluster/taskgraph/actions/registry.py:49
(Diff revision 1)
>  
> +@memoize
> +def hash_taskcluster_yml(template):
> +    """Generate a short hash identifier for the content of .taskcluster.yml"""
> +    h = hashlib.sha256()
> +    h.update(read_taskcluster_yml_raw(template))

Error: Undefined name 'read_taskcluster_yml_raw' [flake8: F821]

Comment 14

[Code Review Bot [:reviewbot]]

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

https://reviewboard.mozilla.org/r/245838/#review253020


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/actions/registry.py:13
(Diff revision 2)
>  
>  import json
>  import os
>  import re
>  import yaml
>  import hashlib

Error: 'hashlib' imported but unused [flake8: F401]

::: taskcluster/taskgraph/actions/registry.py:19
(Diff revision 2)
>  from slugid import nice as slugid
>  from types import FunctionType
>  from collections import namedtuple
>  from taskgraph import create
>  from taskgraph.config import load_graph_config
> -from taskgraph.util import taskcluster, yaml
> +from taskgraph.util import taskcluster, yaml, hash

Error: Redefinition of unused 'yaml' from line 12 [flake8: F811]

Comment 15

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

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245838/diff/2-3/

Comment 16

[Code Review Bot [:reviewbot]]

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

https://reviewboard.mozilla.org/r/245838/#review253024


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/actions/registry.py:13
(Diff revision 3)
>  
>  import json
>  import os
>  import re
>  import yaml
> +import hashlib

Error: 'hashlib' imported but unused [flake8: F401]

::: taskcluster/taskgraph/actions/registry.py:19
(Diff revision 3)
>  from slugid import nice as slugid
>  from types import FunctionType
>  from collections import namedtuple
>  from taskgraph import create
>  from taskgraph.config import load_graph_config
> -from taskgraph.util import taskcluster, yaml
> +from taskgraph.util import taskcluster, yaml, hash

Error: Redefinition of unused 'yaml' from line 12 [flake8: F811]

Comment 17

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

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245838/diff/3-4/

Comment 18

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

every time I delete 'import yaml', 'import hashlib' comes back, and vice versa.  Hg is really being bad today.

Comment 19

[Code Review Bot [:reviewbot]]

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

https://reviewboard.mozilla.org/r/245838/#review253028


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/actions/registry.py:18
(Diff revision 4)
>  from slugid import nice as slugid
>  from types import FunctionType
>  from collections import namedtuple
>  from taskgraph import create
>  from taskgraph.config import load_graph_config
> -from taskgraph.util import taskcluster, yaml
> +from taskgraph.util import taskcluster, yaml, hash

Error: Redefinition of unused 'yaml' from line 12 [flake8: F811]

Comment 20

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

Comment on attachment 8979693 [details]
Bug 1463522 - only read .taskcluster.yml once;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245836/diff/2-3/

Comment 21

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

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245838/diff/4-5/

Comment 22

[Tom Prince [:tomprince]]

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #8)
> Regarding the one generic hook -- [...]

I was suggesting generic over the task body, not over the scopes. Like cron calls in-tree code to generate task graphs. I know you rejected that earlier because having two tasks slowed things down, but it would reduce the complexity here, somewhat.

That is, there would be one hook for each action, not one for each combination of action and .taskcluster.yml.

Comment 23

[Tom Prince [:tomprince]]

Comment on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

https://reviewboard.mozilla.org/r/245838/#review253048

Comment 24

[Tom Prince [:tomprince]]

Comment on attachment 8979693 [details]
Bug 1463522 - only read .taskcluster.yml once;

https://reviewboard.mozilla.org/r/245836/#review253054

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5953dc4b3d7
only read .taskcluster.yml once; r=tomprince
https://hg.mozilla.org/integration/autoland/rev/a7e274c2aadd
include .taskcluster.yml hash in hookId; r=tomprince

Comment 26

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/a5953dc4b3d7
https://hg.mozilla.org/mozilla-central/rev/a7e274c2aadd