JS expressions in Trigger spec

[slinkydeveloper]

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Reflects the work done in #3771 in the broker trigger spec. For more details, look at #3771

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: slinkydeveloper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [slinkydeveloper]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[n3wscott]

what happens when both are given?

[slinkydeveloper]

I wrote that at the end of this paragraph, the trigger MUST do an and of both results. Do you propose to rephrase it?

[n3wscott]

Can you give a couple examples of javascript expressions? I am not familiar with this.

[slinkydeveloper]

Added in the doc

[n3wscott]

I would rather use Rego. https://www.openpolicyagent.org/docs/v0.11.0/language-reference/

It is designed to do this kind of filtering and they have a cross-compiler to web assembly that is stupid fast for filter functions.

[n3wscott]

cc: @grantr

[slinkydeveloper]

It is designed to do this kind of filtering and they have a cross-compiler to web assembly that is stupid fast for filter functions.

That's not easy as it seems, I've looked into it: you need a wasm engine, then you need to implement this ABI https://www.openpolicyagent.org/docs/latest/wasm/#exports, then you need to bring the event into the wasm context (which requires a serde step). JS is far easier to integrate and easier also for the users to look at it.

[slinkydeveloper]

Then rego brings the non-trivial problem that you need to compile the expressions in wasm modules, invoking the rego compiler, and store them somewhere

[n3wscott]

I don't think it is less trivial than embedding a javascript interpreter, and for this to be able to be well understood we need to define or point to a spec of everything you can do in a jsExpression similar to how Rego lists out all the language features.

Plus we don't need to implement rego, we pull OPA in as a sidecar as one impl.

[slinkydeveloper]

I don't think it is less trivial than embedding a javascript interpreter

Look at the code, it's one file of 100 lines of code 😄 And i'm pretty sure it's the very same amount of locs you need in Java to embed a js engine, in rust and so on... All the code to integrate the engine consists in making the event available from the filter code through the engine, then everything else is covered by the js engine itself. Any sidecar solution requires far more, both on sidecar and on filter code + all complexity of configs and tests too. And another point to consider is that this proposal doesn't prevent to have in future rego too.

we need to define or point to a spec of everything you can do in a jsExpression

Not really, I was wondering that all we should say here is: the js engine MUST support EcmaScript 5.1 (which is a requirement every js engine out there supports today) and should serve cloudevent as event (and i think i already covered that part). Maybe we should be more specific on the type coercion between cloudevents types and js types, but overall that's all i think we need here, we don't need to specify all the things you can do in JS

Plus we don't need to implement rego, we pull OPA in as a sidecar as one impl.

That adds quite some overhead to the filtering process and complexity too... I prefer to have something simple that I can embed straight in my code, other solutions that requires some external process are more complex to implement and most notably adds overhead for serializing/deserializing and IPC.

[nachocano]

I'm not convinced on the "MUST support both attributes filter and jsExpression".

I actually like the filter dialects the CloudSubscriptions spec talks about. See https://github.com/cloudevents/spec/blob/master/subscriptions-api.md#323-filter-dialects.
The MUST is to support a basic one (Exact, Prefix and Suffix match).
More advanced dialects can be made optional. I can easily see JS being an optional advanced dialect. Maybe REGO, CEL, or whatever, other ones, and so on...

Overall, I think you will be able to reduce contention if you decouple Trigger from a mandatory JS "dialect", and just frame the problem as Trigger with optional dialects... I think conversations will move forward faster, and we will also be more aligned with the Serverless WG (which in general I think is a good thing).

[slinkydeveloper]

TBH I wasn't aware of these filter dialects from subscriptions spec. Still they don't seem to cover basic nested logic and operations on types (in this doc i explain what a user should be able to express https://docs.google.com/document/d/1Pz2vaLWKUrMQDLNyDW7ksjgLG4GAxBY546dT7_jF5Rk/edit?usp=sharing)

My take on optional and mandatory is that, I have no strong opinion if vendors wants to add their filtering capabilities to their system, but IMHO eventing vanilla should be bundled with a filtering capability that covers a 90% of basic use cases. Not providing a "good enough" bundled filtering capability will make too big the divergence between different implementations (basically, if a user wants to port trigger a to another vendor, he needs to rewrite all the filtering logic...)

JS "dialect"

I hardly name it a JS dialect: In the spec here i'm just stating that the expression is ECMAScript 5.1+ and you access to event using event.attrx, I was very careful to avoid defining something that cannot trivially implemented with an already existing js engine.

[n3wscott]

I would rather look at what is missing in the subscription spec and upstream changes and take what is there for now.

[n3wscott]

crazy thought... what if you proposed a javascript dialect to the spec https://github.com/cloudevents/spec/blob/master/subscriptions-api.md#323-filter-dialects

[slinkydeveloper]

/retest

[slinkydeveloper]

/retest

[slinkydeveloper]

/retest

[knative-test-reporter-robot]

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-integration-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests:

test/e2e.TestApiServerSourceV1Alpha2EventTypes

[slinkydeveloper]

/retest

[matzew]

Why do we not want to ship an engine?

I think this is would be a good message if eventing comes with one engine, and not w/ some "crap" that is

• overriden by every vendor
• and there has a different syntax

I see zero values on that approach.

So, why against one engine?

That said:

Overall, I think you will be able to reduce contention if you decouple Trigger from a mandatory JS "dialect", and just frame the problem as Trigger with optional dialects... I think conversations will move forward faster, and we will also be more aligned with the Serverless WG (which in general I think is a good thing).

I get this point too -

[matzew]

I think if there is NO unique API, this would be bad.

I don't really see, why we can not ship this

[n3wscott]

My issue with adding this is eventing should be some common denominator that can be projected into other systems that implement this, and @nachocano has a very good point with the subscription dialects from CloudEvents Subscription Spec. Being spec based means our adoption of an implementation is portable. At the very least we should aim to be directly translatable into the CloudEvents subscription spec and dialects. If they need more dialects, we can propose more, or have our own...

[slinkydeveloper]

@n3wscott I think we can implement the filtering in the CloudEvents Subscription spec too and even if we plan to expand it, it's still a long road ahead to reach the capabilities of JS or any other expression language. And the CloudEvents Subscription filter might be just another filter capabilities (like our attributes)

[ruromero]

I think one common use case would be to avoid loopback events in generic services, for example I want to subscribe to any event except the ones my service generates (i.e. ce-source is my-generic-service). That doesn't seem possible with CloudEvents Subscriptions ATM.

filter:
  - type: not
    attribute: source
    value: org.example.service:myservice

or using jsExpressions

filter:
  jsExpression: "source !== 'org.example.service:myservice'"

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

[slinkydeveloper]

Closed as explained here: #3771 (comment)