Closed Bug 1463522 Opened 6 years ago Closed 6 years ago

Include hash of .taskcluster.yml in action hookId

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: dustin, Assigned: dustin)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1463522 - only read .taskcluster.yml once;
59 bytes, text/x-review-board-request
tomprince
: review+
Details
Bug 1463522 - include .taskcluster.yml hash in hookId;
59 bytes, text/x-review-board-request
tomprince
: review+
Details 
      No description provided.
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 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 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>`?
Attachment #8979694 - Flags: review?(mozilla) → review-
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.
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 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 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 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]
Attachment #8980754 - Attachment is obsolete: true
Attachment #8980754 - Flags: review?(mozilla)
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]
every time I delete 'import yaml', 'import hashlib' comes back, and vice versa.  Hg is really being bad today.
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]
(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 on attachment 8979694 [details]
Bug 1463522 - include .taskcluster.yml hash in hookId;

https://reviewboard.mozilla.org/r/245838/#review253048
Attachment #8979694 - Flags: review?(mozilla) → review+
Comment on attachment 8979693 [details]
Bug 1463522 - only read .taskcluster.yml once;

https://reviewboard.mozilla.org/r/245836/#review253054
Attachment #8979693 - Flags: review?(mozilla) → review+
Pushed by dmitchell@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a5953dc4b3d7
https://hg.mozilla.org/mozilla-central/rev/a7e274c2aadd
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: