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

add hub.config passthrough and use it for all auth config #1943

Merged
merged 40 commits into from
Jan 12, 2021

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Dec 14, 2020

Closes #1871.
Closes #1379 by updating the documentation examples to be directly mapped to LDAP config.

PR Status

Review -> merge remain, I don't know of anything I'd like to do further at this point for this PR.

  • The deprecation logic is tested against all the auth config examples in the docs we had, and it worked out fine. Automating testing seems overkill as we shouldn't make changes to it over time.
  • An e2e test of the deprecation and that the new configuration is valid has been done successfully.

Review suggestions

For me, the key part to review is the deprecation mechanisms. Secondary, we can focus on the documentation language etc, but I think we should iterate on that later as this PR is required for a release that I'd like to make. Below are some review points anyhow.

  • 1. Updates to schema.yaml in 5d5778f
  • 2. Changes in jupyterhub_config.py e4b64ce 72e3967
  • 3. Adding of null safe lookup utility in ee323db
  • 4. Handling of auth.state.cryptoKey in 9ed09c0
  • 5. Transform of configuration in authentication.md 245e86c
    @minrk I replaced replaced env var based configuration as well, because I recall your refactoring in oauthenticator made us no longer need to use env vars + traitlet config but could instead rely on only traitlet config of token_url etc. I have also verified this worked for a deployment of mine. Does it sound okay to you?
  • 6. The documentation added before the "Configuring authenticator classes" section - PR preview

Summary

This Helm chart's configuration of authenticator classes such as GitHubOAuthenticator were as observed by the influx of issues not so straightforward for users, and it wasn't so straightforward for maintainers either. We had to keep up with adding mappings between the helm charts configuration and the authentication classes configuration.

In this PR, we leave that system behind in favor of a more general system. In this transition users having configured authentication will fail directly when they run helm upgrade and be provided with an useful error message to help with the transition.

Transition message

config.yaml

auth:
  key1: val1
  key2:
    key3: val3
  type: github
  google:
    clientSecret: test-secret
    clientId: test-id
    callbackUrl: callback-url
  scopes:
    - profile
    - email
  state:
    enabled: true
  admin:
    access: true
    users:
      - cat
  whitelist:
    users:
      - kitten1
  allowedUsers: # maps to the same as whitelist.users
    - kitten2

helm upgrade

The JupyterHub Helm chart's auth config has been reworked and requires changes.

The new way to configure authentication in chart version 0.11.0+ is printed
below for your convinience. The values are not shown by default to ensure no
secrets are exposed, run helm upgrade with --set global.safeToShowValues=true to
show them.

hub:
  config:
    Authenticator:
      admin_users: '***'
      allowed_users: '***'
      enable_auth_state: '***'
    JupyterHub:
      admin_access: '***'
      authenticator_class: oauthenticator.github.GitHubOAuthenticator
    OAuthenticator:
      client_id: '***'
      client_secret: '***'
      oauth_callback_url: '***'
      scope: '***'
    WarningUnrecognizedConfig:
      key1: '***'
      key2.key3: '***'

For more details, please see the updated auth config documentation at:
https://zero-to-jupyterhub.readthedocs.io/en/latest/administrator/authentication.html

helm upgrade --set global.safeToShowValues=true

...

hub:
  config:
    Authenticator:
      admin_users:
      - cat
      allowed_users:
      - kitten1
      enable_auth_state: true
    JupyterHub:
      admin_access: true
      authenticator_class: oauthenticator.github.GitHubOAuthenticator
    OAuthenticator:
      client_id: test-id
      client_secret: test-secret
      oauth_callback_url: callback-url
      scope:
      - profile
      - email
    WarningUnrecognizedConfig:
      key1: val1
      key2.key3: val3

...

@consideRatio consideRatio changed the title WIP: Auth config rework Auth config reworked for comprehension and maintenance sustainability Dec 20, 2020
@consideRatio consideRatio requested review from manics, minrk and choldgraf and removed request for manics December 20, 2020 19:06
@consideRatio consideRatio marked this pull request as ready for review December 20, 2020 19:10
@consideRatio consideRatio added this to the 0.11.0 milestone Dec 20, 2020
Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

I made a first pass through the docs, I'll go through the code and schema changes later.

doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member Author

@manics thank you for your review!

I want to clarify that I have not really changed the text or structure of the documentation about how to configure the specific authenticators, but I'll give it a pass given your review comments. The reason for it being listed as an addition is the rST -> markdown conversion I did to not need to work with rST.

@consideRatio
Copy link
Member Author

@manics I've done a full pass of the auth docs following the questions you raised.

Working with Helm templates isn't funny... This has taken a lot of hours
to complete.
While these were devleoped, I relied on the following tests to ensure
they functioned as I wanted.

__A values.yaml file__

```
test:
  key1: val1
  dict1:
    key1: val1
  emptyDict: {}
  emptyList: []
  empty:
```

__A Helm template file__

```
{{- if ne (include "jupyterhub.digToJson" (list "test.key1" .Values)) "\"val1\"" }}{{ "digToJson test 1" | fail }}{{ end }}
{{- if ne (include "jupyterhub.digToJson" (list "test.dict1" .Values)) "{\"key1\":\"val1\"}" }}{{ "digToJson test 2" | fail }}{{ end }}
{{- if ne (include "jupyterhub.digToJson" (list "test.noDict" .Values)) "null" }}{{ "digToJson test 3" | fail }}{{ end }}
{{- if ne (include "jupyterhub.digToJson" (list "test.noDict1.noDict2" .Values)) "null" }}{{ "digToJson test 4" | fail }}{{ end }}
{{- if ne (include "jupyterhub.digToJson" (list "test.emptyDict" .Values)) "{}" }}{{ "digToJson test 5" | fail }}{{ end }}
{{- if ne (include "jupyterhub.digToJson" (list "test.emptyList" .Values)) "[]" }}{{ "digToJson test 6" | fail }}{{ end }}
{{- if ne (include "jupyterhub.digToJson" (list "test.empty" .Values)) "null" }}{{ "digToJson test 7" | fail }}{{ end }}

{{- if not (include "jupyterhub.digToTrue" (list "test.key1" .Values)) }}{{ "digToTrue test 1" | fail }}{{ end }}
{{- if not (include "jupyterhub.digToTrue" (list "test.dict1" .Values)) }}{{ "digToTrue test 2" | fail }}{{ end }}
{{- if (include "jupyterhub.digToTrue" (list "test.noDict" .Values)) }}{{ "digToTrue test 3" | fail }}{{ end }}
{{- if (include "jupyterhub.digToTrue" (list "test.noDict1.noDict2" .Values)) }}{{ "digToTrue test 4" | fail }}{{ end }}
{{- if (include "jupyterhub.digToTrue" (list "test.emptyDict" .Values)) }}{{ "digToTrue test 5" | fail }}{{ end }}
{{- if (include "jupyterhub.digToTrue" (list "test.emptyList" .Values)) }}{{ "digToTrue test 6" | fail }}{{ end }}
{{- if (include "jupyterhub.digToTrue" (list "test.empty" .Values)) }}{{ "digToTrue test 7" | fail }}{{ end }}
```
I updated the logic for checking if we had a valid configuration as
well using the Helm template helpers I introduced as I concluded it was
needed to remain sane while checking nested config that may be null at
any place.

I deleted the entry in secret.yaml as it is only to be used to set an
env variable for use by JupyterHub in the hub pod, which in turn is fine
to delete because its only used by JupyterHub's CryptKeeper class which
is responsible for managing encrypt/decrypt operations for JupyterHub.
@choldgraf
Copy link
Member

choldgraf commented Jan 7, 2021

I wonder if @yuvipanda would be particularly interested in this, since we're doing a lot of auth config in 2i2c and this would probably affect us, no? I would review it myself, but I don't think I'm qualified to do so from a tech perspective :-/

Copy link
Member

@minrk minrk 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 awesome work! I left some minor comments in the docs, but so happy for this to move forward.

