search

oamg / leapp-repository

Leapp repositories containing actors for the Leapp framework (https://github.com/oamg/leapp). Currently provides leapp repositories for in-place upgrades of RHEL systems.
Apache License 2.0
48 stars 144 forks source link

Custom modifications tracking #1148

Closed fernflower closed 8 months ago

fernflower commented 10 months ago

~Please do NOT review until marked as ready. All custom modifications ideas will be put there.~

Report any customisations related to the upgrade process and the upgrade tooling:

The solution introduces CustomModification model which represents the result of the scan.

jira: RHEL-1774

github-actions[bot] commented 10 months ago

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. If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp from PR#42, use /packit test oamg/leapp#42

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones. To launch on-demand tests with packit:

[Deprecated] To launch on-demand regression testing public members of oamg organization can leave the following comment:

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.

fernflower commented 9 months ago

So on a pristine vm with leapp-from-this-pr upon leapp preupgrade --no-rhsm with a new file dropped-in and another actor opened&saved one would get:


HIGH and MEDIUM severity reports:
    1. Packages available in excluded repositories will not be installed
    2. Packages not signed by Red Hat found on the system
    3. Remote root logins globally allowed using password
    4. GRUB2 core will be automatically updated during the upgrade
    5. Custom files were discovered on your system in leapp directories
    6. Modified files were discovered on your system in leapp directories

leapp-report.txt

----------------------------------------
Risk Factor: high 
Title: Custom files were discovered on your system in leapp directories
Summary: Apparently some custom files have been found in leapp installation directories.
Please consult the list of discovered files for more information:
- /usr/share/leapp-repository/repositories/system_upgrade/common/actors/unexpecteddir/nonactorfile
Key: e87b6e8611e851b8447d262f3a1618f6ef0af170
----------------------------------------
Risk Factor: high 
Title: Modified files were discovered on your system in leapp directories
Summary: Apparently some modified files have been found in leapp installation directories.
Please consult the list of discovered files for more information:
- /usr/share/leapp-repository/repositories/system_upgrade/common/actors/checkcustommodifications/__pycache__/actor.cpython-36.pyc
- /usr/share/leapp-repository/repositories/system_upgrade/common/actors/checkcustommodifications/actor.py (CheckCustomModificationsActor Actor)
Key: b291e663e6859bdaf5cefc92e7b6f65eb977283b
----------------------------------------
pirat89 commented 9 months ago

@fernflower once you add also checks for configuration files, we will need an extra paragraph in the report when repomap.json file is modified, as e.g. in case of private AWS regions the file must be always modified now and printing people high risk without closer explanation could be problematic. I will help you later to prepare the wording for it.

fernflower commented 9 months ago

@pirat89 What configuration files are expected to be checked? The usual trio of pes data, repomap and hw deprecation?

pirat89 commented 9 months ago

@pirat89 What configuration files are expected to be checked? The usual trio of pes data, repomap and hw deprecation?

for now, any file marked by rpm as a configuration file.

In future we will extend it possible to all files under a directory, once we introduce "configurable upgrades" (configurations for actors/workflow let's say) . But that part will need to be postponed until we have the full designe finished for that.

fernflower commented 9 months ago

Not sure about generalization "let's track all configuration file changes", as for leapp this means warning users that they have changed leapp.conf and logger.conf. 

[root@vm-10-0-184-88 ~]# rpm -qc leapp
/etc/leapp/leapp.conf
/etc/leapp/logger.conf
[root@vm-10-0-184-88 ~]# vim /etc/leapp/leapp.conf 
[root@vm-10-0-184-88 ~]# rpm -qc leapp-upgrade-el8toel9
/etc/leapp/files/device_driver_deprecation_data.json
/etc/leapp/files/pes-events.json
/etc/leapp/files/repomap.json

So I'd check leapp-repository configurations only. P.S. In the end tracking all configurations was easier than tracking some specific ones, so let's have it all and see how it goes :)

pirat89 commented 9 months ago

Not sure about generalization "let's track all configuration file changes", as for leapp this means warning users that they have changed leapp.conf and logger.conf.

[root@vm-10-0-184-88 ~]# rpm -qc leapp
/etc/leapp/leapp.conf
/etc/leapp/logger.conf
[root@vm-10-0-184-88 ~]# vim /etc/leapp/leapp.conf 
[root@vm-10-0-184-88 ~]# rpm -qc leapp-upgrade-el8toel9
/etc/leapp/files/device_driver_deprecation_data.json
/etc/leapp/files/pes-events.json
/etc/leapp/files/repomap.json

So I'd check leapp-repository configurations only.

well, if user touches now leapp.conf, it can have negative impacts. imagine that they will do it and people who are investigating issues are not able to find out where is the leapp.db file located. as someone who is doing investigations, I would really appreciated to have such information visible at least in the report.

fernflower commented 9 months ago

@pirat89 I'd say worrying about changed leapp config is out of scope of this PR, please let's not forget that we are addressing the concerns of the original ticket, not trying to fit every improvement we have ever wanted to do :) I am 146% sure that if a change to leapp.db location happens our amazing support folks are more than capable of figuring out the new location.

