Allow specifying image pull secret #801

AlexMorreale · 2018-07-30T19:42:07Z

what / why

Allows you to specify an image_pull_secrets option in the jupyterhub docker image.

Updates the helm chart to allow specifying the secret name
Updates the Jupyterhub chart to allow specifying the secret name

PS: I have no idea how to contribute to this repo.

consideRatio

@AlexMorreale Thanks for the PR! ❤️

I lack experience of using image pull secrets like this, but I know a lot of how to handle the helm templates so I added some comments about such details.

I'm hoping to find time to follow up later, working intensively to prepare for JupyterCon 2018 and a release of the chart before it.

consideRatio · 2018-07-31T16:58:35Z

images/hub/jupyterhub_config.py

@@ -48,6 +48,10 @@

 c.KubeSpawner.image_pull_policy = get_config('singleuser.image-pull-policy')

+image_secret = get_config('singleuser.image-pull-secret', None)
+if image_secret:
+  c.KubeSpawner.image_pull_secrets = image_secret


Its fine to assign it right away without the conditional.

right cause the default is 'None' derp. ill get this changed

consideRatio · 2018-07-31T17:13:31Z

jupyterhub/templates/hub/regsecret.yaml

+apiVersion: v1
+metadata:
+  name: {{ .Values.singleuser.image.pullSecret.name }}
+  namespace: {{ .Release.Namespace }}


We don't have to set the namespace explicitly.

The file itself should be moved to the singleuser folder.

We should add labels to it:

metadata: name: singleuser-image-credentials labels: {{- $_ := merge (dict "componentSuffix" "-image-credentials") . }} {{- include "jupyterhub.labels" $_ | nindent 4 }}

You could name the file image-credentials-secret.yaml perhaps.

I will move the file it makes more sense to put it in there.

I am pretty sure this secret needs to be in the same namespace as the pods being spun up. dockerconfig secrets are not global.

The namespace is decided implicitly, if you look at other files, they all end up in the correct namespace even though it isn't explicitly written out. This would work as well though!

oh that makes sense.

consideRatio · 2018-07-31T17:16:48Z

jupyterhub/templates/hub/configmap.yaml

@@ -133,6 +133,9 @@ data:
    {{- .Values.singleuser.cloudMetadata | toYaml | trimSuffix "\n" | nindent 4 }}
  singleuser.start-timeout: {{ .Values.singleuser.startTimeout | quote }}
  singleuser.image-pull-policy: {{ .Values.singleuser.image.pullPolicy | quote }}
+  {{- if .Values.singleuser.image.pullSecret }}
+  singleuser.image-pull-secret: {{ .Values.singleuser.image.pullSecret | quote }}


The value .Values.singleuser.image.pullSecret is not just a string so it can't be added like this, but you must instead use some toYaml trick. Something like this:

{{- if .Values.auth.github.org_whitelist }} auth.github.org_whitelist: | {{- .Values.auth.github.org_whitelist | toYaml | trimSuffix "\n" | nindent 4 }} {{- end }}

sorry this was supposed to be {{- if .Values.singleuser.image.pullSecret.name }}

Ah! I looked at the source code of Kubespawner and Kubernets API and learned some things!

I learned that KubeSpawner's setting image_pull_secrets is not named properly, it should really be named image_pull_secret_name. I suggested below we could name the secret singleuser-image-credentials, this is what we need to assign to singleuser.image-pull-secret, as this is used to set KubeSpawners.image_pull_secrets setting in the jupyterhub_config.py file, and that in turn through make_pod in KubeSpawner creates a pod with a pod having spec.imagePullSecrets being [{'name': 'singleuser-image-credentials'}]

Pweh, this was tricky to detangle!

yeah i dug all yesterday. quite a bit of layers. take a look at what i have now and let me know if that makes sense.

consideRatio · 2018-07-31T17:27:42Z

jupyterhub/templates/hub/regsecret.yaml

+  namespace: {{ .Release.Namespace }}
+type: kubernetes.io/dockerconfigjson
+data:
+  .dockerconfigjson: {{ .Values.singleuser.image.pullSecret.dockerConfigJson }}


I read in the link referenced by the kubespawner documentation that we are supposed to base64 encode the json...

Good thing, Helm will allows to do this quite easily I think. Oh so I googled for helm base64 and found this very relevant documentation!

From the looks of it, perhaps a config like this would make sense?

singleuser: image: name: someimagename tag: sometag credentials: registry: quay.io username: someone password: sillyness

so my thought here was someone with "pre" base64 encode this because it is a json blob and that could be a mess in the chart.yml.

If this is against helm best practices i will gladly change it.

I am new helm, we wrote our own kubernetes manifest/deploy at ezCater and don't leverage helm.

I figure it would be quite easy to grasp for other users of the chart if we set it up like above. It would also be relatively easy to communicate and write the documentation what the users of the chart should do to set an image pull secret.

I figure reducing the effort of the chart user is good enough as motivation to take care of the base64 encoding for the chart users, but I'm afraid that some chart users might think that the credentials are encrypted just because they were base64 encoded even though they are not as base64 is easy to decode. If we make the user write out the credentials in plain text the chart user can't be mislead.

all of this makes sense will fix in my next commit.

consideRatio · 2018-07-31T20:15:56Z

@AlexMorreale Thank you for the work you put in, I appreciate that you are taking time in order to contribute! ❤️

AlexMorreale · 2018-07-31T20:33:04Z

@consideRatio loving the feedback. Like i said in the comments very very very new to helm so the feedback is great. We love Jupyterhub and Kubernetes at ezCater so I want to help in anyway I can.

I will fix the base64 encoding of the dockerconfigjson in my next push!

fix base64 encoding of dockerconfigjson chart value

consideRatio · 2018-07-31T20:49:29Z

jupyterhub/templates/singleuser/image-credentials-secret.yaml

@@ -8,5 +8,5 @@ metadata:
    {{- include "jupyterhub.labels" $_ | nindent 4 }}
 type: kubernetes.io/dockerconfigjson
 data:
-  .dockerconfigjson: {{ .Values.singleuser.image.pullSecret.dockerConfigJson }}
+  .dockerconfigjson: {{ .Values.singleuser.image.pullSecret.dockerConfigJson| b64enc }}


This assumes we have something like this:

singleuser: image: name: ... tag: ... pullSecrets: dockerConfigJson: ...

Do you mind we do it like this instead?

singleuser: image: name: someimagename tag: sometag credentials: registry: quay.io username: someone password: sillyness

You could copy->paste->adjust the template from the Helm tips and tricks section and put it in _helpers.tpl

that comment got lost in me pushing stuff

consideRatio · 2018-07-31T21:21:22Z

jupyterhub/templates/singleuser/image-credentials-secret.yaml

@@ -8,5 +8,5 @@ metadata:
    {{- include "jupyterhub.labels" $_ | nindent 4 }}
 type: kubernetes.io/dockerconfigjson
 data:
-  .dockerconfigjson: {{ .Values.singleuser.image.pullSecret.dockerConfigJson| b64enc }}
+  .dockerconfigjson: {{ template "singleuser.image.pullSecret" . }}


Looking great! Helm is silly, they have two functions doing the same thing: template and include, but include is superior to template so we use it all over instead. In this case both will work fine though but Helms own best practices is to use include even though we actually copied this template from their tips and tricks haha :D

hehe ill update!

consideRatio · 2018-07-31T21:36:14Z

jupyterhub/templates/hub/configmap.yaml

@@ -133,6 +133,9 @@ data:
    {{- .Values.singleuser.cloudMetadata | toYaml | trimSuffix "\n" | nindent 4 }}
  singleuser.start-timeout: {{ .Values.singleuser.startTimeout | quote }}
  singleuser.image-pull-policy: {{ .Values.singleuser.image.pullPolicy | quote }}
+  {{- if .Values.singleuser.image.pullSecret }}
+  singleuser.image-pull-secret: singleuser-image-credentials


Oh thinking about it, let us not propagate this mistake in KubeSpawner but instead try to alleviate it by calling this image-pull-secret-name instead.

