Revert changes to drop wildcard support and send client_envs in kspec

Author: kevin-bates

enterprise_gateway/services/kernels/handlers.py

@@ -54,16 +54,24 @@ async def post(self):
             # Start with the PATH from the current env. Do not provide the entire environment
             # which might contain server secrets that should not be passed to kernels.
             env = {"PATH": os.getenv("PATH", "")}
-            # Whitelist environment variables from current process environment
+            # Transfer inherited environment variables from current process
             env.update(
                 {key: value for key, value in os.environ.items() if key in self.inherited_envs}
             )
+            # Allow all KERNEL_* envs and those specified in client_envs and set from client.  If this EG
+            # instance is configured to accept all envs in the payload (i.e., client_envs == '*'), go ahead
+            # and add those keys to the "working" allowed_envs list, otherwise, just transfer the configured envs.
+            allowed_envs: List[str]
+            if self.client_envs == ["*"]:
+                allowed_envs = model["env"].keys()
+            else:
+                allowed_envs = self.client_envs
             # Allow KERNEL_* args and those allowed by configuration.
             env.update(
                 {
                     key: value
                     for key, value in model["env"].items()
-                    if key.startswith("KERNEL_") or key in self.client_envs
+                    if key.startswith("KERNEL_") or key in allowed_envs
                 }
             )
 

enterprise_gateway/services/kernels/remotemanager.py

@@ -456,7 +456,7 @@ async def start_kernel(self, **kwargs):
 
     def _capture_user_overrides(self, **kwargs):
         """
-        Make a copy of any whitelist or KERNEL_ env values provided by user.  These will be injected
+        Make a copy of any allowed or KERNEL_ env values provided by user.  These will be injected
         back into the env after the kernelspec env has been applied.  This enables defaulting behavior
         of the kernelspec env stanza that would have otherwise overridden the user-provided values.
         """

enterprise_gateway/services/kernelspecs/kernelspec_cache.py

@@ -3,7 +3,7 @@
 """Cache handling for kernel specs."""
 
 import os
-from typing import Dict, Optional, Set, Union
+from typing import Dict, Optional, Union
 
 from jupyter_client.kernelspec import KernelSpec
 from jupyter_server.utils import ensure_async
