Add time-based ACL support for acl-loader #2354

wsycqyz · 2022-09-07T01:56:47Z

What I did

Add time-based ACL support for acl-loader

How I did it

Modify code according to HLD: Dynamic-ACL-Design.md (Currently is under PR review)
New acl-loader CLIs are introduced:
acl-loader update time-based-acl <filename.json>
acl-loader delete-time-based-acl

How to verify it

See HLD unit test and system test.
sonic-net/SONiC#1078

Previous command output (if the output of a command-line utility has changed)

N/A

New command output (if the output of a command-line utility has changed)

admin@hostname: acl-loader update time-based-acl dacl.json
Info: Added CONFIG_DB entry for rule DYNAMIC_RULE_1
Info: {'PRIORITY': '9999', 'ETHER_TYPE': '2048', 'PACKET_ACTION': 'DROP', 'DST_IP': '10.0.0.57/32', 'is_dynamic': 'True', 'ttl': '3600', 'creation_time': '1662515645', 'expiration_time': '1662519245'}
Info: Added CONFIG_DB entry for rule DYNAMIC_RULE_2
Info: {'PRIORITY': '9998', 'ETHER_TYPE': '2048', 'PACKET_ACTION': 'DROP', 'DST_IP': '192.168.0.101/32', 'is_dynamic': 'True', 'ttl': '10', 'creation_time': '1662515645', 'expiration_time': '1662515655'}

admin@hostname: acl-loader delete-time-based-acl BMC_ACL_SOUTHBOUND 1
Info: Removed a dynamic ACL rule BMC_ACL_SOUTHBOUND|DYNAMIC_RULE_1

Signed-off-by: bingwang <bingwang@microsoft.com>

lgtm-com · 2022-09-07T02:07:54Z

This pull request introduces 2 alerts when merging 0006a60 into 3be2ad7 - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'
1 for Unused import

lgtm-com · 2022-09-07T03:08:02Z

This pull request introduces 1 alert when merging 2371472 into 3be2ad7 - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

Blueve · 2022-09-07T04:06:25Z

Let's mark this as draft here since the HLD is still under reviewing

wsycqyz · 2022-09-07T04:34:00Z

Let's mark this as draft here since the HLD is still under reviewing

Sure.

lgtm-com · 2022-09-15T03:07:44Z

This pull request introduces 1 alert when merging 9ab1f48 into 1ac584b - view on LGTM.com

new alerts:

1 for Unused import

Blueve · 2022-11-01T08:21:56Z

acl_loader/main.py

        """
        Load file with ACL rules configuration in openconfig ACL format. Convert rules
        to Config DB schema.
        :param filename: File in openconfig ACL format
+        :param is_time_based: A bool, indicates whether the rule is dynamic or not


Accept and thanks!

Blueve · 2022-11-01T08:22:10Z

acl_loader/main.py

@@ -583,6 +586,41 @@ def convert_input_interface(self, table_name, rule_idx, rule):

        return rule_props

+    def is_db_rule_dynamic(self, key):


is_db_rule_time_based

Accept and thanks!

Blueve · 2022-11-01T08:22:56Z

acl_loader/main.py

+    def is_time_based_rule(self, rule):
+        return rule.time_range.config.start_time
+
+    def convert_dynamic_acl_config(self, key, rule):


convert_time_based_acl_config

Accept and thanks!

Blueve · 2022-11-01T08:23:23Z

acl_loader/main.py

+
+        return rule_props
+
+    def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry):


add_config_db_time_based_acl_rule_entry

Accept and thanks!

Blueve · 2022-11-01T08:23:42Z

acl_loader/main.py

+
+    def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry):
+        """
+        Insert entry to CONFIG_DB for dynamic ACL


...for time-based ACL

Accept and thanks!

Blueve · 2022-11-01T08:24:05Z

acl_loader/main.py

