[generic-config-updater] Add caclrule validator (sonic-net#2103)

wen587 · web-flow · commit b25f1e19dce3 · 2022-03-21T10:27:33.000+08:00
Add 1 sec sleep time to make sure caclmgrd does update

What I did
When GCU make change to control plane ACL_RULE, the iptables doen't change immediately. There is a delay because caclmgrd will update in 0.5 sec if no more update being made to config. So I add a caclrule validator to make sure the iptable is updated.

How I did it
When caclrule is being changed, add a 1sec sleep to make sure caclmgr does update.

How to verify it
Run sonic-utilities unit test
diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py
@@ -107,7 +107,7 @@ def _services_validate(self, old_cfg, upd_cfg, keys):
 
         for cmd in lst_cmds:
             ret = self._invoke_cmd(cmd, old_cfg, upd_cfg, keys)
-            if ret:
+            if not ret:
                 log_error("service invoked: {} failed with ret={}".format(cmd, ret))
                 return ret
             log_debug("service invoked: {}".format(cmd))
diff --git a/generic_config_updater/generic_config_updater.conf.json b/generic_config_updater/generic_config_updater.conf.json
@@ -42,6 +42,9 @@
         },
         "VLAN": {
             "services_to_validate": [ "vlan-service" ]
+        },
+        "ACL_RULE": {
+            "services_to_validate": [ "caclmgrd-service" ]
         }
     },
     "services": {
@@ -59,6 +62,9 @@
         },
         "vlan-service": {
             "validate_commands": [ "generic_config_updater.services_validator.vlan_validator" ]
+        },
+        "caclmgrd-service": {
+            "validate_commands": [ "generic_config_updater.services_validator.caclmgrd_validator" ]
         }
     }
 }
diff --git a/generic_config_updater/services_validator.py b/generic_config_updater/services_validator.py
@@ -71,3 +71,30 @@ def vlan_validator(old_config, upd_config, keys):
     # No update to DHCP servers.
     return True
 
+def caclmgrd_validator(old_config, upd_config, keys):
+    old_acltable = old_config.get("ACL_TABLE", {})
+    upd_acltable = upd_config.get("ACL_TABLE", {})
+
+    old_cacltable = [table for table, fields in old_acltable.items()
+                     if fields.get("type", "") == "CTRLPLANE"]
+    upd_cacltable = [table for table, fields in upd_acltable.items()
+                     if fields.get("type", "") == "CTRLPLANE"]
+
+    old_aclrule = old_config.get("ACL_RULE", {})
+    upd_aclrule = upd_config.get("ACL_RULE", {})
+
+    old_caclrule = [rule for rule in old_aclrule
+                    if rule.split("|")[0] in old_cacltable]
+    upd_caclrule = [rule for rule in upd_aclrule
+                    if rule.split("|")[0] in upd_cacltable]
+
+    # Only sleep when cacl rule is changed as this will update iptable.
+    for key in set(old_caclrule).union(set(upd_caclrule)):
+        if (old_aclrule.get(key, {}) != upd_aclrule.get(key, {})):
+            # caclmgrd will update in 0.5 sec when configuration stops,
+            # we sleep 1 sec to make sure it does update.
+            time.sleep(1)
+            return True
+    # No update to ACL_RULE.
+    return True
+
diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py
@@ -158,6 +158,7 @@ def system_health(old_cfg, new_cfg, keys):
     if svcs != None:
         assert svc_name in svcs
         svcs.remove(svc_name)
+    return True
 
 
 def _validate_keys(keys):
@@ -201,11 +202,13 @@ def _validate_svc(svc_name, old_cfg, new_cfg, keys):
 def acl_validate(old_cfg, new_cfg, keys):
     debug_print("acl_validate called")
     _validate_svc("acl_validate", old_cfg, new_cfg, keys)
+    return True
 
 
 def vlan_validate(old_cfg, new_cfg, keys):
     debug_print("vlan_validate called")
     _validate_svc("vlan_validate", old_cfg, new_cfg, keys)
+    return True
 
 
 class TestChangeApplier(unittest.TestCase):
diff --git a/tests/generic_config_updater/service_validator_test.py b/tests/generic_config_updater/service_validator_test.py
@@ -6,14 +6,16 @@
 from collections import defaultdict
 from unittest.mock import patch
 
-from generic_config_updater.services_validator import vlan_validator, rsyslog_validator
+from generic_config_updater.services_validator import vlan_validator, rsyslog_validator, caclmgrd_validator
 import generic_config_updater.gu_common
 
 
 # Mimics os.system call
 #
 os_system_calls = []
 os_system_call_index = 0
+time_sleep_calls = []
+time_sleep_call_index = 0
 msg = ""
 
 def mock_os_system_call(cmd):
@@ -26,6 +28,15 @@ def mock_os_system_call(cmd):
     assert cmd == entry["cmd"], msg
     return entry["rc"]
 
+def mock_time_sleep_call(sleep_time):
+    global time_sleep_calls, time_sleep_call_index
+
+    assert time_sleep_call_index < len(time_sleep_calls)
+    entry = time_sleep_calls[time_sleep_call_index]
+    time_sleep_call_index += 1
+
+    assert sleep_time == entry["sleep_time"], msg
+
 
 test_data = [
         { "old": {}, "upd": {}, "cmd": "" },
@@ -60,6 +71,77 @@ def mock_os_system_call(cmd):
         }
     ]
 
