From 597fc69c59d560a966fd2c598b90f7e965697bb1 Mon Sep 17 00:00:00 2001 From: Neetha John Date: Mon, 27 Jun 2022 10:27:11 -0700 Subject: [PATCH 1/4] Load backend acl during load minigraph Signed-off-by: Neetha John --- config/main.py | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/config/main.py b/config/main.py index 16aea6b610..c09f58ca94 100644 --- a/config/main.py +++ b/config/main.py @@ -1129,6 +1129,42 @@ def validate_gre_type(ctx, _, value): except ValueError: raise click.UsageError("{} is not a valid GRE type".format(value)) +def _is_storage_device(): + """ + Check if the device is a storage device or not + """ + command = "{} -m -v DEVICE_METADATA.localhost.storage_device".format(SONIC_CFGGEN_PATH) + proc = subprocess.Popen(command, shell=True, text=True, stdout=subprocess.PIPE) + storage_device, err = proc.communicate() + if err: + click.echo("Could not get the storage device info from minigraph, setting storage device to unknown") + storage_device = 'Unknown' + else: + storage_device = storage_device.strip() + return storage_device == "true" + +def load_backend_acl(device_type): + """ + Load acl on backend storage device + """ + + BACKEND_ACL_TEMPLATE_FILE = os.path.join('/', "usr", "share", "sonic", "templates", "backend_acl.j2") + BACKEND_ACL_FILE = os.path.join('/', "etc", "sonic", "backend_acl.json") + + if device_type and device_type == "BackEndToRRouter" and _is_storage_device(): + if os.path.isfile(BACKEND_ACL_TEMPLATE_FILE): + clicommon.run_command( + "{} -d -t {},{}".format( + SONIC_CFGGEN_PATH, + BACKEND_ACL_TEMPLATE_FILE, + BACKEND_ACL_FILE + ), + display_cmd=True + ) + if os.path.isfile(BACKEND_ACL_FILE): + clicommon.run_command("acl-loader update incremental {}".format(BACKEND_ACL_FILE), display_cmd=True) + + # This is our main entrypoint - the main 'config' command @click.group(cls=clicommon.AbbreviationGroup, context_settings=CONTEXT_SETTINGS) @click.pass_context @@ -1684,6 +1720,12 @@ def load_minigraph(db, no_service_restart): if os.path.isfile('/etc/sonic/acl.json'): clicommon.run_command("acl-loader update full /etc/sonic/acl.json", display_cmd=True) + # get the device type + device_type = _get_device_type() + + # Load backend acl + load_backend_acl(device_type) + # Load port_config.json try: load_port_config(db.cfgdb, '/etc/sonic/port_config.json') @@ -1693,8 +1735,6 @@ def load_minigraph(db, no_service_restart): # generate QoS and Buffer configs clicommon.run_command("config qos reload --no-dynamic-buffer", display_cmd=True) - # get the device type - device_type = _get_device_type() if device_type != 'MgmtToRRouter' and device_type != 'MgmtTsToR' and device_type != 'BmcMgmtToRRouter' and device_type != 'EPMS': clicommon.run_command("pfcwd start_default", display_cmd=True) From 8398e8e25313b43fde901db30eefc2a24d6fe531 Mon Sep 17 00:00:00 2001 From: Neetha John Date: Mon, 27 Jun 2022 10:27:39 -0700 Subject: [PATCH 2/4] Add unit tests for backend acl load Signed-off-by: Neetha John --- tests/config_test.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/config_test.py b/tests/config_test.py index 87b66f7e61..ffdced6512 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -321,6 +321,39 @@ def test_load_minigraph_with_port_config(self, get_cmd_module, setup_single_broa port_config = [{"PORT": {"Ethernet0": {"admin_status": "up"}}}] self.check_port_config(db, config, port_config, "config interface startup Ethernet0") + def test_load_backend_acl(self, get_cmd_module, setup_single_broadcom_asic): + self.check_backend_acl(get_cmd_module, storage_device=True, device_type='BackEndToRRouter', condition=True) + + def test_load_backend_acl_not_storage(self, get_cmd_module, setup_single_broadcom_asic): + self.check_backend_acl(get_cmd_module, storage_device=False, device_type='BackEndToRRouter', condition=False) + + def test_load_backend_acl_storage_leaf(self, get_cmd_module, setup_single_broadcom_asic): + self.check_backend_acl(get_cmd_module, storage_device=True, device_type='BackEndLeafRouter', condition=False) + + def check_backend_acl(self, get_cmd_module, storage_device=True, device_type='BackEndToRRouter', condition=True): + with mock.patch('config.main._is_storage_device', mock.MagicMock(return_value=storage_device)): + def is_file_side_effect(filename): + return True if 'backend_acl' in filename else False + with mock.patch('os.path.isfile', mock.MagicMock(side_effect=is_file_side_effect)): + with mock.patch('config.main._get_device_type', mock.MagicMock(return_value=device_type)): + with mock.patch( + "utilities_common.cli.run_command", + mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command: + (config, show) = get_cmd_module + runner = CliRunner() + result = runner.invoke(config.config.commands["load_minigraph"], ["-y"]) + print(result.exit_code) + expected_output = ['Running command: acl-loader update incremental /etc/sonic/backend_acl.json', + 'Running command: /usr/local/bin/sonic-cfggen -d -t /usr/share/sonic/templates/backend_acl.j2,/etc/sonic/backend_acl.json' + ] + print(result.output) + assert result.exit_code == 0 + output = result.output.split('\n') + if condition: + assert set(expected_output).issubset(set(output)) + else: + assert not(set(expected_output).issubset(set(output))) + def check_port_config(self, db, config, port_config, expected_output): def read_json_file_side_effect(filename): return port_config From bfde26f00fac3e726ffe0553d2a692aec6d24127 Mon Sep 17 00:00:00 2001 From: Neetha John Date: Mon, 27 Jun 2022 13:28:15 -0700 Subject: [PATCH 3/4] Fix coverage Signed-off-by: Neetha John --- config/main.py | 19 +++++---------- tests/config_test.py | 56 ++++++++++++++++++++++++-------------------- 2 files changed, 36 insertions(+), 39 deletions(-) diff --git a/config/main.py b/config/main.py index c09f58ca94..49732379e6 100644 --- a/config/main.py +++ b/config/main.py @@ -1129,21 +1129,14 @@ def validate_gre_type(ctx, _, value): except ValueError: raise click.UsageError("{} is not a valid GRE type".format(value)) -def _is_storage_device(): +def _is_storage_device(cfg_db): """ Check if the device is a storage device or not """ - command = "{} -m -v DEVICE_METADATA.localhost.storage_device".format(SONIC_CFGGEN_PATH) - proc = subprocess.Popen(command, shell=True, text=True, stdout=subprocess.PIPE) - storage_device, err = proc.communicate() - if err: - click.echo("Could not get the storage device info from minigraph, setting storage device to unknown") - storage_device = 'Unknown' - else: - storage_device = storage_device.strip() - return storage_device == "true" + device_metadata = cfg_db.get_entry("DEVICE_METADATA", "localhost") + return device_metadata.get("storage_device", "Unknown") == "true" -def load_backend_acl(device_type): +def load_backend_acl(cfg_db, device_type): """ Load acl on backend storage device """ @@ -1151,7 +1144,7 @@ def load_backend_acl(device_type): BACKEND_ACL_TEMPLATE_FILE = os.path.join('/', "usr", "share", "sonic", "templates", "backend_acl.j2") BACKEND_ACL_FILE = os.path.join('/', "etc", "sonic", "backend_acl.json") - if device_type and device_type == "BackEndToRRouter" and _is_storage_device(): + if device_type and device_type == "BackEndToRRouter" and _is_storage_device(cfg_db): if os.path.isfile(BACKEND_ACL_TEMPLATE_FILE): clicommon.run_command( "{} -d -t {},{}".format( @@ -1724,7 +1717,7 @@ def load_minigraph(db, no_service_restart): device_type = _get_device_type() # Load backend acl - load_backend_acl(device_type) + load_backend_acl(db.cfgdb, device_type) # Load port_config.json try: diff --git a/tests/config_test.py b/tests/config_test.py index ffdced6512..67ff8a1a1a 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -322,37 +322,41 @@ def test_load_minigraph_with_port_config(self, get_cmd_module, setup_single_broa self.check_port_config(db, config, port_config, "config interface startup Ethernet0") def test_load_backend_acl(self, get_cmd_module, setup_single_broadcom_asic): - self.check_backend_acl(get_cmd_module, storage_device=True, device_type='BackEndToRRouter', condition=True) + db = Db() + db.cfgdb.set_entry("DEVICE_METADATA", "localhost", {"storage_device": "true"}) + self.check_backend_acl(get_cmd_module, db, device_type='BackEndToRRouter', condition=True) def test_load_backend_acl_not_storage(self, get_cmd_module, setup_single_broadcom_asic): - self.check_backend_acl(get_cmd_module, storage_device=False, device_type='BackEndToRRouter', condition=False) + db = Db() + self.check_backend_acl(get_cmd_module, db, device_type='BackEndToRRouter', condition=False) def test_load_backend_acl_storage_leaf(self, get_cmd_module, setup_single_broadcom_asic): - self.check_backend_acl(get_cmd_module, storage_device=True, device_type='BackEndLeafRouter', condition=False) + db = Db() + db.cfgdb.set_entry("DEVICE_METADATA", "localhost", {"storage_device": "true"}) + self.check_backend_acl(get_cmd_module, db, device_type='BackEndLeafRouter', condition=False) - def check_backend_acl(self, get_cmd_module, storage_device=True, device_type='BackEndToRRouter', condition=True): - with mock.patch('config.main._is_storage_device', mock.MagicMock(return_value=storage_device)): - def is_file_side_effect(filename): - return True if 'backend_acl' in filename else False - with mock.patch('os.path.isfile', mock.MagicMock(side_effect=is_file_side_effect)): - with mock.patch('config.main._get_device_type', mock.MagicMock(return_value=device_type)): - with mock.patch( - "utilities_common.cli.run_command", - mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command: - (config, show) = get_cmd_module - runner = CliRunner() - result = runner.invoke(config.config.commands["load_minigraph"], ["-y"]) - print(result.exit_code) - expected_output = ['Running command: acl-loader update incremental /etc/sonic/backend_acl.json', - 'Running command: /usr/local/bin/sonic-cfggen -d -t /usr/share/sonic/templates/backend_acl.j2,/etc/sonic/backend_acl.json' - ] - print(result.output) - assert result.exit_code == 0 - output = result.output.split('\n') - if condition: - assert set(expected_output).issubset(set(output)) - else: - assert not(set(expected_output).issubset(set(output))) + def check_backend_acl(self, get_cmd_module, db, device_type='BackEndToRRouter', condition=True): + def is_file_side_effect(filename): + return True if 'backend_acl' in filename else False + with mock.patch('os.path.isfile', mock.MagicMock(side_effect=is_file_side_effect)): + with mock.patch('config.main._get_device_type', mock.MagicMock(return_value=device_type)): + with mock.patch( + "utilities_common.cli.run_command", + mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command: + (config, show) = get_cmd_module + runner = CliRunner() + result = runner.invoke(config.config.commands["load_minigraph"], ["-y"], obj=db) + print(result.exit_code) + expected_output = ['Running command: acl-loader update incremental /etc/sonic/backend_acl.json', + 'Running command: /usr/local/bin/sonic-cfggen -d -t /usr/share/sonic/templates/backend_acl.j2,/etc/sonic/backend_acl.json' + ] + print(result.output) + assert result.exit_code == 0 + output = result.output.split('\n') + if condition: + assert set(expected_output).issubset(set(output)) + else: + assert not(set(expected_output).issubset(set(output))) def check_port_config(self, db, config, port_config, expected_output): def read_json_file_side_effect(filename): From f0fe613a84aa92bdcd2aa5eec5fe413c231c180b Mon Sep 17 00:00:00 2001 From: Neetha John Date: Wed, 6 Jul 2022 12:29:36 -0700 Subject: [PATCH 4/4] Add acl table presence to the condition Signed-off-by: Neetha John --- config/main.py | 8 +++++++- tests/config_test.py | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/config/main.py b/config/main.py index 49732379e6..9b3ba84b03 100644 --- a/config/main.py +++ b/config/main.py @@ -1136,6 +1136,12 @@ def _is_storage_device(cfg_db): device_metadata = cfg_db.get_entry("DEVICE_METADATA", "localhost") return device_metadata.get("storage_device", "Unknown") == "true" +def _is_acl_table_present(cfg_db, acl_table_name): + """ + Check if acl table exists + """ + return acl_table_name in cfg_db.get_keys("ACL_TABLE") + def load_backend_acl(cfg_db, device_type): """ Load acl on backend storage device @@ -1144,7 +1150,7 @@ def load_backend_acl(cfg_db, device_type): BACKEND_ACL_TEMPLATE_FILE = os.path.join('/', "usr", "share", "sonic", "templates", "backend_acl.j2") BACKEND_ACL_FILE = os.path.join('/', "etc", "sonic", "backend_acl.json") - if device_type and device_type == "BackEndToRRouter" and _is_storage_device(cfg_db): + if device_type and device_type == "BackEndToRRouter" and _is_storage_device(cfg_db) and _is_acl_table_present(cfg_db, "DATAACL"): if os.path.isfile(BACKEND_ACL_TEMPLATE_FILE): clicommon.run_command( "{} -d -t {},{}".format( diff --git a/tests/config_test.py b/tests/config_test.py index 67ff8a1a1a..7363e5c77b 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -335,6 +335,12 @@ def test_load_backend_acl_storage_leaf(self, get_cmd_module, setup_single_broadc db.cfgdb.set_entry("DEVICE_METADATA", "localhost", {"storage_device": "true"}) self.check_backend_acl(get_cmd_module, db, device_type='BackEndLeafRouter', condition=False) + def test_load_backend_acl_storage_no_dataacl(self, get_cmd_module, setup_single_broadcom_asic): + db = Db() + db.cfgdb.set_entry("DEVICE_METADATA", "localhost", {"storage_device": "true"}) + db.cfgdb.set_entry("ACL_TABLE", "DATAACL", None) + self.check_backend_acl(get_cmd_module, db, device_type='BackEndToRRouter', condition=False) + def check_backend_acl(self, get_cmd_module, db, device_type='BackEndToRRouter', condition=True): def is_file_side_effect(filename): return True if 'backend_acl' in filename else False