jupyterhub/templates/_helpers-auth-rework.tpl Outdated Show resolved Hide resolved
config:
JupyterHub:
admin_access: true
authenticator_class: dummyauthenticator.DummyAuthenticator
Copy link
Member

Choose a reason for hiding this comment

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

shortcut works and is perhaps easier to understand:

Suggested change
authenticator_class: dummyauthenticator.DummyAuthenticator
authenticator_class: dummy

We shouldn't be using dummyauthenticator anymore, as dummy ships with jupyterhub now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! So I'll remove jupyterhub-dummyauthenticator from the requirements.txt, and we deprecate the repsitory?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe so. This change is still needed to avoid the current crash loop backoffs because you've removed the dummyauthenticator package but are still using it in config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved by 5b0a011

doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
doc/source/administrator/authentication.md Outdated Show resolved Hide resolved
@minrk minrk changed the title Auth config reworked for comprehension and maintenance sustainability add hub.config passthrough and use it for all auth config Jan 12, 2021
@minrk minrk merged commit b116faa into jupyterhub:master Jan 12, 2021
@minrk
Copy link
Member

minrk commented Jan 12, 2021

Great work, @consideRatio!

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jan 12, 2021
@consideRatio
Copy link
Member Author

Thanks @minrk and @manics for your review efforts!!! :Heart: ❤️ ❤️

@meeseeksmachine
Copy link

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

https://discourse.jupyter.org/t/jupyterhub-helm-chart-0-11-0-released/7521/1

yuvipanda added a commit to 2i2c-org/infrastructure that referenced this pull request Feb 4, 2021
Breaking changes to fix:

- All authenticator config has changed, and must be
  modified - jupyterhub/zero-to-jupyterhub-k8s#1943
- pdb has been disabled - we should file an issue to investigate
  if we want it - jupyterhub/zero-to-jupyterhub-k8s#1938
- Any networkpolicy changes we might need -
https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/master/CHANGELOG.md#breaking-changes-1
GeorgianaElena pushed a commit to 2i2c-org/infrastructure that referenced this pull request Feb 4, 2021
Breaking changes to fix:

- All authenticator config has changed, and must be
  modified - jupyterhub/zero-to-jupyterhub-k8s#1943
- pdb has been disabled - we should file an issue to investigate
  if we want it - jupyterhub/zero-to-jupyterhub-k8s#1938
- Any networkpolicy changes we might need -
https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/master/CHANGELOG.md#breaking-changes-1
GeorgianaElena pushed a commit to 2i2c-org/infrastructure that referenced this pull request Feb 5, 2021
Breaking changes to fix:

- All authenticator config has changed, and must be
  modified - jupyterhub/zero-to-jupyterhub-k8s#1943
- pdb has been disabled - we should file an issue to investigate
  if we want it - jupyterhub/zero-to-jupyterhub-k8s#1938
- Any networkpolicy changes we might need -
https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/master/CHANGELOG.md#breaking-changes-1
@bsikander
Copy link

bsikander commented Jan 10, 2022

Two cents from my side while upgrading to 0.11.1.

When I tried to upgrade, helm suggested the configuration changes as mentioned in the PR. So, I just copy pasted the output in my CLI and placed in my config. Hub kept of giving 500 internal server errors and after digging deep inside, i found that hub was not recognising my provided authenticator configs and was using the dummy default authenticator. The root cause of the problem was that I had two hub.* tags in my yaml. One for hub.extraConfig.* and other one that CLI suggested hub.config.*. When I manually merged the two seperate yaml sections into one like below, it started to recognize.

hub:
  config:
     ........
  extraConfig:
     ........

Apart from this, dynamically setting the configurations worked also

hub:
    extraConfig:
        authenticator:
            c.JupyterHub.authenticator_class = "oauthenticator.azuread.AzureAdOAuthenticator"
            c.JupyterHub.admin_access = True
            c.AzureAdOAuthenticator.client_id = ""
            c.AzureAdOAuthenticator.client_secret = ""
            c.AzureAdOAuthenticator.tenant_id = ""
            c.AzureAdOAuthenticator.oauth_callback_url = ""
            c.Authenticator.admin_users = {'usera', 'userb'}

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