Added support for ACL entry netmask set with non-contiguous bits #1143
Conversation
- Change type of ipv4 and ipv6 source and destinations address to oc-inet:ipv4-address. - Creation of source/destination-address-mask leaves to alow wildcard masks
Creation of ipv4-prefix-mask-type and ipv6-prefix-mask-type.
Pull-eckermann
changed the title
|
/gcbrun |
|
No major YANG version changes in commit a3504f9 |
|
I'd prefer to see new leafs added instead of changing the existing Changing the type is a major, breaking change and, in my opinion, is not warranted in this case. |
There was a problem hiding this comment.
Thanks for the PR and the research to show support on different platforms -- one note, the fact that platforms use two leaves to express address and mask does not imply that they support wildcard matches, so I'd prefer to keep these as two separate configurable entities.
+1 to comments in the thread from @LimeHat about how we implement the change -- see comments below too.
Add sugestions of the reviewer: - Remove braking change - Add two new leaves to be used in the case of non-wildcard matches that cannot be expressed by a CIDR mask.
Deleting the changes as the typedef is not needed anymore.
|
@LimeHat @robshakir Breaking change removed and implemented the sugested changes. Two new leaves were added to the specific case that is of concern. |
|
/gcbrun |
| leaf masked-source-address { | ||
| type oc-inet:ipv4-address; | ||
| description | ||
| "Source IPv4 address used with a mask."; |
There was a problem hiding this comment.
I would recommend adding more detailed descriptions to these leafs, namely:
- indicate that
masked-source-addressandsource-maskare always used together, but mutually exclusive withsource-address - indicate that
source-addressis the preferred method of configuring ACLs as suggested by @robshakir
There was a problem hiding this comment.
Done in the last commit.
| "Source IPv4 address used with a mask."; | ||
| } | ||
|
|
||
| leaf destination-source-mask { |
There was a problem hiding this comment.
Fixed in the last commit.
| leaf destination-source-mask { | ||
| type oc-inet:ipv4-address; | ||
| description | ||
| "Source IPv4 address mask."; |
There was a problem hiding this comment.
Please specify that this is a wildcard mask. Maybe even better to add wildcard to the name of the leaf.
There was a problem hiding this comment.
Done in the last commit.
| leaf destination-address-mask { | ||
| type oc-inet:ipv4-address; | ||
| description | ||
| "Destination IPv4 address mask."; |
There was a problem hiding this comment.
Please specify that this is a wildcard mask. Maybe even better to add wildcard to the name of the leaf.
There was a problem hiding this comment.
Done in the last commit.
- Changed the name of leaves source-address-mask and destination-address-mask to source-wildcard-mask and destination-wildcard-mask. - Increase specification ot the description of masked-source-address and masked-destination-address leaves.
Added support for ACL entry netmask set with non-contiguous bits
This pull resquest is being created in the context of issue 1082
In the current OpenConfig, configuration of source-address and destination-address leaves in the /oc-acl:acl/acl-sets/acl-set/acl-entries/acl-entry/ipv4/config (or ipv6) xpath are only supported with netmasks that are left-contiguous, cause this leaves are defined with type oc-inet:ipv4-prefix or oc-inet:ipv6-prefix, which only allows CIDR mask format.
The contiguous mask is applicable when assigning an IP address to an interface, or while adding routes. However, it does not necessarily need to be contiguos for ACLs. ACL's should be capable of filtering based on any kinds of masks. This way multiple non consecutive ranges of networks can be covered in one shot.
Change Scope
Platform Implementations