Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TECH DEBT] Salt fails to load top.sls on Python3.13rc2 #66898

Open
marmarek opened this issue Sep 18, 2024 · 6 comments · May be fixed by #66899
Open

[TECH DEBT] Salt fails to load top.sls on Python3.13rc2 #66898

marmarek opened this issue Sep 18, 2024 · 6 comments · May be fixed by #66899

Comments

@marmarek
Copy link
Contributor

Description of the tech debt to be addressed, include links and screenshots

A recent change in Python 3.13 causes salt to fails on loading top.sls. With debugging enabled, I see now:

[DEBUG   ] Could not find file 'salt://.sls' in saltenv 'base'
[DEBUG   ] No contents loaded for saltenv 'base'

This seems to be directly caused by the change in Python: python/cpython#85110

Old behavior:

>>> urllib.parse.urlunparse(("file", "", "top.sls", "", "", ""))
'file:///top.sls'

New behavior:

>>> urllib.parse.urlunparse(("file", "", "top.sls", "", "", ""))
'file:top.sls'

Technically, I think the latter might be more correct. But it still breaks salt's expectations. Specifically here:

salt/salt/utils/url.py

Lines 49 to 50 in 246d066

url = salt.utils.data.decode(urlunparse(("file", "", path, "", query, "")))
return "salt://{}".format(url[len("file:///") :])

With file:top.sls, looking for length of file:/// is wrong now.

Versions Report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

Salt Version:
          Salt: 3006.9
 
Python Version:
        Python: 3.13.0rc2 (main, Sep  7 2024, 00:00:00) [GCC 14.2.1 20240801 (Red Hat 14.2.1-1)]
 
Dependency Versions:
          cffi: 1.17.0
      cherrypy: Not Installed
  cryptography: Not Installed
      dateutil: 2.8.2
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.4
       libgit2: Not Installed
  looseversion: 1.3.0
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.6
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 24.1
     pycparser: 2.20
      pycrypto: Not Installed
  pycryptodome: 3.20.0
        pygit2: Not Installed
  python-gnupg: Not Installed
        PyYAML: 6.0.1
         PyZMQ: 25.1.1
        relenv: Not Installed
         smmap: Not Installed
       timelib: Not Installed
       Tornado: Not Installed
           ZMQ: 4.3.5
 
System Versions:
          dist: qubes 4.3 R4.3
        locale: utf-8
       machine: x86_64
       release: 6.6.48-1.qubes.fc41.x86_64
        system: Linux
       version: Qubes OS 4.3 R4.3

The same issue applies to Fedora 41 (in beta now). And while I tested using Salt 3006.9, the relevant code is the same in master.

@ptitdoc
Copy link

ptitdoc commented Sep 18, 2024

Take care that as if is related to cython it's independent from python3.13.

I have the same bug (and same fix working) for python 3.12.6/cython 3.0.11

@marmarek
Copy link
Contributor Author

Indeed Python 3.12.6 is affected too, and so is Fedora 40 and 39.

marmarek added a commit to marmarek/salt that referenced this issue Oct 4, 2024
Python 3.13 fixed handling relative paths in urllib.parse module.
Specifically, relative file URL is now constructed as file:path instead
of converting it to absolute file:///path. This breaks
salt.utils.url.create which expects file:/// specifically. The mismatch
results in for example changing salt://top.sls into salt://.sls and thus
not finding the top file.

Fix this by handling both prefixes.

Relevant python change: python/cpython#85110
Fixes: saltstack#66898
marmarek added a commit to marmarek/salt that referenced this issue Oct 4, 2024
Python 3.13 fixed handling relative paths in urllib.parse module.
Specifically, relative file URL is now constructed as file:path instead
of converting it to absolute file:///path. This breaks
salt.utils.url.create which expects file:/// specifically. The mismatch
results in for example changing salt://top.sls into salt://.sls and thus
not finding the top file.

Fix this by handling both prefixes.

Relevant python change: python/cpython#85110
Fixes: saltstack#66898
marmarek added a commit to marmarek/salt that referenced this issue Oct 7, 2024
Python 3.13 fixed handling relative paths in urllib.parse module.
Specifically, relative file URL is now constructed as file:path instead
of converting it to absolute file:///path. This breaks
salt.utils.url.create which expects file:/// specifically. The mismatch
results in for example changing salt://top.sls into salt://.sls and thus
not finding the top file.

