SNS: improve SNS Filter Policy engine and implement cidr operator
#11979
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 11m 35s ⏱️ - 40m 27s Results for commit 28652b1. ± Comparison against base commit 78fdf45. This pull request removes 1399 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Great find on the empty array edge case. I am glad our recursive implementation allowed you to fix in such an simple way. Love the new addition to the test and the reusable method to get the sqs message. It cleans up a lot.
I am not fond of creating an array with a gazillion items in it to be discarded right away? Do you know if IPv6 can be used?
| ips = [str(ip) for ip in ipaddress.IPv4Network(cidr)] | ||
| return value in ips |
There was a problem hiding this comment.
That seems super inefficient. 🤔 If the mask is 10.0.0.0/8 that would create a huge list. 👀
If we are sure it will always be an ipv4 address we can maybe do that return ipaddress.IPv4Address(value) in ipaddress.IPv4Network(cidr) and something like return ipaddress.ip_address(value) in ipaddress.ip_network(cidr) would cover ipv6 also, I believe
There was a problem hiding this comment.
Right, yes. Well, we can't be sure the value is a valid ipv4 or v6. Actually, looking at the docs, it seems it should work with ipv6, so this doesn't really work either. I'll update events as well to add IPv6 support.
I'll need to wrap this in a try/except block probably, and do what you proposed.
There was a problem hiding this comment.
time to port back the fix to events 😂 thanks, always pushing further! 💪 always thankful for your reviews!
| if not _object: | ||
| return array |
| try: | ||
| ipaddress.IPv4Network(value) | ||
| except ValueError: | ||
| raise InvalidParameterException( | ||
| f"{self.error_prefix}FilterPolicy: Malformed CIDR, one '/' required" | ||
| ) | ||
| self._validate_cidr_condition(value) |
Motivation
When implementing the EventRuleEngine with #11960, I've implemented different and new behaviors from SNS, and some felt like we just missed them in SNS and were better covered by the event test suite.
This PR implements the
cidroperator, and a missed edge case where one of the field of the payload is an empty array. Also adds a bit of validation around empty list passed to operators.Sorry @maxhoheiser and @joe4dev, I've sneaked in a new test case for the EventRuleEngine here as I wanted to verify a very edge case behavior in both engine when the value is an empty array, making the field not exists, which is an edge case we're also having, which is somewhat funny. I didn't want to open a new PR just for this tiny test case 😅 hope that's ok! no change in behavior as we were already right! For the anecdote, this is because I think
existscan only be applied to "leaf" nodes, and an empty array will make the field not be a leaf anymore, as we're going "into" the array. Funny enough, we have the same edge case 😄Changes
cidroperator and validationevents