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 HTTPS via user-provided key/cert #246

Merged
merged 4 commits into from
Nov 2, 2017

Conversation

minrk
Copy link
Member

@minrk minrk commented Nov 2, 2017

usage:

proxy:
  https:
    type: files
    files:
      cert: |
        <paste cert>
      key: |
        <paste key>

closes #244

@@ -1,4 +1,4 @@
apiVersion: v1
description: Multi-user Jupyter installation
name: jupyterhub
version: v0.5.0
version: v0.5.0-3be4d58
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accidental?

@yuvipanda
Copy link
Collaborator

This is awesome!

What do you think about calling this 'manual' or something like that rather than 'files'? Am unsure what 'files' signifies in this context...

@minrk
Copy link
Member Author

minrk commented Nov 2, 2017

I was thinking 'files' because they came from files, but of course they aren't files anymore when they go in your .yml. manual seems okay, but it's not super obvious what it means.

Does it make sense for the default behavior to use https.key/cert if defined, or is that too many choices to select between automatically?

@@ -198,26 +198,58 @@ your users & the world at large. Zero to JupyterHub makes doing so quite
easy since version 0.5, integrating with `Let's Encrypt <https://letsencrypt.org/>`_
for free HTTPS certificates.

You can also purchase your own SSL certificates from a certificate provider.

1. Buy a domain name from a registrar. Pick whichever one you want.
2. Create an ``A record`` from the domain you want to use, pointing to the
external IP provided to the `proxy-public` service.
Copy link
Member

Choose a reason for hiding this comment

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

could explicitly say this is the <EXTERNAL-IP> field. For whatever reason people almost always get this confused and copypaste the internal IP

Copy link
Member

Choose a reason for hiding this comment

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

(I know this isn't related to this PR so we can punt it to later if you like)

{{- else }}
# unhandled type
# it would be nice if helm had an `error` function
{{ required (printf "https.type must be 'files' or 'letsencrypt', not '%s'" .Values.proxy.https.type) ._undefined }}
Copy link
Member

Choose a reason for hiding this comment

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

just a note to change this if files changes to manual or whatever else

@choldgraf
Copy link
Member

as a non-letsencrypt person, files and manual are equally non-informative to me, just FYI. I'm 33/33/33 on files/manual/something else.

@yuvipanda
Copy link
Collaborator

@minrk yeah, I think that'd be too much magic. Plus, I think there's a good pattern of:

type: someString
someString:  
   <someString specific options>

This occurs throughout the k8s ecosystem, and is a good one to follow for us.

I agree manual seems not entirely clear, but can't think of anything else.

@yuvipanda
Copy link
Collaborator

FWIW I also consider provisioning your own certificates an 'advanced' use case.

@choldgraf
Copy link
Member

agree it's advanced - that's why I'm just giving a datapoint but not weighing in heavily one way or another :-)

@@ -82,6 +82,8 @@ proxy:
type: letsencrypt
letsencrypt:
contactEmail: ''
key:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be manual.key and manual.cert IMO. I think we should be doing:

type: X
X:
   <all-X-related-options>

Copy link
Member Author

Choose a reason for hiding this comment

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

done

instead of files
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.

w00t

@yuvipanda yuvipanda merged commit cdaa49e into jupyterhub:master Nov 2, 2017
@minrk minrk deleted the ssl-files branch November 2, 2017 18:20
@yuvipanda yuvipanda added this to the 0.5 milestone Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SSL key/cert files
3 participants