Repository: ansible/ansible
Pull request: ufw: fix default, direction is not necessary for it
##### SUMMARY
While implementing #49948 and #50402, we forgot to consider that `direction` is not required for `default`. This re-allows not specifying `direction` when `default` is used.

Fixes #53854.

CC @jbarotin

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
ufw


Problem statement:
Ufw module example does not work because of missing parameter
##### SUMMARY
Documentation for Ufw module gives this example:

```
- name: Allow everything and enable UFW
  ufw:
    state: enabled
    policy: allow
```

Trying to run that task fails with error:
```
TASK [Ufw example] *******************************************************************
fatal: [10.100.105.24]: FAILED! => {
    "changed": false
}

MSG:
For default, direction must be one of "outgoing", "incoming" and "routed".
```

I belive that this is issue with documentation because the module works as expected if I add parameter for direction:
```
- name:  Allow everything and enable UFW
  ufw:
    state: enabled
    policy: allow
    direction: outgoing
```

##### ISSUE TYPE
- Documentation Report

##### COMPONENT NAME
Ufw module documentation

##### ANSIBLE VERSION

```
ansible 2.7.8
  config file = /home/user/example-ansible/ansible.cfg
  configured module search path = [u'/home/user/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/dist-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.12 (default, Nov 12 2018, 14:36:49) [GCC 5.4.0 20160609]
```

##### CONFIGURATION

```
ANSIBLE_PIPELINING(/home/user/example-ansible/ansible.cfg) = True
DEFAULT_STDOUT_CALLBACK(/home/user/example-ansible/ansible.cfg) = debug
RETRY_FILES_ENABLED(/home/user/example-ansible/ansible.cfg) = False
```

##### OS / ENVIRONMENT
```
Distributor ID: Ubuntu
Description:    Ubuntu 16.04.6 LTS
Release:        16.04
Codename:       xenial
```

##### ADDITIONAL INFORMATION
<!--- Describe how this improves the documentation, e.g. before/after situation or screenshots -->

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/system/ufw.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/system/ufw.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 @ahtik @ovcharenko @pyykkis
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Bisect points to commit 6438ab2c463b12b78e302f78b3058cf1d0bb7051, PR #50402
Works in devel, though.

Documentation does not state that the param is needed: https://docs.ansible.com/ansible/latest/modules/ufw_module.html
Same issue on Ansible 2.7.9

Omitting the non-required 'direction' param results in failure.

 

I can confirm this issue also with ansible 2.7.9

```bash
fatal: [X.X.X.X]: FAILED! => {"changed": false, "msg": "For default, direction must be one of \"outgoing\", \"incoming\" and \"routed\"."}
```
CC @felixfontein
This is not a problem with the documentation, but with the code. Not specifying `direction` should also be allowed here. I'll create a PR to fix this.
resolved_by_pr #54799
@muffl0n thanks for identifying the PR; please also mention that the PR breaks something in the PR itself, so that the people involved will notice it a bit earlier :)
Will do! Thanks for the fix!

Commit message: 'disabled' values cannot be changed.

Patch to review:
diff --git a/changelogs/fragments/54799-ufw-default-direction.yml b/changelogs/fragments/54799-ufw-default-direction.yml
new file mode 100644
index 00000000000000..5e46fc60d9cf35
--- /dev/null
+++ b/changelogs/fragments/54799-ufw-default-direction.yml
@@ -0,0 +1,2 @@
+bugfixes:
+- "ufw - when ``default`` is specified, ``direction`` does not needs to be specified. This was accidentally introduced in Ansible 2.7.8."
diff --git a/lib/ansible/modules/system/ufw.py b/lib/ansible/modules/system/ufw.py
index c786a4caf121d5..908a1b12928d34 100644
--- a/lib/ansible/modules/system/ufw.py
+++ b/lib/ansible/modules/system/ufw.py
@@ -461,7 +461,7 @@ def ufw_version():
                 execute(cmd + [[command], [value]])
 
         elif command == 'default':
