Repository: ansible/ansible
Pull request: ufw: add support for interface_in and interface_out
##### SUMMARY

The UFW module has support for specifying `direction` and `interface`
for UFW rules.  Rules with these parameters are built such that
per-interface filtering only apply to a single direction based on the
value of `direction`.

Not being able to specify multiple interfaces complicates things for
`routed` rules where one might want to apply filtering only for a
specific combination of `in` and `out` interfaces.

This commit introduces two new parameters to the UFW module:
`interface_in` and `interface_out`.  These rules are mutually exclusive
with the old `direction` and `interface` parameter because of the
ambiguity of having e.g.:

    direction: XXX
    interface: foo
    interface_XXX: bar

Fixes #63903


##### ISSUE TYPE
- Feature Pull Request

##### COMPONENT NAME
lib/ansible/modules/system/ufw.py

##### ADDITIONAL INFORMATION

Problem statement:
ufw module does not support specifying both in & out interfaces for forwarding rules
##### SUMMARY
Currently the module ufw does not allow to specify two network interfaces for forwarding rules (rule='allow' routed='yes').
What I'd like to achieve is this rule:
`ufw route allow in on ethX out on ethY to 10.0.0.0/8 from 192.168.0.0/24`

To my knowledge, the best one can do is pick one interface and decide to flag it with "in" or "out" traffic using the `direction` property. So the equivalent of either:
`ufw route allow in on ethX to 10.0.0.0/8 from 192.168.0.0/24` or 
`ufw route allow out on ethY to 10.0.0.0/8 from 192.168.0.0/24`

Is there a mechanism to provide both interfaces to the module?

##### ISSUE TYPE
- Feature request

##### COMPONENT NAME
module ufw

##### ANSIBLE VERSION
```
ansible 2.8.2
  config file = /mnt/c/dev/sandboxes/ansible/ansible.cfg
  configured module search path = [u'/home/me/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/me/virtualenvs/ansible/local/lib/python2.7/site-packages/ansible
  executable location = /home/me/virtualenvs/ansible/bin/ansible
  python version = 2.7.12 (default, Dec  4 2017, 14:50:18) [GCC 5.4.0 20160609]
```

##### CONFIGURATION
```
ANSIBLE_PIPELINING(/mnt/c/dev/sandboxes/ansible/ansible.cfg) = True
DEFAULT_ROLES_PATH(/mnt/c/dev/sandboxes/ansible/ansible.cfg) = [u'/mnt/c/dev/sandboxes/ansible/.imported_roles', u'/mnt/c/dev/sandboxes/ansible/roles']
DEFAULT_VAULT_PASSWORD_FILE(env: ANSIBLE_VAULT_PASSWORD_FILE) = /home/me/.vault_pass.txt
```

##### OS / ENVIRONMENT
Ubuntu Linux 18.04 LTS


##### STEPS TO REPRODUCE
Cannot really (re)produce, we just can't express it. There is a single parameter `interface` and we would need 2. (Or a convention to pass a tuple of interfaces into `interface`, comma separated?)


##### EXPECTED RESULTS
Being able to specify an "in" interface and an "out" interface.
In the following snippet, only the "out" interface can be mentioned.
```yaml
changed: [testvm1] => (item={u'direction': u'out', u'to_ip': u'0.0.0.0/0', u'interface': u'ethY', u'route': u'yes', u'from_ip': u'192.168.0.0/24', u'rule': u'allow'}) => {
    "ansible_loop_var": "item",
    "changed": true,
    "commands": [
        "/usr/sbin/ufw status verbose",
        "/bin/grep -h '^### tuple' /lib/ufw/user.rules /lib/ufw/user6.rules /etc/ufw/user.rules /etc/ufw/user6.rules /var/lib/ufw/user.rules /var/lib/ufw/user6.rules",
        "/usr/sbin/ufw --version",
        "/usr/sbin/ufw route allow out on ethY from 192.168.0.0/24 to 0.0.0.0/0",
        "/usr/sbin/ufw status verbose",
        "/bin/grep -h '^### tuple' /lib/ufw/user.rules /lib/ufw/user6.rules /etc/ufw/user.rules /etc/ufw/user6.rules /var/lib/ufw/user.rules /var/lib/ufw/user6.rules"
    ],
    "invocation": {
        "module_args": {
            "app": null,
            "comment": null,
            "default": null,
            "delete": false,
            "direction": "out",
            "from_ip": "192.168.0.0/24",
            "from_port": null,
            "insert": null,
            "insert_relative_to": "zero",
            "interface": "ethY",
            "log": false,
            "logging": null,
            "proto": null,
            "route": true,
            "rule": "allow",
            "state": null,
            "to_ip": "0.0.0.0/0",
            "to_port": null
        }
    },
    "item": {
        "direction": "out",
        "from_ip": "192.168.0.0/24",
        "interface": "ethY",
        "route": "yes",
        "rule": "allow",
        "to_ip": "0.0.0.0/0"
    },
    "msg": "Status: active\nLogging: off\nDefault: deny (incoming), allow (outgoing), deny (routed)\nNew profiles: skip\n\nTo                         Action      From\n--                         ------      ----\n10.0.0.0/8 on ethY         ALLOW FWD   10.254.33.0/24"
}
```
##### ACTUAL RESULTS
We must choose which interface we want to specify as there is only one `interface` parameter. (In my case, I will specify the output interface preferably over the incoming interface and count on the address range to be selective enough but YMVV)

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 @felixfontein @ovcharenko @pyykkis
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
It is currently not possible to specify more than one interface. I wouldn't call that a bug though, more like a feature request :)