fernflower commented 9 months ago

@pirat89 I have finished with framework changes tracking and configuration. Some things are still up to discussion (like maybe going with separate messages for each report type is worth it or maybe staying with a single type as I have now is ok at this point / polishing report message wording / etc) - so please review :) I'll leave an extract from a report.txt here for visibility

----------------------------------------
Risk Factor: high 
Title: Custom files were discovered on the system in leapp directories
Summary: Apparently some custom files have been found in leapp installation directories.
Please consult the list of discovered files for more information:
- /usr/share/leapp-repository/repositories/system_upgrade/common/actors/checkcustommodifications/libraries/.checkcustomm
odifications.py.swp (check_custom_modifications_actor Actor)
- /usr/share/leapp-repository/repositories/system_upgrade/common/actors/unexpecteddir/nonactorfile
Key: 782e5c876139f00a5434b76f04e51d3f0aea031a
----------------------------------------
Risk Factor: high 
Title: Modified files were discovered on the system in leapp repository directories
Summary: Apparently some modified files have been found in leapp repository installation directories.
Please consult the list of discovered files for more information:
- /usr/share/leapp-repository/repositories/system_upgrade/common/actors/checkcustommodifications/libraries/checkcustommodifications.py (check_custom_modifications_actor Actor)
- /usr/share/leapp-repository/repositories/system_upgrade/common/actors/scancustommodifications/libraries/scancustommodifications.py (scan_custom_modifications_actor Actor)
Key: 16c67599f6a7d53eb59c965955145c8578395598
----------------------------------------
Risk Factor: high 
Title: Modified files were discovered on the system in leapp configuration directories
Summary: Apparently some modified files have been found in leapp configuration installation directories.
Please consult the list of discovered files for more information:
- /etc/leapp/files/device_driver_deprecation_data.json
Key: 4b383112150a261cfa132c94ef7ec006e6bf3759
----------------------------------------
matejmatuska commented 9 months ago

@pirat89 I'd say worrying about changed leapp config is out of scope of this PR, please let's not forget that we are addressing the concerns of the original ticket, not trying to fit every improvement we have ever wanted to do :) I am 146% sure that if a change to leapp.db location happens our amazing support folks are more than capable of figuring out the new location.

After reading the goals in the ticket I'd say it is in the scope of this PR. Either way I am not sure whether you changed your mind about this because I can't see any changes since the comment in how this is handled, but I did a test and a modified /etc/leapp/leapp.conf (where the leapp.db location is set) was reported just fine:

Risk Factor: high
Title: Modified files were discovered on the system in leapp configuration directories
Summary: Apparently some modified files have been found in leapp configuration installation directories.
Please consult the list of discovered files for more information:
- /etc/leapp/leapp.conf
- /etc/leapp/files/pes-events.json
Key: 4b383112150a261cfa132c94ef7ec006e6bf3759

