From c97cc67a870c7b515830980aa142e279187090c2 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 9 Aug 2018 11:04:24 +0200 Subject: [PATCH 1/5] Tooling for k8s-model verifications --- kubespawner/utils.py | 111 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-) diff --git a/kubespawner/utils.py b/kubespawner/utils.py index 52053df9..ba67b25d 100644 --- a/kubespawner/utils.py +++ b/kubespawner/utils.py @@ -2,7 +2,7 @@ Misc. general utility functions, not tied to Kubespawner directly """ import hashlib - +import copy def generate_hashed_slug(slug, limit=63, hash_length=6): """ @@ -27,3 +27,112 @@ def generate_hashed_slug(slug, limit=63, hash_length=6): prefix=slug[:limit - hash_length - 1], hash=slug_hash[:hash_length], ).lower() + + +def update_k8s_model(target, source, logger=None, origin=None): + """ + Takes a model instance such as V1PodSpec() and updates it with another + model representation. The origin parameter could be "extra_pod_config" for + example. + """ + model = type(target) + if not hasattr(target, 'attribute_map'): + raise AttributeError("Attribute 'target' ({}) must be an object (such as 'V1PodSpec') with an attribute 'attribute_map'.".format(model.__name__)) + if not isinstance(source, model) and not isinstance(source, dict): + raise AttributeError("Attribute 'source' ({}) must be an object of the same type as 'target' ({}) or a 'dict'.".format(type(source).__name__, model.__name__)) + + source_dict = _get_k8s_model_dict(model, source) + for key, value in source_dict.items(): + if key not in target.attribute_map: + raise ValueError("The attribute 'source' ({}) contained '{}' not modeled by '{}'.".format(type(source).__name__, key, model.__name__)) + if getattr(target, key): + if logger and origin: + logger.warning("Overriding KubeSpawner.{}'s value '{}' with '{}'.".format(origin, getattr(target, key), value)) + setattr(target, key, value) + + return target + +def get_k8s_model(model, model_dict): + """ + Returns a model object from an model instance or represantative dictionary. + """ + model_dict = copy.deepcopy(model_dict) + + if isinstance(model_dict, model): + return model_dict + elif isinstance(model_dict, dict): + _map_dict_keys_to_model_attributes(model, model_dict) + return model(**model_dict) + else: + raise AttributeError("Expected object of type 'dict' (or '{}') but got '{}'.".format(model.__type__.__name__, model_dict.__type__.__name__)) + +def _get_k8s_model_dict(model, obj): + """ + Returns a model of dictionary kind + """ + obj = copy.deepcopy(obj) + + if isinstance(obj, model): + return obj.to_dict() + elif isinstance(obj, dict): + return _map_dict_keys_to_model_attributes(model, obj) + else: + raise AttributeError("Expected object of type '{}' (or 'dict') but got '{}'.".format(model.__type__.__name__, obj.__type__.__name__)) + +def _map_dict_keys_to_model_attributes(model, model_dict): + """ + Maps a dict's keys to the provided models attributes using its attribute_map + attribute. This is (always?) the same as converting camelCase to snake_case. + Note that the function will not influence nested object's keys. + """ + + for key in list(model_dict.keys()): + model_dict[_get_k8s_model_attribute(model, key)] = model_dict.pop(key) + + return model_dict + +def _get_k8s_model_attribute(model, field_name): + """ + Takes an kubernetes resource field name such as "serviceAccount" and returns + its associated attribute name "service_account" used by the provided + kubernetes.client.models object representing the resource. + + Example of V1PodSpec's attribute_map: + { + 'active_deadline_seconds': 'activeDeadlineSeconds', + 'affinity': 'affinity', + 'automount_service_account_token': 'automountServiceAccountToken', + 'containers': 'containers', + 'dns_policy': 'dnsPolicy', + 'host_aliases': 'hostAliases', + 'host_ipc': 'hostIPC', + 'host_network': 'hostNetwork', + 'host_pid': 'hostPID', + 'hostname': 'hostname', + 'image_pull_secrets': 'imagePullSecrets', + 'init_containers': 'initContainers', + 'node_name': 'nodeName', + 'node_selector': 'nodeSelector', + 'priority': 'priority', + 'priority_class_name': 'priorityClassName', + 'restart_policy': 'restartPolicy', + 'scheduler_name': 'schedulerName', + 'security_context': 'securityContext', + 'service_account': 'serviceAccount', + 'service_account_name': 'serviceAccountName', + 'subdomain': 'subdomain', + 'termination_grace_period_seconds': 'terminationGracePeriodSeconds', + 'tolerations': 'tolerations', + 'volumes': 'volumes' + } + """ + # if we get "service_account", return + if field_name in model.attribute_map: + return field_name + + # if we get "serviceAccount", then return "service_account" + for key, value in model.attribute_map.items(): + if value == field_name: + return key + else: + raise ValueError("'{}' does not model '{}'".format(model.__name__, field_name)) From fb2de10597affad8fec9d4bc50c16570dc76874f Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 9 Aug 2018 11:04:49 +0200 Subject: [PATCH 2/5] Test k8s-model verification tooling --- tests/test_utils.py | 96 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 tests/test_utils.py diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 00000000..996c6b2e --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,96 @@ +import copy +from kubespawner.utils import get_k8s_model, update_k8s_model, _get_k8s_model_attribute +from kubernetes.client.models import ( + V1PodSpec, V1SecurityContext, V1Container, V1Capabilities, V1Lifecycle +) + +class MockLogger(object): + """Trivial class to store logs for inspection after a test run.""" + + def __init__(self): + self.warning_count = 0 + + def warning(self, message): + """Remembers the most recent warning.""" + self.most_recent_warning = message + self.warning_count += 1 + + +def test__get_k8s_model_attribute(): + """Verifies fundamental behavior""" + assert _get_k8s_model_attribute(V1PodSpec, "service_account") == "service_account" + assert _get_k8s_model_attribute(V1PodSpec, "serviceAccount") == "service_account" + +def test_update_k8s_model(): + """Ensure update_k8s_model does what it should. The test is first updating + attributes using the function and then and manually verifies that the + correct changes have been made.""" + manually_updated_target = V1Container( + name="mock_name", + image="mock_image", + command=['iptables'], + security_context=V1SecurityContext( + privileged=True, + run_as_user=0, + capabilities=V1Capabilities(add=['NET_ADMIN']) + ) + ) + target = copy.deepcopy(manually_updated_target) + source = {"name": "new_mock_name"} + update_k8s_model(target, source) + + manually_updated_target.name = "new_mock_name" + + assert target == manually_updated_target + +def test_update_k8s_models_logger_warning(): + """Ensure that the update_k8s_model function uses the logger to warn about + overwriting previous values.""" + target = V1Container( + name="mock_name" + ) + source = {"name": "new_mock_name", "image_pull_policy": "Always"} + mock_locker = MockLogger() + update_k8s_model(target, source, logger=mock_locker, origin="test-runner") + + assert mock_locker.most_recent_warning.find("KubeSpawner.test-runner's value 'mock_name' with 'new_mock_name'") + assert mock_locker.warning_count == 1 + + +def test_get_k8s_model(): + """Thest that passing either a kubernetes.client.models object or as a + dictionary to representing it get_k8s_model should work.""" + # verify get_k8s_model for when passing dict objects + v1_lifecycle_from_dict = get_k8s_model( + V1Lifecycle, + { + 'preStop': { + 'exec': { + 'command': ['/bin/sh', 'test'] + } + } + }, + ) + + assert isinstance(v1_lifecycle_from_dict, V1Lifecycle) + assert v1_lifecycle_from_dict.to_dict() == { + 'post_start': None, + 'pre_stop': { + 'exec': { + 'command': ['/bin/sh', 'test'] + } + }, + } + + # verify get_k8s_model for when passing model objects + v1_lifecycle_from_model_object = get_k8s_model(V1Lifecycle, v1_lifecycle_from_dict) + + assert isinstance(v1_lifecycle_from_model_object, V1Lifecycle) + assert v1_lifecycle_from_model_object.to_dict() == { + 'post_start': None, + 'pre_stop': { + 'exec': { + 'command': ['/bin/sh', 'test'] + } + }, + } From c44ee1af02f8f1c70e0d0db2373f195261d9b8d3 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 9 Aug 2018 14:33:55 +0200 Subject: [PATCH 3/5] Improved docstrings and parameter names --- kubespawner/utils.py | 92 ++++++++++++++++++++++++++------------------ tests/test_utils.py | 2 +- 2 files changed, 56 insertions(+), 38 deletions(-) diff --git a/kubespawner/utils.py b/kubespawner/utils.py index ba67b25d..e93ce186 100644 --- a/kubespawner/utils.py +++ b/kubespawner/utils.py @@ -29,73 +29,91 @@ def generate_hashed_slug(slug, limit=63, hash_length=6): ).lower() -def update_k8s_model(target, source, logger=None, origin=None): +def update_k8s_model(target, changes, logger=None, target_name=None): """ Takes a model instance such as V1PodSpec() and updates it with another - model representation. The origin parameter could be "extra_pod_config" for - example. + model, which is allowed to be a dict or another model instance of the same + type. The logger is used to warn if any truthy value in the target is is + overridden. The target_name parameter can for example be "extra_pod_config", + allowing the logger to write out something more meaningful to the user. """ - model = type(target) + model_type = type(target) if not hasattr(target, 'attribute_map'): - raise AttributeError("Attribute 'target' ({}) must be an object (such as 'V1PodSpec') with an attribute 'attribute_map'.".format(model.__name__)) - if not isinstance(source, model) and not isinstance(source, dict): - raise AttributeError("Attribute 'source' ({}) must be an object of the same type as 'target' ({}) or a 'dict'.".format(type(source).__name__, model.__name__)) + raise AttributeError("Attribute 'target' ({}) must be an object (such as 'V1PodSpec') with an attribute 'attribute_map'.".format(model_type.__name__)) + if not isinstance(changes, model_type) and not isinstance(changes, dict): + raise AttributeError("Attribute 'changes' ({}) must be an object of the same type as 'target' ({}) or a 'dict'.".format(type(changes).__name__, model_type.__name__)) - source_dict = _get_k8s_model_dict(model, source) - for key, value in source_dict.items(): + changes_dict = _get_k8s_model_dict(model_type, changes) + for key, value in changes_dict.items(): if key not in target.attribute_map: - raise ValueError("The attribute 'source' ({}) contained '{}' not modeled by '{}'.".format(type(source).__name__, key, model.__name__)) - if getattr(target, key): - if logger and origin: - logger.warning("Overriding KubeSpawner.{}'s value '{}' with '{}'.".format(origin, getattr(target, key), value)) - setattr(target, key, value) + raise ValueError("The attribute 'changes' ({}) contained '{}' not modeled by '{}'.".format(type(changes).__name__, key, model_type.__name__)) + + + # If changes are passed as a dict, they will only have a few keys/value + # pairs representing the specific changes. If the changes parameter is a + # model instance on the other hand, the changes parameter will have a + # lot of default values as well. These default values, which are also + # falsy, should not use to override the target's values. + if isinstance(changes, dict) or value: + if getattr(target, key): + if logger and target_name: + logger.warning("Overriding KubeSpawner.{}'s value '{}' with '{}'.".format(target_name, getattr(target, key), value)) + setattr(target, key, value) return target -def get_k8s_model(model, model_dict): +def get_k8s_model(model_type, model_dict): """ - Returns a model object from an model instance or represantative dictionary. + Returns an instance of type specified model_type from an model instance or + represantative dictionary. """ model_dict = copy.deepcopy(model_dict) - if isinstance(model_dict, model): + if isinstance(model_dict, model_type): return model_dict elif isinstance(model_dict, dict): - _map_dict_keys_to_model_attributes(model, model_dict) - return model(**model_dict) + # convert the dictionaries camelCase keys to snake_case keys + _map_dict_keys_to_model_attributes(model_type, model_dict) + # use the dictionary keys to initialize a model of given type + return model_type(**model_dict) else: - raise AttributeError("Expected object of type 'dict' (or '{}') but got '{}'.".format(model.__type__.__name__, model_dict.__type__.__name__)) + raise AttributeError("Expected object of type 'dict' (or '{}') but got '{}'.".format(model_type.__name__, type(model_dict).__name__)) -def _get_k8s_model_dict(model, obj): +def _get_k8s_model_dict(model_type, model): """ - Returns a model of dictionary kind + Returns a dictionary representation of a provided model type """ - obj = copy.deepcopy(obj) + model = copy.deepcopy(model) - if isinstance(obj, model): - return obj.to_dict() - elif isinstance(obj, dict): - return _map_dict_keys_to_model_attributes(model, obj) + if isinstance(model, model_type): + return model.to_dict() + elif isinstance(model, dict): + return _map_dict_keys_to_model_attributes(model_type, model) else: - raise AttributeError("Expected object of type '{}' (or 'dict') but got '{}'.".format(model.__type__.__name__, obj.__type__.__name__)) + raise AttributeError("Expected object of type '{}' (or 'dict') but got '{}'.".format(model_type.__name__, type(model).__name__)) -def _map_dict_keys_to_model_attributes(model, model_dict): +def _map_dict_keys_to_model_attributes(model_type, model_dict): """ Maps a dict's keys to the provided models attributes using its attribute_map attribute. This is (always?) the same as converting camelCase to snake_case. Note that the function will not influence nested object's keys. """ + # it is important to iterate over a copy of the dictionaries keys, as they + # will be updated in the loop for key in list(model_dict.keys()): - model_dict[_get_k8s_model_attribute(model, key)] = model_dict.pop(key) + model_dict[_get_k8s_model_attribute(model_type, key)] = model_dict.pop(key) return model_dict -def _get_k8s_model_attribute(model, field_name): +def _get_k8s_model_attribute(model_type, field_name): """ - Takes an kubernetes resource field name such as "serviceAccount" and returns - its associated attribute name "service_account" used by the provided - kubernetes.client.models object representing the resource. + Takes a model type and a Kubernetes API resource field name (such as + "serviceAccount") and returns a related attribute name (such as + "service_account") to be used with kubernetes.client.models objects. It is + impossible to prove a negative but it seems like it is always a question of + making camelCase to snake_case but by using the provided 'attribute_map' we + also ensure that the fields actually exist. Example of V1PodSpec's attribute_map: { @@ -127,12 +145,12 @@ def _get_k8s_model_attribute(model, field_name): } """ # if we get "service_account", return - if field_name in model.attribute_map: + if field_name in model_type.attribute_map: return field_name # if we get "serviceAccount", then return "service_account" - for key, value in model.attribute_map.items(): + for key, value in model_type.attribute_map.items(): if value == field_name: return key else: - raise ValueError("'{}' does not model '{}'".format(model.__name__, field_name)) + raise ValueError("'{}' did not have an attribute matching '{}'".format(model_type.__name__, field_name)) diff --git a/tests/test_utils.py b/tests/test_utils.py index 996c6b2e..98d2b0b7 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -51,7 +51,7 @@ def test_update_k8s_models_logger_warning(): ) source = {"name": "new_mock_name", "image_pull_policy": "Always"} mock_locker = MockLogger() - update_k8s_model(target, source, logger=mock_locker, origin="test-runner") + update_k8s_model(target, source, logger=mock_locker, target_name="test-runner") assert mock_locker.most_recent_warning.find("KubeSpawner.test-runner's value 'mock_name' with 'new_mock_name'") assert mock_locker.warning_count == 1 From 3102c8f0a0966388e5f95d23063483669c40920a Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 10 Aug 2018 15:41:10 +0200 Subject: [PATCH 4/5] Don't mutate args in _map_attributes... function Thank you @minrk for helping me! --- kubespawner/utils.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/kubespawner/utils.py b/kubespawner/utils.py index e93ce186..2c17a36a 100644 --- a/kubespawner/utils.py +++ b/kubespawner/utils.py @@ -73,7 +73,7 @@ def get_k8s_model(model_type, model_dict): return model_dict elif isinstance(model_dict, dict): # convert the dictionaries camelCase keys to snake_case keys - _map_dict_keys_to_model_attributes(model_type, model_dict) + model_dict = _map_dict_keys_to_model_attributes(model_type, model_dict) # use the dictionary keys to initialize a model of given type return model_type(**model_dict) else: @@ -99,12 +99,11 @@ def _map_dict_keys_to_model_attributes(model_type, model_dict): Note that the function will not influence nested object's keys. """ - # it is important to iterate over a copy of the dictionaries keys, as they - # will be updated in the loop - for key in list(model_dict.keys()): - model_dict[_get_k8s_model_attribute(model_type, key)] = model_dict.pop(key) + new_dict = {} + for key, value in model_dict.items(): + new_dict[_get_k8s_model_attribute(model_type, key)] = value - return model_dict + return new_dict def _get_k8s_model_attribute(model_type, field_name): """ From a2cb445a037472c285b08dfcbbb0a17c7e432127 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 10 Aug 2018 16:19:30 +0200 Subject: [PATCH 5/5] Improved log messages from update_k8s_model --- kubespawner/utils.py | 20 +++++++++++++++----- tests/test_utils.py | 4 ++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/kubespawner/utils.py b/kubespawner/utils.py index 2c17a36a..6d93d655 100644 --- a/kubespawner/utils.py +++ b/kubespawner/utils.py @@ -29,13 +29,15 @@ def generate_hashed_slug(slug, limit=63, hash_length=6): ).lower() -def update_k8s_model(target, changes, logger=None, target_name=None): +def update_k8s_model(target, changes, logger=None, target_name=None, changes_name=None): """ Takes a model instance such as V1PodSpec() and updates it with another model, which is allowed to be a dict or another model instance of the same type. The logger is used to warn if any truthy value in the target is is - overridden. The target_name parameter can for example be "extra_pod_config", - allowing the logger to write out something more meaningful to the user. + overridden. The target_name parameter can for example be "pod.spec", and + changes_name parameter could be "extra_pod_config". These parameters allows + the logger to write out something more meaningful to the user whenever + something is about to become overridden. """ model_type = type(target) if not hasattr(target, 'attribute_map'): @@ -56,8 +58,16 @@ def update_k8s_model(target, changes, logger=None, target_name=None): # falsy, should not use to override the target's values. if isinstance(changes, dict) or value: if getattr(target, key): - if logger and target_name: - logger.warning("Overriding KubeSpawner.{}'s value '{}' with '{}'.".format(target_name, getattr(target, key), value)) + if logger and changes_name: + warning = "'{}.{}' current value: '{}' is overridden with '{}', which is the value of '{}.{}'.".format( + target_name, + key, + getattr(target, key), + value, + changes_name, + key + ) + logger.warning(warning) setattr(target, key, value) return target diff --git a/tests/test_utils.py b/tests/test_utils.py index 98d2b0b7..c8855065 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -51,9 +51,9 @@ def test_update_k8s_models_logger_warning(): ) source = {"name": "new_mock_name", "image_pull_policy": "Always"} mock_locker = MockLogger() - update_k8s_model(target, source, logger=mock_locker, target_name="test-runner") + update_k8s_model(target, source, logger=mock_locker, target_name="notebook_container", changes_name="extra_container_config") - assert mock_locker.most_recent_warning.find("KubeSpawner.test-runner's value 'mock_name' with 'new_mock_name'") + assert mock_locker.most_recent_warning.find("'notebook_container.name' current value: 'mock_name' is overridden with 'new_mock_name', which is the value of 'extra_container_config.name'") != -1 assert mock_locker.warning_count == 1