@@ -135,36 +135,8 @@ def put_item(self, kernel_name: str, cache_item: Union[KernelSpec, CacheItemType
         If it determines the cache entry corresponds to a currently unwatched directory,
         that directory will be added to list of observed directories and scheduled accordingly.
         """
-        self.log.info(f"KernelSpecCache: adding/updating kernelspec: {kernel_name}")
-        # Irrespective of cache enablement, add/update the 'metadata.client_envs' entry
-        # with the set of configured values.  If the stanza already exists in the kernelspec
-        # update with the union of it and those values configured via `EnterpriseGatewayApp'.
-        # We apply this logic here so that its only performed once for cached values or on
-        # every retrieval when caching is not enabled.
-        # Note: We only need to do this if we have a KernelSpec instance, since CacheItemType
-        # instances will have already been modified.
-
-        # Create a set from the configured value, update it with the (potential) value
-        # in the kernelspec, and apply the changes back to the kernelspec.
-
-        client_envs: Set[str] = set(self.parent.client_envs)
-        kspec_client_envs: Set[str]
-        if type(cache_item) is KernelSpec:
-            kspec: KernelSpec = cache_item
-            kspec_client_envs = set(kspec.metadata.get("client_envs", []))
-        else:
-            kspec_client_envs = set(cache_item["spec"].get("metadata", {}).get("client_envs", []))
-
-        client_envs.update(kspec_client_envs)
-        if type(cache_item) is KernelSpec:
-            kspec: KernelSpec = cache_item
-            kspec.metadata["client_envs"] = list(client_envs)
-        else:
-            if "metadata" not in cache_item["spec"]:
-                cache_item["spec"]["metadata"] = {}
-            cache_item["spec"]["metadata"]["client_envs"] = list(client_envs)
-
         if self.cache_enabled:
+            self.log.info(f"KernelSpecCache: adding/updating kernelspec: {kernel_name}")
             if type(cache_item) is KernelSpec:
                 cache_item = KernelSpecCache.kernel_spec_to_cache_item(cache_item)
 

enterprise_gateway/tests/test_handlers.py

@@ -644,44 +644,44 @@ def test_base_url(self):
         self.assertEqual(response.code, 200)
 
 
-# class TestWildcardEnvs(TestHandlers):
-#     """Base class for jupyter-websocket mode tests that spawn kernels."""
-#
-#     def setup_app(self):
-#         """Configure JUPYTER_PATH so that we can use local kernelspec files for testing."""
-#         super().setup_app()
-#         # overwrite env_whitelist
-#         self.app.env_whitelist = ["*"]
-#
-#     @gen_test
-#     def test_kernel_wildcard_env(self):
-#         """Kernel should start with environment vars defined in the request."""
-#         # Note: Since env_whitelist == '*', all values should be present.
-#         kernel_body = json.dumps(
-#             {
-#                 "name": "python",
-#                 "env": {
-#                     "KERNEL_FOO": "kernel-foo-value",
-#                     "OTHER_VAR1": "other-var1-value",
-#                     "OTHER_VAR2": "other-var2-value",
-#                     "TEST_VAR": "test-var-value",
-#                 },
-#             }
-#         )
-#         ws = yield self.spawn_kernel(kernel_body)
-#         req = self.execute_request(
-#             "import os; "
-#             'print(os.getenv("KERNEL_FOO"), '
-#             'os.getenv("OTHER_VAR1"), '
-#             'os.getenv("OTHER_VAR2"), '
-#             'os.getenv("TEST_VAR"))'
-#         )
-#         ws.write_message(json_encode(req))
-#         content = yield self.await_stream(ws)
-#         self.assertEqual(content["name"], "stdout")
-#         self.assertIn("kernel-foo-value", content["text"])
-#         self.assertIn("other-var1-value", content["text"])
-#         self.assertIn("other-var2-value", content["text"])
-#         self.assertIn("test-var-value", content["text"])
-#
-#         ws.close()
+class TestWildcardEnvs(TestHandlers):
+    """Base class for jupyter-websocket mode tests that spawn kernels."""
+
+    def setup_app(self):
+        """Configure JUPYTER_PATH so that we can use local kernelspec files for testing."""
+        super().setup_app()
+        # overwrite env_whitelist
+        self.app.env_whitelist = ["*"]
+
+    @gen_test
+    def test_kernel_wildcard_env(self):
+        """Kernel should start with environment vars defined in the request."""
+        # Note: Since env_whitelist == '*', all values should be present.
+        kernel_body = json.dumps(
+            {
+                "name": "python",
+                "env": {
+                    "KERNEL_FOO": "kernel-foo-value",
+                    "OTHER_VAR1": "other-var1-value",
+                    "OTHER_VAR2": "other-var2-value",
+                    "TEST_VAR": "test-var-value",
+                },
+            }
+        )
+        ws = yield self.spawn_kernel(kernel_body)
+        req = self.execute_request(
+            "import os; "
+            'print(os.getenv("KERNEL_FOO"), '
+            'os.getenv("OTHER_VAR1"), '
+            'os.getenv("OTHER_VAR2"), '
+            'os.getenv("TEST_VAR"))'
+        )
+        ws.write_message(json_encode(req))
+        content = yield self.await_stream(ws)
+        self.assertEqual(content["name"], "stdout")
+        self.assertIn("kernel-foo-value", content["text"])
+        self.assertIn("other-var1-value", content["text"])
+        self.assertIn("other-var2-value", content["text"])
+        self.assertIn("test-var-value", content["text"])
+
+        ws.close()

enterprise_gateway/tests/test_kernelspec_cache.py

@@ -12,7 +12,6 @@
 import pytest
 from jupyter_client.kernelspec import KernelSpecManager, NoSuchKernel
 
-from enterprise_gateway.enterprisegatewayapp import EnterpriseGatewayApp
 from enterprise_gateway.services.kernelspecs import KernelSpecCache
 
 
@@ -61,16 +60,10 @@ def environ(
 
 # END - Remove once transition to jupyter_server occurs
 
-GLOBAL_CLIENT_ENVS = ["TEST_VAR", "OTHER_VAR1", "OTHER_VAR2"]
-KSPEC_CLIENT_ENVS = ["TEST_VAR", "OTHER_VAR1", "OTHER_VAR3", "OTHER_VAR4"]
-UNION_CLIENT_ENVS = ["TEST_VAR", "OTHER_VAR1", "OTHER_VAR2", "OTHER_VAR3", "OTHER_VAR4"]
 
 kernelspec_json = {
     "argv": ["cat", "{connection_file}"],
     "display_name": "Test kernel: {kernel_name}",
-    "metadata": {
-        "client_envs": KSPEC_CLIENT_ENVS,
-    }
 }
 
 
@@ -110,22 +103,17 @@ def setup_kernelspecs(environ, kernelspec_location):
 
 @pytest.fixture
 def kernel_spec_manager(environ, setup_kernelspecs):
-    app = EnterpriseGatewayApp()
-    app.client_envs = ["TEST_VAR", "OTHER_VAR1", "OTHER_VAR2"]
-
-    yield KernelSpecManager(ensure_native_kernel=False, parent=app)
+    yield KernelSpecManager(ensure_native_kernel=False)
 
 
 @pytest.fixture
 def kernel_spec_cache(is_enabled, kernel_spec_manager):
-
     kspec_cache = KernelSpecCache.instance(
-        kernel_spec_manager=kernel_spec_manager,
-        cache_enabled=is_enabled,
-        parent=kernel_spec_manager.parent,
+        kernel_spec_manager=kernel_spec_manager, cache_enabled=is_enabled
     )
     yield kspec_cache
-    kspec_cache.clear_instance()
+    kspec_cache = None
+    KernelSpecCache.clear_instance()
 
 
 @pytest.fixture(params=[False, True])  # Add types as needed
@@ -146,14 +134,12 @@ async def tests_get_named_spec(kernel_spec_cache):
 async def tests_get_modified_spec(kernel_spec_cache):
     kspec = await kernel_spec_cache.get_kernel_spec("test2")
     assert kspec.display_name == "Test kernel: test2"
-    assert set(kspec.metadata['client_envs']) == set(UNION_CLIENT_ENVS)
 
     # Modify entry
     _modify_kernelspec(kspec.resource_dir, "test2")
     await asyncio.sleep(0.5)  # sleep for a half-second to allow cache to update item
     kspec = await kernel_spec_cache.get_kernel_spec("test2")
     assert kspec.display_name == "test2 modified!"
-    assert set(kspec.metadata['client_envs']) == set(UNION_CLIENT_ENVS)
 
 
 async def tests_add_spec(kernel_spec_cache, kernelspec_location, other_kernelspec_location):