-            if params['direction'] not in ['outgoing', 'incoming', 'routed']:
+            if params['direction'] not in ['outgoing', 'incoming', 'routed', None]:
                 module.fail_json(msg='For default, direction must be one of "outgoing", "incoming" and "routed".')
             if module.check_mode:
                 regexp = r'Default: (deny|allow|reject) \(incoming\), (deny|allow|reject) \(outgoing\), (deny|allow|reject|disabled) \(routed\)'
@@ -471,8 +471,13 @@ def ufw_version():
                     current_default_values["incoming"] = extract.group(1)
                     current_default_values["outgoing"] = extract.group(2)
                     current_default_values["routed"] = extract.group(3)
-                    if current_default_values[params['direction']] != value:
-                        changed = True
+                    if params['direction'] is None:
+                        if any(v not in (value, 'disabled') for v in current_default_values.values()):
+                            changed = True
+                    else:
+                        v = current_default_values[params['direction']]
+                        if v not in (value, 'disabled'):
+                            changed = True
                 else:
                     changed = True
             else:
diff --git a/test/integration/targets/ufw/tasks/tests/global-state.yml b/test/integration/targets/ufw/tasks/tests/global-state.yml
index 28c9156295b0e2..021e6fc66ea1d5 100644
--- a/test/integration/targets/ufw/tasks/tests/global-state.yml
+++ b/test/integration/targets/ufw/tasks/tests/global-state.yml
@@ -103,6 +103,35 @@
   register: ufw_defaults_change
   environment:
     LC_ALL: C
+- name: Default (change again)
+  ufw:
+    default: deny
+    direction: incoming
+  register: default_change_2
+- name: Default (change all, check mode)
+  ufw:
+    default: allow
+  check_mode: yes
+  register: default_change_all_check
+- name: Default (change all)
+  ufw:
+    default: allow
+  register: default_change_all
+- name: Get defaults
+  shell: |
+    ufw status verbose | grep "^Default:"
+  register: ufw_defaults_change_all
+  environment:
+    LC_ALL: C
+- name: Default (change all, idempotent, check mode)
+  ufw:
+    default: allow
+  check_mode: yes
+  register: default_change_all_idem_check
+- name: Default (change all, idempotent)
+  ufw:
+    default: allow
+  register: default_change_all_idem
 - assert:
     that:
     - default_check is changed
@@ -113,3 +142,10 @@
     - default_change_check is changed
     - default_change is changed
     - "'allow (incoming)' in ufw_defaults_change.stdout"
+    - default_change_2 is changed
+    - default_change_all_check is changed
+    - default_change_all is changed
+    - default_change_all_idem_check is not changed
+    - default_change_all_idem is not changed
+    - "'allow (incoming)' in ufw_defaults_change_all.stdout"
+    - "'allow (outgoing)' in ufw_defaults_change_all.stdout"


Code review comments:
diff_hunk: @@ -461,7 +461,7 @@ def ufw_version():
                 execute(cmd + [[command], [value]])
 
         elif command == 'default':
-            if params['direction'] not in ['outgoing', 'incoming', 'routed']:
+            if params['direction'] not in ['outgoing', 'incoming', 'routed', None]:
                 module.fail_json(msg='For default, direction must be one of "outgoing", "incoming" and "routed".')
path: lib/ansible/modules/system/ufw.py
text: @user1:
Maybee you should change the message too. 

'For default, direction must be empty or one of "outgoing", "incoming" and "routed"" '

@author:
I've changed both messages.

diff_hunk: @@ -471,8 +471,13 @@ def ufw_version():
                     current_default_values["incoming"] = extract.group(1)
                     current_default_values["outgoing"] = extract.group(2)
                     current_default_values["routed"] = extract.group(3)
-                    if current_default_values[params['direction']] != value:
-                        changed = True
+                    if params['direction'] is None:
+                        if any(v not in (value, 'disabled') for v in current_default_values.values()):
path: lib/ansible/modules/system/ufw.py
text: @user1:
I'm not a big fan with this... there's too much instructions on the same line. It's not easy to understand for other contributers.

@author:
I've expanded it into a `for` loop.