1227606 - hookGroupId and hookId id format limitations

Status: RESOLVED FIXED

(Taskcluster :: Services, defect)

Description

[Jonas Finnemann Jensen (:jonasfj)]

We need to specify identifier limitations here:
https://github.com/taskcluster/taskcluster-hooks/blob/945f9d18c33094abbbbab7edfada8858715ab01b/routes/v1.js#L27
And in JSON schema..

Similar to how it's done here:
https://github.com/taskcluster/taskcluster-queue/blob/c82c5d0ba580e6389a97c748ed4b088e572f6117/routes/v1.js#L90-L97

I propose that we use:
var GENERIC_ID_PATTERN  = /^[a-zA-Z0-9-_]{1,22}$/;

Maybe we can allow hookId to be all printable ascii chars. But having short
ids is useful for having them fit into pulse messages, task definitions, etc.
Of course they are slightly harder to scope.

---
If we don't we clearly need to specify exactly what they are. Maybe "all printable ascii chars" is okay. I'm just no a huge fan of / in the hookGroupId, though it would work as long as it's URI encoded which API clients will do automatically for URL parameters.

Comment 1

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

I'm OK with GENERIC_ID_PATTERN.

I did verify that slashes work, but it's true that they are ambiguous.

Comment 2

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

I think we should just limit this to printable ascii (same a scopes), but not limit its length.  We don't currently include these identifiers in pulse routes, and I don't see why we would.

Jonas, concur?

Comment 3

[Alexandra Silva (:alexandra_)]

(In reply to Dustin J. Mitchell [:dustin] from comment #2)
> I think we should just limit this to printable ascii (same a scopes), but
> not limit its length.  We don't currently include these identifiers in pulse
> routes, and I don't see why we would.
> 
> Jonas, concur?

keeping in eye on it.

Comment 4

[Jonas Finnemann Jensen (:jonasfj)]

We picked GENERIC_ID_PATTERN back in the day, because we can fit those in to pulse routing keys.
I suggested it because I wanted us to publish pulse message whenever hooks are created, modified, deleted or triggered.

If you guys want to use printable ascii (max 255 chars), then I would suggest:
 A) merge hookGroupId and hookId into one identifier
 B) make pulse exchanges include a sha256(hookId)[0:32] in the routing key

note: totally unrelated, pulse exchanges were never implemented.

Comment 5

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

> note: totally unrelated, pulse exchanges were never implemented.

Alex is working on adding this now, and there's really no good reason to include the hookId / hookGroupId in the routing key.  So I think we can leave these as they are (well, with some minor restrictions to hookId, which is currently just "string").

Comment 6

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

https://github.com/taskcluster/taskcluster-hooks/pull/91

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/taskcluster/taskcluster-hooks

https://github.com/taskcluster/taskcluster-hooks/commit/a9e03a25e9ef890db3e54b7631728e1508ac013b
Bug 1227606 - allow longer hookGroupId, restrict charset of hookId

This unifies both on 64 characters, alphanumeric plus `-` and `_`.  The
idea is that this is more open than 22 characters, but not so open that
we will later want to dial it back.