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

[caclmgrd] Fix application of IPv6 service ACL rules #3917

Merged
merged 1 commit into from
Dec 19, 2019
Merged

[caclmgrd] Fix application of IPv6 service ACL rules #3917

merged 1 commit into from
Dec 19, 2019

Conversation

jleveque
Copy link
Contributor

@jleveque jleveque commented Dec 16, 2019

Caclmgrd was written with the understanding that the "SRC_IP" field of the rule properties would contain either a v4 or v6 IP address, thus we examined the IP address in that field to make an educated guess as to whether the table which contained that rule contained v4 or v6 addresses.

However, this PR: sonic-net/sonic-utilities#377 broke the logic in caclmgrd, because it now causes the acl-loader application to perform the same check and if the IP address is a v4 address, it inserts it into the "SRC_IP" field as it did previously. However, if the IP address is a v6 address, it inserts the address into a new "SRC_IPV6" field, leaving the "SRC_IP" field empty, which caused the logic in caclmgrd to fail to determine tables which contain IPv6 addresses, therefore not applying IPv6 service ACLs. This was noticed via log messages like the following:

WARNING caclmgrd: Unable to determine if ACL table 'IPV6_SNMP_ACL' contains IPv4 or IPv6 rules. Skipping table...

This PR now adapts to the new acl-loader logic, and determines whether the table contains v4 or v6 address based on whether the rules in the table contain "SRC_IP"/"DST_IP" fields (v4), or "SRC_IPV6"/"DST_IPV6" fields (v6).

@jleveque
Copy link
Contributor Author

Retest vsimage please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants