Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Store provided environment of key/value kind into a k8s Secret owned by the spawned pod #471

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Store provided environment of key/value kind into a k8s Secret owned by the spawned pod #471

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69e7fcc
read env from secrets
tirumerla Dec 23, 2020
ce9eb63
fixes to allow valueFrom for env. variables
tirumerla Jan 6, 2021
76eb430
fix name, empty array & remove tuple
tirumerla Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 8 additions & 46 deletions kubespawner/objects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import os
import re
from urllib.parse import urlparse

from kubernetes.client.models import (
V1Affinity,
V1Container,
Expand Down Expand Up @@ -36,6 +35,8 @@
V1PreferredSchedulingTerm,
V1ResourceRequirements,
V1Secret,
V1EnvFromSource,
V1SecretEnvSource,
V1SecurityContext,
V1Service,
V1ServicePort,
Expand Down Expand Up @@ -63,6 +64,7 @@ def make_pod(
run_privileged=False,
allow_privilege_escalation=True,
env=None,
env_from=None,
working_dir=None,
volumes=None,
volume_mounts=None,
Expand Down Expand Up @@ -266,6 +268,7 @@ def make_pod(

pod.spec = V1PodSpec(containers=[])
pod.spec.restart_policy = 'OnFailure'


if image_pull_secrets is not None:
# image_pull_secrets as received by the make_pod function should always
Expand All @@ -288,12 +291,6 @@ def make_pod(
}
)

env['JUPYTERHUB_SSL_KEYFILE'] = ssl_secret_mount_path + "ssl.key"
env['JUPYTERHUB_SSL_CERTFILE'] = ssl_secret_mount_path + "ssl.crt"
env['JUPYTERHUB_SSL_CLIENT_CA'] = (
ssl_secret_mount_path + "notebooks-ca_trust.crt"
)

if not volume_mounts:
volume_mounts = []
volume_mounts.append(
Expand Down Expand Up @@ -340,24 +337,13 @@ def make_pod(
if all([e is None for e in container_security_context.to_dict().values()]):
container_security_context = None

# Transform a dict into valid Kubernetes EnvVar Python representations. This
# representation shall always have a "name" field as well as either a
# "value" field or "value_from" field. For examples see the
# test_make_pod_with_env function.
prepared_env = []
for k, v in (env or {}).items():
if type(v) == dict:
if not "name" in v:
v["name"] = k
prepared_env.append(get_k8s_model(V1EnvVar, v))
else:
prepared_env.append(V1EnvVar(name=k, value=v))
notebook_container = V1Container(
name='notebook',
image=image,
working_dir=working_dir,
ports=[V1ContainerPort(name='notebook-port', container_port=port)],
env=prepared_env,
env=env,
env_from=[V1EnvFromSource(secret_ref=V1SecretEnvSource(env_from))],
args=cmd,
image_pull_policy=image_pull_policy,
lifecycle=lifecycle_hooks,
Expand Down Expand Up @@ -671,9 +657,8 @@ def make_owner_reference(name, uid):

def make_secret(
name,
str_data,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let this be named string_data to not create a deviation from the naming convention of k8s secret resources.

username,
cert_paths,
hub_ca,
owner_references,
labels=None,
annotations=None,
Expand All @@ -688,10 +673,6 @@ def make_secret(
going to be created in.
username:
The name of the user notebook.
cert_paths:
JupyterHub spawners cert_paths dictionary container certificate path references
hub_ca:
Path to the hub certificate authority
labels:
Labels to add to the secret.
annotations:
Expand All @@ -706,26 +687,7 @@ def make_secret(
secret.metadata.annotations = (annotations or {}).copy()
secret.metadata.labels = (labels or {}).copy()
secret.metadata.owner_references = owner_references

secret.data = {}

with open(cert_paths['keyfile'], 'r') as file:
encoded = base64.b64encode(file.read().encode("utf-8"))
secret.data['ssl.key'] = encoded.decode("utf-8")

with open(cert_paths['certfile'], 'r') as file:
encoded = base64.b64encode(file.read().encode("utf-8"))
secret.data['ssl.crt'] = encoded.decode("utf-8")

with open(cert_paths['cafile'], 'r') as file:
encoded = base64.b64encode(file.read().encode("utf-8"))
secret.data["notebooks-ca_trust.crt"] = encoded.decode("utf-8")

with open(hub_ca, 'r') as file:
encoded = base64.b64encode(file.read().encode("utf-8"))
secret.data["notebooks-ca_trust.crt"] = secret.data[
"notebooks-ca_trust.crt"
] + encoded.decode("utf-8")
secret.string_data = str_data

return secret

Expand Down
117 changes: 81 additions & 36 deletions kubespawner/spawner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import asyncio
import json
import base64
import multiprocessing
import os
import string
Expand All @@ -27,6 +28,8 @@
from jupyterhub.spawner import Spawner
from jupyterhub.traitlets import Command
from jupyterhub.utils import exponential_backoff
from kubespawner.utils import get_k8s_model
from kubernetes.client.models import V1EnvVar
from kubernetes import client
from kubernetes.client.rest import ApiException
from slugify import slugify
Expand Down Expand Up @@ -1624,7 +1627,8 @@ async def get_pod_manifest(self):
supplemental_gids=supplemental_gids,
run_privileged=self.privileged,
allow_privilege_escalation=self.allow_privilege_escalation,
env=self.get_env(),
env=self.get_env()[1],
env_from=self.secret_name,
volumes=self._expand_all(self.volumes),
volume_mounts=self._expand_all(self.volume_mounts),
working_dir=self.working_dir,
Expand Down Expand Up @@ -1665,12 +1669,11 @@ def get_secret_manifest(self, owner_reference):
annotations = self._build_common_annotations(
self._expand_all(self.extra_annotations)
)

return make_secret(
name=self.secret_name,
str_data=self.get_env()[0],
username=self.user.name,
cert_paths=self.cert_paths,
hub_ca=self.internal_trust_bundles['hub-ca'],
owner_references=[owner_reference],
labels=labels,
annotations=annotations,
Expand Down Expand Up @@ -1768,11 +1771,51 @@ def get_env(self):
"""

env = super(KubeSpawner, self).get_env()

"""Separate env. variables into two dicts
- Dict containing only "valueFrom" env. variables, these are passed as-is.
- replace existing env dict with only "value" env. varaibles, these are passed into secret.
"""
prepared_env = []
# create a separate dict for all "valueFrom" environment variables
extra_env = {k: v for k, v in (env or {}).items() if type(v) == dict}
# Replace existing env dict without "valueFrom" env. variables and pass it to secret
env = {k: v for k, v in (env or {}).items() if type(v) != dict}
for k, v in (extra_env or {}).items():
if not "name" in v:
v["name"] = k
extra_env[k] = v
prepared_env.append(get_k8s_model(V1EnvVar, v))

# deprecate image
env['JUPYTER_IMAGE_SPEC'] = self.image
env['JUPYTER_IMAGE'] = self.image

return env

if self.internal_ssl:
"""
cert_paths:
certificate path references
hub_ca:
Path to the hub certificate authority
"""
with open(self.cert_paths['keyfile'], 'r') as file:
env['ssl.key'] = file.read()

with open(self.cert_paths['certfile'], 'r') as file:
env['ssl.crt'] = file.read()

with open(self.cert_paths['cafile'], 'r') as file:
env["notebooks-ca_trust.crt"] = file.read()

with open(self.internal_trust_bundles['hub-ca'], 'r') as file:
env["notebooks-ca_trust.crt"] = env[
"notebooks-ca_trust.crt"
] + file.read()
env['JUPYTERHUB_SSL_KEYFILE'] = self.secret_mount_path + "ssl.key"
env['JUPYTERHUB_SSL_CERTFILE'] = self.secret_mount_path + "ssl.crt"
env['JUPYTERHUB_SSL_CLIENT_CA'] = (self.secret_mount_path + "notebooks-ca_trust.crt")

return env, prepared_env
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider introducing get_env_from alongside get_env here, I dislike the complexity we embed in this by returning a tuple.

I think prepared_env also should be named more appropriately now that it means specifically env_from.

The Container object has env and envFrom, both being arrays, but of the kind EnvVar and EnvFromSource.

I'd love for this source code to be simpler to understand rather than more complicated, of course adding this feature will make it more complicated no matter what, but how to minimize the complexity of understanding this code in general in the future is a focus of mine.

I have no concrete suggested action =/ but thats what goes in my mind while looking at this code base which I have struggled to understand in the past and think is quite hard to grasp still.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider introducing get_env_from alongside get_env here, I dislike the complexity we embed in this by returning a tuple.

Yes, this is a good point. I thought about this, creating a new get_env_from just for env. variables using "valueFrom" but in the moment i went with getting something up quicker rather than simpler :). You're right, i will take a look at this to make it simpler.


def load_state(self, state):
"""
Expand Down Expand Up @@ -2182,34 +2225,35 @@ async def _start(self):
timeout=self.k8s_api_request_retry_timeout
)

if self.internal_ssl:
try:
# wait for pod to have uid,
# required for creating owner reference
await exponential_backoff(
lambda: self.pod_has_uid(
self.pod_reflector.pods.get(self.pod_name, None)
),
"pod/%s does not have a uid!" % (self.pod_name),
)
try:
# wait for pod to have uid,
# required for creating owner reference
await exponential_backoff(
lambda: self.pod_has_uid(
self.pod_reflector.pods.get(self.pod_name, None)
),
"pod/%s does not have a uid!" % (self.pod_name),
)

pod = self.pod_reflector.pods[self.pod_name]
owner_reference = make_owner_reference(
self.pod_name, pod["metadata"]["uid"]
)
pod = self.pod_reflector.pods[self.pod_name]
owner_reference = make_owner_reference(
self.pod_name, pod["metadata"]["uid"]
)

# internal ssl, create secret object
secret_manifest = self.get_secret_manifest(owner_reference)
await exponential_backoff(
partial(
self._ensure_not_exists, "secret", secret_manifest.metadata.name
),
f"Failed to delete secret {secret_manifest.metadata.name}",
)
await exponential_backoff(
partial(self._make_create_resource_request, "secret", secret_manifest),
f"Failed to create secret {secret_manifest.metadata.name}",
)
# create secret object
secret_manifest = self.get_secret_manifest(owner_reference)
await exponential_backoff(
partial(
self._ensure_not_exists, "secret", secret_manifest.metadata.name
),
f"Failed to delete secret {secret_manifest.metadata.name}",
)
await exponential_backoff(
partial(self._make_create_resource_request, "secret", secret_manifest),
f"Failed to create secret {secret_manifest.metadata.name}",
)

if self.internal_ssl:

service_manifest = self.get_service_manifest(owner_reference)
await exponential_backoff(
Expand All @@ -2224,10 +2268,11 @@ async def _start(self):
),
f"Failed to create service {service_manifest.metadata.name}",
)
except Exception:
# cleanup on failure and re-raise
await self.stop(True)
raise

except Exception:
# cleanup on failure and re-raise
await self.stop(True)
raise

# we need a timeout here even though start itself has a timeout
# in order for this coroutine to finish at some point.
Expand Down
Loading