Accomadating lower case ACL rules for Control Plane ACLs #5748
Conversation
|
@jleveque can you please review this PR? |
|
@madhanmellanox: Can you please explain the need for this? Why can't we continue relying on the rule prop keys to be all caps? And why the need for this now? What changed? |
|
@jleveque because we check for SRC_IPV6 / DST_IPV6 / TCP_FLAGs in caclmgrd code, the checks are becoming case sensitive and when we configure using lower case letters, the ACL rule is not working. For example, in my ACL rule if I have SRC_IPv6 instead of SRC_IPV6, the rule in ip6tables is not right. This is because of cases sensitive checks we make in the code. It is a Day-1 issue. It is not new. |
|
@madhanmellanox: The acl-loader application configures these to be all-caps before inserting them into config DB. The only way I can see this being a problem is if you're adding them manually, in which case, why not add them as all caps? I'm not opposed to this change, but I'm just not convinced that it's necessary. |
|
Yes, this is our QA defect and they individually configure the ACLs manually. In any case it should not fail. |
|
I see. Technically, this is human error, as it doesn't follow the spec we had defined, where these keys should always be uppercase in config DB. I feel like we should firm up a spec here, but as this change seems safe, I think we can go ahead with it. However, please note that I have an open PR to migrate caclmgrd to Python 3 and move it to a new package in the master branch: #5739. That PR should be merging soon, in which case, this PR will have conflicts, and will need to be rebased, but then it will not be cherry-pickable to 201911, so a separate PR would be needed. |
|
retest this please |
|
@jleveque I discussed with QA and they said ACL High level design, has these fields in lower case when defining config db schemas. That is the reason for ambiguity to enter config in config_db.json in lower case letters. Can schema be defined in Upper case letters? Can I change the ACL HLD document? |
|
@madhanmellanox: Thank you for pointing to the root cause of the ambiguity. I think the crux of the issue is that if the ACLs are defined in an acl.json file and loaded using acl-loader, it will convert to all caps and also replace hyphens with underscores before pushing to Config DB. I originally wrote caclmgrd assuming that all ACLs will be loaded this way, and didn't consider the case where they would be manually entered into a config_db.json file and loaded. This, combined with the HLD you provided makes the case that caclmgrd should be able to handle both upper-case and lower-case. For now, I think we can go ahead and add your change. However, the HLD might also need some clarification, as it states things like "acl_table_name must be unique" -- we might want to clarify that "acl_table_name must be unique, regardless of case" or similar. Can you please fix the conflicts here, since my other PR has merged to move the file and convert it to Python 3? |
madhanmellanox
force-pushed
the
cacl_lc_madhan
branch
from
a93a8b3 to
150e136
Compare
|
retest this please |
1 similar comment
|
retest this please |
|
retest vsimage |
|
retest this please |
|
@jleveque vstest cacl.test_cacl_application.test_cacl_application is failing for me consistently. Not sure because of converting the case of rules. There is a list of Missing IPtables rules listed in the output. Can you please check. |
|
@madhanmellanox: I can only assume that this patch is causing the test to fail. Do you have a testbed you can apply your change to and run the test to reproduce? |
|
@jleveque I don't find the test case tests/cacl/test_cacl_application.py in sonic-buildimage repository or sonic-swss repository. Where can I find it. I thought it is a vstest and I will find it in sonic-swss/tests. But, I don't find it there. |
|
@jleveque I found the test case part of sonic-mgmt framework. |
|
retest this please |
|
@jleveque for testing purpose, I commented my code of making all rules Upper case, even with this change commented out, I am seeing vsimage cacl test case failing. I also tried checking with my team here regarding running vstest with kvm sonic image. But, they are not familiar. That is why I tried commenting out my change and still seeing the same test case failure. |
|
Retest vs please |
|
Retest vsimage please |
1 similar comment
|
Retest vsimage please |
|
@jleveque with changing iteritems to items to traverse the rule props, the CACL test case is still failing, do you want me to comment my code and run? |
|
@madhanmellanox: This is a different test failure. You can try commenting out your change and see if it passes. |
|
@jleveque commenting out my code the vsimage - all test cases passed. So, I am uncommenting out my code and submitting it again. |
|
@jleveque the vsimage is failing again in test_cacl_function with error log "SSH did not time out when expected". I am not sure what my change has to do with this error. |
|
retest broadcom |
|
retest this please |
|
@jleveque although VSimage tests are passing now, baseimage is always failing. This is the build error log. |
|
Looks like a transient Jenkins issue. Retesting should fix. Might take a couple tries, unfortunately. |
|
Retest baseimage please |
|
@jleveque can you please approve this PR? |
|
@madhanmellanox: Since caclmgrd has moved to a new location, this will not cherry-pick cleanly to 201911. Can you please open a separate PR against the 201911 branch? |
Why I did it
To make Control plane ACLs handle case insensitive ACL rules. Currently, it handles only upper case ACL rules.
How I did it
I set the rule prop dictionary to be all upper case before reading the rule props for processing.
How to verify it
Configure ACL rules with any combination of case of ACL rules, either it be upper case or lower case. All rules should be created properly in the switch and working properly.
- Which release branch to backport (provide reason below if selected)
- Description for the changelog
The change is a one line change in caclmgrd script.
- A picture of a cute animal (not mandatory but encouraged)