Sort env to reliably expand nested env references #510
Conversation
consideRatio
force-pushed
the
pr/sort-env-smart
branch
from
ff9a79d to
6d6b232
Compare
consideRatio
force-pushed
the
pr/sort-env-smart
branch
from
6d6b232 to
2110d32
Compare
There was a problem hiding this comment.
@yuvipanda @minrk could you have a look at this? I'd like to cut z2jh 1.1.0 with this included in a kubespawner release.
| { | ||
| 'name': 'JUPYTERHUB_SSL_KEYFILE', | ||
| 'value': '/etc/jupyterhub/ssl/ssl.key', | ||
| }, | ||
| { | ||
| 'name': 'JUPYTERHUB_SSL_CERTFILE', | ||
| 'value': '/etc/jupyterhub/ssl/ssl.crt', | ||
| }, | ||
| {'name': 'JUPYTERHUB_USER', 'value': 'TEST'}, | ||
| { | ||
| 'name': 'JUPYTERHUB_SSL_CLIENT_CA', | ||
| 'value': '/etc/jupyterhub/ssl/notebooks-ca_trust.crt', | ||
| }, | ||
| { | ||
| 'name': 'JUPYTERHUB_SSL_KEYFILE', | ||
| 'value': '/etc/jupyterhub/ssl/ssl.key', | ||
| }, | ||
| {'name': 'JUPYTERHUB_USER', 'value': 'TEST'}, |
|
|
||
|
|
||
| def test_make_namespace(): | ||
| labels = { | ||
| 'heritage': 'jupyterhub', | ||
| 'component': 'singleuser-server', | ||
| } | ||
| namespace = api_client.sanitize_for_serialization( | ||
| make_namespace(name='test-namespace', labels=labels) | ||
| ) | ||
| assert namespace == { | ||
| 'metadata': { | ||
| 'annotations': {}, | ||
| 'labels': { | ||
| 'component': 'singleuser-server', | ||
| 'heritage': 'jupyterhub', | ||
| }, | ||
| 'name': 'test-namespace', | ||
| }, | ||
| } |
There was a problem hiding this comment.
This test function was defined twice. Removed.
| def test_make_pod_with_ssl(): | ||
| """ | ||
| Test specification of a pod with ssl enabled | ||
| """ | ||
| assert api_client.sanitize_for_serialization( | ||
| make_pod( | ||
| name='ssl', | ||
| image='jupyter/singleuser:latest', | ||
| env={ | ||
| 'JUPYTERHUB_SSL_KEYFILE': 'TEST_VALUE', | ||
| 'JUPYTERHUB_SSL_CERTFILE': 'TEST', | ||
| 'JUPYTERHUB_USER': 'TEST', | ||
| }, | ||
| working_dir='/', | ||
| cmd=['jupyterhub-singleuser'], | ||
| port=8888, | ||
| image_pull_policy='IfNotPresent', | ||
| ssl_secret_name='ssl', | ||
| ssl_secret_mount_path="/etc/jupyterhub/ssl/", | ||
| ) | ||
| ) == { | ||
| "metadata": { | ||
| "name": "ssl", | ||
| "annotations": {}, | ||
| "labels": {}, | ||
| }, | ||
| "spec": { | ||
| 'automountServiceAccountToken': False, | ||
| "containers": [ | ||
| { | ||
| "env": [ | ||
| { | ||
| 'name': 'JUPYTERHUB_SSL_KEYFILE', | ||
| 'value': '/etc/jupyterhub/ssl/ssl.key', | ||
| }, | ||
| { | ||
| 'name': 'JUPYTERHUB_SSL_CERTFILE', | ||
| 'value': '/etc/jupyterhub/ssl/ssl.crt', | ||
| }, | ||
| {'name': 'JUPYTERHUB_USER', 'value': 'TEST'}, | ||
| { | ||
| 'name': 'JUPYTERHUB_SSL_CLIENT_CA', | ||
| 'value': '/etc/jupyterhub/ssl/notebooks-ca_trust.crt', | ||
| }, | ||
| ], | ||
| "name": "notebook", | ||
| "image": "jupyter/singleuser:latest", | ||
| "imagePullPolicy": "IfNotPresent", | ||
| "args": ["jupyterhub-singleuser"], | ||
| "ports": [{"name": "notebook-port", "containerPort": 8888}], | ||
| 'volumeMounts': [ | ||
| { | ||
| 'mountPath': '/etc/jupyterhub/ssl/', | ||
| 'name': 'jupyterhub-internal-certs', | ||
| } | ||
| ], | ||
| 'workingDir': '/', | ||
| "resources": {"limits": {}, "requests": {}}, | ||
| } | ||
| ], | ||
| 'restartPolicy': 'OnFailure', | ||
| 'volumes': [ | ||
| { | ||
| 'name': 'jupyterhub-internal-certs', | ||
| 'secret': {'defaultMode': 511, 'secretName': 'ssl'}, | ||
| } | ||
| ], | ||
| }, | ||
| "kind": "Pod", | ||
| "apiVersion": "v1", | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
This test function was added twice. Removed this entry.
consideRatio
changed the title
consideRatio
changed the title
| "env_var_key": env_var_key, | ||
| "env_var_obj": env_var_obj, | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this can be simplified with a single key function passed to sorted. To do that, you'll need all transitive dependencies in the deps field, which can be computed after this direct-dependency dict exists. Need to avoid infinite loops with cycles.
There was a problem hiding this comment.
@minrk I gave this a shot but I failed to arrive at something that I felt were about was simpler. I believe that even if I end up going for a pairwise comparison strategy which can have a clean sorted() statement, I perceived myself ending up shifting the complexity to first create a list that contained enough information to be pairwise comparable.
I did refactor my existing code to be simpler to read though. If you think it is acceptable as it is now, I suggest we go for it. I'd also be happy to replace this logic with an alternative implementation if you'd like to provide one, but I'd like that implementation to pass the test cases I've implemented.
consideRatio
force-pushed
the
pr/sort-env-smart
branch
from
b4aa542 to
9b98ac3
Compare
KubeSpawner exposes a dictionary as configuration of environment variables, but a k8s container specification contain an ordered list where the order actually matters.
How the order matters
If an environment variable is set to be a string that contains
$(SOME_ENV), it will be expanded ifSOME_ENVhas been defined earlier in the ordered list in the container specification.What this PR does
This PR makes KubeSpawner emit the container's environment list in a way that first and foremost ensures as many environment variables are expanded as possible.
Is this a breaking change?
In a way yes, but I think most users will think of it as a bugfix - that the changed behavior will always be appreciated.
Related