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: EventBus: Exists prefix not considered for rule matching #9931

Merged
merged 9 commits into from Dec 29, 2023

Conversation

maxhoheiser
Copy link
Member

@maxhoheiser maxhoheiser commented Dec 22, 2023

Motivation

As reported in #8512 and #7305 the current behavior does not fully match the aws cli behavior regarding the EventBridge pattern {"exists": True}.

This rule:

{
	"source": ["core.update-account-command"],
	"detail-type": ["core.update-account-command"],
	"detail": {"key": [{"exists": True}]},
}

in combination with this message:

{
	"version": "0",
	"id": "<uuid:2>",
	"detail-type": "core.update-account-command",
	"source": "core.update-account-command",
	"account": "111111111111",
	"time": "date",
	"region": "<region>",
	"resources": [],
	"detail": {
	    "key":"value",
		"payload":"baz"
	}
}

does not match on LocalStack, but does correctly match on AWS.

The issue arises from the prefix exists not being considered.

Furthermore, the opposite pattern [("exists"): False] was also not correctly matched

This rule:

{
	"source": ["core.update-account-command"],
	"detail-type": ["core.update-account-command"],
	"detail": {"key": [{"exists": False}]},
}

in combination with this message:

{
	"version": "0",
	"id": "<uuid:2>",
	"detail-type": "core.update-account-command",
	"source": "core.update-account-command",
	"account": "111111111111",
	"time": "date",
	"region": "<region>",
	"resources": [],
	"detail": {
	    "no-key":"no-value",
		"payload":"baz"
	}
}

Changes

Adding a condition to check for the prefix exists and use the boolean value in combination with the condition if there value to return exists for the pattern [("exists"): True].

The pattern [("exists"): False] needs different handling, since per definition, the getter of the message event for the in the pattern defined key will always fail to the fallback object. To still send the message correctly, an additional check is put in place handling this particular instance when the return value is not presented, that is only True in the condition that the pattern prefix equal "exists" see (provider.py line 516).

Fixes #8512 and fixes #7305

Testing

Polling for sqs messages was changed to long poll due to inconsistencies when generating the snapshots against aws with messages not being received.

@maxhoheiser maxhoheiser added aws:sqs Amazon Simple Queue Service aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases labels Dec 22, 2023
Copy link

github-actions bot commented Dec 22, 2023

LocalStack Community integration with Pro

    2 files      2 suites   1h 12m 42s ⏱️
2 425 tests 2 197 ✅ 228 💤 0 ❌
2 426 runs  2 197 ✅ 229 💤 0 ❌

Results for commit e3283a4.

♻️ This comment has been updated with latest results.

@maxhoheiser maxhoheiser force-pushed the bugfix/eventbridge/8512-exists-true-not-matching branch from 3a69385 to 11dafcc Compare December 27, 2023 22:14
@maxhoheiser maxhoheiser force-pushed the bugfix/eventbridge/8512-exists-true-not-matching branch from 11dafcc to 0f1b97d Compare December 27, 2023 22:17
@coveralls
Copy link

coveralls commented Dec 27, 2023

Coverage Status

coverage: 83.974% (-0.02%) from 83.993%
when pulling e3283a4 on bugfix/eventbridge/8512-exists-true-not-matching
into 418a240 on master.

@maxhoheiser maxhoheiser reopened this Dec 28, 2023
@maxhoheiser maxhoheiser self-assigned this Dec 28, 2023
@maxhoheiser maxhoheiser marked this pull request as ready for review December 28, 2023 15:58
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

PR is already in a good state to merge, but I have a few suggestions. They can be either tackled in this PR, later or just added as TODOs for future us 👍

Sorry you had to deal with such an annoying test as one of your first bigger AWS tests 😅 Glad we could find a reliable solution in the end.

localstack/services/events/provider.py Outdated Show resolved Hide resolved
tests/aws/services/events/test_events.py Outdated Show resolved Hide resolved
Comment on lines 552 to 562
messages = put_events_with_filter_to_sqs(
pattern=test_event_pattern_exists,
entries_asserts=entries_asserts,
)
snapshot.match("rule-exists-true", messages)

messages_not_exists = put_events_with_filter_to_sqs(
pattern=test_event_pattern_not_exists,
entries_asserts=entries_asserts_exists_false,
)
snapshot.match("rule-exists-false", messages_not_exists)
Copy link
Member

Choose a reason for hiding this comment

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

From the docs: Exists matching only works on leaf nodes. It does not work on intermediate nodes.

Might be something that needs to be covered via a negative test as well at some point

tests/aws/services/events/test_events.py Outdated Show resolved Hide resolved
Comment on lines 424 to 425
if element_value and event_value:
return True
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if AWS behaves the same here with checking truthiness vs. just "not being None". Can be tackled in the future though

Copy link
Member

Choose a reason for hiding this comment

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

E.g.

{
  "detail": {
    "state": [ { "exists": true  } ]
  }
}

with

{
  ...
  "detail": {
    "state": 0
  }
}

I'd assume this should still select but we'd need to this this 🤷‍♂️

localstack/services/events/provider.py Show resolved Hide resolved
Split exists True and exists False into two separate tests for improved readability.
@maxhoheiser maxhoheiser merged commit 563edf7 into master Dec 29, 2023
26 checks passed
@maxhoheiser maxhoheiser deleted the bugfix/eventbridge/8512-exists-true-not-matching branch December 29, 2023 12:40
@maxhoheiser maxhoheiser added this to the 3.1 milestone Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:events Amazon EventBridge aws:sqs Amazon Simple Queue Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: EventBridge event-pattern not correctly matched on detail content bug: Bug with EventBus filters
3 participants