jupyter-server / enterprise_gateway

A lightweight, multi-tenant, scalable and secure gateway that enables Jupyter Notebooks to share resources across distributed clusters such as Apache Spark, Kubernetes and others.
https://jupyter-enterprise-gateway.readthedocs.io/en/latest/
Other
623 stars 222 forks source link

gateway default env overwrite env passed from NB2KG #456

Closed ktong closed 6 years ago

ktong commented 6 years ago

I set environment variable with default value on gateway in case client does not pass it. But even client passed the environment variable, it still use default value in gateway to create kernel as the code below overwrite all env from client with env from gateway.

https://github.com/jupyter/enterprise_gateway/blob/322bd13519586652e510a255d1a948fb9d18d512/enterprise_gateway/services/processproxies/k8s.py#L62

kevin-bates commented 6 years ago

Thanks for the issue.

Could you please provide the name of the variable in question here? The reason I ask this is because not all env variables are passed from NB2KG. Only those variables prefixed with KERNEL_ or appear in the KG_ENV_WHITELIST are propagated, and I just want to make sure your issue isn't due to this behavior. If your variable does not consist of a KERNEL_ prefix, can you please confirm that it appears in the KG_ENV_WHITELIST?

However, the line you point out is problematic, even after #429 (which you probably want to have in your build as well).

Did you want to take a crack at fixing this issue? I would suggest removing the highlighted line and determine what breaks, then working backwards.

On the bright side, this is only an issue for k8s and that line should have probably been revisited sooner. Sorry for the inconvenience.

ktong commented 6 years ago

Yes, the variable name starts KERNEL_ which I would like to pass to kernel. I would try to build my own image without this line and test it.

kevin-bates commented 6 years ago

Thank you. Should you run into issues due to that line's removal, the following would likely be sufficient:

kw['env'] = dict(os.environ.copy().items() + list(kw['env'].items()))

ktong commented 6 years ago

Thanks. The statement above work well.

kevin-bates commented 6 years ago

@ktong - glad to hear that.

Did you happen to try removing the statement altogether? I'm curious what issues you may have found.

Did you want to create a pull request for your changes?

ktong commented 6 years ago

Removing the statement throws exception: Traceback (most recent call last): File "/usr/local/share/jupyter/kernels/launch_kubernetes.py", line 104, in launch_kubernetes_kernel(connection_file, response_addr, spark_context_init_mode) File "/usr/local/share/jupyter/kernels/launch_kubernetes.py", line 15, in launch_kubernetes_kernel config.load_incluster_config() File "/usr/lib/python2.7/site-packages/kubernetes/config/incluster_config.py", line 93, in load_incluster_config cert_filename=SERVICE_CERT_FILENAME).load_and_set() File "/usr/lib/python2.7/site-packages/kubernetes/config/incluster_config.py", line 45, in load_and_set self._load_config() File "/usr/lib/python2.7/site-packages/kubernetes/config/incluster_config.py", line 51, in _load_config raise ConfigException("Service host/port is not set.") kubernetes.config.config_exception.ConfigException: Service host/port is not set.

I will create a PR base on your modification of statement.