From 1a7eab8e7506f8ea7ada5619bc258cc890df6a03 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 6 Sep 2023 23:04:38 -0700 Subject: [PATCH 1/3] Stop mocking the entire Kubernetes ApiClient When mocking the Kubernetes API, stop mocking the entire ApiClient, since we need to use its deserialize method to convert watch events to their proper types. Due to the other mocking, we should never make network calls, but mock the request method of ApiClient just in case. --- src/safir/testing/kubernetes.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/safir/testing/kubernetes.py b/src/safir/testing/kubernetes.py index daf91dbf..b7c238b4 100644 --- a/src/safir/testing/kubernetes.py +++ b/src/safir/testing/kubernetes.py @@ -2282,10 +2282,7 @@ def mock_kubernetes() -> Iterator[MockKubernetesApi]: mock_class = patcher.start() mock_class.return_value = mock_api patchers.append(patcher) - mock_api_client = Mock(spec=client.ApiClient) - mock_api_client.close = AsyncMock() - with patch.object(client, "ApiClient") as mock_client: - mock_client.return_value = mock_api_client + with patch.object(client.ApiClient, "request"): os.environ["KUBERNETES_PORT"] = "tcp://10.0.0.1:443" yield mock_api del os.environ["KUBERNETES_PORT"] From fc617c006220b057f36361e566107bd5045e2c70 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 6 Sep 2023 23:40:12 -0700 Subject: [PATCH 2/3] Fix serialization of objects in watch events In the Kubernetes mock, objects serialized into watch events were not being serialized properly. They were keeping the Python snake-case attribute names instead of the Kubernetes camel-case names. This prevented deserialization to Kubernetes model objects and was also confusing. Fix this by passing the serialize parameter to to_dict when serializing the objects, and restructure the code a bit to reduce duplicate work. Add an explicit test for proper object serialization and deserialization. --- docs/_rst_epilog.rst | 1 + src/safir/testing/kubernetes.py | 34 +++++++++++++++++++++++--------- tests/testing/kubernetes_test.py | 15 +++++++------- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/docs/_rst_epilog.rst b/docs/_rst_epilog.rst index 36d38ddb..64b13af6 100644 --- a/docs/_rst_epilog.rst +++ b/docs/_rst_epilog.rst @@ -7,6 +7,7 @@ .. _Gafaelfawr: https://gafaelfawr.lsst.io/ .. _Gidgethub: https://gidgethub.readthedocs.io/en/latest/ .. _HTTPX: https://www.python-httpx.org/ +.. _kubernetes_asyncio: https://github.com/tomplus/kubernetes_asyncio .. _mypy: https://www.mypy-lang.org .. _Phalanx: https://phalanx.lsst.io .. _pre-commit: https://pre-commit.com diff --git a/src/safir/testing/kubernetes.py b/src/safir/testing/kubernetes.py index b7c238b4..355eda4d 100644 --- a/src/safir/testing/kubernetes.py +++ b/src/safir/testing/kubernetes.py @@ -43,6 +43,7 @@ V1ServiceList, V1Status, ) +from typing_extensions import Protocol from ..asyncio import AsyncMultiQueue @@ -158,6 +159,20 @@ def strip_none(model: dict[str, Any]) -> dict[str, Any]: return result +class _KubernetesModel(Protocol): + """Protocol describing common features of Kubernetes objects. + + The kubernetes_asyncio_ library doesn't provide full typing of its methods + or objects, so use a protocol to describe the functionality that we rely + on when passing a generic object around. + """ + + def to_dict( + self, serialize: bool = False # noqa: FBT001, FBT002 + ) -> dict[str, Any]: + ... + + class _EventStream: """Holds the data for a stream of watchable events. @@ -184,7 +199,7 @@ def next_resource_version(self) -> str: """ return str(self._queue.qsize() + 1) - def add_event(self, event: dict[str, Any]) -> None: + def add_event(self, action: str, obj: _KubernetesModel) -> None: """Add a new event and notify all watchers. Parameters @@ -192,6 +207,7 @@ def add_event(self, event: dict[str, Any]) -> None: event New event. """ + event = {"type": action, "object": obj.to_dict(serialize=True)} self._queue.put(event) def build_watch_response( @@ -801,7 +817,7 @@ async def create_namespaced_event( self._update_metadata(body, "v1", "Event", namespace) stream = self._event_streams[namespace]["Event"] body.metadata.resource_version = stream.next_resource_version - stream.add_event({"type": "ADDED", "object": body.to_dict()}) + stream.add_event("ADDED", body) self._events[namespace].append(body) async def list_namespaced_event( @@ -895,7 +911,7 @@ async def create_namespaced_ingress( ) self._store_object(namespace, "Ingress", name, body) stream = self._event_streams[namespace]["Ingress"] - stream.add_event({"type": "ADDED", "object": body.to_dict()}) + stream.add_event("ADDED", body) async def delete_namespaced_ingress( self, @@ -931,7 +947,7 @@ async def delete_namespaced_ingress( self._maybe_error("delete_namespaced_ingress", name, namespace) ingress = self._get_object(namespace, "Ingress", name) stream = self._event_streams[namespace]["Ingress"] - stream.add_event({"type": "DELETED", "object": ingress.to_dict()}) + stream.add_event("DELETED", ingress) return self._delete_object(namespace, "Ingress", name) async def read_namespaced_ingress( @@ -1072,7 +1088,7 @@ async def patch_namespaced_ingress_status( stream = self._event_streams[namespace]["Ingress"] ingress.metadata.resource_version = stream.next_resource_version self._store_object(namespace, "Ingress", name, ingress, replace=True) - stream.add_event({"type": "MODIFIED", "object": ingress.to_dict()}) + stream.add_event("MODIFIED", ingress) return ingress # JOB API @@ -1181,7 +1197,7 @@ async def delete_namespaced_job( job = self._get_object(namespace, "Job", name) stream = self._event_streams[namespace]["Job"] - stream.add_event({"type": "DELETED", "object": job.to_dict()}) + stream.add_event("DELETED", job) return self._delete_object(namespace, "Job", name) async def list_namespaced_job( @@ -1481,7 +1497,7 @@ async def create_namespaced_pod(self, namespace: str, body: V1Pod) -> None: stream = self._event_streams[namespace]["Pod"] body.metadata.resource_version = stream.next_resource_version self._store_object(namespace, "Pod", body.metadata.name, body) - stream.add_event({"type": "ADDED", "object": body.to_dict()}) + stream.add_event("ADDED", body) if self.initial_pod_phase == "Running": event = CoreV1Event( metadata=V1ObjectMeta( @@ -1529,7 +1545,7 @@ async def delete_namespaced_pod( pod = self._get_object(namespace, "Pod", name) result = self._delete_object(namespace, "Pod", name) stream = self._event_streams[namespace]["Pod"] - stream.add_event({"type": "DELETED", "object": pod.to_dict()}) + stream.add_event("DELETED", pod) return result async def list_namespaced_pod( @@ -1643,7 +1659,7 @@ async def patch_namespaced_pod_status( stream = self._event_streams[namespace]["Pod"] pod.metadata.resource_version = stream.next_resource_version self._store_object(namespace, "Pod", name, pod, replace=True) - stream.add_event({"type": "MODIFIED", "object": pod.to_dict()}) + stream.add_event("MODIFIED", pod) return pod async def read_namespaced_pod(self, name: str, namespace: str) -> V1Pod: diff --git a/tests/testing/kubernetes_test.py b/tests/testing/kubernetes_test.py index ce7425ec..b7ef731b 100644 --- a/tests/testing/kubernetes_test.py +++ b/tests/testing/kubernetes_test.py @@ -178,14 +178,14 @@ async def test_mock_events(mock_kubernetes: MockKubernetesApi) -> None: assert results[0] == results[1] assert results[0] == results[2] result = results[0] - assert result[1] == some_event.to_dict() - assert result[2] == done_event.to_dict() + assert result[1] == some_event.to_dict(serialize=True) + assert result[2] == done_event.to_dict(serialize=True) # The first event should have been the pod running event. assert result[0]["message"] == "Pod foo started" - assert result[0]["involved_object"]["kind"] == "Pod" - assert result[0]["involved_object"]["name"] == "foo" - assert result[0]["involved_object"]["namespace"] == "stuff" + assert result[0]["involvedObject"]["kind"] == "Pod" + assert result[0]["involvedObject"]["name"] == "foo" + assert result[0]["involvedObject"]["namespace"] == "stuff" # Starting with resource version "1" should skip the first event, since # the semantics of a watch are to show any events *after* the provided @@ -223,10 +223,11 @@ async def watch_pod_events( "namespace": namespace, "timeout_seconds": 10, # Just in case, so tests don't hang } - async with Watch().stream(method, **watch_args) as stream: + async with Watch(V1Pod).stream(method, **watch_args) as stream: seen = [] async for event in stream: seen.append(event) + assert isinstance(event["object"], V1Pod) if event["type"] == "DELETED": return seen return seen @@ -295,6 +296,6 @@ async def test_pod_status(mock_kubernetes: MockKubernetesApi) -> None: assert results[0] == results[2] result = results[0] assert result[0]["type"] == "MODIFIED" - assert result[0]["object"]["status"]["phase"] == "Running" + assert result[0]["object"].status.phase == "Running" assert result[1]["type"] == "DELETED" assert result[1]["object"] == result[0]["object"] From 96d174cb467b96864c236308f8f92fee822827ef Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Thu, 7 Sep 2023 11:16:08 -0700 Subject: [PATCH 3/3] Add documentation Kubernetes changes Update the documentation for watches and mention passing the type of the model explicitly into the Watch constructor. Mention the serialize parameter to the to_dict method. Add a change log entry for Kubernetes mock fixes. --- changelog.d/20230907_110606_rra_DM_40638b.md | 4 ++++ docs/user-guide/kubernetes.rst | 20 +++++++++++--------- 2 files changed, 15 insertions(+), 9 deletions(-) create mode 100644 changelog.d/20230907_110606_rra_DM_40638b.md diff --git a/changelog.d/20230907_110606_rra_DM_40638b.md b/changelog.d/20230907_110606_rra_DM_40638b.md new file mode 100644 index 00000000..c547e30f --- /dev/null +++ b/changelog.d/20230907_110606_rra_DM_40638b.md @@ -0,0 +1,4 @@ +### Bug fixes + +- Kubernetes objects included in events are now serialized properly using the Kubernetes camel-case field names instead of the Python snake-case names. In addition to matching Kubernetes behavior more closely, this allows a watch configured with the Kubernetes model type to deserialize the object in the `object` key of the event dictionary. The type must be passed explicitly to the `Watch` constructor, since kubernetes_asyncio's type autodetection does not work with Safir's mock. +- `safir.testing.kubernetes.patch_kubernetes` no longer mocks the entire `ApiClient` class since it is required for deserialization of objects in Kubernetes events. It instead mocks the `request` method of that class for safety, to prevent any network requests to Kubernetes clusters when Kubernetes is mocked. diff --git a/docs/user-guide/kubernetes.rst b/docs/user-guide/kubernetes.rst index 5af24ec5..af54513a 100644 --- a/docs/user-guide/kubernetes.rst +++ b/docs/user-guide/kubernetes.rst @@ -75,15 +75,16 @@ Limitations of the mock ----------------------- Only a limited subset of the API is supported, and only the most commonly-used parameters of those APIs are supported. -Expect to need to add additional APIs and parameters, either by subclassing this mock or by contributing them back to Safir, when testing a new application. +Expect to need to add additional APIs and parameters, when testing a new application. +Contributions of those additional APIs and parameters will be gratefully reviewed and normally merged. Namespaces are only partially modeled. A namespace can be explicitly created with `~safir.testing.kubernetes.MockKubernetesApi.create_namespace`, in which case the provided ``V1Namespace`` object will be stored and returned by a subsequent `~safir.testing.kubernetes.MockKubernetesApi.read_namespace` or similar call. However, namespace creation is optional. If an object is created in a namespace, that namespace will magically come into existence, and a subsequent `~safir.testing.kubernetes.MockKubernetesApi.list_namespace` or `~safir.testing.kubernetes.MockKubernetesApi.read_namespace` call will return a synthetic namespace object. -Most mock APIs do not support watches. -The only exception is `~safir.testing.kubernetes.MockKubernetesApi.list_namespaced_event` (see :ref:`kubernetes-testing-events`). +When creating Kubernetes watches, the caller will have to pass the expected model type explicitly as the first argument to the constructor of the ``Watch`` object in order to ensure correct deserialization of the raw object when using the mock. +Unfortunately, the type autodetection support in kubernetes_asyncio_ does not work with our mock since it relies on docstring inspection. .. warning:: @@ -140,11 +141,7 @@ If this is any value other than ``Running``, the pod startup event for the names Testing events -------------- -Currently, `~safir.testing.kubernetes.MockKubernetesApi.list_namespaced_event` is the only API that supports watches. -Multiple watchers and timeouts are supported. -The ``field_selector`` parameter is accepted, but is currently ignored. - -The only event that will be posted automatically by the mock is a pod started event when creating a pod with `~safir.testing.kubernetes.MockKubernetesApi.create_namespaced_pod`, provided that the ``initial_pod_phase`` attribute on the mock is set to its default value of ``Running``. +The only event that will be posted automatically by the mock Kubernetes API is a pod started event when creating a pod with `~safir.testing.kubernetes.MockKubernetesApi.create_namespaced_pod`, provided that the ``initial_pod_phase`` attribute on the mock is set to its default value of ``Running``. All other events must be injected manually with `~safir.testing.kubernetes.MockKubernetesApi.create_namespaced_event`. Testing node state @@ -181,6 +178,11 @@ Here is an example of how this function could be used in a test: pod = await mock_kubernetes.read_namespaced_pod("pod", "namespace") data_path = Path(__name__).parent / "data" / "pod.json" expected = json.loads(data_path.read_text()) - assert strip_none(pod.to_dict()) == expected + assert strip_none(pod.to_dict(serialize=True)) == expected The data stored in :file:`tests/data/pod.json` can then contain only the interesting elements of the data model (the ones that are not `None`). + +.. note:: + + As in the above example, consider passing ``serialize=True`` whenever calling the ``to_dict`` method on a Kubernetes model. + This tells the Kubernetes library to use the correct Kubernetes camel-case attribute names rather than the Python snake-case attribute names.