[TEST FAILURE] failures of Windows functional tests with "ValueError: Paths don't have the same drive"

[barneysowood]

What's happened

Some integration tests were failing, because the modules that they rely on were not available in the base saltenv when they were running.

• modules are in the git repo at tests/integration/files/file/base/
• this path is added to the file_roots for the salt master provided by pytest-salt-factories
• the other directories configured in file_roots are below the directory provided by the salt_factories_default_root_dir fixture
• on windows salt_factories_default_root_dir defaults to C:\temp

On the GH hosted windows runners, there are two drives, C: and D: - the first being used for the Windows install, software etc and the second being used for runner operations.

The checkout GH action checks out the repo to D:\a\salt\salt. I suspect on the original AWS runners there was only one drive (C:).

The failure that was occuring was this (from #67140 (comment)):

16:01:30,027 [salt.master                                                         :1933][ERROR   ][MWorker-0(1276)] [SaltMaster(id='master-PeY1oA')] Error in function _serve_file:
Traceback (most recent call last):
  File "D:\a\salt\salt\artifacts\salt\Lib\site-packages\salt\master.py", line 1927, in run_func
    ret = getattr(self, func)(load)
  File "D:\a\salt\salt\artifacts\salt\Lib\site-packages\salt\fileserver\__init__.py", line 618, in serve_file
    return self.servers[fstr](load, fnd)
  File "D:\a\salt\salt\artifacts\salt\Lib\site-packages\salt\loader\lazy.py", line 159, in __call__
    ret = self.loader.run(run_func, *args, **kwargs)
  File "D:\a\salt\salt\artifacts\salt\Lib\site-packages\salt\loader\lazy.py", line 1245, in run
    return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
  File "D:\a\salt\salt\artifacts\salt\Lib\site-packages\salt\loader\lazy.py", line 1260, in _run_as
    ret = _func_or_method(*args, **kwargs)
  File "D:\a\salt\salt\artifacts\salt\Lib\site-packages\salt\fileserver\roots.py", line 154, in serve_file
    if salt.utils.verify.clean_path(
  File "D:\a\salt\salt\artifacts\salt\Lib\site-packages\salt\utils\verify.py", line 533, in clean_path
    if os.path.commonpath([path, root]) == root:
  File "D:\a\salt\salt\artifacts\salt\lib\ntpath.py", line 807, in commonpath
    raise ValueError("Paths don't have the same drive")
ValueError: Paths don't have the same drive

I suspect that the intent of this is to stop path traversal attacks.

What this means is we need the various saltenvs defined in file_roots to all be on the same drive.

Initial fix

As the intent of the D: drive on the GH hosted runners appears to be working space for the runner and that is already where the git repo is checked out, it seemed reasonable to make that the target drive.

To fix the initial issue with integration tests, I modified the salt_factories_default_root_dir to check if we are running on Windows and in the CI environment and if so, make the value returned be $RUNNER_TEMP\stsuite, where $RUNNER_TEMP is a directory setup by GH Actions to provide temp space that is cleared between jobs and points to D:\a\_temp - see https://github.com/saltstack/salt/pull/67164/files#diff-e52e4ddd58b7ef887ab03%20c04116e676f6280b824ab7469d5d3080e5cba4f2128

This resolved the issue for the integration tests and was merged in #67164

New issues

Whilst the initial fix resolved the issues for the integration tests, it caused a number of functional tests to start failing:

ERROR tests/pytests/functional/modules/state/test_jinja_renderer.py::test_jinja_renderer_argline - ValueError: Paths don't have the same drive

These tests use the state_tree fixture from tests/pytests/functional/conftest.py to be able to create temporary states and have thoe available to the tests.

The state_tree fixture uses the builtin pytest fixture tmp_path_factory to get a temporary directory as documented here. That uses the python tempfile.gettempdir() function, which searches for various env vars then, on windows resorts to some defaults on C: as documented here.

This all means that the temporary state trees are created on C: whilst rootdir for the pytest-salt-factories salt masters are on D: triggering the same exception.

Possible fixes

1. Revert Convert test_custom grains test to pytest and fix integration tests fileserver issue #67164 and update the integration tests to copy their states and modules from the checked out repo on D: to a temporary saltenv dir on C:. Whilst this would work, these integration tests are not pytest tests, so it would take quite a lot of work to fix the relevant classes and fixture to do this and that work will be pretty much thrown away when these get migrated to pytest.

2. Alter the directory returned by tmp_path_factory to one on D:, specifically using the value of $RUNNER_TEMP as temproot for pytest. There are some different way that can be done. Documenting here as I'm not completely sure which is the best option:

2.1 Alter the commandline for pytest in CI envs on widows to pass --basetemp=${RUNNER_TEMP}

2.2 Alter the output of tempfile.gettempdir() by setting one of the environment vars it inspects. This should only be set in a CI environment. It shouldn't matter if we set it for all runners (ie not just windows), although it may be better to just scope it to windows if that is practical.

[barneysowood]

Testing a fix for that in #67188 by setting TMPDIR == ${{ vars.RUNNER_TMP }}. If that actually works, probably want some feedback from @dwoz as to whether .github/workflows/test-action.yml is really the right place to set that.

[barneysowood]

Occurs to me that it would probably be sensible to add a couple of tests on windows:

• In tests/integration to check that the path to the git repo and the path returned by salt_factories_default_root_dir are on the same drive
• In tests/pytest/functional to check that the path returned by tmp_path_factory and the path returned by salt_factories_default_root_dir are on the same drive

At least that way we can catch those issues early and with a clear reason

[barneysowood]

Testing a fix for that in #67188 by setting TMPDIR == ${{ vars.RUNNER_TMP }}. If that actually works, probably want some feedback from @dwoz as to whether .github/workflows/test-action.yml is really the right place to set that.

Actually needed to use ${{ runner.temp }} and that had to in the context of a job (otherwise I guess it doesn't know details about the runner). That's now working.

Had a further issue due to some win_dacl tests (see here). Worked out a fix for that, but they may want re-visiting to make them a bit more robust.