fix: Improve Knative trigger filter

[christophd]

• Fixes Unable to specify any CloudEvent attributes or extensions, except for type #5537:
  Support filter attributes other than event type (e.g. source, subject, extensions)
• Fixes property "type" must be provided when reading from the Broker #5529:
  Allows empty filter to consume the full event stream
• Fixes Knative Trigger creation is only based on event type attribute #5446:
  Knative Trigger creation is only based on event type attribute
• Fixes Consistently support "cloudEventsType" property in Pipes source/sink #5577:
  Consistently support "cloudEventsType" property in Pipes source/sink
• Update documentation and improve Knative Kamelet/Pipe user guide

Release Note

fix(#5537): Support filter attributes other than event type
fix(#5529): Allows empty filter to consume the full event stream
fix(#5446): Knative Trigger creation is only based on event type attribute
fix(#5577): Consistently support "cloudEventsType" property in Pipes source/sink

[github-actions]

✔️ Unit test coverage report - coverage increased from 39.6% to 39.7% (+0.1%)

[squakez]

I have some design doubt about certain coupling we're adding. I need to review this deeply to understand what's the design implication of such change.

[squakez]

I am not sure it is a good way to configure it. The binding should know nothing about traits and its configuration I think.

[christophd]

I think the binding is supposed to adjust/configure the traits by design as it is part of the binding interface API and the trait config set by the binding explicitly gets merged into the integration traits.

See https://github.com/apache/camel-k/blob/main/pkg/util/bindings/api.go#L41

[squakez]

Yes, I think the original idea was to use a way to represent certain options in order to later merge in the Integration [1] (ie, knative trait only). However, this changed when we have done the trait refactoring, creating a direct dependency between traits and bindings [2]. This was in fact only used in KnativeURI translator so far. I have the feeling that we should rework this part and find a different approach instead of extending a potential wrong design. What I mean is that the "trait" concept is an Integration thing and extending it widely creates coupling we can regret in the future. @lburgazzoli wdyt?

[1] 19b66cf#diff-abb6a6ae31c506ba2b634a29eba8930cd19766cdec4d7d3aae43fbc3b4a08996
[2] a92aaa4#diff-abb6a6ae31c506ba2b634a29eba8930cd19766cdec4d7d3aae43fbc3b4a08996

[squakez]

BTW, I think it would be wiser to split each issue you're fixing in separate PR. Maybe some of them can be merged straight away. If there is some code change in common for each, maybe you can start with a first PR and then issue any dependant PR once the others are approved and merged.

[christophd]

Sorry, for the bigger PR. Yes, I thought about splitting into multiple PRs but it all somehow touches the same bits so I kept it in one single PR. Please let me know if it works for you as I am having a look into the failing E2E tests right now

[squakez]

Sorry, for the bigger PR. Yes, I thought about splitting into multiple PRs but it all somehow touches the same bits so I kept it in one single PR. Please let me know if it works for you as I am having a look into the failing E2E tests right now

No problem. I say more in the interest to maintain separate discussions and let the other fixes to go through independently.

[github-actions]

✔️ Unit test coverage report - coverage increased from 39.6% to 39.7% (+0.1%)

[github-actions]

✔️ Unit test coverage report - coverage increased from 39.8% to 39.9% (+0.1%)

[github-actions]

✔️ Unit test coverage report - coverage increased from 39.8% to 39.9% (+0.1%)

[github-actions]

✔️ Unit test coverage report - coverage increased from 39.8% to 39.9% (+0.1%)

[github-actions]

✔️ Unit test coverage report - coverage increased from 39.8% to 39.9% (+0.1%)

[github-actions]

✔️ Unit test coverage report - coverage increased from 39.8% to 39.9% (+0.1%)

[christophd]

@squakez all tests are green now. I have also added some E2E test to verify the different Knative trigger possibilities (e.g. no filter, filter by ce-source and so on)

In terms of design considerations I think the binding provider is meant to create/configure the integration from a Pipe that may reference other K8s resources. The binding provider translates these resource references into Camel components and the YAML route DSL that is being used in the integration. The traits are part of the integration so the binding provider has access to it, which is not a new design introduced in this PR.

We can open a new issue to discuss this current design if you want. WDYT?

[squakez]

Please, verify if @lburgazzoli can provide his thoughts about this before going ahead. Thanks.

[lburgazzoli]

Does it also relate to #3378 ?

[christophd]

@lburgazzoli this PR does not include the support for the extended triggers in Knative. At the moment the extended triggers are experimental and we would need more logic on the Knative trait to support the extended filters for the trigger.

But yes, the Pipe configuring the extended Knative triggers via traits is one possible option.

[lburgazzoli]

Since the Pipe is an higher level resource, I think it is fine if it has access to any of the spec properties of the Integration CRD like if it were any other client (in fact, in the KameletBinding reconciler has been originally implemented as a dedicated operator, and then merged in the camel-k operator).

For the specific question about the fact that a binding resolver has access to the traits, then I think it is perfectly fine as the resolver should be able to influence how an integration behave. In term of code, I don't know if there is any better, less coupled option, but given it is already the way it works as today, then I'm fine with the implementation.

The only two notes I can add are about UX, probably something not strictly related to this PR but, let me write them down here:

• the fact that there is no subgroup for filter elements may be a little bit confusing as you don’t really know which properties are part of the filter and which are not. i.e. I recall one can add name property and that property would be used to determine the name of the broker if not provided in the ref
• about the knative trait, the same as above can apply as there (maybe) are some conflicting properties maybe ? i.e. what happen if you set a filter for a non broker resource ?

[christophd]

the fact that there is no subgroup for filter elements may be a little bit confusing as you don’t really know which properties are part of the filter and which are not. i.e. I recall one can add name property and that property would be used to determine the name of the broker if not provided in the ref

true, I will add this to the documentation so the user has an idea how and what properties are set as a trigger filter

[christophd]

about the knative trait, the same as above can apply as there (maybe) are some conflicting properties maybe ? i.e. what happen if you set a filter for a non broker resource?

I have checked in the code and Camel K will create the trigger only when using the broker resource as a source. when referencing a Knative channel resource for instance Camel K will not create the trigger and it is assumed that the channel and the trigger are present already.

So setting the trigger filter on the trait is only valid for broker type events. This is the result of us having a single Knative trait config for multiple Knative resources (broker, channel, service) as a source/sink. Again, this needs to be made clear in the documentation. I will take care of it (probably in a separate PR)