Skip to content

current.hooks.fetch_hooks skips hook groups when managed patterns are regex-escaped, causing phantom additions #327

Description

@hneiva

current.hooks.fetch_hooks uses a plain string .startswith() check against regex managed-pattern strings to decide whether to query a hook group. When a hook is managed by an exact, re.escape-d id (the normal way to manage a single resource in an externally-managed namespace), the escaping breaks that string check, the whole group is skipped, and hooks that are actually managed and present in Taskcluster are never fetched, so they show up as phantom + additions in diff (and a subsequent apply would createHook on already-existing hooks).

Affected version: tc-admin 5.2.1

Affected code

permalink

tcadmin/current/hooks.py, fetch_hooks:

for hookGroupId in (await hooks.listHookGroups())["groups"]:
    idPrefix = "Hook={}/".format(hookGroupId)          # e.g. "Hook=project-fuzzing/"
    # if no hook with this hookGroupId is managed, skip it
    is_managed = any(m.startswith(idPrefix) for m in resources.managed)   # (a)
    is_managed = is_managed or resources.is_managed(idPrefix)             # (b)
    if not is_managed:
        continue
    for hook in (await hooks.listHooks(hookGroupId))["hooks"]:
        hook = Hook.from_api(hook)
        if resources.is_managed(hook.id):
            resources.add(hook)

resources.managed holds regex patterns, but check (a) treats them as literal strings via .startswith()

Root cause

Managing a single resource by exact id typically registers
re.escape("Hook=project-fuzzing/bugmon") + "$"Hook=project\-fuzzing/bugmon$

  • Check (a) fails: "Hook=project\-fuzzing/bugmon$".startswith("Hook=project-fuzzing/") is False. re.escape rewrote the - in the group name as \-
  • Check (b) fails: is_managed("Hook=project-fuzzing/") is False because the exact-id pattern requires the full id (…/bugmon$), and any broad pattern for that namespace is typically excluded (externally-managed)

→ the group is skipped, its hooks are never added to current, and every individually-managed hook in it appears as a spurious addition.

Reproduction

  1. Manage a hook by exact id in a group whose name contains -, e.g. resources.manage(re.escape("Hook=project-fuzzing/bugmon") + "$"), and have that hook exist in Taskcluster.
  2. Run tc-admin diff.
  3. The hook is reported as a + addition even though it exists and is managed.

Impact

diff shows phantom additions for individually-managed hooks in externally-managed / hyphenated groups.
apply would attempt to create hooks that already exist (409 conflict).

Suggested fix

The group-relevance shortcut shouldn't do a string .startswith() on regex patterns. Options are:

  • Drop the (a) micro-optimization and rely solely on the per-hook is_managed(hook.id) filter (correct, at the cost of listing hooks for more groups); or
  • Make the group check regex-aware. e.g. test whether any managed pattern can match something under idPrefix rather than string-prefixing the raw patterns.

Note: I've implemented a workaround in fxci-config, but it only covers the externally managed hooks for Firefox-ci TC.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions