[acl-loader] Add support for matching on ICMP and VLAN info #1469
Conversation
- Add ICMP and VLAN fields - Add new unit test cases Signed-off-by: Danny Allen <[email protected]>
|
retest this please |
acl_loader/main.py
Outdated
| type_key = "ICMPV6_TYPE" if is_table_v6 else "ICMP_TYPE" | ||
| code_key = "ICMPV6_CODE" if is_table_v6 else "ICMP_CODE" | ||
|
|
||
| if rule.icmp.config.type or rule.icmp.config.type == 0: |
There was a problem hiding this comment.
rule.icmp.config.type [](start = 11, length = 21)
Is it possible that rule.icmp.config.type == "null"?
Do you have a test case to make sure "null" is handled correctly?
There was a problem hiding this comment.
"null" is a string so it will give an error when we try to cast it (which we do check). The user shouldn't ever be passing null explicitly, we just have the value defined in the yang model so that the user is able to pass 0 as a valid ICMP type/code.
There was a problem hiding this comment.
Understood. To make code more readable, you may check rule.icmp.config.type == "null" first, if not equal, rule.icmp.config.type must be an integer, no need to cast.
In reply to: 586069013 [](ancestors = 586069013)
There was a problem hiding this comment.
OK, that sounds good to me.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
- Add ICMP and VLAN fields - Add new unit test cases Signed-off-by: Danny Allen <[email protected]>
Signed-off-by: Danny Allen [email protected]
What I did
I added support for loading rules that match on VLAN ID and ICMP type/code in acl-loader.
How I did it
These fields were added to the model in sonic-net/sonic-buildimage#6896. This PR extracts those fields so that users can use them in their rules.
How to verify it
Updated unit tests. Can also verify locally that existing acl.json files can still be loaded, and those using the new fields add the correct output to config DB.
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)