fix(utils/url): handle Windows relative path backslash when caching file

[myii]

What does this PR do?

Jinja imports with relative paths was introduced in #47490.

Unfortunately, relative imports have never been working on Windows.
That's due to backslashes being unhandled by the cache_file function:

• salt/salt/utils/jinja.py

  Line 114 in 7275ecf

  saltpath = salt.utils.url.create(template)

Instead of getting:

• salt://packages/map.jinja

We're ending up with:

• salt://packages\map.jinja

Thus, the file isn't cached from the master.

There's actually a FIXME note about it:

• salt/salt/utils/jinja.py

  Line 136 in 7275ecf

  # FIXME: somewhere do seprataor replacement: '\\' => '/'

And the replacement is actually done for setting tpldir:

• salt/salt/utils/jinja.py

  Line 165 in 7275ecf

  tpldir = os.path.dirname(_template).replace("\\", "/")

This commit ensures the replacement is also done when caching files.

CC: @twangboy.

What issues does this PR fix or reference?

Fix #61040.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

[waynew]

🙌 Awesome find/fix!

We discussed this a bit on Slack, and I think we've come to the conclusion that this is the right fix in the wrong place 🙃

Specifically, my thinking is that salt:// is a URI (and arguably URL, and certainly URN as well), and since RFC 2396 doesn't allow the backslash, and even file:// uses forward slashes for Windows paths... I'm confident that salt:// URI/URLs should always use / from within a Salt perspective. I'm OK if backslashes accidentally work because Python or whatever, but our documentation should (if it doesn't already) state that salt:// URI/URLs should only use forward slashes, regardless of the salt master or salt minion operating system.

Naturally, the Salt master & minion should translate salt:// URLs to the appropriate underlying file paths to match their OSes preference, but I'm pretty sure that's already happening.

Soooo all of that being said, I'm totally on board with this fix, but since the documentation says that we're creating a salt:// url (

salt/salt/utils/url.py

Line 42 in fd216e1

join `path` and `saltenv` into a 'salt://' URL.

) then we should make sure that nothing comes out of that function wearing \ - / only.

[myii]

Thanks for the advice, @waynew. I made the adjustments and added a couple of tests. These are the failures before the proposed fix is applied:

======================================================================
FAIL: test_create_url_with_backslash_in_path (tests.unit.utils.test_url.UrlTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../salt/tests/unit/utils/test_url.py", line 81, in test_create_url_with_backslash_in_path
    self.assertEqual(salt.utils.url.create(src_path), url)
AssertionError: 'salt://? interesting\\&path.filetype' != 'salt://? interesting/&path.filetype'
- salt://? interesting\&path.filetype
?                     ^
+ salt://? interesting/&path.filetype
?                     ^


======================================================================
FAIL: test_create_url_saltenv_with_backslash_in_path (tests.unit.utils.test_url.UrlTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../salt/tests/unit/utils/test_url.py", line 95, in test_create_url_saltenv_with_backslash_in_path
    self.assertEqual(salt.utils.url.create(src_path, saltenv), url)
AssertionError: 'salt://? interesting\\&path.filetype?saltenv=raumklang' != 'salt://? interesting/&path.filetype?saltenv=raumklang'
- salt://? interesting\&path.filetype?saltenv=raumklang
?                     ^
+ salt://? interesting/&path.filetype?saltenv=raumklang
?                     ^

[waynew]

https://jenkins.saltproject.io/job/pr-windows-2019-x64-py3-pytest/job/PR-61829/3/testReport/tests.pytests.unit.utils.jinja/test_salt_cache_loader/test_relative_import/ Mentioned in chat, but looks like there's a test that was requesting the broken behavior 🙃

Should just be a matter of changing the \ to / in that test, though I'd double check to make sure that it really is an incorrect test 🤔

[waynew]

Heeeey! Checks all passing! 👍 must've just been some flaky infrastructure