Closed kevin-bates closed 1 year ago
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
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.
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_whitelist
andenv_process_whitelist
have been deprecated in favor ofclient_envs
andinherited_envs
respectively. 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_envs
can 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_envs
defined 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_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 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_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 adjustGatewayClient
to handle this and continue to support them.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 theirGatewayClient.env_whitelist
values and send whatever they have configured only if the EG server is running withenv_whitelist = '*'
.client_envs
in the kernelspecs, then a similar client-side configuration would be required (like today).env_whitelist
, they will still function correctly provided their configuredenv_whitelist
is a subset of the EG'sclient_envs
.