From ab72eefd3267a5539c5c4306cdd619e1a5b1df25 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 8 Oct 2022 05:59:12 +0000 Subject: [PATCH 1/8] add mirror session use case --- config/main.py | 68 +++++++++++++++++-------- config/validated_config_db_connector.py | 17 +++++++ 2 files changed, 64 insertions(+), 21 deletions(-) diff --git a/config/main.py b/config/main.py index b35d507b1c..7a61f35e08 100644 --- a/config/main.py +++ b/config/main.py @@ -952,7 +952,7 @@ def validate_mirror_session_config(config_db, session_name, dst_port, src_port, """ Check if SPAN mirror-session config is valid """ ctx = click.get_current_context() if len(config_db.get_entry('MIRROR_SESSION', session_name)) != 0: - click.echo("Error: {} already exists".format(session_name)) + click.echo("Error: {} already exists".format(session_name)) # TODO: MISSING CONSTRAINT IN YANG MODEL return False vlan_member_table = config_db.get_table('VLAN_MEMBER') @@ -2333,25 +2333,35 @@ def add_erspan(session_name, src_ip, dst_ip, dscp, ttl, gre_type, queue, policer session_info['gre_type'] = gre_type session_info = gather_session_info(session_info, policer, queue, src_port, direction) + ctx = click.get_current_context() """ For multi-npu platforms we need to program all front asic namespaces """ namespaces = multi_asic.get_all_namespaces() if not namespaces['front_ns']: - config_db = ConfigDBConnector() + config_db = ValidatedConfigDBConnector(ConfigDBConnector()) config_db.connect() - if validate_mirror_session_config(config_db, session_name, None, src_port, direction) is False: - return - config_db.set_entry("MIRROR_SESSION", session_name, session_info) + if ADHOC_VALIDATION: + if validate_mirror_session_config(config_db, session_name, None, src_port, direction) is False: + return + try: + config_db.set_entry("MIRROR_SESSION", session_name, session_info) + except ValueError as e: + ctx.fail("Invalid mirror session config. Error: {}".format(e)) + else: per_npu_configdb = {} for front_asic_namespaces in namespaces['front_ns']: - per_npu_configdb[front_asic_namespaces] = ConfigDBConnector(use_unix_socket_path=True, namespace=front_asic_namespaces) + per_npu_configdb[front_asic_namespaces] = ValidatedConfigDBConnector(ConfigDBConnector(use_unix_socket_path=True, namespace=front_asic_namespaces)) per_npu_configdb[front_asic_namespaces].connect() - if validate_mirror_session_config(per_npu_configdb[front_asic_namespaces], session_name, None, src_port, direction) is False: - return - per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, session_info) + if ADHOC_VALIDATION: + if validate_mirror_session_config(per_npu_configdb[front_asic_namespaces], session_name, None, src_port, direction) is False: + return + try: + per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, session_info) + except ValueError as e: + ctx.fail("Invalid mirror session config. Error: {}".format(e)) @mirror_session.group(cls=clicommon.AbbreviationGroup, name='span') @click.pass_context @@ -2383,25 +2393,34 @@ def add_span(session_name, dst_port, src_port, direction, queue, policer): } session_info = gather_session_info(session_info, policer, queue, src_port, direction) + ctx = click.get_current_context() """ For multi-npu platforms we need to program all front asic namespaces """ namespaces = multi_asic.get_all_namespaces() if not namespaces['front_ns']: - config_db = ConfigDBConnector() + config_db = ValidatedConfigDBConnector(ConfigDBConnector()) config_db.connect() - if validate_mirror_session_config(config_db, session_name, dst_port, src_port, direction) is False: - return - config_db.set_entry("MIRROR_SESSION", session_name, session_info) + if ADHOC_VALIDATION: + if validate_mirror_session_config(config_db, session_name, dst_port, src_port, direction) is False: + return + try: + config_db.set_entry("MIRROR_SESSION", session_name, session_info) + except ValueError as e: + ctx.fail("Invalid mirror session config. Error: {}".format(e)) else: per_npu_configdb = {} for front_asic_namespaces in namespaces['front_ns']: - per_npu_configdb[front_asic_namespaces] = ConfigDBConnector(use_unix_socket_path=True, namespace=front_asic_namespaces) + per_npu_configdb[front_asic_namespaces] = ValidatedConfigDBConnector(ConfigDBConnector(use_unix_socket_path=True, namespace=front_asic_namespaces)) per_npu_configdb[front_asic_namespaces].connect() - if validate_mirror_session_config(per_npu_configdb[front_asic_namespaces], session_name, dst_port, src_port, direction) is False: - return - per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, session_info) + if ADHOC_VALIDATION: + if validate_mirror_session_config(per_npu_configdb[front_asic_namespaces], session_name, dst_port, src_port, direction) is False: + return + try: + per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, session_info) + except ValueError as e: + ctx.fail("Invalid mirror session config. Error: {}".format(e)) @mirror_session.command() @@ -2413,16 +2432,23 @@ def remove(session_name): For multi-npu platforms we need to program all front asic namespaces """ namespaces = multi_asic.get_all_namespaces() + ctx = click.get_current_context() if not namespaces['front_ns']: - config_db = ConfigDBConnector() + config_db = ValidatedConfigDBConnector(ConfigDBConnector()) config_db.connect() - config_db.set_entry("MIRROR_SESSION", session_name, None) + try: + config_db.set_entry("MIRROR_SESSION", session_name, None) + except JsonPatchConflict: + ctx.fail("Failed to remove MIRROR_SESSION {}".format(session_name)) else: per_npu_configdb = {} for front_asic_namespaces in namespaces['front_ns']: - per_npu_configdb[front_asic_namespaces] = ConfigDBConnector(use_unix_socket_path=True, namespace=front_asic_namespaces) + per_npu_configdb[front_asic_namespaces] = ValidatedConfigDBConnector(ConfigDBConnector(use_unix_socket_path=True, namespace=front_asic_namespaces)) per_npu_configdb[front_asic_namespaces].connect() - per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, None) + try: + per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, None) + except JsonPatchConflict: + ctx.fail("Failed to remove MIRROR_SESSION {}".format(session_name)) # # 'pfcwd' group ('config pfcwd ...') diff --git a/config/validated_config_db_connector.py b/config/validated_config_db_connector.py index b94a2df4a5..c7352d29b3 100644 --- a/config/validated_config_db_connector.py +++ b/config/validated_config_db_connector.py @@ -1,4 +1,5 @@ import jsonpatch +import subprocess from jsonpointer import JsonPointer from sonic_py_common import device_info @@ -12,6 +13,15 @@ def ValidatedConfigDBConnector(config_db_connector): config_db_connector.delete_table = validated_delete_table return config_db_connector +def is_table_present_config_db(table): + cmd = "sonic-cfggen -d --print-data" + result = subprocess.Popen(cmd, shell=True, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + text, err = result.communicate() + return_code = result.returncode + if return_code: # non-zero means failure + raise Exception(f"Failed to get running config, Return code: {return_code}, Error: {err}") + return table in text + def make_path_value_jsonpatch_compatible(table, key, value): if type(key) == tuple: path = JsonPointer.from_parts([table, '|'.join(key)]).path @@ -22,6 +32,13 @@ def make_path_value_jsonpatch_compatible(table, key, value): return path, value def create_gcu_patch(op, table, key=None, value=None): + gcu_json_input = [] + if op == "add" and not is_table_present_config_db(table): + gcu_json = {"op": "{}".format(op), + "path": "/{}".format(table), + "value": {}} + gcu_json_input.append(gcu_json) + if key: path, value = make_path_value_jsonpatch_compatible(table, key, value) else: From 366b5824e71d6ea65fac3a1296d38d0761cbf05c Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Thu, 2 Mar 2023 22:58:36 +0000 Subject: [PATCH 2/8] remove comment --- config/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/main.py b/config/main.py index 0ab3cb6c97..a042bfb725 100644 --- a/config/main.py +++ b/config/main.py @@ -983,7 +983,7 @@ def validate_mirror_session_config(config_db, session_name, dst_port, src_port, """ Check if SPAN mirror-session config is valid """ ctx = click.get_current_context() if len(config_db.get_entry('MIRROR_SESSION', session_name)) != 0: - click.echo("Error: {} already exists".format(session_name)) # TODO: MISSING CONSTRAINT IN YANG MODEL + click.echo("Error: {} already exists".format(session_name)) return False vlan_member_table = config_db.get_table('VLAN_MEMBER') From 151ce68b7bfcd0f5b3f79d36d3103437efc0150e Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 4 Mar 2023 04:28:53 +0000 Subject: [PATCH 3/8] fix UT --- tests/config_mirror_session_test.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/config_mirror_session_test.py b/tests/config_mirror_session_test.py index 5585cab87a..deda5dc6c5 100644 --- a/tests/config_mirror_session_test.py +++ b/tests/config_mirror_session_test.py @@ -1,7 +1,10 @@ import pytest import config.main as config +import jsonpatch from unittest import mock from click.testing import CliRunner +from mock import patch +from jsonpatch import JsonPatchConflict ERR_MSG_IP_FAILURE = "does not appear to be an IPv4 or IPv6 network" ERR_MSG_IP_VERSION_FAILURE = "not a valid IPv4 address" @@ -172,7 +175,20 @@ def test_mirror_session_erspan_add(): mocked.assert_called_with("test_session", "100.1.1.1", "2.2.2.2", 8, 63, 0, 0, None, None, None) +@patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) +@patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=ValueError)) +def test_mirror_session_erspan_add_invalid_yang_validation(): + config.ADHOC_VALIDATION = False + runner = CliRunner() + result = runner.invoke( + config.config.commands["mirror_session"].commands["erspan"].commands["add"], + ["test_session", "1.1.1.1", "2.2.2.2", "6", "63", "65", "65536"]) + print(result.output) + assert "Invalid ConfigDB. Error" in result.output + + def test_mirror_session_span_add(): + config.ADHOC_VALIDATION = True runner = CliRunner() # Verify invalid queue From d226e17549691625134c10b4e5fe8014d3772477 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 4 Mar 2023 04:29:33 +0000 Subject: [PATCH 4/8] fix UT --- tests/config_mirror_session_test.py | 53 ++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/tests/config_mirror_session_test.py b/tests/config_mirror_session_test.py index deda5dc6c5..fcb4f7e50e 100644 --- a/tests/config_mirror_session_test.py +++ b/tests/config_mirror_session_test.py @@ -5,6 +5,7 @@ from click.testing import CliRunner from mock import patch from jsonpatch import JsonPatchConflict +from sonic_py_common import multi_asic ERR_MSG_IP_FAILURE = "does not appear to be an IPv4 or IPv6 network" ERR_MSG_IP_VERSION_FAILURE = "not a valid IPv4 address" @@ -182,7 +183,21 @@ def test_mirror_session_erspan_add_invalid_yang_validation(): runner = CliRunner() result = runner.invoke( config.config.commands["mirror_session"].commands["erspan"].commands["add"], - ["test_session", "1.1.1.1", "2.2.2.2", "6", "63", "65", "65536"]) + ["test_session", "100.1.1.1", "2.2.2.2", "8", "63", "10", "100"]) + print(result.output) + assert "Invalid ConfigDB. Error" in result.output + + + +@patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) +@patch("config.validated_config_db_connector.ValidatedConfigDBConnector", mock.Mock()) +@patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=ValueError)) +def test_mirror_session_erspan_add_multi_asic_invalid_yang_validation(): + config.ADHOC_VALIDATION = False + runner = CliRunner() + result = runner.invoke( + config.config.commands["mirror_session"].commands["erspan"].commands["add"], + ["test_session", "100.1.1.1", "2.2.2.2", "8", "63", "10", "100"]) print(result.output) assert "Invalid ConfigDB. Error" in result.output @@ -289,3 +304,39 @@ def test_mirror_session_span_add(): mocked.assert_called_with("test_session", "Ethernet0", "Ethernet4", "rx", 0, None) + +@patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) +@patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=ValueError)) +def test_mirror_session_span_add_invalid_yang_validation(): + config.ADHOC_VALIDATION = False + runner = CliRunner() + result = runner.invoke( + config.config.commands["mirror_session"].commands["span"].commands["add"], + ["test_session", "Ethernet0", "Ethernet4", "rx", "0"]) + print(result.output) + assert "Invalid ConfigDB. Error" in result.output + + +@patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) +@patch("config.validated_config_db_connector.ValidatedConfigDBConnector", mock.Mock()) +@patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=JsonPatchConflict)) +def test_mirror_session_remove_multi_asic_invalid_yang_validation(): + config.ADHOC_VALIDATION = False + runner = CliRunner() + result = runner.invoke( + config.config.commands["mirror_session"].commands["remove"], + ["mrr_sample"]) + print(result.output) + assert "Invalid ConfigDB. Error" in result.output + + +@patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) +@patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=JsonPatchConflict)) +def test_mirror_session_remove_invalid_yang_validation(): + config.ADHOC_VALIDATION = False + runner = CliRunner() + result = runner.invoke( + config.config.commands["mirror_session"].commands["remove"], + ["mrr_sample"]) + print(result.output) + assert "Invalid ConfigDB. Error" in result.output From 914a547f0e40ec40a26bad27b824901778374352 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 4 Mar 2023 05:48:14 +0000 Subject: [PATCH 5/8] fix UT --- config/main.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/config/main.py b/config/main.py index a042bfb725..0a894ceb13 100644 --- a/config/main.py +++ b/config/main.py @@ -2382,7 +2382,7 @@ def add_erspan(session_name, src_ip, dst_ip, dscp, ttl, gre_type, queue, policer try: config_db.set_entry("MIRROR_SESSION", session_name, session_info) except ValueError as e: - ctx.fail("Invalid mirror session config. Error: {}".format(e)) + ctx.fail("Invalid ConfigDB. Error: {}".format(e)) else: per_npu_configdb = {} @@ -2395,7 +2395,7 @@ def add_erspan(session_name, src_ip, dst_ip, dscp, ttl, gre_type, queue, policer try: per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, session_info) except ValueError as e: - ctx.fail("Invalid mirror session config. Error: {}".format(e)) + ctx.fail("Invalid ConfigDB. Error: {}".format(e)) @mirror_session.group(cls=clicommon.AbbreviationGroup, name='span') @click.pass_context @@ -2442,7 +2442,7 @@ def add_span(session_name, dst_port, src_port, direction, queue, policer): try: config_db.set_entry("MIRROR_SESSION", session_name, session_info) except ValueError as e: - ctx.fail("Invalid mirror session config. Error: {}".format(e)) + ctx.fail("Invalid ConfigDB. Error: {}".format(e)) else: per_npu_configdb = {} for front_asic_namespaces in namespaces['front_ns']: @@ -2454,7 +2454,7 @@ def add_span(session_name, dst_port, src_port, direction, queue, policer): try: per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, session_info) except ValueError as e: - ctx.fail("Invalid mirror session config. Error: {}".format(e)) + ctx.fail("Invalid ConfigDB. Error: {}".format(e)) @mirror_session.command() @@ -2472,8 +2472,8 @@ def remove(session_name): config_db.connect() try: config_db.set_entry("MIRROR_SESSION", session_name, None) - except JsonPatchConflict: - ctx.fail("Failed to remove MIRROR_SESSION {}".format(session_name)) + except JsonPatchConflict as e: + ctx.fail("Invalid ConfigDB. Error: {}".format(e)) else: per_npu_configdb = {} for front_asic_namespaces in namespaces['front_ns']: @@ -2481,8 +2481,8 @@ def remove(session_name): per_npu_configdb[front_asic_namespaces].connect() try: per_npu_configdb[front_asic_namespaces].set_entry("MIRROR_SESSION", session_name, None) - except JsonPatchConflict: - ctx.fail("Failed to remove MIRROR_SESSION {}".format(session_name)) + except JsonPatchConflict as e: + ctx.fail("Invalid ConfigDB. Error: {}".format(e)) # # 'pfcwd' group ('config pfcwd ...') From b89db6eaef8028fff0cacf5e7bc48d646eacb129 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 4 Mar 2023 07:08:13 +0000 Subject: [PATCH 6/8] fix UT --- tests/config_snmp_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/config_snmp_test.py b/tests/config_snmp_test.py index 096f21cb80..76f5675690 100644 --- a/tests/config_snmp_test.py +++ b/tests/config_snmp_test.py @@ -118,6 +118,7 @@ def setup_class(cls): # Add snmp community tests def test_config_snmp_community_add_new_community_ro(self): + config.ADHOC_VALIDATION = True db = Db() runner = CliRunner() with mock.patch('utilities_common.cli.run_command') as mock_run_command: From a17fef7997097c6f9bc39fc08ab93646b2d7a877 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 4 Mar 2023 09:04:40 +0000 Subject: [PATCH 7/8] fix UT --- tests/config_mirror_session_test.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/config_mirror_session_test.py b/tests/config_mirror_session_test.py index fcb4f7e50e..f561a94e64 100644 --- a/tests/config_mirror_session_test.py +++ b/tests/config_mirror_session_test.py @@ -188,11 +188,11 @@ def test_mirror_session_erspan_add_invalid_yang_validation(): assert "Invalid ConfigDB. Error" in result.output - +@patch("config.main.ConfigDBConnector", spec=True, connect=mock.Mock()) +@patch("config.main.multi_asic.get_all_namespaces", mock.Mock(return_value={'front_ns': 'sample_ns'})) @patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) -@patch("config.validated_config_db_connector.ValidatedConfigDBConnector", mock.Mock()) @patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=ValueError)) -def test_mirror_session_erspan_add_multi_asic_invalid_yang_validation(): +def test_mirror_session_erspan_add_multi_asic_invalid_yang_validation(mock_db_connector): config.ADHOC_VALIDATION = False runner = CliRunner() result = runner.invoke( @@ -317,10 +317,11 @@ def test_mirror_session_span_add_invalid_yang_validation(): assert "Invalid ConfigDB. Error" in result.output +@patch("config.main.multi_asic.get_all_namespaces", mock.Mock(return_value={'front_ns': 'sample_ns'})) @patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) -@patch("config.validated_config_db_connector.ValidatedConfigDBConnector", mock.Mock()) +@patch("config.main.ConfigDBConnector", spec=True, connect=mock.Mock()) @patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=JsonPatchConflict)) -def test_mirror_session_remove_multi_asic_invalid_yang_validation(): +def test_mirror_session_remove_multi_asic_invalid_yang_validation(mock_db_connector): config.ADHOC_VALIDATION = False runner = CliRunner() result = runner.invoke( From 6a0c851ea55b110ec3419a782806018544a15686 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 4 Mar 2023 09:10:00 +0000 Subject: [PATCH 8/8] fix UT --- tests/config_mirror_session_test.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/config_mirror_session_test.py b/tests/config_mirror_session_test.py index f561a94e64..ccbc196b50 100644 --- a/tests/config_mirror_session_test.py +++ b/tests/config_mirror_session_test.py @@ -305,6 +305,20 @@ def test_mirror_session_span_add(): mocked.assert_called_with("test_session", "Ethernet0", "Ethernet4", "rx", 0, None) +@patch("config.main.ConfigDBConnector", spec=True, connect=mock.Mock()) +@patch("config.main.multi_asic.get_all_namespaces", mock.Mock(return_value={'front_ns': 'sample_ns'})) +@patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) +@patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=ValueError)) +def test_mirror_session_span_add_multi_asic_invalid_yang_validation(mock_db_connector): + config.ADHOC_VALIDATION = False + runner = CliRunner() + result = runner.invoke( + config.config.commands["mirror_session"].commands["span"].commands["add"], + ["test_session", "Ethernet0", "Ethernet4", "rx", "0"]) + print(result.output) + assert "Invalid ConfigDB. Error" in result.output + + @patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) @patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=ValueError)) def test_mirror_session_span_add_invalid_yang_validation():