If I understand the code correctly, it looks generic so pretty much all the config files should get detected or am I wrong?

matejmatuska commented 9 months ago

Also can you please rebase?

fernflower commented 9 months ago

@matejmatuska thanks for review!

After reading the goals in the ticket I'd say it is in the scope of this PR.

The way I see it there are 2 types of configuration - critical ones (the trio of pes data/repomapping/hw modification) where a change can really mess things up and "normal" configuration files which are intended, well, for configuration and where changing values should not break anything (might raise a few eyebrows at most because of untypical setup). The way I read the ticket - we should inform about critical changes only, that's why I stay by my suggestion that tracking normal configuration changes is out of scope of the task.

Either way I am not sure whether you changed your mind about this because I can't see any changes since the comment in how this is handled, but I did a test and a modified /etc/leapp/leapp.conf (where the leapp.db location is set) was reported just fine:

Tracking all configurations (even non-critical ones that I'd personally prefer to ignore) is much simpler than tracking only specific ones so I decided not to argue on this. I still feel we should not be doing this but if that will make a review process smoother - let's go for it. So yes, you are totally right and now all configurations are tracked :)

fernflower commented 8 months ago

@mreznik squashed commits, removed POC

pirat89 commented 8 months ago

Output from manual testing:

----------------------------------------
Risk Factor: high
Title: Custom files were discovered on the system in leapp directories
Summary: Apparently some custom files have been found in leapp installation directories.
Please consult the list of discovered files for more information:
    - /usr/share/leapp-repository/custom-repositories/supplements/.leapp/info
    - /usr/share/leapp-repository/custom-repositories/supplements/.leapp/leapp.conf
    - /usr/share/leapp-repository/custom-repositories/supplements/actors/checkreboothygiene/actor.py (check_reboot_hygiene Actor)
    - /usr/share/leapp-repository/custom-repositories/supplements/actors/checkreboothygiene/libraries/checkreboothygiene.py (check_reboot_hygiene Actor)
    - /usr/share/leapp-repository/custom-repositories/supplements/actors/checkreboothygiene/tests/unit_test_checkreboothygiene.py
    - /usr/share/leapp-repository/repositories/system_upgrade/el7toel8/tools/foobar
Key: 782e5c876139f00a5434b76f04e51d3f0aea031a
----------------------------------------
Risk Factor: high
Title: Modified files were discovered on the system in leapp repository directories
Summary: Apparently some modified files have been found in leapp repository installation directories.
Please consult the list of discovered files for more information:
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/actor.py
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/actor.pyo
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/libraries
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/libraries/checkinstalledkernels.py
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/libraries/checkinstalledkernels.pyo
    - /usr/share/leapp-repository/repositories/system_upgrade/el7toel8/actors/quaggatofrr/libraries/quaggatofrr.py (quagga_to_frr Actor)
Key: 16c67599f6a7d53eb59c965955145c8578395598
----------------------------------------
Risk Factor: high
Title: Modified files were discovered on the system in leapp configuration directories
Summary: Apparently some modified files have been found in leapp configuration installation directories.
Please consult the list of discovered files for more information:
    - /etc/leapp/files/repomap.json
Key: 4b383112150a261cfa132c94ef7ec006e6bf3759
----------------------------------------

Which reflects all changes I made on the system. Tested on RHEL 7. In case of RHEL 8, I'm blocked on very rare bug that's most likely unrelated to this PR (reported in the passed already) and for which we do not have reproducer. So keeping that precious machine preserved now for another investigation later.

pirat89 commented 8 months ago

@fernflower I am not blocking the current PR, but I think that current report msgs could be improved as current msg has potential to unexpectedly create additional pressure on support teams. User reading these msgs will not have answers on questions like

Also hint with reinstalling the "leapp" package in case of the "framework" component could lead to the situation they

dnf reinstall leapp -y

which does not have to fix the problem, because they need to reinstall possibly python[23]-leapp, etc. I will create a commit/pr/.. with proposed improvement to save you a time.

