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

use oauthenticator 17.2 for pkce-experiment (jupyter-health) #5217

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

minrk
Copy link
Contributor

@minrk minrk commented Dec 3, 2024

enables refreshing auth tokens, used in jupyter-health

@minrk minrk changed the title use oauthenticator 17.2 for pkce-experiment use oauthenticator 17.2 for pkce-experiment (jupyter-health) Dec 3, 2024
Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

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

Thanks @minrk, I believe this needs building and then updating the image tag in the hub config here ⬇️ but I'm not sure if you have push rights on 2i2c quay

hub:
# FIXME: Experiment to use oauthenticator 17.1, should be transitioned away
# as part of upgrading to z2jh 4, see
# https://github.com/2i2c-org/infrastructure/pull/4968
#
image:
name: quay.io/2i2c/pkce-experiment
tag: 0.0.1-0.dev.git.10892.h37c70b2e

I will try to do it

Copy link

github-actions bot commented Dec 3, 2024

Merging this PR will trigger the following deployment actions.

Support and Staging deployments

Cloud Provider Cluster Name Upgrade Support? Reason for Support Redeploy Upgrade Staging? Reason for Staging Redeploy
aws jupyter-health No Yes Following helm chart values files were modified: common.values.yaml

Production deployments

Cloud Provider Cluster Name Hub Name Reason for Redeploy
aws jupyter-health prod Following helm chart values files were modified: common.values.yaml

minrk and others added 2 commits December 3, 2024 14:31
enables refreshing auth, used in jupyter-health
@GeorgianaElena GeorgianaElena merged commit 793b6e7 into 2i2c-org:main Dec 3, 2024
8 checks passed
Copy link

github-actions bot commented Dec 3, 2024

🎉🎉🎉🎉

Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/12139768532

@minrk minrk deleted the refresh-tokens branch December 3, 2024 13:00
@minrk
Copy link
Contributor Author

minrk commented Dec 3, 2024

@GeorgianaElena thanks! Looks like it deployed successfully and things are working (I see in the Hub logs that my token was expired, but rightly refreshed using a refresh token when I launched my server), but the health check failed. I don't see any information about the health check or its failure, though.

I suspect it's due to refresh_auth not succeeding for the fake deployment-service-check user and the fact that refresh_pre_spawn is True. I think OAuthenticator may need some handling of these fake users to bypass refresh_auth. Disabling refresh_pre_spawn altogether is one workaround.

@GeorgianaElena
Copy link
Member

GeorgianaElena commented Dec 3, 2024

@minrk, I've disabled refresh_pre_spawn and re-ran the test. It still fails.
With refresh_pre_spawn True, it failed to start the server, now it fails to start the kernel here

And the actual error is:

    async def ensure_kernel(self, kernel_spec=None):
        kernel_specs = await self.list_kernel_specs()
        if kernel_spec is None:
>           kernel_spec = kernel_specs["default"]
E           KeyError: 'default'

Looking at the oauthenticator PR now.

Update, hub logs:

[I 2024-12-03 15:16:27.999 JupyterHub oauth2:1330] No auth_state found for user deployment-service-check refresh, need full authentication
[W 2024-12-03 15:16:27.999 JupyterHub base:411] User deployment-service-check has stale auth info. Login is required to refresh.
[W 2024-12-03 15:16:27.999 JupyterHub log:192] 403 GET /hub/api/user (@192.168.29.46) 7.36ms

@minrk
Copy link
Contributor Author

minrk commented Dec 3, 2024

Okay, I think that makes sense, actually.

If we want to be able to use OAuthenticator with users that aren't actually users, I think we need an explicit bypass of the refresh_user machinery somewhere.

It is a generic issue, I suppose, but perhaps the fix belongs in OAuthenticator for now to somehow bypass either for specific users or some property of users, e.g. having no auth_state at all.

@GeorgianaElena
Copy link
Member

Agree! I was leaning towards the same solution. I think I will revert this PR for now, if that's ok and we can test it again once we have the oauthenticator bypass implemented.

@minrk
Copy link
Contributor Author

minrk commented Dec 3, 2024

@GeorgianaElena actually, can I point the health check to another user rather than the fake one? Then the health check should work and we can keep testing.

@minrk
Copy link
Contributor Author

minrk commented Dec 3, 2024

Ah, I guess I can't really do that because the test hardcodes the username and makes quite a few assumptions about how users are handled by the Authenticator and that invalid users won't have any problems, which isn't necessarily the case, as seen here.

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.

2 participants