[HLD] Dynamic ACL rule high-level design doc #952
Conversation
Signed-off-by: bingwang <wang.bing@microsoft.com>
Signed-off-by: bingwang <wang.bing@microsoft.com>
Signed-off-by: bingwang <bingwang@microsoft.com>
|
|
||
| 1. Test case 1 Verify dynamic ACL rule is created as expected | ||
| 2. Test case 2 Verify dynamic ACL rule can be refreshed | ||
| 3. Test case 3 Verify expired dynamic ACL rule can be removed |
There was a problem hiding this comment.
I think we can add more tests for negative operations:
- Convert static ACL rule to dynamic ACL rule is not allowed
- dynamic ACL should be removed from config_db after reboot
There was a problem hiding this comment.
The second one dynamic ACL should be removed from config_db after reboot makes sense to me. I'll add a test case to verify dynamic ACL is cleared after config reload or reboot.
But why do we need the first verification Convert static ACL rule to dynamic ACL rule is not allowed?
There was a problem hiding this comment.
In case someone change existing static rules to dynamic rules.
For dynamic ACL, the basic principle should be JIT change. The state of switch shouldn't be changed after dynamic ACL expired.
Consider below use case:
- Someone pushed a dynamic rule that have exact same name with existing static rule
- The rule will be removed after it expired, the switch no longer have that static rule anymore
We should prevent override static rule by pushing dynamic rule. Otherwise the dynamic rule will have side-effect after it expired.
| #### Unit Test cases | ||
|
|
||
| 1. Enhance unit test for `acl-loader` to verify ACL rule with TTL is created as expected; verify `ACL_TTL_TABLE` entry is created as expected. | ||
| 2. Add unit test for `acl_ttl_checker` to verify expired ACL entries are cleared. |
There was a problem hiding this comment.
Add test to ensure acl_ttl_checker can only remove is_dynamic: true entry from config db.
| 3. Test case 3 Verify expired dynamic ACL rule can be removed | ||
|
|
||
| ## Open questions | ||
| 1. memory leak issue detection and validation. |
There was a problem hiding this comment.
What could cause memory leak in current context?
There was a problem hiding this comment.
There is supposed to be no memory leak in this context. The question here is as a reminder that we are lack of verification of memory leak issues for daemon process.
yxieca
force-pushed
the
master
branch
2 times, most recently
from
8498931 to
8837dc2
Compare
|
|
||
| #### acl_ttl_checker | ||
|
|
||
| A helper script will be added to `swss` container. The checker is started after `orchagent` and check the TTL of dynamic ACL rules every 10 seconds by default. It will walk through all entries in `ACL_TTL_TABLE` and delete the corresponding `ACL_RULE` from `config_db` if |
There was a problem hiding this comment.
why cannot incorporate into the orchagent?
| ### Work flow | ||
| #### Add dynamic ACL rule | ||
| <p align=center> | ||
| <img src="img/dynamic_acl_creation.png" alt="Figure 1. Create dynamic ACL rule workflow"> |
There was a problem hiding this comment.
Suggest to have orchagent update the state_db for ACL_TTL_TABLE and update the timestamp and ttl. Let's not have acl-loader directly update the state_DB
| </p> | ||
|
|
||
| #### Remove ACL rule when TTL expires | ||
| <p align=center> |
There was a problem hiding this comment.
What if user does a 'config save" during the time acl rule is active? A future config reload can re-install the rule.
This PR is a high-level design doc for dynamic ACL rules.
We propose a design to add TTL to ACL rules, and the rules are cleared after TTL expired.