[auth] More easily add developers to developer namespaces #13180
Conversation
| @@ -481,21 +398,14 @@ async def _create_user(app, user, skip_trial_bp, cleanup): | |||
| updates['hail_credentials_secret_name'] = hail_credentials_secret_name | |||
|
|
|||
| namespace_name = user['namespace_name'] | |||
| if namespace_name is None and user['is_developer'] == 1: | |||
| # auth services in test namespaces cannot/should not be creating and deleting namespaces | |||
| if namespace_name is None and user['is_developer'] == 1 and not is_test_deployment: | |||
There was a problem hiding this comment.
Prior to this line of code, adding a developer to a dev namespace through the auth API would cause their namespace to be deleted. It feels generally correct that a dev/test auth should not be creating/deleting namespaces.
| @@ -53,7 +53,6 @@ async def _deploy(branch: str, steps: List[str], excluded_steps: List[str], extr | |||
|
|
|||
| deploy_config = get_deploy_config() | |||
| steps = unpack_comma_delimited_inputs(steps) | |||
| print(steps) | |||
There was a problem hiding this comment.
This accidentally snuck in when I was rewriting hailctl.
There was a problem hiding this comment.
This looks great! Thank you for the comments inlined into the code.
For the developer to submit jobs to the namespace, they must first manually copy in the secret from default if it does not already exist inside the namespace.
Can we dummy proof this a bit more to have a nice error message if this is not the case?
Happy to, but I'm not exactly sure what you mean by "if this is not the case". What would you like to add? |
| @@ -481,21 +398,14 @@ async def _create_user(app, user, skip_trial_bp, cleanup): | |||
| updates['hail_credentials_secret_name'] = hail_credentials_secret_name | |||
|
|
|||
| namespace_name = user['namespace_name'] | |||
| if namespace_name is None and user['is_developer'] == 1: | |||
| # auth services in test namespaces cannot/should not be creating and deleting namespaces | |||
There was a problem hiding this comment.
I think I would want a check here if it is a dev deployment that the expected secret given by hail_credentials_secret_name exists in that namespace using the k8s client.
There was a problem hiding this comment.
Actually, this should be the create_user function in auth.py
daniel-goldstein
dismissed
jigold’s stale review
Done. I tried to add jigold to my namespace in azure using hailctl before copying the secret, got the BadRequest error. Copied the secret, and then created the user successfully with hailctl.
auth/auth/auth.py
Outdated
| try: | ||
| k8s_cache: K8sCache = request.app['k8s_cache'] | ||
| res = await k8s_cache.read_secret(hail_credentials_secret_name, DEFAULT_NAMESPACE) | ||
| log.info(res) |
auth/auth/auth.py
Outdated
| log.info(res) | ||
| except kubernetes_asyncio.client.rest.ApiException as e: | ||
| raise web.HTTPBadRequest( | ||
| text=f'hail credentials secret name specified but was not found: {hail_credentials_secret_name}' |
There was a problem hiding this comment.
Can we add the namespace we're looking at as well in the error message?
I decided to break off this chunk from another PR that has stalled. That PR will ultimately build on this to add all developers automatically to dev AND test namespaces, but this should be an improvement for now. A few things in here:
DatabaseResourcestuff in the auth driver. Since databases now are created and destroyed with the namespace and not the developer, this is basically dead code.create_initial_account.pytries to copy the<dev-name>-gsa-keysecret from default into the developer namespace and this code will not do that anymore. For the developer to submit jobs to the namespace, they must first manually copy in the secret fromdefaultif it does not already exist inside the namespace. This is awkward, but IMO acceptable because:create_initial_account.pyis already broken anyway because when that script is run in a dev deploy it does not have access to production secretscreate_initial_account.pyin favor of using our actual API to create the dev user.