From afeddf58cb0d12a49dd87f4d32cd0b9b52f1b6cb Mon Sep 17 00:00:00 2001 From: charlotte Date: Thu, 3 Jul 2025 14:42:27 +0100 Subject: [PATCH 1/3] updates ready endpoint behaviour, adds testing --- packages/opal-client/opal_client/client.py | 8 +- .../opal-client/opal_client/data/updater.py | 10 ++ .../policy_store/base_policy_store_client.py | 5 +- .../policy_store/mock_policy_store_client.py | 8 +- .../opal_client/policy_store/opa_client.py | 43 +++++- .../opal_client/tests/client_test.py | 120 ++++++++++++++++ .../opal_client/tests/data_updater_test.py | 75 ++++++++++ .../opal_client/tests/opa_client_test.py | 136 ++++++++++++++++++ 8 files changed, 392 insertions(+), 13 deletions(-) create mode 100644 packages/opal-client/opal_client/tests/client_test.py diff --git a/packages/opal-client/opal_client/client.py b/packages/opal-client/opal_client/client.py index 7944f65d8..5dfda86f6 100644 --- a/packages/opal-client/opal_client/client.py +++ b/packages/opal-client/opal_client/client.py @@ -257,9 +257,9 @@ def _init_fast_api_app(self): self._configure_lifecycle_callbacks(app) return app - async def _is_ready(self): + async def _is_ready(self, wait_for_all_data_sources_loaded: bool = False): # Data loaded from file or from server - return self._backup_loaded or await self.policy_store.is_ready() + return self._backup_loaded or await self.policy_store.is_ready(wait_for_all_data_sources_loaded=wait_for_all_data_sources_loaded) def _configure_api_routes(self, app: FastAPI): """Mounts the api routes on the app object.""" @@ -307,9 +307,9 @@ async def healthy(): ) @app.get("/ready", include_in_schema=False) - async def ready(): + async def ready(wait_for_all_data_sources_loaded: bool = False): """Returns 200 if the policy store is ready to serve requests.""" - if await self._is_ready(): + if await self._is_ready(wait_for_all_data_sources_loaded=wait_for_all_data_sources_loaded): return JSONResponse( status_code=status.HTTP_200_OK, content={"status": "ok"} ) diff --git a/packages/opal-client/opal_client/data/updater.py b/packages/opal-client/opal_client/data/updater.py index 2df151423..ceedf39c0 100644 --- a/packages/opal-client/opal_client/data/updater.py +++ b/packages/opal-client/opal_client/data/updater.py @@ -268,6 +268,16 @@ async def get_base_policy_data( # Fetch the base config with all data entries sources_config = await self.get_policy_data_config(url=config_url) + # Set the expected data transaction count for readiness checking + # This count represents all data sources that will be loaded initially + total_expected_data_transactions = len(sources_config.entries) + if hasattr(self._policy_store, 'set_expected_data_transaction_count'): + await self._policy_store.set_expected_data_transaction_count(total_expected_data_transactions) + logger.info( + "Set expected data transaction count to {count}", + count=total_expected_data_transactions + ) + init_entries, periodic_entries = [], [] for entry in sources_config.entries: if entry.periodic_update_interval is not None: diff --git a/packages/opal-client/opal_client/policy_store/base_policy_store_client.py b/packages/opal-client/opal_client/policy_store/base_policy_store_client.py index d7c11b745..66ba1a405 100644 --- a/packages/opal-client/opal_client/policy_store/base_policy_store_client.py +++ b/packages/opal-client/opal_client/policy_store/base_policy_store_client.py @@ -67,9 +67,12 @@ async def log_transaction(self, transaction: StoreTransaction): async def is_healthy(self) -> bool: raise NotImplementedError() - async def is_ready(self) -> bool: + async def is_ready(self, wait_for_all_data_sources_loaded: bool = False) -> bool: raise NotImplementedError() + async def set_expected_data_transaction_count(self, count: int) -> None: + pass + async def full_export(self, writer: AsyncTextIOWrapper) -> None: raise NotImplementedError() diff --git a/packages/opal-client/opal_client/policy_store/mock_policy_store_client.py b/packages/opal-client/opal_client/policy_store/mock_policy_store_client.py index 549dd8435..27f6d6320 100644 --- a/packages/opal-client/opal_client/policy_store/mock_policy_store_client.py +++ b/packages/opal-client/opal_client/policy_store/mock_policy_store_client.py @@ -18,7 +18,7 @@ class MockPolicyStoreClient(BasePolicyStoreClient): def __init__(self) -> None: super().__init__() - self._has_data_event: asyncio.Event() = None + self._has_data_event: Optional[asyncio.Event] = None self._data = {} @property @@ -102,5 +102,11 @@ async def init_healthcheck_policy( async def log_transaction(self, transaction: StoreTransaction): pass + async def is_ready(self, wait_for_all_data_sources_loaded: bool = False) -> bool: + return self.has_data_event.is_set() + async def is_healthy(self) -> bool: return self.has_data_event.is_set() + + async def set_expected_data_transaction_count(self, count: int) -> None: + pass diff --git a/packages/opal-client/opal_client/policy_store/opa_client.py b/packages/opal-client/opal_client/policy_store/opa_client.py index 86caa5d72..b86357bd9 100644 --- a/packages/opal-client/opal_client/policy_store/opa_client.py +++ b/packages/opal-client/opal_client/policy_store/opa_client.py @@ -107,16 +107,27 @@ def __init__( self._num_failed_policy_transactions = 0 self._num_successful_data_transactions = 0 self._num_failed_data_transactions = 0 + self._num_expected_data_transactions = 0 # Track total expected data transactions self._last_policy_transaction: Optional[StoreTransaction] = None self._last_failed_policy_transaction: Optional[StoreTransaction] = None self._last_data_transaction: Optional[StoreTransaction] = None self._last_failed_data_transaction: Optional[StoreTransaction] = None @property - def ready(self) -> bool: - is_ready: bool = self._num_successful_policy_transactions > 0 and ( - self._data_updater_disabled or self._num_successful_data_transactions > 0 - ) + def ready(self, wait_for_all_data_sources_loaded: bool = False) -> bool: + if wait_for_all_data_sources_loaded: + # require all expected data transactions to be successful + is_ready: bool = self._num_successful_policy_transactions > 0 and ( + self._data_updater_disabled or + self._num_successful_data_transactions == self._num_expected_data_transactions + ) + else: + # Default behavior + is_ready: bool = self._num_successful_policy_transactions > 0 and ( + self._data_updater_disabled or + self._num_successful_data_transactions > 0 or + self._num_expected_data_transactions == 0 + ) return is_ready @property @@ -188,6 +199,15 @@ def _is_policy_transaction(self, transaction: StoreTransaction): def _is_data_transaction(self, transaction: StoreTransaction): return transaction.transaction_type == TransactionType.data + def set_expected_data_transaction_count(self, count: int) -> None: + """Set the expected number of data transactions for readiness checking. + Args: + count (int): The total number of data transactions expected to be processed + before the policy store can be considered fully ready (determined + by data sources config). + """ + self._num_expected_data_transactions = count + def process_transaction(self, transaction: StoreTransaction): """Mutates the state into a new state that can be then persisted as hardcoded policy.""" @@ -440,7 +460,7 @@ async def _get_oauth_token(self): logger.warning("OAuth server connection error: {err}", err=repr(e)) raise - async def _get_auth_headers(self) -> {}: + async def _get_auth_headers(self) -> dict: headers = {} if self._auth_type == PolicyStoreAuth.TOKEN: if self._token is not None: @@ -945,12 +965,21 @@ async def log_transaction(self, transaction: StoreTransaction): data=transaction_data, ) - async def is_ready(self) -> bool: - return self._transaction_state.ready + async def is_ready(self, wait_for_all_data_sources_loaded: bool = False) -> bool: + return self._transaction_state.ready(wait_for_all_data_sources_loaded=wait_for_all_data_sources_loaded) async def is_healthy(self) -> bool: return self._transaction_state.healthy + async def set_expected_data_transaction_count(self, count: int) -> None: + """Set the expected number of data transactions for readiness checking. + Args: + count (int): The total number of data transactions expected to be processed + before the policy store can be considered fully ready (determined + by the data sources config). + """ + self._transaction_state.set_expected_data_transaction_count(count) + async def full_export(self, writer: AsyncTextIOWrapper) -> None: policies = await self.get_policies() data = self._policy_data_cache.get_data() diff --git a/packages/opal-client/opal_client/tests/client_test.py b/packages/opal-client/opal_client/tests/client_test.py new file mode 100644 index 000000000..b513edf90 --- /dev/null +++ b/packages/opal-client/opal_client/tests/client_test.py @@ -0,0 +1,120 @@ +import pytest +from unittest.mock import patch +from fastapi import status +from fastapi.testclient import TestClient + +from opal_client.client import OpalClient +from opal_client.policy_store.base_policy_store_client import BasePolicyStoreClient +from opal_client.config import PolicyStoreTypes + + +class MockPolicyStoreClient(BasePolicyStoreClient): + """Mock policy store client for testing.""" + + def __init__(self, is_ready_value=True, is_healthy_value=True): + self._is_ready_value = is_ready_value + self._is_healthy_value = is_healthy_value + + async def is_ready(self, wait_for_all_data_sources_loaded: bool = False) -> bool: + return self._is_ready_value + + async def is_healthy(self) -> bool: + return self._is_healthy_value + + +@pytest.fixture +def mock_policy_store_ready(): + """Mock policy store that is ready.""" + return MockPolicyStoreClient(is_ready_value=True, is_healthy_value=True) + + +@pytest.fixture +def mock_policy_store_not_ready(): + """Mock policy store that is not ready.""" + return MockPolicyStoreClient(is_ready_value=False, is_healthy_value=False) + + +@pytest.fixture +def opal_client_ready(mock_policy_store_ready): + """Create an OpalClient with a ready policy store.""" + with patch('opal_client.client.PolicyStoreClientFactory.create') as mock_factory: + mock_factory.return_value = mock_policy_store_ready + + client = OpalClient( + policy_store_type=PolicyStoreTypes.OPA, + policy_store=mock_policy_store_ready, + data_updater=False, # Disable data updater + policy_updater=False, # Disable policy updater + inline_opa_enabled=False, + inline_cedar_enabled=False, + ) + yield client + + +@pytest.fixture +def opal_client_not_ready(mock_policy_store_not_ready): + """Create an OpalClient with a policy store that is not ready.""" + with patch('opal_client.client.PolicyStoreClientFactory.create') as mock_factory: + mock_factory.return_value = mock_policy_store_not_ready + + client = OpalClient( + policy_store_type=PolicyStoreTypes.OPA, + policy_store=mock_policy_store_not_ready, + data_updater=False, # Disable data updater + policy_updater=False, # Disable policy updater + inline_opa_enabled=False, + inline_cedar_enabled=False, + ) + yield client + + +@pytest.fixture +def test_client_ready(opal_client_ready): + """Create a FastAPI test client with a ready OPAL client.""" + return TestClient(opal_client_ready.app) + + +@pytest.fixture +def test_client_not_ready(opal_client_not_ready): + """Create a FastAPI test client with a not ready OPAL client.""" + return TestClient(opal_client_not_ready.app) + + +def test_ready_endpoint_when_ready(test_client_ready, opal_client_ready): + """Test that /ready endpoint returns 200 when policy store is ready and _is_ready is called with correct arguments.""" + with patch.object(opal_client_ready, '_is_ready', return_value=True) as mock_is_ready: + response = test_client_ready.get("/ready") + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"status": "ok"} + mock_is_ready.assert_called_once_with(wait_for_all_data_sources_loaded=False) + + +def test_ready_endpoint_when_not_ready(test_client_not_ready, opal_client_not_ready): + """Test that /ready endpoint returns 503 when policy store is not ready and _is_ready is called with correct arguments.""" + with patch.object(opal_client_not_ready, '_is_ready', return_value=False) as mock_is_ready: + response = test_client_not_ready.get("/ready") + + assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE + assert response.json() == {"status": "unavailable"} + mock_is_ready.assert_called_once_with(wait_for_all_data_sources_loaded=False) + + +def test_ready_endpoint_with_query_parameter_true(test_client_ready, opal_client_ready): + """Test that /ready endpoint works with wait_for_all_data_sources_loaded parameter and _is_ready is called with correct arguments.""" + with patch.object(opal_client_ready, '_is_ready', return_value=True) as mock_is_ready: + response = test_client_ready.get("/ready?wait_for_all_data_sources_loaded=true") + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"status": "ok"} + mock_is_ready.assert_called_once_with(wait_for_all_data_sources_loaded=True) + + +def test_ready_endpoint_with_query_parameter_false(test_client_ready, opal_client_ready): + """Test that /ready endpoint works with wait_for_all_data_sources_loaded=false and _is_ready is called with correct arguments.""" + with patch.object(opal_client_ready, '_is_ready', return_value=True) as mock_is_ready: + response = test_client_ready.get("/ready?wait_for_all_data_sources_loaded=false") + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"status": "ok"} + mock_is_ready.assert_called_once_with(wait_for_all_data_sources_loaded=False) \ No newline at end of file diff --git a/packages/opal-client/opal_client/tests/data_updater_test.py b/packages/opal-client/opal_client/tests/data_updater_test.py index cad760a10..631496119 100644 --- a/packages/opal-client/opal_client/tests/data_updater_test.py +++ b/packages/opal-client/opal_client/tests/data_updater_test.py @@ -26,6 +26,7 @@ PolicyStoreClientFactory, ) from opal_client.policy_store.schemas import PolicyStoreTypes +from opal_client.policy_store.opa_client import OpaClient from opal_common.schemas.data import ( DataSourceConfig, DataUpdateReport, @@ -327,3 +328,77 @@ async def test_client_get_initial_data(server): # cleanup finally: await updater.stop() + + +@pytest.mark.asyncio +async def test_data_updater_sets_expected_data_transaction_count(server): + """Test that DataUpdater sets the expected data transaction count correctly when fetching initial data. + + This test verifies that: + 1. The DataUpdater correctly calls set_expected_data_transaction_count() on the OPA policy store + 2. The _num_expected_data_transactions property in OpaTransactionLogState is set to the correct value + 3. The count equals the number of data source entries in the config + 5. The count is updated when the configuration changes + """ + # config to use OPA client (not mock) to test the OpaTransactionLogState + policy_store = PolicyStoreClientFactory.create( + store_type=PolicyStoreTypes.OPA, + url="http://localhost:8181", # dummy URL + data_updater_enabled=True, + policy_updater_enabled=True, + ) + + assert isinstance(policy_store, OpaClient) + + updater = DataUpdater( + data_sources_config_url=DATA_CONFIG_URL, + policy_store=policy_store, + fetch_on_connect=False, # Don't fetch on connect + data_topics=DATA_TOPICS, + should_send_reports=False, + ) + + # Verify initial state + initial_count = policy_store._transaction_state._num_expected_data_transactions + assert initial_count == 0, f"Expected initial count to be 0 but got {initial_count}" + + # Mock the get_policy_data_config method to return a DataSourceConfig directly + # This avoids the actual fetching which would fail due to network setup + test_config = DataSourceConfig(entries=[ + {"url": DATA_URL, "topics": DATA_TOPICS}, + ]) + + import unittest.mock + with unittest.mock.patch.object(updater, 'get_policy_data_config', return_value=test_config): + # Mock the trigger_data_update method to avoid actual data fetching + with unittest.mock.patch.object(updater, 'trigger_data_update') as mock_trigger: + await updater.get_base_policy_data() + + mock_trigger.assert_called_once() + + # Verify that the expected data transaction count was set correctly + expected_count = len(test_config.entries) + actual_count = policy_store._transaction_state._num_expected_data_transactions + + assert actual_count == expected_count, f"Expected {expected_count} but got {actual_count}" + + # Test with multiple data sources to ensure count is set correctly + multi_source_config = DataSourceConfig(entries=[ + {"url": DATA_URL, "topics": DATA_TOPICS}, + {"url": DATA_URL, "topics": DATA_TOPICS}, + {"url": DATA_URL, "topics": DATA_TOPICS} + ]) + + # Mock the get_policy_data_config method to return our multi-source config + with unittest.mock.patch.object(updater, 'get_policy_data_config', return_value=multi_source_config): + # Mock the trigger_data_update method to avoid actual data fetching + with unittest.mock.patch.object(updater, 'trigger_data_update') as mock_trigger: + await updater.get_base_policy_data() + + mock_trigger.assert_called_once() + + # Verify that the expected count was updated to match the number of entries + expected_multi_count = len(multi_source_config.entries) + actual_multi_count = policy_store._transaction_state._num_expected_data_transactions + + assert actual_multi_count == expected_multi_count, f"Expected {expected_multi_count} but got {actual_multi_count}" diff --git a/packages/opal-client/opal_client/tests/opa_client_test.py b/packages/opal-client/opal_client/tests/opa_client_test.py index fe15cb7f3..d844e6fb5 100644 --- a/packages/opal-client/opal_client/tests/opa_client_test.py +++ b/packages/opal-client/opal_client/tests/opa_client_test.py @@ -5,7 +5,9 @@ import pytest from fastapi import Response, status from opal_client.policy_store.opa_client import OpaClient, should_ignore_path +from opal_client.policy_store.opa_client import OpaTransactionLogState from opal_client.policy_store.schemas import PolicyStoreAuth +from opal_common.schemas.store import StoreTransaction, TransactionType TEST_CA_CERT = """-----BEGIN CERTIFICATE----- MIIBdjCCAR2gAwIBAgIUaQ/M1qL0GzsTMChEAJsLLFgz7a4wCgYIKoZIzj0EAwIw @@ -227,3 +229,137 @@ def test_should_ignore_path_keeping_higher_priority_to_ones_defined_as_not_to_ig ) assert should_ignore_path("otherFolder", ignore_paths) == True assert should_ignore_path("otherFolder/file.txt", ignore_paths) == True + + +def test_opa_transaction_log_state_ready_with_wait_for_all_data_sources_loaded_false(): + """Test OpaTransactionLogState ready behavior with wait_for_all_data_sources_loaded=False. + + When wait_for_all_data_sources_loaded=False (default behavior): + - Ready if there's at least one successful policy transaction AND + - (data updater is disabled OR at least one successful data transaction OR no data transactions expected) + """ + # Test initial state - should not be ready + state = OpaTransactionLogState(data_updater_enabled=True, policy_updater_enabled=True) + assert state.is_ready(wait_for_all_data_sources_loaded=False) == False + + # Add successful policy transaction + policy_transaction = StoreTransaction( + id="policy-123", + actions=["set_policy"], + transaction_type=TransactionType.policy, + success=True, + creation_time="2025-01-01T00:00:00", + end_time="2025-01-01T00:00:01" + ) + state.process_transaction(policy_transaction) + + # Should not be ready yet because we need at least one data transaction or none expected + assert state.is_ready(wait_for_all_data_sources_loaded=False) == False + + # Add successful data transaction + data_transaction = StoreTransaction( + id="data-123", + actions=["set_policy_data"], + transaction_type=TransactionType.data, + success=True, + creation_time="2025-01-01T00:00:00", + end_time="2025-01-01T00:00:01" + ) + state.process_transaction(data_transaction) + + # Should be ready now + assert state.is_ready(wait_for_all_data_sources_loaded=False) == True + + #### Test case where no data transactions are expected #### + state_no_data = OpaTransactionLogState(data_updater_enabled=True, policy_updater_enabled=True) + state_no_data.set_expected_data_transaction_count(0) + state_no_data.process_transaction(policy_transaction) + + # Should be ready because no data transactions are expected + assert state_no_data.is_ready(wait_for_all_data_sources_loaded=False) == True + + #### Test case where data updater is disabled #### + state_data_disabled = OpaTransactionLogState(data_updater_enabled=False, policy_updater_enabled=True) + state_data_disabled.process_transaction(policy_transaction) + + # Should be ready because data updater is disabled + assert state_data_disabled.is_ready(wait_for_all_data_sources_loaded=False) == True + + +def test_opa_transaction_log_state_ready_with_wait_for_all_data_sources_loaded_true(): + """Test OpaTransactionLogState ready behavior with wait_for_all_data_sources_loaded=True. + + When wait_for_all_data_sources_loaded=True: + - Ready if there's at least one successful policy transaction AND + - (data updater is disabled OR successful data transactions == expected data transactions) + """ + # Test initial state - should not be ready + state = OpaTransactionLogState(data_updater_enabled=True, policy_updater_enabled=True) + state.set_expected_data_transaction_count(2) # Expect 2 data transactions + assert state.is_ready(wait_for_all_data_sources_loaded=True) == False + + # Add successful policy transaction + policy_transaction = StoreTransaction( + id="policy-123", + actions=["set_policy"], + transaction_type=TransactionType.policy, + success=True, + creation_time="2025-01-01T00:00:00", + end_time="2025-01-01T00:00:01" + ) + state.process_transaction(policy_transaction) + + assert state.is_ready(wait_for_all_data_sources_loaded=True) == False + + # Add one successful data transaction + data_transaction_1 = StoreTransaction( + id="data-123", + actions=["set_policy_data"], + transaction_type=TransactionType.data, + success=True, + creation_time="2025-01-01T00:00:00", + end_time="2025-01-01T00:00:01" + ) + state.process_transaction(data_transaction_1) + + assert state.is_ready(wait_for_all_data_sources_loaded=True) == False + + # Add second successful data transaction + data_transaction_2 = StoreTransaction( + id="data-456", + actions=["set_policy_data"], + transaction_type=TransactionType.data, + success=True, + creation_time="2025-01-01T00:00:00", + end_time="2025-01-01T00:00:01" + ) + state.process_transaction(data_transaction_2) + + assert state.is_ready(wait_for_all_data_sources_loaded=True) == True + + #### Test case where data updater is disabled #### + state_data_disabled = OpaTransactionLogState(data_updater_enabled=False, policy_updater_enabled=True) + state_data_disabled.set_expected_data_transaction_count(5) + # Add successful policy transaction + state_data_disabled.process_transaction(policy_transaction) + + assert state_data_disabled.is_ready(wait_for_all_data_sources_loaded=True) == True + + #### Test failed data transaction - should not count towards successful count #### + failed_data_transaction = StoreTransaction( + id="data-failed", + actions=["set_policy_data"], + transaction_type=TransactionType.data, + success=False, + error="Test error", + creation_time="2025-01-01T00:00:00", + end_time="2025-01-01T00:00:01" + ) + + state_with_failure = OpaTransactionLogState(data_updater_enabled=True, policy_updater_enabled=True) + state_with_failure.set_expected_data_transaction_count(1) # Expect 1 successful transaction + state_with_failure.process_transaction(policy_transaction) + state_with_failure.process_transaction(failed_data_transaction) + + # Should not be ready because failed transaction doesn't count as successful + assert state_with_failure.is_ready(wait_for_all_data_sources_loaded=True) == False From 5d02fae076a682b85b1ca2d7bf4d2f52336bcb51 Mon Sep 17 00:00:00 2001 From: charlotte Date: Thu, 3 Jul 2025 16:49:07 +0100 Subject: [PATCH 2/3] test fix --- .../opal_client/policy_store/opa_client.py | 17 ++++++----- .../opal_client/tests/client_test.py | 30 +++++++++++++------ .../opal_client/tests/data_updater_test.py | 22 +++++++++----- 3 files changed, 45 insertions(+), 24 deletions(-) diff --git a/packages/opal-client/opal_client/policy_store/opa_client.py b/packages/opal-client/opal_client/policy_store/opa_client.py index b86357bd9..eea1c40f4 100644 --- a/packages/opal-client/opal_client/policy_store/opa_client.py +++ b/packages/opal-client/opal_client/policy_store/opa_client.py @@ -107,26 +107,27 @@ def __init__( self._num_failed_policy_transactions = 0 self._num_successful_data_transactions = 0 self._num_failed_data_transactions = 0 - self._num_expected_data_transactions = 0 # Track total expected data transactions + self._num_expected_data_transactions = None # Track total expected data transactions (None = not set) self._last_policy_transaction: Optional[StoreTransaction] = None self._last_failed_policy_transaction: Optional[StoreTransaction] = None self._last_data_transaction: Optional[StoreTransaction] = None self._last_failed_data_transaction: Optional[StoreTransaction] = None - @property - def ready(self, wait_for_all_data_sources_loaded: bool = False) -> bool: + def is_ready(self, wait_for_all_data_sources_loaded: bool = False) -> bool: if wait_for_all_data_sources_loaded: # require all expected data transactions to be successful is_ready: bool = self._num_successful_policy_transactions > 0 and ( self._data_updater_disabled or - self._num_successful_data_transactions == self._num_expected_data_transactions + (self._num_expected_data_transactions is not None and + self._num_successful_data_transactions == self._num_expected_data_transactions) ) else: # Default behavior is_ready: bool = self._num_successful_policy_transactions > 0 and ( self._data_updater_disabled or self._num_successful_data_transactions > 0 or - self._num_expected_data_transactions == 0 + (self._num_expected_data_transactions is not None and + self._num_expected_data_transactions == 0) ) return is_ready @@ -253,7 +254,7 @@ async def persist(self, state: OpaTransactionLogState): OPA.""" logger.info( "persisting health check policy: ready={ready}, healthy={healthy}", - ready=state.ready, + ready=state.is_ready(), healthy=state.healthy, ) logger.info( @@ -265,7 +266,7 @@ async def persist(self, state: OpaTransactionLogState): ) policy_code = self._format_with_json( self._policy_template, - ready=state.ready, + ready=state.is_ready(), healthy=state.healthy, last_policy_transaction=state.last_policy_transaction, last_failed_policy_transaction=state.last_failed_policy_transaction, @@ -966,7 +967,7 @@ async def log_transaction(self, transaction: StoreTransaction): ) async def is_ready(self, wait_for_all_data_sources_loaded: bool = False) -> bool: - return self._transaction_state.ready(wait_for_all_data_sources_loaded=wait_for_all_data_sources_loaded) + return self._transaction_state.is_ready(wait_for_all_data_sources_loaded=wait_for_all_data_sources_loaded) async def is_healthy(self) -> bool: return self._transaction_state.healthy diff --git a/packages/opal-client/opal_client/tests/client_test.py b/packages/opal-client/opal_client/tests/client_test.py index b513edf90..47f7fdc85 100644 --- a/packages/opal-client/opal_client/tests/client_test.py +++ b/packages/opal-client/opal_client/tests/client_test.py @@ -37,7 +37,7 @@ def mock_policy_store_not_ready(): @pytest.fixture def opal_client_ready(mock_policy_store_ready): """Create an OpalClient with a ready policy store.""" - with patch('opal_client.client.PolicyStoreClientFactory.create') as mock_factory: + with patch("opal_client.client.PolicyStoreClientFactory.create") as mock_factory: mock_factory.return_value = mock_policy_store_ready client = OpalClient( @@ -54,7 +54,7 @@ def opal_client_ready(mock_policy_store_ready): @pytest.fixture def opal_client_not_ready(mock_policy_store_not_ready): """Create an OpalClient with a policy store that is not ready.""" - with patch('opal_client.client.PolicyStoreClientFactory.create') as mock_factory: + with patch("opal_client.client.PolicyStoreClientFactory.create") as mock_factory: mock_factory.return_value = mock_policy_store_not_ready client = OpalClient( @@ -82,7 +82,9 @@ def test_client_not_ready(opal_client_not_ready): def test_ready_endpoint_when_ready(test_client_ready, opal_client_ready): """Test that /ready endpoint returns 200 when policy store is ready and _is_ready is called with correct arguments.""" - with patch.object(opal_client_ready, '_is_ready', return_value=True) as mock_is_ready: + with patch.object( + opal_client_ready, "_is_ready", return_value=True + ) as mock_is_ready: response = test_client_ready.get("/ready") assert response.status_code == status.HTTP_200_OK @@ -92,7 +94,9 @@ def test_ready_endpoint_when_ready(test_client_ready, opal_client_ready): def test_ready_endpoint_when_not_ready(test_client_not_ready, opal_client_not_ready): """Test that /ready endpoint returns 503 when policy store is not ready and _is_ready is called with correct arguments.""" - with patch.object(opal_client_not_ready, '_is_ready', return_value=False) as mock_is_ready: + with patch.object( + opal_client_not_ready, "_is_ready", return_value=False + ) as mock_is_ready: response = test_client_not_ready.get("/ready") assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE @@ -102,7 +106,9 @@ def test_ready_endpoint_when_not_ready(test_client_not_ready, opal_client_not_re def test_ready_endpoint_with_query_parameter_true(test_client_ready, opal_client_ready): """Test that /ready endpoint works with wait_for_all_data_sources_loaded parameter and _is_ready is called with correct arguments.""" - with patch.object(opal_client_ready, '_is_ready', return_value=True) as mock_is_ready: + with patch.object( + opal_client_ready, "_is_ready", return_value=True + ) as mock_is_ready: response = test_client_ready.get("/ready?wait_for_all_data_sources_loaded=true") assert response.status_code == status.HTTP_200_OK @@ -110,11 +116,17 @@ def test_ready_endpoint_with_query_parameter_true(test_client_ready, opal_client mock_is_ready.assert_called_once_with(wait_for_all_data_sources_loaded=True) -def test_ready_endpoint_with_query_parameter_false(test_client_ready, opal_client_ready): +def test_ready_endpoint_with_query_parameter_false( + test_client_ready, opal_client_ready +): """Test that /ready endpoint works with wait_for_all_data_sources_loaded=false and _is_ready is called with correct arguments.""" - with patch.object(opal_client_ready, '_is_ready', return_value=True) as mock_is_ready: - response = test_client_ready.get("/ready?wait_for_all_data_sources_loaded=false") + with patch.object( + opal_client_ready, "_is_ready", return_value=True + ) as mock_is_ready: + response = test_client_ready.get( + "/ready?wait_for_all_data_sources_loaded=false" + ) assert response.status_code == status.HTTP_200_OK assert response.json() == {"status": "ok"} - mock_is_ready.assert_called_once_with(wait_for_all_data_sources_loaded=False) \ No newline at end of file + mock_is_ready.assert_called_once_with(wait_for_all_data_sources_loaded=False) diff --git a/packages/opal-client/opal_client/tests/data_updater_test.py b/packages/opal-client/opal_client/tests/data_updater_test.py index 631496119..349578883 100644 --- a/packages/opal-client/opal_client/tests/data_updater_test.py +++ b/packages/opal-client/opal_client/tests/data_updater_test.py @@ -60,7 +60,7 @@ ) DATA_UPDATE_ROUTE = f"http://localhost:{PORT}/data/config" -PATCH_DATA_UPDATE = [JSONPatchAction(op="add", path="/", value=TEST_DATA)] +PATCH_DATA_UPDATE = [JSONPatchAction(op="add", path="/patch_data", value={"patched": True})] def setup_server(event): @@ -83,14 +83,16 @@ def fetchable_data(): # route to report complition to @server_app.post(DATA_UPDATE_CALLBACK_ROUTE) def callback(report: DataUpdateReport): - if len(callbacks) == 1: - assert report.reports[0].hash == DataUpdater.calc_hash(PATCH_DATA_UPDATE) - else: + if len(callbacks) == 0: + # First callback should be from trigger_update (TEST_DATA) assert report.reports[0].hash == DataUpdater.calc_hash(TEST_DATA) + elif len(callbacks) == 1: + # Second callback should be from trigger_update_patch (PATCH_DATA_UPDATE) + assert report.reports[0].hash == DataUpdater.calc_hash(PATCH_DATA_UPDATE) callbacks.append(report) return "OKAY" - # route to report complition to + # route to report completion to @server_app.get(CHECK_DATA_UPDATE_CALLBACK_ROUTE) def check() -> int: return len(callbacks) @@ -267,8 +269,10 @@ async def test_data_updater_with_report_callback(server): res = await session.get(CHECK_DATA_UPDATE_CALLBACK_URL) current_callback_count = await res.json() + proc = None proc2 = None try: + # First update proc = multiprocessing.Process(target=trigger_update, daemon=True) proc.start() # wait until new data arrives into the store via the updater @@ -286,6 +290,9 @@ async def test_data_updater_with_report_callback(server): res = await session.get(CHECK_DATA_UPDATE_CALLBACK_URL) current_callback_count = await res.json() + # Second update + # Reset the event so we can wait for the second update + policy_store.has_data_event.clear() proc2 = multiprocessing.Process(target=trigger_update_patch, daemon=True) proc2.start() # wait until new data arrives into the store via the updater @@ -302,7 +309,8 @@ async def test_data_updater_with_report_callback(server): # cleanup finally: await updater.stop() - proc.terminate() + if proc: + proc.terminate() if proc2: proc2.terminate() @@ -360,7 +368,7 @@ async def test_data_updater_sets_expected_data_transaction_count(server): # Verify initial state initial_count = policy_store._transaction_state._num_expected_data_transactions - assert initial_count == 0, f"Expected initial count to be 0 but got {initial_count}" + assert initial_count == None, f"Expected initial count to be None but got {initial_count}" # Mock the get_policy_data_config method to return a DataSourceConfig directly # This avoids the actual fetching which would fail due to network setup From 9dc2e8ceacb0c784694d68b8f03ae16ce141523e Mon Sep 17 00:00:00 2001 From: charlotte Date: Thu, 3 Jul 2025 16:50:29 +0100 Subject: [PATCH 3/3] misc --- .../opal_client/tests/data_updater_test.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/opal-client/opal_client/tests/data_updater_test.py b/packages/opal-client/opal_client/tests/data_updater_test.py index 349578883..f3ebed01e 100644 --- a/packages/opal-client/opal_client/tests/data_updater_test.py +++ b/packages/opal-client/opal_client/tests/data_updater_test.py @@ -60,7 +60,7 @@ ) DATA_UPDATE_ROUTE = f"http://localhost:{PORT}/data/config" -PATCH_DATA_UPDATE = [JSONPatchAction(op="add", path="/patch_data", value={"patched": True})] +PATCH_DATA_UPDATE = [JSONPatchAction(op="add", path="/", value=TEST_DATA)] def setup_server(event): @@ -83,12 +83,10 @@ def fetchable_data(): # route to report complition to @server_app.post(DATA_UPDATE_CALLBACK_ROUTE) def callback(report: DataUpdateReport): - if len(callbacks) == 0: - # First callback should be from trigger_update (TEST_DATA) - assert report.reports[0].hash == DataUpdater.calc_hash(TEST_DATA) - elif len(callbacks) == 1: - # Second callback should be from trigger_update_patch (PATCH_DATA_UPDATE) + if len(callbacks) == 1: assert report.reports[0].hash == DataUpdater.calc_hash(PATCH_DATA_UPDATE) + else: + assert report.reports[0].hash == DataUpdater.calc_hash(TEST_DATA) callbacks.append(report) return "OKAY" @@ -272,7 +270,6 @@ async def test_data_updater_with_report_callback(server): proc = None proc2 = None try: - # First update proc = multiprocessing.Process(target=trigger_update, daemon=True) proc.start() # wait until new data arrives into the store via the updater @@ -290,9 +287,6 @@ async def test_data_updater_with_report_callback(server): res = await session.get(CHECK_DATA_UPDATE_CALLBACK_URL) current_callback_count = await res.json() - # Second update - # Reset the event so we can wait for the second update - policy_store.has_data_event.clear() proc2 = multiprocessing.Process(target=trigger_update_patch, daemon=True) proc2.start() # wait until new data arrives into the store via the updater