Do you have a good idea how two interfaces could be specified in the module's interface without making things way more complicated?
Yes indeed I did not change the issue type and used blindly the one suggested in the issue template. I'll edit right away. This is clearly a feature request to get on par with what ufw can do. 

I'm not familiar with ufw module internals nor with ufw syntax. I'm currently learning both at the same time while trying to setup a box (first manually using ufw, then translating to ufw module syntax).

> Do you have a good idea how two interfaces could be specified in the module's interface without making things way more complicated?

Short answer is no, unfortunately.  As a newcomers I'm already confused as hell about which parameters can/cannot be filled depending on the command I'm trying to issue, and what are the allowed values in the context of a command. So adding to the module interface more parameters which can only be used in certain circumstances is clearly not going to improve the on-boarding.

My first thought is 3 parameters:
* `interface`: for backward compatibility, really it should be allowed only when used in conjunction of `direction` (but that would break backward compatibility? TBV), forbidden for a forwarding rule because ambiguous
* `interface_in`: explicitly define an interface and the incoming direction, may conflict with `direction` value, can be used for a filtering rule and for a forwarding rule
* `interface_out`: explicitly define an interface and the outgoing direction, may conflict with `direction` value, can be used for a filtering rule and for a forwarding rule

That would be quite a lot to explain in the doc about what are the different scenario allowed.

Maybe one way to simplify a bit would be to split ufw module into several (sub)modules, each one limited to a single ufw command (like (de)activation, default policies setup, logging setup, rule creation, rule deletion). This would limit the parameters list to those really in relation with the command. And it would also allow to document the only allowed values of a parameter, given the command context. A quick example is `direction` that currently allows values `in` and `incoming` (among others). But really `in` is not a convenient shortcut for `incoming`. You have to use `in` while managing rules whereas `incoming` is used for setting up the default policy. That is not documented and does not make sense until you study closely the exact ufw syntax. (And I will forget in a jizzy)

We could even go as far as having one module for filtering rules and one for forwarding rules.  This way it is the module interface that clearly state what parameters can be used. (There would be no `interface` for the module for forwarding rules for example). But I guess that this approach would require a lot of code duplication? And in the end, a forwarding rule is like a filtering rule but with an extra piece of information about interfaces so I guess we will always have the tendency to generalize into a single module?

Commit message: ufw: add unit tests for direction and interface

Extend the unit tests for the UFW module to test the `direction` and
`interface` parameters.  This will help in the implementation of a fix
for issue #63903.

Patch to review:
diff --git a/test/integration/targets/ufw/tasks/main.yml b/test/integration/targets/ufw/tasks/main.yml
index f7975de9337061..28198cd600feec 100644
--- a/test/integration/targets/ufw/tasks/main.yml
+++ b/test/integration/targets/ufw/tasks/main.yml
@@ -7,8 +7,10 @@
 - name: Install iptables (SuSE only)
   package:
     name: iptables
+  become: yes
   when: ansible_os_family == 'Suse'
 - name: Install ufw
+  become: yes
   package:
     name: ufw
 
@@ -17,6 +19,7 @@
   - include_tasks: run-test.yml
     with_fileglob:
     - "tests/*.yml"
+  become: yes
 
   # Cleanup
   always:
diff --git a/test/units/modules/system/test_ufw.py b/test/units/modules/system/test_ufw.py
index 1cfbac2782101b..703f10e738c910 100644
--- a/test/units/modules/system/test_ufw.py
+++ b/test/units/modules/system/test_ufw.py
@@ -52,6 +52,10 @@
     "ufw --dry-run allow from any to any port 7000 proto tcp": skippg_adding_existing_rules,
     "ufw --dry-run delete allow from any to any port 7000 proto tcp": "",
     "ufw --dry-run delete allow from any to any port 7001 proto tcp": user_rules_with_port_7000,
+    "ufw --dry-run allow in on foo from any to any port 7003 proto tcp": "",
+    "ufw --dry-run allow in on foo from 1.1.1.1 port 7002 to 8.8.8.8 port 7003 proto tcp": "",
+    "ufw --dry-run allow out on foo from any to any port 7004 proto tcp": "",
+    "ufw --dry-run allow out on foo from 1.1.1.1 port 7003 to 8.8.8.8 port 7004 proto tcp": "",
     grep_config_cli: user_rules_with_port_7000
 }
 
@@ -169,6 +173,60 @@ def test_check_mode_add_rules(self):
         result = self.__getResult(do_nothing_func_port_7000)
         self.assertFalse(result.exception.args[0]['changed'])
 
+    def test_check_mode_add_direction_in(self):
+        set_module_args({
+            'rule': 'allow',
+            'proto': 'tcp',
+            'port': '7003',
+            'direction': 'in',
+            'interface': 'foo',
+            '_ansible_check_mode': True
+        })
+        result = self.__getResult(do_nothing_func_port_7000)
+        self.assertTrue(result.exception.args[0]['changed'])
+
+    def test_check_mode_add_direction_in_with_ip(self):
+        set_module_args({
+            'rule': 'allow',
+            'proto': 'tcp',
+            'from_ip': '1.1.1.1',
+            'from_port': '7002',
+            'to_ip': '8.8.8.8',
+            'to_port': '7003',
+            'direction': 'in',
+            'interface': 'foo',
+            '_ansible_check_mode': True
+        })
+        result = self.__getResult(do_nothing_func_port_7000)
+        self.assertTrue(result.exception.args[0]['changed'])
+
+    def test_check_mode_add_direction_out(self):
+        set_module_args({
+            'rule': 'allow',
+            'proto': 'tcp',
+            'port': '7004',
+            'direction': 'out',
+            'interface': 'foo',
+            '_ansible_check_mode': True
+        })
+        result = self.__getResult(do_nothing_func_port_7000)
+        self.assertTrue(result.exception.args[0]['changed'])
+
+    def test_check_mode_add_direction_out_with_ip(self):
+        set_module_args({
+            'rule': 'allow',
+            'proto': 'tcp',
+            'from_ip': '1.1.1.1',
+            'from_port': '7003',
+            'to_ip': '8.8.8.8',
+            'to_port': '7004',
+            'direction': 'out',
+            'interface': 'foo',
+            '_ansible_check_mode': True
+        })
+        result = self.__getResult(do_nothing_func_port_7000)
+        self.assertTrue(result.exception.args[0]['changed'])
+
     def test_check_mode_delete_existing_rules(self):
 
         set_module_args({


Code review comments:
diff_hunk: @@ -327,6 +349,8 @@ def main():
         supports_check_mode=True,
         mutually_exclusive=[
             ['name', 'proto', 'logging'],
+            ['direction', 'interface_in'],
+            ['direction', 'interface_out'],
path: lib/ansible/modules/system/ufw.py
text: @user1:
You need two more entries `['interface', 'interface_in']` and `['interface', 'interface_out']`.

@author:
Is that required even though `interface` depends on `direction`?

https://github.com/ansible/ansible/blob/0d34a2eceea9178553e23f53ed24ba58e3722833/lib/ansible/modules/system/ufw.py#L332-L334

I couldn't find much documentation on `required_by` other than a one-sentence description in `developing_modules_general_windows.rst`, but my interpretation is that an error will always be raised if the param in the key is given without the param(s) in the value?  If not I'll fix it.

Thanks for the review!

@user1:
You are right, they are not required because of that. Also, `required_by` works as you describe (I checked its implementation to be sure: https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/common/validation.py#L133-L166). But maybe adding a comment that `required_if` implies that `interface` is also mutually exclusive to both `interface_in` and `interface_out` would probably help future readers as well :)