Repository: ansible/ansible
Pull request: Add fail_on_autoremove option to apt module to avoid unintended package removals.
##### SUMMARY
Fixes #63231

As described in https://github.com/ansible/ansible/issues/63231, apt module removes the existing packages which conflict with the package to install (without no warnings or error), and breaks the system.

In this PR, `--no-remove` will be passed to apt-get command to avoid this issue.

##### ISSUE TYPE
- Feature Pull Request

##### COMPONENT NAME

apt


Problem statement:
apt uninstalls packages and breaks machine
##### SUMMARY
When doing a harmless apt install for a package, this might uninstall other packages and break stuff heavy, especially if there are lots of pending upgrades.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
apt

##### ANSIBLE VERSION
```
ansible 2.8.2
  config file = /home/alexg/mw-config/ansible.cfg
  configured module search path = [u'/home/alexg/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/alexg/.local/lib/python2.7/site-packages/ansible
  executable location = /home/alexg/.local/bin/ansible
  python version = 2.7.15+ (default, Jul  9 2019, 16:51:35) [GCC 7.4.0]
```

##### CONFIGURATION
```
DEFAULT_CALLBACK_PLUGIN_PATH(/home/alexg/mw-config/ansible.cfg) = [u'/home/alexg/mw-config/ansible/plugins/callback']
DEFAULT_CALLBACK_WHITELIST(/home/alexg/mw-config/ansible.cfg) = [u'zabbix']
DEFAULT_HOST_LIST(/home/alexg/mw-config/ansible.cfg) = [u'/home/alexg/mw-config/inventory']
DEFAULT_ROLES_PATH(/home/alexg/mw-config/ansible.cfg) = [u'/home/alexg/mw-config/roles']
DEFAULT_STDOUT_CALLBACK(/home/alexg/mw-config/ansible.cfg) = nice_failures
```

##### OS / ENVIRONMENT
target server was a debian machine, pending an upgrade stretch->buster

##### STEPS TO REPRODUCE
Just running something like
```yaml
  - name: "Install some packages"
    apt:
      name: vim
      state: present
      update_cache: yes
```
can make the underlying apt uninstall some packages, depending on the current apt status. In this case installation of 4 basic packages on the machine with 600+ packages pending upgrade caused apt to uninstalled the VM hypervisor...

##### EXPECTED RESULTS
Need more control on dependency handling:
default should be to install package + requirements, but never uninstall other packages. That should get a separate option.
Alternative would be to fail when there is a uninstall going to happen, or just fail when there are pending upgrades at all.

