From 55bbdb6f429c679572d5439cbf4cfbd9447c38a6 Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Thu, 17 Nov 2022 16:59:32 +0100 Subject: [PATCH 1/4] refactor: improve git credential handling in Kubernetes The git credentials are now stored in a temporary secret following recommended best practice. The secret will have the same lifespan as the builder pod. --- binderhub/build.py | 63 +++++++++++++++++++++++++---------- binderhub/tests/test_build.py | 11 ++++-- 2 files changed, 53 insertions(+), 21 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index 50d683792..a48c6eca1 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -2,6 +2,7 @@ Contains build of a docker image from a git repository. """ +import base64 import datetime import json import os @@ -382,8 +383,24 @@ def submit(self): env = [] if self.git_credentials: + secret_content = base64.b64encode( + self.git_credentials.encode("utf-8") + ).decode("utf-8") + data = {"credentials": secret_content} + + secret = client.V1Secret() + secret.data = data + secret.metadata = {"name": self.name} + secret.type = "Opaque" + + self.api.create_namespaced_secret(self.namespace, secret) + + secret_key_ref = client.V1SecretKeySelector( + name=self.name, key="credentials", optional=False + ) + value_from = client.V1EnvVarSource(secret_key_ref=secret_key_ref) env.append( - client.V1EnvVar(name="GIT_CREDENTIAL_ENV", value=self.git_credentials) + client.V1EnvVar(name="GIT_CREDENTIAL_ENV", value_from=value_from) ) self.pod = client.V1Pod( @@ -515,10 +532,9 @@ def submit(self): f"Found unknown phase {phase} when building {self.name}" ) - if self.pod.status.phase == "Succeeded": - self.cleanup() - elif self.pod.status.phase == "Failed": + if self.pod.status.phase in ["Succeeded", "Failed"]: self.cleanup() + except Exception: app_log.exception("Error in watch stream for %s", self.name) raise @@ -568,21 +584,32 @@ def stream_logs(self): def cleanup(self): """ - Delete the kubernetes build pod + Delete the kubernetes build pod and secret if exists """ - try: - self.api.delete_namespaced_pod( - name=self.name, - namespace=self.namespace, - body=client.V1DeleteOptions(grace_period_seconds=0), - _request_timeout=KUBE_REQUEST_TIMEOUT, - ) - except client.rest.ApiException as e: - if e.status == 404: - # Is ok, someone else has already deleted it - pass - else: - raise + + exceptions = [] + deletion_methods = [self.api.delete_namespaced_pod] + + if self.git_credentials: + deletion_methods.append(self.api.delete_namespaced_secret) + + for deletion_method in deletion_methods: + try: + deletion_method( + name=self.name, + namespace=self.namespace, + body=client.V1DeleteOptions(grace_period_seconds=0), + _request_timeout=KUBE_REQUEST_TIMEOUT, + ) + except client.rest.ApiException as e: + if e.status == 404: + # Is ok, someone else has already deleted it + pass + else: + exceptions.append(str(e)) + + if exceptions: + raise RuntimeError("Error(s) occurred during cleanup", exceptions) class KubernetesCleaner(LoggingConfigurable): diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index ba13ab83b..6fe626538 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -236,9 +236,14 @@ def test_git_credentials_passed_to_podspec_upon_submit(): assert len(pod.spec.containers) == 1 - env = {env_var.name: env_var.value for env_var in pod.spec.containers[0].env} - - assert env["GIT_CREDENTIAL_ENV"] == git_credentials + env = {env_var.name: env_var.value_from for env_var in pod.spec.containers[0].env} + + credentials_value = env["GIT_CREDENTIAL_ENV"] + assert isinstance(credentials_value, client.V1EnvVarSource) + assert isinstance(credentials_value.secret_key_ref, client.V1SecretKeySelector) + assert credentials_value.secret_key_ref.key == "credentials" + assert credentials_value.secret_key_ref.name == "test_build" + assert credentials_value.secret_key_ref.optional == False async def test_local_repo2docker_build(): From b5eef26a392445dbc7a1c72d6ccdd8ccd921ddda Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Fri, 18 Nov 2022 11:12:35 +0100 Subject: [PATCH 2/4] refactor: using stringData field to setup secret This makes the code simpler and lets k8s encode the data --- binderhub/build.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index a48c6eca1..a215f79f8 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -2,7 +2,6 @@ Contains build of a docker image from a git repository. """ -import base64 import datetime import json import os @@ -383,13 +382,8 @@ def submit(self): env = [] if self.git_credentials: - secret_content = base64.b64encode( - self.git_credentials.encode("utf-8") - ).decode("utf-8") - data = {"credentials": secret_content} - secret = client.V1Secret() - secret.data = data + secret.string_data = {"credentials": self.git_credentials} secret.metadata = {"name": self.name} secret.type = "Opaque" From 274e5257d477f28829ea8ec5e9691f40717db00b Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Mon, 5 Dec 2022 09:21:52 +0100 Subject: [PATCH 3/4] refactor: make the pod owner of the secret once created --- binderhub/build.py | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index a215f79f8..60f73599c 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -382,10 +382,17 @@ def submit(self): env = [] if self.git_credentials: - secret = client.V1Secret() - secret.string_data = {"credentials": self.git_credentials} - secret.metadata = {"name": self.name} - secret.type = "Opaque" + secret = client.V1Secret( + metadata=client.V1ObjectMeta( + name=self.name, + labels={ + "name": self.name, + "component": self._component_label, + }, + ), + string_data={"credentials": self.git_credentials}, + type="Opaque", + ) self.api.create_namespaced_secret(self.namespace, secret) @@ -497,6 +504,24 @@ def submit(self): # https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-phase phase = self.pod.status.phase if phase == "Pending": + if self.git_credentials: + owner_reference = client.V1OwnerReference( + api_version="v1", + kind="Pod", + name=self.pod.metadata.name, + uid=self.pod.metadata.uid, + ) + self.api.patch_namespaced_secret( + namespace=self.namespace, + name=self.pod.metadata.name, + body=[ + { + "op": "replace", + "path": "/metadata/ownerReferences", + "value": [owner_reference], + } + ], + ) self.progress( ProgressEvent.Kind.BUILD_STATUS_CHANGE, ProgressEvent.BuildStatus.PENDING, From a81abe3e1547b3a9024f09e3e1ce31b7eedca7d1 Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Mon, 5 Dec 2022 17:17:21 +0100 Subject: [PATCH 4/4] refactor: move the secret creation after the pod is created The final implementation comes from the following design decision: Only create the secret once the pod is ready so we reduce the surface were sensitive data is managed and avoid its management if the pod fails for some reason. --- binderhub/build.py | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index 60f73599c..0f6de094d 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -382,20 +382,6 @@ def submit(self): env = [] if self.git_credentials: - secret = client.V1Secret( - metadata=client.V1ObjectMeta( - name=self.name, - labels={ - "name": self.name, - "component": self._component_label, - }, - ), - string_data={"credentials": self.git_credentials}, - type="Opaque", - ) - - self.api.create_namespaced_secret(self.namespace, secret) - secret_key_ref = client.V1SecretKeySelector( name=self.name, key="credentials", optional=False ) @@ -511,16 +497,21 @@ def submit(self): name=self.pod.metadata.name, uid=self.pod.metadata.uid, ) - self.api.patch_namespaced_secret( - namespace=self.namespace, - name=self.pod.metadata.name, - body=[ - { - "op": "replace", - "path": "/metadata/ownerReferences", - "value": [owner_reference], - } - ], + secret = client.V1Secret( + metadata=client.V1ObjectMeta( + name=self.name, + labels={ + "name": self.name, + "component": self._component_label, + }, + owner_references=[owner_reference], + ), + string_data={"credentials": self.git_credentials}, + type="Opaque", + ) + + self.api.create_namespaced_secret( + self.namespace, secret ) self.progress( ProgressEvent.Kind.BUILD_STATUS_CHANGE,