Support dynamic push credentials obtained from registry

[manics]

Extracted from #1637
This is required to support time-limited registry credentials, as required by AWS ECR.

Required for jupyterhub/mybinder.org-deploy#2629

[consideRatio]

This is less secure than the push_secret option I think, because kubectl get pod can reveal content while when using a dedicated k8s Secret to house the content only kubectl get secret can reveal the content.

KubeSpawner has ended up dynamically creating also a k8s Secret for user pods to house misc information, for example info relevant for internal_ssl. It certainly adds complexity to do that, and this is far less complicated.

I think it may be acceptable to do this if we describe it clearly in the help string of the config.

[manics]

I agree it's not as good as using a secret, however we had a long discussion in #1577 over the pros and cons of dynamically creating secrets attached to pods, as the same problem arises with dynamic Git credentials.

[consideRatio]

Overall, the push_secret config has been challenging for me to understand well. Here are some notes on what I think can help.

• Its not obvious that its a push_secret references a name (k8s Secret resource for example) as compared to content to put inside one.
• Its not obvious that its going to be created or needs to be created manually
• Its not obvious what kind of content to put in the referenced secret created manually

Thinking about what I'd appreciate from docs to understand this good enough, I'd say something like the following, where its not obvious whats in scope and not for this PR:

1. I think "implementation dependant" could be written more verbosively to clarify that: its dependant on the BuildExecutor implementation, and no base implemention exist.
2. push_secret and push_secret_content's help strings are both are updated to reflect that there are separate ways of providing container registry credentials to the builder, where push_secret references something existing by name, and push_secret_content is content.
3. push_secret and push_secret_content's help strings are both are updated on how to know what kind of resource to reference and what content to provide. This may be implementation dependent and not something BuildExecutor can know, but I think we can declare here where to find that kind of details.

[manics]

I'll update the docs. AFAIK it should be fine to duplicate the traitlet in the subclass to have more specialist documentation?

If it's easier (I'm not sure whether it is), we could switch push_secret to be of type (Unicode, Dict). where Dict would for example take the form {content: <secret>} or {secret_name: <k8s-secret-name>}. Unicode would support the current default behaviour.

[manics]

What do you think of the updated docs? I've renamed the new property.

Do you think it's worth switching to a dynamic secret instead, and not have a new property a new property is required, otherwise the secret needs to be created outside the Kubernetes builder which breaks the abstraction.

[consideRatio]

I've googled on CONTAINER_ENGINE_REGISTRY_CREDENTIALS and understand the name to be defined by us and not an env var name respected by another project already, right?

What do you think about transition to prefixing it with BINDERHUB_ as this is the software project that defines them and sets them?

I've grown opinionated about this after seeing env vars respected by jupyter/docker-stacks images show up in z2jh and other places, where its very hard to understand they won't work for other images.

Reviewing this PR, I've googled the internet to find if CONTAINER_ENGINE_REGISTRY_CREDENTIALS and GIT_CREDENTIAL_ENV was used elsewhere, for example is GIT_CREDENTAL_ENV an env variable respected by git? I suspected it was because of its name, but it probably isn't as its not listed in git documentation. If it was named BINDERHUB_GIT_CREDENTIALS it would be obvious to me git wouldn't respect them, so then I could understand that it had to be consumed by something else. Searching in repo2docker, it seems that it isn't respected by that either but that there was some legacy notes about it leaving me confused if it will be respected by repo2docker.

I'd like to see us not lock into defining another env variable name that isn't obviously defined by this project by not prefixing it, but besides that I think we should merge this.

[manics]

Both of these environment variables are used by repo2docker:

• https://github.com/jupyterhub/repo2docker/blob/247e9535b167112cabf69eed59a6947e4af1ee34/docker/git-credential-env
• https://github.com/jupyterhub/repo2docker/blob/247e9535b167112cabf69eed59a6947e4af1ee34/repo2docker/engine.py#L148-L177

In hindsight prefixing them with R2D_ would've been a good idea and makes sense for future additions. We could add new R2D_ env vars and check for both old and new, but that risks adding to the confusion.