[DASH] ACL tags HLD #1427
[DASH] ACL tags HLD #1427
Conversation
|
@oleksandrivantsiv , please create a folder named |
oleksandrivantsiv
force-pushed
the
dash-acl-tags-stage-1
branch
from
406e14e
to
28c856d
Compare
|
@prsunny done |
|
lets have hld in |
|
|
||
| If the operation is to update the list of prefixes in the ACL tag in DASH_PREFIX_TAG_TABLE DashAclOrch will do the following | ||
| - Based on the list of ACL rules that uses the tag get list of affected ACL groups | ||
| - For each affected ACL group |
There was a problem hiding this comment.
In my understanding, due to memory limitation, it may be not available to keep a cache in orchagent.
So, do you have any idea to get each affected ACL group and its original ACL attributes?
Query from APP DB of Redis or Query from SAI?
There was a problem hiding this comment.
I didn't plan to implement such optimization in the stage 1 implementation. I think that querying data from APP DB should work perfectly. We can consider this optimization when we finish with the basic implementation in stage 2 or post-stage 2. What do you think?
| - if the ACL group to which the ACL rule belongs is bound to the ENI | ||
| - Create new ACL group | ||
| - For each ACL rule from the original ACL group | ||
| - Copy original ACL rule attributes | ||
| - Update attributes if required | ||
| - Create new ACL rule and attach it to the new ACL group | ||
| - Bind new ACL group to the ENI | ||
| - Remove all old ACL rules | ||
| - Remove old ACL group |
There was a problem hiding this comment.
Why cannot we directly create a new ACL rule and delete the old ACL rule other than refreshing the whole acl group?
There was a problem hiding this comment.
When the ACL group is not bound to any ENIs we can do an update rule by rule, as this won't affect the traffic.
But if the ACL group is bound to the ACL we want to perform update operation atomically. The tag may be used by more than one ACL rule in the group. If we update the rules one by one this will create inconsistency of the ACL group. Some of the ACL rules will have updated prefixes and some of them not. To prevent inconsistency we need to refresh the entire ACL group.
There was a problem hiding this comment.
Yes, totally agree. Your mentioned is about updating TAG entry. But this section is about updating one ACL rule.
For example,
We want to update tag1 to tag2 for rule_1,
DASH_ACL_RULE_TABLE:group_1:rule_1
"src_tag": tag1
=>
DASH_ACL_RULE_TABLE:group_1:rule_1
"src_tag": tag2
, I feel we can directly create a new ACL rule and delete the old one.
There was a problem hiding this comment.
Got you. I've just updated the Remove ACL rule and Update ACL rule sections to correctly cover this.
According to the ACL requirements in the DASH SONiC HLD document it is not allowed to update ACL rule from the ACL group that is bound to the ENI:
It is not permitted to modify rules within a group that is currently bound to an ENI. For such scenario, a new group shall be created by application with the modified set of rules which could then be bind to ENI. An exception is if tags are expanded to individual prefixes in an ACL rule. In such cases, if tags are modified, application shall update the corresponding rule by adding/removing a prefix.
In both cases, we will allow updating (or removing) the ACL rule only if the group is not bound to the ENI. Otherwise, an error will be removed.
If the updating or removal of the ACL rule from the ACL group that is bound to the ENI should be supported, we can use the same strategy that we have in the update tag flow:
- Create new ACL group
- For each ACL rule from the original ACL group
- If the rule shall be updated:
- Update attributes
- Create new ACL rule and attach it to the new ACL group
- Else if the rule shall be removed
- Continue (do not create rule)
- Else
- Create rule with no changes
- Bind new ACL group to the ENI
- If the rule shall be updated:
- Remove all old ACL rules
- Remove old ACL group
There was a problem hiding this comment.
@Pterosaur kindly reminder. Please review my comment and let me know if you agree.
There was a problem hiding this comment.
Hi, agree with your point. Approved.
But I'm just curious why it is not permitted to modify rules within a group? It's due to hardware limitations or just software implementation considerations?
oleksandrivantsiv
force-pushed
the
dash-acl-tags-stage-1
branch
from
28c856d
to
6d61ef5
Compare
oleksandrivantsiv
changed the title
|
@prsunny should we merge or wait for additional feedback? Code PR should be ready very soon! |
|
HLD was reviewed more than a month ago with the Dash community and approved already. Moving forward to merge it. |
|
moving forward, such change should be reviewed in sonic community after workgroup review to get alignment with others. |
|
@zhangyanzhao this HLD was reviewed with sonic dash subgroup |
|
@oleksandrivantsiv Can you please update the Quality Metric (Alpha/Beta/GA) for the feature either in this PR comments or in HLD itself based on https://github.com/sonic-net/SONiC/blob/master/doc/SONiC%20feature%20quality%20definition.md |
In a DASH SONiC, a service tag represents a group of IP address prefixes from a given service. The controller manages the address prefixes encompassed by the service tag and automatically updates the service tag as addresses change, minimizing the complexity of frequent updates to network security rules. Mapping a prefix to a tag can reduce the repetition of prefixes across different ACL rules and optimize memory usage.
Note that the HLD provides 2 stages for implementation.
Nvidia delivery will focus on stage 1 only while the community can add/extend to the second phase if it is required.