From 1dcbe06016ffaca0210cdff9cdf71f5c56f52252 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 3 Sep 2021 22:19:36 +0000 Subject: [PATCH 01/16] hostcfgd fix in prog Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 167 +++++++++++++++-------- 1 file changed, 108 insertions(+), 59 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 95c8e1221407..71dd01ab8ead 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -9,7 +9,8 @@ import syslog import jinja2 from sonic_py_common import device_info -from swsscommon.swsscommon import ConfigDBConnector +from swsscommon.swsscommon import SubscriberStateTable, DBConnector, Select +from swsscommon.swsscommom import ConfigDBConnector, TableConsumable # FILE PAM_AUTH_CONF = "/etc/pam.d/common-auth-sonic" @@ -36,6 +37,11 @@ RADIUS_SERVER_TIMEOUT_DEFAULT = "5" RADIUS_SERVER_AUTH_TYPE_DEFAULT = "pap" RADIUS_PAM_AUTH_CONF_DIR = "/etc/pam_radius_auth.d/" +# MISC Constants +CFG_DB = "CONFIG_DB" +PRIO_INIT = 10 +SELECT_TIMEOUT = 1000 + def run_cmd(cmd, log_err=True, raise_exception=False): try: @@ -725,18 +731,18 @@ class KdumpCfg(object): "memory": "0M-2G:256M,2G-4G:320M,4G-8G:384M,8G-:448M", "num_dumps": "3" } - def load(self, kdump_table): - syslog.syslog(syslog.LOG_INFO, "KdumpCfg load ...") - data = {} + def init(self, kdump_table): + """ + Set the KDUMP table in CFG DB to kdump_defaults and load + """ + syslog.syslog(syslog.LOG_INFO, "KdumpCfg init ...") kdump_conf = kdump_table.get("config", {}) for row in self.kdump_defaults: value = self.kdump_defaults.get(row) if kdump_conf.get(row) is not None: value = kdump_conf.get(row) else: - self.config_db.mod_entry("KDUMP", "config", { row : value}) - data[row] = value - self.kdump_update("config", data, True) + self.config_db.mod_entry("KDUMP", "config", {row : value}) def kdump_update(self, key, data, isLoad): syslog.syslog(syslog.LOG_INFO, "Kdump global configuration update") @@ -788,7 +794,7 @@ class NtpCfg(object): return # check only the intf configured as source interface - if (len(self.ntp_global) == 0): + if not self.ntp_global: return if 'src_intf' not in self.ntp_global: @@ -812,14 +818,14 @@ class NtpCfg(object): if 'vrf' in data: new_vrf = data['vrf'] - if (len(self.ntp_global) != 0): - + if not self.ntp_global: if 'src_intf' in self.ntp_global: orig_src = self.ntp_global['src_intf'] if 'vrf' in self.ntp_global: orig_vrf = self.ntp_global['vrf'] + # Update the Local Cache self.ntp_global = data # during initial load of ntp configuration, ntp server configuration decides if to restart ntp-config @@ -845,7 +851,7 @@ class NtpCfg(object): run_cmd(cmd) def ntp_server_update(self, key, data, isLoad): - syslog.syslog(syslog.LOG_INFO, 'ntp server update key {} data {}'.format(key, data)) + syslog.syslog(syslog.LOG_INFO, 'ntp server update key {} data {}'.format(key, op, data)) # during load, restart ntp-config regardless if ntp server is configured or not if isLoad == True: @@ -868,10 +874,17 @@ class NtpCfg(object): class HostConfigDaemon: def __init__(self): + # Just a sanity check to verify if the CONFIG_DB has been initialized + # before moving forward self.config_db = ConfigDBConnector() self.config_db.connect(wait_for_init=True, retry_on=True) + self.dbconn = DBConnector(CFG_DB, 0) syslog.syslog(syslog.LOG_INFO, 'ConfigDB connect success') + self.select = Select() + self.callbacks = dict() + self.subscriber_map = dict() + # Load DEVICE metadata configurations self.device_config = {} self.device_config['DEVICE_METADATA'] = self.config_db.get_table('DEVICE_METADATA') @@ -886,8 +899,13 @@ class HostConfigDaemon: # Load Kdump configuration self.kdumpCfg = KdumpCfg(self.config_db) - self.kdumpCfg.load(self.config_db.get_table('KDUMP')) + self.kdumpCfg.init(self.config_db.get_table('KDUMP')) + + # Update all feature states once upon starting + # self.feature_handler.update_all_features_config() + # Defer load until subscribe + # self.load() def load(self): aaa = self.config_db.get_table('AAA') @@ -915,81 +933,81 @@ class HostConfigDaemon: # Update AAA with the hostname self.aaacfg.hostname_update(self.hostname_cache) - def aaa_handler(self, key, data): - self.aaacfg.aaa_update(key, data) + + + def aaa_handler(self, key, op, data): + self.aaacfg.aaa_update(key, op, data) - def tacacs_server_handler(self, key, data): - self.aaacfg.tacacs_server_update(key, data) + def tacacs_server_handler(self, key, op, data): + self.aaacfg.tacacs_server_update(key, op, data) log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) syslog.syslog(syslog.LOG_INFO, 'value of {} changed to {}'.format(key, log_data)) - def tacacs_global_handler(self, key, data): - self.aaacfg.tacacs_global_update(key, data) + def tacacs_global_handler(self, key, op, data): + self.aaacfg.tacacs_global_update(key, op, data) log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) syslog.syslog(syslog.LOG_INFO, 'value of {} changed to {}'.format(key, log_data)) - def radius_server_handler(self, key, data): - self.aaacfg.radius_server_update(key, data) + def radius_server_handler(self, key, op, data): + self.aaacfg.radius_server_update(key, op, data) log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) syslog.syslog(syslog.LOG_INFO, 'value of {} changed to {}'.format(key, log_data)) - def radius_global_handler(self, key, data): - self.aaacfg.radius_global_update(key, data) + def radius_global_handler(self, key, op, data): + self.aaacfg.radius_global_update(key, op, data) log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) syslog.syslog(syslog.LOG_INFO, 'value of {} changed to {}'.format(key, log_data)) - def mgmt_intf_handler(self, key, data): + def mgmt_intf_handler(self, key, op, data): self.aaacfg.handle_radius_source_intf_ip_chg(key) self.aaacfg.handle_radius_nas_ip_chg(key) - def lpbk_handler(self, key, data): + def lpbk_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) - # Check if delete operation by fetch existing keys - keys = self.config_db.get_keys('LOOPBACK_INTERFACE') - if key in keys: - add = True - else: + if op == "DEL": add = False + else: + add = True self.iptables.iptables_handler(key, data, add) self.ntpcfg.handle_ntp_source_intf_chg(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) - def vlan_intf_handler(self, key, data): + def vlan_intf_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) - def vlan_sub_intf_handler(self, key, data): + def vlan_sub_intf_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) - def portchannel_intf_handler(self, key, data): + def portchannel_intf_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) - def phy_intf_handler(self, key, data): + def phy_intf_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) - def ntp_server_handler (self, key, data): + def ntp_server_handler (self, key, op, data): syslog.syslog(syslog.LOG_INFO, 'NTP server handler...') ntp_server_db = self.config_db.get_table('NTP_SERVER') data = ntp_server_db self.ntpcfg.ntp_server_update(key, data, False) - def ntp_global_handler (self, key, data): + def ntp_global_handler (self, key, op, data): syslog.syslog(syslog.LOG_INFO, 'NTP global handler...') self.ntpcfg.ntp_global_update(key, data, False) - def kdump_handler (self, key, data): + def kdump_handler (self, key, op, data): syslog.syslog(syslog.LOG_INFO, 'Kdump handler...') self.kdumpCfg.kdump_update(key, data, False) @@ -1000,36 +1018,60 @@ class HostConfigDaemon: systemctl_cmd = "sudo systemctl is-system-running --wait --quiet" subprocess.call(systemctl_cmd, shell=True) - def start(self): - self.config_db.subscribe('AAA', lambda table, key, data: self.aaa_handler(key, data)) - self.config_db.subscribe('TACPLUS_SERVER', lambda table, key, data: self.tacacs_server_handler(key, data)) - self.config_db.subscribe('TACPLUS', lambda table, key, data: self.tacacs_global_handler(key, data)) - self.config_db.subscribe('RADIUS_SERVER', lambda table, key, data: self.radius_server_handler(key, data)) - self.config_db.subscribe('RADIUS', lambda table, key, data: self.radius_global_handler(key, data)) - self.config_db.subscribe('MGMT_INTERFACE', lambda table, key, data: self.mgmt_intf_handler(key, data)) - self.config_db.subscribe('LOOPBACK_INTERFACE', lambda table, key, data: self.lpbk_handler(key, data)) - self.config_db.subscribe('FEATURE', lambda table, key, data: self.feature_handler.handle(key, data)) - self.config_db.subscribe('VLAN_INTERFACE', lambda table, key, data: self.vlan_intf_handler(key, data)) - self.config_db.subscribe('VLAN_SUB_INTERFACE', lambda table, key, data: self.vlan_sub_intf_handler(key, data)) - self.config_db.subscribe('PORTCHANNEL_INTERFACE', lambda table, key, data: self.portchannel_intf_handler(key, data)) - self.config_db.subscribe('INTERFACE', lambda table, key, data: self.phy_intf_handler(key, data)) - self.config_db.subscribe('NTP_SERVER', lambda table, key, data: self.ntp_server_handler(key, data)) - self.config_db.subscribe('NTP', lambda table, key, data: self.ntp_global_handler(key, data)) - self.config_db.subscribe('KDUMP', lambda table, key, data: self.kdump_handler(key, data)) + def subscribe(self, table, callback, pri): + if table not in self.callbacks: + self.callbacks[table] = [] + subscriber = self.SubscriberStateTable(self.dbconn, table, TableConsumable.DEFAULT_POP_BATCH_SIZE, pri) + self.selector.addSelectable(subscriber) # Add to the Selector + self.subscriber_map[subscriber.getFd()] = (subscriber, table) # Maintain a mapping b/w subscriber & fd + + self.callbacks[table].append(callback) + def start(self): + self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), PRIO_INIT) + # Handle Global NTP updated before NTP_SERVER updates + self.subscribe('NTP', lambda table, key, op, data: self.ntp_global_handler(key, op, data), PRIO_INIT-1) + self.subscribe('NTP_SERVER', lambda table, key, op, data: self.ntp_server_handler(key, op, data), PRIO_INIT-2) + # Handle FEATURE updates before other tables + self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), PRIO_INIT-3) + # Handle AAA, TACACS and RADIUS tables first + self.subscribe('AAA', lambda table, key, op, data: self.aaa_handler(key, op, data), PRIO_INIT-4) + self.subscribe('TACPLUS', lambda table, key, op, data: self.tacacs_global_handler(key, op, data), PRIO_INIT-4) + self.subscribe('TACPLUS_SERVER', lambda table, key, op, data: self.tacacs_server_handler(key, op, data), PRIO_INIT-4) + self.subscribe('RADIUS', lambda table, key, op, data: self.radius_global_handler(key, op, data), PRIO_INIT-4) + self.subscribe('RADIUS_SERVER', lambda table, key, op, data: self.radius_server_handler(key, op, data), PRIO_INIT-4) + self.subscribe('MGMT_INTERFACE', lambda table, key, op, data: self.mgmt_intf_handler(key, op, data), PRIO_INIT-5) + self.subscribe('LOOPBACK_INTERFACE', lambda table, key, op, data: self.lpbk_handler(key, op, data), PRIO_INIT-5) + self.subscribe('VLAN_INTERFACE', lambda table, key, op, data: self.vlan_intf_handler(key, op, data), PRIO_INIT-5) + self.subscribe('VLAN_SUB_INTERFACE', lambda table, key, op, data: self.vlan_sub_intf_handler(key, op, data), PRIO_INIT-5) + self.subscribe('PORTCHANNEL_INTERFACE', lambda table, key, op, data: self.portchannel_intf_handler(key, op, data), PRIO_INIT-5) + self.subscribe('INTERFACE', lambda table, key, op, data: self.phy_intf_handler(key, op, data), PRIO_INIT-5) + syslog.syslog(syslog.LOG_INFO, "Waiting for systemctl to finish initialization") self.wait_till_system_init_done() syslog.syslog(syslog.LOG_INFO, "systemctl has finished initialization -- proceeding ...") - # Update all feature states once upon starting - self.feature_handler.update_all_features_config() - - # Defer load until subscribe - self.load() - - self.config_db.listen() + while True: + state, selectable_ = self.selector.select(SELECT_TIMEOUT) + if state == self.selector.TIMEOUT: + continue + elif state == self.selector.ERROR: + raise Exception("Received error from select") + + fd = selectable_.getFd() + # Get the Corresponding subscriber & table + subscriber, table = subscriber_map.get(fd, (None, "")) + if not subscriber: + syslog.syslog(syslog.LOG_ERR, + "No Subscriber object found for fd: {}, subscriber map: {}".format(fd. subscriber_map)) + continue + key, op, fvs = subscriber.pop() + # Get the registered callback + callback = self.callbacks.get(table, None) + if callback: + callback(key, op dict(fvs)) def main(): @@ -1039,3 +1081,10 @@ def main(): if __name__ == "__main__": main() + +# self.cfgdb = DBConnector(CFG_DB, 0) +# syslog.syslog(syslog.LOG_INFO, 'ConfigDB connect success') + +# self.select = Select() +# self.callbacks = {} +# self.subscribers = set() \ No newline at end of file From 799943d937e0c126ea7a5620e01ea621558b75a4 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Wed, 8 Sep 2021 06:17:06 +0000 Subject: [PATCH 02/16] AAA tables edited Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 203 +++++++++++++---------- 1 file changed, 119 insertions(+), 84 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 71dd01ab8ead..14f4e5bd0623 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -11,6 +11,43 @@ import jinja2 from sonic_py_common import device_info from swsscommon.swsscommon import SubscriberStateTable, DBConnector, Select from swsscommon.swsscommom import ConfigDBConnector, TableConsumable +from jsondiff import diff + + +def is_true_cb(val): + return is_true(val) + + +def cfg_diff(cached, xcvd, callbacks={}): + """ + Returns the diff as formatted by jsondiff + + parameters: + cached: saved config + xcvd: modified config + callbacks: Mutate the values in xcvd for comparison + + returns: + df: diff as returned by jsondiff + xcvd_copy: recieved config mutated by callbacks + """ + if not cached and not xcvd: + return True + if isinstance(cached, dict) != isinstance(xcvd, dict): + return False + + cached_copy = copy.deepcopy(cached) + xcvd_copy = copy.deepcopy(xcvd) + for key in xcvd_copy: + func = callbacks.get(key, None) + if func and key in xcvd_copy: + try: + xcvd_copy[key] = func(xcvd_copy[key]) + except: + continue + df = diff(cached_copy, xcvd_copy) + return df, xcvd_copy + # FILE PAM_AUTH_CONF = "/etc/pam.d/common-auth-sonic" @@ -386,6 +423,7 @@ class Iptables(object): syslog.syslog(syslog.LOG_ERR, "'{}' failed. RC: {}, output: {}" .format(err.cmd, err.returncode, err.output)) + class AaaCfg(object): def __init__(self): self.auth_default = { @@ -433,14 +471,16 @@ class AaaCfg(object): self.modify_conf_file() def aaa_update(self, key, data, modify_conf=True): - if key == 'authentication': - self.auth = data - if 'failthrough' in data: - self.auth['failthrough'] = is_true(data['failthrough']) - if 'debug' in data: - self.debug = is_true(data['debug']) - if modify_conf: + if key.lower() != 'authentication': + return False + cbs = {'failthrough': is_true_cb} + df, data_modified = cfg_diff(self.auth, data, cbs) + self.auth = data_modified + if 'debug' in self.auth: + self.debug = is_true_cb(data['debug']) + if modify_conf and df: self.modify_conf_file() + return True if df not False def pick_src_intf_ipaddrs(self, keys, src_intf): new_ipv4_addr = "" @@ -466,30 +506,29 @@ class AaaCfg(object): return(new_ipv4_addr, new_ipv6_addr) def tacacs_global_update(self, key, data, modify_conf=True): - if key == 'global': - self.tacplus_global = data - if modify_conf: - self.modify_conf_file() + if key.lower() != 'global': + return False + df, data_modified = cfg_diff(self.tacplus_global, data, {}) + self.tacplus_global = data_modified + if modify_conf and df: + self.modify_conf_file() + return True if df not False def tacacs_server_update(self, key, data, modify_conf=True): - if data == {}: - if key in self.tacplus_servers: - del self.tacplus_servers[key] - else: - self.tacplus_servers[key] = data - - if modify_conf: + cached_data = self.tacplus_servers.get(key, {}) + df, data_modified = cfg_diff(cached_data, data, {}) + self.tacplus_servers[key] = data_modified + if modify_conf and df: self.modify_conf_file() + return True if df not False def handle_radius_source_intf_ip_chg(self, key): - modify_conf=False - if 'src_intf' in self.radius_global: - if key[0] == self.radius_global['src_intf']: - modify_conf=True + modify_conf = False + if key == self.radius_global.get('src_intf', ""): + modify_conf = True for addr in self.radius_servers: - if ('src_intf' in self.radius_servers[addr]) and \ - (key[0] == self.radius_servers[addr]['src_intf']): - modify_conf=True + if key == self.radius_servers.get(addr, {}).get('src_intf', ""): + modify_conf = True break if not modify_conf: @@ -514,21 +553,19 @@ class AaaCfg(object): self.modify_conf_file() def radius_global_update(self, key, data, modify_conf=True): - if key == 'global': - self.radius_global = data - if 'statistics' in data: - self.radius_global['statistics'] = is_true(data['statistics']) - if modify_conf: - self.modify_conf_file() + if key.lower() != 'global': + return None + cbs = {'statistics': is_true_cb} + df, data_modified = cfg_diff(self.radius_global, data, cbs) + self.radius_global = data_modified + if modify_conf and df: + self.modify_conf_file() def radius_server_update(self, key, data, modify_conf=True): - if data == {}: - if key in self.radius_servers: - del self.radius_servers[key] - else: - self.radius_servers[key] = data - - if modify_conf: + cached_data = self.radius_servers.get(key, {}) + df, data_modified = cfg_diff(cached_data, data, {}) + self.radius_servers[key] = data_modified + if modify_conf and df: self.modify_conf_file() def hostname_update(self, hostname, modify_conf=True): @@ -733,7 +770,7 @@ class KdumpCfg(object): def init(self, kdump_table): """ - Set the KDUMP table in CFG DB to kdump_defaults and load + Set the KDUMP table in CFG DB to kdump_defaults if npt set by the user """ syslog.syslog(syslog.LOG_INFO, "KdumpCfg init ...") kdump_conf = kdump_table.get("config", {}) @@ -851,7 +888,7 @@ class NtpCfg(object): run_cmd(cmd) def ntp_server_update(self, key, data, isLoad): - syslog.syslog(syslog.LOG_INFO, 'ntp server update key {} data {}'.format(key, op, data)) + syslog.syslog(syslog.LOG_INFO, 'ntp server update key {} data {}'.format(key, data)) # during load, restart ntp-config regardless if ntp server is configured or not if isLoad == True: @@ -901,11 +938,15 @@ class HostConfigDaemon: self.kdumpCfg = KdumpCfg(self.config_db) self.kdumpCfg.init(self.config_db.get_table('KDUMP')) - # Update all feature states once upon starting - # self.feature_handler.update_all_features_config() - - # Defer load until subscribe - # self.load() + try: + dev_meta = self.config_db.get_table('DEVICE_METADATA') + if 'localhost' in dev_meta: + if 'hostname' in dev_meta['localhost']: + self.hostname_cache = dev_meta['localhost']['hostname'] + except Exception as e: + pass + # Update AAA with the hostname + self.aaacfg.hostname_update(self.hostname_cache) def load(self): aaa = self.config_db.get_table('AAA') @@ -923,48 +964,46 @@ class HostConfigDaemon: ntp_global = self.config_db.get_table('NTP') self.ntpcfg.load(ntp_global, ntp_server) - try: - dev_meta = self.config_db.get_table('DEVICE_METADATA') - if 'localhost' in dev_meta: - if 'hostname' in dev_meta['localhost']: - self.hostname_cache = dev_meta['localhost']['hostname'] - except Exception as e: - pass - # Update AAA with the hostname - self.aaacfg.hostname_update(self.hostname_cache) - - - def aaa_handler(self, key, op, data): - self.aaacfg.aaa_update(key, op, data) + cfg_change = self.aaacfg.aaa_update(key, data) + if cfg_change: + syslog.syslog(syslog.LOG_INFO, 'AAA Update: key: {}, op: {}, data: {}'.format(key, op, data)) def tacacs_server_handler(self, key, op, data): - self.aaacfg.tacacs_server_update(key, op, data) + cfg_change = self.aaacfg.tacacs_server_update(key, data) + if not cfg_change: + return log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) - syslog.syslog(syslog.LOG_INFO, 'value of {} changed to {}'.format(key, log_data)) + syslog.syslog(syslog.LOG_INFO, 'TACPLUS_SERVER update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def tacacs_global_handler(self, key, op, data): - self.aaacfg.tacacs_global_update(key, op, data) + cfg_change = self.aaacfg.tacacs_global_update(key, op, data) + if not cfg_change: + return log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) - syslog.syslog(syslog.LOG_INFO, 'value of {} changed to {}'.format(key, log_data)) + syslog.syslog(syslog.LOG_INFO, 'TACPLUS Global update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def radius_server_handler(self, key, op, data): - self.aaacfg.radius_server_update(key, op, data) + cfg_change = self.aaacfg.radius_server_update(key, op, data) + if not cfg_change: + return log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) - syslog.syslog(syslog.LOG_INFO, 'value of {} changed to {}'.format(key, log_data)) + syslog.syslog(syslog.LOG_INFO, 'RADIUS_SERVER update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def radius_global_handler(self, key, op, data): - self.aaacfg.radius_global_update(key, op, data) + cfg_change = self.aaacfg.radius_global_update(key, op, data) + if not cfg_change: + return log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) - syslog.syslog(syslog.LOG_INFO, 'value of {} changed to {}'.format(key, log_data)) + syslog.syslog(syslog.LOG_INFO, 'RADIUS Global update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def mgmt_intf_handler(self, key, op, data): self.aaacfg.handle_radius_source_intf_ip_chg(key) @@ -1029,19 +1068,21 @@ class HostConfigDaemon: def start(self): self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), PRIO_INIT) - # Handle Global NTP updated before NTP_SERVER updates - self.subscribe('NTP', lambda table, key, op, data: self.ntp_global_handler(key, op, data), PRIO_INIT-1) - self.subscribe('NTP_SERVER', lambda table, key, op, data: self.ntp_server_handler(key, op, data), PRIO_INIT-2) # Handle FEATURE updates before other tables - self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), PRIO_INIT-3) - # Handle AAA, TACACS and RADIUS tables first - self.subscribe('AAA', lambda table, key, op, data: self.aaa_handler(key, op, data), PRIO_INIT-4) - self.subscribe('TACPLUS', lambda table, key, op, data: self.tacacs_global_handler(key, op, data), PRIO_INIT-4) - self.subscribe('TACPLUS_SERVER', lambda table, key, op, data: self.tacacs_server_handler(key, op, data), PRIO_INIT-4) - self.subscribe('RADIUS', lambda table, key, op, data: self.radius_global_handler(key, op, data), PRIO_INIT-4) - self.subscribe('RADIUS_SERVER', lambda table, key, op, data: self.radius_server_handler(key, op, data), PRIO_INIT-4) + self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), PRIO_INIT-1) + # Handle AAA, TACACS and RADIUS related tables + self.subscribe('AAA', lambda table, key, op, data: self.aaa_handler(key, op, data), PRIO_INIT-2) + self.subscribe('TACPLUS', lambda table, key, op, data: self.tacacs_global_handler(key, op, data), PRIO_INIT-2) + self.subscribe('TACPLUS_SERVER', lambda table, key, op, data: self.tacacs_server_handler(key, op, data), PRIO_INIT-2) + self.subscribe('RADIUS', lambda table, key, op, data: self.radius_global_handler(key, op, data), PRIO_INIT-2) + self.subscribe('RADIUS_SERVER', lambda table, key, op, data: self.radius_server_handler(key, op, data), PRIO_INIT-2) + # Handle IPTables configuration + self.subscribe('LOOPBACK_INTERFACE', lambda table, key, op, data: self.lpbk_handler(key, op, data), PRIO_INIT-3) + # Handle NTP & NTP_SERVER updates + self.subscribe('NTP', lambda table, key, op, data: self.ntp_global_handler(key, op, data), PRIO_INIT-4) + self.subscribe('NTP_SERVER', lambda table, key, op, data: self.ntp_server_handler(key, op, data), PRIO_INIT-4) + # Handle updates to src intf changes in radius self.subscribe('MGMT_INTERFACE', lambda table, key, op, data: self.mgmt_intf_handler(key, op, data), PRIO_INIT-5) - self.subscribe('LOOPBACK_INTERFACE', lambda table, key, op, data: self.lpbk_handler(key, op, data), PRIO_INIT-5) self.subscribe('VLAN_INTERFACE', lambda table, key, op, data: self.vlan_intf_handler(key, op, data), PRIO_INIT-5) self.subscribe('VLAN_SUB_INTERFACE', lambda table, key, op, data: self.vlan_sub_intf_handler(key, op, data), PRIO_INIT-5) self.subscribe('PORTCHANNEL_INTERFACE', lambda table, key, op, data: self.portchannel_intf_handler(key, op, data), PRIO_INIT-5) @@ -1071,7 +1112,7 @@ class HostConfigDaemon: # Get the registered callback callback = self.callbacks.get(table, None) if callback: - callback(key, op dict(fvs)) + callback(key, op, dict(fvs)) def main(): @@ -1082,9 +1123,3 @@ def main(): if __name__ == "__main__": main() -# self.cfgdb = DBConnector(CFG_DB, 0) -# syslog.syslog(syslog.LOG_INFO, 'ConfigDB connect success') - -# self.select = Select() -# self.callbacks = {} -# self.subscribers = set() \ No newline at end of file From 484f66ab12c4a9a9d9c1f6faf3fd8ad9b9f7b951 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Wed, 8 Sep 2021 07:49:39 +0000 Subject: [PATCH 03/16] Ntpcfg & Iptables updated Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 140 ++++++++--------------- 1 file changed, 49 insertions(+), 91 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 14f4e5bd0623..795fe672c38c 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -363,10 +363,6 @@ class Iptables(object): ''' return (isinstance(key, tuple)) - def load(self, lpbk_table): - for row in lpbk_table: - self.iptables_handler(row, lpbk_table[row]) - def command(self, chain, ip, ver, op): cmd = 'iptables' if ver == '4' else 'ip6tables' cmd += ' -t mangle --{} {} -p tcp --tcp-flags SYN SYN'.format(op, chain) @@ -524,10 +520,10 @@ class AaaCfg(object): def handle_radius_source_intf_ip_chg(self, key): modify_conf = False - if key == self.radius_global.get('src_intf', ""): + if key == self.radius_global.get('src_intf', ''): modify_conf = True for addr in self.radius_servers: - if key == self.radius_servers.get(addr, {}).get('src_intf', ""): + if key == self.radius_servers.get(addr, {}).get('src_intf', ''): modify_conf = True break @@ -538,12 +534,12 @@ class AaaCfg(object): self.modify_conf_file() def handle_radius_nas_ip_chg(self, key): - modify_conf=False + modify_conf = False # Mgmt IP configuration affects only the default nas_ip if 'nas_ip' not in self.radius_global: for addr in self.radius_servers: if 'nas_ip' not in self.radius_servers[addr]: - modify_conf=True + modify_conf = True break if not modify_conf: @@ -575,7 +571,7 @@ class AaaCfg(object): self.hostname = hostname # Currently only used for RADIUS - if len(self.radius_servers) == 0: + if not self.radius_servers: return if modify_conf: @@ -815,97 +811,66 @@ class NtpCfg(object): def __init__(self, CfgDb): self.config_db = CfgDb self.ntp_global = {} - self.has_ntp_servers = False - - def load(self, ntp_global_conf, ntp_server_conf): - syslog.syslog(syslog.LOG_INFO, "NtpCfg load ...") - - for row in ntp_global_conf: - self.ntp_global_update(row, ntp_global_conf[row], True) + self.ntp_servers = set() + self.init_flag = True - self.ntp_server_update(0, ntp_server_conf, True) - - def handle_ntp_source_intf_chg (self, key): + def handle_ntp_source_intf_chg(self, key): # if no ntp server configured, do nothing - if self.has_ntp_servers == False: + if not self.ntp_servers: return # check only the intf configured as source interface - if not self.ntp_global: - return - - if 'src_intf' not in self.ntp_global: - return - - if key[0] != self.ntp_global['src_intf']: + if key != self.ntp_global.get('src_intf', ''): return else: # just restart ntp config cmd = 'systemctl restart ntp-config' run_cmd(cmd) - def ntp_global_update(self, key, data, isLoad): - syslog.syslog(syslog.LOG_INFO, "ntp global configuration update") - - new_src = new_vrf = orig_src = orig_vrf = "" - - if 'src_intf' in data: - new_src = data['src_intf'] - - if 'vrf' in data: - new_vrf = data['vrf'] - - if not self.ntp_global: - if 'src_intf' in self.ntp_global: - orig_src = self.ntp_global['src_intf'] - - if 'vrf' in self.ntp_global: - orig_vrf = self.ntp_global['vrf'] + def ntp_global_update(self, key, data): + syslog.syslog(syslog.LOG_INFO, 'NTP GLOBAL Update') + orig_src = self.ntp_global.get('src_intf', '') + orig_vrf = self.ntp_global.get('vrf', '') + new_src = data.get('src_intf', '') + new_vrf = data.get('vrf', '') + # Update the Local Cache self.ntp_global = data - # during initial load of ntp configuration, ntp server configuration decides if to restart ntp-config - if (isLoad): - syslog.syslog(syslog.LOG_INFO, "ntp global update in load") - return + # during init, restart ntp-config regardless if ntp server is configured or not + if self.init_flag: + syslog.syslog(syslog.LOG_INFO, "NtpCfg load ...") + run_cmd('systemctl restart ntp-config') + self.init_flag = False + return # check if ntp server configured, if not, do nothing - if self.has_ntp_servers == False: - syslog.syslog(syslog.LOG_INFO, "no ntp server when global config change, do nothing") - return + if self.ntp_servers: + syslog.syslog(syslog.LOG_INFO, "No ntp server when global config change, do nothing") + return - if (new_src != orig_src): + if new_src != orig_src: syslog.syslog(syslog.LOG_INFO, "ntp global update for source intf old {} new {}, restarting ntp-config" .format(orig_src, new_src)) cmd = 'systemctl restart ntp-config' run_cmd(cmd) - else: - if (new_vrf != orig_vrf): - syslog.syslog(syslog.LOG_INFO, "ntp global update for vrf old {} new {}, restarting ntp service" - .format(orig_vrf, new_vrf)) - cmd = 'service ntp restart' - run_cmd(cmd) + elif new_vrf != orig_vrf: + syslog.syslog(syslog.LOG_INFO, "ntp global update for vrf old {} new {}, restarting ntp service" + .format(orig_vrf, new_vrf)) + cmd = 'service ntp restart' + run_cmd(cmd) - def ntp_server_update(self, key, data, isLoad): + def ntp_server_update(self, key, op): syslog.syslog(syslog.LOG_INFO, 'ntp server update key {} data {}'.format(key, data)) - # during load, restart ntp-config regardless if ntp server is configured or not - if isLoad == True: - if data != {}: - self.has_ntp_servers = True - else: - # for runtime ntp server change, to determine if there is ntp server configured, need to - # get from configDB, as delete triggers 2 event handling - ntp_servers_tbl = self.config_db.get_table('NTP_SERVER') - if ntp_servers_tbl != {}: - self.has_ntp_servers = True - else: - self.has_ntp_servers = False + if op == "SET": + self.ntp_servers.add(key) + elif op == "DEL": + self.ntp_servers.remove(key) cmd = 'systemctl restart ntp-config' - syslog.syslog(syslog.LOG_INFO, 'ntp server update, restarting ntp-config, ntp server exists {}'.format(self.has_ntp_servers)) - + syslog.syslog(syslog.LOG_INFO, 'ntp server update, restarting ntp-config, ntp servers configured {}'.format(self.ntp_servers)) run_cmd(cmd) @@ -1010,7 +975,6 @@ class HostConfigDaemon: self.aaacfg.handle_radius_nas_ip_chg(key) def lpbk_handler(self, key, op, data): - key = ConfigDBConnector.deserialize_key(key) if op == "DEL": add = False else: @@ -1021,50 +985,44 @@ class HostConfigDaemon: self.aaacfg.handle_radius_source_intf_ip_chg(key) def vlan_intf_handler(self, key, op, data): - key = ConfigDBConnector.deserialize_key(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) def vlan_sub_intf_handler(self, key, op, data): - key = ConfigDBConnector.deserialize_key(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) def portchannel_intf_handler(self, key, op, data): - key = ConfigDBConnector.deserialize_key(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) def phy_intf_handler(self, key, op, data): - key = ConfigDBConnector.deserialize_key(key) self.aaacfg.handle_radius_source_intf_ip_chg(key) def ntp_server_handler (self, key, op, data): - syslog.syslog(syslog.LOG_INFO, 'NTP server handler...') - ntp_server_db = self.config_db.get_table('NTP_SERVER') - data = ntp_server_db - self.ntpcfg.ntp_server_update(key, data, False) + self.ntpcfg.ntp_server_update(key, op) def ntp_global_handler (self, key, op, data): - syslog.syslog(syslog.LOG_INFO, 'NTP global handler...') - self.ntpcfg.ntp_global_update(key, data, False) + self.ntpcfg.ntp_global_update(key, data) def kdump_handler (self, key, op, data): syslog.syslog(syslog.LOG_INFO, 'Kdump handler...') self.kdumpCfg.kdump_update(key, data, False) def wait_till_system_init_done(self): - # No need to print the output in the log file so using the "--quiet" # flag systemctl_cmd = "sudo systemctl is-system-running --wait --quiet" subprocess.call(systemctl_cmd, shell=True) def subscribe(self, table, callback, pri): - if table not in self.callbacks: - self.callbacks[table] = [] - subscriber = self.SubscriberStateTable(self.dbconn, table, TableConsumable.DEFAULT_POP_BATCH_SIZE, pri) - self.selector.addSelectable(subscriber) # Add to the Selector - self.subscriber_map[subscriber.getFd()] = (subscriber, table) # Maintain a mapping b/w subscriber & fd + try: + if table not in self.callbacks: + self.callbacks[table] = [] + subscriber = SubscriberStateTable(self.dbconn, table, TableConsumable.DEFAULT_POP_BATCH_SIZE, pri) + self.selector.addSelectable(subscriber) # Add to the Selector + self.subscriber_map[subscriber.getFd()] = (subscriber, table) # Maintain a mapping b/w subscriber & fd - self.callbacks[table].append(callback) + self.callbacks[table].append(callback) + except Exception as err: + syslog.syslog(syslog.LOG_ERR, "Subscribe to table {} failed with error {}".format(table, err)) def start(self): self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), PRIO_INIT) From fd28bb406111b57a14248591098f0db07ea7159a Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Thu, 9 Sep 2021 22:59:19 +0000 Subject: [PATCH 04/16] Feature handler and UT's completed Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 79 ++++++++++++------- src/sonic-host-services/setup.py | 5 +- .../tests/hostcfgd/hostcfgd_test.py | 39 ++++----- 3 files changed, 77 insertions(+), 46 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 795fe672c38c..e93c3ab0f99c 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -8,9 +8,9 @@ import subprocess import syslog import jinja2 -from sonic_py_common import device_info +from sonic_py_common.device_info import is_multi_npu, get_num_npus from swsscommon.swsscommon import SubscriberStateTable, DBConnector, Select -from swsscommon.swsscommom import ConfigDBConnector, TableConsumable +from swsscommon.swsscommon import ConfigDBConnector, TableConsumable from jsondiff import diff @@ -129,9 +129,9 @@ class Feature(object): self.name = feature_name self.state = self._get_target_state(feature_cfg.get('state'), device_config or {}) self.auto_restart = feature_cfg.get('auto_restart', 'disabled') - self.has_timer = ast.literal_eval(feature_cfg.get('has_timer', 'False')) - self.has_global_scope = ast.literal_eval(feature_cfg.get('has_global_scope', 'True')) - self.has_per_asic_scope = ast.literal_eval(feature_cfg.get('has_per_asic_scope', 'False')) + self.has_timer = self.__safe_eval_bool(feature_cfg.get('has_timer', 'False')) + self.has_global_scope = self.__safe_eval_bool(feature_cfg.get('has_global_scope', 'True')) + self.has_per_asic_scope = self.__safe_eval_bool(feature_cfg.get('has_per_asic_scope', 'False')) def _get_target_state(self, state_configuration, device_config): """ Returns the target state for the feature by rendering the state field as J2 template. @@ -152,6 +152,22 @@ class Feature(object): raise ValueError('Invalid state rendered for feature {}: {}'.format(self.name, target_state)) return target_state + def __safe_eval_bool(self, val): + """ Safely evaluate the boolean expression, without raising an exception """ + try: + ret = ast.literal_eval(val) + except Exception as e: + ret = False + return ret + + def compare_state(self, feature_name, feature_cfg): + if self.name != feature_name or not isinstance(feature_cfg, dict): + return False + + if self.state != feature_cfg.get('state', ''): + return False + return True + class FeatureHandler(object): """ Handles FEATURE table updates. """ @@ -163,7 +179,7 @@ class FeatureHandler(object): self._config_db = config_db self._device_config = device_config self._cached_config = {} - self.is_multi_npu = device_info.is_multi_npu() + self.is_multi_npu_device = is_multi_npu() def handle(self, feature_name, feature_cfg): if not feature_cfg: @@ -179,8 +195,17 @@ class FeatureHandler(object): # the next called self.update_feature_state will start it again. If it will fail # again the auto restart will kick-in. Another order may leave it in failed state # and not auto restart. - if self._cached_config[feature_name].auto_restart != feature.auto_restart: + + dir_name = self.SYSTEMD_SERVICE_CONF_DIR.format(feature_name) + auto_restart_conf = os.path.join(dir_name, 'auto_restart.conf') + updated_auto_restart = False + if not os.path.exists(auto_restart_conf): # if the auto_restart_conf file is not found, set it self.update_feature_auto_restart(feature) + updated_auto_restart = True + + if self._cached_config[feature_name].auto_restart != feature.auto_restart: + if not updated_auto_restart: + self.update_feature_auto_restart(feature) self._cached_config[feature_name].auto_restart = feature.auto_restart # Enable/disable the container service if the feature state was changed from its previous state. @@ -190,7 +215,12 @@ class FeatureHandler(object): else: self.resync_feature_state(self._cached_config[feature_name]) - def update_all_features_config(self): + def sync_state_field(self): + """ + Summary: + Updates the state field in the FEATURE|* tables as the state field + might have to to be rendered based on DEVICE_METADATA table + """ feature_table = self._config_db.get_table('FEATURE') for feature_name in feature_table.keys(): if not feature_name: @@ -198,12 +228,8 @@ class FeatureHandler(object): continue feature = Feature(feature_name, feature_table[feature_name], self._device_config) - self._cached_config.setdefault(feature_name, feature) - - self.update_feature_auto_restart(feature) - - self.update_feature_state(feature) - self.resync_feature_state(feature) + if not feature.compare_state(feature_name, feature_table.get(feature_name, {})): + self.resync_feature_state(feature) def update_feature_state(self, feature): cached_feature = self._cached_config[feature.name] @@ -266,9 +292,9 @@ class FeatureHandler(object): def get_feature_attribute(self, feature): # Create feature name suffix depending feature is running in host or namespace or in both feature_names = ( - ([feature.name] if feature.has_global_scope or not self.is_multi_npu else []) + - ([(feature.name + '@' + str(asic_inst)) for asic_inst in range(device_info.get_num_npus()) - if feature.has_per_asic_scope and self.is_multi_npu]) + ([feature.name] if feature.has_global_scope or not self.is_multi_npu_device else []) + + ([(feature.name + '@' + str(asic_inst)) for asic_inst in range(get_num_npus()) + if feature.has_per_asic_scope and self.is_multi_npu_device]) ) if not feature_names: @@ -292,7 +318,6 @@ class FeatureHandler(object): props = dict([line.split("=") for line in stdout.decode().strip().splitlines()]) return props["UnitFileState"] - def enable_feature(self, feature): cmds = [] feature_names, feature_suffixes = self.get_feature_attribute(feature) @@ -476,7 +501,7 @@ class AaaCfg(object): self.debug = is_true_cb(data['debug']) if modify_conf and df: self.modify_conf_file() - return True if df not False + return True if df else False def pick_src_intf_ipaddrs(self, keys, src_intf): new_ipv4_addr = "" @@ -508,7 +533,7 @@ class AaaCfg(object): self.tacplus_global = data_modified if modify_conf and df: self.modify_conf_file() - return True if df not False + return True if df else False def tacacs_server_update(self, key, data, modify_conf=True): cached_data = self.tacplus_servers.get(key, {}) @@ -516,7 +541,7 @@ class AaaCfg(object): self.tacplus_servers[key] = data_modified if modify_conf and df: self.modify_conf_file() - return True if df not False + return True if df else False def handle_radius_source_intf_ip_chg(self, key): modify_conf = False @@ -808,11 +833,10 @@ class KdumpCfg(object): run_cmd("sonic-kdump-config --num_dumps " + num_dumps) class NtpCfg(object): - def __init__(self, CfgDb): - self.config_db = CfgDb + def __init__(self): self.ntp_global = {} self.ntp_servers = set() - self.init_flag = True + self.init_flag = True def handle_ntp_source_intf_chg(self, key): # if no ntp server configured, do nothing @@ -895,9 +919,10 @@ class HostConfigDaemon: self.aaacfg = AaaCfg() self.iptables = Iptables() self.feature_handler = FeatureHandler(self.config_db, self.device_config) - self.ntpcfg = NtpCfg(self.config_db) + self.feature_handler.sync_state_field() + self.ntpcfg = NtpCfg() - self.is_multi_npu = device_info.is_multi_npu() + self.is_multi_npu_device = is_multi_npu() # Load Kdump configuration self.kdumpCfg = KdumpCfg(self.config_db) @@ -1027,7 +1052,7 @@ class HostConfigDaemon: def start(self): self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), PRIO_INIT) # Handle FEATURE updates before other tables - self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), PRIO_INIT-1) + self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, data), PRIO_INIT-1) # Handle AAA, TACACS and RADIUS related tables self.subscribe('AAA', lambda table, key, op, data: self.aaa_handler(key, op, data), PRIO_INIT-2) self.subscribe('TACPLUS', lambda table, key, op, data: self.tacacs_global_handler(key, op, data), PRIO_INIT-2) diff --git a/src/sonic-host-services/setup.py b/src/sonic-host-services/setup.py index 3d90a93dbaa5..9fd9901117a1 100644 --- a/src/sonic-host-services/setup.py +++ b/src/sonic-host-services/setup.py @@ -28,6 +28,7 @@ 'PyGObject', 'sonic-py-common', 'systemd-python', + 'jsondiff' ], setup_requires = [ 'pytest-runner', @@ -37,7 +38,9 @@ 'parameterized', 'pytest', 'pyfakefs', - 'sonic-py-common' + 'sonic-py-common', + 'jsondiff', + 'deepdiff' ], classifiers = [ 'Development Status :: 3 - Alpha', diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py index 9bd82a76afc7..2e0b64227c0f 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py @@ -10,9 +10,8 @@ from tests.common.mock_configdb import MockConfigDb from pyfakefs.fake_filesystem_unittest import patchfs +from deepdiff import DeepDiff - -swsscommon.swsscommon.ConfigDBConnector = MockConfigDb test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) modules_path = os.path.dirname(test_path) scripts_path = os.path.join(modules_path, "scripts") @@ -21,6 +20,7 @@ # Load the file under test hostcfgd_path = os.path.join(scripts_path, 'hostcfgd') hostcfgd = load_module_from_source('hostcfgd', hostcfgd_path) +hostcfgd.ConfigDBConnector = MockConfigDb class TestHostcfgd(TestCase): @@ -40,18 +40,9 @@ def __verify_table(self, table, expected_table): Returns: None """ - is_equal = len(table) == len(expected_table) - if is_equal: - for key, fields in expected_table.items(): - is_equal = is_equal and key in table and len(fields) == len(table[key]) - if is_equal: - for field, value in fields.items(): - is_equal = is_equal and value == table[key][field] - if not is_equal: - break; - else: - break - return is_equal + ddiff = DeepDiff(table, expected_table, ignore_order=True) + print("DIFF:", ddiff) + return True if not ddiff else False def __verify_fs(self, table): """ @@ -82,9 +73,9 @@ def __verify_fs(self, table): @parameterized.expand(HOSTCFGD_TEST_VECTOR) @patchfs - def test_hostcfgd(self, test_name, test_data, fs): + def test_hostcfgd_feature_handler(self, test_name, test_data, fs): """ - Test hostcfd daemon initialization + Test feature config capability in the hostcfd Args: test_name(str): test name @@ -101,9 +92,21 @@ def test_hostcfgd(self, test_name, test_data, fs): attrs = test_data["popen_attributes"] popen_mock.configure_mock(**attrs) mocked_subprocess.Popen.return_value = popen_mock + hostcfgd.is_multi_npu = mock.Mock(return_value=False) + hostcfgd.get_num_npus = mock.Mock(return_value=1) + + # Initialize Feature Handler + device_config = {} + device_config['DEVICE_METADATA'] = MockConfigDb.CONFIG_DB['DEVICE_METADATA'] + feature_handler = hostcfgd.FeatureHandler(MockConfigDb(), device_config) + + # sync the state field and Handle Feature Updates + feature_handler.sync_state_field() + features = MockConfigDb.CONFIG_DB['FEATURE'] + for key, fvs in features.items(): + feature_handler.handle(key, fvs) - host_config_daemon = hostcfgd.HostConfigDaemon() - host_config_daemon.feature_handler.update_all_features_config() + # Verify if the updates are properly updated assert self.__verify_table( MockConfigDb.get_config_db()["FEATURE"], test_data["expected_config_db"]["FEATURE"] From 02741a1c5dc6471e5180cecd816ecf8b8460625f Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 10 Sep 2021 00:08:27 +0000 Subject: [PATCH 05/16] AAA & RADIUS test in prog Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 21 ++++-------- .../tests/hostcfgd/hostcfgd_radius_test.py | 34 ++++++++++++------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index e93c3ab0f99c..ab77cdbbaabf 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -438,11 +438,7 @@ class Iptables(object): for cmd in iptables_cmds: syslog.syslog(syslog.LOG_INFO, "Running cmd - {}".format(cmd)) - try: - subprocess.check_call(cmd, shell=True) - except subprocess.CalledProcessError as err: - syslog.syslog(syslog.LOG_ERR, "'{}' failed. RC: {}, output: {}" - .format(err.cmd, err.returncode, err.output)) + run_cmd(cmd) class AaaCfg(object): @@ -497,7 +493,7 @@ class AaaCfg(object): cbs = {'failthrough': is_true_cb} df, data_modified = cfg_diff(self.auth, data, cbs) self.auth = data_modified - if 'debug' in self.auth: + if 'debug' in data: self.debug = is_true_cb(data['debug']) if modify_conf and df: self.modify_conf_file() @@ -775,12 +771,7 @@ class AaaCfg(object): else: cmd = 'service aaastatsd stop' syslog.syslog(syslog.LOG_INFO, "cmd - {}".format(cmd)) - try: - subprocess.check_call(cmd, shell=True) - except subprocess.CalledProcessError as err: - syslog.syslog(syslog.LOG_ERR, - "{} - failed: return code - {}, output:\n{}" - .format(err.cmd, err.returncode, err.output)) + run_cmd(cmd) class KdumpCfg(object): def __init__(self, CfgDb): @@ -969,7 +960,7 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_INFO, 'TACPLUS_SERVER update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def tacacs_global_handler(self, key, op, data): - cfg_change = self.aaacfg.tacacs_global_update(key, op, data) + cfg_change = self.aaacfg.tacacs_global_update(key, data) if not cfg_change: return log_data = copy.deepcopy(data) @@ -978,7 +969,7 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_INFO, 'TACPLUS Global update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def radius_server_handler(self, key, op, data): - cfg_change = self.aaacfg.radius_server_update(key, op, data) + cfg_change = self.aaacfg.radius_server_update(key, data) if not cfg_change: return log_data = copy.deepcopy(data) @@ -987,7 +978,7 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_INFO, 'RADIUS_SERVER update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def radius_global_handler(self, key, op, data): - cfg_change = self.aaacfg.radius_global_update(key, op, data) + cfg_change = self.aaacfg.radius_global_update(key, data) if not cfg_change: return log_data = copy.deepcopy(data) diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py index 5ae05bd9ac6e..ebf55eda970c 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py @@ -37,7 +37,7 @@ class TestHostcfgdRADIUS(TestCase): Test hostcfd daemon - RADIUS """ def run_diff(self, file1, file2): - return subprocess.check_output('diff -uR {} {} || true'.format(file1, file2), shell=True) + return subprocess.check_output('diff -u {} {} || true'.format(file1, file2), shell=True).decode('utf-8').strip() @parameterized.expand(HOSTCFGD_TEST_RADIUS_VECTOR) @@ -69,34 +69,42 @@ def test_hostcfgd_radius(self, test_name, test_data): hostcfgd.ETC_PAMD_LOGIN = op_path + "/login" hostcfgd.RADIUS_PAM_AUTH_CONF_DIR = op_path + "/" - shutil.rmtree( op_path, ignore_errors=True) - os.mkdir( op_path) + shutil.rmtree(op_path, ignore_errors=True) + os.mkdir(op_path) - shutil.copyfile( sop_path + "/sshd.old", op_path + "/sshd") - shutil.copyfile( sop_path + "/login.old", op_path + "/login") + shutil.copyfile(sop_path + "/sshd.old", op_path + "/sshd") + shutil.copyfile(sop_path + "/login.old", op_path + "/login") MockConfigDb.set_config_db(test_data["config_db"]) - host_config_daemon = hostcfgd.HostConfigDaemon() - - aaa = host_config_daemon.config_db.get_table('AAA') + # host_config_daemon = hostcfgd.HostConfigDaemon() + aaacfg = hostcfgd.AaaCfg() + aaacfg.hostname_update(MockConfigDb.CONFIG_DB["DEVICE_METADATA"]["localhost"]["hostname"]) + aaa = MockConfigDb().get_table('AAA') try: - radius_global = host_config_daemon.config_db.get_table('RADIUS') + radius_global = MockConfigDb().get_table('RADIUS') except: - radius_global = [] + radius_global = {} try: radius_server = \ - host_config_daemon.config_db.get_table('RADIUS_SERVER') + MockConfigDb().get_table('RADIUS_SERVER') except: - radius_server = [] + radius_server = {} + + for key, fvs in aaa.items(): + aaacfg.aaa_update(key, fvs) + for key, fvs in radius_global.items(): + aaacfg.radius_global_update(key, fvs) + for key, fvs in radius_server.items(): + aaacfg.radius_server_update(key, fvs) - host_config_daemon.aaacfg.load(aaa,[],[],radius_global,radius_server) dcmp = filecmp.dircmp(sop_path, op_path) diff_output = "" for name in dcmp.diff_files: diff_output += \ "Diff: file: {} expected: {} output: {}\n".format(\ name, dcmp.left, dcmp.right) + print(diff_output) diff_output += self.run_diff( dcmp.left + "/" + name,\ dcmp.right + "/" + name) self.assertTrue(len(diff_output) == 0, diff_output) From 50ff897176fe0b191d7e8ebe904ded17bf5542dc Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 10 Sep 2021 08:56:41 +0000 Subject: [PATCH 06/16] NtpCfg Completed, AaaCfgd is progress Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 128 +++++++++++------- .../tests/hostcfgd/hostcfgd_radius_test.py | 67 ++++----- .../tests/hostcfgd/hostcfgd_test.py | 53 +++++++- 3 files changed, 166 insertions(+), 82 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index ab77cdbbaabf..c68f14c196b9 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -181,7 +181,7 @@ class FeatureHandler(object): self._cached_config = {} self.is_multi_npu_device = is_multi_npu() - def handle(self, feature_name, feature_cfg): + def handle(self, feature_name, op, feature_cfg): if not feature_cfg: self._cached_config.pop(feature_name) syslog.syslog(syslog.LOG_INFO, "Deregistering feature {}".format(feature_name)) @@ -442,7 +442,9 @@ class Iptables(object): class AaaCfg(object): - def __init__(self): + def __init__(self, cfg_db): + self.config_db = cfg_db + self.auth_default = { 'login': 'local', } @@ -494,7 +496,7 @@ class AaaCfg(object): df, data_modified = cfg_diff(self.auth, data, cbs) self.auth = data_modified if 'debug' in data: - self.debug = is_true_cb(data['debug']) + self.debug = is_true(data['debug']) if modify_conf and df: self.modify_conf_file() return True if df else False @@ -539,19 +541,19 @@ class AaaCfg(object): self.modify_conf_file() return True if df else False - def handle_radius_source_intf_ip_chg(self, key): + def handle_radius_source_intf_ip_chg(self, intf): modify_conf = False - if key == self.radius_global.get('src_intf', ''): + if intf == self.radius_global.get('src_intf', ''): modify_conf = True for addr in self.radius_servers: - if key == self.radius_servers.get(addr, {}).get('src_intf', ''): + if intf == self.radius_servers.get(addr, {}).get('src_intf', ''): modify_conf = True break if not modify_conf: return - syslog.syslog(syslog.LOG_INFO, 'RADIUS IP change - key:{}, current server info {}'.format(key, self.radius_servers)) + syslog.syslog(syslog.LOG_INFO, 'RADIUS IP change - Interface:{}, current server info {}'.format(intf, self.radius_servers)) self.modify_conf_file() def handle_radius_nas_ip_chg(self, key): @@ -824,18 +826,24 @@ class KdumpCfg(object): run_cmd("sonic-kdump-config --num_dumps " + num_dumps) class NtpCfg(object): + """ + NtpCfg Config Daemon + 1) ntp-config.service handles the configuration updates and then starts ntp.service + 2) Both of them start after all the feature services start + 3) Purpose of this daemon is to propagate runtime config changes in + NTP, NTP_SERVER and LOOPBACK_INTERFACE + """ def __init__(self): self.ntp_global = {} self.ntp_servers = set() - self.init_flag = True - def handle_ntp_source_intf_chg(self, key): + def handle_ntp_source_intf_chg(self, intf_name): # if no ntp server configured, do nothing if not self.ntp_servers: return # check only the intf configured as source interface - if key != self.ntp_global.get('src_intf', ''): + if intf_name != self.ntp_global.get('src_intf', ''): return else: # just restart ntp config @@ -845,29 +853,24 @@ class NtpCfg(object): def ntp_global_update(self, key, data): syslog.syslog(syslog.LOG_INFO, 'NTP GLOBAL Update') orig_src = self.ntp_global.get('src_intf', '') + orig_src_set = set(orig_src.split(";")) orig_vrf = self.ntp_global.get('vrf', '') new_src = data.get('src_intf', '') + new_src_set = set(new_src.split(";")) new_vrf = data.get('vrf', '') # Update the Local Cache self.ntp_global = data - # during init, restart ntp-config regardless if ntp server is configured or not - if self.init_flag: - syslog.syslog(syslog.LOG_INFO, "NtpCfg load ...") - run_cmd('systemctl restart ntp-config') - self.init_flag = False - return - # check if ntp server configured, if not, do nothing - if self.ntp_servers: + if not self.ntp_servers: syslog.syslog(syslog.LOG_INFO, "No ntp server when global config change, do nothing") return - if new_src != orig_src: + if orig_src_set != new_src_set: syslog.syslog(syslog.LOG_INFO, "ntp global update for source intf old {} new {}, restarting ntp-config" - .format(orig_src, new_src)) + .format(orig_src_set, new_src_set)) cmd = 'systemctl restart ntp-config' run_cmd(cmd) elif new_vrf != orig_vrf: @@ -877,17 +880,20 @@ class NtpCfg(object): run_cmd(cmd) def ntp_server_update(self, key, op): - syslog.syslog(syslog.LOG_INFO, 'ntp server update key {} data {}'.format(key, data)) + syslog.syslog(syslog.LOG_INFO, 'ntp server update key {}'.format(key)) - if op == "SET": + restart_config = False + if op == "SET" and key not in self.ntp_servers: + restart_config = True self.ntp_servers.add(key) - elif op == "DEL": + elif op == "DEL" and key in self.ntp_server: + restart_config = True self.ntp_servers.remove(key) - cmd = 'systemctl restart ntp-config' - syslog.syslog(syslog.LOG_INFO, 'ntp server update, restarting ntp-config, ntp servers configured {}'.format(self.ntp_servers)) - run_cmd(cmd) - + if restart_config: + cmd = 'systemctl restart ntp-config' + syslog.syslog(syslog.LOG_INFO, 'ntp server update, restarting ntp-config, ntp servers configured {}'.format(self.ntp_servers)) + run_cmd(cmd) class HostConfigDaemon: def __init__(self): @@ -906,19 +912,22 @@ class HostConfigDaemon: self.device_config = {} self.device_config['DEVICE_METADATA'] = self.config_db.get_table('DEVICE_METADATA') - self.hostname_cache="" - self.aaacfg = AaaCfg() + # Initialize KDump Config and set the config to default if nothing is provided + self.kdumpCfg = KdumpCfg(self.config_db) + self.kdumpCfg.init(self.config_db.get_table('KDUMP')) + + # Initialize IpTables self.iptables = Iptables() + + # Intialize Feature Handler self.feature_handler = FeatureHandler(self.config_db, self.device_config) self.feature_handler.sync_state_field() + + # Initialize Ntp Config Handler self.ntpcfg = NtpCfg() self.is_multi_npu_device = is_multi_npu() - # Load Kdump configuration - self.kdumpCfg = KdumpCfg(self.config_db) - self.kdumpCfg.init(self.config_db.get_table('KDUMP')) - try: dev_meta = self.config_db.get_table('DEVICE_METADATA') if 'localhost' in dev_meta: @@ -926,6 +935,10 @@ class HostConfigDaemon: self.hostname_cache = dev_meta['localhost']['hostname'] except Exception as e: pass + + # Initialize AAACfg + self.hostname_cache="" + self.aaacfg = AaaCfg(self.config_db) # Update AAA with the hostname self.aaacfg.hostname_update(self.hostname_cache) @@ -940,10 +953,12 @@ class HostConfigDaemon: lpbk_table = self.config_db.get_table('LOOPBACK_INTERFACE') self.iptables.load(lpbk_table) - # Load NTP configurations - ntp_server = self.config_db.get_table('NTP_SERVER') - ntp_global = self.config_db.get_table('NTP') - self.ntpcfg.load(ntp_global, ntp_server) + def __get_intf_name(self, key): + if isinstance(key, tuple) and key: + intf = key[0] + else: + intf = key + return intf def aaa_handler(self, key, op, data): cfg_change = self.aaacfg.aaa_update(key, data) @@ -987,35 +1002,47 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_INFO, 'RADIUS Global update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def mgmt_intf_handler(self, key, op, data): - self.aaacfg.handle_radius_source_intf_ip_chg(key) - self.aaacfg.handle_radius_nas_ip_chg(key) + key = ConfigDBConnector.deserialize_key(key) + mgmt_intf_name = self.__get_intf_name(key) + self.aaacfg.handle_radius_source_intf_ip_chg(mgmt_intf_name) + self.aaacfg.handle_radius_nas_ip_chg(mgmt_intf_name) def lpbk_handler(self, key, op, data): + key = ConfigDBConnector.deserialize_key(key) if op == "DEL": add = False else: add = True self.iptables.iptables_handler(key, data, add) - self.ntpcfg.handle_ntp_source_intf_chg(key) - self.aaacfg.handle_radius_source_intf_ip_chg(key) + lpbk_name = self.__get_intf_name(key) + self.ntpcfg.handle_ntp_source_intf_chg(lpbk_name) + self.aaacfg.handle_radius_source_intf_ip_chg(lpbk_name) def vlan_intf_handler(self, key, op, data): - self.aaacfg.handle_radius_source_intf_ip_chg(key) + key = ConfigDBConnector.deserialize_key(key) + vlan_name = self.__get_intf_name(key) + self.aaacfg.handle_radius_source_intf_ip_chg(vlan_name) def vlan_sub_intf_handler(self, key, op, data): - self.aaacfg.handle_radius_source_intf_ip_chg(key) + key = ConfigDBConnector.deserialize_key(key) + sub_intf_name = self.__get_intf_name(key) + self.aaacfg.handle_radius_source_intf_ip_chg(sub_intf_name) def portchannel_intf_handler(self, key, op, data): - self.aaacfg.handle_radius_source_intf_ip_chg(key) + key = ConfigDBConnector.deserialize_key(key) + lag_name = self.__get_intf_name(key) + self.aaacfg.handle_radius_source_intf_ip_chg(lag_name) def phy_intf_handler(self, key, op, data): - self.aaacfg.handle_radius_source_intf_ip_chg(key) + key = ConfigDBConnector.deserialize_key(key) + phy_intf_name = self.__get_intf_name(key) + self.aaacfg.handle_radius_source_intf_ip_chg(phy_intf_name) - def ntp_server_handler (self, key, op, data): + def ntp_server_handler(self, key, op, data): self.ntpcfg.ntp_server_update(key, op) - def ntp_global_handler (self, key, op, data): + def ntp_global_handler(self, key, op, data): self.ntpcfg.ntp_global_update(key, data) def kdump_handler (self, key, op, data): @@ -1043,7 +1070,7 @@ class HostConfigDaemon: def start(self): self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), PRIO_INIT) # Handle FEATURE updates before other tables - self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, data), PRIO_INIT-1) + self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), PRIO_INIT-1) # Handle AAA, TACACS and RADIUS related tables self.subscribe('AAA', lambda table, key, op, data: self.aaa_handler(key, op, data), PRIO_INIT-2) self.subscribe('TACPLUS', lambda table, key, op, data: self.tacacs_global_handler(key, op, data), PRIO_INIT-2) @@ -1068,6 +1095,7 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_INFO, "systemctl has finished initialization -- proceeding ...") + init_load_flag = True while True: state, selectable_ = self.selector.select(SELECT_TIMEOUT) if state == self.selector.TIMEOUT: @@ -1087,6 +1115,10 @@ class HostConfigDaemon: callback = self.callbacks.get(table, None) if callback: callback(key, op, dict(fvs)) + + if init_load_flag and table == "FEATURE" and key.lower() == "swss": + init_load_flag = False + self.load() def main(): diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py index ebf55eda970c..e476190a7382 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py @@ -75,36 +75,37 @@ def test_hostcfgd_radius(self, test_name, test_data): shutil.copyfile(sop_path + "/sshd.old", op_path + "/sshd") shutil.copyfile(sop_path + "/login.old", op_path + "/login") - MockConfigDb.set_config_db(test_data["config_db"]) - # host_config_daemon = hostcfgd.HostConfigDaemon() - aaacfg = hostcfgd.AaaCfg() - aaacfg.hostname_update(MockConfigDb.CONFIG_DB["DEVICE_METADATA"]["localhost"]["hostname"]) - aaa = MockConfigDb().get_table('AAA') - - try: - radius_global = MockConfigDb().get_table('RADIUS') - except: - radius_global = {} - try: - radius_server = \ - MockConfigDb().get_table('RADIUS_SERVER') - except: - radius_server = {} - - for key, fvs in aaa.items(): - aaacfg.aaa_update(key, fvs) - for key, fvs in radius_global.items(): - aaacfg.radius_global_update(key, fvs) - for key, fvs in radius_server.items(): - aaacfg.radius_server_update(key, fvs) - - dcmp = filecmp.dircmp(sop_path, op_path) - diff_output = "" - for name in dcmp.diff_files: - diff_output += \ - "Diff: file: {} expected: {} output: {}\n".format(\ - name, dcmp.left, dcmp.right) - print(diff_output) - diff_output += self.run_diff( dcmp.left + "/" + name,\ - dcmp.right + "/" + name) - self.assertTrue(len(diff_output) == 0, diff_output) + with mock.patch("hostcfgd.subprocess") as mocked_subprocess: + popen_mock = mock.Mock() + attrs = {'communicate.return_value': ('output', 'error')} + popen_mock.configure_mock(**attrs) + mocked_subprocess.Popen.return_value = popen_mock + MockConfigDb.set_config_db(test_data["config_db"]) + + aaacfg = hostcfgd.AaaCfg() + aaacfg.hostname_update(MockConfigDb.CONFIG_DB["DEVICE_METADATA"]["localhost"]["hostname"]) + aaa = MockConfigDb().get_table('AAA') + + try: + radius_global = MockConfigDb().get_table('RADIUS') + except: + radius_global = [] + try: + radius_server = \ + MockConfigDb().get_table('RADIUS_SERVER') + except: + radius_server = [] + + aaacfg.load(aaa,[],[],radius_global,radius_server) + # print(aaacfg.auth, aaacfg.radius_global, aaacfg.radius_servers) + mocked_subprocess.check_call.assert_has_calls(test_data["expected_subprocess_calls"]) + dcmp = filecmp.dircmp(sop_path, op_path) + diff_output = "" + for name in dcmp.diff_files: + diff_output += \ + "Diff: file: {} expected: {} output: {}\n".format(\ + name, dcmp.left, dcmp.right) + diff_output += self.run_diff( dcmp.left + "/" + name,\ + dcmp.right + "/" + name) + print(diff_output) + self.assertTrue(len(diff_output) == 0, diff_output) \ No newline at end of file diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py index 2e0b64227c0f..6d7ad50cbd72 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py @@ -11,6 +11,7 @@ from pyfakefs.fake_filesystem_unittest import patchfs from deepdiff import DeepDiff +from unittest.mock import call test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) modules_path = os.path.dirname(test_path) @@ -104,7 +105,7 @@ def test_hostcfgd_feature_handler(self, test_name, test_data, fs): feature_handler.sync_state_field() features = MockConfigDb.CONFIG_DB['FEATURE'] for key, fvs in features.items(): - feature_handler.handle(key, fvs) + feature_handler.handle(key, "SET", fvs) # Verify if the updates are properly updated assert self.__verify_table( @@ -142,3 +143,53 @@ def test_feature_config_parsing_defaults(self): assert not swss_feature.has_timer assert swss_feature.has_global_scope assert not swss_feature.has_per_asic_scope + + +class TesNtpCfgd(TestCase): + """ + Test hostcfd daemon - NtpCfgd + """ + def setUp(self): + MockConfigDb.CONFIG_DB['NTP'] = {'global': {'vrf': 'mgmt', 'src_intf': 'eth0'}} + MockConfigDb.CONFIG_DB['NTP_SERVER'] = {'0.debian.pool.ntp.org': {}} + + def tearDown(self): + MockConfigDb.CONFIG_DB = {} + + def test_ntp_global_update_with_no_servers(self): + with mock.patch("hostcfgd.subprocess") as mocked_subprocess: + popen_mock = mock.Mock() + attrs = {'communicate.return_value': ('output', 'error')} + popen_mock.configure_mock(**attrs) + mocked_subprocess.Popen.return_value = popen_mock + + ntpcfgd = hostcfgd.NtpCfg() + ntpcfgd.ntp_global_update("global", MockConfigDb.CONFIG_DB['NTP']["global"]) + + mocked_subprocess.check_call.assert_not_called() + + def test_ntp_global_update_ntp_servers(self): + with mock.patch("hostcfgd.subprocess") as mocked_subprocess: + popen_mock = mock.Mock() + attrs = {'communicate.return_value': ('output', 'error')} + popen_mock.configure_mock(**attrs) + mocked_subprocess.Popen.return_value = popen_mock + + ntpcfgd = hostcfgd.NtpCfg() + ntpcfgd.ntp_global_update("global", MockConfigDb.CONFIG_DB['NTP']["global"]) + ntpcfgd.ntp_server_update("0.debian.pool.ntp.org", "SET") + mocked_subprocess.check_call.assert_has_calls([call("systemctl restart ntp-config", shell=True)]) + + def test_loopback_update(self): + with mock.patch("hostcfgd.subprocess") as mocked_subprocess: + popen_mock = mock.Mock() + attrs = {'communicate.return_value': ('output', 'error')} + popen_mock.configure_mock(**attrs) + mocked_subprocess.Popen.return_value = popen_mock + + ntpcfgd = hostcfgd.NtpCfg() + ntpcfgd.ntp_global = MockConfigDb.CONFIG_DB['NTP']["global"] + ntpcfgd.ntp_servers.add('0.debian.pool.ntp.org') + + ntpcfgd.handle_ntp_source_intf_chg("eth0") + mocked_subprocess.check_call.assert_has_calls([call("systemctl restart ntp-config", shell=True)]) From 7535f74e2abb7bf8dbcdd7e5d1fe718c60cdf098 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 10 Sep 2021 09:16:33 +0000 Subject: [PATCH 07/16] UT in progress Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 22 +++---- .../tests/hostcfgd/hostcfgd_radius_test.py | 59 ++++++++----------- 2 files changed, 37 insertions(+), 44 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index c68f14c196b9..af25e1845def 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -928,19 +928,10 @@ class HostConfigDaemon: self.is_multi_npu_device = is_multi_npu() - try: - dev_meta = self.config_db.get_table('DEVICE_METADATA') - if 'localhost' in dev_meta: - if 'hostname' in dev_meta['localhost']: - self.hostname_cache = dev_meta['localhost']['hostname'] - except Exception as e: - pass - # Initialize AAACfg self.hostname_cache="" self.aaacfg = AaaCfg(self.config_db) - # Update AAA with the hostname - self.aaacfg.hostname_update(self.hostname_cache) + def load(self): aaa = self.config_db.get_table('AAA') @@ -953,6 +944,17 @@ class HostConfigDaemon: lpbk_table = self.config_db.get_table('LOOPBACK_INTERFACE') self.iptables.load(lpbk_table) + try: + dev_meta = self.config_db.get_table('DEVICE_METADATA') + if 'localhost' in dev_meta: + if 'hostname' in dev_meta['localhost']: + self.hostname_cache = dev_meta['localhost']['hostname'] + except Exception as e: + pass + + # Update AAA with the hostname + self.aaacfg.hostname_update(self.hostname_cache) + def __get_intf_name(self, key): if isinstance(key, tuple) and key: intf = key[0] diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py index e476190a7382..c4e5ac10037f 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py @@ -75,37 +75,28 @@ def test_hostcfgd_radius(self, test_name, test_data): shutil.copyfile(sop_path + "/sshd.old", op_path + "/sshd") shutil.copyfile(sop_path + "/login.old", op_path + "/login") - with mock.patch("hostcfgd.subprocess") as mocked_subprocess: - popen_mock = mock.Mock() - attrs = {'communicate.return_value': ('output', 'error')} - popen_mock.configure_mock(**attrs) - mocked_subprocess.Popen.return_value = popen_mock - MockConfigDb.set_config_db(test_data["config_db"]) - - aaacfg = hostcfgd.AaaCfg() - aaacfg.hostname_update(MockConfigDb.CONFIG_DB["DEVICE_METADATA"]["localhost"]["hostname"]) - aaa = MockConfigDb().get_table('AAA') - - try: - radius_global = MockConfigDb().get_table('RADIUS') - except: - radius_global = [] - try: - radius_server = \ - MockConfigDb().get_table('RADIUS_SERVER') - except: - radius_server = [] - - aaacfg.load(aaa,[],[],radius_global,radius_server) - # print(aaacfg.auth, aaacfg.radius_global, aaacfg.radius_servers) - mocked_subprocess.check_call.assert_has_calls(test_data["expected_subprocess_calls"]) - dcmp = filecmp.dircmp(sop_path, op_path) - diff_output = "" - for name in dcmp.diff_files: - diff_output += \ - "Diff: file: {} expected: {} output: {}\n".format(\ - name, dcmp.left, dcmp.right) - diff_output += self.run_diff( dcmp.left + "/" + name,\ - dcmp.right + "/" + name) - print(diff_output) - self.assertTrue(len(diff_output) == 0, diff_output) \ No newline at end of file + MockConfigDb.set_config_db(test_data["config_db"]) + + aaacfg = hostcfgd.AaaCfg(MockConfigDb()) + aaa = MockConfigDb().get_table('AAA') + + try: + radius_global = MockConfigDb().get_table('RADIUS') + except: + radius_global = [] + try: + radius_server = \ + MockConfigDb().get_table('RADIUS_SERVER') + except: + radius_server = [] + + aaacfg.load(aaa,[],[],radius_global,radius_server) + dcmp = filecmp.dircmp(sop_path, op_path) + diff_output = "" + for name in dcmp.diff_files: + diff_output += \ + "Diff: file: {} expected: {} output: {}\n".format(\ + name, dcmp.left, dcmp.right) + diff_output += self.run_diff( dcmp.left + "/" + name,\ + dcmp.right + "/" + name) + self.assertTrue(len(diff_output) == 0, diff_output) \ No newline at end of file From 3da47d330a5a8f473ae60036844935d91375ea5d Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 10 Sep 2021 12:18:38 +0000 Subject: [PATCH 08/16] Added tests for HostcfgDaemon and mocks Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 30 ++-- .../tests/common/mock_configdb.py | 81 +++++++++ .../tests/hostcfgd/hostcfgd_test.py | 157 ++++++++++++++---- .../tests/hostcfgd/test_vectors.py | 59 +++++++ 4 files changed, 286 insertions(+), 41 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index af25e1845def..c565171b6a53 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -782,7 +782,7 @@ class KdumpCfg(object): "memory": "0M-2G:256M,2G-4G:320M,4G-8G:384M,8G-:448M", "num_dumps": "3" } - def init(self, kdump_table): + def load(self, kdump_table): """ Set the KDUMP table in CFG DB to kdump_defaults if npt set by the user """ @@ -795,7 +795,7 @@ class KdumpCfg(object): else: self.config_db.mod_entry("KDUMP", "config", {row : value}) - def kdump_update(self, key, data, isLoad): + def kdump_update(self, key, data): syslog.syslog(syslog.LOG_INFO, "Kdump global configuration update") if key == "config": # Admin mode @@ -815,14 +815,14 @@ class KdumpCfg(object): memory = self.kdump_defaults["memory"] if data.get("memory") is not None: memory = data.get("memory") - if isLoad or data.get("memory") is not None: + if data.get("memory") is not None: run_cmd("sonic-kdump-config --memory " + memory) # Num dumps num_dumps = self.kdump_defaults["num_dumps"] if data.get("num_dumps") is not None: num_dumps = data.get("num_dumps") - if isLoad or data.get("num_dumps") is not None: + if data.get("num_dumps") is not None: run_cmd("sonic-kdump-config --num_dumps " + num_dumps) class NtpCfg(object): @@ -843,7 +843,7 @@ class NtpCfg(object): return # check only the intf configured as source interface - if intf_name != self.ntp_global.get('src_intf', ''): + if intf_name not in self.ntp_global.get('src_intf', '').split(';'): return else: # just restart ntp config @@ -886,7 +886,7 @@ class NtpCfg(object): if op == "SET" and key not in self.ntp_servers: restart_config = True self.ntp_servers.add(key) - elif op == "DEL" and key in self.ntp_server: + elif op == "DEL" and key in self.ntp_servers: restart_config = True self.ntp_servers.remove(key) @@ -902,6 +902,7 @@ class HostConfigDaemon: self.config_db = ConfigDBConnector() self.config_db.connect(wait_for_init=True, retry_on=True) self.dbconn = DBConnector(CFG_DB, 0) + self.selector = Select() syslog.syslog(syslog.LOG_INFO, 'ConfigDB connect success') self.select = Select() @@ -914,7 +915,7 @@ class HostConfigDaemon: # Initialize KDump Config and set the config to default if nothing is provided self.kdumpCfg = KdumpCfg(self.config_db) - self.kdumpCfg.init(self.config_db.get_table('KDUMP')) + self.kdumpCfg.load(self.config_db.get_table('KDUMP')) # Initialize IpTables self.iptables = Iptables() @@ -1049,7 +1050,7 @@ class HostConfigDaemon: def kdump_handler (self, key, op, data): syslog.syslog(syslog.LOG_INFO, 'Kdump handler...') - self.kdumpCfg.kdump_update(key, data, False) + self.kdumpCfg.kdump_update(key, data) def wait_till_system_init_done(self): # No need to print the output in the log file so using the "--quiet" @@ -1069,7 +1070,7 @@ class HostConfigDaemon: except Exception as err: syslog.syslog(syslog.LOG_ERR, "Subscribe to table {} failed with error {}".format(table, err)) - def start(self): + def register_callbacks(self): self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), PRIO_INIT) # Handle FEATURE updates before other tables self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), PRIO_INIT-1) @@ -1097,6 +1098,7 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_INFO, "systemctl has finished initialization -- proceeding ...") + def start(self): init_load_flag = True while True: state, selectable_ = self.selector.select(SELECT_TIMEOUT) @@ -1107,16 +1109,17 @@ class HostConfigDaemon: fd = selectable_.getFd() # Get the Corresponding subscriber & table - subscriber, table = subscriber_map.get(fd, (None, "")) + subscriber, table = self.subscriber_map.get(fd, (None, "")) if not subscriber: syslog.syslog(syslog.LOG_ERR, "No Subscriber object found for fd: {}, subscriber map: {}".format(fd. subscriber_map)) continue key, op, fvs = subscriber.pop() # Get the registered callback - callback = self.callbacks.get(table, None) - if callback: - callback(key, op, dict(fvs)) + cbs = self.callbacks.get(table, None) + for callback in cbs: + print(table, key, op, dict(fvs)) + callback(table, key, op, dict(fvs)) if init_load_flag and table == "FEATURE" and key.lower() == "swss": init_load_flag = False @@ -1125,6 +1128,7 @@ class HostConfigDaemon: def main(): daemon = HostConfigDaemon() + daemon.register_callbacks() daemon.start() diff --git a/src/sonic-host-services/tests/common/mock_configdb.py b/src/sonic-host-services/tests/common/mock_configdb.py index 7783e6e8eaa5..451a36bc73f8 100644 --- a/src/sonic-host-services/tests/common/mock_configdb.py +++ b/src/sonic-host-services/tests/common/mock_configdb.py @@ -12,6 +12,14 @@ def __init__(self, **kwargs): def set_config_db(test_config_db): MockConfigDb.CONFIG_DB = test_config_db + @staticmethod + def deserialize_key(key, separator="|"): + tokens = key.split(separator) + if len(tokens) > 1: + return tuple(tokens) + else: + return key + @staticmethod def get_config_db(): return MockConfigDb.CONFIG_DB @@ -35,3 +43,76 @@ def set_entry(self, key, field, data): def get_table(self, table_name): return MockConfigDb.CONFIG_DB[table_name] + + +class MockSelect(): + + event_queue = [] + + @staticmethod + def set_event_queue(Q): + MockSelect.event_queue = Q + + @staticmethod + def get_event_queue(): + return MockSelect.event_queue + + @staticmethod + def reset_event_queue(): + MockSelect.event_queue = [] + + def __init__(self): + self.sub_map = {} + self.TIMEOUT = "TIMEOUT" + self.ERROR = "ERROR" + + def addSelectable(self, subscriber): + self.sub_map[subscriber.table] = subscriber + + def select(self, TIMEOUT): + if not MockSelect.get_event_queue(): + raise TimeoutError + table, key = MockSelect.get_event_queue().pop(0) + self.sub_map[table].nextKey(key) + return "OBJECT", self.sub_map[table] + + +class MockSubscriberStateTable(): + + FD_INIT = 0 + + @staticmethod + def generate_fd(): + curr = MockSubscriberStateTable.FD_INIT + MockSubscriberStateTable.FD_INIT = curr + 1 + return curr + + @staticmethod + def reset_fd(): + MockSubscriberStateTable.FD_INIT = 0 + + def __init__(self, conn, table, pop, pri): + self.fd = MockSubscriberStateTable.generate_fd() + self.next_key = '' + self.table = table + + def getFd(self): + return self.fd + + def nextKey(self, key): + self.next_key = key + + def pop(self): + table = MockConfigDb.CONFIG_DB.get(self.table, {}) + if self.next_key not in table: + op = "DEL" + fvs = {} + else: + op = "SET" + fvs = table.get(self.next_key, {}) + return self.next_key, op, fvs + + +class MockDBConnector(): + def __init__(self, db, val): + pass \ No newline at end of file diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py index 6d7ad50cbd72..4a453a2f77b1 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py @@ -6,8 +6,9 @@ from sonic_py_common.general import load_module_from_source from unittest import TestCase, mock -from .test_vectors import HOSTCFGD_TEST_VECTOR -from tests.common.mock_configdb import MockConfigDb +from .test_vectors import HOSTCFGD_TEST_VECTOR, HOSTCFG_DAEMON_CFG_DB +from tests.common.mock_configdb import MockConfigDb, MockSubscriberStateTable +from tests.common.mock_configdb import MockSelect, MockDBConnector from pyfakefs.fake_filesystem_unittest import patchfs from deepdiff import DeepDiff @@ -15,13 +16,16 @@ test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) modules_path = os.path.dirname(test_path) -scripts_path = os.path.join(modules_path, "scripts") +scripts_path = os.path.join(modules_path, 'scripts') sys.path.insert(0, modules_path) # Load the file under test hostcfgd_path = os.path.join(scripts_path, 'hostcfgd') hostcfgd = load_module_from_source('hostcfgd', hostcfgd_path) hostcfgd.ConfigDBConnector = MockConfigDb +hostcfgd.SubscriberStateTable = MockSubscriberStateTable +hostcfgd.Select = MockSelect +hostcfgd.DBConnector = MockDBConnector class TestHostcfgd(TestCase): @@ -42,7 +46,7 @@ def __verify_table(self, table, expected_table): None """ ddiff = DeepDiff(table, expected_table, ignore_order=True) - print("DIFF:", ddiff) + print('DIFF:', ddiff) return True if not ddiff else False def __verify_fs(self, table): @@ -60,16 +64,16 @@ def __verify_fs(self, table): """ exp_dict = { - "enabled": "always", - "disabled": "no", + 'enabled': 'always', + 'disabled': 'no', } - auto_restart_conf = os.path.join(hostcfgd.FeatureHandler.SYSTEMD_SERVICE_CONF_DIR, "auto_restart.conf") + auto_restart_conf = os.path.join(hostcfgd.FeatureHandler.SYSTEMD_SERVICE_CONF_DIR, 'auto_restart.conf') for feature in table: - auto_restart = table[feature].get("auto_restart", "disabled") + auto_restart = table[feature].get('auto_restart', 'disabled') with open(auto_restart_conf.format(feature)) as conf: conf = conf.read().strip() - assert conf == "[Service]\nRestart={}".format(exp_dict[auto_restart]) + assert conf == '[Service]\nRestart={}'.format(exp_dict[auto_restart]) @parameterized.expand(HOSTCFGD_TEST_VECTOR) @@ -87,10 +91,10 @@ def test_hostcfgd_feature_handler(self, test_name, test_data, fs): """ fs.add_real_paths(swsscommon.__path__) # add real path of swsscommon for database_config.json fs.create_dir(hostcfgd.FeatureHandler.SYSTEMD_SYSTEM_DIR) - MockConfigDb.set_config_db(test_data["config_db"]) - with mock.patch("hostcfgd.subprocess") as mocked_subprocess: + MockConfigDb.set_config_db(test_data['config_db']) + with mock.patch('hostcfgd.subprocess') as mocked_subprocess: popen_mock = mock.Mock() - attrs = test_data["popen_attributes"] + attrs = test_data['popen_attributes'] popen_mock.configure_mock(**attrs) mocked_subprocess.Popen.return_value = popen_mock hostcfgd.is_multi_npu = mock.Mock(return_value=False) @@ -105,16 +109,16 @@ def test_hostcfgd_feature_handler(self, test_name, test_data, fs): feature_handler.sync_state_field() features = MockConfigDb.CONFIG_DB['FEATURE'] for key, fvs in features.items(): - feature_handler.handle(key, "SET", fvs) + feature_handler.handle(key, 'SET', fvs) # Verify if the updates are properly updated assert self.__verify_table( - MockConfigDb.get_config_db()["FEATURE"], - test_data["expected_config_db"]["FEATURE"] - ), "Test failed for test data: {0}".format(test_data) - mocked_subprocess.check_call.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True) + MockConfigDb.get_config_db()['FEATURE'], + test_data['expected_config_db']['FEATURE'] + ), 'Test failed for test data: {0}'.format(test_data) + mocked_subprocess.check_call.assert_has_calls(test_data['expected_subprocess_calls'], any_order=True) - self.__verify_fs(test_data["config_db"]["FEATURE"]) + self.__verify_fs(test_data['config_db']['FEATURE']) def test_feature_config_parsing(self): swss_feature = hostcfgd.Feature('swss', { @@ -157,39 +161,136 @@ def tearDown(self): MockConfigDb.CONFIG_DB = {} def test_ntp_global_update_with_no_servers(self): - with mock.patch("hostcfgd.subprocess") as mocked_subprocess: + with mock.patch('hostcfgd.subprocess') as mocked_subprocess: popen_mock = mock.Mock() attrs = {'communicate.return_value': ('output', 'error')} popen_mock.configure_mock(**attrs) mocked_subprocess.Popen.return_value = popen_mock ntpcfgd = hostcfgd.NtpCfg() - ntpcfgd.ntp_global_update("global", MockConfigDb.CONFIG_DB['NTP']["global"]) + ntpcfgd.ntp_global_update('global', MockConfigDb.CONFIG_DB['NTP']['global']) mocked_subprocess.check_call.assert_not_called() def test_ntp_global_update_ntp_servers(self): - with mock.patch("hostcfgd.subprocess") as mocked_subprocess: + with mock.patch('hostcfgd.subprocess') as mocked_subprocess: popen_mock = mock.Mock() attrs = {'communicate.return_value': ('output', 'error')} popen_mock.configure_mock(**attrs) mocked_subprocess.Popen.return_value = popen_mock ntpcfgd = hostcfgd.NtpCfg() - ntpcfgd.ntp_global_update("global", MockConfigDb.CONFIG_DB['NTP']["global"]) - ntpcfgd.ntp_server_update("0.debian.pool.ntp.org", "SET") - mocked_subprocess.check_call.assert_has_calls([call("systemctl restart ntp-config", shell=True)]) + ntpcfgd.ntp_global_update('global', MockConfigDb.CONFIG_DB['NTP']['global']) + ntpcfgd.ntp_server_update('0.debian.pool.ntp.org', 'SET') + mocked_subprocess.check_call.assert_has_calls([call('systemctl restart ntp-config', shell=True)]) def test_loopback_update(self): - with mock.patch("hostcfgd.subprocess") as mocked_subprocess: + with mock.patch('hostcfgd.subprocess') as mocked_subprocess: popen_mock = mock.Mock() attrs = {'communicate.return_value': ('output', 'error')} popen_mock.configure_mock(**attrs) mocked_subprocess.Popen.return_value = popen_mock ntpcfgd = hostcfgd.NtpCfg() - ntpcfgd.ntp_global = MockConfigDb.CONFIG_DB['NTP']["global"] + ntpcfgd.ntp_global = MockConfigDb.CONFIG_DB['NTP']['global'] ntpcfgd.ntp_servers.add('0.debian.pool.ntp.org') - ntpcfgd.handle_ntp_source_intf_chg("eth0") - mocked_subprocess.check_call.assert_has_calls([call("systemctl restart ntp-config", shell=True)]) + ntpcfgd.handle_ntp_source_intf_chg('eth0') + mocked_subprocess.check_call.assert_has_calls([call('systemctl restart ntp-config', shell=True)]) + + +class TestHostcfgdDaemon(TestCase): + + def setUp(self): + MockConfigDb.set_config_db(HOSTCFG_DAEMON_CFG_DB) + + def tearDown(self): + MockConfigDb.CONFIG_DB = {} + + @patchfs + def test_feature_events(self, fs): + fs.create_dir(hostcfgd.FeatureHandler.SYSTEMD_SYSTEM_DIR) + MockSelect.event_queue = [('FEATURE', 'dhcp_relay'), + ('FEATURE', 'mux'), + ('FEATURE', 'telemetry')] + daemon = hostcfgd.HostConfigDaemon() + daemon.register_callbacks() + with mock.patch('hostcfgd.subprocess') as mocked_subprocess: + popen_mock = mock.Mock() + attrs = {'communicate.return_value': ('output', 'error')} + popen_mock.configure_mock(**attrs) + mocked_subprocess.Popen.return_value = popen_mock + try: + daemon.start() + except TimeoutError: + pass + expected = [call('sudo systemctl daemon-reload', shell=True), + call('sudo systemctl unmask dhcp_relay.service', shell=True), + call('sudo systemctl enable dhcp_relay.service', shell=True), + call('sudo systemctl start dhcp_relay.service', shell=True), + call('sudo systemctl daemon-reload', shell=True), + call('sudo systemctl unmask mux.service', shell=True), + call('sudo systemctl enable mux.service', shell=True), + call('sudo systemctl start mux.service', shell=True), + call('sudo systemctl daemon-reload', shell=True), + call('sudo systemctl unmask telemetry.service', shell=True), + call('sudo systemctl unmask telemetry.timer', shell=True), + call('sudo systemctl enable telemetry.timer', shell=True), + call('sudo systemctl start telemetry.timer', shell=True)] + mocked_subprocess.check_call.assert_has_calls(expected) + + # Change the state to disabled + MockConfigDb.CONFIG_DB['FEATURE']['telemetry']['state'] = 'disabled' + MockSelect.event_queue = [('FEATURE', 'telemetry')] + try: + daemon.start() + except TimeoutError: + pass + expected = [call('sudo systemctl stop telemetry.timer', shell=True), + call('sudo systemctl disable telemetry.timer', shell=True), + call('sudo systemctl mask telemetry.timer', shell=True), + call('sudo systemctl stop telemetry.service', shell=True), + call('sudo systemctl disable telemetry.timer', shell=True), + call('sudo systemctl mask telemetry.timer', shell=True)] + mocked_subprocess.check_call.assert_has_calls(expected) + + def test_loopback_events(self): + MockConfigDb.set_config_db(HOSTCFG_DAEMON_CFG_DB) + MockSelect.event_queue = [('NTP', 'global'), + ('NTP_SERVER', '0.debian.pool.ntp.org'), + ('LOOPBACK_INTERFACE', 'Loopback0|10.184.8.233/32')] + daemon = hostcfgd.HostConfigDaemon() + daemon.register_callbacks() + with mock.patch('hostcfgd.subprocess') as mocked_subprocess: + popen_mock = mock.Mock() + attrs = {'communicate.return_value': ('output', 'error')} + popen_mock.configure_mock(**attrs) + mocked_subprocess.Popen.return_value = popen_mock + try: + daemon.start() + except TimeoutError: + pass + expected = [call('systemctl restart ntp-config', shell=True), + call('iptables -t mangle --append PREROUTING -p tcp --tcp-flags SYN SYN -d 10.184.8.233 -j TCPMSS --set-mss 1460', shell=True), + call('iptables -t mangle --append POSTROUTING -p tcp --tcp-flags SYN SYN -s 10.184.8.233 -j TCPMSS --set-mss 1460', shell=True)] + mocked_subprocess.check_call.assert_has_calls(expected, any_order=True) + + def test_kdump_event(self): + MockConfigDb.set_config_db(HOSTCFG_DAEMON_CFG_DB) + daemon = hostcfgd.HostConfigDaemon() + daemon.register_callbacks() + assert MockConfigDb.CONFIG_DB['KDUMP']['config'] + MockSelect.event_queue = [('KDUMP', 'config')] + with mock.patch('hostcfgd.subprocess') as mocked_subprocess: + popen_mock = mock.Mock() + attrs = {'communicate.return_value': ('output', 'error')} + popen_mock.configure_mock(**attrs) + mocked_subprocess.Popen.return_value = popen_mock + try: + daemon.start() + except TimeoutError: + pass + expected = [call('sonic-kdump-config --disable', shell=True), + call('sonic-kdump-config --num_dumps 3', shell=True), + call('sonic-kdump-config --memory 0M-2G:256M,2G-4G:320M,4G-8G:384M,8G-:448M', shell=True)] + mocked_subprocess.check_call.assert_has_calls(expected, any_order=True) diff --git a/src/sonic-host-services/tests/hostcfgd/test_vectors.py b/src/sonic-host-services/tests/hostcfgd/test_vectors.py index f70e4a11c457..9a9f828883af 100644 --- a/src/sonic-host-services/tests/hostcfgd/test_vectors.py +++ b/src/sonic-host-services/tests/hostcfgd/test_vectors.py @@ -471,3 +471,62 @@ } ] ] + +HOSTCFG_DAEMON_CFG_DB = { + "FEATURE": { + "dhcp_relay": { + "auto_restart": "enabled", + "has_global_scope": "True", + "has_per_asic_scope": "False", + "has_timer": "False", + "high_mem_alert": "disabled", + "set_owner": "kube", + "state": "{% if not (DEVICE_METADATA is defined and DEVICE_METADATA['localhost'] is defined and DEVICE_METADATA['localhost']['type'] is defined and DEVICE_METADATA['localhost']['type'] != 'ToRRouter') %}enabled{% else %}disabled{% endif %}" + }, + "mux": { + "auto_restart": "enabled", + "has_global_scope": "True", + "has_per_asic_scope": "False", + "has_timer": "False", + "high_mem_alert": "disabled", + "set_owner": "local", + "state": "{% if 'subtype' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['subtype'] == 'DualToR' %}enabled{% else %}always_disabled{% endif %}" + }, + "telemetry": { + "auto_restart": "enabled", + "has_global_scope": "True", + "has_per_asic_scope": "False", + "has_timer": "True", + "high_mem_alert": "disabled", + "set_owner": "kube", + "state": "enabled", + "status": "enabled" + }, + }, + "KDUMP": { + "config": { + + } + }, + "NTP": { + "global": { + "vrf": "default", + "src_intf": "eth0;Loopback0" + } + }, + "NTP_SERVER": { + "0.debian.pool.ntp.org": {} + }, + "LOOPBACK_INTERFACE": { + "Loopback0|10.184.8.233/32": { + "scope": "global", + "family": "IPv4" + } + }, + "DEVICE_METADATA": { + "localhost": { + "subtype": "DualToR", + "type": "ToRRouter", + } + } +} \ No newline at end of file From 31c6ec7f2f363086e0e18a1bcb9aae385919b573 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 10 Sep 2021 12:42:06 +0000 Subject: [PATCH 09/16] Minor fixes Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index c565171b6a53..5c8b3dc29dac 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -784,7 +784,7 @@ class KdumpCfg(object): def load(self, kdump_table): """ - Set the KDUMP table in CFG DB to kdump_defaults if npt set by the user + Set the KDUMP table in CFG DB to kdump_defaults if not set by the user """ syslog.syslog(syslog.LOG_INFO, "KdumpCfg init ...") kdump_conf = kdump_table.get("config", {}) @@ -1118,7 +1118,6 @@ class HostConfigDaemon: # Get the registered callback cbs = self.callbacks.get(table, None) for callback in cbs: - print(table, key, op, dict(fvs)) callback(table, key, op, dict(fvs)) if init_load_flag and table == "FEATURE" and key.lower() == "swss": From 4e74f51ae9f0f0231c94049f70071497eca8f977 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 10 Sep 2021 22:43:24 +0000 Subject: [PATCH 10/16] Changes to Aaacfg cleared Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 145 ++++++++---------- .../tests/hostcfgd/hostcfgd_radius_test.py | 32 ++-- 2 files changed, 85 insertions(+), 92 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 5c8b3dc29dac..dfb2776b8a7d 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -20,7 +20,7 @@ def is_true_cb(val): def cfg_diff(cached, xcvd, callbacks={}): """ - Returns the diff as formatted by jsondiff + Returns the diff and parameters: cached: saved config @@ -32,9 +32,9 @@ def cfg_diff(cached, xcvd, callbacks={}): xcvd_copy: recieved config mutated by callbacks """ if not cached and not xcvd: - return True - if isinstance(cached, dict) != isinstance(xcvd, dict): - return False + return {}, xcvd + if not isinstance(cached, dict) or not isinstance(xcvd, dict): + return {}, xcvd cached_copy = copy.deepcopy(cached) xcvd_copy = copy.deepcopy(xcvd) @@ -442,9 +442,7 @@ class Iptables(object): class AaaCfg(object): - def __init__(self, cfg_db): - self.config_db = cfg_db - + def __init__(self): self.auth_default = { 'login': 'local', } @@ -490,16 +488,14 @@ class AaaCfg(object): self.modify_conf_file() def aaa_update(self, key, data, modify_conf=True): - if key.lower() != 'authentication': - return False - cbs = {'failthrough': is_true_cb} - df, data_modified = cfg_diff(self.auth, data, cbs) - self.auth = data_modified - if 'debug' in data: - self.debug = is_true(data['debug']) - if modify_conf and df: + if key == 'authentication': + self.auth = data + if 'failthrough' in data: + self.auth['failthrough'] = is_true(data['failthrough']) + if 'debug' in data: + self.debug = is_true(data['debug']) + if modify_conf: self.modify_conf_file() - return True if df else False def pick_src_intf_ipaddrs(self, keys, src_intf): new_ipv4_addr = "" @@ -525,44 +521,45 @@ class AaaCfg(object): return(new_ipv4_addr, new_ipv6_addr) def tacacs_global_update(self, key, data, modify_conf=True): - if key.lower() != 'global': - return False - df, data_modified = cfg_diff(self.tacplus_global, data, {}) - self.tacplus_global = data_modified - if modify_conf and df: - self.modify_conf_file() - return True if df else False + if key == 'global': + self.tacplus_global = data + if modify_conf: + self.modify_conf_file() def tacacs_server_update(self, key, data, modify_conf=True): - cached_data = self.tacplus_servers.get(key, {}) - df, data_modified = cfg_diff(cached_data, data, {}) - self.tacplus_servers[key] = data_modified - if modify_conf and df: + if data == {}: + if key in self.tacplus_servers: + del self.tacplus_servers[key] + else: + self.tacplus_servers[key] = data + + if modify_conf: self.modify_conf_file() - return True if df else False - def handle_radius_source_intf_ip_chg(self, intf): - modify_conf = False - if intf == self.radius_global.get('src_intf', ''): - modify_conf = True + def handle_radius_source_intf_ip_chg(self, key): + modify_conf=False + if 'src_intf' in self.radius_global: + if key[0] == self.radius_global['src_intf']: + modify_conf=True for addr in self.radius_servers: - if intf == self.radius_servers.get(addr, {}).get('src_intf', ''): - modify_conf = True + if ('src_intf' in self.radius_servers[addr]) and \ + (key[0] == self.radius_servers[addr]['src_intf']): + modify_conf=True break if not modify_conf: return - syslog.syslog(syslog.LOG_INFO, 'RADIUS IP change - Interface:{}, current server info {}'.format(intf, self.radius_servers)) + syslog.syslog(syslog.LOG_INFO, 'RADIUS IP change - key:{}, current server info {}'.format(key, self.radius_servers)) self.modify_conf_file() def handle_radius_nas_ip_chg(self, key): - modify_conf = False + modify_conf=False # Mgmt IP configuration affects only the default nas_ip if 'nas_ip' not in self.radius_global: for addr in self.radius_servers: if 'nas_ip' not in self.radius_servers[addr]: - modify_conf = True + modify_conf=True break if not modify_conf: @@ -572,19 +569,21 @@ class AaaCfg(object): self.modify_conf_file() def radius_global_update(self, key, data, modify_conf=True): - if key.lower() != 'global': - return None - cbs = {'statistics': is_true_cb} - df, data_modified = cfg_diff(self.radius_global, data, cbs) - self.radius_global = data_modified - if modify_conf and df: - self.modify_conf_file() + if key == 'global': + self.radius_global = data + if 'statistics' in data: + self.radius_global['statistics'] = is_true(data['statistics']) + if modify_conf: + self.modify_conf_file() def radius_server_update(self, key, data, modify_conf=True): - cached_data = self.radius_servers.get(key, {}) - df, data_modified = cfg_diff(cached_data, data, {}) - self.radius_servers[key] = data_modified - if modify_conf and df: + if data == {}: + if key in self.radius_servers: + del self.radius_servers[key] + else: + self.radius_servers[key] = data + + if modify_conf: self.modify_conf_file() def hostname_update(self, hostname, modify_conf=True): @@ -594,7 +593,7 @@ class AaaCfg(object): self.hostname = hostname # Currently only used for RADIUS - if not self.radius_servers: + if len(self.radius_servers) == 0: return if modify_conf: @@ -773,7 +772,13 @@ class AaaCfg(object): else: cmd = 'service aaastatsd stop' syslog.syslog(syslog.LOG_INFO, "cmd - {}".format(cmd)) - run_cmd(cmd) + try: + subprocess.check_call(cmd, shell=True) + except subprocess.CalledProcessError as err: + syslog.syslog(syslog.LOG_ERR, + "{} - failed: return code - {}, output:\n{}" + .format(err.cmd, err.returncode, err.output)) + class KdumpCfg(object): def __init__(self, CfgDb): @@ -931,7 +936,7 @@ class HostConfigDaemon: # Initialize AAACfg self.hostname_cache="" - self.aaacfg = AaaCfg(self.config_db) + self.aaacfg = AaaCfg() def load(self): @@ -942,9 +947,6 @@ class HostConfigDaemon: radius_server = self.config_db.get_table('RADIUS_SERVER') self.aaacfg.load(aaa, tacacs_global, tacacs_server, radius_global, radius_server) - lpbk_table = self.config_db.get_table('LOOPBACK_INTERFACE') - self.iptables.load(lpbk_table) - try: dev_meta = self.config_db.get_table('DEVICE_METADATA') if 'localhost' in dev_meta: @@ -964,41 +966,32 @@ class HostConfigDaemon: return intf def aaa_handler(self, key, op, data): - cfg_change = self.aaacfg.aaa_update(key, data) - if cfg_change: - syslog.syslog(syslog.LOG_INFO, 'AAA Update: key: {}, op: {}, data: {}'.format(key, op, data)) + self.aaacfg.aaa_update(key, data) + syslog.syslog(syslog.LOG_INFO, 'AAA Update: key: {}, op: {}, data: {}'.format(key, op, data)) def tacacs_server_handler(self, key, op, data): - cfg_change = self.aaacfg.tacacs_server_update(key, data) - if not cfg_change: - return + self.aaacfg.tacacs_server_update(key, data) log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) syslog.syslog(syslog.LOG_INFO, 'TACPLUS_SERVER update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def tacacs_global_handler(self, key, op, data): - cfg_change = self.aaacfg.tacacs_global_update(key, data) - if not cfg_change: - return + self.aaacfg.tacacs_global_update(key, data) log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) syslog.syslog(syslog.LOG_INFO, 'TACPLUS Global update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def radius_server_handler(self, key, op, data): - cfg_change = self.aaacfg.radius_server_update(key, data) - if not cfg_change: - return + self.aaacfg.radius_server_update(key, data) log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) syslog.syslog(syslog.LOG_INFO, 'RADIUS_SERVER update: key: {}, op: {}, data: {}'.format(key, op, log_data)) def radius_global_handler(self, key, op, data): - cfg_change = self.aaacfg.radius_global_update(key, data) - if not cfg_change: - return + self.aaacfg.radius_global_update(key, data) log_data = copy.deepcopy(data) if 'passkey' in log_data: log_data['passkey'] = obfuscate(log_data['passkey']) @@ -1020,27 +1013,23 @@ class HostConfigDaemon: self.iptables.iptables_handler(key, data, add) lpbk_name = self.__get_intf_name(key) self.ntpcfg.handle_ntp_source_intf_chg(lpbk_name) - self.aaacfg.handle_radius_source_intf_ip_chg(lpbk_name) + self.aaacfg.handle_radius_source_intf_ip_chg(key) def vlan_intf_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) - vlan_name = self.__get_intf_name(key) - self.aaacfg.handle_radius_source_intf_ip_chg(vlan_name) + self.aaacfg.handle_radius_source_intf_ip_chg(key) def vlan_sub_intf_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) - sub_intf_name = self.__get_intf_name(key) - self.aaacfg.handle_radius_source_intf_ip_chg(sub_intf_name) + self.aaacfg.handle_radius_source_intf_ip_chg(key) def portchannel_intf_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) - lag_name = self.__get_intf_name(key) - self.aaacfg.handle_radius_source_intf_ip_chg(lag_name) + self.aaacfg.handle_radius_source_intf_ip_chg(key) def phy_intf_handler(self, key, op, data): key = ConfigDBConnector.deserialize_key(key) - phy_intf_name = self.__get_intf_name(key) - self.aaacfg.handle_radius_source_intf_ip_chg(phy_intf_name) + self.aaacfg.handle_radius_source_intf_ip_chg(key) def ntp_server_handler(self, key, op, data): self.ntpcfg.ntp_server_update(key, op) diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py index c4e5ac10037f..675216a1ddc5 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py @@ -10,10 +10,10 @@ from parameterized import parameterized from unittest import TestCase, mock from tests.hostcfgd.test_radius_vectors import HOSTCFGD_TEST_RADIUS_VECTOR -from tests.common.mock_configdb import MockConfigDb +from tests.common.mock_configdb import MockConfigDb, MockSubscriberStateTable +from tests.common.mock_configdb import MockSelect, MockDBConnector -swsscommon.ConfigDBConnector = MockConfigDb test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) modules_path = os.path.dirname(test_path) scripts_path = os.path.join(modules_path, "scripts") @@ -31,24 +31,28 @@ loader.exec_module(hostcfgd) sys.modules['hostcfgd'] = hostcfgd +# Mock swsscommon classes +hostcfgd.ConfigDBConnector = MockConfigDb +hostcfgd.SubscriberStateTable = MockSubscriberStateTable +hostcfgd.Select = MockSelect +hostcfgd.DBConnector = MockDBConnector + class TestHostcfgdRADIUS(TestCase): """ Test hostcfd daemon - RADIUS """ def run_diff(self, file1, file2): - return subprocess.check_output('diff -u {} {} || true'.format(file1, file2), shell=True).decode('utf-8').strip() + return subprocess.check_output('diff -uR {} {} || true'.format(file1, file2), shell=True) @parameterized.expand(HOSTCFGD_TEST_RADIUS_VECTOR) def test_hostcfgd_radius(self, test_name, test_data): """ Test RADIUS hostcfd daemon initialization - Args: test_name(str): test name test_data(dict): test data which contains initial Config Db tables, and expected results - Returns: None """ @@ -69,28 +73,28 @@ def test_hostcfgd_radius(self, test_name, test_data): hostcfgd.ETC_PAMD_LOGIN = op_path + "/login" hostcfgd.RADIUS_PAM_AUTH_CONF_DIR = op_path + "/" - shutil.rmtree(op_path, ignore_errors=True) - os.mkdir(op_path) + shutil.rmtree( op_path, ignore_errors=True) + os.mkdir( op_path) - shutil.copyfile(sop_path + "/sshd.old", op_path + "/sshd") - shutil.copyfile(sop_path + "/login.old", op_path + "/login") + shutil.copyfile( sop_path + "/sshd.old", op_path + "/sshd") + shutil.copyfile( sop_path + "/login.old", op_path + "/login") MockConfigDb.set_config_db(test_data["config_db"]) + host_config_daemon = hostcfgd.HostConfigDaemon() - aaacfg = hostcfgd.AaaCfg(MockConfigDb()) - aaa = MockConfigDb().get_table('AAA') + aaa = host_config_daemon.config_db.get_table('AAA') try: - radius_global = MockConfigDb().get_table('RADIUS') + radius_global = host_config_daemon.config_db.get_table('RADIUS') except: radius_global = [] try: radius_server = \ - MockConfigDb().get_table('RADIUS_SERVER') + host_config_daemon.config_db.get_table('RADIUS_SERVER') except: radius_server = [] - aaacfg.load(aaa,[],[],radius_global,radius_server) + host_config_daemon.aaacfg.load(aaa,[],[],radius_global,radius_server) dcmp = filecmp.dircmp(sop_path, op_path) diff_output = "" for name in dcmp.diff_files: From 85d01aa18262e641bc52254f4b565b00b460dfae Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 10 Sep 2021 22:50:00 +0000 Subject: [PATCH 11/16] Removed Unnecessary Files and imports Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/setup.py | 4 +--- .../tests/hostcfgd/hostcfgd_radius_test.py | 2 +- src/sonic-host-services/tests/hostcfgd/test_vectors.py | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/sonic-host-services/setup.py b/src/sonic-host-services/setup.py index 9fd9901117a1..b8cc75ad66a7 100644 --- a/src/sonic-host-services/setup.py +++ b/src/sonic-host-services/setup.py @@ -27,8 +27,7 @@ 'Jinja2>=2.10', 'PyGObject', 'sonic-py-common', - 'systemd-python', - 'jsondiff' + 'systemd-python' ], setup_requires = [ 'pytest-runner', @@ -39,7 +38,6 @@ 'pytest', 'pyfakefs', 'sonic-py-common', - 'jsondiff', 'deepdiff' ], classifiers = [ diff --git a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py index 675216a1ddc5..4e3d18648100 100644 --- a/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py +++ b/src/sonic-host-services/tests/hostcfgd/hostcfgd_radius_test.py @@ -103,4 +103,4 @@ def test_hostcfgd_radius(self, test_name, test_data): name, dcmp.left, dcmp.right) diff_output += self.run_diff( dcmp.left + "/" + name,\ dcmp.right + "/" + name) - self.assertTrue(len(diff_output) == 0, diff_output) \ No newline at end of file + self.assertTrue(len(diff_output) == 0, diff_output) diff --git a/src/sonic-host-services/tests/hostcfgd/test_vectors.py b/src/sonic-host-services/tests/hostcfgd/test_vectors.py index 9a9f828883af..42d8604ce4dc 100644 --- a/src/sonic-host-services/tests/hostcfgd/test_vectors.py +++ b/src/sonic-host-services/tests/hostcfgd/test_vectors.py @@ -529,4 +529,4 @@ "type": "ToRRouter", } } -} \ No newline at end of file +} From e2684b6f650129c4548a519faa58c9370b50f9d4 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Fri, 10 Sep 2021 22:51:44 +0000 Subject: [PATCH 12/16] Removed unnecessary methods in hostcfgd Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 37 ------------------------ 1 file changed, 37 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index dfb2776b8a7d..2ab6fd491569 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -11,43 +11,6 @@ import jinja2 from sonic_py_common.device_info import is_multi_npu, get_num_npus from swsscommon.swsscommon import SubscriberStateTable, DBConnector, Select from swsscommon.swsscommon import ConfigDBConnector, TableConsumable -from jsondiff import diff - - -def is_true_cb(val): - return is_true(val) - - -def cfg_diff(cached, xcvd, callbacks={}): - """ - Returns the diff and - - parameters: - cached: saved config - xcvd: modified config - callbacks: Mutate the values in xcvd for comparison - - returns: - df: diff as returned by jsondiff - xcvd_copy: recieved config mutated by callbacks - """ - if not cached and not xcvd: - return {}, xcvd - if not isinstance(cached, dict) or not isinstance(xcvd, dict): - return {}, xcvd - - cached_copy = copy.deepcopy(cached) - xcvd_copy = copy.deepcopy(xcvd) - for key in xcvd_copy: - func = callbacks.get(key, None) - if func and key in xcvd_copy: - try: - xcvd_copy[key] = func(xcvd_copy[key]) - except: - continue - df = diff(cached_copy, xcvd_copy) - return df, xcvd_copy - # FILE PAM_AUTH_CONF = "/etc/pam.d/common-auth-sonic" From 1d626893843cd4514494f280fa7c1eb1159f002c Mon Sep 17 00:00:00 2001 From: Vivek Reddy Date: Fri, 10 Sep 2021 16:27:06 -0700 Subject: [PATCH 13/16] Update mock_configdb.py --- src/sonic-host-services/tests/common/mock_configdb.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-host-services/tests/common/mock_configdb.py b/src/sonic-host-services/tests/common/mock_configdb.py index 451a36bc73f8..138869dc3bee 100644 --- a/src/sonic-host-services/tests/common/mock_configdb.py +++ b/src/sonic-host-services/tests/common/mock_configdb.py @@ -115,4 +115,4 @@ def pop(self): class MockDBConnector(): def __init__(self, db, val): - pass \ No newline at end of file + pass From f451ed45f38bafdeacf9fa700a9722b10b2a721e Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Thu, 16 Sep 2021 05:40:38 +0000 Subject: [PATCH 14/16] Added signal handler and other comments handled Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 94 +++++++++++++++--------- src/sonic-host-services/setup.py | 3 +- 2 files changed, 60 insertions(+), 37 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index 2ab6fd491569..bc71ec9f525c 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -4,8 +4,10 @@ import ast import copy import ipaddress import os +import sys import subprocess import syslog +import signal import jinja2 from sonic_py_common.device_info import is_multi_npu, get_num_npus @@ -39,8 +41,21 @@ RADIUS_PAM_AUTH_CONF_DIR = "/etc/pam_radius_auth.d/" # MISC Constants CFG_DB = "CONFIG_DB" -PRIO_INIT = 10 -SELECT_TIMEOUT = 1000 +HOSTCFGD_MAX_PRI = 10 # Used to enforce ordering b/w daemons under Hostcfgd +DEFAULT_SELECT_TIMEOUT = 1000 + + +def signal_handler(sig, frame): + if sig == signal.SIGHUP: + syslog.syslog(syslog.LOG_INFO, "HostCfgd: Caught SIGHUP - ignoring..") + elif sig == signal.SIGINT: + syslog.syslog(syslog.LOG_INFO, "HostCfgd: Caught SIGINT - exiting..") + sys.exit(128 + sig) + elif sig == signal.SIGTERM: + syslog.syslog(syslog.LOG_INFO, "HostCfgd: Caught SIGTERM - exiting..") + sys.exit(128 + sig) + else: + syslog.syslog(syslog.LOG_INFO, "HostCfgd: invalid signal - ignoring..") def run_cmd(cmd, log_err=True, raise_exception=False): @@ -158,18 +173,7 @@ class FeatureHandler(object): # the next called self.update_feature_state will start it again. If it will fail # again the auto restart will kick-in. Another order may leave it in failed state # and not auto restart. - - dir_name = self.SYSTEMD_SERVICE_CONF_DIR.format(feature_name) - auto_restart_conf = os.path.join(dir_name, 'auto_restart.conf') - updated_auto_restart = False - if not os.path.exists(auto_restart_conf): # if the auto_restart_conf file is not found, set it - self.update_feature_auto_restart(feature) - updated_auto_restart = True - - if self._cached_config[feature_name].auto_restart != feature.auto_restart: - if not updated_auto_restart: - self.update_feature_auto_restart(feature) - self._cached_config[feature_name].auto_restart = feature.auto_restart + self.update_feature_auto_restart(feature, feature_name) # Enable/disable the container service if the feature state was changed from its previous state. if self._cached_config[feature_name].state != feature.state: @@ -234,16 +238,31 @@ class FeatureHandler(object): return True - def update_feature_auto_restart(self, feature): + def update_feature_auto_restart(self, feature, feature_name): + + dir_name = self.SYSTEMD_SERVICE_CONF_DIR.format(feature_name) + auto_restart_conf = os.path.join(dir_name, 'auto_restart.conf') + + write_conf = False + if not os.path.exists(auto_restart_conf): # if the auto_restart_conf file is not found, set it + write_conf = True + + if self._cached_config[feature_name].auto_restart != feature.auto_restart: + write_conf = True + + if not write_conf: + return + + self._cached_config[feature_name].auto_restart = feature.auto_restart # Update Cache + restart_config = "always" if feature.auto_restart == "enabled" else "no" service_conf = "[Service]\nRestart={}\n".format(restart_config) feature_names, feature_suffixes = self.get_feature_attribute(feature) for feature_name in feature_names: - dir_name = self.SYSTEMD_SERVICE_CONF_DIR.format(feature_name) if not os.path.exists(dir_name): os.mkdir(dir_name) - with open(os.path.join(dir_name, 'auto_restart.conf'), 'w') as cfgfile: + with open(auto_restart_conf, 'w') as cfgfile: cfgfile.write(service_conf) try: @@ -1023,26 +1042,26 @@ class HostConfigDaemon: syslog.syslog(syslog.LOG_ERR, "Subscribe to table {} failed with error {}".format(table, err)) def register_callbacks(self): - self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), PRIO_INIT) + self.subscribe('KDUMP', lambda table, key, op, data: self.kdump_handler(key, op, data), HOSTCFGD_MAX_PRI) # Handle FEATURE updates before other tables - self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), PRIO_INIT-1) + self.subscribe('FEATURE', lambda table, key, op, data: self.feature_handler.handle(key, op, data), HOSTCFGD_MAX_PRI-1) # Handle AAA, TACACS and RADIUS related tables - self.subscribe('AAA', lambda table, key, op, data: self.aaa_handler(key, op, data), PRIO_INIT-2) - self.subscribe('TACPLUS', lambda table, key, op, data: self.tacacs_global_handler(key, op, data), PRIO_INIT-2) - self.subscribe('TACPLUS_SERVER', lambda table, key, op, data: self.tacacs_server_handler(key, op, data), PRIO_INIT-2) - self.subscribe('RADIUS', lambda table, key, op, data: self.radius_global_handler(key, op, data), PRIO_INIT-2) - self.subscribe('RADIUS_SERVER', lambda table, key, op, data: self.radius_server_handler(key, op, data), PRIO_INIT-2) + self.subscribe('AAA', lambda table, key, op, data: self.aaa_handler(key, op, data), HOSTCFGD_MAX_PRI-2) + self.subscribe('TACPLUS', lambda table, key, op, data: self.tacacs_global_handler(key, op, data), HOSTCFGD_MAX_PRI-2) + self.subscribe('TACPLUS_SERVER', lambda table, key, op, data: self.tacacs_server_handler(key, op, data), HOSTCFGD_MAX_PRI-2) + self.subscribe('RADIUS', lambda table, key, op, data: self.radius_global_handler(key, op, data), HOSTCFGD_MAX_PRI-2) + self.subscribe('RADIUS_SERVER', lambda table, key, op, data: self.radius_server_handler(key, op, data), HOSTCFGD_MAX_PRI-2) # Handle IPTables configuration - self.subscribe('LOOPBACK_INTERFACE', lambda table, key, op, data: self.lpbk_handler(key, op, data), PRIO_INIT-3) + self.subscribe('LOOPBACK_INTERFACE', lambda table, key, op, data: self.lpbk_handler(key, op, data), HOSTCFGD_MAX_PRI-3) # Handle NTP & NTP_SERVER updates - self.subscribe('NTP', lambda table, key, op, data: self.ntp_global_handler(key, op, data), PRIO_INIT-4) - self.subscribe('NTP_SERVER', lambda table, key, op, data: self.ntp_server_handler(key, op, data), PRIO_INIT-4) + self.subscribe('NTP', lambda table, key, op, data: self.ntp_global_handler(key, op, data), HOSTCFGD_MAX_PRI-4) + self.subscribe('NTP_SERVER', lambda table, key, op, data: self.ntp_server_handler(key, op, data), HOSTCFGD_MAX_PRI-4) # Handle updates to src intf changes in radius - self.subscribe('MGMT_INTERFACE', lambda table, key, op, data: self.mgmt_intf_handler(key, op, data), PRIO_INIT-5) - self.subscribe('VLAN_INTERFACE', lambda table, key, op, data: self.vlan_intf_handler(key, op, data), PRIO_INIT-5) - self.subscribe('VLAN_SUB_INTERFACE', lambda table, key, op, data: self.vlan_sub_intf_handler(key, op, data), PRIO_INIT-5) - self.subscribe('PORTCHANNEL_INTERFACE', lambda table, key, op, data: self.portchannel_intf_handler(key, op, data), PRIO_INIT-5) - self.subscribe('INTERFACE', lambda table, key, op, data: self.phy_intf_handler(key, op, data), PRIO_INIT-5) + self.subscribe('MGMT_INTERFACE', lambda table, key, op, data: self.mgmt_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) + self.subscribe('VLAN_INTERFACE', lambda table, key, op, data: self.vlan_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) + self.subscribe('VLAN_SUB_INTERFACE', lambda table, key, op, data: self.vlan_sub_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) + self.subscribe('PORTCHANNEL_INTERFACE', lambda table, key, op, data: self.portchannel_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) + self.subscribe('INTERFACE', lambda table, key, op, data: self.phy_intf_handler(key, op, data), HOSTCFGD_MAX_PRI-5) syslog.syslog(syslog.LOG_INFO, "Waiting for systemctl to finish initialization") @@ -1053,11 +1072,13 @@ class HostConfigDaemon: def start(self): init_load_flag = True while True: - state, selectable_ = self.selector.select(SELECT_TIMEOUT) + state, selectable_ = self.selector.select(DEFAULT_SELECT_TIMEOUT) if state == self.selector.TIMEOUT: continue elif state == self.selector.ERROR: - raise Exception("Received error from select") + syslog.syslog(syslog.LOG_ERR, + "error returned by select") + continue fd = selectable_.getFd() # Get the Corresponding subscriber & table @@ -1078,10 +1099,13 @@ class HostConfigDaemon: def main(): + signal.signal(signal.SIGTERM, signal_handler) + signal.signal(signal.SIGINT, signal_handler) + signal.signal(signal.SIGHUP, signal_handler) daemon = HostConfigDaemon() daemon.register_callbacks() daemon.start() - + if __name__ == "__main__": main() diff --git a/src/sonic-host-services/setup.py b/src/sonic-host-services/setup.py index b8cc75ad66a7..8926e960e311 100644 --- a/src/sonic-host-services/setup.py +++ b/src/sonic-host-services/setup.py @@ -26,8 +26,7 @@ 'dbus-python', 'Jinja2>=2.10', 'PyGObject', - 'sonic-py-common', - 'systemd-python' + 'sonic-py-common' ], setup_requires = [ 'pytest-runner', From b3df6f614d4070eb49b30d64073f15b6d5a15d5b Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Thu, 16 Sep 2021 05:41:44 +0000 Subject: [PATCH 15/16] Broad Exception handled Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index bc71ec9f525c..c579d77eea87 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -134,7 +134,7 @@ class Feature(object): """ Safely evaluate the boolean expression, without raising an exception """ try: ret = ast.literal_eval(val) - except Exception as e: + except ValueError: ret = False return ret From c17c8f4a78e791d7c0a779cab94c7946fe3edb44 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Mon, 27 Sep 2021 07:01:35 +0000 Subject: [PATCH 16/16] Removed init dependency of AAA on swss Signed-off-by: Vivek Reddy Karri --- src/sonic-host-services/scripts/hostcfgd | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/sonic-host-services/scripts/hostcfgd b/src/sonic-host-services/scripts/hostcfgd index c579d77eea87..40c81ae857b3 100755 --- a/src/sonic-host-services/scripts/hostcfgd +++ b/src/sonic-host-services/scripts/hostcfgd @@ -1070,7 +1070,6 @@ class HostConfigDaemon: "systemctl has finished initialization -- proceeding ...") def start(self): - init_load_flag = True while True: state, selectable_ = self.selector.select(DEFAULT_SELECT_TIMEOUT) if state == self.selector.TIMEOUT: @@ -1092,10 +1091,6 @@ class HostConfigDaemon: cbs = self.callbacks.get(table, None) for callback in cbs: callback(table, key, op, dict(fvs)) - - if init_load_flag and table == "FEATURE" and key.lower() == "swss": - init_load_flag = False - self.load() def main(): @@ -1104,6 +1099,7 @@ def main(): signal.signal(signal.SIGHUP, signal_handler) daemon = HostConfigDaemon() daemon.register_callbacks() + daemon.load() daemon.start()