Repository: ansible/ansible
Pull request: task_executor - use correct value for ssh connection retries
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Since the task and connection both have the same 'retries' keyword, the task default would override the connection value. Making sure not to pass the task value for 'retries' to the connection options resolves the issue.

Set the default value for `ssh` connection `retries` to `0`.

Rename the config key from `retries` to `reconnection_retries` as a belt and suspenders approach.

Fixes #75142.
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request


##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
`lib/ansible/executor/task_executor.py`



Problem statement:
SSH module takes retry option from task keywords, defaulting to 3
### Summary

Starting with ansible-core-2.11.0, the `ssh` module operates with a `retry` setting of `3`.

This has undesired impact when using a `sudo` password and making a typo in the password: the module would make 4 connection attempts, attempting a sudo in each connection attempt.

On hosts that report failed sudo attempts via email - this generates 4 emails per Ansible run per host - 4x increase from behaviour with previous version.

I have tracked this to commit 935528e22e5, which changed how the ssh module gets its settings - using `get_option` instead of the ConfigurationManager.

I have worked around this by explicitly setting `retries` on the first task in the playbook - but this is just a workaround.

I hope there would still be a way to set retries for the SSH module (or make it not retry on failed privilege escalation).

Cheers,
Vlad

### Issue Type

Bug Report

### Component Name

ssh

### Ansible Version

```console
ansible [core 2.11.2]
  config file = /Users/vlad/reannz/code/ansible-playbooks/ansible.cfg
  configured module search path = ['/Users/vlad/reannz/code/ansible-playbooks/library']
  ansible python module location = /usr/local/Cellar/ansible/4.1.0/libexec/lib/python3.9/site-packages/ansible
  ansible collection location = /Users/vlad/.ansible/collections:/opt/ansible/collections
  executable location = /usr/local/bin/ansible
  python version = 3.9.5 (default, May  4 2021, 03:29:30) [Clang 11.0.0 (clang-1100.0.33.17)]
  jinja version = 3.0.1
  libyaml = True
```


### Configuration

```console
DEFAULT_BECOME(/Users/vlad/reannz/code/ansible-playbooks/ansible.cfg) = True
DEFAULT_BECOME_ASK_PASS(/Users/vlad/reannz/code/ansible-playbooks/ansible.cfg) = False
DEFAULT_BECOME_METHOD(/Users/vlad/reannz/code/ansible-playbooks/ansible.cfg) = sudo
DEFAULT_BECOME_USER(/Users/vlad/reannz/code/ansible-playbooks/ansible.cfg) = root
DEFAULT_HOST_LIST(/Users/vlad/reannz/code/ansible-playbooks/ansible.cfg) = ['/Users/vlad/reannz/code/ansible-playbooks/environment/production']
```


### OS / Environment

Controller: OSX Mojave 10.14.6
Target: Ubuntu 20.04

### Steps to Reproduce

<!--- Paste example playbooks or commands between quotes below -->
```ini
# ansible.cfg
[privilege_escalation]
become=True
become_method=sudo
become_user=root
become_ask_pass=False
```
Run:
```
ansible -u $USER -m command -a 'id' 'target-host' -K
```


### Expected Results

I expect sudo attempted only once.

### Actual Results

```console
Sudo is attempted 4 times (as per `/var/log/auth.log` on target host)
```


### Code of Conduct

- [X] I agree to follow the Ansible Code of Conduct

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/plugins/connection/ssh.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/connection/ssh.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 --->
It does look like the default changed from 0 to 3 with the move to config. We should probably change the default back to 0. In the mean time, you can change the retries back to 0 to avoid excessive notifications.

```ini
[ssh_connection]
retries = 0
```
Hi @samdoran ,

Thanks for the quick reply!

I believe what happened is that two options previously independent of each other (`[ssh_connection]` `retries` and task keyword `retries`) got merged into one.

And the `retries` task keyword had the default of `3` already before - just now, that started applying also to the ssh_connection retries.

Cheers,
Vlad

PS: I tried setting the retries in `ansible.cfg` with exactly what you suggest already during my initial troubleshooting, but this is ignored.

My understanding of the code is that as the module calls `get_option`, it always find the value (`3`) in the `_options` dict, because it is already evaluated for the current task (from the task defaults) and set via `set_options`.

So `get_option` never gets to check the value from the module configuration.
PPS: But setting `retries` on the first task in the playbook does work around it....
> I believe what happened is that two options previously independent of each other ([ssh_connection] retries and task keyword retries) got merged into one.

Ok, so it's definitely a bit more complicated than I thought at first. It's a collision of task keywords and config options, as you stated.
@vladimir-mencl-eresearch Can you test with #75155 to see if that fixes the problem for you?

Commit message: Change option name

Belt and suspenders

Patch to review:
diff --git a/changelogs/fragments/75142-ssh-retries-collision.yml b/changelogs/fragments/75142-ssh-retries-collision.yml
new file mode 100644
index 00000000000000..5725702abc48cf
--- /dev/null
+++ b/changelogs/fragments/75142-ssh-retries-collision.yml
@@ -0,0 +1,6 @@
+bugfixes:
+  - >-
+    task_exector/ssh_connection - use the ``retries`` value from ``ssh_connection`` settings,
+    not the default from the ``Task`` field attributes
+
+  - ssh_connection - set the default for ``retries`` back to ``0``.
diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py
index fe54b559a1c91e..e9f9007f1b88ea 100644
--- a/lib/ansible/executor/task_executor.py
+++ b/lib/ansible/executor/task_executor.py
@@ -1020,6 +1020,9 @@ def _set_connection_options(self, variables, templar):
             # config system instead of directly accessing play_context.
             task_keys['password'] = self._play_context.password
 
