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

Added dependency restrictions to apt module #21514

Closed
wants to merge 1 commit into from

Conversation

ppanczyk
Copy link
Contributor

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

packaging/os/apt module

ANSIBLE VERSION
ansible 2.3.0 (devel 34da024e06) last updated 2017/02/16 08:26:54 (GMT +200)
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
SUMMARY

Sometimes, especially on systems that do not have test environments or haven't been upgraded recently, it's quite dangerous to run "apt-get" with "--assume-yes" option. In some cases, it may, for example, remove some packages the user wouldn't like to remove. Then it's better to stop Ansible run and inspect the situation manually rather than break the system.

So far I've used additional tasks to simulate apt-get run (I was grepping for "0 upgraded, 0 to remove" string), but this had 3 major disadvantages:

  • playbooks were much less clear and more complex (each use of apt module had to be preceded by an additional task),
  • plays were running longer,
  • there wasn't any control on WHAT would be removed/upgraded, so each time when ANYTHING was going to be removed/upgraded, I had to do manual inspection.

To solve this, I added 8 options to the apt module. They can be used to control dependency processing and fail the run if the system is going to do something unexpected. Apart from controlling removes and upgrades, I implemented also installs and downgrades, as this was really similar, even though less useful.
The options are combinations of states "allow" and "prohibit" with actions "install", "upgrade", "remove" and "downgrade":

  • allow_install
  • prohibit_install
  • allow_upgrade
  • prohibit_upgrade
  • allow_remove
  • prohibit_remove
  • allow_downgrade
  • prohibit_downgrade

Each option accepts a package or list of packages (their names). Wildcards are also supported. For example, if anything is specified in "allow_upgrade" option, the need of upgrade of anything else causes failure. If anything is specified in "prohibit_upgrade" option, the need of upgrade of any of the specified packages causes failure. The options are mutually exclusive. Not specifying anything works just as before - accepts any solution. Prohibition of any upgrades can be achieved by passing empty list ("[]") to the "allow_upgrade" option.

The example of breaking execution - test playbook:

---
- hosts: ansible-wheezy
  tasks:
  - apt:
      name: syslog-ng
      state: present
      allow_remove: []

Result (I use the "debug" callback):

TASK [apt] *********************************************************************
fatal: [ansible-wheezy]: FAILED! => {
    "changed": false, 
    "failed": true
}

MSG:

The following packages were prohibited to be removed: rsyslog.
The action was not allowed by module's parameters.

The module was tested on Debian (6 - 9) and Ubuntu (14.04 and 16.04).
I have read the module checklist and I believe my modifications meet these requirements.

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 core_review In order to be merged, this PR must follow the core review workflow. feature_pull_request module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 16, 2017
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Feb 17, 2017
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. affects_2.4 This issue/PR affects Ansible v2.4 and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 23, 2017
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 3, 2017
@ansibot
Copy link
Contributor

ansibot commented Apr 3, 2017

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/packaging/os/apt.py:256:161: E501 line too long (170 > 160 characters)
lib/ansible/modules/packaging/os/apt.py:555:161: E501 line too long (168 > 160 characters)
lib/ansible/modules/packaging/os/apt.py:687:161: E501 line too long (179 > 160 characters)
lib/ansible/modules/packaging/os/apt.py:836:161: E501 line too long (179 > 160 characters)
lib/ansible/modules/packaging/os/apt.py:901:161: E501 line too long (175 > 160 characters)
lib/ansible/modules/packaging/os/apt.py:1026:161: E501 line too long (219 > 160 characters)

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (allow_downgrade) should be 2.4. Currently 2.3
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (allow_install) should be 2.4. Currently 2.3
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (allow_remove) should be 2.4. Currently 2.3
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (allow_upgrade) should be 2.4. Currently 2.3
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (prohibit_downgrade) should be 2.4. Currently 2.3
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (prohibit_install) should be 2.4. Currently 2.3
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (prohibit_remove) should be 2.4. Currently 2.3
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (prohibit_upgrade) should be 2.4. Currently 2.3

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Apr 3, 2017
@ansibot
Copy link
Contributor

ansibot commented Apr 4, 2017

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/packaging/os/apt.py:256:142: W291 trailing whitespace
lib/ansible/modules/packaging/os/apt.py:689:141: W291 trailing whitespace

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/packaging/os/apt.py:0:0: E311 EXAMPLES is not valid YAML. Line 258 column 3

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 4, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 12, 2017
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 30, 2017
@ppanczyk
Copy link
Contributor Author

Any update on this? I have just rebased the PR and resolved conflicts (for the second time, since the first commit). Are there any chances to merge it?

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 24, 2017
@ppanczyk
Copy link
Contributor Author

ppanczyk commented Feb 8, 2018

@maxamillion I can do this, but I need some time.

@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 2, 2018
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 26, 2018
@ansibot
Copy link
Contributor

ansibot commented Apr 10, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 8 errors:

lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (allow_downgrade) should be 2.6. Currently 2.5
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (allow_install) should be 2.6. Currently 2.5
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (allow_remove) should be 2.6. Currently 2.5
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (allow_upgrade) should be 2.6. Currently 2.5
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (prohibit_downgrade) should be 2.6. Currently 2.5
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (prohibit_install) should be 2.6. Currently 2.5
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (prohibit_remove) should be 2.6. Currently 2.5
lib/ansible/modules/packaging/os/apt.py:0:0: E309 version_added for new option (prohibit_upgrade) should be 2.6. Currently 2.5

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 10, 2018
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed ci_verified Changes made in this PR are causing tests to fail. labels Apr 10, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 18, 2018
@valentin-krasontovitsch
Copy link
Contributor

valentin-krasontovitsch commented Jan 30, 2019

Hey @ppanczyk - was just looking through the changes and your code, and I'm not sure I understand quite where you're actually using for instance the allow_downgrade option, in the install function, as part of cmd in this line - it seems to me like it doesn't affect cmd at all, but I'm probably just missing something...

If I understand correctly, you simulate and check whether a downgrade is necessary and allowed, and if you have to downgrade a package but it't not supplied through the option, you fail, correct? But what happens if the simulation passes - how is apt told that it should actually allow to downgrade the specified packages?

EDIT: Oh, it's passing the force: yes that makes it possible, correct?

@ppanczyk
Copy link
Contributor Author

ppanczyk commented Jan 31, 2019

Hello @valentin-krasontovitsch,
please notice that allow_* and prohibit_* options are lists of packages, not booleans. In general - in my code, the installation process consists of two steps: simulation and installation itself. Simulation is performed with identical apt options as the installation, just the --simulate flag is added (look here). Apt is not told what it is allowed to do, it is run only to parse the output and - on it's basis - make a list of packages that apt WOULD install, upgrade, remove and downgrade and compare these lists with those given in allow_* and remove_* options. If the simulation output doesn't contain any forbidden packages, installation is performed. However, if Ansible is run in check mode, only the simulation process is necessary (see the comment; it was done like this also before my modifications). The line you mentioned runs the installation after the simulation passes.

Downgrade option is quite specific - force: yes is not added by default, but it is necessary for the downgrade - I've mentioned this here in the docs. There's no difference if failure to downgrade (if force: no) occurs in the simulation or installation - the result is the same. I've done it this way because force: yes was needed for any downgrades even before my mods, but it may be worth considering to automatically imply this if any of allow_downgrade or prohibit_downgrade options is given.

What's done in #33677 is completely different thing, but it should work perfectly with my changes - instead of force: yes, one could use allow_downgrades: yes. However, --allow-downgrades is a quite new option (in Debian available since Debian 9, the current stable) and it can cause problems on older apt versions. I think that my PR is a quite good workaround for this.

BTW. I still have no enough time to write tests, but if anyone would like to help, I can find some to at least rebase this.

@valentin-krasontovitsch
Copy link
Contributor

Hey @ppanczyk, thanks a lot for the detailed answer! I think I understand now.

I'm curious - you say in one of the comments above that installs and downgrades are not very useful. Would you care to elaborate why you don't think downgrades are useful?

My personal impression is that as soon as you have dependencies that need (?) to be downgraded as well, it's not gonna work. Would you agree with that?

@ppanczyk
Copy link
Contributor Author

ppanczyk commented Feb 1, 2019

@valentin-krasontovitsch, by "less useful" I don't yet mean "not very useful", just "used less frequently" ;) The apt module's most common use case is to install or upgrade something, removes and downgrades are usually rarer. Personally, I use allow_remove: [] for almost every installation done by the apt module and allow_upgrade: [<some packages>] for most of them.

Installation rarely needs to downgrade anything (and would not do that anyway if not force: yes) and installing dependencies is pretty normal and mandatory, so prohibiting installation of additional packages will just not work. In fact, I haven't yet come to any use case of *_install, but I believe it's not useless. The only situation I can imagine is removing package A, which removes package B because of dependencies. Package B is one of alternatives B, C, D (which can be installed simultaneously) provided by metapackage E. Say that something depends on E. On some systems you have installed D, so it wouldn't install the default alternative C. On other systems, without D, you may want to examine the situation manually, prohibiting the installation of C. I'm not sure, but I believe that this is a probable situation, however very, very specific and that's why I think *_install options are the least useful.

As it comes to downgrades - I don't really agree. I can think of downgrading curl, which could require to downgrade also libcurl and that's OK. But if you try to downgrade too much and it would try to install older libc6 and half of the system, that can be prevented by allow_downgrade: [libcurl3]. This is also a quite specific example and that's why I said that *_downgrade options are less useful.

@ansibot ansibot added the packaging Packaging category label Feb 17, 2019
@ansibot ansibot removed the module This issue/PR relates to a module. label May 17, 2020
@ansibot ansibot added the module This issue/PR relates to a module. label May 25, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 6, 2020
@jborean93
Copy link
Contributor

Closing due to the age of the PR and the conflicts that have occurred. It seems like some of the functionality has been implemented by #74852. If there is still a feature needed for apt please open an issue/PR for it. Any new features should have integration tests alongside them to have a better chance of being merged.

@jborean93 jborean93 closed this Jan 26, 2022
@ansible ansible locked and limited conversation to collaborators Feb 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.3 This issue/PR affects Ansible v2.3 affects_2.4 This issue/PR affects Ansible v2.4 feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. packaging Packaging category pre_azp This PR was last tested before migration to Azure Pipelines. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants