build_docker_config added, enables augmentation of the build pod's docker config

[g-braeunlich]

PR Summary by Erik

The BinderHub configuration build_docker_config is added to allow an augmentation of the build pods docker config (config.json) that previously have only contained container registry credentials when use_registry was set to to True.

A motivation for adding this configuration option is to enable the ability to configure network proxy settings for docker, so for example traffic going to download an apt package during the build process is done through a proxy. Such configuration is documented here for example.

This PR does not introduce any breaking changes, but it does rename a k8s Secret (managed directly by the Helm chart) from binder-push-secret to binder-build-docker-config. By doing so, a build pod starting up in the wrong moment could get stuck in pending by referencing the old name but not being able to mount it. If a pod is already running everything will be fine though, the content mounted won't be deleted just because the mounted k8s Secret was deleted.

I think this PR makes sense, is well scoped, and while could cause a disruption during a very brief moment in time for new builds does the right thing to rename the k8s Secret.

---

Original PR summary

This is the build pod part of the PR #990
It will allow to build images with repo2docker behind a proxy.
Prerequisite:
jupyterhub/repo2docker#1003 (review)

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[meeseeksmachine]

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/binder-behind-outbound-proxy/7428/5

[g-braeunlich]

Just found out:
if push_secret is set, then this will mount a read only secret over .docker/config.json
This will prevent the proxy functionality

[manics]

Looks liek these are the relevant lines:

binderhub/binderhub/build.py

Lines 248 to 253 in e2f23e9

	if self.push_secret:
	volume_mounts.append(client.V1VolumeMount(mount_path="/root/.docker", name='docker-push-secret'))
	volumes.append(client.V1Volume(
	name='docker-push-secret',
	secret=client.V1SecretVolumeSource(secret_name=self.push_secret)
	))

binderhub/helm-chart/binderhub/templates/secret.yaml

Lines 19 to 27 in 7cedaa7

	{{- if .Values.config.BinderHub.use_registry }}
	kind: Secret
	apiVersion: v1
	metadata:
	name: binder-push-secret
	type: Opaque
	data:
	config.json: {{ include "registryDockerConfig" . | b64enc | quote }}
	{{- end }}

[g-braeunlich]

Thanks. Exactly. What do you think? Should I try to include the proxy vars into the secret in the registryDockerConfig helper?

[manics]

I'm not sure. Adding the proxy config to registryDockerConfig is an easy solution. On the other hand the name of the parameter registryDockerConfig is no longer correct, and it'd be nice to separate the configuration (proxy vars, registry credentials) from the implementation in repo2docker (read those vars, create the required config file). Anyone else have thoughts?

[g-braeunlich]

@manics I just pushed a proposal

[consideRatio]

I made a code suggestion refactoring some helm template logic that I'd like to see accepted, but otherwise this LGTM and feel good about merging this!

[consideRatio]

@betatim @sgibson91 @manics @MridulS this was the PR brought for discussion during our team meet.

I suggest we go for a merge and updated the description to summarize the situation as i see it.

[manics]

Is this change of default a breaking change?

[consideRatio]

I discussed it in the PR summary i updated above. Yes it can in theory cause a new build pod get stuck pending if started in a timeframe of a second for 1 replica binder deployments or a 10 second window for 2 replica deployments. I consider this acceptable.

If chart is upgraded, the old secret us gone, and an old binderhub replica awaiting termination until another binder pod starts up during a rolling upgrade, would starts a new build pod - then it would get stuck pending not being able to mount old k8s secret.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[manics]

Hello from the distant future! I'm reviewing the BinderHub config properties as part of #1521 and as far as I can tell this property isn't used. It's passed to builder.py, and referenced in a conditional, but its value is never used. Instead the helm chart references .Values.config.BinderHub.buildDockerConfig in helm-chart/binderhub/templates/_helpers.tpl.

Can you remember what's happening here?