Ability for Github Service to trigger hooks

[ahal]

Motivation

If the Github service can trigger arbitrary hooks, then we can define decision tasks in a central place (like fxci-config). This eliminates the need to copy/paste Decision task definitions around from project to project. It also hides the complexity from end users who may not be familiar with JSON-e or task schemas.

This proposal would also subsume the need for https://github.com/taskcluster/taskcluster-rfcs/blob/main/rfcs/0182-taskcluster-yml-remote-references.md

Prototype

I created a working prototype over at:
https://github.com/ahal/taskcluster/tree/github-custom-pulse-messages

See .taskcluster.yml there for an example format. This prototype causes the Github service to emit a pulse event for each item defined in the messages array. The event payload contains additional context defined in .taskcluster.yml, and can be used to render any hooks that get fired in response to the event.

Possible Solutions

There are many different ways this could be implemented, and even more ways we could structure the .taskcluster.yml. Here are a few high level possibilities:

1. Context is defined in .taskcluster.yml and gets added to the existing pulse events that the Github service emits. This feels the cleanest to me, but I ran into some implementation problems while prototyping. The existing events get emitted very early on, before we read .taskcluster.yml. They also get emitted regardless of whether tasks were created or not. So this approach might require some heavier refactoring than the others.

2. Context is defined in .taskcluster.yml and gets added to a pulse event over a new exchange that gets triggered in the appropriate place. This is simpler, but requires creation of a new exchange for the Github service.

3. Context is defined in .taskcluster.yml and the Github service triggers the hooks directly using the triggerHook API. This is nearly identical to 2, except we cut out pulse from the equation. This could be even simpler, but could also be a bit less flexible in what's possible.

Problems

Versioning

The biggest gotcha here is that when a project needs to specify some new context, we may need to adjust the decision task definition at the same time. And it may not always be possible to do so in a backwards compatible manner.

Updating all Taskcluster consumers at once is not feasible, so we'd want some kind of versioning for the Decision task hooks so we can roll things out gradually. This would be possible by having each version of the hook subscribe to a topic that includes that same version. Then repos could update by adjusting the version in their .taskcluster.yml files.

This adds some maintenance complexity, and will require some logic in fxci-config to generate multiple versions of hooks.

Chain of Trust

It's unclear if there will be chain of trust implications to this. CoT ignores anything that happens before the Decision task, so in theory it should be fine. Though if we have multiple versions of the Decision task definition, we would need to make sure CoT can work across all of them. This doesn't feel like a new problem though, as we already have many variations on a Decision task definition today.. each project uses slightly different ones already!

Security

I haven't thought very hard about the security impacts of this yet. We'd need to make sure we aren't regressing the security model in some way by doing this.

[jcristau]

I created a working prototype over at:
https://github.com/ahal/taskcluster/tree/github-custom-pulse-messages

See .taskcluster.yml there for an example format.

.taskcluster.yml there doesn't seem to be modified?

It's unclear if there will be chain of trust implications to this. CoT ignores anything that happens before the Decision task, so in theory it should be fine. Though if we have multiple versions of the Decision task definition, we would need to make sure CoT can work across all of them. This doesn't feel like a new problem though, as we already have many variations on a Decision task definition today.. each project uses slightly different ones already!

CoT wants to rebuild the decision task's definition itself though, to verify it's legit, and it does that by grabbing the .tc.yml from the repo and rendering it using a particular context, then comparing against the actual task's definition.

One other thing I don't see addressed here is action and cron tasks?

[lotas]

thanks @ahal for the PoC, I have few questions.

Thinking about users outside of the fxci context it might look confusing and not obvious on the first glance that those exchanges/messages would be triggering something that other service would be expecting. Or phrasing it differently if I'm setting up new repo, what those messages tell me? Maybe it is just a naming issue (for me)?

And since we are using exchanges, why can't we reuse existing exchanges (pull-request, push, release), how does adding named topics help here?

Slightly related but maybe tangent - why don't we filter events here right away? Would allow to remove the complexity from the decision task, for example:

messages:
  - topic: new-pr
    on: pull-request.open
  - topic: test-only 
    on: push
    branch: [main, dev]

Still, I'm confused why not triggering hooks directly from the github service to avoid running through pulse? Would also solve the versioning issue, since it will be explicit which hook to call, for example:

hooks:
  - groupId: hookGroupId
    hookId: decision_v1
    context:
      environment: production
      buildType: release
      commitSha: {$eval: event.after}

internally github would simply do hooksClient.triggerHook(hookGroupId, decision_v1, { payload + context })

I would personally be less confused looking at such config as to the one with messages unless I'm missing something :)

Maybe even combining this with filtering (which might not cover all possible filtering needs)

hooks:
  - groupId: ci-tasks
    hookId: push-decision
    on: [push, pull-request.opened, pull-request.synchronized]

[bhearsum]

I'm a huge fan of this idea overall; I agree that it would accomplish what https://github.com/taskcluster/taskcluster-rfcs/blob/main/rfcs/0182-taskcluster-yml-remote-references.md set out to do, but in a much nicer way!

What is responsible for rendering the full .taskcluster.yml when Pulse is involved? The taskcluster-github service no longer does it AFAICT. Whatever it is, we'll need a way to make sure that any rendering failures of the full .taskcluster.yml can be reported back in a useful way -- ideally as comments in GitHub as they currently are. (This wouldn't be a concern if we use triggerHook directly AFAICT.)

I'm not sure if this is really a concern or not, but it strikes me that whatever route we take here, there's now an implied dependency between taskcluster-github and fxci-config. Obviously this is already the case for scopes and other things, but this seems like it would take it to a point where the taskcluster-github service can be completely non-functional (and possibly non-debuggable without log access) if fxci-config is misconfigured. I don't think this should block this work from proceeding, but it feels notable enough to call out. Concretely, I think it's important to make sure that we preserve the ability to debug issues like "why aren't my tasks firing?" without having privileged access to the taskcluster services.

Context is defined in .taskcluster.yml and the Github service triggers the hooks directly using the triggerHook API. This is nearly identical to 2, except we cut out pulse from the equation. This could be even simpler, but could also be a bit less flexible in what's possible.

Do you have concrete things that we wouldn't be able to do if we go this route, or this more of a general flexibility in the future concern?

Still, I'm confused why not triggering hooks directly from the github service to avoid running through pulse? Would also solve the versioning issue, since it will be explicit which hook to call

This would also make it easier to understand overall; it removes one system from the architecture, and provides a direct link between the .taskcluster.yml and something you can find in the TC UI when debugging. (This alone shouldn't push us here, but it's a nice side effect.)

[ahal]

CoT wants to rebuild the decision task's definition itself though, to verify it's legit, and it does that by grabbing the .tc.yml from the repo and rendering it using a particular context, then comparing against the actual task's definition.

Ah fair! There would be something in the .tc.yml that links back to the hook that was used. In my prototype, this would be the topic, e.g.. it might be decision-v3. So CoT could parse this and obtain the corresponding hook task definition via the API.

I'm not suggesting the prototype is a good format.. whatever we come up with, it should be very easy to map back to the hook definition.

One other thing I don't see addressed here is action and cron tasks?

Good call out. Build decision would also need to know which version of the hook to use.. It could call trigger hook manually. So similar to CoT, it would need to map back to the hook being used.

[ahal]

Or phrasing it differently if I'm setting up new repo, what those messages tell me? Maybe it is just a naming issue (for me)?

Please don't get too tripped up by the naming.. I really just wanted to prove out the high level concept was feasible. I agree the naming in the prototype is bad.

And since we are using exchanges, why can't we reuse existing exchanges (pull-request, push, release), how does adding named topics help here?

I think that would be ideal, it was option 1 in the description, but there were some implementation challenges I didn't want to deal with.

Slightly related but maybe tangent - why don't we filter events here right away? Would allow to remove the complexity from the decision task, for example:

We could. I personally wouldn't find the feature that useful as we need to do more filtering than just the high level Github event, but if it seems like a nice general feature that others would want I'm not opposed.

Still, I'm confused why not triggering hooks directly from the github service to avoid running through pulse? Would also solve the versioning issue, since it will be explicit which hook to call

