Add Conditionals to Resource Creation

[vtereso]

Expected Behavior

Users should be able to specify a Conditional against event payload values to ensure that resources are generated dependent on the payload data. For context, webhook POST data from GitHub specifies the source organization/repository/branch. Users will want to make TriggerBindings that execute against particular branches, etc.

Actual Behavior

Currently, this is no conditional creation.

Additional Info

Add Conditionals to the TriggerBindingSpec. Values should be interpolated from the event into the fields being asserted. Documentation should be included.

[vincent-pli]

So, the Condition means general Condition, not the Condition in tekton pipeline?

[vtereso]

So, the Condition means general Condition, not the Condition in tekton pipeline?

I believe we should be using Tekton pipeline Conditions for this.

[vincent-pli]

I feel it's a little heavy, Conditions in pipeline will launch container to do the comparing.
Image that, when received a event, we need create pod only for string comparison...

[vtereso]

I feel it's a little heavy, Conditions in pipeline will launch container to do the comparing.
Image that, when received a event, we need create pod only for string comparison...

@vincent-pli That's true. I made a point in counter argument to the Conditionals initially because I can't really see much besides string comparison to be done, especially here. Tekton is probably better served to used Conditions because the work has already been kicked off and the time it takes the Condition to complete is minuscule in comparison to the entire workload. Where as here, many events are likely to come in and we don't want to get bottle-necked this way. I see both sides because maybe the the payload has some special value that has to be computed upon/set somewhere and only providing some internal string match might not be sufficient.

[vtereso]

This work is still blocked, but I think discussion in the issues like this is great. Opinions? @bobcatfish @ncskier @iancoffey @EliZucker

[dlorenc]

So to clarify, today we have something like:

http:
  headerMatch:
    foo=bar

We could extend this to support more advanced filtering with something like:

http:
  headerMatch:
    foo=bar
  bodyMatch:
    a.b.c=foo

Where a.b.c is a nested json field.

Other questions:

• What type of matching?
• Do we need regex?
• Not equals? Equals?

[vtereso]

The header match field is sort of a contrived way of allowing access to the headers without having to introduce a new interpolation variable (like ${header}) since ${event} extracts from the payload (no access to headers currently). This was the delineation between headers and conditionals in a way. If we were to introduce direct/interpolation access to the headers, maybe conditions/conditonals could encapsulate headers? Maybe having an explicit headers field is still favorable?

Adding a usecase here in support of regex: On a previous team we used a regex branch match to determine whether a CI pipeline should run in response to a push event or not. I forget the exact pattern but it was something relatively simple/naive like "ignore branches named master, qa, and dev": ^((?!(master|qa|dev)).*)$

[bobcatfish]

I made a point in counter argument to the Conditionals initially because I can't really see much besides string comparison to be done, especially here.

I have a feeling that no matter what we do, people are going to want to express more complex Conditions eventually.

Here's an example of some slightly more complicated logic a person might want to use: https://github.com/wlynch/cel-playground/tree/get-commit#example-expressions

ce.type == "com.github.pull_request.create"
ce.source == "foo"
ce.source != "foo" && ce.type != "bar"
ce.source.matches("com\\.github\\..*") # backslash must be escaped

That's using a language called CEL and it's conceivable that folks will very quickly want to:

1. Choose the language they can use for expressing this type of thing (e.g. maybe you want to use Rego instead https://www.openpolicyagent.org/docs/latest/policy-language/)
2. Want to be able to reuse conditions, e.g. they wouldn't want to copy + paste the above for all their trigger setups - and related to this comment Enable TriggerBindings to validate requests (e.g. GitHub) #45 (comment) if we're gonna use something like headersMatch for security ( + @dlorenc ) then I think folks will want to have a reusuable chunk of logic they can easily apply to all their trigger setups
3. Folks are going to need to do even more complicated stuff, for example @wlynch has pointed out that in some cases you'll actually need to make requests back to GitHub for further information before determining whether to trigger or not (e.g. 'is the user that created this PR is such and such org')

I feel it's a little heavy, Conditions in pipeline will launch container to do the comparing.
Image that, when received a event, we need create pod only for string comparison...

I think we can find ways to optimize this, for example I think @dibyom has proposed in Pipelines having ways to more easily express common operations. And I'm not against having a regex type of Condition that maybe gets executed in some optimized way.

Are we certain that this is too heavy? I'd be interested to see what the startup time is for a pod that does nothing but run a very tiny container, e.g. a regex container.

[vtereso]

Are we certain that this is too heavy?

Definitely not, but it was my main/only consideration in opposition to utilizing containers.

I think providing some optimized conditional checks down the line (should they be necessary) is a great point. Also, it wouldn't be very secure to match some potentially secret authentication header in plaintext. Using a container, a secret or such could be mounted. If nothing else than the sake of simplicity (rather than engineering matching piece meal), I think containers are the way to go 👍

[dibyom]

Some relevant discussion here as well: #45 (comment)