-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Part 3 of #205] Added utils for k8s model verification #232
Changes from 3 commits
c97cc67
fb2de10
c44ee1a
3102c8f
a2cb445
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,130 @@ 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, changes, logger=None, target_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. | ||
""" | ||
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_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__)) | ||
|
||
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 '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_type, model_dict): | ||
""" | ||
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_type): | ||
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) | ||
# 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__, type(model_dict).__name__)) | ||
|
||
def _get_k8s_model_dict(model_type, model): | ||
""" | ||
Returns a dictionary representation of a provided model type | ||
""" | ||
model = copy.deepcopy(model) | ||
|
||
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__, type(model).__name__)) | ||
|
||
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_type, key)] = model_dict.pop(key) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should create a new dictionary instead of mutating one in-place There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for considering this @minrk. I'm intending to use this helper twice as written out below, this code is from PR Part 4. # the assignments to notebook_container and pod.spec is pointless atm, but won't be if I update the utility function update_k8s_model to be get_updated_copy_of_k8s_model or similar.
if extra_container_config:
notebook_container = update_k8s_model(
target=notebook_container,
changes=extra_container_config,
logger=logger,
target_name="extra_container_config",
)
# ...
if extra_pod_config:
pod.spec = update_k8s_model(
target=pod.spec,
changes=extra_pod_config,
logger=logger,
target_name="extra_pod_config",
) I reasoned that we are already making a lot of changes to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I trust your experience on this, so I'll go with whatever you think is best and be right on it, the change won't be causing much work at all if we do it! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm... |
||
|
||
return model_dict | ||
|
||
def _get_k8s_model_attribute(model_type, field_name): | ||
""" | ||
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: | ||
{ | ||
'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_type.attribute_map: | ||
return field_name | ||
|
||
# if we get "serviceAccount", then return "service_account" | ||
for key, value in model_type.attribute_map.items(): | ||
if value == field_name: | ||
return key | ||
else: | ||
raise ValueError("'{}' did not have an attribute matching '{}'".format(model_type.__name__, field_name)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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, 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 | ||
|
||
|
||
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'] | ||
} | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since i modify the model_dict while looping over its keys, I better loop over a copy of the keys rather than evaluating then while modifying them. So, I'm creating a new list from model_dict.keys() by passing it to list(), perhaps i should use copy() to make it more obvious?