Rename allowed env configuration traits#1137
Merged
kevin-bates merged 4 commits intojupyter-server:mainfrom Aug 2, 2022
Merged
Rename allowed env configuration traits#1137kevin-bates merged 4 commits intojupyter-server:mainfrom
kevin-bates merged 4 commits intojupyter-server:mainfrom
Conversation
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 |
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 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. |
69e9b02 to
afd000e
Compare
afd000e to
5a3092f
Compare
lresende
approved these changes
Aug 2, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.
env_whitelistandenv_process_whitelisthave been deprecated in favor ofclient_envsandinherited_envsrespectively. Hopefully, the names are more clear than the previous ones.EDIT - these changes have been reverted per discussion...
3. The use ofenv_whitelist = '*'has been removed.4. The set ofclient_envscan be specified globally, via the EG configuration, but also at the kernelspec level viametadata.client_envs. Kernelspecs retrieved from the EG server will be updated to include BOTH thoseclient_envsdefined in the spec, as well as those configured at the global level.I believe items 3 and 4 require further discussion:
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 ownGatewayClient.env_whitelistconfiguration) 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 allowclient_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 theinherited_envs.client_envscan 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 adjustGatewayClientto handle this and continue to support them.client_envsmust 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 theirGatewayClient.env_whitelistvalues and send whatever they have configured only if the EG server is running withenv_whitelist = '*'.client_envsin the kernelspecs, then a similar client-side configuration would be required (like today).env_whitelist, they will still function correctly provided their configuredenv_whitelistis a subset of the EG'sclient_envs.