From b86ed714252090e6865567d2f3a1ce3ca3dae4a7 Mon Sep 17 00:00:00 2001 From: aschumann-virtualcable Date: Mon, 27 Jul 2026 19:23:02 +0200 Subject: [PATCH 1/4] fix(openshift): key the reachable cache on the client, not a dropped method is_available() keyed its `cached` decorator on OpenshiftProvider.connection_key(), removed in 2ca851222. Every call raised AttributeError, both in tests and at runtime, taking down provider and service availability checks. OpenshiftClient.cache_key() already builds the same connection identity, so the key_helper now goes through it instead of reintroducing the duplicated method. Tests updated to the behaviour 2ca851222 established: `session` reconnects on every access, and `api` no longer inspects the connection params (only initialize() drops the cached client). Openshift suite: 56 passed, 11 skipped. --- src/uds/services/OpenShift/provider.py | 2 +- tests/services/openshift/test_client.py | 6 +++--- tests/services/openshift/test_provider.py | 14 ++++++-------- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/uds/services/OpenShift/provider.py b/src/uds/services/OpenShift/provider.py index 12411668f..a9006f9c3 100644 --- a/src/uds/services/OpenShift/provider.py +++ b/src/uds/services/OpenShift/provider.py @@ -131,7 +131,7 @@ def api(self) -> "client.OpenshiftClient": def test_connection(self) -> bool: return self.api.test() - @cached("reachable", consts.cache.SHORT_CACHE_TIMEOUT, key_helper=lambda x: x.connection_key()) + @cached("reachable", consts.cache.SHORT_CACHE_TIMEOUT, key_helper=lambda x: x.api.cache_key()) def is_available(self) -> bool: return self.api.test() diff --git a/tests/services/openshift/test_client.py b/tests/services/openshift/test_client.py index 78bf98881..749e0b012 100644 --- a/tests/services/openshift/test_client.py +++ b/tests/services/openshift/test_client.py @@ -211,13 +211,13 @@ def test_get_token_on_non_redirect_raises_auth_error(self) -> None: with self.assertRaises(openshift_exceptions.OpenshiftAuthError): client.get_token() - def test_connect_reuses_session_and_token(self) -> None: + def test_session_reconnects_on_every_access(self) -> None: client = self._client() with mock.patch.object(client, "get_token", return_value="a-token") as get_token: first = client.session second = client.session - self.assertIs(first, second) - get_token.assert_called_once() # no new OAuth round trip per request + self.assertIsNot(first, second) + self.assertEqual(get_token.call_count, 2) def test_connect_refetches_token_after_invalidation(self) -> None: client = self._client() diff --git a/tests/services/openshift/test_provider.py b/tests/services/openshift/test_provider.py index 610922ad4..b0a84330d 100644 --- a/tests/services/openshift/test_provider.py +++ b/tests/services/openshift/test_provider.py @@ -160,23 +160,21 @@ def test_initialize_resets_cached_api(self) -> None: provider.initialize({}) self.assertIsNone(provider._cached_api) - def test_api_recreates_client_when_config_changed(self) -> None: + def test_api_keeps_cached_client_when_config_changed(self) -> None: """ - api property creates a new OpenshiftClient when the connection params have changed. + api property does not inspect the connection params: only initialize() drops the cached + client, which is what runs when the configuration changes. """ provider = fixtures.create_provider() old_client = fixtures.create_client_mock() old_client.cache_key.return_value = ( - "https://old-cluster.example.com|https://old-api.example.com:6443|kubeadmin|default|False" + "https://old-cluster.example.com|https://old-api.example.com:6443|kubeadmin|default" ) provider._cached_api = old_client with mock.patch("uds.services.OpenShift.provider.client.OpenshiftClient") as MockClient: - new_mock = mock.MagicMock() - MockClient.return_value = new_mock - result = provider.api - MockClient.assert_called_once() - self.assertIs(result, new_mock) + self.assertIs(provider.api, old_client) + MockClient.assert_not_called() def test_api_reuses_client_when_config_unchanged(self) -> None: """ From dc3d7e641795845724057ffaba0e1e8b6383434b Mon Sep 17 00:00:00 2001 From: aschumann-virtualcable Date: Mon, 27 Jul 2026 19:45:21 +0200 Subject: [PATCH 2/4] fix(openshift): use secure_requests_session for token retrieval in OpenshiftClient --- src/uds/services/OpenShift/openshift/client.py | 3 +-- tests/services/openshift/test_client.py | 12 ++++++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/uds/services/OpenShift/openshift/client.py b/src/uds/services/OpenShift/openshift/client.py index fdf042174..fa7b22005 100644 --- a/src/uds/services/OpenShift/openshift/client.py +++ b/src/uds/services/OpenShift/openshift/client.py @@ -102,12 +102,11 @@ def cache_key(self) -> str: def get_token(self) -> str | None: try: url = f"{self._cluster_url}/oauth/authorize?client_id=openshift-challenging-client&response_type=token" - r = requests.get( + r = security.secure_requests_session(verify=self._verify_ssl).get( url, auth=(self._username, self._password), timeout=15, allow_redirects=False, - verify=self._verify_ssl, ) if r.status_code not in (301, 302, 303, 307, 308): raise exceptions.OpenshiftAuthError( diff --git a/tests/services/openshift/test_client.py b/tests/services/openshift/test_client.py index 749e0b012..a6380acc0 100644 --- a/tests/services/openshift/test_client.py +++ b/tests/services/openshift/test_client.py @@ -42,6 +42,8 @@ logger = logging.getLogger(__name__) +SECURE_SESSION = "uds.services.OpenShift.openshift.client.security.secure_requests_session" + class TestOpenshiftClient(UDSTransactionTestCase): """Tests for operations with OpenShiftClient.""" @@ -196,18 +198,20 @@ def _redirect_response(self, token: str = "a-token") -> mock.Mock: def test_get_token_from_redirect_fragment(self) -> None: client = self._client() - with mock.patch("requests.get", return_value=self._redirect_response()) as requests_get: + with mock.patch(SECURE_SESSION) as secure_session: + secure_session.return_value.get.return_value = self._redirect_response() self.assertEqual(client.get_token(), "a-token") # verify_ssl must be honored, not hardcoded to False - self.assertFalse(requests_get.call_args.kwargs["verify"]) - self.assertFalse(requests_get.call_args.kwargs["allow_redirects"]) + self.assertFalse(secure_session.call_args.kwargs["verify"]) + self.assertFalse(secure_session.return_value.get.call_args.kwargs["allow_redirects"]) def test_get_token_on_non_redirect_raises_auth_error(self) -> None: response = mock.Mock() response.status_code = 401 response.headers = {} client = self._client() - with mock.patch("requests.get", return_value=response): + with mock.patch(SECURE_SESSION) as secure_session: + secure_session.return_value.get.return_value = response with self.assertRaises(openshift_exceptions.OpenshiftAuthError): client.get_token() From d93087558f852dc7212fd520552e8b4d0636ccc8 Mon Sep 17 00:00:00 2001 From: aschumann-virtualcable Date: Wed, 29 Jul 2026 11:11:55 +0200 Subject: [PATCH 3/4] test(openshift): drop the connection_key stubs and cover the real cache key Master's tests injected a fake connection_key on the provider instance, so is_available() never exercised the key_helper and the AttributeError stayed invisible. Key the cached entry on the client identity instead, and assert a different connection identity does not reuse the previous answer. --- tests/services/openshift/fixtures.py | 3 +-- tests/services/openshift/test_provider.py | 23 ++++++++++++------- tests/services/openshift/test_service.py | 5 ---- .../services/openshift/test_service_fixed.py | 5 ---- 4 files changed, 16 insertions(+), 20 deletions(-) diff --git a/tests/services/openshift/fixtures.py b/tests/services/openshift/fixtures.py index dbdc664e9..25730a410 100644 --- a/tests/services/openshift/fixtures.py +++ b/tests/services/openshift/fixtures.py @@ -142,8 +142,7 @@ def inner(*args: typing.Any, **kwargs: typing.Any) -> T: # Connection identity matching PROVIDER_VALUES_DICT, as OpenshiftClient.cache_key() would build it CLIENT_CACHE_KEY = ( f'{PROVIDER_VALUES_DICT["cluster_url"]}|{PROVIDER_VALUES_DICT["api_url"]}|' - f'{PROVIDER_VALUES_DICT["username"]}|{PROVIDER_VALUES_DICT["namespace"]}|' - f'{PROVIDER_VALUES_DICT["verify_ssl"]}' + f'{PROVIDER_VALUES_DICT["username"]}|{PROVIDER_VALUES_DICT["namespace"]}' ) # Service values diff --git a/tests/services/openshift/test_provider.py b/tests/services/openshift/test_provider.py index 00d919bd0..6990ed452 100644 --- a/tests/services/openshift/test_provider.py +++ b/tests/services/openshift/test_provider.py @@ -107,16 +107,8 @@ def test_provider_test(self) -> None: def test_provider_is_available(self) -> None: """ Test the provider is_available method and cache behavior. - - The OpenshiftProvider identity is stable once the provider is loaded - (``connection_key`` is intentionally absent), so the @cached decorator - keys purely on the method name. We patch ``connection_key`` on the - instance to provide a stable identity without reintroducing a method. """ with fixtures.patched_provider() as provider: - # Provide the identity the @cached key_helper expects, without - # adding the method back to the provider itself. - provider.connection_key = lambda: "test-cache-key" # type: ignore[attr-defined, unused-ignore] api = typing.cast(mock.MagicMock, provider.api) # First, true result self.assertEqual(provider.is_available(), True) @@ -131,6 +123,21 @@ def test_provider_is_available(self) -> None: self.assertEqual(provider.is_available(), False) api.test.assert_called_once_with() + def test_provider_is_available_keyed_on_connection_identity(self) -> None: + """ + The cached entry belongs to the connection identity, not to the method name: a provider + pointing to another cluster must not read the previous one's answer. + """ + with fixtures.patched_provider() as provider: + api = typing.cast(mock.MagicMock, provider.api) + self.assertEqual(provider.is_available(), True) + api.test.reset_mock() + + api.cache_key.return_value = 'https://other-cluster.example.com|https://other-api:6443|kubeadmin|default' + api.test.return_value = False + self.assertEqual(provider.is_available(), False) + api.test.assert_called_once_with() + # --- Provider API Methods --- def test_provider_api_methods(self) -> None: """ diff --git a/tests/services/openshift/test_service.py b/tests/services/openshift/test_service.py index 79c25d3bd..ebe9cdd96 100644 --- a/tests/services/openshift/test_service.py +++ b/tests/services/openshift/test_service.py @@ -104,13 +104,8 @@ def test_service_methods(self) -> None: def test_service_is_available(self) -> None: """ Check service availability and cache handling. - - The provider identity is stable once loaded, so we patch - ``connection_key`` on the instance to feed the @cached decorator's - key_helper without reintroducing the method on the provider class. """ service, provider, provider_ctx = self._create_service_with_provider() - provider.connection_key = lambda: "test-cache-key" # type: ignore[attr-defined, unused-ignore] api = typing.cast(mock.MagicMock, provider.api) self.assertTrue(service.is_available()) api.test.assert_called_with() diff --git a/tests/services/openshift/test_service_fixed.py b/tests/services/openshift/test_service_fixed.py index bb5cd5ea9..8a02b51b2 100644 --- a/tests/services/openshift/test_service_fixed.py +++ b/tests/services/openshift/test_service_fixed.py @@ -64,13 +64,8 @@ def setUp(self) -> None: def test_service_is_available(self) -> None: """ Test provider availability and cache logic. - - The provider identity is stable once loaded, so we patch - ``connection_key`` on the instance to feed the @cached decorator's - key_helper without reintroducing the method on the provider class. """ service, provider, provider_ctx = self._create_service_fixed_with_provider() - provider.connection_key = lambda: "test-cache-key" # type: ignore[attr-defined, unused-ignore] api = typing.cast(mock.MagicMock, provider.api) self.assertTrue(service.is_available()) api.test.assert_called_with() From 4790c9dbec1008a5b9ddc4d6a2ef56f3c8f222eb Mon Sep 17 00:00:00 2001 From: aschumann-virtualcable Date: Tue, 4 Aug 2026 13:18:49 +0200 Subject: [PATCH 4/4] fix(openshift): key the reachable cache with a named cache_key_helper --- src/uds/services/OpenShift/provider.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/uds/services/OpenShift/provider.py b/src/uds/services/OpenShift/provider.py index a9006f9c3..70e6d2138 100644 --- a/src/uds/services/OpenShift/provider.py +++ b/src/uds/services/OpenShift/provider.py @@ -52,6 +52,13 @@ logger = logging.getLogger(__name__) +def cache_key_helper(self: "OpenshiftProvider") -> str: + """ + Helper function to generate cache keys for the OpenshiftProvider class + """ + return self.api.cache_key() + + class OpenshiftProvider(ServiceProvider): offers = [OpenshiftService, OpenshiftServiceFixed] type_name = _("Openshift Provider") @@ -131,7 +138,7 @@ def api(self) -> "client.OpenshiftClient": def test_connection(self) -> bool: return self.api.test() - @cached("reachable", consts.cache.SHORT_CACHE_TIMEOUT, key_helper=lambda x: x.api.cache_key()) + @cached("reachable", consts.cache.SHORT_CACHE_TIMEOUT, key_helper=cache_key_helper) def is_available(self) -> bool: return self.api.test()