Skip to content

Rename allowed env configuration traits #1137

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

Merged
merged 4 commits into from
Aug 2, 2022

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented Jul 26, 2022

This pull request is a partial "redo" of #1000 and attempts to move EG in the right direction, although not completely. It consists of the following changes, some of which require further discussion.

  1. env_whitelist and env_process_whitelist have been deprecated in favor of client_envs and inherited_envs respectively. Hopefully, the names are more clear than the previous ones.
  2. Applicable documentation and deployment scripts have been updated accordingly.

EDIT - these changes have been reverted per discussion...
3. The use of env_whitelist = '*' has been removed.
4. The set of client_envs can be specified globally, via the EG configuration, but also at the kernelspec level via metadata.client_envs. Kernelspecs retrieved from the EG server will be updated to include BOTH those client_envs defined in the spec, as well as those configured at the global level.

I believe items 3 and 4 require further discussion:

  • Regarding item 3 (The use of env_whitelist = '*' has been removed.), because the intention of this PR is to (eventually) simplify the gateway client configuration (where it will not require its own GatewayClient.env_whitelist configuration) the model of conveying allowed env variables is essentially changing from a push model (where the client uses its own configuration to determine what envs to include, potentially irrespective of the EG's) to a pull model (where EG advertises the set of allowed envs). As a result, if we allow client_envs = '*', this would imply that all envs in the client should be included, which we clearly do not want to do as this could "explode" payloads and side-effect the envs that are statically configured in the kernel spec and/or in the inherited_envs.
    • Since operators of EG and their client servers that use EG, must share with those clients the set of allowed envs, it seems reasonable that it is their responsibility to do so as well as it is the responsibility of the clients to have the envs configured and available in their runtime environments.
  • Regarding item 4 (The set of client_envs can be specified globally...), my main concern about sending these in the kernelspec is whether this is too soon. What I don't want to happen is have to change this when proper kernel parameterization is introduced, which will likely need to include the corresponding schema to describe the envs, etc. That said, we could probably adjust GatewayClient to handle this and continue to support them.
    • Also note that client_envs must still be known ahead of the client server's startup so that those envs can be set - although this is really no different than today, except that, today, admins can set their envs, configure their GatewayClient.env_whitelist values and send whatever they have configured only if the EG server is running with env_whitelist = '*'.
    • If we were to NOT send the client_envs in the kernelspecs, then a similar client-side configuration would be required (like today).
    • If clients are still running with env_whitelist, they will still function correctly provided their configured env_whitelist is a subset of the EG's client_envs.

@kevin-bates kevin-bates marked this pull request as draft July 26, 2022 23:45
@kevin-bates
Copy link
Member Author

I've placed this into draft mode simply to prevent its merging until we've completed our discussion.

cc: @telamonian, @kiersten-stokes, and designated reviewers

@kevin-bates kevin-bates added this to the v3.0 milestone Jul 27, 2022
@kevin-bates
Copy link
Member Author

After discussion with maintainers, we've decided to pull back on this and only address the configuration name changes for now (items 1 & 2) in the 3.0 release.

Sending the set of configured client_envs (item 4) seemed better suited in 4.0 when (hopefully) we can address parameterization. And since we won't be using a pull model, using '*' as a wildcard (item 3) to trust the payload envs still held value.

The next commit will essentially remove code related to these (and restore code for item 3), after which I'll take this out of draft mode.

@kevin-bates kevin-bates force-pushed the rework-env-whitelists branch from 69e9b02 to afd000e Compare July 27, 2022 22:47
@kevin-bates kevin-bates force-pushed the rework-env-whitelists branch from afd000e to 5a3092f Compare July 27, 2022 23:40
@kevin-bates kevin-bates marked this pull request as ready for review July 27, 2022 23:48
@kevin-bates kevin-bates changed the title Rename/simplify allowed env configurations Rename allowed env configuration traits Jul 28, 2022
@kevin-bates kevin-bates merged commit 9dd52e1 into jupyter-server:main Aug 2, 2022
@kevin-bates kevin-bates deleted the rework-env-whitelists branch August 2, 2022 15:05
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