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

Add missing requires python3-cryptography for x509 #99

Open
wants to merge 1 commit into
base: release/3006.0
Choose a base branch
from

Conversation

asdil12
Copy link
Member

@asdil12 asdil12 commented Oct 11, 2024

See https://progress.opensuse.org/issues/167830

[DEBUG   ] Failed to import utils x509:
Traceback (most recent call last):
  File "/usr/lib/python3.11/site-packages/salt/loader/lazy.py", line 772, in _load_module
    mod = self.run(spec.loader.load_module)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/salt/loader/lazy.py", line 1234, in run
    return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/salt/loader/lazy.py", line 1249, in _run_as
    ret = _func_or_method(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap_external>", line 605, in _check_name_wrapper
  File "<frozen importlib._bootstrap_external>", line 1120, in load_module
  File "<frozen importlib._bootstrap_external>", line 945, in load_module
  File "<frozen importlib._bootstrap>", line 290, in _load_module_shim
  File "<frozen importlib._bootstrap>", line 721, in _load
  File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 940, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/usr/lib/python3.11/site-packages/salt/utils/x509.py", line 11, in <module>
    import cryptography
ModuleNotFoundError: No module named 'cryptography'

@vzhestkov
Copy link
Contributor

@asdil12 it's a debug message on attempt to load custom module, which is intended to work this way and cryptography is not a hard dependency for salt in this case, but it's required if you need to call some x509 module functions. I would suggest not to add cryptography as a hard dependency to the package as the most of internal cryptography related calls are performed with M2Crypto and it's present in the dependencies.

In case of using x509 modules on the minion side it's better to include the package as the dependency part for the state.

@asdil12
Copy link
Member Author

asdil12 commented Oct 15, 2024

Is the additional dependency really that expensive that we risk having runtime crashes when making use of the x509 module?

@vzhestkov
Copy link
Contributor

It's not a crash of the service, it's just the debug message that the module can't be loaded. This message is not visible with the default logging level. Better to use Recommends: python3-cryptography there, but I would personally vote for not having it at all, as x509 is not used by default by most of the users, so for the others this dependency is redundant.

@asdil12
Copy link
Member Author

asdil12 commented Oct 17, 2024

I think that if a package contains a file that imports a module, the package should require all the dependencies.
It is such a huge improvement on user experience. Also it IMHO makes a very bad impression.
It is beyond annoying to figure out that a dependency is missing. The fact that this error is not properly shown in every log level makes it even worse by hiding the error.
And what for? Saving 100kb of disk space?

@vzhestkov
Copy link
Contributor

There is a list of the modules which are shipped with the salt, but are not intended to be used on all possible systems, for example there are windows related modules which dont make any sense in linux but are there anyway, and most of the modules are checking if the module can be used on the client with __virtual__ method, if there is something preventing certain module to be loaded it doesn't mean that everything shoulld be included to the salt package dependencies.
If you need any specific functions from salt which are not availabe with the default installation it's better to install the dependencies for such modules explicitly with state for examle or during the system deployment.

@vzhestkov
Copy link
Contributor

I just checked the output from https://gitlab.suse.de/openqa/salt-states-openqa/-/jobs/3195852 and it actually means that x509 module can't be loaded due to the missing dependency, just the message in DEBUG log looks so scary and it's the only issue I see with it. But it's a bad idea to solve such cosmetic messages by adding extra and extra dependencies for the package.

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.

2 participants