Master fixes openshift

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:

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

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()
 

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

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)

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:
         """