Enable (consistently) the preserve environment options with containers.

Author: rupertnash

cwltool/argparser.py

@@ -49,10 +49,10 @@ def arg_parser() -> argparse.ArgumentParser:
         type=str,
         action="append",
         help="Preserve specific environment variable when running "
-        "CommandLineTools without a software container.  May be provided "
-        "multiple times. The default is to preserve only the PATH.",
+        "CommandLineTools. May be provided multiple times. By default PATH is "
+        "preserved when not running in a container.",
         metavar="ENVVAR",
-        default=["PATH"],
+        default=[],
         dest="preserve_environment",
     )
     envgroup.add_argument(

cwltool/context.py

@@ -95,7 +95,7 @@ def __init__(self, kwargs: Optional[Dict[str, Any]] = None) -> None:
         self.no_read_only = False  # type: bool
         self.custom_net = ""  # type: Optional[str]
         self.no_match_user = False  # type: bool
-        self.preserve_environment = ""  # type: Optional[Iterable[str]]
+        self.preserve_environment = None  # type: Optional[Iterable[str]]
         self.preserve_entire_environment = False  # type: bool
         self.use_container = True  # type: bool
         self.force_docker_pull = False  # type: bool

cwltool/docker.py

@@ -78,6 +78,8 @@ def _check_docker_machine_path(path: Optional[str]) -> None:
 class DockerCommandLineJob(ContainerCommandLineJob):
     """Runs a CommandLineJob in a sofware container using the Docker engine."""
 
+    CONTAINER_TMPDIR: str = "/tmp"  # nosec
+
     def __init__(
         self,
         builder: Builder,
@@ -318,6 +320,15 @@ def add_writable_directory_volume(
                     shutil.copytree(volume.resolved, host_outdir_tgt)
                 ensure_writable(host_outdir_tgt or new_dir)
 
+    def _required_env(self) -> Dict[str, str]:
+        # spec currently says "HOME must be set to the designated output
+        # directory." but spec might change to designated temp directory.
+        # runtime.append("--env=HOME=/tmp")
+        return {
+            "TMPDIR": self.CONTAINER_TMPDIR,
+            "HOME": self.builder.outdir,
+        }
+
     def create_runtime(
         self, env: MutableMapping[str, str], runtimeContext: RuntimeContext
     ) -> Tuple[List[str], Optional[str]]:
@@ -335,9 +346,8 @@ def create_runtime(
         self.append_volume(
             runtime, os.path.realpath(self.outdir), self.builder.outdir, writable=True
         )
-        tmpdir = "/tmp"  # nosec
         self.append_volume(
-            runtime, os.path.realpath(self.tmpdir), tmpdir, writable=True
+            runtime, os.path.realpath(self.tmpdir), self.CONTAINER_TMPDIR, writable=True
         )
         self.add_volumes(
             self.pathmapper,
@@ -385,13 +395,6 @@ def create_runtime(
         if runtimeContext.rm_container:
             runtime.append("--rm")
 
-        runtime.append("--env=TMPDIR=/tmp")
-
-        # spec currently says "HOME must be set to the designated output
-        # directory." but spec might change to designated temp directory.
-        # runtime.append("--env=HOME=/tmp")
-        runtime.append("--env=HOME=%s" % self.builder.outdir)
-
         cidfile_path = None  # type: Optional[str]
         # add parameters to docker to write a container ID file
         if runtimeContext.user_space_docker_cmd is None:

cwltool/job.py

@@ -15,8 +15,9 @@
 from io import IOBase
 from threading import Timer
 from typing import (
-    IO,
     Callable,
+    Dict,
+    IO,
     Iterable,
     List,
     Mapping,
@@ -420,6 +421,23 @@ def _execute(
             )
             shutil.rmtree(self.tmpdir, True)
 
+    @abstractmethod
+    def _required_env(self) -> Dict[str, str]:
+        """Variables required by the CWL spec (HOME, TMPDIR, etc).
+
+        Note that with containers, the paths will (likely) be those from
+        inside.
+        """
+        pass
+
+    def _preserve_environment_on_containers_warning(
+        self, varname: Optional[str] = None
+    ) -> None:
+        """When running in a container, issue a warning."""
+        # By default, don't do anything; ContainerCommandLineJob below
+        # will issue a warning.
+        pass
+
     def prepare_environment(
         self, runtimeContext: RuntimeContext, envVarReq: Mapping[str, str]
     ) -> None:
@@ -431,24 +449,24 @@ def prepare_environment(
         applied (in that order).
         """
         # Start empty
-        env = {}
+        env: Dict[str, str] = {}
 
         # Preserve any env vars
-        vars_to_preserve = runtimeContext.preserve_environment
-        if runtimeContext.preserve_entire_environment is not False:
-            vars_to_preserve = os.environ
-        if vars_to_preserve:
-            for key, value in os.environ.items():
-                if key in vars_to_preserve and key not in env:
-                    env[key] = value
+        if runtimeContext.preserve_entire_environment:
+            self._preserve_environment_on_containers_warning()
+            env.update(os.environ)
+        elif runtimeContext.preserve_environment:
+            for key in runtimeContext.preserve_environment:
+                self._preserve_environment_on_containers_warning(key)
+                try:
+                    env[key] = os.environ[key]
+                except KeyError:
+                    _logger.warning(
+                        f"Attempting to preserve environment variable '{key}' which is not present"
+                    )
 
         # Set required env vars
-        env["HOME"] = self.outdir
-        env["TMPDIR"] = self.tmpdir
-        if "PATH" not in env:
-            env["PATH"] = os.environ["PATH"]
-        if "SYSTEMROOT" not in env and "SYSTEMROOT" in os.environ:
-            env["SYSTEMROOT"] = os.environ["SYSTEMROOT"]
+        env.update(self._required_env())
 
         # Apply EnvVarRequirement
         env.update(envVarReq)
@@ -530,6 +548,15 @@ def run(
 
         self._execute([], self.environment, runtimeContext, monitor_function)
 
+    def _required_env(self) -> Dict[str, str]:
+        env = {}
+        env["HOME"] = self.outdir
+        env["TMPDIR"] = self.tmpdir
+        env["PATH"] = os.environ["PATH"]
+        if "SYSTEMROOT" in os.environ:
+            env["SYSTEMROOT"] = os.environ["SYSTEMROOT"]
+        return env
+
 
 CONTROL_CODE_RE = r"\x1b\[[0-9;]*[a-zA-Z]"
 
@@ -588,6 +615,19 @@ def add_writable_directory_volume(
     ) -> None:
         """Append a writable directory mapping to the runtime option list."""
 
+    def _preserve_environment_on_containers_warning(
+        self, varname: Optional[str] = None
+    ) -> None:
+        """When running in a container, issue a warning."""
+        if varname is None:
+            _logger.warning(
+                "You have specified `--preserve-entire-environment` while running a container"
+            )
+        else:
+            _logger.warning(
+                f"You have specified `--preserve-environment={varname}` while running a container"
+            )
+
     def create_file_and_add_volume(
         self,
         runtime: List[str],

cwltool/singularity.py

@@ -83,6 +83,8 @@ def _normalize_sif_id(string: str) -> str:
 
 
 class SingularityCommandLineJob(ContainerCommandLineJob):
+    CONTAINER_TMPDIR: str = "/tmp"  # nosec
+
     def __init__(
         self,
         builder: Builder,
@@ -370,6 +372,12 @@ def add_writable_directory_volume(
                 self.append_volume(runtime, source, volume.target, writable=True)
                 ensure_writable(source)
 
+    def _required_env(self) -> Dict[str, str]:
+        return {
+            "TMPDIR": self.CONTAINER_TMPDIR,
+            "HOME": self.builder.outdir,
+        }
+
     def create_runtime(
         self, env: MutableMapping[str, str], runtime_context: RuntimeContext
     ) -> Tuple[List[str], Optional[str]]:
@@ -381,6 +389,7 @@ def create_runtime(
             "exec",
             "--contain",
             "--ipc",
+            "--cleanenv",
         ]
         if _singularity_supports_userns():
             runtime.append("--userns")
@@ -403,8 +412,12 @@ def create_runtime(
                 )
             )
         runtime.append("--bind")
-        tmpdir = "/tmp"  # nosec
-        runtime.append("{}:{}:rw".format(os.path.realpath(self.tmpdir), tmpdir))
+        runtime.append(
+            "{}:{}:rw".format(
+                os.path.realpath(self.tmpdir),
+                self.CONTAINER_TMPDIR,
+            )
+        )
 
         self.add_volumes(
             self.pathmapper,
@@ -432,9 +445,6 @@ def create_runtime(
         elif runtime_context.disable_net:
             runtime.append("--net")
 
-        env["SINGULARITYENV_TMPDIR"] = tmpdir
-        env["SINGULARITYENV_HOME"] = self.builder.outdir
-
         for name, value in self.environment.items():
             env[f"SINGULARITYENV_{name}"] = str(value)
         return (runtime, None)