-
Notifications
You must be signed in to change notification settings - Fork 163
Add detection for third-party target Python modules #1400
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
Note that first time contributors cannot run tests automatically - they need to be started by a reviewer. It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported,
See other labels for particular jobs defined in the Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra. |
3c38570 to
c7ae623
Compare
repos/system_upgrade/common/actors/checkunsafepythonpaths/libraries/checkunsafepythonpaths.py
Outdated
Show resolved
Hide resolved
c7ae623 to
c4c7af3
Compare
|
Note that current solution doesn't have solution for preventing actual broken system after reboot. |
90c9fdc to
21709b0
Compare
282f582 to
05d4ca9
Compare
05d4ca9 to
b70c6ec
Compare
|
/packit copr-build |
64448bb to
e8a3486
Compare
repos/system_upgrade/common/actors/scanunsafepythonpaths/actor.py
Outdated
Show resolved
Hide resolved
pirat89
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs yet some changes. See my comments for more details.
| """ | ||
| topic = SystemInfoTopic | ||
|
|
||
| is_third_party_module_present = fields.Boolean(default=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a question whether it has a value if third party python modules are listed. but currently they are not listed in the model et
| third_party_rpm_names = fields.List(fields.String(), default=[]) | ||
| """ | ||
| List of names of RPMs that own third-party Python modules. Empty list if no modules found. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly we could hav ethe list of such third party python modules for the target python version listed here as well
repos/system_upgrade/common/actors/checkunsafepythonpaths/actor.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/checkunsafepythonpaths/libraries/checkunsafepythonpaths.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/checkunsafepythonpaths/libraries/checkunsafepythonpaths.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/checkunsafepythonpaths/libraries/checkunsafepythonpaths.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/common/actors/scanunsafepythonpaths/libraries/scanunsafepythonpaths.py
Outdated
Show resolved
Hide resolved
a16a8bd to
2c934cd
Compare
50aefbd to
1352064
Compare
| if third_party_rpms: | ||
| api.current_logger().info( | ||
| 'Complete list of third-party RPM packages:\n{}'.format( | ||
| '\n'.join(' - {}'.format(rpm) for rpm in third_party_rpms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use please the FMT_LIST_SEPARATOR = '\n - ' here as well - that's for all other logs as well. also I suggest to use the _formatted_list_output function, so the refactoring in future will be easier.
029f602 to
e07c47c
Compare
Introduce actors to detect presence of third-party Python modules installed for target Python. Those modules could interfere with the upgrade process or cause issues after rebooting into the target system. Scanner (scanthirdpartytargetpythonmodules): - Identifies the target Python interpreter - Queries the target Python's sys.path to determine where it searches for modules - Recursively scans these directories for Python files (.py, .so, .pyc) - Cross-references found files against the RPM database to determine ownership and categorize them Checker (checkthirdpartytargetpythonmodules) creates a high severity report to inform users about findings and presents full list of them in logs and short version in report. Jira: RHEL-71882
e07c47c to
34a5d75
Compare
MichalHe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solution looks good logic-wise 👍 . Probably the biggest issue is the code emitting warnings into the upgrade log about something that is considered normal and should not have a potential to negatively impact the upgrade process.
| reporting.Summary( | ||
| 'Third-party target Python modules may interfere with ' | ||
| 'the upgrade process or cause unexpected behavior after the upgrade.\n\n' | ||
| 'Non-distribution RPM packages detected:{}\n\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there are no third party rpms? The output would contain only:
Non-distribution RPM packages detected:
Non-distribution modules detected (list possibly incomplete): ...
Which I think will be confusing for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This report will be created only if there are found any modules or RPMs, however, you are right that it's possible that only one of it will be present.
| result = run([python_interpreter, '-c', 'import sys, json; print(json.dumps(sys.path))'])['stdout'] | ||
| return json.loads(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are frequently ad-hoc constructing a Path object and then throwing it away (when resolving symlinks, when using rglob). Why not construct it once, and use it where required?
| result = run([python_interpreter, '-c', 'import sys, json; print(json.dumps(sys.path))'])['stdout'] | |
| return json.loads(result) | |
| result = run([python_interpreter, '-c', 'import sys, json; print(json.dumps(sys.path))'])['stdout'] | |
| raw_paths = json.loads(result) | |
| paths = [Path(raw_path) for raw_path in raw_paths] | |
| return paths |
...mmon/actors/scanthirdpartytargetpythonmodules/libraries/scanthirdpartytargetpythonmodules.py
Show resolved
Hide resolved
| api.current_logger().warning( | ||
| 'Found Python files from non-distribution RPM package: {}'.format(rpm_name) | ||
| ) | ||
| third_party_files.extend([str(f) for f in files]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files should already be a list of strings, right?
| third_party_files.extend([str(f) for f in files]) | |
| third_party_files.extend(files) |
|
|
||
| rpms_to_check, third_party_unowned_files = scan_python_files(system_paths[1:], rpm_files) | ||
|
|
||
| third_party_rpms, rpm_owned_files = identify_unsigned_rpms(rpms_to_check) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is confusing, I was not sure whether these files come from signed or unsigned RPMs (= whether they are problematic or not)
| third_party_rpms, rpm_owned_files = identify_unsigned_rpms(rpms_to_check) | |
| third_party_rpms, unsigned_rpm_files = identify_unsigned_rpms(rpms_to_check) |
Uh oh!
There was an error while loading. Please reload this page.