From 50e85816523a5583344a79d41bab1a4699be775f Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 23 Apr 2024 06:24:27 +0000 Subject: [PATCH 01/13] Enable per-command authorization in db_migrator --- scripts/db_migrator.py | 34 +++++++++++++++++++ .../config_db/per_command_aaa_disable.json | 10 ++++++ .../per_command_aaa_disable_expected.json | 10 ++++++ .../config_db/per_command_aaa_enable.json | 13 +++++++ .../per_command_aaa_enable_expected.json | 19 +++++++++++ .../config_db/per_command_aaa_no_change.json | 19 +++++++++++ .../per_command_aaa_no_change_expected.json | 19 +++++++++++ .../config_db/per_command_aaa_no_passkey.json | 12 +++++++ .../per_command_aaa_no_passkey_expected.json | 15 ++++++++ tests/db_migrator_test.py | 22 ++++++++++++ 10 files changed, 173 insertions(+) create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_disable.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_enable.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_change.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index c4d4e2da9c..a425f69487 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -1200,6 +1200,38 @@ def set_version(self, version=None): entry = { self.TABLE_FIELD : version } self.configDB.set_entry(self.TABLE_NAME, self.TABLE_KEY, entry) + def enable_per_command_aaa(self): + if not self.config_src_data or 'AAA' not in self.config_src_data: + log.log_info('AAA table does not exist ignore setup per-command accounting&authorization.') + return + + authentication_config_old = self.configDB.get_entry('AAA', 'authentication') + # If device enabled TACACS authentication, then enable TACACS per-command accounting and authorization + if 'login' in authentication_config_old and 'tacacs+' == authentication_config_old.get('login'): + + accounting_config_old = self.configDB.get_entry('AAA', 'accounting') + if 'login' not in accounting_config_old: + accounting_config = { 'login' : 'tacacs+,local' } + self.configDB.set_entry('AAA', 'accounting', accounting_config) + log.log_info('Setup per-command accounting to: {}'.format(accounting_config)) + else: + log.log_info('TACACS per-command accounting already setup.') + + tacplus_config_old = self.configDB.get_entry('TACPLUS', 'global') + if 'passkey' in tacplus_config_old and '' != tacplus_config_old.get('passkey'): + authorization_config_old = self.configDB.get_entry('AAA', 'authorization') + if 'login' not in authorization_config_old: + authorization_config = { 'login' : 'tacacs+' } + self.configDB.set_entry('AAA', 'authorization', authorization_config) + log.log_info('Setup per-command authorization to: {}'.format(authorization_config)) + else: + log.log_info('TACACS per-command authorization already setup.') + else: + log.log_info('TACACS passkey does not exist, ignore setup per-command authorization.') + + else: + log.log_info('TACACS authentication disabled, ignore setup per-command accounting&authorization.') + def common_migration_ops(self): try: with open(INIT_CFG_FILE) as f: @@ -1234,6 +1266,8 @@ def common_migration_ops(self): self.update_edgezone_aggregator_config() # update FRR config mode based on minigraph parser on target image self.migrate_routing_config_mode() + # enable per-command authorization/accounting feature for 202205 & 202305 + self.enable_per_command_aaa() def migrate(self): version = self.get_version() diff --git a/tests/db_migrator_input/config_db/per_command_aaa_disable.json b/tests/db_migrator_input/config_db/per_command_aaa_disable.json new file mode 100644 index 0000000000..e69628e446 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_disable.json @@ -0,0 +1,10 @@ +{ + "AAA": { + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json new file mode 100644 index 0000000000..e69628e446 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json @@ -0,0 +1,10 @@ +{ + "AAA": { + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable.json b/tests/db_migrator_input/config_db/per_command_aaa_enable.json new file mode 100644 index 0000000000..f817701b0f --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable.json @@ -0,0 +1,13 @@ +{ + "AAA": { + "authentication": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json new file mode 100644 index 0000000000..005a2fd398 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json @@ -0,0 +1,19 @@ +{ + "AAA": { + "accounting": { + "login": "tacacs+,local" + }, + "authentication": { + "login": "tacacs+" + }, + "authorization": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_change.json b/tests/db_migrator_input/config_db/per_command_aaa_no_change.json new file mode 100644 index 0000000000..2e83bab807 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_change.json @@ -0,0 +1,19 @@ +{ + "AAA": { + "accounting": { + "login": "local" + }, + "authentication": { + "login": "tacacs+" + }, + "authorization": { + "login": "local" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json new file mode 100644 index 0000000000..2e83bab807 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json @@ -0,0 +1,19 @@ +{ + "AAA": { + "accounting": { + "login": "local" + }, + "authentication": { + "login": "tacacs+" + }, + "authorization": { + "login": "local" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json new file mode 100644 index 0000000000..c5b59d46a4 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json @@ -0,0 +1,12 @@ +{ + "AAA": { + "authentication": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json new file mode 100644 index 0000000000..b06af48439 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json @@ -0,0 +1,15 @@ +{ + "AAA": { + "accounting": { + "login": "tacacs+,local" + }, + "authentication": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index 21ca9148df..c74edf9c6e 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -944,3 +944,25 @@ def test_dns_nameserver_migrator_configdb(self): diff = DeepDiff(resulting_table, expected_table, ignore_order=True) assert not diff + + def test_migrator_configdb_per_command_aaa_enable(self): + import db_migrator + + test_json_list = ('per_command_aaa_enable', + 'per_command_aaa_no_passkey_expected', + 'per_command_aaa_disable') + + for test_json in test_json_list: + dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json) + dbmgtr = db_migrator.DBMigrator(None) + # Set config_src_data + dbmgtr.config_src_data = {} + dbmgtr.migrate() + dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json + '_expected') + expected_db = Db() + advance_version_for_expected_database(dbmgtr.configDB, expected_db.cfgdb, 'version_202405_01') + resulting_table = dbmgtr.configDB.get_table("AAA") + expected_table = expected_db.cfgdb.get_table("AAA") + + diff = DeepDiff(resulting_table, expected_table, ignore_order=True) + assert not diff From 991150c42143891102708579c306f1ad5a26ceda Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 23 Apr 2024 06:44:27 +0000 Subject: [PATCH 02/13] Move test case to new class --- tests/db_migrator_test.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index c74edf9c6e..1a0d882686 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -945,12 +945,23 @@ def test_dns_nameserver_migrator_configdb(self): diff = DeepDiff(resulting_table, expected_table, ignore_order=True) assert not diff +class TestAAAMigrator(object): + @classmethod + def setup_class(cls): + os.environ['UTILITIES_UNIT_TESTING'] = "2" + + @classmethod + def teardown_class(cls): + os.environ['UTILITIES_UNIT_TESTING'] = "0" + dbconnector.dedicated_dbs['CONFIG_DB'] = None + def test_migrator_configdb_per_command_aaa_enable(self): import db_migrator test_json_list = ('per_command_aaa_enable', 'per_command_aaa_no_passkey_expected', - 'per_command_aaa_disable') + 'per_command_aaa_disable'. + 'per_command_aaa_no_change') for test_json in test_json_list: dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json) From ceb66139a60f6ecd0bf36f0e2ad366a9e8fe6033 Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 23 Apr 2024 07:06:59 +0000 Subject: [PATCH 03/13] Fix code error --- tests/db_migrator_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index 1a0d882686..dd895d2928 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -960,7 +960,7 @@ def test_migrator_configdb_per_command_aaa_enable(self): test_json_list = ('per_command_aaa_enable', 'per_command_aaa_no_passkey_expected', - 'per_command_aaa_disable'. + 'per_command_aaa_disable', 'per_command_aaa_no_change') for test_json in test_json_list: From aac4f8ebbfadc60a9e511f275906ab37b9e006ec Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 23 Apr 2024 07:50:00 +0000 Subject: [PATCH 04/13] Improve UT --- tests/db_migrator_test.py | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index dd895d2928..23ce40ff81 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -955,25 +955,22 @@ def teardown_class(cls): os.environ['UTILITIES_UNIT_TESTING'] = "0" dbconnector.dedicated_dbs['CONFIG_DB'] = None - def test_migrator_configdb_per_command_aaa_enable(self): + @pytest.mark.parametrize('test_json', ['per_command_aaa_enable', + 'per_command_aaa_no_passkey_expected', + 'per_command_aaa_disable', + 'per_command_aaa_no_change']) + def test_migrator_configdb_per_command_aaa(self, test_json): import db_migrator + dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json) + dbmgtr = db_migrator.DBMigrator(None) + # Set config_src_data + dbmgtr.config_src_data = {} + dbmgtr.migrate() + dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json + '_expected') + expected_db = Db() + advance_version_for_expected_database(dbmgtr.configDB, expected_db.cfgdb, 'version_202405_01') + resulting_table = dbmgtr.configDB.get_table("AAA") + expected_table = expected_db.cfgdb.get_table("AAA") - test_json_list = ('per_command_aaa_enable', - 'per_command_aaa_no_passkey_expected', - 'per_command_aaa_disable', - 'per_command_aaa_no_change') - - for test_json in test_json_list: - dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json) - dbmgtr = db_migrator.DBMigrator(None) - # Set config_src_data - dbmgtr.config_src_data = {} - dbmgtr.migrate() - dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json + '_expected') - expected_db = Db() - advance_version_for_expected_database(dbmgtr.configDB, expected_db.cfgdb, 'version_202405_01') - resulting_table = dbmgtr.configDB.get_table("AAA") - expected_table = expected_db.cfgdb.get_table("AAA") - - diff = DeepDiff(resulting_table, expected_table, ignore_order=True) - assert not diff + diff = DeepDiff(resulting_table, expected_table, ignore_order=True) + assert not diff From 316a9c6163add271fe82d1b7082e2b8756dd94eb Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 23 Apr 2024 08:26:49 +0000 Subject: [PATCH 05/13] Improve UT --- tests/db_migrator_test.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index 23ce40ff81..70bcd8c34e 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -971,6 +971,10 @@ def test_migrator_configdb_per_command_aaa(self, test_json): advance_version_for_expected_database(dbmgtr.configDB, expected_db.cfgdb, 'version_202405_01') resulting_table = dbmgtr.configDB.get_table("AAA") expected_table = expected_db.cfgdb.get_table("AAA") + + print("test_migrator_configdb_per_command_aaa: {}".format(test_json)) + print("test_migrator_configdb_per_command_aaa, resulting_table: {}".format(resulting_table)) + print("test_migrator_configdb_per_command_aaa, expected_table: {}".format(expected_table)) diff = DeepDiff(resulting_table, expected_table, ignore_order=True) assert not diff From 316c179aa7e12fbcd5a6e578b0b68d808ed120cc Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 23 Apr 2024 08:51:30 +0000 Subject: [PATCH 06/13] Fix code --- scripts/db_migrator.py | 3 ++- tests/db_migrator_test.py | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index a425f69487..c61049bb76 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -1201,7 +1201,8 @@ def set_version(self, version=None): self.configDB.set_entry(self.TABLE_NAME, self.TABLE_KEY, entry) def enable_per_command_aaa(self): - if not self.config_src_data or 'AAA' not in self.config_src_data: + aaa_table = self.configDB.get_table('AAA') + if not aaa_table: log.log_info('AAA table does not exist ignore setup per-command accounting&authorization.') return diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index 70bcd8c34e..45aeaf3e6e 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -966,10 +966,10 @@ def test_migrator_configdb_per_command_aaa(self, test_json): # Set config_src_data dbmgtr.config_src_data = {} dbmgtr.migrate() + resulting_table = dbmgtr.configDB.get_table("AAA") + dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json + '_expected') expected_db = Db() - advance_version_for_expected_database(dbmgtr.configDB, expected_db.cfgdb, 'version_202405_01') - resulting_table = dbmgtr.configDB.get_table("AAA") expected_table = expected_db.cfgdb.get_table("AAA") print("test_migrator_configdb_per_command_aaa: {}".format(test_json)) From c1c277bf023cdd82aae722074011da2251746f8c Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 23 Apr 2024 10:01:33 +0000 Subject: [PATCH 07/13] Enable test message to debug --- pytest.ini | 2 +- tests/db_migrator_test.py | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pytest.ini b/pytest.ini index 536299b8bf..76b21c4412 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,3 +1,3 @@ [pytest] -addopts = --cov-config=.coveragerc --cov --cov-report html --cov-report term --cov-report xml --junitxml=test-results.xml -vv +addopts = --cov-config=.coveragerc --cov --cov-report html --cov-report term --cov-report xml --junitxml=test-results.xml -vv --capture=no pythonpath = . diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index 45aeaf3e6e..0111ba669f 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -960,11 +960,9 @@ def teardown_class(cls): 'per_command_aaa_disable', 'per_command_aaa_no_change']) def test_migrator_configdb_per_command_aaa(self, test_json): - import db_migrator dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json) + import db_migrator dbmgtr = db_migrator.DBMigrator(None) - # Set config_src_data - dbmgtr.config_src_data = {} dbmgtr.migrate() resulting_table = dbmgtr.configDB.get_table("AAA") From 752ae432cd5abe4112a4e061e52c7864d17de2ed Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Tue, 23 Apr 2024 10:43:11 +0000 Subject: [PATCH 08/13] Improve debug information --- tests/mock_tables/dbconnector.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/mock_tables/dbconnector.py b/tests/mock_tables/dbconnector.py index 4ccb392368..2570967192 100644 --- a/tests/mock_tables/dbconnector.py +++ b/tests/mock_tables/dbconnector.py @@ -121,6 +121,8 @@ def __init__(self, *args, **kwargs): else: for attr, value in v.items(): self.hset(k, attr, value) + else: + print("SwssSyncClient: failed to load mock database json file: {}".format(fname)) # Patch mockredis/mockredis/client.py # The offical implementation assume decode_responses=False From 50470965313ce285f58405c25fd0b0eb993de56a Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Wed, 24 Apr 2024 06:34:03 +0000 Subject: [PATCH 09/13] Improve code and test case --- pytest.ini | 2 +- scripts/db_migrator.py | 60 ++++++++++++------- .../config_db/per_command_aaa_disable.json | 11 ++-- .../per_command_aaa_disable_expected.json | 11 ++-- .../config_db/per_command_aaa_enable.json | 17 +++--- .../per_command_aaa_enable_expected.json | 29 +++++---- .../per_command_aaa_enable_mgmt.json | 12 ++++ .../per_command_aaa_enable_mgmt_expected.json | 18 ++++++ .../config_db/per_command_aaa_no_change.json | 29 +++++---- .../per_command_aaa_no_change_expected.json | 29 +++++---- .../config_db/per_command_aaa_no_passkey.json | 15 +++-- .../per_command_aaa_no_passkey_expected.json | 21 ++++--- tests/db_migrator_test.py | 3 +- tests/mock_tables/dbconnector.py | 2 - 14 files changed, 148 insertions(+), 111 deletions(-) create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt_expected.json diff --git a/pytest.ini b/pytest.ini index 76b21c4412..536299b8bf 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,3 +1,3 @@ [pytest] -addopts = --cov-config=.coveragerc --cov --cov-report html --cov-report term --cov-report xml --junitxml=test-results.xml -vv --capture=no +addopts = --cov-config=.coveragerc --cov --cov-report html --cov-report term --cov-report xml --junitxml=test-results.xml -vv pythonpath = . diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index c61049bb76..997890e747 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -1208,30 +1208,46 @@ def enable_per_command_aaa(self): authentication_config_old = self.configDB.get_entry('AAA', 'authentication') # If device enabled TACACS authentication, then enable TACACS per-command accounting and authorization - if 'login' in authentication_config_old and 'tacacs+' == authentication_config_old.get('login'): - - accounting_config_old = self.configDB.get_entry('AAA', 'accounting') - if 'login' not in accounting_config_old: - accounting_config = { 'login' : 'tacacs+,local' } - self.configDB.set_entry('AAA', 'accounting', accounting_config) - log.log_info('Setup per-command accounting to: {}'.format(accounting_config)) - else: - log.log_info('TACACS per-command accounting already setup.') - - tacplus_config_old = self.configDB.get_entry('TACPLUS', 'global') - if 'passkey' in tacplus_config_old and '' != tacplus_config_old.get('passkey'): - authorization_config_old = self.configDB.get_entry('AAA', 'authorization') - if 'login' not in authorization_config_old: - authorization_config = { 'login' : 'tacacs+' } - self.configDB.set_entry('AAA', 'authorization', authorization_config) - log.log_info('Setup per-command authorization to: {}'.format(authorization_config)) - else: - log.log_info('TACACS per-command authorization already setup.') - else: - log.log_info('TACACS passkey does not exist, ignore setup per-command authorization.') + if 'login' not in authentication_config_old or 'tacacs+' != authentication_config_old.get('login'): + log.log_info('TACACS authentication disabled, ignore setup per-command accounting&authorization.') + return + # setup per-command accounting + accounting_config_old = self.configDB.get_entry('AAA', 'accounting') + if 'login' not in accounting_config_old: + accounting_config = { 'login' : 'tacacs+,local' } + self.configDB.set_entry('AAA', 'accounting', accounting_config) + log.log_info('Setup per-command accounting to: {}'.format(accounting_config)) else: - log.log_info('TACACS authentication disabled, ignore setup per-command accounting&authorization.') + log.log_info('TACACS per-command accounting already setup.') + + # setup per-command authorization + tacplus_config_old = self.configDB.get_entry('TACPLUS', 'global') + if 'passkey' not in tacplus_config_old or '' == tacplus_config_old.get('passkey'): + # If no passkey, setup per-command authorization will block remote user command + log.log_info('TACACS passkey does not exist, ignore setup per-command authorization.') + return + + localhost_info_old = self.configDB.get_entry('DEVICE_METADATA', 'localhost') + if 'type' not in localhost_info_old: + # If no type, setup per-command authorization has potensial risk on mgmt device + log.log_info('Device type does not exist, ignore setup per-command authorization.') + return + + authorization_config_old = self.configDB.get_entry('AAA', 'authorization') + if 'login' in authorization_config_old: + log.log_info('TACACS per-command authorization already setup.') + return + + # setup per-command authorization by device type + authorization_config = { 'login' : 'tacacs+' } + if "Mgmt" in localhost_info_old.get('type'): + # On mgmt device, failback to local authorizarion when TACACS unreachable + authorization_config = { 'login' : 'tacacs+,local' } + + self.configDB.set_entry('AAA', 'authorization', authorization_config) + log.log_info('Setup per-command authorization to: {}'.format(authorization_config)) + def common_migration_ops(self): try: diff --git a/tests/db_migrator_input/config_db/per_command_aaa_disable.json b/tests/db_migrator_input/config_db/per_command_aaa_disable.json index e69628e446..db9d4a7a9f 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_disable.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_disable.json @@ -1,10 +1,9 @@ { - "AAA": { + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" }, - "TACPLUS": { - "global": { - "auth_type": "login", - "passkey": "testpasskey" - } + "DEVICE_METADATA|localhost": { + "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json index e69628e446..db9d4a7a9f 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json @@ -1,10 +1,9 @@ { - "AAA": { + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" }, - "TACPLUS": { - "global": { - "auth_type": "login", - "passkey": "testpasskey" - } + "DEVICE_METADATA|localhost": { + "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable.json b/tests/db_migrator_input/config_db/per_command_aaa_enable.json index f817701b0f..abf3112ac1 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_enable.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable.json @@ -1,13 +1,12 @@ { - "AAA": { - "authentication": { - "login": "tacacs+" - } + "AAA|authentication": { + "login": "tacacs+" }, - "TACPLUS": { - "global": { - "auth_type": "login", - "passkey": "testpasskey" - } + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + }, + "DEVICE_METADATA|localhost": { + "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json index 005a2fd398..0b9f92a1a8 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json @@ -1,19 +1,18 @@ { - "AAA": { - "accounting": { - "login": "tacacs+,local" - }, - "authentication": { - "login": "tacacs+" - }, - "authorization": { - "login": "tacacs+" - } + "AAA|accounting": { + "login": "tacacs+,local" }, - "TACPLUS": { - "global": { - "auth_type": "login", - "passkey": "testpasskey" - } + "AAA|authentication": { + "login": "tacacs+" + }, + "AAA|authorization": { + "login": "tacacs+" + }, + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + }, + "DEVICE_METADATA|localhost": { + "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt.json new file mode 100644 index 0000000000..a7d7b8d68c --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt.json @@ -0,0 +1,12 @@ +{ + "AAA|authentication": { + "login": "tacacs+" + }, + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + }, + "DEVICE_METADATA|localhost": { + "type": "MgmtToRRouter" + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt_expected.json new file mode 100644 index 0000000000..c39f71d1f5 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt_expected.json @@ -0,0 +1,18 @@ +{ + "AAA|accounting": { + "login": "tacacs+,local" + }, + "AAA|authentication": { + "login": "tacacs+" + }, + "AAA|authorization": { + "login": "tacacs+,local" + }, + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + }, + "DEVICE_METADATA|localhost": { + "type": "MgmtToRRouter" + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_change.json b/tests/db_migrator_input/config_db/per_command_aaa_no_change.json index 2e83bab807..91b15f1903 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_no_change.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_change.json @@ -1,19 +1,18 @@ { - "AAA": { - "accounting": { - "login": "local" - }, - "authentication": { - "login": "tacacs+" - }, - "authorization": { - "login": "local" - } + "AAA|accounting": { + "login": "local" }, - "TACPLUS": { - "global": { - "auth_type": "login", - "passkey": "testpasskey" - } + "AAA|authentication": { + "login": "tacacs+" + }, + "AAA|authorization": { + "login": "local" + }, + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + }, + "DEVICE_METADATA|localhost": { + "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json index 2e83bab807..91b15f1903 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json @@ -1,19 +1,18 @@ { - "AAA": { - "accounting": { - "login": "local" - }, - "authentication": { - "login": "tacacs+" - }, - "authorization": { - "login": "local" - } + "AAA|accounting": { + "login": "local" }, - "TACPLUS": { - "global": { - "auth_type": "login", - "passkey": "testpasskey" - } + "AAA|authentication": { + "login": "tacacs+" + }, + "AAA|authorization": { + "login": "local" + }, + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + }, + "DEVICE_METADATA|localhost": { + "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json index c5b59d46a4..00c2ab2c30 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json @@ -1,12 +1,11 @@ { - "AAA": { - "authentication": { - "login": "tacacs+" - } + "AAA|authentication": { + "login": "tacacs+" }, - "TACPLUS": { - "global": { - "auth_type": "login" - } + "TACPLUS|global": { + "auth_type": "login" + }, + "DEVICE_METADATA|localhost": { + "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json index b06af48439..e22339369d 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json @@ -1,15 +1,14 @@ { - "AAA": { - "accounting": { - "login": "tacacs+,local" - }, - "authentication": { - "login": "tacacs+" - } + "AAA|accounting": { + "login": "tacacs+,local" }, - "TACPLUS": { - "global": { - "auth_type": "login" - } + "AAA|authentication": { + "login": "tacacs+" + }, + "TACPLUS|global": { + "auth_type": "login" + }, + "DEVICE_METADATA|localhost": { + "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index 0111ba669f..d26c959b64 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -956,7 +956,8 @@ def teardown_class(cls): dbconnector.dedicated_dbs['CONFIG_DB'] = None @pytest.mark.parametrize('test_json', ['per_command_aaa_enable', - 'per_command_aaa_no_passkey_expected', + 'per_command_aaa_enable_mgmt', + 'per_command_aaa_no_passkey', 'per_command_aaa_disable', 'per_command_aaa_no_change']) def test_migrator_configdb_per_command_aaa(self, test_json): diff --git a/tests/mock_tables/dbconnector.py b/tests/mock_tables/dbconnector.py index 2570967192..4ccb392368 100644 --- a/tests/mock_tables/dbconnector.py +++ b/tests/mock_tables/dbconnector.py @@ -121,8 +121,6 @@ def __init__(self, *args, **kwargs): else: for attr, value in v.items(): self.hset(k, attr, value) - else: - print("SwssSyncClient: failed to load mock database json file: {}".format(fname)) # Patch mockredis/mockredis/client.py # The offical implementation assume decode_responses=False From 61e9f3aa432eb7ffa0170e5c4984c273934b32ad Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Mon, 29 Apr 2024 03:33:22 +0000 Subject: [PATCH 10/13] Add both accoutning and authorization --- scripts/db_migrator.py | 97 +++++++++---------- .../config_db/per_command_aaa_disable.json | 3 - .../per_command_aaa_disable_expected.json | 3 - .../per_command_aaa_disable_golden.json | 8 ++ .../config_db/per_command_aaa_enable.json | 3 - .../per_command_aaa_enable_expected.json | 3 - .../per_command_aaa_enable_golden.json | 19 ++++ .../per_command_aaa_enable_mgmt.json | 12 --- .../per_command_aaa_enable_mgmt_expected.json | 18 ---- .../config_db/per_command_aaa_no_change.json | 3 - .../per_command_aaa_no_change_expected.json | 3 - .../per_command_aaa_no_change_golden.json | 19 ++++ .../config_db/per_command_aaa_no_passkey.json | 3 - .../per_command_aaa_no_passkey_expected.json | 3 - .../per_command_aaa_no_passkey_golden.json | 15 +++ tests/db_migrator_test.py | 28 ++++-- 16 files changed, 128 insertions(+), 112 deletions(-) create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_disable_golden.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_enable_golden.json delete mode 100644 tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt.json delete mode 100644 tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt_expected.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_change_golden.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_passkey_golden.json diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index 997890e747..0b870c3e9b 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -808,6 +808,52 @@ def migrate_sflow_table(self): sflow_key = "SFLOW_SESSION_TABLE:{}".format(key) self.appDB.set(self.appDB.APPL_DB, sflow_key, 'sample_direction','rx') + def migrate_tacplus(self): + if not self.config_src_data or 'TACPLUS' not in self.config_src_data: + return + + tacplus_new = self.config_src_data['TACPLUS'] + log.log_notice('Migrate TACPLUS configuration') + + global_old = self.configDB.get_entry('TACPLUS', 'global') + if not global_old: + global_new = tacplus_new.get("global") + self.configDB.set_entry("TACPLUS", "global", global_new) + log.log_info('Migrate TACPLUS global: {}'.format(global_new)) + + def migrate_aaa(self): + if not self.config_src_data or 'AAA' not in self.config_src_data: + return + + aaa_new = self.config_src_data['AAA'] + log.log_notice('Migrate AAA configuration') + + authentication = self.configDB.get_entry('AAA', 'authentication') + if not authentication: + authentication_new = aaa_new.get("authentication") + self.configDB.set_entry("AAA", "authentication", authentication_new) + log.log_info('Migrate AAA authentication: {}'.format(authentication_new)) + + # setup per-command accounting + accounting = self.configDB.get_entry('AAA', 'accounting') + if not accounting: + accounting_new = aaa_new.get("accounting") + self.configDB.set_entry("AAA", "accounting", accounting_new) + log.log_info('Migrate AAA accounting: {}'.format(accounting_new)) + + # setup per-command authorization + tacplus_config = self.configDB.get_entry('TACPLUS', 'global') + if 'passkey' in tacplus_config and '' != tacplus_config.get('passkey'): + authorization = self.configDB.get_entry('AAA', 'authorization') + if not authorization: + authorization_new = aaa_new.get("authorization") + self.configDB.set_entry("AAA", "authorization", authorization_new) + log.log_info('Migrate AAA authorization: {}'.format(authorization_new)) + else: + # If no passkey, setup per-command authorization will block remote user command + log.log_info('TACACS passkey does not exist, ignore setup per-command authorization.') + + def version_unknown(self): """ version_unknown tracks all SONiC versions that doesn't have a version @@ -1200,55 +1246,6 @@ def set_version(self, version=None): entry = { self.TABLE_FIELD : version } self.configDB.set_entry(self.TABLE_NAME, self.TABLE_KEY, entry) - def enable_per_command_aaa(self): - aaa_table = self.configDB.get_table('AAA') - if not aaa_table: - log.log_info('AAA table does not exist ignore setup per-command accounting&authorization.') - return - - authentication_config_old = self.configDB.get_entry('AAA', 'authentication') - # If device enabled TACACS authentication, then enable TACACS per-command accounting and authorization - if 'login' not in authentication_config_old or 'tacacs+' != authentication_config_old.get('login'): - log.log_info('TACACS authentication disabled, ignore setup per-command accounting&authorization.') - return - - # setup per-command accounting - accounting_config_old = self.configDB.get_entry('AAA', 'accounting') - if 'login' not in accounting_config_old: - accounting_config = { 'login' : 'tacacs+,local' } - self.configDB.set_entry('AAA', 'accounting', accounting_config) - log.log_info('Setup per-command accounting to: {}'.format(accounting_config)) - else: - log.log_info('TACACS per-command accounting already setup.') - - # setup per-command authorization - tacplus_config_old = self.configDB.get_entry('TACPLUS', 'global') - if 'passkey' not in tacplus_config_old or '' == tacplus_config_old.get('passkey'): - # If no passkey, setup per-command authorization will block remote user command - log.log_info('TACACS passkey does not exist, ignore setup per-command authorization.') - return - - localhost_info_old = self.configDB.get_entry('DEVICE_METADATA', 'localhost') - if 'type' not in localhost_info_old: - # If no type, setup per-command authorization has potensial risk on mgmt device - log.log_info('Device type does not exist, ignore setup per-command authorization.') - return - - authorization_config_old = self.configDB.get_entry('AAA', 'authorization') - if 'login' in authorization_config_old: - log.log_info('TACACS per-command authorization already setup.') - return - - # setup per-command authorization by device type - authorization_config = { 'login' : 'tacacs+' } - if "Mgmt" in localhost_info_old.get('type'): - # On mgmt device, failback to local authorizarion when TACACS unreachable - authorization_config = { 'login' : 'tacacs+,local' } - - self.configDB.set_entry('AAA', 'authorization', authorization_config) - log.log_info('Setup per-command authorization to: {}'.format(authorization_config)) - - def common_migration_ops(self): try: with open(INIT_CFG_FILE) as f: @@ -1283,8 +1280,6 @@ def common_migration_ops(self): self.update_edgezone_aggregator_config() # update FRR config mode based on minigraph parser on target image self.migrate_routing_config_mode() - # enable per-command authorization/accounting feature for 202205 & 202305 - self.enable_per_command_aaa() def migrate(self): version = self.get_version() diff --git a/tests/db_migrator_input/config_db/per_command_aaa_disable.json b/tests/db_migrator_input/config_db/per_command_aaa_disable.json index db9d4a7a9f..215e3d7fe3 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_disable.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_disable.json @@ -2,8 +2,5 @@ "TACPLUS|global": { "auth_type": "login", "passkey": "testpasskey" - }, - "DEVICE_METADATA|localhost": { - "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json index db9d4a7a9f..215e3d7fe3 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_disable_expected.json @@ -2,8 +2,5 @@ "TACPLUS|global": { "auth_type": "login", "passkey": "testpasskey" - }, - "DEVICE_METADATA|localhost": { - "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_disable_golden.json b/tests/db_migrator_input/config_db/per_command_aaa_disable_golden.json new file mode 100644 index 0000000000..abc38879b6 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_disable_golden.json @@ -0,0 +1,8 @@ +{ + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable.json b/tests/db_migrator_input/config_db/per_command_aaa_enable.json index abf3112ac1..0026e03850 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_enable.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable.json @@ -5,8 +5,5 @@ "TACPLUS|global": { "auth_type": "login", "passkey": "testpasskey" - }, - "DEVICE_METADATA|localhost": { - "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json index 0b9f92a1a8..0e832d1d73 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json @@ -11,8 +11,5 @@ "TACPLUS|global": { "auth_type": "login", "passkey": "testpasskey" - }, - "DEVICE_METADATA|localhost": { - "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_golden.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_golden.json new file mode 100644 index 0000000000..005a2fd398 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable_golden.json @@ -0,0 +1,19 @@ +{ + "AAA": { + "accounting": { + "login": "tacacs+,local" + }, + "authentication": { + "login": "tacacs+" + }, + "authorization": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt.json deleted file mode 100644 index a7d7b8d68c..0000000000 --- a/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "AAA|authentication": { - "login": "tacacs+" - }, - "TACPLUS|global": { - "auth_type": "login", - "passkey": "testpasskey" - }, - "DEVICE_METADATA|localhost": { - "type": "MgmtToRRouter" - } -} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt_expected.json deleted file mode 100644 index c39f71d1f5..0000000000 --- a/tests/db_migrator_input/config_db/per_command_aaa_enable_mgmt_expected.json +++ /dev/null @@ -1,18 +0,0 @@ -{ - "AAA|accounting": { - "login": "tacacs+,local" - }, - "AAA|authentication": { - "login": "tacacs+" - }, - "AAA|authorization": { - "login": "tacacs+,local" - }, - "TACPLUS|global": { - "auth_type": "login", - "passkey": "testpasskey" - }, - "DEVICE_METADATA|localhost": { - "type": "MgmtToRRouter" - } -} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_change.json b/tests/db_migrator_input/config_db/per_command_aaa_no_change.json index 91b15f1903..518e1af6db 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_no_change.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_change.json @@ -11,8 +11,5 @@ "TACPLUS|global": { "auth_type": "login", "passkey": "testpasskey" - }, - "DEVICE_METADATA|localhost": { - "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json index 91b15f1903..518e1af6db 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_change_expected.json @@ -11,8 +11,5 @@ "TACPLUS|global": { "auth_type": "login", "passkey": "testpasskey" - }, - "DEVICE_METADATA|localhost": { - "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_change_golden.json b/tests/db_migrator_input/config_db/per_command_aaa_no_change_golden.json new file mode 100644 index 0000000000..005a2fd398 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_change_golden.json @@ -0,0 +1,19 @@ +{ + "AAA": { + "accounting": { + "login": "tacacs+,local" + }, + "authentication": { + "login": "tacacs+" + }, + "authorization": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json index 00c2ab2c30..6ec39507a1 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json @@ -4,8 +4,5 @@ }, "TACPLUS|global": { "auth_type": "login" - }, - "DEVICE_METADATA|localhost": { - "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json index e22339369d..690620e52f 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_expected.json @@ -7,8 +7,5 @@ }, "TACPLUS|global": { "auth_type": "login" - }, - "DEVICE_METADATA|localhost": { - "type": "ToRRouter" } } \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_golden.json b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_golden.json new file mode 100644 index 0000000000..b06af48439 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_passkey_golden.json @@ -0,0 +1,15 @@ +{ + "AAA": { + "accounting": { + "login": "tacacs+,local" + }, + "authentication": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index d26c959b64..e743fef886 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -4,6 +4,7 @@ import argparse from unittest import mock from deepdiff import DeepDiff +import json from swsscommon.swsscommon import SonicV2Connector from sonic_py_common import device_info @@ -955,25 +956,38 @@ def teardown_class(cls): os.environ['UTILITIES_UNIT_TESTING'] = "0" dbconnector.dedicated_dbs['CONFIG_DB'] = None + def load_golden_config(self, dbmgtr, test_json): + dbmgtr.config_src_data = {} + + json_path = os.path.join(mock_db_path, 'config_db', test_json + ".json") + if os.path.exists(json_path): + with open(json_path) as f: + dbmgtr.config_src_data = json.load(f) + print("test_per_command_aaa load golden config success, config_src_data: {}".format(dbmgtr.config_src_data)) + else: + print("test_per_command_aaa load golden config failed, file {} does not exist.".format(test_json)) + + @pytest.mark.parametrize('test_json', ['per_command_aaa_enable', - 'per_command_aaa_enable_mgmt', 'per_command_aaa_no_passkey', 'per_command_aaa_disable', 'per_command_aaa_no_change']) - def test_migrator_configdb_per_command_aaa(self, test_json): + def test_per_command_aaa(self, test_json): dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json) import db_migrator dbmgtr = db_migrator.DBMigrator(None) - dbmgtr.migrate() + self.load_golden_config(dbmgtr, test_json + '_golden') + dbmgtr.migrate_tacplus() + dbmgtr.migrate_aaa() resulting_table = dbmgtr.configDB.get_table("AAA") dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json + '_expected') expected_db = Db() expected_table = expected_db.cfgdb.get_table("AAA") - - print("test_migrator_configdb_per_command_aaa: {}".format(test_json)) - print("test_migrator_configdb_per_command_aaa, resulting_table: {}".format(resulting_table)) - print("test_migrator_configdb_per_command_aaa, expected_table: {}".format(expected_table)) + + print("test_per_command_aaa: {}".format(test_json)) + print("test_per_command_aaa, resulting_table: {}".format(resulting_table)) + print("test_per_command_aaa, expected_table: {}".format(expected_table)) diff = DeepDiff(resulting_table, expected_table, ignore_order=True) assert not diff From b35dc64eefe0a453cb351c4b2729a672921a8feb Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Mon, 29 Apr 2024 05:29:32 +0000 Subject: [PATCH 11/13] Add minrate method --- scripts/db_migrator.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index 0b870c3e9b..1af4059804 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -1281,6 +1281,9 @@ def common_migration_ops(self): # update FRR config mode based on minigraph parser on target image self.migrate_routing_config_mode() + self.migrate_tacplus() + self.migrate_aaa() + def migrate(self): version = self.get_version() log.log_info('Upgrading from version ' + version) From 008092a72167d03e1e98f27dc2d478e4eecdf11c Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Mon, 29 Apr 2024 05:33:51 +0000 Subject: [PATCH 12/13] remove migrate accoutning part to another PR --- scripts/db_migrator.py | 13 ------------- .../config_db/per_command_aaa_enable_expected.json | 3 --- 2 files changed, 16 deletions(-) diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index 1af4059804..9a1c36a88c 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -841,19 +841,6 @@ def migrate_aaa(self): self.configDB.set_entry("AAA", "accounting", accounting_new) log.log_info('Migrate AAA accounting: {}'.format(accounting_new)) - # setup per-command authorization - tacplus_config = self.configDB.get_entry('TACPLUS', 'global') - if 'passkey' in tacplus_config and '' != tacplus_config.get('passkey'): - authorization = self.configDB.get_entry('AAA', 'authorization') - if not authorization: - authorization_new = aaa_new.get("authorization") - self.configDB.set_entry("AAA", "authorization", authorization_new) - log.log_info('Migrate AAA authorization: {}'.format(authorization_new)) - else: - # If no passkey, setup per-command authorization will block remote user command - log.log_info('TACACS passkey does not exist, ignore setup per-command authorization.') - - def version_unknown(self): """ version_unknown tracks all SONiC versions that doesn't have a version diff --git a/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json index 0e832d1d73..d39c98b7a5 100644 --- a/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json +++ b/tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json @@ -5,9 +5,6 @@ "AAA|authentication": { "login": "tacacs+" }, - "AAA|authorization": { - "login": "tacacs+" - }, "TACPLUS|global": { "auth_type": "login", "passkey": "testpasskey" From 3f3dcbfff653ccdf07a5bf073c85c3516e282e1b Mon Sep 17 00:00:00 2001 From: liuh-80 Date: Mon, 29 Apr 2024 06:51:09 +0000 Subject: [PATCH 13/13] Improve test coverage --- .../per_command_aaa_no_authentication.json | 9 +++++++++ ...ommand_aaa_no_authentication_expected.json | 12 ++++++++++++ ..._command_aaa_no_authentication_golden.json | 19 +++++++++++++++++++ .../config_db/per_command_aaa_no_tacplus.json | 5 +++++ .../per_command_aaa_no_tacplus_expected.json | 12 ++++++++++++ .../per_command_aaa_no_tacplus_golden.json | 19 +++++++++++++++++++ tests/db_migrator_test.py | 4 +++- 7 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_authentication.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_authentication_expected.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_authentication_golden.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_tacplus.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_expected.json create mode 100644 tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_golden.json diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_authentication.json b/tests/db_migrator_input/config_db/per_command_aaa_no_authentication.json new file mode 100644 index 0000000000..694d2f5cb3 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_authentication.json @@ -0,0 +1,9 @@ +{ + "AAA|accounting": { + "login": "tacacs+,local" + }, + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_authentication_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_no_authentication_expected.json new file mode 100644 index 0000000000..d39c98b7a5 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_authentication_expected.json @@ -0,0 +1,12 @@ +{ + "AAA|accounting": { + "login": "tacacs+,local" + }, + "AAA|authentication": { + "login": "tacacs+" + }, + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_authentication_golden.json b/tests/db_migrator_input/config_db/per_command_aaa_no_authentication_golden.json new file mode 100644 index 0000000000..005a2fd398 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_authentication_golden.json @@ -0,0 +1,19 @@ +{ + "AAA": { + "accounting": { + "login": "tacacs+,local" + }, + "authentication": { + "login": "tacacs+" + }, + "authorization": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus.json b/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus.json new file mode 100644 index 0000000000..c45e0745ed --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus.json @@ -0,0 +1,5 @@ +{ + "AAA|authentication": { + "login": "tacacs+" + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_expected.json b/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_expected.json new file mode 100644 index 0000000000..d39c98b7a5 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_expected.json @@ -0,0 +1,12 @@ +{ + "AAA|accounting": { + "login": "tacacs+,local" + }, + "AAA|authentication": { + "login": "tacacs+" + }, + "TACPLUS|global": { + "auth_type": "login", + "passkey": "testpasskey" + } +} \ No newline at end of file diff --git a/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_golden.json b/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_golden.json new file mode 100644 index 0000000000..005a2fd398 --- /dev/null +++ b/tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_golden.json @@ -0,0 +1,19 @@ +{ + "AAA": { + "accounting": { + "login": "tacacs+,local" + }, + "authentication": { + "login": "tacacs+" + }, + "authorization": { + "login": "tacacs+" + } + }, + "TACPLUS": { + "global": { + "auth_type": "login", + "passkey": "testpasskey" + } + } +} \ No newline at end of file diff --git a/tests/db_migrator_test.py b/tests/db_migrator_test.py index e743fef886..9a054d8924 100644 --- a/tests/db_migrator_test.py +++ b/tests/db_migrator_test.py @@ -971,7 +971,9 @@ def load_golden_config(self, dbmgtr, test_json): @pytest.mark.parametrize('test_json', ['per_command_aaa_enable', 'per_command_aaa_no_passkey', 'per_command_aaa_disable', - 'per_command_aaa_no_change']) + 'per_command_aaa_no_change', + 'per_command_aaa_no_tacplus', + 'per_command_aaa_no_authentication']) def test_per_command_aaa(self, test_json): dbconnector.dedicated_dbs['CONFIG_DB'] = os.path.join(mock_db_path, 'config_db', test_json) import db_migrator