Fix this by handling both prefixes.

Relevant python change: python/cpython#85110
Fixes: saltstack#66898
@jfindlay
Copy link
Contributor

This is the only bug I have seen so far running on python 3.12.

@jfindlay
Copy link
Contributor

jfindlay commented Oct 10, 2024

This seems to be directly caused by the change in Python: python/cpython#85110

This seems to be the effective point of code change for the upstream issue.

marmarek added a commit to marmarek/salt that referenced this issue Oct 12, 2024
Python 3.13 fixed handling relative paths in urllib.parse module.
Specifically, relative file URL is now constructed as file:path instead
of converting it to absolute file:///path. This breaks
salt.utils.url.create which expects file:/// specifically. The mismatch
results in for example changing salt://top.sls into salt://.sls and thus
not finding the top file.

Fix this by handling both prefixes.

Relevant python change: python/cpython#85110
Fixes: saltstack#66898
marmarek added a commit to marmarek/salt that referenced this issue Oct 12, 2024
Python 3.13 fixed handling relative paths in urllib.parse module.
Specifically, relative file URL is now constructed as file:path instead
of converting it to absolute file:///path. This breaks
salt.utils.url.create which expects file:/// specifically. The mismatch
results in for example changing salt://top.sls into salt://.sls and thus
not finding the top file.

Fix this by handling both prefixes.

Relevant python change: python/cpython#85110
Fixes: saltstack#66898
marmarek added a commit to marmarek/salt that referenced this issue Oct 12, 2024
Python 3.13 fixed handling relative paths in urllib.parse module.
Specifically, relative file URL is now constructed as file:path instead
of converting it to absolute file:///path. This breaks
salt.utils.url.create which expects file:/// specifically. The mismatch
results in for example changing salt://top.sls into salt://.sls and thus
not finding the top file.

Fix this by handling both prefixes.

Relevant python change: python/cpython#85110
Fixes: saltstack#66898
marmarek added a commit to marmarek/salt that referenced this issue Oct 13, 2024
Python 3.13 fixed handling relative paths in urllib.parse module.
Specifically, relative file URL is now constructed as file:path instead
of converting it to absolute file:///path. This breaks
salt.utils.url.create which expects file:/// specifically. The mismatch
results in for example changing salt://top.sls into salt://.sls and thus
not finding the top file.

Fix this by handling both prefixes.

Relevant python change: python/cpython#85110
Fixes: saltstack#66898
@hartwork
Copy link

hartwork commented Dec 18, 2024

@marmarek thanks for your report and candidate pull request #66899, it proved to be of great help already at #66588 (comment) 🙏

What I don't understand is the label "tech debt". It seems like a plain bug (and a big one that made salt-ssh completely unusable to me without the fix), and "tech debt" may send the wrong signal. Could it be dropped from the title? What was the idea?

@Mutisk
Copy link

Mutisk commented Dec 29, 2024

What I don't understand is the label "tech debt". It seems like a plain bug (and a big one that made salt-ssh completely unusable to me without the fix).

(+1 ...maybe)
It can also cause incorrect states to be executed.

# cat /srv/salt/k/init.sls
always-passes-with-any-kwarg k:
  test.nop:
    - name: foo
    - something: {{ tplfile }}
    - foo: bar

A show_sls then gives:

root@ryz1:/srv/salt# salt-call --local state.show_sls uuuuuuk -l debug
local:
    ----------
    always-passes-with-any-kwarg k:
        ----------
        test:
            |_
              ----------
              name:
                  foo
            |_
              ----------
              something:
                  /var/cache/salt/minion/files/base/k/init.sls
            |_
              ----------
              foo:
                  bar
            - nop
            |_
              ----------
              order:
                  10000
        __sls__:
            uuuuuuk
        __env__:
            base

Every preceeding 'u' in the state specification is ignored, and instead state 'k' is/would be applied.

Regarding "Tech Debt"-label; it has no describing text so I can only guess from it's name the bug is related to salt-stack falling behind the newest and greatest technology. I don't think it says or send any signal regarding it's severity, it's just a clue to getting it fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants