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.

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

Support chart wide and pod specific config of imagePullSecrets #1794

Merged
merged 18 commits into from Oct 19, 2020

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Oct 4, 2020

Motivation

Before a k8s pod can starts its containers, Kubelet, a k8s component will download the images from the image registry. If this image registry requires credentials to access it, the Pod can specify spec.imagePullSecrets. If that is done, Kubelet will look in the referenced Kubernetes Secret resources and use the credentials within them to download the images.

This Helm chart both supports the creation of such k8s Secret with credentials, and the configuration of the hub pods and user pods spec.imagePullSecrets that reference the created Secret. What is missing though, is the same kind of configuration for all other pods.

This PR aims to resolve this once and for all by providing a chart wide configuration that augments all pods spec.imagePullSecrets, either with a manually created k8s Secret, or by a k8s Secret created by the Helm chart given configured credentials.

Helm chart config changes

Added

  • imagePullSecrets - A place to specify manually created k8s Secret references for all pods.
  • imagePullSecret.create|registry|username|password - A place to specify credentials for the creation of a k8s Secret that will be added to the list of all pods spec.imagePullSecrets.

Updated

  • ...image.pullPolicy configuration is now available for all pods
  • ...image.pullSecrets configuration is now available for all pods

Deprecated

  • hub.imagePullSecret - Deprecated in favor of chart wide imagePullSecret
  • singleuser.imagePullSecret - Deprecated in favor of chart wide imagePullSecret

Implementation steps

  • Add config imagePullSecret.create etc, to create a k8s Secret
    resource with image registry credentials.
  • Define a helper function ("jupyterhub.imagePullSecrets") that returns a
    JSON formatted list of Kubernetes Secret names or a blank string representing:
    the Secret created by imagePullSecret, the imagePullSecrets
    list, and the pod's specific config in ...image.pullSecrets.
  • Enable all pods to use these credentials to pull images with the helper
    function.
  • Add a hard deprecation of hub.imagePullSecret and
    singleuser.imagePullSecret. A message will show asking the user to use
    imagePullSecret instead from now on.
  • Add config imagePullSecrets to allow users to specify manually
    created Kubernetes Secret's for all pods to make use of.
  • Add config ...image.pullSecrets and ...image.pullPolicy where it was
    missing for certain pods.
  • Update schema.yaml to reflect changes.
  • Update jupyterhub_config.py logic of the user pod's imagePullPolicy
    and imagePullSecrets

Closes #981, Closes #1504, Closes #1467, Closes #1465

@manics
Copy link
Member

manics commented Oct 7, 2020

It's quite a big change to review without knowledge of exactly how this all works. Is it possible someone wants to pull their singleuser image from a different registry to the hub and proxy?

Do you have any idea how many users rely on a private registry? If it's a significant number maybe we should convert one of the existing CI jobs to test it?

jupyterhub/files/hub/jupyterhub_config.py Show resolved Hide resolved
jupyterhub/schema.yaml Outdated Show resolved Hide resolved
jupyterhub/templates/NOTES.txt Show resolved Hide resolved
jupyterhub/templates/_helpers.tpl Show resolved Hide resolved
jupyterhub/templates/_helpers.tpl Show resolved Hide resolved
jupyterhub/templates/_helpers.tpl Show resolved Hide resolved
jupyterhub/templates/hub/deployment.yaml Show resolved Hide resolved
jupyterhub/templates/image-puller/_daemonset-helper.yaml Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member Author

It's quite a big change to review without knowledge of exactly how this all works.

Yeah =/ Thank you for trying your best! I added some review comments which can potentially make it a bit easier to understand what various sections are about.

Is it possible someone wants to pull their singleuser image from a different registry to the hub and proxy?

They can reference singleuser.image.pullSecrets to add references to k8s secrets in addition to anything declared on the global.imagePullSecret(s) level influencing all pods. This is because the logic will augment them in a list.

Do you have any idea how many users rely on a private registry? If it's a significant number maybe we should convert one of the existing CI jobs to test it?

Not sure, but it's quite common. I've tested it quite well manually by rendering templates and ensuring the output makes sense for various pods. We could add this as a test quite easily without much complexity added by adding a dummy image to a fake-private registry with credentials that are public, and then we try it as an extra initContainer to some pod which we configure to use these credentials for.

I'm not sure how to go about getting such registry wich public credentials that we could make use of right now though, if I had that I could go ahead and add that to this PR as well right now.

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

This is great work! I'm happy to dogfood this in one of the clusters I maintain that require imagePullSecrets.

A few comments!

  1. Can we just use imagePullSecrets instead of global.imagePullSecrets? This is what we do for other global things (like rbac), and it would be nice to keep that consistent, instead of adding another layer.
  2. Does this mean you can't have a separate secret for hub & user pods? I don't fully understand that, can you explain why?
  3. Pre-puller modes might need more than just single-user secrets, since they might also be pulling in additional images that are from different image sources - something specified with singleuser.extraContainers, for example. I think this is also configurable here, but wanted to make sure.

In general, <3 the globalized imagePullSecrets along with per-pod overrides!

@consideRatio
Copy link
Member Author

consideRatio commented Oct 8, 2020

  1. Can we just use imagePullSecrets instead of global.imagePullSecrets? This is what we do for other global things (like rbac), and it would be nice to keep that consistent, instead of adding another layer.

Okay! See 247fcbf


  1. Does this mean you can't have a separate secret for hub & user pods? I don't fully understand that, can you explain why?

You can have a separate secret(s) for hub & user pods. The docs were improved to make this clearer in 3bb4c91, and further improved in f9d6575 which hopefully makes it clear also under the ...image.pullPolicy reference docs.


  1. Pre-puller modes might need more than just single-user secrets, since they might also be pulling in additional images that are from different image sources - something specified with singleuser.extraContainers, for example. I think this is also configurable here, but wanted to make sure.

This is fine, since imagePullSecrets part of the Pod resource are pod scoped rather than container scoped, so the case for singleuser.extraContainer's needs are met by the singleuser.image.pullSecrets configuration for both the default container and extraContainers.

@consideRatio
Copy link
Member Author

consideRatio commented Oct 8, 2020

@yuvipanda similarly to a discussion you had with regards to create not meant to suggest something else than just create, I added another option: imagePullSecret.automaticReferenceInjection, this will toggle if the created k8s Secret is referenced automatically alongside other things in the pods spec.imagePullSecrets fields.

consideRatio and others added 11 commits October 9, 2020 21:13
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
A imagePullPolicy doesn't make much sense here, as the context is to
pull something ahead of time. Always for example will not really help
much. I think?

Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
…gePullSecrets

Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
Co-authored-by: Harris Dimitriou <dimitriou.xar@gmail.com>
@consideRatio
Copy link
Member Author

@yuvipanda I've now addressed your comments in #1794 (comment) and feel happy about this PR in general.

If you think this looks good, perhaps we can merge @yuvipanda?

@consideRatio consideRatio added this to the 0.10.0 milestone Oct 18, 2020
@yuvipanda yuvipanda merged commit b1d96ed into jupyterhub:master Oct 19, 2020
@yuvipanda
Copy link
Collaborator

Thanks for thoroughly working on this, @consideRatio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants