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

[HLD] Dynamic ACL rule high-level design doc #952

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 190 additions & 0 deletions doc/acl/Dynamic-ACL-Design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@

# Dynamic ACL #


## Table of Content

- [Revision](#revision)
- [Scope](#scope)
- [Definitions/Abbreviations](#definitionsabbreviations)
- [Overview](#overview)
- [Requirements](#requirements)
- [Architecture Design](#architecture-design)
- [High-Level Design](#high-level-design)
- [db schema](#db-schema)
- [CONFIG DB](#config-db)
- [STATE DB](#state-db)
- [acl-loader](#acl-loader)
- [acl_ttl_checker](#aclttlchecker)
-[Work flow](#work-flow)
- [Add dynamic ACL rule](#add-dynamic-acl-rule)
- [Refresh TTL of existing ACL rule](#refresh-ttl-of-existing-acl-rule)
- [Remove ACL rule when TTL expires](#remove-acl-rule-when-ttl-expires)
- [Testing Requirements/Design](#testing-requirementsdesign)
- [Unit Test cases](#unit-test-cases)
- [System Test cases](#system-test-cases)
- [Open questions](#open-questions)


## Revision

| Rev | Date | Author | Change Description |
|:---:|:-----------:|:------------------:|-----------------------------------|
| 0.1 | | Bing Wang | Initial version |

## Scope

The scope of this document covers the design of dynamic data plane ACL, which assigns a TTL to ACL rule, and removes the expired rule. Control plane ACL is not covered in this document.

## Definitions/Abbreviations

| Definitions/Abbreviation | Description |
|--------------------------|--------------------------------------------|
| ACL | Access Control List |


## Overview

The current design of data plane ACL supports only persistent rules, that is, once the rule is applied to SONiC, it will be there until manually removed or config reload.

This doc proposes an enhancement to current ACL, which add a TTL to ACL rules and the rules are deleted after TTL expires.

## Requirements

- ACL rules are removed when TTL expired
- Support TTL refreshment
- The count down of TTL keeps going after warm-boot

## Architecture Design

No SONiC architecture change is required to support dynamic ACL.

## High-Level Design

### db schema

#### CONFIG DB
Add a `dynamic` flag to current `ACL_RULE` table
```
key: ACL_RULE_TABLE:table_name:seq ; key of the rule entry in the table, seq is the order of the rules
; when the packet is filtered by the ACL "policy_name".
; A rule is always assocaited with a policy.

;field = value
is_dynamic = true/false ; Boolean; Optional. There will be a TTL assigned to this rule if is_dynamic is true, the
;rule will be removed from CONFIG DB.
;If is_dynamic is false or absent, the rule will be persistent
......
```
A sample config for ACL rule
```json
{
"ACL_RULE|DATAACL|RULE_1":{
"DST_IP":"192.168.0.3/32",
"ETHER_TYPE":"2048",
"PACKET_ACTION":"FORWARD",
"PRIORITY":"9999",
"SRC_IP":"192.168.0.2/32",
"is_dynamic":"true"
}
}
```
A sample json config for ACL rule
```json
{
"acl":{
"acl-sets":{
"acl-set":{
"DATAACL":{
"acl-entries":{
"acl-entry":{
"1":{
"actions":{
"config":{
"forwarding-action":"ACCEPT"
}
},
"config":{
"sequence-id":1
},
"ip":{
"config":{
"source-ip-address":"192.168.0.2/32",
"destination-ip-address":"192.168.0.3/32"
}
},
"ttl":"300"
}
}
}
}
}
}
}
}
```
The YANG of ACL_RULE is required to be updated to accept new field `is_dynamic`

Orchagent (actually `aclorch`) won't consume the value of `is_dynamic`. So no change is required to `orchagent`.
#### STATE DB
A new table is introduced to state_db to record the timestamp of creation, expiration and TTL of an ACL rule.
```
key = ACL_TTL_TABLE:acl_rule_name ; acl_rule_name specifies the corresponding ACL rule name, must be unique
;field = value
creation_time = Integer ; timestamp when the rule is created or refreshed
expiration_time = Integer ; timestamp when the rule will expire
ttl = Integer ; the ttl, in second
```
A sample
```json
{
"ACL_TTL_TABLE|DATAACL|RULE_1":{
"creation_time":1645686213,
"expiration_time":1645686513,
"ttl":300
}
}
```
#### acl-loader

Update `acl-loader` script to parse new field `is_dynamic`. The entry will be created in `ACL_TTL_TABLE` in `state_db` if `ttl` is present for certain an ACL rule in `acl.json`. Please find more details in workflow diagram.

#### 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why cannot incorporate into the orchagent?


- Current timestamp is larger than the `expiration_time` in the entry
- The corresponding ACL rule is marked as dynamic

### Work flow
#### Add dynamic ACL rule
<p align=center>
<img src="img/dynamic_acl_creation.png" alt="Figure 1. Create dynamic ACL rule workflow">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

#### Refresh TTL of existing ACL rule
<p align=center>
<img src="img/dynamic_acl_refresh.png" alt="Figure 2. Refresh dynamic ACL rule workflow">
</p>

#### Remove ACL rule when TTL expires
<p align=center>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if user does a 'config save" during the time acl rule is active? A future config reload can re-install the rule.

<img src="img/dynamic_acl_expiration.png" alt="Figure 1. Create dynamic ACL rule workflow">
</p>

### Testing Requirements/Design

#### 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test to ensure acl_ttl_checker can only remove is_dynamic: true entry from config db.



#### System Test cases

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add more tests for negative operations:

  1. Convert static ACL rule to dynamic ACL rule is not allowed
  2. dynamic ACL should be removed from config_db after reboot

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@Blueve Blueve Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Someone pushed a dynamic rule that have exact same name with existing static rule
  2. 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.


## Open questions
1. memory leak issue detection and validation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What could cause memory leak in current context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Binary file added doc/acl/img/dynamic_acl_creation.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added doc/acl/img/dynamic_acl_expiration.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added doc/acl/img/dynamic_acl_refresh.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.