Do not insert local paths before standard library paths

[marmarek]

What does this PR do?

Inserting local paths like salt/utils before standard library makes many modules in utils conflict with standard library. This is especially relevant for salt-ssh. For example salt.utils.configparser - it tries to import configparser from stdlib, but due to salt/utils path being prepended to sys.path, it imports itself:

[ERROR   ] Failed to import fileserver gitfs, this is due most likely to a syntax error:
Traceback (most recent call last):
  File "/var/tmp/.root_dd8a91_salt/pyall/salt/loader/lazy.py", line 902, in _load_module
    self.run(spec.loader.exec_module, mod)
    ~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/tmp/.root_dd8a91_salt/pyall/salt/loader/lazy.py", line 1365, in run
    return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/tmp/.root_dd8a91_salt/pyall/salt/loader/lazy.py", line 1380, in _run_as
    ret = _func_or_method(*args, **kwargs)
  File "<frozen importlib._bootstrap_external>", line 1023, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "/var/tmp/.root_dd8a91_salt/pyall/salt/fileserver/gitfs.py", line 52, in <module>
    import salt.utils.gitfs
  File "/var/tmp/.root_dd8a91_salt/pyall/salt/utils/gitfs.py", line 31, in <module>
    import salt.utils.configparser
  File "/var/tmp/.root_dd8a91_salt/pyall/salt/utils/configparser.py", line 7, in <module>
    from configparser import *  # pylint: disable=no-name-in-module,wildcard-import,unused-wildcard-import
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/tmp/.root_dd8a91_salt/running_data/var/cache/salt/minion/extmods/utils/configparser.py", line 12, in <module>
    class GitConfigParser(RawConfigParser):
                          ^^^^^^^^^^^^^^^
NameError: name 'RawConfigParser' is not defined

On the other hand, places that try to use such duplicated modules from utils, import them via full name (as seen above: import salt.utils.configparser).

Change the insert_system_path() function to not insert paths before standard library.

What issues does this PR fix or reference?

Fixes #68755

Previous Behavior

Several state modules (including pkg.installed on RedHat-based distro) are unavailable via salt-ssh

New Behavior

No import conflict, pkg.installed works.

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

[marmarek]

The failure I get is:

_______________________________________________________________________________________ test_allow_tgt _______________________________________________________________________________________

salt_run_cli = SaltRun(id='master-V1jApH', config_file='/tmp/stsuite/master-V1jApH/conf/master', config_dir='/tmp/stsuite/master-V1jA...slow_stop=True, timeout=30, script_name='/tmp/stsuite/scripts/cli_salt_run.py', base_script_args=[], display_name=None)
salt_minion = SaltMinion(id='minion-TwcZ68', config_file='/tmp/stsuite/minion-TwcZ68/conf/minion', config_dir='/tmp/stsuite/minion-T...=[], extra_cli_arguments_after_first_start_failure=['--log-level=info'], display_name="SaltMinion(id='minion-TwcZ68')")

    @pytest.mark.usefixtures("pillar_tree", "master_id", "salt_minion_id")
    def test_allow_tgt(salt_run_cli, salt_minion):
        tgt = salt_minion.id
        fun = "test_fun"
    
        ret = salt_run_cli.run("mine.get", tgt, fun)
>       assert ret.data == {salt_minion.id: "hello test"}
E       AssertionError: assert {} == {'minion-TwcZ68': 'hello test'}
E         
E         Right contains 1 more item:
E         {'minion-TwcZ68': 'hello test'}
E         
E         Full diff:
E         + {}
E         - {
E         -     'minion-TwcZ68': 'hello test',
E         - }

Is it related to my change? I think it isn't given I got it also in https://github.com/saltstack/salt/actions/runs/22261566839, which doesn't include this change.

[twangboy]

Please create this against the 3006.x branch as the bug also exists there.

[marmarek]

Please create this against the 3006.x branch as the bug also exists there.

Done.

[marmarek]

Would you be able to write a test for this?

I can try, but I believe it might be related to python version (not sure why else I didn't hit it before). With Fedora 42 target, reproducer is as simple as salt-ssh target state.single pkg.installed mc. I haven't managed to reproduce the issue on old versions currently in CI. I tried re-enabling salt-ssh tests on Ubuntu 24.04 or Fedora 42, but that looks like way more complex thing.

I can try harder to come up with some state that would trigger the issue regardless of distribution version.

[tidenhub]

We were able to reproduce the issue for openSUSE Leap 15.6 with Python 3.11. And we are using this patch now to fix our deployment.

[marmarek]

@twangboy do you have some idea what is different in CI that the bug doesn't happen here? In the process of testing the test, I found another sys.path related bug (3acb784) that kinda cancel out the initial issue, but even after fixing it, the test doesn't detect the problem. Note this branch now has the fix reverted, so the test should fail. I tried also some hacks with limiting what modules get loaded, to limit scope of what could interfere, but that also didn't help

[twangboy]

I don't, maybe @dwoz could take a look. You're saying that the passing tests below should not be passing, correct?

[marmarek]

You're saying that the passing tests below should not be passing, correct?

Yes...

[frebib]

Any sign of this getting fixed up and merged? This problem has bitten us many times in the past too and I'd love to see it merged.

Hopefully this ridiculous problem serves as a warning to anyone else thinking they could fuck with sys.modules