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

[BUG] Maintain version locking/compatibility/updates with optional modules #64299

Closed
4 of 9 tasks
DaAwesomeP opened this issue May 17, 2023 · 13 comments
Closed
4 of 9 tasks
Assignees
Labels
Bug broken, incorrect, or confusing behavior dependency underlying Salt dependency issue Packaging Related to packaging of Salt, not Salt's support for package management. Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged

Comments

@DaAwesomeP
Copy link
Contributor

Description
In an enterprise environment machines often sit behind a proxy. As such using Saltstack requires the use of pycurl. This broke in #62949 and then with relenv a solution is presented here: https://relenv.readthedocs.io/en/latest/additional-dependencies.html

This is more of a workaround than a solution. Similarly in enterprise environments machines are kept as clean as possible, so if the applications running on the machine do not require build tools then build tools will not be permitted to be installed.

Additionally, building modules from source creates then additional required upkeep to keep these modules up to date. Specifically with curl the chance of a CVE or other security vulnerability requiring a patch is a concern.

Manually compiling against the system libraries is also bound to create issues that are the same problem that relenv seeks to solve. Relenv does not make Saltstack easier to use or more consistent across machines if manual compilation is required.

Python modules may deprecate the ability to link with older dependencies provided by the OS. At the same time Saltstack may bundle a newer version of Python or other dependencies that prevent the use of older modules that can link with the OS. Ultimately, the user has to perform trial and error to figure out which version works now but might break or become a security concern in 6 months. See #58898 and #61790.

Now that onedir is a requirement this is becoming a large issue that could result in dropping Saltstack in environments behind proxies. I say this in the most constructive way possible; I enjoy using Saltstack but I currently have run out of workarounds.

Same issue with other packages than pycurl: #63142

This issue does not affect other modules that bundle wheels (such as pygit2) pending fix of #64121.

