From c1b772182a65ce3458ff04b66c72b47edb95bbee Mon Sep 17 00:00:00 2001 From: aschumann-virtualcable Date: Mon, 25 May 2026 13:40:32 +0200 Subject: [PATCH 1/3] Improves OpenShift client caching and token handling Adds URL change detection for cached client reuse and refetch, ensuring updated configuration is respected without stale connections. Refines token retrieval to handle redirects and error cases more robustly, improving authentication reliability. Updates caching keys to include connection parameters for better cache validity. Enhances related tests to cover new logic and edge cases. --- src/uds/services/OpenShift/deployment.py | 1 + .../services/OpenShift/deployment_fixed.py | 1 + .../services/OpenShift/openshift/client.py | 39 ++++++--- .../OpenShift/openshift/exceptions.py | 1 - src/uds/services/OpenShift/provider.py | 18 +++-- src/uds/services/OpenShift/publication.py | 1 + src/uds/services/OpenShift/service.py | 1 + src/uds/services/OpenShift/service_fixed.py | 2 +- tests/services/openshift/fixtures.py | 2 + tests/services/openshift/test_provider.py | 80 +++++++++++++++++++ 10 files changed, 129 insertions(+), 17 deletions(-) diff --git a/src/uds/services/OpenShift/deployment.py b/src/uds/services/OpenShift/deployment.py index f98f06189..4473f18b4 100644 --- a/src/uds/services/OpenShift/deployment.py +++ b/src/uds/services/OpenShift/deployment.py @@ -7,6 +7,7 @@ ''' Author: Adolfo Gómez, dkmaster at dkmon dot com ''' + import logging import typing diff --git a/src/uds/services/OpenShift/deployment_fixed.py b/src/uds/services/OpenShift/deployment_fixed.py index b0b85919c..63e6e8002 100644 --- a/src/uds/services/OpenShift/deployment_fixed.py +++ b/src/uds/services/OpenShift/deployment_fixed.py @@ -28,6 +28,7 @@ """ Author: Adolfo Gómez, dkmaster at dkmon dot com """ + import logging import typing diff --git a/src/uds/services/OpenShift/openshift/client.py b/src/uds/services/OpenShift/openshift/client.py index 4eeaea6d8..a142089f2 100644 --- a/src/uds/services/OpenShift/openshift/client.py +++ b/src/uds/services/OpenShift/openshift/client.py @@ -27,12 +27,15 @@ """ Author: Adolfo Gómez, dkmaster at dkmon dot com """ + import collections.abc import typing import datetime import urllib.parse import logging import requests +import urllib3 +from urllib3.exceptions import InsecureRequestWarning from uds.core.util import security from uds.core.util.cache import Cache @@ -88,19 +91,33 @@ def session(self) -> requests.Session: def get_token(self) -> str | None: try: + urllib3.disable_warnings(InsecureRequestWarning) url = ( f"{self.cluster_url}/oauth/authorize?client_id=openshift-challenging-client&response_type=token" ) r = requests.get( - url, auth=(self.username, self.password), timeout=15, allow_redirects=True, verify=False + url, + auth=(self.username, self.password), + timeout=15, + allow_redirects=False, + verify=self._verify_ssl, ) - if "access_token=" not in r.url: - raise Exception("access_token not found in response URL") - token = r.url.split("access_token=")[1].split("&")[0] + if r.status_code not in (301, 302, 303, 307, 308): + raise exceptions.OpenshiftAuthError( + f"Unexpected response status while fetching token: {r.status_code}" + ) + location = r.headers.get('Location', '') + parsed = urllib.parse.urlparse(location) + params = urllib.parse.parse_qs(parsed.fragment) or urllib.parse.parse_qs(parsed.query) + token = params.get('access_token', [None])[0] + if not token: + raise exceptions.OpenshiftAuthError("access_token not found in redirect Location") return token - except Exception as ex: - logging.error(f"Could not obtain token: {ex}") + except exceptions.OpenshiftError: raise + except Exception as ex: + logger.error("Could not obtain token: %s", ex) + raise exceptions.OpenshiftConnectionError(str(ex)) def connect(self, force: bool = False) -> requests.Session: # For testing, always use the fixed token @@ -117,9 +134,7 @@ def connect(self, force: bool = False) -> requests.Session: def get_api_url(self, path: str, *parameters: tuple[str, str]) -> str: url = self.api_url + path if parameters: - url += '?' + urllib.parse.urlencode( - parameters, doseq=True, safe='[]' - ) + url += '?' + urllib.parse.urlencode(parameters, doseq=True, safe='[]') return url def do_request( @@ -581,7 +596,11 @@ def test(self) -> bool: logger.error(f"Error testing Openshift by enumerating VMs: {e}") raise exceptions.OpenshiftConnectionError(str(e)) from e - @cached('vms', consts.CACHE_INFO_DURATION) + @cached( + 'vms', + consts.CACHE_INFO_DURATION, + key_helper=lambda x: f'{x.cluster_url}|{x.api_url}|{x.username}|{x.namespace}|{x._verify_ssl}', + ) def list_vms(self) -> collections.abc.Iterator[types.VM]: """ Fetch all VMs from KubeVirt API in the current namespace as VMDefinition objects using do_request. diff --git a/src/uds/services/OpenShift/openshift/exceptions.py b/src/uds/services/OpenShift/openshift/exceptions.py index 2178e9526..3811bde07 100644 --- a/src/uds/services/OpenShift/openshift/exceptions.py +++ b/src/uds/services/OpenShift/openshift/exceptions.py @@ -28,7 +28,6 @@ Author: Adolfo Gómez, dkmaster at dkmon dot com """ - from uds.core import exceptions diff --git a/src/uds/services/OpenShift/provider.py b/src/uds/services/OpenShift/provider.py index 1c1804528..a3c9e59e3 100644 --- a/src/uds/services/OpenShift/provider.py +++ b/src/uds/services/OpenShift/provider.py @@ -86,15 +86,23 @@ class OpenshiftProvider(ServiceProvider): concurrent_removal_limit = fields.concurrent_removal_limit_field() timeout = fields.timeout_field() - _cached_api: 'client.OpenshiftClient | None' = None # Cached API client + _cached_api: 'client.OpenshiftClient | None' = None # Cached API client — lock-free check-then-set benign here (worst case: duplicate client construct, no network I/O in __init__) def initialize(self, values: 'core_types.core.ValuesType') -> None: - # No port validation needed, URLs are used - pass + self._cached_api = None # Force refresh on config change + + def _urls_changed(self) -> bool: + return ( + self._cached_api is not None + and ( + self._cached_api.cluster_url != self.cluster_url.value + or self._cached_api.api_url != self.api_url.value + ) + ) @property def api(self) -> 'client.OpenshiftClient': - if self._cached_api is None: + if self._cached_api is None or self._urls_changed(): self._cached_api = client.OpenshiftClient( cluster_url=self.cluster_url.value, api_url=self.api_url.value, @@ -110,7 +118,7 @@ def api(self) -> 'client.OpenshiftClient': def test_connection(self) -> bool: return self.api.test() - @cached('reachable', consts.cache.SHORT_CACHE_TIMEOUT) + @cached('reachable', consts.cache.SHORT_CACHE_TIMEOUT, key_helper=lambda x: f'{x.cluster_url.value}|{x.api_url.value}|{x.username.value}|{x.namespace.value}|{x.verify_ssl.as_bool()}') def is_available(self) -> bool: return self.api.test() diff --git a/src/uds/services/OpenShift/publication.py b/src/uds/services/OpenShift/publication.py index fca450d31..65e49f63d 100644 --- a/src/uds/services/OpenShift/publication.py +++ b/src/uds/services/OpenShift/publication.py @@ -6,6 +6,7 @@ """ Author: Adolfo Gómez, dkmaster at dkmon dot com """ + import logging import typing diff --git a/src/uds/services/OpenShift/service.py b/src/uds/services/OpenShift/service.py index 55e560528..fdfb6ab52 100644 --- a/src/uds/services/OpenShift/service.py +++ b/src/uds/services/OpenShift/service.py @@ -8,6 +8,7 @@ ''' Author: Adolfo Gómez, dkmaster at dkmon dot com ''' + import logging import collections.abc import typing diff --git a/src/uds/services/OpenShift/service_fixed.py b/src/uds/services/OpenShift/service_fixed.py index 9e1199427..52272601e 100644 --- a/src/uds/services/OpenShift/service_fixed.py +++ b/src/uds/services/OpenShift/service_fixed.py @@ -28,6 +28,7 @@ """ Author: Adolfo Gómez, dkmaster at dkmon dot com """ + import collections.abc import logging import typing @@ -49,7 +50,6 @@ from .openshift import exceptions as oshift_exceptions from .openshift import exceptions as oshift_exceptions - logger = logging.getLogger(__name__) diff --git a/tests/services/openshift/fixtures.py b/tests/services/openshift/fixtures.py index ea4fcfc5e..03c5d5166 100644 --- a/tests/services/openshift/fixtures.py +++ b/tests/services/openshift/fixtures.py @@ -166,6 +166,8 @@ def create_client_mock() -> mock.Mock: client = mock.MagicMock() # Prepare deep copies of default data + client.cluster_url = PROVIDER_VALUES_DICT['cluster_url'] + client.api_url = PROVIDER_VALUES_DICT['api_url'] client.test.return_value = True client.list_vms.return_value = copy.deepcopy(DEF_VMS) client.start_vm_instance.return_value = True diff --git a/tests/services/openshift/test_provider.py b/tests/services/openshift/test_provider.py index f8c29d32f..d75c59986 100644 --- a/tests/services/openshift/test_provider.py +++ b/tests/services/openshift/test_provider.py @@ -128,6 +128,86 @@ def test_provider_api_methods(self) -> None: self.assertTrue(provider.api.stop_vm('vm-1')) self.assertTrue(provider.api.delete_vm('vm-1')) + # --- URL Change Detection --- + def test_urls_changed_none_cached_api(self) -> None: + """ + _urls_changed() returns False when _cached_api is None. + """ + provider = fixtures.create_provider() + provider._cached_api = None + self.assertFalse(provider._urls_changed()) + + def test_urls_changed_same_urls(self) -> None: + """ + _urls_changed() returns False when cached client URLs match provider field values. + """ + provider = fixtures.create_provider() + client_mock = fixtures.create_client_mock() + client_mock.cluster_url = fixtures.PROVIDER_VALUES_DICT['cluster_url'] + client_mock.api_url = fixtures.PROVIDER_VALUES_DICT['api_url'] + provider._cached_api = client_mock + self.assertFalse(provider._urls_changed()) + + def test_urls_changed_cluster_url_differs(self) -> None: + """ + _urls_changed() returns True when cached client cluster_url differs. + """ + provider = fixtures.create_provider() + client_mock = fixtures.create_client_mock() + client_mock.cluster_url = 'https://different-cluster.example.com' + client_mock.api_url = fixtures.PROVIDER_VALUES_DICT['api_url'] + provider._cached_api = client_mock + self.assertTrue(provider._urls_changed()) + + def test_urls_changed_api_url_differs(self) -> None: + """ + _urls_changed() returns True when cached client api_url differs. + """ + provider = fixtures.create_provider() + client_mock = fixtures.create_client_mock() + client_mock.cluster_url = fixtures.PROVIDER_VALUES_DICT['cluster_url'] + client_mock.api_url = 'https://different-api.example.com:6443' + provider._cached_api = client_mock + self.assertTrue(provider._urls_changed()) + + def test_initialize_resets_cached_api(self) -> None: + """ + initialize() should set _cached_api to None to force refresh on config change. + """ + provider = fixtures.create_provider() + provider._cached_api = mock.MagicMock() + provider.initialize({}) + self.assertIsNone(provider._cached_api) + + def test_api_recreates_client_when_urls_changed(self) -> None: + """ + api property creates a new OpenshiftClient when URLs have changed. + """ + provider = fixtures.create_provider() + old_client = fixtures.create_client_mock() + old_client.cluster_url = 'https://old-cluster.example.com' + old_client.api_url = 'https://old-api.example.com:6443' + 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) + + def test_api_reuses_client_when_urls_unchanged(self) -> None: + """ + api property reuses the cached client when URLs haven't changed. + """ + provider = fixtures.create_provider() + old_client = fixtures.create_client_mock() + old_client.cluster_url = fixtures.PROVIDER_VALUES_DICT['cluster_url'] + old_client.api_url = fixtures.PROVIDER_VALUES_DICT['api_url'] + provider._cached_api = old_client + result = provider.api + self.assertIs(result, old_client) + # --- Name Sanitization --- def test_sanitized_name(self) -> None: """ From e2f24c937e7646ab56e6f6cbf12874050ecd0464 Mon Sep 17 00:00:00 2001 From: aschumann-virtualcable Date: Tue, 14 Jul 2026 14:17:53 +0200 Subject: [PATCH 2/3] Cache the OpenShift session and token instead of reauthenticating per request connect() built a new requests.Session and performed a full OAuth round trip on every single API call, since do_request() reaches the session through a property that always called it. The _access_token and _token_expiry attributes were already declared but never used; they now hold the token for consts.TOKEN_VALIDITY (30 min). do_request() already clears _session on a 401, so an early expiry self-heals on the next request. The force argument now actually forces a refresh. Also drops the global urllib3.disable_warnings(InsecureRequestWarning) call, which silenced warnings process-wide for every other service and is no longer needed now that get_token() honors verify_ssl. Connection identity is now built in a single place, OpenshiftClient.cache_key(), and mirrored by OpenshiftProvider.connection_key(). Both the cache key helpers and the cached-client validity check use it, so they can no longer disagree: the previous check only compared the two URLs while the cache key covered five fields. Adds offline tests for token parsing, session/token reuse and refetch after invalidation, and for the client/provider connection keys staying in sync. --- .../services/OpenShift/openshift/client.py | 28 +++++--- .../services/OpenShift/openshift/consts.py | 5 ++ src/uds/services/OpenShift/provider.py | 21 +++--- tests/services/openshift/fixtures.py | 8 +++ tests/services/openshift/test_client.py | 64 +++++++++++++++++++ tests/services/openshift/test_provider.py | 61 ++++-------------- 6 files changed, 118 insertions(+), 69 deletions(-) diff --git a/src/uds/services/OpenShift/openshift/client.py b/src/uds/services/OpenShift/openshift/client.py index a142089f2..04402e737 100644 --- a/src/uds/services/OpenShift/openshift/client.py +++ b/src/uds/services/OpenShift/openshift/client.py @@ -34,8 +34,6 @@ import urllib.parse import logging import requests -import urllib3 -from urllib3.exceptions import InsecureRequestWarning from uds.core.util import security from uds.core.util.cache import Cache @@ -89,9 +87,16 @@ def __init__( def session(self) -> requests.Session: return self.connect() + def cache_key(self) -> str: + """ + Identity of the connection parameters. Any change here must invalidate both the cached + client on the provider and any data cached through the `cached` decorator. + Password is deliberately excluded, it does not change what data we see. + """ + return f'{self.cluster_url}|{self.api_url}|{self.username}|{self.namespace}|{self._verify_ssl}' + def get_token(self) -> str | None: try: - urllib3.disable_warnings(InsecureRequestWarning) url = ( f"{self.cluster_url}/oauth/authorize?client_id=openshift-challenging-client&response_type=token" ) @@ -120,15 +125,22 @@ def get_token(self) -> str | None: raise exceptions.OpenshiftConnectionError(str(ex)) def connect(self, force: bool = False) -> requests.Session: - # For testing, always use the fixed token + now = datetime.datetime.now() + if not force and self._session is not None and now < self._token_expiry: + return self._session + + self._access_token = self.get_token() or '' session = self._session = security.secure_requests_session(verify=self._verify_ssl) session.headers.update( { 'Accept': 'application/json', 'Content-Type': 'application/json', - 'Authorization': f'Bearer {self.get_token()}', + 'Authorization': f'Bearer {self._access_token}', } ) + # ponytail: fixed TTL instead of parsing expires_in from the redirect. do_request already + # drops _session on a 401, so an early expiry self-heals on the next request. + self._token_expiry = now + consts.TOKEN_VALIDITY return session def get_api_url(self, path: str, *parameters: tuple[str, str]) -> str: @@ -596,11 +608,7 @@ def test(self) -> bool: logger.error(f"Error testing Openshift by enumerating VMs: {e}") raise exceptions.OpenshiftConnectionError(str(e)) from e - @cached( - 'vms', - consts.CACHE_INFO_DURATION, - key_helper=lambda x: f'{x.cluster_url}|{x.api_url}|{x.username}|{x.namespace}|{x._verify_ssl}', - ) + @cached('vms', consts.CACHE_INFO_DURATION, key_helper=lambda x: x.cache_key()) def list_vms(self) -> collections.abc.Iterator[types.VM]: """ Fetch all VMs from KubeVirt API in the current namespace as VMDefinition objects using do_request. diff --git a/src/uds/services/OpenShift/openshift/consts.py b/src/uds/services/OpenShift/openshift/consts.py index 7c561c5d2..e85d8ba41 100644 --- a/src/uds/services/OpenShift/openshift/consts.py +++ b/src/uds/services/OpenShift/openshift/consts.py @@ -32,8 +32,13 @@ from uds.core import consts +import datetime import typing +# OAuth tokens issued by Openshift last much longer (24h by default), but we prefer to +# renew ours often enough to not have to care about the cluster's configured lifetime +TOKEN_VALIDITY: typing.Final[datetime.timedelta] = datetime.timedelta(minutes=30) + CACHE_DURATION: typing.Final[int] = consts.cache.DEFAULT_CACHE_TIMEOUT CACHE_INFO_DURATION: typing.Final[int] = consts.cache.SHORT_CACHE_TIMEOUT CACHE_VM_INFO_DURATION: typing.Final[int] = consts.cache.SHORTEST_CACHE_TIMEOUT diff --git a/src/uds/services/OpenShift/provider.py b/src/uds/services/OpenShift/provider.py index a3c9e59e3..456685a5c 100644 --- a/src/uds/services/OpenShift/provider.py +++ b/src/uds/services/OpenShift/provider.py @@ -86,23 +86,24 @@ class OpenshiftProvider(ServiceProvider): concurrent_removal_limit = fields.concurrent_removal_limit_field() timeout = fields.timeout_field() - _cached_api: 'client.OpenshiftClient | None' = None # Cached API client — lock-free check-then-set benign here (worst case: duplicate client construct, no network I/O in __init__) + _cached_api: 'client.OpenshiftClient | None' = None # Cached API client def initialize(self, values: 'core_types.core.ValuesType') -> None: - self._cached_api = None # Force refresh on config change + self._cached_api = None # Config may have changed, do not reuse the client - def _urls_changed(self) -> bool: + def connection_key(self) -> str: + """ + Identity of the connection parameters, in the same format as `OpenshiftClient.cache_key`, + so a cached client can be checked against the current configuration. + """ return ( - self._cached_api is not None - and ( - self._cached_api.cluster_url != self.cluster_url.value - or self._cached_api.api_url != self.api_url.value - ) + f'{self.cluster_url.value}|{self.api_url.value}|{self.username.value}|' + f'{self.namespace.value or "default"}|{self.verify_ssl.as_bool()}' ) @property def api(self) -> 'client.OpenshiftClient': - if self._cached_api is None or self._urls_changed(): + if self._cached_api is None or self._cached_api.cache_key() != self.connection_key(): self._cached_api = client.OpenshiftClient( cluster_url=self.cluster_url.value, api_url=self.api_url.value, @@ -118,7 +119,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: f'{x.cluster_url.value}|{x.api_url.value}|{x.username.value}|{x.namespace.value}|{x.verify_ssl.as_bool()}') + @cached('reachable', consts.cache.SHORT_CACHE_TIMEOUT, key_helper=lambda x: x.connection_key()) def is_available(self) -> bool: return self.api.test() diff --git a/tests/services/openshift/fixtures.py b/tests/services/openshift/fixtures.py index 03c5d5166..c58dbe10a 100644 --- a/tests/services/openshift/fixtures.py +++ b/tests/services/openshift/fixtures.py @@ -139,6 +139,13 @@ def inner(*args: typing.Any, **kwargs: typing.Any) -> T: 'timeout': 10, } +# 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"]}' +) + # Service values SERVICE_VALUES_DICT: gui.ValuesDictType = { 'template': VMS[0].name, @@ -168,6 +175,7 @@ def create_client_mock() -> mock.Mock: # Prepare deep copies of default data client.cluster_url = PROVIDER_VALUES_DICT['cluster_url'] client.api_url = PROVIDER_VALUES_DICT['api_url'] + client.cache_key.return_value = CLIENT_CACHE_KEY client.test.return_value = True client.list_vms.return_value = copy.deepcopy(DEF_VMS) client.start_vm_instance.return_value = True diff --git a/tests/services/openshift/test_client.py b/tests/services/openshift/test_client.py index ad31df68d..276d05236 100644 --- a/tests/services/openshift/test_client.py +++ b/tests/services/openshift/test_client.py @@ -32,8 +32,10 @@ """ import logging +from unittest import mock from uds.services.OpenShift.openshift import client as openshift_client +from uds.services.OpenShift.openshift import exceptions as openshift_exceptions from tests.utils.test import UDSTransactionTestCase from tests.utils import vars @@ -165,3 +167,65 @@ def test_datavolume_phase_invalid(self): """ phase = self.os_client.get_datavolume_phase('nonexistent-dv') self.assertIsInstance(phase, str) + + +class TestOpenshiftClientToken(UDSTransactionTestCase): + """Offline tests for token retrieval and session/token caching.""" + + def _client(self) -> openshift_client.OpenshiftClient: + return openshift_client.OpenshiftClient( + cluster_url='https://oauth-openshift.apps-crc.testing', + api_url='https://api.crc.testing:6443', + username='kubeadmin', + password='test-password', + namespace='default', + ) + + def _redirect_response(self, token: str = 'a-token') -> mock.Mock: + response = mock.Mock() + response.status_code = 302 + response.headers = { + 'Location': f'https://oauth-openshift.apps-crc.testing/oauth/token/implicit' + f'#access_token={token}&expires_in=86400&token_type=Bearer' + } + return response + + 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: + 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']) + + 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 self.assertRaises(openshift_exceptions.OpenshiftAuthError): + client.get_token() + + def test_connect_reuses_session_and_token(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 + + def test_connect_refetches_token_after_invalidation(self) -> None: + client = self._client() + with mock.patch.object(client, 'get_token', return_value='a-token') as get_token: + client.connect() + client._session = None # what do_request does on a 401 + client.connect() + self.assertEqual(get_token.call_count, 2) + + def test_connect_refetches_token_when_forced(self) -> None: + client = self._client() + with mock.patch.object(client, 'get_token', return_value='a-token') as get_token: + client.connect() + client.connect(force=True) + self.assertEqual(get_token.call_count, 2) diff --git a/tests/services/openshift/test_provider.py b/tests/services/openshift/test_provider.py index d75c59986..5b3849aa4 100644 --- a/tests/services/openshift/test_provider.py +++ b/tests/services/openshift/test_provider.py @@ -128,47 +128,14 @@ def test_provider_api_methods(self) -> None: self.assertTrue(provider.api.stop_vm('vm-1')) self.assertTrue(provider.api.delete_vm('vm-1')) - # --- URL Change Detection --- - def test_urls_changed_none_cached_api(self) -> None: + # --- Config Change Detection --- + def test_connection_key_matches_client_cache_key(self) -> None: """ - _urls_changed() returns False when _cached_api is None. + provider.connection_key() and OpenshiftClient.cache_key() must build the same string, + or the cached client would be recreated on every access. """ provider = fixtures.create_provider() - provider._cached_api = None - self.assertFalse(provider._urls_changed()) - - def test_urls_changed_same_urls(self) -> None: - """ - _urls_changed() returns False when cached client URLs match provider field values. - """ - provider = fixtures.create_provider() - client_mock = fixtures.create_client_mock() - client_mock.cluster_url = fixtures.PROVIDER_VALUES_DICT['cluster_url'] - client_mock.api_url = fixtures.PROVIDER_VALUES_DICT['api_url'] - provider._cached_api = client_mock - self.assertFalse(provider._urls_changed()) - - def test_urls_changed_cluster_url_differs(self) -> None: - """ - _urls_changed() returns True when cached client cluster_url differs. - """ - provider = fixtures.create_provider() - client_mock = fixtures.create_client_mock() - client_mock.cluster_url = 'https://different-cluster.example.com' - client_mock.api_url = fixtures.PROVIDER_VALUES_DICT['api_url'] - provider._cached_api = client_mock - self.assertTrue(provider._urls_changed()) - - def test_urls_changed_api_url_differs(self) -> None: - """ - _urls_changed() returns True when cached client api_url differs. - """ - provider = fixtures.create_provider() - client_mock = fixtures.create_client_mock() - client_mock.cluster_url = fixtures.PROVIDER_VALUES_DICT['cluster_url'] - client_mock.api_url = 'https://different-api.example.com:6443' - provider._cached_api = client_mock - self.assertTrue(provider._urls_changed()) + self.assertEqual(provider.connection_key(), provider.api.cache_key()) def test_initialize_resets_cached_api(self) -> None: """ @@ -179,14 +146,13 @@ def test_initialize_resets_cached_api(self) -> None: provider.initialize({}) self.assertIsNone(provider._cached_api) - def test_api_recreates_client_when_urls_changed(self) -> None: + def test_api_recreates_client_when_config_changed(self) -> None: """ - api property creates a new OpenshiftClient when URLs have changed. + api property creates a new OpenshiftClient when the connection params have changed. """ provider = fixtures.create_provider() old_client = fixtures.create_client_mock() - old_client.cluster_url = 'https://old-cluster.example.com' - old_client.api_url = 'https://old-api.example.com:6443' + old_client.cache_key.return_value = 'https://old-cluster.example.com|https://old-api.example.com:6443|kubeadmin|default|False' provider._cached_api = old_client with mock.patch('uds.services.OpenShift.provider.client.OpenshiftClient') as MockClient: @@ -196,17 +162,14 @@ def test_api_recreates_client_when_urls_changed(self) -> None: MockClient.assert_called_once() self.assertIs(result, new_mock) - def test_api_reuses_client_when_urls_unchanged(self) -> None: + def test_api_reuses_client_when_config_unchanged(self) -> None: """ - api property reuses the cached client when URLs haven't changed. + api property reuses the cached client when connection params haven't changed. """ provider = fixtures.create_provider() - old_client = fixtures.create_client_mock() - old_client.cluster_url = fixtures.PROVIDER_VALUES_DICT['cluster_url'] - old_client.api_url = fixtures.PROVIDER_VALUES_DICT['api_url'] + old_client = fixtures.create_client_mock() # cache_key() matches PROVIDER_VALUES_DICT provider._cached_api = old_client - result = provider.api - self.assertIs(result, old_client) + self.assertIs(provider.api, old_client) # --- Name Sanitization --- def test_sanitized_name(self) -> None: From 93ed6feeaa6c8ff5293e3c3927d81dd5ab28ab5a Mon Sep 17 00:00:00 2001 From: aschumann-virtualcable Date: Mon, 27 Jul 2026 14:58:38 +0200 Subject: [PATCH 3/3] fix(openshift): restore token caching lost in merge with master The merge with origin/master took master's version of the OpenShift client, provider and consts, which undid most of the session/token caching commit but kept part of connect(), leaving code that referenced things no longer present. connect() raised AttributeError on consts.TOKEN_VALIDITY and built the Authorization header with a second get_token() round trip. Restores TOKEN_VALIDITY, get_token() parsing the redirect Location with allow_redirects=False and verify_ssl honored, the provider connection_key() used both by the cached client check and the is_available key helper, and initialize() clearing the cached client on config change. --- .../services/OpenShift/openshift/client.py | 23 +++++++++++++++---- .../services/OpenShift/openshift/consts.py | 5 ++++ src/uds/services/OpenShift/provider.py | 17 ++++++++++---- tests/services/openshift/test_client.py | 1 + 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/uds/services/OpenShift/openshift/client.py b/src/uds/services/OpenShift/openshift/client.py index 2cbe2d5ca..87cb4b078 100644 --- a/src/uds/services/OpenShift/openshift/client.py +++ b/src/uds/services/OpenShift/openshift/client.py @@ -101,10 +101,23 @@ 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(url, auth=(self.username, self.password), timeout=15, allow_redirects=True, verify=False) - if "access_token=" not in r.url: - raise Exception("access_token not found in response URL") - token = r.url.split("access_token=")[1].split("&")[0] + r = requests.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( + f"Unexpected response status while fetching token: {r.status_code}" + ) + location = r.headers.get('Location', '') + parsed = urllib.parse.urlparse(location) + params = urllib.parse.parse_qs(parsed.fragment) or urllib.parse.parse_qs(parsed.query) + token = params.get('access_token', [None])[0] + if not token: + raise exceptions.OpenshiftAuthError("access_token not found in redirect Location") return token except exceptions.OpenshiftError: raise @@ -123,7 +136,7 @@ def connect(self, force: bool = False) -> requests.Session: { "Accept": "application/json", "Content-Type": "application/json", - "Authorization": f"Bearer {self.get_token()}", + "Authorization": f"Bearer {self._access_token}", } ) # ponytail: fixed TTL instead of parsing expires_in from the redirect. do_request already diff --git a/src/uds/services/OpenShift/openshift/consts.py b/src/uds/services/OpenShift/openshift/consts.py index eb3255dfa..a8eadcf57 100644 --- a/src/uds/services/OpenShift/openshift/consts.py +++ b/src/uds/services/OpenShift/openshift/consts.py @@ -30,10 +30,15 @@ # DEFAULT_PORT = 8006 +import datetime import typing from uds.core import consts +# OAuth tokens issued by Openshift last much longer (24h by default), but we prefer to +# renew ours often enough to not have to care about the cluster's configured lifetime +TOKEN_VALIDITY: typing.Final[datetime.timedelta] = datetime.timedelta(minutes=30) + CACHE_DURATION: typing.Final[int] = consts.cache.DEFAULT_CACHE_TIMEOUT CACHE_INFO_DURATION: typing.Final[int] = consts.cache.SHORT_CACHE_TIMEOUT CACHE_VM_INFO_DURATION: typing.Final[int] = consts.cache.SHORTEST_CACHE_TIMEOUT diff --git a/src/uds/services/OpenShift/provider.py b/src/uds/services/OpenShift/provider.py index e29e1b4dc..76789b2a3 100644 --- a/src/uds/services/OpenShift/provider.py +++ b/src/uds/services/OpenShift/provider.py @@ -111,12 +111,21 @@ class OpenshiftProvider(ServiceProvider): @typing.override def initialize(self, values: "core_types.core.ValuesType") -> None: - # No port validation needed, URLs are used - pass + self._cached_api = None # Config may have changed, do not reuse the client + + def connection_key(self) -> str: + """ + Identity of the connection parameters, in the same format as `OpenshiftClient.cache_key`, + so a cached client can be checked against the current configuration. + """ + return ( + f'{self.cluster_url.value}|{self.api_url.value}|{self.username.value}|' + f'{self.namespace.value or "default"}|{self.verify_ssl.as_bool()}' + ) @property def api(self) -> "client.OpenshiftClient": - if self._cached_api is None: + if self._cached_api is None or self._cached_api.cache_key() != self.connection_key(): self._cached_api = client.OpenshiftClient( cluster_url=self.cluster_url.value, api_url=self.api_url.value, @@ -132,7 +141,7 @@ def api(self) -> "client.OpenshiftClient": def test_connection(self) -> bool: return self.api.test() - @cached("reachable", consts.cache.SHORT_CACHE_TIMEOUT) + @cached("reachable", consts.cache.SHORT_CACHE_TIMEOUT, key_helper=lambda x: x.connection_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 d92a557e4..c955bf4f2 100644 --- a/tests/services/openshift/test_client.py +++ b/tests/services/openshift/test_client.py @@ -37,6 +37,7 @@ from tests.utils import vars from tests.utils.test import UDSTransactionTestCase from uds.services.OpenShift.openshift import client as openshift_client +from uds.services.OpenShift.openshift import exceptions as openshift_exceptions logger = logging.getLogger(__name__)