From 451cfd65320397db15b9526087e1904b4ae6040f Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Thu, 5 Sep 2024 13:26:52 -0700 Subject: [PATCH 1/7] Add 'controller' option to audit.query Valid 'controller' values are 'MASTER','Active','BACKUP','Standby'. The default is the 'current' controller. Add retries with delay on remote DB call in CI test. Enhance the CI test asserts. --- .../middlewared/plugins/audit/audit.py | 26 +++++++++- .../middlewared/plugins/audit/utils.py | 2 + tests/api2/test_audit_basic.py | 51 ++++++++++++++++++- 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/middlewared/middlewared/plugins/audit/audit.py b/src/middlewared/middlewared/plugins/audit/audit.py index 07a029f55bd5..9b336d928089 100644 --- a/src/middlewared/middlewared/plugins/audit/audit.py +++ b/src/middlewared/middlewared/plugins/audit/audit.py @@ -12,6 +12,7 @@ from truenas_api_client import json as ejson from .utils import ( + AUDIT_CONTROLLER_SELECTIONS, AUDIT_DATASET_PATH, AUDIT_LIFETIME, AUDIT_DEFAULT_RESERVATION, @@ -135,6 +136,7 @@ async def compress(self, data): List('services', items=[Str('db_name', enum=ALL_AUDITED)], default=NON_BULK_AUDIT), Ref('query-filters'), Ref('query-options'), + Str('controller', enum=AUDIT_CONTROLLER_SELECTIONS, null=True, default=None), register=True )) @filterable_returns(Dict( @@ -159,6 +161,10 @@ async def query(self, data): converted into a more efficient form for better performance. This will not be possible if filters use keys within `svc_data` and `event_data`. + HA systems may specify the controller. The active controller may be + selected with 'MASTER' or 'Active'. The standby controller may be selected + with 'BACKUP' or 'Standby'. The default is the 'current' controller. + Each audit entry contains the following keys: `audit_id` - GUID uniquely identifying this specific audit event. @@ -193,9 +199,27 @@ async def query(self, data): `success` - boolean value indicating whether the action generating the event message succeeded. """ - sql_filters = data['query-options']['force_sql_filters'] verrors = ValidationErrors() + + # If HA, handle the possibility of remote controller requests + ctrlr_state = await self.middleware.call('failover.status') + + # want_ctrlr: Default is 'current' controller else use selection + want_ctrlr = ctrlr_state if data['controller'] is None else ( + 'MASTER' if data['controller'] in ['MASTER', 'Active'] else 'BACKUP' + ) + if ctrlr_state != 'SINGLE': + if ctrlr_state not in ['MASTER', 'BACKUP']: + verrors.add('audit.query.controller', f'controller status: {ctrlr_state}') + verrors.check() + else: + if want_ctrlr != ctrlr_state: + # Get the data from the other controller + return await self.middleware.call('failover.call_remote', 'audit.query', [data]) + + sql_filters = data['query-options']['force_sql_filters'] + if (select := data['query-options'].get('select')): for idx, entry in enumerate(select): if isinstance(entry, list): diff --git a/src/middlewared/middlewared/plugins/audit/utils.py b/src/middlewared/middlewared/plugins/audit/utils.py index 230169b54289..6791c2e0cbee 100644 --- a/src/middlewared/middlewared/plugins/audit/utils.py +++ b/src/middlewared/middlewared/plugins/audit/utils.py @@ -24,6 +24,8 @@ AuditEventParam.EVENT.value, AuditEventParam.SUCCESS.value, ) +AUDIT_CONTROLLER_SELECTIONS = ['MASTER', 'Active', 'BACKUP', 'Standby'] + AuditBase = declarative_base() diff --git a/tests/api2/test_audit_basic.py b/tests/api2/test_audit_basic.py index ec89b5dced2c..b98862090096 100644 --- a/tests/api2/test_audit_basic.py +++ b/tests/api2/test_audit_basic.py @@ -4,6 +4,7 @@ from middlewared.test.integration.utils import call, url from middlewared.test.integration.utils.audit import get_audit_entry +from auto_config import ha from protocols import smb_connection from time import sleep @@ -13,6 +14,8 @@ import secrets import string +ha_test = pytest.mark.skipif(not (ha and "virtual_ip" in os.environ), reason="Skip HA tests") + SMBUSER = 'audit-smb-user' PASSWD = ''.join(secrets.choice(string.ascii_letters + string.digits) for i in range(10)) @@ -43,7 +46,6 @@ class AUDIT_CONFIG(): } -# def get_zfs(key, zfs_config): def get_zfs(data_type, key, zfs_config): """ Get the equivalent ZFS value associated with the audit config setting """ @@ -64,7 +66,6 @@ def get_zfs(data_type, key, zfs_config): 'used_by_reservation': zfs_config['properties']['usedbyrefreservation']['parsed'] } } - # return zfs[key] return types[data_type][key] @@ -97,6 +98,27 @@ def init_audit(): call('audit.update', AUDIT_CONFIG.defaults) +@pytest.fixture(scope="function") +def standby_user(): + """ HA system: Create a user on the BACKUP node + This will generate a 'create' audit entry, yield, + and on exit generate a 'delete' audit entry. + """ + try: + name = "StandbyUser" + PASSWD + user_id = call('failover.call_remote', 'user.create', [{ + "username": name, + "full_name": name + " Deleteme", + "group": 100, + "smb": False, + "home_create": False, + "password": "testing" + }]) + yield name + finally: + call('failover.call_remote', 'user.delete', [user_id]) + + # ===================================================================== # Tests # ===================================================================== @@ -282,3 +304,28 @@ def test_audit_timestamps(self, svc): ae_ts_ts = int(audit_entry['timestamp'].timestamp()) ae_msg_ts = int(audit_entry['message_timestamp']) assert abs(ae_ts_ts - ae_msg_ts) < 2, f"$date='{ae_ts_ts}, message_timestamp={ae_msg_ts}" + + @ha_test + def test_audit_ha_query(self, standby_user): + name = standby_user + remote_user = call('failover.call_remote', 'user.query', [[["username", "=", name]]]) + assert remote_user != [] + + # Handle delays in the audit database + remote_audit_entry = [] + tries = 3 + while tries > 0 and remote_audit_entry == []: + sleep(1) + remote_audit_entry = call('audit.query', { + "query-filters": [["event_data.description", "$", name]], + "query-options": {"select": ["event_data", "success"]}, + "controller": "Standby" + }) + if remote_audit_entry != []: + break + tries -= 1 + + assert tries > 0, "Failed to get expected audit entry" + assert remote_audit_entry != [] + params = remote_audit_entry[0]['event_data']['params'][0] + assert params['username'] == name From 6353dbd15e6d80513b3bcd35c7d92bc4ecc2ee4e Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Mon, 9 Sep 2024 18:06:09 -0700 Subject: [PATCH 2/7] Add download CI tests. Still in debug. --- tests/api2/test_audit_basic.py | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/tests/api2/test_audit_basic.py b/tests/api2/test_audit_basic.py index b98862090096..005539d09b73 100644 --- a/tests/api2/test_audit_basic.py +++ b/tests/api2/test_audit_basic.py @@ -98,12 +98,13 @@ def init_audit(): call('audit.update', AUDIT_CONFIG.defaults) -@pytest.fixture(scope="function") +@pytest.fixture(scope='class') def standby_user(): """ HA system: Create a user on the BACKUP node This will generate a 'create' audit entry, yield, and on exit generate a 'delete' audit entry. """ + user_id = None try: name = "StandbyUser" + PASSWD user_id = call('failover.call_remote', 'user.create', [{ @@ -116,7 +117,8 @@ def standby_user(): }]) yield name finally: - call('failover.call_remote', 'user.delete', [user_id]) + if user_id is not None: + call('failover.call_remote', 'user.delete', [user_id]) # ===================================================================== @@ -305,7 +307,9 @@ def test_audit_timestamps(self, svc): ae_msg_ts = int(audit_entry['message_timestamp']) assert abs(ae_ts_ts - ae_msg_ts) < 2, f"$date='{ae_ts_ts}, message_timestamp={ae_msg_ts}" - @ha_test + +@ha_test +class TestAuditOpsHA: def test_audit_ha_query(self, standby_user): name = standby_user remote_user = call('failover.call_remote', 'user.query', [[["username", "=", name]]]) @@ -329,3 +333,26 @@ def test_audit_ha_query(self, standby_user): assert remote_audit_entry != [] params = remote_audit_entry[0]['event_data']['params'][0] assert params['username'] == name + + def test_audit_ha_export(self): + # for backend in ['CSV', 'JSON', 'YAML']: + report_path = call('audit.export', {'export_format': 'CSV'}, job=True) + # assert report_path.startswith('/audit/reports/root/') + # st = call('filesystem.stat', report_path) + # assert st['size'] != 0, str(st) + + job_id, path = call( + "core.download", "audit.download_report", + [{"report_name": os.path.basename(report_path)}], + "report.csv" + # f"report.{backend.lower()}" + ) + r = requests.get(f"{url()}{path}") + r.raise_for_status() + # assert len(r.content) == st['size'] + # lines = 1 + # for line in r.iter_content(): + # print(f"[MCG DEBUG] [{lines}] {line}") + # lines += 1 + # if lines > 10: + # break From b9909c5f4c7b647f0e6ca08147d71887a7e69844 Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Tue, 10 Sep 2024 10:37:04 -0700 Subject: [PATCH 3/7] Add wait for audit entry. Create common helper function. Complete download CI test. --- tests/api2/test_audit_basic.py | 67 +++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/tests/api2/test_audit_basic.py b/tests/api2/test_audit_basic.py index 005539d09b73..ab39a4e09cca 100644 --- a/tests/api2/test_audit_basic.py +++ b/tests/api2/test_audit_basic.py @@ -69,6 +69,24 @@ def get_zfs(data_type, key, zfs_config): return types[data_type][key] +def check_audit_download(report_path, report_type, tag=None): + """ Download audit DB (root user) + If requested, assert the tag is present + INPUT: report_type ['CSV'|'JSON'|'YAML'] + RETURN: lenght of content (bytes) + """ + job_id, url_path = call( + "core.download", "audit.download_report", + [{"report_name": os.path.basename(report_path)}], + f"report.{report_type.lower()}" + ) + r = requests.get(f"{url()}{url_path}") + r.raise_for_status() + if tag is not None: + assert f"{tag}" in r.text + return len(r.content) + + @pytest.fixture(scope='class') def initialize_for_smb_tests(): with dataset('audit-test-basic', data={'share_type': 'SMB'}) as ds: @@ -266,14 +284,8 @@ def test_audit_export(self): st = call('filesystem.stat', report_path) assert st['size'] != 0, str(st) - job_id, path = call( - "core.download", "audit.download_report", - [{"report_name": os.path.basename(report_path)}], - f"report.{backend.lower()}" - ) - r = requests.get(f"{url()}{path}") - r.raise_for_status() - assert len(r.content) == st['size'] + content_len = check_audit_download(report_path, backend) + assert content_len == st['size'] def test_audit_export_nonroot(self): with unprivileged_user_client(roles=['SYSTEM_AUDIT_READ', 'FILESYSTEM_ATTRS_READ']) as c: @@ -286,6 +298,7 @@ def test_audit_export_nonroot(self): st = c.call('filesystem.stat', report_path) assert st['size'] != 0, str(st) + # Make the call as the client job_id, path = c.call( "core.download", "audit.download_report", [{"report_name": os.path.basename(report_path)}], @@ -334,25 +347,19 @@ def test_audit_ha_query(self, standby_user): params = remote_audit_entry[0]['event_data']['params'][0] assert params['username'] == name - def test_audit_ha_export(self): - # for backend in ['CSV', 'JSON', 'YAML']: - report_path = call('audit.export', {'export_format': 'CSV'}, job=True) - # assert report_path.startswith('/audit/reports/root/') - # st = call('filesystem.stat', report_path) - # assert st['size'] != 0, str(st) - - job_id, path = call( - "core.download", "audit.download_report", - [{"report_name": os.path.basename(report_path)}], - "report.csv" - # f"report.{backend.lower()}" - ) - r = requests.get(f"{url()}{path}") - r.raise_for_status() - # assert len(r.content) == st['size'] - # lines = 1 - # for line in r.iter_content(): - # print(f"[MCG DEBUG] [{lines}] {line}") - # lines += 1 - # if lines > 10: - # break + def test_audit_ha_export(self, standby_user): + """ + Confirm we can download 'Active' and 'Standby' audit DB. + With a user created on the 'Standby' controller download the + audit DB from both controllers and confirm the user create is + in the 'Standby' audit DB and not in the 'Active' audit DB. + """ + report_path_active = call('audit.export', {'export_format': 'CSV', 'controller': 'MASTER'}, job=True) + report_path_standby = call('audit.export', {'export_format': 'CSV', 'controller': 'BACKUP'}, job=True) + + # Confirm entry NOT in active controller audit DB + with pytest.raises(AssertionError): + check_audit_download(report_path_active, 'CSV', f"Create user {standby_user}") + + # Confirm entry IS in standby controller audit DB + check_audit_download(report_path_standby, 'CSV', f"Create user {standby_user}") From 4ad858ea212c9051393e899a583356cbc4e1f3bd Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Wed, 11 Sep 2024 08:17:55 -0700 Subject: [PATCH 4/7] Change 'contoller' option to T/F and rename to 'remote_controller' Improve failure handling on the 'remote_call'. Remove changes to audit/utils.py Update CI tests. --- .../middlewared/plugins/audit/audit.py | 34 +++++++++---------- .../middlewared/plugins/audit/utils.py | 1 - tests/api2/test_audit_basic.py | 10 +++--- 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/middlewared/middlewared/plugins/audit/audit.py b/src/middlewared/middlewared/plugins/audit/audit.py index 9b336d928089..f749398a590c 100644 --- a/src/middlewared/middlewared/plugins/audit/audit.py +++ b/src/middlewared/middlewared/plugins/audit/audit.py @@ -12,7 +12,6 @@ from truenas_api_client import json as ejson from .utils import ( - AUDIT_CONTROLLER_SELECTIONS, AUDIT_DATASET_PATH, AUDIT_LIFETIME, AUDIT_DEFAULT_RESERVATION, @@ -32,7 +31,7 @@ accepts, Bool, Datetime, Dict, Int, List, Patch, Ref, returns, Str, UUID ) from middlewared.service import filterable, filterable_returns, job, private, ConfigService -from middlewared.service_exception import CallError, ValidationErrors +from middlewared.service_exception import CallError, ValidationErrors, ValidationError from middlewared.utils import filter_list from middlewared.utils.mount import getmntinfo from middlewared.utils.functools_ import cache @@ -136,7 +135,7 @@ async def compress(self, data): List('services', items=[Str('db_name', enum=ALL_AUDITED)], default=NON_BULK_AUDIT), Ref('query-filters'), Ref('query-options'), - Str('controller', enum=AUDIT_CONTROLLER_SELECTIONS, null=True, default=None), + Bool('remote_controller', default=False), register=True )) @filterable_returns(Dict( @@ -161,9 +160,8 @@ async def query(self, data): converted into a more efficient form for better performance. This will not be possible if filters use keys within `svc_data` and `event_data`. - HA systems may specify the controller. The active controller may be - selected with 'MASTER' or 'Active'. The standby controller may be selected - with 'BACKUP' or 'Standby'. The default is the 'current' controller. + HA systems may direct the query to the 'remote' controller by + including 'remote_controller=True'. The default is the 'current' controller. Each audit entry contains the following keys: @@ -204,19 +202,19 @@ async def query(self, data): # If HA, handle the possibility of remote controller requests ctrlr_state = await self.middleware.call('failover.status') - - # want_ctrlr: Default is 'current' controller else use selection - want_ctrlr = ctrlr_state if data['controller'] is None else ( - 'MASTER' if data['controller'] in ['MASTER', 'Active'] else 'BACKUP' - ) - if ctrlr_state != 'SINGLE': - if ctrlr_state not in ['MASTER', 'BACKUP']: - verrors.add('audit.query.controller', f'controller status: {ctrlr_state}') - verrors.check() + if ctrlr_state != 'SINGLE' and data['remote_controller']: + try: + if audit_query := self.middleware.call( + 'failover.call_remote', + 'audit.query', + [data], + {'raise_connect_error': False, 'timeout': 2, 'connect_timeout': 2} + ): + return audit_query + except Exception: + self.logger.exception('Unexpected failure querying remote node for audit entries') else: - if want_ctrlr != ctrlr_state: - # Get the data from the other controller - return await self.middleware.call('failover.call_remote', 'audit.query', [data]) + raise ValidationError('audit.query.remote_controller', 'Failed to query audit logs of remote controller') sql_filters = data['query-options']['force_sql_filters'] diff --git a/src/middlewared/middlewared/plugins/audit/utils.py b/src/middlewared/middlewared/plugins/audit/utils.py index 6791c2e0cbee..f63d96e0757b 100644 --- a/src/middlewared/middlewared/plugins/audit/utils.py +++ b/src/middlewared/middlewared/plugins/audit/utils.py @@ -24,7 +24,6 @@ AuditEventParam.EVENT.value, AuditEventParam.SUCCESS.value, ) -AUDIT_CONTROLLER_SELECTIONS = ['MASTER', 'Active', 'BACKUP', 'Standby'] AuditBase = declarative_base() diff --git a/tests/api2/test_audit_basic.py b/tests/api2/test_audit_basic.py index ab39a4e09cca..ac9690b06399 100644 --- a/tests/api2/test_audit_basic.py +++ b/tests/api2/test_audit_basic.py @@ -14,8 +14,6 @@ import secrets import string -ha_test = pytest.mark.skipif(not (ha and "virtual_ip" in os.environ), reason="Skip HA tests") - SMBUSER = 'audit-smb-user' PASSWD = ''.join(secrets.choice(string.ascii_letters + string.digits) for i in range(10)) @@ -321,7 +319,7 @@ def test_audit_timestamps(self, svc): assert abs(ae_ts_ts - ae_msg_ts) < 2, f"$date='{ae_ts_ts}, message_timestamp={ae_msg_ts}" -@ha_test +@pytest.mark.skipif(not ha, reason="Skip HA tests") class TestAuditOpsHA: def test_audit_ha_query(self, standby_user): name = standby_user @@ -336,7 +334,7 @@ def test_audit_ha_query(self, standby_user): remote_audit_entry = call('audit.query', { "query-filters": [["event_data.description", "$", name]], "query-options": {"select": ["event_data", "success"]}, - "controller": "Standby" + "remote_controller": True }) if remote_audit_entry != []: break @@ -354,8 +352,8 @@ def test_audit_ha_export(self, standby_user): audit DB from both controllers and confirm the user create is in the 'Standby' audit DB and not in the 'Active' audit DB. """ - report_path_active = call('audit.export', {'export_format': 'CSV', 'controller': 'MASTER'}, job=True) - report_path_standby = call('audit.export', {'export_format': 'CSV', 'controller': 'BACKUP'}, job=True) + report_path_active = call('audit.export', {'export_format': 'CSV'}, job=True) + report_path_standby = call('audit.export', {'export_format': 'CSV', 'remote_controller': True}, job=True) # Confirm entry NOT in active controller audit DB with pytest.raises(AssertionError): From 0fb20b947d6a9f66fb7418767eb55e47d69433f8 Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Wed, 11 Sep 2024 09:29:21 -0700 Subject: [PATCH 5/7] Change HA check from 'status' to 'licensed' --- src/middlewared/middlewared/plugins/audit/audit.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/middlewared/middlewared/plugins/audit/audit.py b/src/middlewared/middlewared/plugins/audit/audit.py index f749398a590c..dcdeb471c747 100644 --- a/src/middlewared/middlewared/plugins/audit/audit.py +++ b/src/middlewared/middlewared/plugins/audit/audit.py @@ -201,8 +201,7 @@ async def query(self, data): verrors = ValidationErrors() # If HA, handle the possibility of remote controller requests - ctrlr_state = await self.middleware.call('failover.status') - if ctrlr_state != 'SINGLE' and data['remote_controller']: + if await self.middleware.call('failover.licensed') and data['remote_controller']: try: if audit_query := self.middleware.call( 'failover.call_remote', From 273c889d591b3143847fe65d59e0c396bbc999aa Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Wed, 11 Sep 2024 15:57:59 -0700 Subject: [PATCH 6/7] Fix remote call. --- .../middlewared/plugins/audit/audit.py | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/middlewared/middlewared/plugins/audit/audit.py b/src/middlewared/middlewared/plugins/audit/audit.py index dcdeb471c747..5577ec1a254b 100644 --- a/src/middlewared/middlewared/plugins/audit/audit.py +++ b/src/middlewared/middlewared/plugins/audit/audit.py @@ -202,18 +202,29 @@ async def query(self, data): # If HA, handle the possibility of remote controller requests if await self.middleware.call('failover.licensed') and data['remote_controller']: + data.pop('remote_controller') try: - if audit_query := self.middleware.call( + audit_query = await self.middleware.call( 'failover.call_remote', 'audit.query', [data], {'raise_connect_error': False, 'timeout': 2, 'connect_timeout': 2} - ): - return audit_query + ) + return audit_query + except CallError as e: + if e.errno in [errno.ECONNABORTED, errno.ECONNREFUSED, errno.ECONNRESET, errno.EHOSTDOWN, + errno.ETIMEDOUT, CallError.EALERTCHECKERUNAVAILABLE]: + raise ValidationError( + 'audit.query.remote_controller', + 'Temporarily failed to communicate to remote controller' + ) + raise ValidationError( + 'audit.query.remote_controller', + 'Failed to query audit logs of remote controller' + ) except Exception: self.logger.exception('Unexpected failure querying remote node for audit entries') - else: - raise ValidationError('audit.query.remote_controller', 'Failed to query audit logs of remote controller') + raise sql_filters = data['query-options']['force_sql_filters'] From eac8c6b0d3519fc5d94b288def05e29e65ad029a Mon Sep 17 00:00:00 2001 From: Mark Grimes Date: Wed, 11 Sep 2024 17:12:30 -0700 Subject: [PATCH 7/7] Fixup remote calls. --- tests/api2/test_audit_basic.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/tests/api2/test_audit_basic.py b/tests/api2/test_audit_basic.py index ac9690b06399..d76a5b87c8a9 100644 --- a/tests/api2/test_audit_basic.py +++ b/tests/api2/test_audit_basic.py @@ -123,14 +123,17 @@ def standby_user(): user_id = None try: name = "StandbyUser" + PASSWD - user_id = call('failover.call_remote', 'user.create', [{ - "username": name, - "full_name": name + " Deleteme", - "group": 100, - "smb": False, - "home_create": False, - "password": "testing" - }]) + user_id = call( + 'failover.call_remote', 'user.create', [{ + "username": name, + "full_name": name + " Deleteme", + "group": 100, + "smb": False, + "home_create": False, + "password": "testing" + }], + {'raise_connect_error': False, 'timeout': 2, 'connect_timeout': 2} + ) yield name finally: if user_id is not None: @@ -323,7 +326,11 @@ def test_audit_timestamps(self, svc): class TestAuditOpsHA: def test_audit_ha_query(self, standby_user): name = standby_user - remote_user = call('failover.call_remote', 'user.query', [[["username", "=", name]]]) + remote_user = call( + 'failover.call_remote', 'user.query', + [[["username", "=", name]]], + {'raise_connect_error': False, 'timeout': 2, 'connect_timeout': 2} + ) assert remote_user != [] # Handle delays in the audit database