Setup
(Please provide relevant configs and/or SLS files (be sure to remove sensitive info. There is no general set-up of Salt.)

Please be as specific as possible and give set-up details.

  • on-prem machine
  • VM (Virtualbox, KVM, etc. please specify)
  • VM running on a cloud service, please be explicit and add details
  • container (Kubernetes, Docker, containerd, etc. please specify)
  • or a combination, please be explicit
  • jails if it is FreeBSD
  • classic packaging
  • onedir packaging
  • used bootstrap to install

Usually a RHEL8 x86_64 or Debian 11 amd64 machine with Saltstack installed via the official Saltstack repos.

Steps to Reproduce the behavior
N/A

Expected behavior
Pycurl installation should not require manual compilation.

Screenshots
If applicable, add screenshots to help explain your problem.

Versions Report
N/A

Additional context
N/A

@DaAwesomeP DaAwesomeP added Bug broken, incorrect, or confusing behavior needs-triage labels May 17, 2023
@OrangeDog OrangeDog added Packaging Related to packaging of Salt, not Salt's support for package management. dependency underlying Salt dependency issue labels May 18, 2023
@OrangeDog
Copy link
Contributor

Another related issue is #62758, which is additional friction for the only remaining feasible workaround.

@DaAwesomeP
Copy link
Contributor Author

DaAwesomeP commented May 18, 2023

There are a few possible solutions that I can think of:

  1. Bundle shared libraries and/or wheels with relenv
    Since all of the required modules are known (and reported by salt --versions-report), the required shared libraries and/or wheels could come preinstalled in the salt relenv. Wheels are probably more difficult to bundle in relenv but the shared libraries should be relatively simple and would effectively solve the issue.

    This is probably the least maintenance-intensive solution for the Saltstack maintainers. Security patches would be released by increasing the build number of the package (i.e. on Debian 3006.1-1, 3006.1-2, etc.).

    This also ties in nicely with [TECH DEBT] Automate tests for packaging and onedir with optional modules #64296 to automate tests that check if these optional modules break.

    This fulfills the promise of relenv by ensuring that every installation runs the same known version of each module. Each module will be effectively tied to the installed Saltstack version so no version-mismatching/compatibility issues will arise.

    I think this also solves a lot of the unclear documentation issues with these modules. Currently, different pages across the Saltstack documentation mention different methods of installing these modules or specific (possibly out of date) versions to use. If they come preinstalled and matched to the Saltstack version, then this issue goes away.

  2. Distribute wheels separately
    This is not my favorite solution but it is a possible one. Saltstack can add an additional package source to salt-pip in a similar way that piwheels does (adding an additional repo to pip). This means Saltstack maintainers would have to maintain all of these wheels keep them up-to-date in a more manual fashion not tied to Saltstack releases. These wheels would also need to be separately tested to work on each supported Saltstack platform (whereas in the bundling method these can be tested easily at the packaging stage for each platform).

@DaAwesomeP DaAwesomeP changed the title [BUG] pycurl Installation Not Always Feasible [BUG] Bundle Optional Modules with relenv (pycurl Installation Not Always Feasible) May 18, 2023
@OrangeDog
Copy link
Contributor

OrangeDog commented May 18, 2023

and reported by salt --versions-report

That's not all of them. There are "core" Salt modules with dependencies not on that list. People can also add/write extensions with any dependencies they want. I've got one for berkeleydb, for example.

@DaAwesomeP
Copy link
Contributor Author

That's not all of them. There are "core" Salt modules with dependencies not on that list.

I would assume that the "core" modules are already bundle or already working otherwise onedir wouldn't function at all? I think at the bare minimum bundling dependencies for the Saltstack states and modules included out-of-the-box would be a big step. This means the likes of pycurl, python-ldap, and etc.

Really this issue is to solve modules that are:

  1. Modules officially supported by/depended on by Saltstack out-of-the-box (not user modules)
  2. Any of:
    a. Modules that do not ship prebuilt wheels/shared libraries via Pypi (pycurl, python-ldap)
    b. Modules where tying a module version to Saltstack greatly reduces compatibility issues (pygit2, python-docker)
    c. Modules that should be tested as a part of releasing since they are officially supported by Saltstack (all of the above)

People can also add/write extensions with any dependencies they want. I've got one for berkeleydb, for example.

Installing a user-provided extension is understandably harder to support and would maybe rely on the extension providing further dependencies; that is a much bigger problem to solve. Those modules have to be built somewhere, but I think it is hard to place the responsibility on the Saltstack maintainers for every possible user-provided module. Extension developers could possibly package and distribute their own wheels either via Pypi or #62758 (I definitely agree that would be a great feature). I know this possibly requires a separate mechanism for distributing user-provided extensions, but I think it is reasonable to place that out of the scope of distributing officially supported modules.

@ITJamie
Copy link
Contributor

ITJamie commented May 24, 2023

use of 3005.x classic packaging builds is going to become long in the tooth until this works.

I keep trying to move to "onedir"/relenv/whatever comes next builds of salt, but i keep hitting walls. The concept is good, the reality is painful and feels like core modules were never tested (nacl on debian 11 segfaults #64342)

at a minimum the core modules need to work (nacl, pycurl, python-ldap, gitfs, (ive not even gotten to the point of trying docker modules yet, or salt-proxys with napalm....) ).

@dwoz dwoz added Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged and removed needs-triage labels May 24, 2023
@dwoz dwoz modified the milestones: Sulfur v3006.0, Chlorine v3007.0 May 24, 2023
@Ch3LL Ch3LL modified the milestones: Sulfur v3006.0, Sulfur v3006.2 May 25, 2023
@dwoz
Copy link
Contributor

dwoz commented May 26, 2023

Our take on this is that python libraries which have c dependencies should be distributing wheels. Salt should do it's best to reduce dependencies on libraries that require compilation in favor of pure python versions, versions using cffi/ctypes and libraries that are being maintained by shipping wheels (cryptography is a good example). I understand that today this is not always the case and the comments made about users having to compile software to enable features in Salt are entirely valid. Bundling additional c dependencies in a onedir build is not completely off the table but it is something we'd like to avoid. I'll address our plans for the issues listed thus far.

Salt extensions are, in general a good approach for handling functionality that requires third party libraries not bundled with Salt. We have started down the path of migrating a lot of the bloat out of the core Salt repository and into extensions. At some point in the future this should mean that Salt has far less dependencies in the core codebase.

I hope this provides everyone with good understanding of how we plan to tackle these issues. As I stated above; the concerns here are valid. I hope this alleviates those concerns. I am looking forward to the discussions that follow.

@DaAwesomeP
Copy link
Contributor Author

Since we're only using pycurl for proxy support in salt.utils.http I think it makes sense to remove this dependency. python-requests can certainly handle the task. I've opened

Amazing!

Salt extensions are, in general a good approach for handling functionality that requires third party libraries not bundled with Salt. We have started down the path of migrating a lot of the bloat out of the core Salt repository and into extensions. At some point in the future this should mean that Salt has far less dependencies in the core codebase.

If this is the intended path then I think there should be much better version locking/compatibility with these dependencies. Currently to install a module you run salt-pip install and it installs the latest version, and this has the potential to break compatibility. Even if these dependencies aren't bundled we need a way to choose which versions will be tested against (see #64296) and which versions are compatible with specific versions of Saltstack. The Docker module specifically often has API changes that break.

I think we should try to either:

  1. Create dummy wrapper packages that depend on these dependencies. For example, you could run salt-pip install salt-module-pygit2==3006.1 to install the version of pygit2 known to work with that version of Saltstack. We could probably have the deb/rpm/etc packages automatically update these Pypi packages if they are found to be installed.
  2. Specify optional-dependencies for existing Saltstack modules to try to influence salt-pip to install and update compatible versions. Although in my experience this can cause annoying issues with updating.

@DaAwesomeP
Copy link
Contributor Author

I hope this provides everyone with good understanding of how we plan to tackle these issues. As I stated above; the concerns here are valid. I hope this alleviates those concerns. I am looking forward to the discussions that follow.

I am really happy that we are moving toward solving these issues! Thank you for all of your work!

@OrangeDog
Copy link
Contributor

this has the potential to break compatibility

Indeed. But if you don't install the latest then you're going to miss security fixes, as most python libraries do not maintain their own old versions. The only way to maintain both security and stability is either using the distro-managed packages, or having Salt bundling everything itself and backporting any fixes.

@ITJamie
Copy link
Contributor

ITJamie commented May 27, 2023

The big elephant in the room is #64296, testing of the ability to actually install and run these modules across the built releases on each os. Otherwise were only going to catch these kind of issues with user reports.

@DaAwesomeP
Copy link
Contributor Author

this has the potential to break compatibility

Indeed. But if you don't install the latest then you're going to miss security fixes, as most python libraries do not maintain their own old versions. The only way to maintain both security and stability is either using the distro-managed packages, or having Salt bundling everything itself and backporting any fixes.

I don't think this actually requires bundling. It does require Salt to pay attention and track these security updates. If there is a security update for a module, then Salt can release a patch that updates it's optional dependency version constraint (not actually bundled, just an optional dependency for Pip) without actually shipping the modules themselves or needing to update any other parts of Salt.

Obviously there will eventually be a dead end with some of these modules between Salt's LTS release timeline and the modules making breaking changes, but I think it is reasonable to match working versions. When a security issue is found but there is not a compatible replacement for an older Salt version, Salt should probably release a CVE and say "using this module with this version of Salt is now a security risk and no longer recommend; since the patched versions are not compatible with this Salt version please update to the next version of Salt." Without this announcement users may simply roll back the module they are trying to install without any notice of security risk. There are already a lot GitHub issues describing different old versions to lock into for compatibility.

Also probably most users do not remember to update salt-pip modules after initial install as it is. Automating salt-pip to bring optional dependencies up to the latest secure and compatible version when salt-common or the Salt deb/rpm package updates helps this issue.

@anilsil anilsil removed this from the Sulfur v3006.3 milestone Sep 8, 2023
@anilsil anilsil added this to the Sulfur v3006.4 milestone Sep 8, 2023
@DaAwesomeP DaAwesomeP changed the title [BUG] Bundle Optional Modules with relenv (pycurl Installation Not Always Feasible) [BUG] Maintain version locking/compatibility/updates with optional modules Nov 2, 2023
@dwoz dwoz modified the milestones: Sulfur v3006.5, Sulfur v3006.9 May 1, 2024
@dwoz
Copy link
Contributor

dwoz commented Jun 18, 2024

@DaAwesomeP The curl backend was replaced by requests and proxy support should now (in 3006.8) be working. Closing this one.

@dwoz dwoz closed this as completed Jun 18, 2024
@DaAwesomeP
Copy link
Contributor Author

@dwoz I don't think that resolves this issue. This issue is about all Salt optional dependencies and their lack of version locking/matching with the current Salt version, not just that one in particular. Even if the current specific issues due to individual optional dependencies' breaking changes are fixed, that still doesn't address that it can continue to happen in the future (as it has many times before).

Please see earlier comments about a few suggested schemes to prevent dependency issues in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior dependency underlying Salt dependency issue Packaging Related to packaging of Salt, not Salt's support for package management. Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

No branches or pull requests

8 participants