Skip to content

Comments

Do not insert local paths before standard library paths#68756

Open
marmarek wants to merge 1 commit intosaltstack:3007.xfrom
marmarek:import-conflict
Open

Do not insert local paths before standard library paths#68756
marmarek wants to merge 1 commit intosaltstack:3007.xfrom
marmarek:import-conflict

Conversation

@marmarek
Copy link
Contributor

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.

Commits signed with GPG?

Yes

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.

Fixes saltstack#68755
@marmarek
Copy link
Contributor Author

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant