Proposal to separate Validation / Filtering from TriggerBinding

[bobcatfish]

Expected Behavior

As much as it makes sense (our design should be as simple as possible and no simpler! :D) we should try to make our CRDs:

• Have one clear job
• Minimize repetition (e.g. in Proposal to move TriggerBinding-TriggerTemplate connection out of TriggerBinding definition #55 we made TriggerBindings more reusable b/c we figured otherwise folks could end up having to create the same TriggerBindings over and over again).

In light of #55 (see "actual behaviour" below), I propose that:

• Folks may want to reuse "validation"/filtering logic across many EventListeners
• Validation/filtering logic is a different responsibility than extracting values from events (which seems to be TriggerBindings main job)

Ideas

We could add a Filter type, maybe even something as specific as HTTPFilter, something like:

apiVersion: tekton.dev/v1alpha1
kind: HTTPFilter
metadata:
  name: requester-is-github-push
spec:
  inputParams:
  - name: secret
    description: The git webhook secret
  headerMatches:
  - X-GitHub-Event: push
  - X-GitHub-Secret: $(inputParams.secret)

Or we could take this one step further and have both an HTTPFilter and HTTPValidation but that feels like it's going too far?

Then TriggerBinding would become just:

apiVersion: tekton.dev/v1alpha1
kind: TriggerBinding
metadata:
  name: my-triggerbinding
spec:
  # I removed conditionals also!
  outputParams:
  - name: gitbranch
    value: $(event.ref)
  - name: gitrevision
    value: $(event.head_commit.id)
  - name: gitrepositoryurl
    value: $(event.repository.url)

At this point TriggerBinding might make sense to rename to something like EventParser?

And then EventListener would be:

kind: EventListener
metadata:
  name: my-eventlistener
spec:
  bindings:
    filterRef: requester-is-github-push
      params:
      - name: secret
        value: somehow-from-kube-secret
    triggerTemplateRef: my-triggertemplate
    triggerBindingRef: my-triggerbinding
    conditions:
       - name: is-master
         params:
           - $(my-triggerbinding.gitbranch): refs/heads/master

TriggerBindingRef would be responsible for extracting values that are available to conditions - and maybe filters? Or if that's crazy then conditions can go back into TemplateBinding :D

Or something like that! What I do like though is keeping the responsibilities separate, and using EventListener to bring them together.

Actual Behavior

In our most recent design (#55 (comment)) we have added some values to the TriggerBinding which are for filtering (i.e. only create these resources if conditions is true) and validation (i.e. only create these resources if headerMatches is true, if it isn't true, this indicates that the requester is bogus :O - this can be viewed as a type of filtering too?)

This means TriggerBinding now has 3 jobs:

1. Extract values from events and make them available via outputparams
2. Extract values from events and use them to filter via conditions
3. Extract values from events and use them to validate the caller via headerMatches

apiVersion: tekton.dev/v1alpha1
kind: TriggerBinding
metadata:
  name: my-triggerbinding
spec:
  inputParams:
  - name: secret
    description: The git webhook secret
  - name: branch
    description: The git branch
    default: master
  headerMatches:
  - X-GitHub-Event: push
  - X-GitHub-Secret: $(inputParams.secret)
  conditionals:
  - $(event.ref): refs/heads/$(inputParams.branch)
  outputParams:
  - name: gitrevision
    value: $(event.head_commit.id)
  - name: gitrepositoryurl
    value: $(event.repository.url)

Additional Info

Related issues:

• Adding conditionals Add Conditionals to Resource Creation #49
• Renaming TriggerBinding Proposal to rename TriggerBinding #62 <-- depending on how this issue goes, it might become more obvious if TriggerBinding should be renamed and to what :D
• Making TriggerBindings reusable Proposal to move TriggerBinding-TriggerTemplate connection out of TriggerBinding definition #55

[dibyom]

I like this proposal especially the bit about resources especially doing a single thing well.

Off the top of my head:

• If we go ahead with this proposal, I like the EventParser name a lot!

• I'm not sure I fully understand the difference between Filters and Conditions and why we need both. It seems like they are both validating some conditions are true before the resources get created. Maybe we could use just one (I like Filter)

• The spec.bindings.filterRef syntax in the EventListener YAML is almost exactly the same as using a Condition in a PipelineTask (just replace filterRef with conditionRef). The spec.bindings.conditions syntax also looks similar enough. It seems like we could reuse Conditionals here (maybe just as an implementation detail if we want to keep distinct types like HTTPFilter)

[khrm]

/assign

[dibyom]

From the WG today, we are already doing portions of this in #45 and #89 i.e. moving the validation away from TriggerBinding (thanks @khrm ) So, @bobcatfish suggested we close out this proposal and maybe open a new one in the future for Filtering only!