consideRatio · 2018-07-31T21:46:24Z

Wieee this looks great!!! It may work as it is but my experience is telling me to be cautious for the unknown so I'd like us to have it tested once at least. If you can do it yourself that would be fine but it may be a bit cumbersome. Have you upgraded a cluster to use the locally cloned helm chart before? If not I'll find time to do it later, we should update the documentation on how to get setup in order to do this better.

It would also be great to have some documentation in schema.yaml about the added fields. That documentation will propagate to the guide's Helm chart configuration reference. I can do it also later.

AlexMorreale · 2018-08-01T16:45:27Z

I have tested this locally in Minikube and it works like a charm. I actually dont use Helm or this helm chart for my companies cluster. We use a statefulset for the hub.... either way. I can kill and recreate testing with the ezCater private reg in minikube. Would that be okay?

Also i can update the docs as well, if you can point me in the right direction.

Also also let me fix the code related to this comment:

Oh thinking about it, let us not propagate this mistake in KubeSpawner but instead try to alleviate it by calling this image-pull-secret-name instead.

@consideRatio

AlexMorreale · 2018-08-01T16:51:38Z

also should i consider this changing it from

singleuser:    
  image:                   
    name: private.reg.com/jhub-singlueuser
    tag: latest            
    pullSecret:            
      registry: private.reg.com
      username: name
      password:  passpaspaspas

to:

singleuser:    
  image:                   
    name: private.reg.com/jhub-singlueuser
    tag: latest            
  imagePullSecret:            
    registry: private.reg.com
    username: name
    password:  passpaspaspas

to be more like imagePullPolicy? which is not under the image: key?

@consideRatio

consideRatio · 2018-08-01T16:56:52Z

Awesome! I have never used a secret for image pulling before so It's great that you verify that it works as it should.

The documentation (mainly a reference) can be written in the schema.yaml file. Simply try to mimic what they have done there.

Ah yes excellent reflection! I considered that also and I don't know what will be best. In the future we may want to be able to specify various images and not only one. Hmmm... When the image is being pulled, what binary does the pulling and is actually consuming the secret? How does that application know what secret to use for a certain image?

Aaaa... The application doing the pulling is probably looking for secrets with a certain registry, and then uses those credentials... If that is the case, we should indeed switch so that imagePullSecret is extracted so it later can become a List of imagePullSecrets.

consideRatio · 2018-08-01T17:02:36Z

Oh well the pod has a field called imagePullSecrets, only those listed there will be used, hmmm...

Let us go with the latter option that you proposed, where the secret is not nested under the image!

AlexMorreale · 2018-08-01T20:17:05Z

After my latest changes:

Killed minikube.
Checked out master
Ran through the contributing guide start to finish.
Authed into Jupyterhub with the dummy authenticator
Spun up my own single-user server.
Works fine.

Then:

Check out my branch
Updated my singluser portion of minikube-config.yml:

singleuser:                                         
  image:                                            
    name: ezcater-test.privatereg.docker/jupyterhub-user
    tag: latest                                     
  imagePullSecret:                                  
    registry: ezcater-test.privatereg.docker        
    username: alex@test.com                         
    password: <redacted password>                   
  storage:                                          
    type: none                                      
  memory:                                           
    guarantee: null

Ran chartpress
Ran helm upgrade...... minikube-config.yml
Refreshed and reauth into jupyterhub
Started my server just fine

Something to note: If you have a larger singleuser image with multiple languages like we do at ezCater(R, Python, & Julia) it's worth bumping the singluser.startTimeout

AlexMorreale · 2018-08-01T20:17:37Z

if i get some cycles tonight i will update the docs with the info i posted above in my testing cycle

AlexMorreale · 2018-08-02T15:01:07Z