Yep, that's option 3 :)

If we can't think of a reason that a pulse message would be useful outside of triggering hooks, then yes I think this would be the cleanest method. The versioning isn't particularly hard with my prototype though.. A repo would set e.g:

topic: decision-v3

then the corresponding decision-v3 hook would bind to primary.#.decision-v3 (or whatever).

I mainly thought using pulse messages would allow for more flexible integration points.. E.g, maybe instead of a hook, someone would have a service that listens to pulse and calls createTask directly. But I'm likely overthinking things :p

[ahal]

It just occurred to me that the context portion, could be a top-level key in the .tc.yml. Then it could be used in the JSON-e context to render tasks, as well as passed into the pulse body / triggerHook input (whichever we end up going with).

I think there was even an issue or RFC lying around somewhere to allow defining variables outside of the tasks key already...

Edit: Found it:
taskcluster/taskcluster#6008

[ahal]

What is responsible for rendering the full .taskcluster.yml when Pulse is involved? The taskcluster-github service no longer does it AFAICT. Whatever it is, we'll need a way to make sure that any rendering failures of the full .taskcluster.yml can be reported back in a useful way -- ideally as comments in GitHub as they currently are. (This wouldn't be a concern if we use triggerHook directly AFAICT.)

The hook service would do the rendering.. but yes this is a very good point. If we used pulse I'm not sure how the Github service would be able to retrieve failures, so this makes me lean very heavily to option 3.

Another benefit of using triggerHook, is that the task ID would be returned in the response. So we wouldn't need to pass taskGroupId in as context anymore in order to track the build in the Github checks UI.

Do you have concrete things that we wouldn't be able to do if we go this route, or this more of a general flexibility in the future concern?

I was thinking maybe some company would want to have a service that listens to pulse and calls createTask in response rather than doing it all in JSON-e via hooks.. But this is likely YAGNI, at least for Mozilla.

This would also make it easier to understand overall; it removes one system from the architecture, and provides a direct link between the .taskcluster.yml and something you can find in the TC UI when debugging. (This alone shouldn't push us here, but it's a nice side effect.)

Ok, I think I'm sold on using triggerHook already :)

[ahal]

I'm not sure if this is really a concern or not, but it strikes me that whatever route we take here, there's now an implied dependency between taskcluster-github and fxci-config. Obviously this is already the case for scopes and other things, but this seems like it would take it to a point where the taskcluster-github service can be completely non-functional (and possibly non-debuggable without log access) if fxci-config is misconfigured. I don't think this should block this work from proceeding, but it feels notable enough to call out. Concretely, I think it's important to make sure that we preserve the ability to debug issues like "why aren't my tasks firing?" without having privileged access to the taskcluster services.

This doesn't seem too concerning, especially if we stick to the triggerHook approach (which I think would provide debugging on par with what we have today). If we used pulse and lost the ability to add comments when the .tc.yml fails to render.. then I'd agree. I also realize you commented this before I said I was sold on using triggerhook :)

[bhearsum]

I'm not sure if this is really a concern or not, but it strikes me that whatever route we take here, there's now an implied dependency between taskcluster-github and fxci-config. Obviously this is already the case for scopes and other things, but this seems like it would take it to a point where the taskcluster-github service can be completely non-functional (and possibly non-debuggable without log access) if fxci-config is misconfigured. I don't think this should block this work from proceeding, but it feels notable enough to call out. Concretely, I think it's important to make sure that we preserve the ability to debug issues like "why aren't my tasks firing?" without having privileged access to the taskcluster services.

This doesn't seem too concerning, especially if we stick to the triggerHook approach (which I think would provide debugging on par with what we have today). If we used pulse and lost the ability to add comments when the .tc.yml fails to render.. then I'd agree. I also realize you commented this before I said I was sold on using triggerhook :)

I think this is still a theoretical concern with triggerHook as well. Obviously we can easily maintain the comments on error with that, but it's still not necessarily easy or obvious to find the .taskcluster.yml. (Perhaps we should provide a link to it on error?)

Again, this is absolutely not a blocker or even necessarily an issue, just a notable change.