Skip to content

Commit

Permalink
[generic-config-updater] Add caclrule validator (sonic-net#2103)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@wen587
wen587 authored Mar 21, 2022
1 parent 968900c commit b25f1e1
Show file tree
Hide file tree
Showing 5 changed files with 135 additions and 6 deletions.
2 changes: 1 addition & 1 deletion generic_config_updater/change_applier.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
6 changes: 6 additions & 0 deletions generic_config_updater/generic_config_updater.conf.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
},
"VLAN": {
"services_to_validate": [ "vlan-service" ]
},
"ACL_RULE": {
"services_to_validate": [ "caclmgrd-service" ]
}
},
"services": {
Expand All @@ -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" ]
}
}
}
Expand Down
27 changes: 27 additions & 0 deletions generic_config_updater/services_validator.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

3 changes: 3 additions & 0 deletions tests/generic_config_updater/change_applier_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
103 changes: 98 additions & 5 deletions tests/generic_config_updater/service_validator_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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": "" },
Expand Down Expand Up @@ -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
#
Expand All @@ -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 })
Expand All @@ -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
Expand All @@ -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)

0 comments on commit b25f1e1

Please sign in to comment.