Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 native JupyterHub OAuthenticator in 2i2c-managed hubs #706
Support native JupyterHub OAuthenticator in 2i2c-managed hubs #706
Changes from 12 commits
ff9cf07
3093a35
d45b1d7
92887fc
fe7cd40
99e7f82
f98a63c
4f53a4c
ffb4b95
c2587f5
7d1c6d7
abaa223
81c40be
93a792a
969869a
e3f88f0
2646ecf
ba6735e
fe4ab01
c937694
5ca78c9
b1d6aac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not get this removal... don't you need to keep this in auth0 enabled clusters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it might be that this line
https://github.com/sgibson91/pilot-hubs/blob/fe4ab0101b6900997b9c1c0fc90e62b1b57e1720/deployer/hub.py#L351
should be the following instead
I wasn't sure and was a bit confused, hence why I asked for my work to be checked in this discussion thread #706 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure the alternative you wrote in the previous comment will work in your pangeo (non-auth0) case.
What about doing all the stuff under 06-custom-authenticator conditional if
auth0
is configured? As it is done here with thescratch_bucket
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgibson91, I believe so too!
At some point, I did this chart about how different configs are put together. I'm not sure if it's super accurate now, but for me, at least it provides a visual starting point, though I'm still a bit confused about them (hence, my forever-taking PR with the staff admin users 🙄 )
One thing that's not accurate in that chart (I think), is that although
values.yaml
template is the first one that gets loaded, , theextra_config
invalues.yaml
is actually the last one that get loaded by the hub. At least that's what I understand from the hub logs first few lines:@damianavila, it shouldn't be set for non-auth0 cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @GeorgianaElena! I will update that line and see what people think. Also very nice chart! 😉
@damianavila for clarity's sake, we now need to dynamically set the value of
JupyterHub.authenticator_class
based on which authentication method we are using, hence why these values are being removed from the chart. So when using auth0,authenticator_class
will be set in the deployer in this block:https://github.com/sgibson91/pilot-hubs/blob/fe4ab0101b6900997b9c1c0fc90e62b1b57e1720/deployer/hub.py#L336-L352
And when using the native GitHubOAuthenticator, we set the class in the
*.cluster.yaml
hub config, as demonstrated in the accompanying docs, e.g. Step 3 here: https://github.com/sgibson91/pilot-hubs/blob/jupyterhub-oauth-github/docs/howto/configure/auth-management.md#native-jupyterhub-oauthenticator-for-github-orgs-and-teamsI am not convinced that method will work. If I understand the code correctly, the highest level config key that
get_config
reads in is everything underjupyterhub
and theauth0.enabled
key is much higher in the hierarchy of keys, adjacent to hubs, so I'm not sure thez2jh
module can even see it?Image to clarify what I mean by the hierarchy:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the
authenticator_class
for auth0 hubs inb1d6aac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks @GeorgianaElena and @sgibson91 for the discussion and the clarifications!
I now understand why I did not catch it... I totally missed this line. This is why I proposed the alternative "conditional approach" (without any testing, of course 😉 ).
In fact, I was expecting that auth0 "enabled" property to have some representation in the schema (I guess that is another discussion to have...), and since I did not find it there, and because I did not check some lines above 🤦 , I assumed there was not a conditional approach.
I concur with the last commit you pushed @sgibson91.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you would need to have somehow an
auth
under custom as well for being able to catch it... but no need to test that now 😜 .