+test_caclrule = [
+        { "old": {}, "upd": {}, "sleep_time": 0 },
+        {
+            "old": {
+                "ACL_TABLE": { "XXX": { "type": "CTRLPLANE" } },
+                "ACL_RULE": { "XXX|RULE_1": { "SRC_IP": "10.10.10.10/16" } }
+            },
+            "upd": {
+                "ACL_TABLE": { "XXX": { "type": "CTRLPLANE" } },
+                "ACL_RULE": { "XXX|RULE_1": { "SRC_IP": "10.10.10.10/16" } }
+            },
+            "sleep_time": 0
+        },
+        {
+            "old": {
+                "ACL_TABLE": {
+                    "XXX": { "type": "CTRLPLANE" },
+                    "YYY": { "type": "L3" }
+                },
+                "ACL_RULE": {
+                    "XXX|RULE_1": { "SRC_IP": "10.10.10.10/16" },
+                    "YYY|RULE_1": { "SRC_IP": "192.168.1.10/32" }
+                }
+            },
+            "upd": {
+                "ACL_TABLE": {
+                    "XXX": { "type": "CTRLPLANE" }
+                },
+                "ACL_RULE": {
+                    "XXX|RULE_1": { "SRC_IP": "10.10.10.10/16" }
+                }
+            },
+            "sleep_time": 0
+        },
+        {
+            "old": {
+                "ACL_TABLE": { "XXX": { "type": "CTRLPLANE" } },
+                "ACL_RULE": { "XXX|RULE_1": { "SRC_IP": "10.10.10.10/16" } }
+            },
+            "upd": {
+                "ACL_TABLE": { "XXX": { "type": "CTRLPLANE" } },
+                "ACL_RULE": { "XXX|RULE_1": { "SRC_IP": "11.11.11.11/16" } }
+            },
+            "sleep_time": 1
+        },
+        {
+            "old": {
+                "ACL_TABLE": { "XXX": { "type": "CTRLPLANE" } },
+                "ACL_RULE": {
+                    "XXX|RULE_1": { "SRC_IP": "10.10.10.10/16" }
+                }
+            },
+            "upd": {
+                "ACL_TABLE": { "XXX": { "type": "CTRLPLANE" } },
+                "ACL_RULE": {
+                    "XXX|RULE_1": { "SRC_IP": "10.10.10.10/16" },
+                    "XXX|RULE_2": { "SRC_IP": "12.12.12.12/16" }
+                }
+            },
+            "sleep_time": 1
+        },
+        {
+            "old": {
+                "ACL_TABLE": { "XXX": { "type": "CTRLPLANE" } },
+                "ACL_RULE": { "XXX|RULE_1": { "SRC_IP": "10.10.10.10/16" } }
+            },
+            "upd": {},
+            "sleep_time": 1
+        },
+    ]
+
 test_rsyslog_fail = [
         # Fail the calls, to get the entire fail path calls invoked
         #
@@ -70,17 +152,14 @@ def mock_os_system_call(cmd):
         { "cmd": "systemctl restart rsyslog", "rc": 1 },         # restart again; fails
     ]
 
-
 class TestServiceValidator(unittest.TestCase):
 
     @patch("generic_config_updater.change_applier.os.system")
-    def test_change_apply(self, mock_os_sys):
-        global os_system_expected_cmd
+    def test_change_apply_os_system(self, mock_os_sys):
         global os_system_calls, os_system_call_index
 
         mock_os_sys.side_effect = mock_os_system_call
 
-        i = 0
         for entry in test_data:
             if entry["cmd"]:
                 os_system_calls.append({"cmd": entry["cmd"], "rc": 0 })
@@ -89,6 +168,7 @@ def test_change_apply(self, mock_os_sys):
             vlan_validator(entry["old"], entry["upd"], None)
 
 
+
         # Test failure case
         #
         os_system_calls = test_rsyslog_fail
@@ -97,3 +177,16 @@ def test_change_apply(self, mock_os_sys):
         rc = rsyslog_validator("", "", "")
         assert not rc, "rsyslog_validator expected to fail"
 
+    @patch("generic_config_updater.services_validator.time.sleep")
+    def test_change_apply_time_sleep(self, mock_time_sleep):
+        global time_sleep_calls, time_sleep_call_index
+
+        mock_time_sleep.side_effect = mock_time_sleep_call
+
+        for entry in test_caclrule:
+            if entry["sleep_time"]:
+                time_sleep_calls.append({"sleep_time": entry["sleep_time"]})
+            msg = "case failed: {}".format(str(entry))
+
+            caclmgrd_validator(entry["old"], entry["upd"], None)
+

Original file line number	Diff line number	Diff line change
`@@ -42,6 +42,9 @@`
`42`	`42`	`},`
`43`	`43`	`"VLAN": {`
`44`	`44`	`"services_to_validate": [ "vlan-service" ]`
	`45`	`+ },`
	`46`	`+ "ACL_RULE": {`
	`47`	`+ "services_to_validate": [ "caclmgrd-service" ]`
`45`	`48`	`}`
`46`	`49`	`},`
`47`	`50`	`"services": {`
`@@ -59,6 +62,9 @@`
`59`	`62`	`},`
`60`	`63`	`"vlan-service": {`
`61`	`64`	`"validate_commands": [ "generic_config_updater.services_validator.vlan_validator" ]`
	`65`	`+ },`
	`66`	`+ "caclmgrd-service": {`
	`67`	`+ "validate_commands": [ "generic_config_updater.services_validator.caclmgrd_validator" ]`
`62`	`68`	`}`
`63`	`69`	`}`
`64`	`70`	`}`