From c358388db6dbcdb773c4938e46475d1ed94037a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Ingebrigtsen=20=C3=98vergaard?= Date: Wed, 2 Nov 2022 15:39:31 +0100 Subject: [PATCH 1/4] Verify current behavior for events in watch_list - Valid events are mapped to WatchEvent which contain the event type and Model instance - Events which do not unmarshal from json string to dict cleanly are dropped (exception is logged) --- k8s/base.py | 3 ++ tests/k8s/test_client.py | 87 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/k8s/base.py b/k8s/base.py index 006823e..7f45524 100644 --- a/k8s/base.py +++ b/k8s/base.py @@ -307,6 +307,9 @@ def __repr__(self): return "{cls}(type={type}, object={object})".format(cls=self.__class__.__name__, type=self.type, object=self.object) + def __eq__(self, other): + return self.type == other.type and self.object == other.object + class LabelSelector(object): """Base for label select operations""" diff --git a/tests/k8s/test_client.py b/tests/k8s/test_client.py index a0dc552..d3cd2b8 100644 --- a/tests/k8s/test_client.py +++ b/tests/k8s/test_client.py @@ -2,13 +2,13 @@ # -*- coding: utf-8 -*- # Copyright 2017-2019 The FIAAS Authors -# +# # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -20,7 +20,8 @@ import pytest from k8s import config -from k8s.base import Model, Field +from k8s.base import Model, WatchEvent +from k8s.fields import Field, RequiredField from k8s.client import Client, SENSITIVE_HEADERS, _session_factory import requests @@ -187,6 +188,83 @@ def test_redacts_sensitive_headers(self, key): assert sensitive_value not in text +@pytest.mark.usefixtures("k8s_config") +class TestWatchListEvents(object): + + def test_watch_list_payload_ok(self, get): + """ + verify watch events of WatchListExample create WatchEvent with the appropriate type and object + """ + response = mock.create_autospec(requests.Response) + response.status_code = 200 + response.iter_lines.return_value = [''' +{ + "type": "ADDED", + "object": { + "value": 1, + "requiredValue": 2 + } +}''', ''' +{ + "type": "MODIFIED", + "object": { + "value": 3, + "requiredValue": 4 + } +} +'''] + get.return_value = response + + expected = [ + _create_watchevent(WatchEvent.ADDED, WatchListExample(value=1, requiredValue=2)), + _create_watchevent(WatchEvent.MODIFIED, WatchListExample(value=3, requiredValue=4)), + ] + + items = list(WatchListExample.watch_list()) + assert items == expected + + def test_watch_list_payload_invalid_json(self, get): + """ + verify event which does not cleanly unmarshal from json to dict is discarded + """ + response = mock.create_autospec(requests.Response) + response.status_code = 200 + response.iter_lines.return_value = [''' +{ + "type": "ADDED", + "object": { + "value": 1, + "requiredValue": 2 + } +} +''', ''' +definitely not valid json +''', ''' +{ + "type": "ADDED", + "object": { + "value": 5, + "requiredValue": 6 + } +}'''] + get.return_value = response + + expected = [ + _create_watchevent(WatchEvent.ADDED, WatchListExample(value=1, requiredValue=2)), + # "definitely not valid json" should be discarded + _create_watchevent(WatchEvent.ADDED, WatchListExample(value=5, requiredValue=6)), + ] + + items = list(WatchListExample.watch_list()) + assert items == expected + + +def _create_watchevent(type, object): + """factory function for WatchEvent to make it easier to create test data from actual objects, as the constructor + takes a dict (unmarshaled json)""" + return WatchEvent({"type": type, "object": object.as_dict()}, object.__class__) + + def _absolute_url(url): return config.api_server + url @@ -199,6 +277,7 @@ class Meta: watch_list_url_template = "/watch/{namespace}/example" value = Field(int) + requiredValue = RequiredField(int) class WatchListExampleUnsupported(Model): From 1cc941a4edc6defe6316aeb443438ea2b1777bcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Ingebrigtsen=20=C3=98vergaard?= Date: Wed, 2 Nov 2022 10:35:23 +0100 Subject: [PATCH 2/4] discard watch event if Model instance can't be created watch_list is a generator function intended to be used as a continuous stream of updates on a resource kind. When it fails to create an instance of the Model type from the payload of a watch event because the resource in the payload is not valid according to the Model class, a TypeError is raised. This stops the stream of watch events and the client has to try again/restart it by calling watch_list again. Since the watch API returns all/most resources on the initial call, if the resource that caused the exception to be raised is still present, the same thing will happen again. In this case the client will not be able to process all resources using watch_list until the invalid resource is removed. With this change, when creating a Model object from the payload of an event raises TypeError, the exception is logged at ERROR level and the event is discarded. This means that clients will be able to process all valid resources if there is one or more resources that are not valid according to the Model type. A drawback is that clients can't set their own error handling for this failure mode. Since watch_list already does the same thing if the payload of an event can not be parsed as JSON, and a watch event without the object on it isn't particularly useful, I think that is probably okay. If it becomes necessary to support custom error handling later, one option might perhaps be to let watch_list take function(s) as optional parameters, which called in the except blocks for these failure modes. The default can be the current behavior (logging the exception). --- k8s/base.py | 9 +++++++-- tests/k8s/test_client.py | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/k8s/base.py b/k8s/base.py index 7f45524..ffe9bdd 100644 --- a/k8s/base.py +++ b/k8s/base.py @@ -137,8 +137,13 @@ def watch_list(cls, namespace=None): if line: try: event_json = json.loads(line) - event = WatchEvent(event_json, cls) - yield event + try: + event = WatchEvent(event_json, cls) + yield event + except TypeError: + LOG.exception( + "Unable to create instance of %s from watch event json, discarding event. event_json=%r", + cls.__name__, event_json) except ValueError: LOG.exception("Unable to parse JSON on watch event, discarding event. Line: %r", line) diff --git a/tests/k8s/test_client.py b/tests/k8s/test_client.py index d3cd2b8..eb1fb82 100644 --- a/tests/k8s/test_client.py +++ b/tests/k8s/test_client.py @@ -258,6 +258,46 @@ def test_watch_list_payload_invalid_json(self, get): items = list(WatchListExample.watch_list()) assert items == expected + def test_watch_list_payload_invalid_object(self, get): + """ + verify event which contains a resource not valid according to the Model class is discarded + """ + response = mock.create_autospec(requests.Response) + response.status_code = 200 + response.iter_lines.return_value = [''' +{ + "type": "ADDED", + "object": { + "value": 1, + "requiredValue": 2 + } +} +''', ''' +{ + "type": "ADDED", + "object": { + "value": 10, + } +} +''', ''' +{ + "type": "ADDED", + "object": { + "value": 5, + "requiredValue": 6 + } +}'''] + get.return_value = response + + expected = [ + _create_watchevent(WatchEvent.ADDED, WatchListExample(value=1, requiredValue=2)), + # event with value=10 and requiredValue missing should be discarded + _create_watchevent(WatchEvent.ADDED, WatchListExample(value=5, requiredValue=6)), + ] + + items = list(WatchListExample.watch_list()) + assert items == expected + def _create_watchevent(type, object): """factory function for WatchEvent to make it easier to create test data from actual objects, as the constructor From 4052f6d06ebc3eced1d84b58ab9506b4bab9d031 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Ingebrigtsen=20=C3=98vergaard?= Date: Fri, 4 Nov 2022 16:57:49 +0100 Subject: [PATCH 3/4] Disable no-self-use pylint warning for test classes --- tests/k8s/test_client.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/k8s/test_client.py b/tests/k8s/test_client.py index eb1fb82..7777cc4 100644 --- a/tests/k8s/test_client.py +++ b/tests/k8s/test_client.py @@ -27,6 +27,7 @@ import requests +# pylint: disable=R0201 @pytest.mark.usefixtures("k8s_config") class TestClient(object): @pytest.fixture @@ -188,6 +189,7 @@ def test_redacts_sensitive_headers(self, key): assert sensitive_value not in text +# pylint: disable=R0201 @pytest.mark.usefixtures("k8s_config") class TestWatchListEvents(object): From 95ae77e4b82a0af091171702973dde55915462f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Ingebrigtsen=20=C3=98vergaard?= Date: Fri, 4 Nov 2022 16:59:08 +0100 Subject: [PATCH 4/4] Avoid redefining builtins --- tests/k8s/test_client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/k8s/test_client.py b/tests/k8s/test_client.py index 7777cc4..2447b12 100644 --- a/tests/k8s/test_client.py +++ b/tests/k8s/test_client.py @@ -301,10 +301,10 @@ def test_watch_list_payload_invalid_object(self, get): assert items == expected -def _create_watchevent(type, object): +def _create_watchevent(event_type, event_object): """factory function for WatchEvent to make it easier to create test data from actual objects, as the constructor takes a dict (unmarshaled json)""" - return WatchEvent({"type": type, "object": object.as_dict()}, object.__class__) + return WatchEvent({"type": event_type, "object": event_object.as_dict()}, event_object.__class__) def _absolute_url(url):