Types of GatewayClient.env_whitelist and EnterpriseGatewayApp.env_whitelist are not compatible

[telamonian]

It appears that in order to get any use from the GatewayClient.env_whitelist and the EnterpriseGatewayApp.env_whitelist options, their values have to match. This is made somewhat awkward by the fact that the GatewayClient.env_whitelist option is backed by a unicode traitlet (and is meant to be a comma separated string of values) and the EnterpriseGatewayApp.env_whitelist is a proper list traitlet. These should match, so one of these should change.

@kevin-bates I can submit the PR for this. Should I change EnterpriseGatewayApp.env_whitelist to a unicode traitlet, or vice versa?

[kevin-bates]

Hi Max - good catch. I think treating this trait as an actual list is more correct, although that can be a bit more difficult in scripting, etc. Still, I think that's where the change should be made. However, I believe this would qualify as an API change and I wonder if we should deprecate GatewayClient.env_whitelist for GatewayClient.allowed_envs (or something of that nature)? This would probably best be done in jupyter_server as notebook is winding down for this level of change.

There's also a pending EG 3.0 release on the horizon where we could make a similar name change.

That all said, I'm flexible (especially if I'm not doing the work 😄 )!

[telamonian]

Deprecation seems like a good idea to me, but there's already a very complicated story around the 3 similar options:

• GatewayClient.env_whitelist
• EnterpriseGatewayApp.env_whitelist
• EnterpriseGatewayApp.env_process_whitelist

If we start changing only some of the names around I think it will make it even more confusing. So should we change all of them? Maybe.

So that's fairly complicated. Still, the types mismatch thing is a major (or at least a moderate) usability problem; for example, I just wasted half an hour debugging around the fact that EG_ENV_WHITELIST and EG_KERNEL_WHITELIST require completely different syntax (I guess cuz EG_KERNEL_WHITELIST is consumed by helm), even though both are fundamentally meant to hold the same sort of data (ie a list of strings).

Related, how would you feel about bumping the traitlets dep to >=5.0? If we're talking about breaking changes, then I think the ideal endgame here would be enable the new EZ container value passing that was implemented in 5.0 for all "List-like" options.

[kevin-bates]

I think what you say makes a lot of sense and we should strongly consider these changes (traitlets >= 5 and renames) - after all, a major release is when these things can happen. I'm thinking env_whitelist -> allowed_envs while env_process_whitelist -> inherited_envs unless there are other options.

[telamonian]

allowed_envs sounds pretty good. The thing is, the client env_whitelist option doesn't just allow or whitelist a set of vars, it actively forwards them (or at least tries to) from wherever the user's individual jupyter_server instance is to the environment of the kickoff process of any kernel said user starts via the gateway.

It seems like forward_env would make the intent of the option a bit clearer. There could be a separate forward_env option for both client and gateway, equivalent to the current env_whitelist (on the client) and env_process_whitelist (on the gateway) options. So you'd end up with:

• GatewayClient.forward_env (replacing GatewayClient.env_whitelist)
• EnterpriseGatewayApp.forward_env (replacing EnterpriseGatewayApp.env_process_whitelist)
• EnterpriseGatewayApp.allowed_env (replacing EnterpriseGatewayApp.env_whitelist)

[telamonian]

Getting even more ambitious, it would also be nice if said forward_env options were able to forward vars all the way to the environment of the kernel itself. I think I know what would be involved in implementing this for kubernetes at least. In practice I've just been adding new env vars to kernel_pod.yaml as needed

[kevin-bates]

I like the naming suggestions, although I think EnterpriseGatewayApp.forward_env will be confused with GatewayClient.forward_env and the two are different.

Does EnterpriseGatewayApp.inherited_envs for EnterpriseGatewayApp.env_process_whitelist sound less confusing?

You'd then have:

• GatewayClient.forward_env indicates which envs from the client are forwarded to EG
• EnterpriseGatewayApp.allowed_env indicates, of those envs in the start request, which are allowed to propagate to the kernel process
• EnterpriseGatewayApp.inherited_envs indicates which envs from the EG process will be inherited by the kernel process

Getting even more ambitious, it would also be nice if said forward_env options were able to forward vars all the way to the environment of the kernel itself. I think I know what would be involved in implementing this for kubernetes at least. In practice I've just been adding new env vars to kernel_pod.yaml as needed

I totally agree (and love your ambition)! I think it would be nice to have the "factory values" exist as they do in the template, but perhaps we could make the "user values" be built dynamically. It looks like there are looping constructs that could be used, or build the entries in the script itself. That would be great.

Yeah, adding piecemeal vars to the yaml file as needed was the minimally viable approach. This would be really useful to have a general flow what's there mechanism.

[telamonian]

Those names sound reasonable. I'll start working on this.

perhaps we could make the "user values" be built dynamically

pyyaml is good and flexible, so I think this is the way to go: in launch_kubernetes.py, we loop over all vars that are both forwarded by forward_env and allowed by allowed_env and add them to the env list in the loaded yaml, for example:

    k8s_obj['spec']['containers'][0]['env'].append({'name': 'FOO', 'value': 'bar'})

assuming I got the structure of the parsed yaml right.

I think the big downside of anything jinja2-based is that it kills every static parser yet invented. Thus any jinja template with conditionals inside loops is going to be both hard to read and finiky to modify.

[telamonian]

ooo, and speaking of pyyaml, it looks like a pyyaml requirement should get added to setup.py and requirements.yml. Although the launch_kubernetes.py script technically isn't part of the enterprise_gateway python pkg proper, said script does get called directly by it, and is presumably going to be running on the same machine. So it makes sense to include it as one of the required enterprise_gateway dependencies

[kevin-bates]

Thank you Max - good points.

Because the launch_kubernetes script is actually what is launched by the framework, whatever environment variables are present should be the result of both allowed and inherited settings. However, I now recall that we don't really honor the inherited_envs on k8s and consider them all inherited. So, at this moment, the envs set in the launch_kubernetes script process will be all current process envs plus those forward_envs filtered via the allowed_envs list.

I think we should try following the FIXME comment and essentially remove the expansion of kwargs['envs'] since the handler should have correctly handled the list traits. Kubernetes should be propagating its system-level envs upon pod creation anyway.

Thank you for spending time on this!

[telamonian]

@kevin-bates Sorry for the delay, work on this is now underway over at #1000