@consideRatio added documentation for singleuser.imagePullSecret as well as singleuser.imagePullPolicy (which wasn't present, I hope that is okay)

consideRatio

LGTM!

consideRatio · 2018-08-02T15:50:53Z

Wieeeeeeeeeeeee this is awesome!!! @AlexMorreale thank you for your time making this contribution!

Congratulations on your first PR to this repo!

I hope to give you and others a better contributing experience where we help out more with having more tests etc setup as well as guidance on what is appreciated pre-merge such as adding documentation where applicable. Thank you for staying with it even though that wasn't in place already.

❤️

consideRatio · 2018-08-08T01:36:40Z

@AlexMorreale I'm working on adding the email part, could you verify if it still works if you specify an email as well? Using the command to generate a secret forced the user to use a email hmmm...

consideRatio · 2018-08-08T03:32:27Z

Or nevermind, i made a helper allowing specification of the email to be optional as well as the image registry to default to Docker the companys image repository - like kubectl does when creating secrets.

AlexMorreale · 2018-08-08T20:31:03Z

@consideRatio let me know if you need review on the code!

AlexMorreale added 3 commits July 30, 2018 15:37

allow specifying a image pull secret for the kubespawner

27d243f

allow adding an image pull secret name via helm

f3e0da1

allow generating secret

bb6612a

consideRatio requested changes Jul 31, 2018

View reviewed changes

AlexMorreale added 2 commits July 31, 2018 15:12

remove useless conditional

0088fd6

use name field

241e78c

AlexMorreale added 2 commits July 31, 2018 16:26

move regsecret to image-credential-secret and add labels to secret

b0da471

simplify using a pull secret

1412641

base64 encode in template for dockerconfigjson secret

72d201f

consideRatio reviewed Jul 31, 2018

View reviewed changes

use singleuser.image.pullSecret helper to create pullSecret

a94dd0d

consideRatio reviewed Jul 31, 2018

View reviewed changes

include > template

cbc53aa

consideRatio reviewed Jul 31, 2018

View reviewed changes

consideRatio self-assigned this Jul 31, 2018

consideRatio added this to the 0.7 milestone Jul 31, 2018

add name prefix so that it will one day match kube-spawner code

fead3a8

move image pull secret up a level in chart.yml

ffe59fa

AlexMorreale added 2 commits August 2, 2018 10:58

add documentation for imagePullSecret for singleuser container

84d295d

add documentation about singluser imagePullPolicy

1724563

consideRatio approved these changes Aug 2, 2018

View reviewed changes

consideRatio merged commit c0157fc into jupyterhub:master Aug 2, 2018

consideRatio mentioned this pull request Aug 2, 2018

Add imagePullSecrets #447

Closed

AlexMorreale deleted the acm-image-sercret branch August 2, 2018 21:54

manics mentioned this pull request Aug 15, 2018

CHANGELOG for v0.7 #838

Closed

7 tasks

This was referenced Aug 16, 2018

Pulling images from external private image repositories #851

Merged

Already developed - Extracting smaller PRs from #758 #841

Closed

Allow specifying image pull secret #801

Allow specifying image pull secret #801

Conversation

AlexMorreale commented Jul 30, 2018 • edited Loading

what / why

consideRatio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio commented Jul 31, 2018

AlexMorreale commented Jul 31, 2018 • edited Loading

consideRatio Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio commented Jul 31, 2018

AlexMorreale commented Aug 1, 2018 • edited Loading

AlexMorreale commented Aug 1, 2018

consideRatio commented Aug 1, 2018

consideRatio commented Aug 1, 2018

AlexMorreale commented Aug 1, 2018

AlexMorreale commented Aug 1, 2018

AlexMorreale commented Aug 2, 2018 • edited Loading

consideRatio left a comment

Choose a reason for hiding this comment

consideRatio commented Aug 2, 2018

consideRatio commented Aug 8, 2018 • edited Loading

consideRatio commented Aug 8, 2018

AlexMorreale commented Aug 8, 2018

AlexMorreale commented Jul 30, 2018 •

edited

Loading

consideRatio Jul 31, 2018 •

edited

Loading

AlexMorreale commented Jul 31, 2018 •

edited

Loading

consideRatio Jul 31, 2018 •

edited

Loading

AlexMorreale commented Aug 1, 2018 •

edited

Loading

AlexMorreale commented Aug 2, 2018 •

edited

Loading

consideRatio commented Aug 8, 2018 •

edited

Loading