diff --git a/src/uds/services/OpenShift/openshift/client.py b/src/uds/services/OpenShift/openshift/client.py index 159b84159..87cb4b078 100644 --- a/src/uds/services/OpenShift/openshift/client.py +++ b/src/uds/services/OpenShift/openshift/client.py @@ -90,28 +90,58 @@ 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: 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 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 + 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: 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/fixtures.py b/tests/services/openshift/fixtures.py index bcb106b2f..dbdc664e9 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, @@ -166,6 +173,9 @@ 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.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 f83960772..c955bf4f2 100644 --- a/tests/services/openshift/test_client.py +++ b/tests/services/openshift/test_client.py @@ -32,10 +32,12 @@ """ import logging +from unittest import mock 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__) @@ -167,3 +169,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 44f517d7f..4866e1213 100644 --- a/tests/services/openshift/test_provider.py +++ b/tests/services/openshift/test_provider.py @@ -141,6 +141,49 @@ def test_provider_api_methods(self) -> None: self.assertTrue(provider.api.stop_vm("vm-1")) self.assertTrue(provider.api.delete_vm("vm-1")) + # --- Config Change Detection --- + def test_connection_key_matches_client_cache_key(self) -> 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() + self.assertEqual(provider.connection_key(), provider.api.cache_key()) + + 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_config_changed(self) -> None: + """ + api property creates a new OpenshiftClient when the connection params have changed. + """ + 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' + 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_config_unchanged(self) -> None: + """ + api property reuses the cached client when connection params haven't changed. + """ + provider = fixtures.create_provider() + old_client = fixtures.create_client_mock() # cache_key() matches PROVIDER_VALUES_DICT + provider._cached_api = old_client + self.assertIs(provider.api, old_client) + # --- Name Sanitization --- def test_sanitized_name(self) -> None: """