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?

[ghost]

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)

[vtereso]

Current status from WG: We have already integrated validation of Triggers using tasks, which validates the event source. This issue should create a separate conditions (plural) field for the event trigger for the following reasons:

• In order to allow for reusable components, we wouldn't want to combine validation with potentially multiple conditions
• If there is any validation, this should happen prior to conditions
• It is advantageous to log these differently
• Easily distinguish the optional validation from optional conditions (makes validating a single validation task easier as well)

[vtereso]

Whoever picks this up should also add the test builder(s) for the validation task fields within the EventListener. It seem to be agreed on that OR conditions should be resolved internally so that all conditions are additive booleans and short circuit Trigger actuation on failure.

[khrm]

/assign

I am taking this task. Will send pr for #117 and then work on this. According to discussion, it's schedule for 0.2 release but may be we can push for it to be release with 0.1. Will see.

[ncskier]

We should be wary of the Pod proliferation problem discussed in issue #125 which led to the creation of the interceptors.

[kav]

Might be worth considering using JsonPath here as well as (#178). I'm definitely for lightweight conditional filtering without making more pods. As a user though whichever language is used to navigate and select object properties should likely also be used for conditionals if that functionality is available.

[vtereso]

@kav Regarding this, allowing conditional filters also allows the potential for people to use them as validators, which was the reason we replaced them with interceptors.

[kav]

That makes sense to a degree but when I looked at interceptors as an option they felt a bit heavy when all I really wanted was a bit of string equality comparison on a property or two, though I'll admit it does seem like a pretty robust solution to validation, filtering, and any clean up or data conversion

[vtereso]

I meant to specify conditional filters as containers, my apologies for any confusion. I have been back and forth on the container issue, but in consideration for the security, I have come back to my skepticism. I agree interceptors might be too heavy weight in many circumstances and we could add string comparisons (and maybe regex), which might handle most cases. This has been discussed since the initial MVP so I don't entirely consider this "extra behavior" for Triggers, but ideally, I would keep Triggers as slim as possible so as not to reinvent the wheel.

[vtereso]

As per the design document, we will be using CEL to satisfy this conditional/filter request so that interceptors are reusable. This should also enable people to get away with simple validation as well outside of VCS like GitHub, GitLab, etc.

[bigkevmcd]

@dibyom I had a very rough go at this last week

https://github.com/bigkevmcd/triggers/tree/master/pkg/interceptors/cel

[dibyom]

Awesome! @wlynch mentioned that we could consider a "core" CEL interceptor as an alternative to a top level filters field

[bigkevmcd]

Now that CEL interceptor is in there, I'd like to see if we can't agree on how to do CEL munging, i.e. the original values implementation in early days of that branch.

  triggers:
    - name: cel-trig
      interceptor:
        cel:
          filter: "headers.match('X-GitHub-Event', 'pull_request')"
          values:
            "pr.url": body.pull_request.url
            "pr.short_sha": truncate(body.head.sha, 7)

I'd propose something similar, I don't have a better name than values, but I think it should be a list of key/values, with "key" and "expression" as the keys for each thing to update.

so...

triggers:
- name: cel-trig
  interceptor:
  cel:
    filter: "headers.match('X-GitHub-Event', 'pull_request')"
    values:
      - key: pr.url
        expression: body.pull_request.url
      - key: pr.short_sha
        expression: truncate(body.head.sha, 7)

This would be used in the same way as the original values implementation, i.e. if the filter was successful, using sjson to set the keys in the body to the values returned by the expressions.

This allows an interceptor to add custom keys to the body, or, modify existing ones if it wants to.

[dibyom]

Yeah, using to modify body will be an interesting feature though I think we should track it in its own issue. I'll close this and open a new one for that!