Comment 7 for bug 2007241

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote (last edit ):

I came across this bug while reviewing https://code.launchpad.net/~orndorffgrant/ubuntu/+source/ubuntu-advantage-tools/+git/ubuntu-advantage-tools/+merge/438992.

There, it proposes pushing the results of the following upstream change to the package: https://github.com/canonical/ubuntu-pro-client/pull/2448.

In short, the proposed fix creates a yaml module inside the uaclient package, explicitly imports the yaml package from /usr/lib/python3/dist-packages in this new module using importlib, and exposes the real yaml module and some of its methods by defining variables in the module's global scope.

I wanted to bring this discussion here so the approach taken gets more visibility so it can guide us in future similar issues.

IMHO, there are 3 issues with the current approach:

1) We (silently) take away the customization powers given to users by design;

2) we will need to edit the uaclient.yaml module whenever we want to use other features of the yaml package; and most importantly

3) the approach only applies to the yaml package, but any other package we depend on (directly or indirectly) could be affected.

There is not much we can do about (1) if we really want to make it harder to users to use custom python packages in their systems. Note that while adding the restriction may be beneficial to users who do not understand how those installations may affect their system, such restriction hinders usage for users who actually understand what they are doing and even though decide to use a local installation of a specific dependency. One example would be an upstream developer trying to verify how his latest local build of a package interacts with other systems by running integration tests in a CI env.

One alternative here would be to, instead of forcing the usage of system installed dependencies, detect if any loaded modules are coming from anywhere other than /usr/lib and throw a verbose warning about our concerns regarding those.

For direct dependencies such as yaml, this could be done with something like:

```
modulenames = set(sys.modules) & set(globals())
imported_paths = [sys.modules[name].__file__ for name in modulenames if hasattr(sys.modules[name], '__file__')]
for p in imported_path:
    if not p.startswith('/usr/lib/'):
        func_to_throw_verbose_warning()
```

This would need to be called after importing all the needed modules in every module. If we also want to deal with the cases described in LP: #2007234, where the user removed the dependency from the system path in /usr/lib and the python minor version got bumped (i.e., python will now look for local deps in /usr/local/pythonX.Y+1/ instead of pythonX.Y), we could also add try/except blocks for the import paths to provide better error messages as suggested by Christian (although, IMHO, this is not necessary since the default error will already inform the user the module is not available).

For (2), a possible work around would be to use uaclient.yaml.yaml instead of defining additional variables in the new module.

However, (3) is the major concern here.
The approach must apply to all external python packages being imported both directly and indirectly. Otherwise, we may eventually realize that we will need a wapper for python3-apt and python3-pkg-resources, or for any indirect dependency which users are installing in local paths.

An alternative approach, could be to edit sys.path in the package initializer (uaclient/__init__.py).

From the sys.path list, we could remove anything but '' and entries starting with '/usr/lib/'. from sys.path, forcing the users to use our system installed libraries. This have one downside already present in the current proposal: this hinders the setup of development environments. Still, this would fix (2) and (3) and would make the fix shorter.

thoughts?