pirat89 commented 8 months ago

@fernflower @Rezney : added commit with the proposal of the new error msgs. Tests have been updated and fixed. Also the logic of the check actor is a little bit different to reflect better expectations. only 3 types of reports can be created now.

Adding changes as part of the squash commit to not interfere with existing commits. But pushing here directly to run all tests to see nothing is broken. In case it's ok, squash commit to one.

pirat89 commented 8 months ago

Here is the new output from the updated version (I've realied a typo - missing space - so we should fix that one before the merge yet, but it's a nitpick.). In this case I've modified also a schema file from python2-leapp.

----------------------------------------
Risk Factor: high
Title: Detected custom leapp actors or files.
Summary: We have detected installed custom actors or files on the system. These can be provided e.g. by third party vendors, Red Hat consultants, or can be created by users to customize the upgrade (e.g. to migrate custom applications). This is allowed and appreciated. However Red Hat is not responsible for any issues caused by these custom leapp actors. Note that upgrade tooling is under agile development which could require more frequent update of custom actors.The list of custom leapp actors and files:
    - /usr/share/leapp-repository/custom-repositories/supplements/.leapp/info
    - /usr/share/leapp-repository/custom-repositories/supplements/.leapp/leapp.conf
    - /usr/share/leapp-repository/custom-repositories/supplements/actors/checkreboothygiene/actor.py (Actor: check_reboot_hygiene)
    - /usr/share/leapp-repository/custom-repositories/supplements/actors/checkreboothygiene/libraries/checkreboothygiene.py (Actor: check_reboot_hygiene)
    - /usr/share/leapp-repository/custom-repositories/supplements/actors/checkreboothygiene/tests/unit_test_checkreboothygiene.py
    - /usr/share/leapp-repository/repositories/system_upgrade/el7toel8/tools/foobar
Related links:
    - Customizing your Red Hat Enterprise Linux in-place upgrade: https://red.ht/customize-rhel-upgrade
Remediation: [hint] In case of any issues connected to custom or third party actors, contact vendor of such actors. Also we suggest to ensure the installed custom leapp actors are up to date, compatible with the installed packages.
Key: 2064870018370ce2bde3f977cf753ed8c59848d0
----------------------------------------
Risk Factor: high
Title: Detected modified configuration files in leapp configuration directories.
Summary: We have detected that some configuration files related to leapp or upgrade process have been modified. Some of these changes could be intended (e.g. modified repomap.json file in case of private cloud regions or customisations done on used Satellite server) so it is not always needed to worry about them. However they can impact the in-place upgrade and it is good to be aware of potential problems or unexpected results if they are not intended.
The list of modified configuration files:
    - /etc/leapp/files/repomap.json
Remediation: [hint] If some of changes in listed configuration files have not been intended, you can restore original files by following procedure:
1. Remove (or back up) modified files that you want to restore.
2. Reinstall packages which owns these files.
Key: 949e7060e875be8f50c3d3534d8124e473c65b2c
----------------------------------------
Risk Factor: high
Title: Detected modified files of the in-place upgrade tooling.
Summary: We have detected that some files of the tooling processing the in-place upgrade have been modified. Note that such modifications can be allowed only after consultation with Red Hat - e.g. when support suggests the change to resolve discovered problem. If these changes have not been approved by Red Hat, the in-place upgrade is unsupported.
Following files have been modified:
    - /usr/lib/python2.7/site-packages/leapp/utils/schemas.py
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/checkmemory/actor.py (Actor: checkmemory)
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/actor.py
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/actor.pyo
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/libraries
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/libraries/checkinstalledkernels.py
    - /usr/share/leapp-repository/repositories/system_upgrade/common/actors/kernel/checkinstalledkernels/libraries/checkinstalledkernels.pyo
Remediation: [hint] To restore original files reinstall related packages.
Key: 5532a4fe27dc0b05de1e9e77bda407ea47ad6971
----------------------------------------