+        """
+        key = table + '|' + rule
+        try:
+            entry.pop('is_time_based', None)


Why we have this entry? It is not exist in HLD

"is_time_based" is an internal field to mark if the ACL is time-based not or during acl-loader parsing the json file.

Blueve · 2022-11-01T08:25:25Z

acl_loader/main.py

@@ -596,16 +634,27 @@ def validate_rule_fields(self, rule_props):
            if ("ICMPV6_TYPE" in rule_props or "ICMPV6_CODE" in rule_props) and protocol != 58:
                raise AclLoaderException("IP_PROTOCOL={} is not ICMPV6, but ICMPV6 fields were provided".format(protocol))

-    def convert_rule_to_db_schema(self, table_name, rule):
+    def convert_rule_to_db_schema(self, table_name, rule, rule_name, is_time_based):


Better use an enum type instead. In future, we might introduce more ACL types, use enum can avoiding change the API:

Standard

TimeBased

...

This is an optional comments. We can do the refactor after this PR for sure.

Blueve · 2022-11-01T08:26:39Z

acl_loader/main.py

        rule_props = {}
-        rule_data = {(table_name, "RULE_" + str(rule_idx)): rule_props}
+        if is_time_based and not self.is_time_based_rule(rule):
+            warning("Attempting to insert time-based ACL rule without TTL, refused")


Without start_time and end_time

Accept and thanks!

Blueve · 2022-11-01T08:27:06Z

acl_loader/main.py

+            return False
+
+    def is_time_based_rule(self, rule):
+        return rule.time_range.config.start_time


We'd better check if end_time provided here

Accept and thanks!

Blueve · 2022-11-01T08:28:04Z

acl_loader/main.py

@@ -642,9 +692,10 @@ def deny_rule(self, table_name):
            rule_props["ETHER_TYPE"] = str(self.ethertype_map["ETHERTYPE_IPV4"])
        return rule_data

-    def convert_rules(self):
+    def convert_rules(self, is_time_based):


Same previous comment, we'd better use enum type here.
We can do refactor in future. Add this comment here as a reminder

Blueve · 2022-11-01T08:28:18Z

acl_loader/main.py

        """
        Convert rules in openconfig ACL format to Config DB schema
+        :param is_time_based: A bool, indicates whether the rule is dynamic or not


the rule is time_based or not

Accept and thanks!

Blueve · 2022-11-01T08:28:35Z

acl_loader/main.py

@@ -765,6 +822,33 @@ def incremental_update(self):
                for namespace_configdb in self.per_npu_configdb.values():
                    namespace_configdb.set_entry(self.ACL_RULE, key, self.rules_info[key])

+    def update_time_based_acl_rules(self):
+        """
+        Perform update of dynamic ACL rule.


Perform update of time-based ACL rule.

Accept and thanks!

Blueve · 2022-11-01T08:29:09Z

acl_loader/main.py

+    def update_time_based_acl_rules(self):
+        """
+        Perform update of dynamic ACL rule.
+        Renew the TTL of ACL rule if existing, or insert new dynamic ACL rule


Update the start_time and end_time if existing, or insert new time-based ACL rule

Accept and thanks!

bingwang-ms and others added 3 commits June 28, 2022 15:29

Update acl-loader to support dynamic ACL

cd85b69

Signed-off-by: bingwang <bingwang@microsoft.com>

Update according to 0905 desgin

eafc0f8

fix typo

0006a60

wsycqyz added the Request for 202205 Branch label Sep 7, 2022

Fix PR merge alerts

2371472

wsycqyz mentioned this pull request Sep 7, 2022

Add time-based ACL High Level Design document sonic-net/SONiC#1078

Open

Fix PR alert

fe80fc4

wsycqyz marked this pull request as draft September 7, 2022 04:37

wsycqyz added 5 commits September 8, 2022 01:01

Modify according to PR review comment

b35c326

Modify according to PR review comment

4460dac

Modify according to PR review comment

bec2e4b

Format info output

803a1d0

Update acl-loader according to HLD

9ab1f48

wsycqyz added 2 commits September 14, 2022 20:13

Remove unused import

b850485

Remove unused variables

b939ebc

Blueve removed the Request for 202205 Branch label Nov 1, 2022

Blueve reviewed Nov 1, 2022

View reviewed changes

Update according to review comments

57c9f7a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add time-based ACL support for acl-loader #2354

Add time-based ACL support for acl-loader #2354

wsycqyz commented Sep 7, 2022 •

edited

Loading

lgtm-com bot commented Sep 7, 2022

lgtm-com bot commented Sep 7, 2022

Blueve commented Sep 7, 2022

wsycqyz commented Sep 7, 2022

lgtm-com bot commented Sep 15, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022 •

edited

Loading

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

Blueve Nov 1, 2022

wsycqyz Nov 4, 2022

		@@ -583,6 +586,41 @@ def convert_input_interface(self, table_name, rule_idx, rule):

		return rule_props

		def is_db_rule_dynamic(self, key):


		return rule_props

		def add_config_db_dynamic_acl_rule_entry(self, table, rule, entry):

Add time-based ACL support for acl-loader #2354

Are you sure you want to change the base?

Add time-based ACL support for acl-loader #2354

Conversation

wsycqyz commented Sep 7, 2022 • edited Loading

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

lgtm-com bot commented Sep 7, 2022

lgtm-com bot commented Sep 7, 2022

Blueve commented Sep 7, 2022

wsycqyz commented Sep 7, 2022

lgtm-com bot commented Sep 15, 2022

Choose a reason for hiding this comment

wsycqyz Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsycqyz commented Sep 7, 2022 •

edited

Loading

wsycqyz Nov 4, 2022 •

edited

Loading