Fix bug with sandboxing with values from env/settings (#978) (#981)

[backport][release_2.1] Fix bug with sandboxing with values from env/settings (#978)

Backport of PR #978
This also standardizes .gitignore from a change that was not backported to this branch.
(cherry picked from commit 996a00d)

Reviewed-by: Alexander Sowitzki <dev@eqrx.net>
Reviewed-by: None <None>

.gitignore

@@ -1,29 +1,34 @@
-# Mac OS X
-*.DS_Store
+# Test artifacts
+*.py,cover
+.pytest_cache/
+docs/_build/
+docs/build/
+pytestdebug.log
+test/coverage/
+
+# Byte-complied files
+*.py[cod]
+__pycache__/
 
-# Editors
-*.sw[poj]
-*~
-/.idea
+# Distribution / packaging
+*.egg
+*.egg-info/
+.eggs/
+AUTHORS
+Changelog
+build/
+deb-build/
+dist/
+eggs/
+rpm-build/
+
+# Environments
+.env
+.python-version
+.tox/
+.venv
+venv/
 
+# Demo files
 /demo/artifacts
 /demo/daemon.log
-/docs/_build
-/.tox
-/dist
-/build
-/rpm-build
-/deb-build
-/*.egg-info
-*.py[c,o]
-.pytest_cache
-pytestdebug.log
-.coverage
-*,cover
-.venv
-/venv
-.env
-/.eggs/
-docs/build
-AUTHORS
-ChangeLog

ansible_runner/config/runner.py

@@ -23,8 +23,8 @@
 import stat
 import tempfile
 import six
+import distutils.dir_util
 
-from distutils.dir_util import copy_tree
 from six import string_types, text_type
 
 from ansible_runner import output
@@ -135,14 +135,18 @@ def prepare(self):
             raise ConfigurationError("Only one of playbook and module options are allowed")
         if not os.path.exists(self.artifact_dir):
             os.makedirs(self.artifact_dir, mode=0o700)
+
+        # Since the `sandboxed` property references attributes that may come from `env/settings`,
+        # we must call prepare_env() before we can reference it.
+        self.prepare_env()
+
         if self.sandboxed and self.directory_isolation_path is not None:
             self.directory_isolation_path = tempfile.mkdtemp(prefix='runner_di_', dir=self.directory_isolation_path)
             if os.path.exists(self.project_dir):
                 output.debug("Copying directory tree from {} to {} for working directory isolation".format(self.project_dir,
                                                                                                            self.directory_isolation_path))
-                copy_tree(self.project_dir, self.directory_isolation_path, preserve_symlinks=True)
+                distutils.dir_util.copy_tree(self.project_dir, self.directory_isolation_path, preserve_symlinks=True)
 
-        self.prepare_env()
         self.prepare_inventory()
         self.prepare_command()
 
@@ -186,6 +190,7 @@ def prepare_env(self):
         self.process_isolation_hide_paths = self.settings.get('process_isolation_hide_paths', self.process_isolation_hide_paths)
         self.process_isolation_show_paths = self.settings.get('process_isolation_show_paths', self.process_isolation_show_paths)
         self.process_isolation_ro_paths = self.settings.get('process_isolation_ro_paths', self.process_isolation_ro_paths)
+        self.directory_isolation_path = self.settings.get('directory_isolation_base_path', self.directory_isolation_path)
         self.directory_isolation_cleanup = bool(self.settings.get('directory_isolation_cleanup', True))
 
         self.resource_profiling = self.settings.get('resource_profiling', self.resource_profiling)

test/conftest.py

@@ -1,6 +1,7 @@
 import shutil
 
 from distutils.version import LooseVersion
+from pathlib import Path
 
 from ansible_runner import defaults
 
@@ -64,3 +65,14 @@ def pytest_generate_tests(metafunc):
             )
             metafunc.parametrize('runtime', args)
             break
+
+
+@pytest.fixture
+def project_fixtures(tmp_path):
+    source = Path(__file__).parent / 'fixtures' / 'projects'
+    dest = tmp_path / 'projects'
+    shutil.copytree(source, dest)
+
+    yield dest
+
+    shutil.rmtree(dest, ignore_errors=True)

test/fixtures/projects/directory_isolation/env/settings

@@ -0,0 +1,3 @@
+process_isolation: True
+process_isolation_executable: bwrap
+directory_isolation_base_path: /tmp/runner

test/fixtures/projects/directory_isolation/project/main.yml

@@ -0,0 +1,5 @@
+- hosts: all
+  gather_facts: no
+
+  tasks:
+    - debug: msg="directory_isolation test"

test/integration/conftest.py

@@ -1,11 +1,7 @@
 import json
 import os
-import shutil
 import subprocess
 import yaml
-
-from pathlib import Path
-
 import pytest
 import pexpect
 
@@ -80,14 +76,3 @@ def run(args, *a, **kw):
 
         return ret
     return run
-
-
-@pytest.fixture
-def project_fixtures(tmp_path):
-    source = Path(__file__).parent / 'fixtures' / 'projects'
-    dest = tmp_path / 'projects'
-    shutil.copytree(source, dest)
-
-    yield dest
-
-    shutil.rmtree(dest, ignore_errors=True)

test/unit/config/test_runner.py

@@ -4,8 +4,8 @@
 from io import StringIO
 import os
 import re
-
 import six
+
 from pexpect import TIMEOUT, EOF
 
 import pytest
@@ -217,6 +217,37 @@ def test_prepare_env_directory_isolation(mocker):
     assert rc.cwd == '/tmp/foo'
 
 
+def test_prepare_env_directory_isolation_from_settings(mocker, project_fixtures):
+    '''
+    Test that sandboxing with directory isolation works correctly with `env/settings` values.
+    '''
+    # Mock away the things that would actually prepare the isolation directory.
+    mocker.patch('os.makedirs', return_value=True)
+    copy_tree = mocker.patch('distutils.dir_util.copy_tree')
+    mkdtemp = mocker.patch('tempfile.mkdtemp')
+    mkdtemp.return_value = '/tmp/runner/runner_di_XYZ'
+    mocker.patch('ansible_runner.config.runner.RunnerConfig.build_process_isolation_temp_dir')
+
+    # The `directory_isolation` test data sets up an `env/settings` file for us.
+    private_data_dir = project_fixtures / 'directory_isolation'
+    rc = RunnerConfig(private_data_dir=str(private_data_dir), playbook='main.yaml')
+
+    # This is where all the magic happens
+    rc.prepare()
+
+    assert rc.sandboxed
+    assert rc.process_isolation_executable == 'bwrap'
+    assert rc.project_dir == str(private_data_dir / 'project')
+    assert os.path.exists(rc.project_dir)
+
+    # `directory_isolation_path` should be used to create a new temp path underneath
+    assert rc.directory_isolation_path == '/tmp/runner/runner_di_XYZ'
+    mkdtemp.assert_called_once_with(prefix='runner_di_', dir='/tmp/runner')
+
+    # The project files should be copied to the isolation path.
+    copy_tree.assert_called_once_with(rc.project_dir, rc.directory_isolation_path, preserve_symlinks=True)
+
+
 def test_prepare_inventory(mocker):
     mocker.patch('os.makedirs', return_value=True)
     path_exists = mocker.patch('os.path.exists', return_value=True)