##### ACTUAL RESULTS
Uninstalled libvirtd, shtudown of 5 VMs...Can't reproduce right now, because had to rush to fix it.
I guess this is obvious behaviour when you run apt install -y $package.

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/packaging/os/apt.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/packaging/os/apt.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @mgwilliams
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Files identified in the description:
* [`lib/ansible/modules/apt.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/apt.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
We are also facing this issue and have been breaking the production environment many times :sweat_smile: 

How about adding a `--no-remove` option to here to avoid unintended package removals?

https://github.com/ansible/ansible/blob/4c4406b2df3eb23ae0015067fa8c026777cd961e/lib/ansible/modules/apt.py#L672-L675

I also think it's great if `ansible-playbook hoge.yaml ... --diff --check` prints the result of `apt install --dry-run`, like:

```bash
% apt install zsh --dry-run
NOTE: This is only a simulation!
      apt needs root privileges for real execution.
      Keep also in mind that locking is deactivated,
      so don't depend on the relevance to the real current situation!
Reading package lists... Done
Building dependency tree       
Reading state information... Done
The following additional packages will be installed:
  zsh-common
Suggested packages:
  zsh-doc
The following NEW packages will be installed:
  zsh zsh-common
0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded.
Inst zsh-common (5.8-3ubuntu1 Ubuntu:20.04/focal [all])
Inst zsh (5.8-3ubuntu1 Ubuntu:20.04/focal [amd64])
Conf zsh-common (5.8-3ubuntu1 Ubuntu:20.04/focal [all])
Conf zsh (5.8-3ubuntu1 Ubuntu:20.04/focal [amd64])
```
Hmm... It looks there are some tests that expect the packages will be removed.
https://app.shippable.com/github/ansible/ansible/runs/168423/86/tests
https://app.shippable.com/github/ansible/ansible/runs/168423/85/tests

How about adding `no-remove` parameter to this module? > @sivel @mgwilliams

Commit message: Persuade linters

Patch to review:
diff --git a/changelogs/fragments/70056-add-a-param-to-apt-module-to-avoid-unintended-uninstalls.yml b/changelogs/fragments/70056-add-a-param-to-apt-module-to-avoid-unintended-uninstalls.yml
new file mode 100644
index 00000000000000..4c31a33ed2e849
--- /dev/null
+++ b/changelogs/fragments/70056-add-a-param-to-apt-module-to-avoid-unintended-uninstalls.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - apt - add ``fail_on_autoremove`` param to apt module to avoid unintended package removals (https://github.com/ansible/ansible/issues/63231)
diff --git a/lib/ansible/modules/apt.py b/lib/ansible/modules/apt.py
index d90d12dca638ea..466fdf6f665ee3 100644
--- a/lib/ansible/modules/apt.py
+++ b/lib/ansible/modules/apt.py
@@ -135,6 +135,14 @@
     type: bool
     default: 'no'
     version_added: "2.1"
+  fail_on_autoremove:
+    description:
+      - 'Corresponds to the C(--no-remove) option for I(apt).'
+      - 'If C(yes), it is ensured that no packages will be removed or the task will fail.'
+      - 'C(fail_on_autoremove) is only supported with state except I(absent)'
+    type: bool
+    default: 'no'
+    version_added: "2.11"
   force_apt_get:
     description:
       - Force usage of apt-get instead of aptitude
@@ -194,6 +202,12 @@
     default_release: squeeze-backports
     update_cache: yes
 
+- name: Install foo with ensuring no packages will be removed.
+  apt:
+    name: foo
+    state: latest
+    fail_on_autoremove: yes
+
 - name: Install latest version of "openjdk-6-jdk" ignoring "install-recommends"
   apt:
     name: openjdk-6-jdk
@@ -613,7 +627,7 @@ def mark_installed_manually(m, packages):
 def install(m, pkgspec, cache, upgrade=False, default_release=None,
             install_recommends=None, force=False,
             dpkg_options=expand_dpkg_options(DPKG_OPTIONS),
-            build_dep=False, fixed=False, autoremove=False, only_upgrade=False,
+            build_dep=False, fixed=False, autoremove=False, fail_on_autoremove=False, only_upgrade=False,
             allow_unauthenticated=False):
     pkg_list = []
     packages = ""
@@ -656,6 +670,11 @@ def install(m, pkgspec, cache, upgrade=False, default_release=None,
         else:
             autoremove = ''
 
+        if fail_on_autoremove:
+            fail_on_autoremove = '--no-remove'
+        else:
+            fail_on_autoremove = ''
+
         if only_upgrade:
             only_upgrade = '--only-upgrade'
         else:
@@ -667,9 +686,10 @@ def install(m, pkgspec, cache, upgrade=False, default_release=None,
             fixed = ''
 
         if build_dep:
-            cmd = "%s -y %s %s %s %s %s build-dep %s" % (APT_GET_CMD, dpkg_options, only_upgrade, fixed, force_yes, check_arg, packages)
+            cmd = "%s -y %s %s %s %s %s %s build-dep %s" % (APT_GET_CMD, dpkg_options, only_upgrade, fixed, force_yes, fail_on_autoremove, check_arg, packages)
         else:
-            cmd = "%s -y %s %s %s %s %s %s install %s" % (APT_GET_CMD, dpkg_options, only_upgrade, fixed, force_yes, autoremove, check_arg, packages)
+            cmd = "%s -y %s %s %s %s %s %s %s install %s" % \
+                  (APT_GET_CMD, dpkg_options, only_upgrade, fixed, force_yes, autoremove, fail_on_autoremove, check_arg, packages)
 
         if default_release:
             cmd += " -t '%s'" % (default_release,)
@@ -719,7 +739,7 @@ def get_field_of_deb(m, deb_file, field="Version"):
     return to_native(stdout).strip('\n')
 
 
-def install_deb(m, debs, cache, force, install_recommends, allow_unauthenticated, dpkg_options):
+def install_deb(m, debs, cache, force, fail_on_autoremove, install_recommends, allow_unauthenticated, dpkg_options):
     changed = False
     deps_to_install = []
     pkgs_to_install = []
@@ -761,6 +781,7 @@ def install_deb(m, debs, cache, force, install_recommends, allow_unauthenticated
     if deps_to_install:
         (success, retvals) = install(m=m, pkgspec=deps_to_install, cache=cache,
                                      install_recommends=install_recommends,
+                                     fail_on_autoremove=fail_on_autoremove,
                                      allow_unauthenticated=allow_unauthenticated,
                                      dpkg_options=expand_dpkg_options(dpkg_options))
         if not success:
@@ -890,7 +911,7 @@ def cleanup(m, purge=False, force=False, operation=None,
 
 def upgrade(m, mode="yes", force=False, default_release=None,
             use_apt_get=False,
-            dpkg_options=expand_dpkg_options(DPKG_OPTIONS), autoremove=False,
+            dpkg_options=expand_dpkg_options(DPKG_OPTIONS), autoremove=False, fail_on_autoremove=False,
             allow_unauthenticated=False,
             ):
 
@@ -932,6 +953,11 @@ def upgrade(m, mode="yes", force=False, default_release=None,
     else:
         force_yes = ''
 
+    if fail_on_autoremove:
+        fail_on_autoremove = '--no-remove'
+    else:
+        fail_on_autoremove = ''
+
     allow_unauthenticated = '--allow-unauthenticated' if allow_unauthenticated else ''
 
     if apt_cmd is None:
@@ -942,8 +968,7 @@ def upgrade(m, mode="yes", force=False, default_release=None,
                             "to have APTITUDE in path or use 'force_apt_get=True'")
     apt_cmd_path = m.get_bin_path(apt_cmd, required=True)
 
-    cmd = '%s -y %s %s %s %s %s' % (apt_cmd_path, dpkg_options, force_yes, allow_unauthenticated,
-                                    check_arg, upgrade_command)
+    cmd = '%s -y %s %s %s %s %s %s' % (apt_cmd_path, dpkg_options, force_yes, fail_on_autoremove, allow_unauthenticated, check_arg, upgrade_command)
 
     if default_release:
         cmd += " -t '%s'" % (default_release,)
@@ -1029,6 +1054,7 @@ def main():
             dpkg_options=dict(type='str', default=DPKG_OPTIONS),
             autoremove=dict(type='bool', default=False),
             autoclean=dict(type='bool', default=False),
+            fail_on_autoremove=dict(type='bool', default=False),
             policy_rc_d=dict(type='int', default=None),
             only_upgrade=dict(type='bool', default=False),
             force_apt_get=dict(type='bool', default=False),
@@ -1084,6 +1110,7 @@ def main():
     allow_unauthenticated = p['allow_unauthenticated']
     dpkg_options = expand_dpkg_options(p['dpkg_options'])
     autoremove = p['autoremove']
+    fail_on_autoremove = p['fail_on_autoremove']
     autoclean = p['autoclean']
 
     # Get the cache object
@@ -1145,7 +1172,7 @@ def main():
         force_yes = p['force']
 
         if p['upgrade']:
-            upgrade(module, p['upgrade'], force_yes, p['default_release'], use_apt_get, dpkg_options, autoremove, allow_unauthenticated)
+            upgrade(module, p['upgrade'], force_yes, p['default_release'], use_apt_get, dpkg_options, autoremove, fail_on_autoremove, allow_unauthenticated)
 
         if p['deb']:
             if p['state'] != 'present':
@@ -1155,7 +1182,7 @@ def main():
             install_deb(module, p['deb'], cache,
                         install_recommends=install_recommends,
                         allow_unauthenticated=allow_unauthenticated,
-                        force=force_yes, dpkg_options=p['dpkg_options'])
+                        force=force_yes, fail_on_autoremove=fail_on_autoremove, dpkg_options=p['dpkg_options'])
 
         unfiltered_packages = p['package'] or ()
         packages = [package.strip() for package in unfiltered_packages if package != '*']
@@ -1165,7 +1192,7 @@ def main():
         if latest and all_installed:
             if packages:
                 module.fail_json(msg='unable to install additional packages when upgrading all installed packages')
-            upgrade(module, 'yes', force_yes, p['default_release'], use_apt_get, dpkg_options, autoremove, allow_unauthenticated)
+            upgrade(module, 'yes', force_yes, p['default_release'], use_apt_get, dpkg_options, autoremove, fail_on_autoremove, allow_unauthenticated)
 
         if packages:
             for package in packages:
@@ -1203,6 +1230,7 @@ def main():
                 build_dep=state_builddep,
                 fixed=state_fixed,
                 autoremove=autoremove,
+                fail_on_autoremove=fail_on_autoremove,
                 only_upgrade=p['only_upgrade'],
                 allow_unauthenticated=allow_unauthenticated
             )
diff --git a/test/integration/targets/apt/tasks/apt.yml b/test/integration/targets/apt/tasks/apt.yml
index 871cb0d7ba996e..7dcc423196bca1 100644
--- a/test/integration/targets/apt/tasks/apt.yml
+++ b/test/integration/targets/apt/tasks/apt.yml
@@ -179,6 +179,45 @@
 - name: uninstall hello with apt
   apt: pkg=hello state=absent purge=yes
 
+# INSTALL WITHOUT REMOVALS
+- name: Install hello, that conflicts with hello-traditional
+  apt: 
+    pkg: hello
+    state: present
+    update_cache: no
+
+- name: check hello
+  shell: dpkg-query -l hello
+  failed_when: False
+  register: dpkg_result
+
+- name: verify installation of hello
+  assert:
+    that:
+        - "apt_result.changed"
+        - "dpkg_result.rc == 0"
+
+- name: Try installing hello-traditional, that conflicts with hello
+  apt: 
+    pkg: hello-traditional
+    state: present
+    fail_on_autoremove: yes
+  ignore_errors: yes
+  register: apt_result
+
+- name: verify failure of installing hello-traditional, because it is required to remove hello to install.
+  assert:
+    that:
+      - apt_result is failed
+      - '"Packages need to be removed but remove is disabled." in apt_result.msg'
+
+- name: uninstall hello with apt
+  apt: 
+    pkg: hello
+    state: absent
+    purge: yes
+    update_cache: no
+
 - name: install deb file
   apt: deb="/var/cache/apt/archives/hello_{{ hello_version.stdout }}_{{ hello_architecture.stdout }}.deb"
   register: apt_initial


Code review comments:
diff_hunk: @@ -179,6 +179,35 @@
 - name: uninstall hello with apt
   apt: pkg=hello state=absent purge=yes
 
+# INSTALL WITHOUT REMOVALS
+- name: Install hello, that conflicts with hello-traditional
+  apt: pkg=hello state=present update_cache=no
+
+- name: check hello
+  shell: dpkg-query -l hello
+  failed_when: False
path: test/integration/targets/apt/tasks/apt.yml
text: @user1:
Yes. Tests should not ignore failure unless absolutely necessary.

diff_hunk: @@ -194,6 +202,12 @@
     default_release: squeeze-backports
     update_cache: yes
 
+- name: Install foo with ensuring no packages will be removed.
+  apt:
+    name: foo
path: lib/ansible/modules/apt.py
text: @user1:
Could we use a real package name here?

@author:
OK, I will use `zfsutils-linux` package which conflicts with `zfs-fuse`.

diff_hunk: @@ -135,6 +135,14 @@
     type: bool
     default: 'no'
     version_added: "2.1"
+  fail_on_autoremove:
+    description:
+      - 'Corresponds to the C(--no-remove) option for I(apt).'
+      - 'If C(yes), it is ensured that no packages will be removed or the task will fail.'
+      - 'C(fail_on_autoremove) is only supported with state except I(absent)'
path: lib/ansible/modules/apt.py
text: @user1:
`I()` formats as italic. These should receive inline code formatting.
```suggestion
      - 'Corresponds to the C(--no-remove) option for C(apt).'
      - 'If C(yes), it is ensured that no packages will be removed or the task will fail.'
      - 'C(fail_on_autoremove) is only supported with state except C(absent)'
```