Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove string replacement on expressions for event filtering #1670

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

dpadhiar
Copy link
Member

@dpadhiar dpadhiar commented Feb 24, 2022

Signed-off-by: Dillen Padhiar dpadhiar99@gmail.com

Fixes #1667

Previously, expressions were filtered to replace - with _ similar to how params for their keys of their pairs. However this causes unintended behavior and makes filters fail on messages which it shouldn't.

Example yaml from issue:

apiVersion: argoproj.io/v1alpha1
kind: EventSource
metadata:
  name: kafka-eventsource-test
spec:
  kafka:  
    my-test-event:
[...]
       expression: "(body.someUUIDfield == '7e577e57-7e57-7e57-7e57-7e577e577e57')"

Sample JSON message:

{"someUUIDfield": "7e577e57-7e57-7e57-7e57-7e577e577e57",
  "id": 1}

We expect this message to be published since it will evaluate true with our expression.

Logs for this message being sent:

2022-02-24T18:14:12.584Z        INFO    argo-events.eventsource kafka/start.go:217      dispatching event on the data channel...        {"eventSourceName": "kafka", "eventSourceType": "kafka", "eventName": "example", "partition-id": "0"}
2022-02-24T18:14:12.607Z        INFO    argo-events.eventsource eventsources/eventing.go:512    succeeded to publish an event   {"eventSourceName": "kafka", "eventName": "example", "eventSourceType": "kafka", "eventID": "kafka:example:kafka-broker:9092:topic-2:0:12"}

We also test a JSON message with all _ instead to show that we have removed string replacement from the expression as previously this would evaluate to true.

{"someUUIDfield": "7e577e57_7e57_7e57_7e57_7e577e577e57",
  "id": 1}

Logs for this message being sent:

2022-02-24T18:06:16.493Z        INFO    argo-events.eventsource kafka/start.go:217      dispatching event on the data channel...        {"eventSourceName": "kafka", "eventSourceType": "kafka", "eventName": "example", "partition-id": "0"}
2022-02-24T18:06:16.550Z        DEBUG   argo-events.eventsource eventsources/eventing.go:477    Do not publish event, filter condition not met  {"eventSourceName": "kafka"}

Signed-off-by: Dillen Padhiar <dpadhiar99@gmail.com>
@@ -565,5 +565,5 @@ func filterEvent(data []byte, filter *v1alpha1.EventSourceFilter) (bool, error)
params[strings.ReplaceAll(key, "-", "_")] = value
}
env := expr.GetFuncMap(params)
return expr.EvalBool(strings.ReplaceAll(filter.Expression, "-", "_"), env)
return expr.EvalBool(filter.Expression, env)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and a test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test case for EvalBool for expressions with - and added output for example for original issue to PR.

Signed-off-by: Dillen Padhiar <dpadhiar99@gmail.com>
@alexec alexec merged commit 0db531c into argoproj:master Feb 24, 2022
@dpadhiar dpadhiar deleted the quick-fix branch February 24, 2022 18:43
whynowy pushed a commit that referenced this pull request Feb 27, 2022
)

Signed-off-by: Dillen Padhiar <dpadhiar99@gmail.com>
whynowy pushed a commit that referenced this pull request Feb 27, 2022
)

Signed-off-by: Dillen Padhiar <dpadhiar99@gmail.com>
BulkBeing pushed a commit to BulkBeing/argo-events that referenced this pull request Mar 7, 2022
juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eventsource filtering modifies the filter expression at runtime -> doesn't work as expected
2 participants