+        # Prevent task retries from overriding connection retries
+        del(task_keys['retries'])
+
         # set options with 'templated vars' specific to this plugin and dependent ones
         self._connection.set_options(task_keys=task_keys, var_options=options)
         varnames.extend(self._set_plugin_options('shell', variables, templar, task_keys))
diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py
index e869753259a5c7..da7f38704a8e6a 100644
--- a/lib/ansible/plugins/connection/ssh.py
+++ b/lib/ansible/plugins/connection/ssh.py
@@ -167,9 +167,9 @@
               version_added: '2.7'
           cli:
             - name: ssh_extra_args
-      retries:
+      reconnection_retries:
           description: Number of attempts to connect.
-          default: 3
+          default: 0
           type: integer
           env:
             - name: ANSIBLE_SSH_RETRIES
@@ -453,7 +453,7 @@ def _ssh_retry(func):
     """
     @wraps(func)
     def wrapped(self, *args, **kwargs):
-        remaining_tries = int(self.get_option('retries')) + 1
+        remaining_tries = int(self.get_option('reconnection_retries')) + 1
         cmd_summary = u"%s..." % to_text(args[0])
         conn_password = self.get_option('password') or self._play_context.password
         for attempt in range(remaining_tries):
diff --git a/test/units/plugins/connection/test_ssh.py b/test/units/plugins/connection/test_ssh.py
index 7c7afa049b6bdf..d693313ff01be4 100644
--- a/test/units/plugins/connection/test_ssh.py
+++ b/test/units/plugins/connection/test_ssh.py
@@ -234,7 +234,7 @@ def test_plugins_connection_ssh_put_file(self, mock_ospe, mock_sleep):
         conn._bare_run.return_value = (0, '', '')
         conn.host = "some_host"
 
-        conn.set_option('retries', 9)
+        conn.set_option('reconnection_retries', 9)
         conn.set_option('ssh_transfer_method', None)  # unless set to None scp_if_ssh is ignored
 
         # Test with SCP_IF_SSH set to smart
@@ -292,7 +292,7 @@ def test_plugins_connection_ssh_fetch_file(self, mock_sleep):
         conn._bare_run.return_value = (0, '', '')
         conn.host = "some_host"
 
-        conn.set_option('retries', 9)
+        conn.set_option('reconnection_retries', 9)
         conn.set_option('ssh_transfer_method', None)  # unless set to None scp_if_ssh is ignored
 
         # Test with SCP_IF_SSH set to smart
@@ -535,7 +535,7 @@ def test_pasword_without_data(self):
 class TestSSHConnectionRetries(object):
     def test_incorrect_password(self, monkeypatch):
         self.conn.set_option('host_key_checking', False)
-        self.conn.set_option('retries', 5)
+        self.conn.set_option('reconnection_retries', 5)
         monkeypatch.setattr('time.sleep', lambda x: None)
 
         self.mock_popen_res.stdout.read.side_effect = [b'']
@@ -560,7 +560,7 @@ def test_incorrect_password(self, monkeypatch):
 
     def test_retry_then_success(self, monkeypatch):
         self.conn.set_option('host_key_checking', False)
-        self.conn.set_option('retries', 3)
+        self.conn.set_option('reconnection_retries', 3)
 
         monkeypatch.setattr('time.sleep', lambda x: None)
 
@@ -589,7 +589,7 @@ def test_retry_then_success(self, monkeypatch):
 
     def test_multiple_failures(self, monkeypatch):
         self.conn.set_option('host_key_checking', False)
-        self.conn.set_option('retries', 9)
+        self.conn.set_option('reconnection_retries', 9)
 
         monkeypatch.setattr('time.sleep', lambda x: None)
 
@@ -612,7 +612,7 @@ def test_multiple_failures(self, monkeypatch):
 
     def test_abitrary_exceptions(self, monkeypatch):
         self.conn.set_option('host_key_checking', False)
-        self.conn.set_option('retries', 9)
+        self.conn.set_option('reconnection_retries', 9)
 
         monkeypatch.setattr('time.sleep', lambda x: None)
 
@@ -625,7 +625,7 @@ def test_abitrary_exceptions(self, monkeypatch):
 
     def test_put_file_retries(self, monkeypatch):
         self.conn.set_option('host_key_checking', False)
-        self.conn.set_option('retries', 3)
+        self.conn.set_option('reconnection_retries', 3)
 
         monkeypatch.setattr('time.sleep', lambda x: None)
         monkeypatch.setattr('ansible.plugins.connection.ssh.os.path.exists', lambda x: True)
@@ -656,7 +656,7 @@ def test_put_file_retries(self, monkeypatch):
 
     def test_fetch_file_retries(self, monkeypatch):
         self.conn.set_option('host_key_checking', False)
-        self.conn.set_option('retries', 3)
+        self.conn.set_option('reconnection_retries', 3)
 
         monkeypatch.setattr('time.sleep', lambda x: None)
         monkeypatch.setattr('ansible.plugins.connection.ssh.os.path.exists', lambda x: True)


Code review comments:
diff_hunk: @@ -0,0 +1,7 @@
+bugfixes:
+  - >-
+    task_exector/ssh_connection - use the ``retries`` value from ``ssh_connection`` settings,
path: changelogs/fragments/75142-ssh-retries-collision.yml
text: @user1:
Is this a typo in the change-log entry? `task_exector` => `task_executor` ?