From c949f3a8341a291967dff3a0ba55e63d10a06cb0 Mon Sep 17 00:00:00 2001
From: Sam Potekhin <heaveaxy@gmail.com>
Date: Tue, 9 May 2023 00:44:30 +0700
Subject: [PATCH] nmcli: added new module option 'slave_type' to allow create
 non-ethernet slave connections (#6108)

* nmcli: added new module option 'slave_type' to allow create non-ethernet slave connections

* argument specs updated

* documentation updated

* examples updated

* added warning message when using type='bridge-slave'

* remove trailing whitespace

* Added warnings about rewrite 'slave-type' property when using type one of 'bond-slave', 'bridge-slave', 'team-slave'.
Added module fails when user sets contradicting values of 'slave-type' for types 'bond-slave', 'bridge-slave', 'team-slave'.
Returned back checking for types that can be a slave to assign 'master' and 'slave-type' properties.

* Extending list of slave-conn-types

* Update plugins/modules/nmcli.py

Version updated

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

* Update plugins/modules/nmcli.py

Updated documentation for `slave_type`

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

* Updated argspec's 'required_by' for 'master' property.

* Fixed mistake in property  naming in module argspec.

* changelog fragment and module docs updated

* Validation of 'master', 'slave_type' options improved. (rebased)

* Validation of 'master' and 'slave_type' separated to special method.

* Wrote 6 tests for slave_type option behaviour

* Removed erroneously added property 'hairpin'

* Update version_added for 'slave_type'

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

* Update changelogs/fragments/473-nmcli-slave-type-implemented.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/nmcli.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Let master be without slave_type

---------

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Co-authored-by: Felix Fontein <felix@fontein.de>
---
 .../473-nmcli-slave-type-implemented.yml      |   2 +
 plugins/modules/nmcli.py                      | 103 +++++-
 tests/unit/plugins/modules/test_nmcli.py      | 292 ++++++++++++++++++
 3 files changed, 389 insertions(+), 8 deletions(-)
 create mode 100644 changelogs/fragments/473-nmcli-slave-type-implemented.yml

diff --git a/changelogs/fragments/473-nmcli-slave-type-implemented.yml b/changelogs/fragments/473-nmcli-slave-type-implemented.yml
new file mode 100644
index 0000000000..aa9dabe12e
--- /dev/null
+++ b/changelogs/fragments/473-nmcli-slave-type-implemented.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - nmcli - new module option ``slave_type`` added to allow creation of various types of slave devices (https://github.com/ansible-collections/community.general/issues/473, https://github.com/ansible-collections/community.general/pull/6108).
diff --git a/plugins/modules/nmcli.py b/plugins/modules/nmcli.py
index 4c7920dd03..053182c2c0 100644
--- a/plugins/modules/nmcli.py
+++ b/plugins/modules/nmcli.py
@@ -66,6 +66,8 @@ options:
             - Type C(macvlan) is added in community.general 6.6.0.
             - Type C(wireguard) is added in community.general 4.3.0.
             - Type C(vpn) is added in community.general 5.1.0.
+            - Using C(bond-slave), C(bridge-slave) or C(team-slave) implies C(ethernet) connection type with corresponding I(slave_type) option.
+            - If you want to control non-ethernet connection attached to C(bond), C(bridge) or C(team) consider using C(slave_type) option.
         type: str
         choices: [ bond, bond-slave, bridge, bridge-slave, dummy, ethernet, generic, gre, infiniband, ipip, macvlan, sit, team, team-slave, vlan, vxlan,
             wifi, gsm, wireguard, vpn ]
@@ -81,9 +83,16 @@ options:
         type: str
         choices: [ datagram, connected ]
         version_added: 5.8.0
+    slave_type:
+        description:
+            - Type of the device of this slave's master connection (for example C(bond)).
+        type: str
+        choices: [ 'bond', 'bridge', 'team' ]
+        version_added: 7.0.0
     master:
         description:
             - Master <master (ifname, or connection UUID or conn_name) of bridge, team, bond master connection profile.
+            - Mandatory if I(slave_type) is defined.
         type: str
     ip4:
         description:
@@ -1429,6 +1438,39 @@ EXAMPLES = r'''
     autoconnect: false
     state: present
 
+## Creating bond attached to bridge example
+- name: Create bond attached to bridge
+  community.general.nmcli:
+    type: bond
+    conn_name: bond0
+    slave_type: bridge
+    master: br0
+    state: present
+
+- name: Create master bridge
+  community.general.nmcli:
+    type: bridge
+    conn_name: br0
+    method4: disabled
+    method6: disabled
+    state: present
+
+## Creating vlan connection attached to bridge
+- name: Create master bridge
+  community.general.nmcli:
+    type: bridge
+    conn_name: br0
+    state: present
+
+- name: Create VLAN 5
+  community.general.nmcli:
+    type: vlan
+    conn_name: eth0.5
+    slave_type: bridge
+    master: br0
+    vlandev: eth0
+    vlanid: 5
+    state: present
 '''
 
 RETURN = r"""#
@@ -1475,6 +1517,7 @@ class Nmcli(object):
         self.ignore_unsupported_suboptions = module.params['ignore_unsupported_suboptions']
         self.autoconnect = module.params['autoconnect']
         self.conn_name = module.params['conn_name']
+        self.slave_type = module.params['slave_type']
         self.master = module.params['master']
         self.ifname = module.params['ifname']
         self.type = module.params['type']
@@ -1570,6 +1613,14 @@ class Nmcli(object):
 
         self.edit_commands = []
 
+        self.extra_options_validation()
+
+    def extra_options_validation(self):
+        """ Additional validation of options set passed to module that cannot be implemented in module's argspecs. """
+        if self.type not in ("bridge-slave", "team-slave", "bond-slave"):
+            if self.master is None and self.slave_type is not None:
+                self.module.fail_json(msg="'master' option is required when 'slave_type' is specified.")
+
     def execute_command(self, cmd, use_unsafe_shell=False, data=None):
         if isinstance(cmd, list):
             cmd = [to_text(item) for item in cmd]
@@ -1634,6 +1685,7 @@ class Nmcli(object):
         if self.slave_conn_type:
             options.update({
                 'connection.master': self.master,
+                'connection.slave-type': self.slave_type,
             })
 
         # Options specific to a connection type.
@@ -1649,9 +1701,17 @@ class Nmcli(object):
                 'xmit_hash_policy': self.xmit_hash_policy,
             })
         elif self.type == 'bond-slave':
-            options.update({
-                'connection.slave-type': 'bond',
-            })
+            if self.slave_type and self.slave_type != 'bond':
+                self.module.fail_json(msg="Connection type '%s' cannot be combined with '%s' slave-type. "
+                                          "Allowed slave-type for '%s' is 'bond'."
+                                          % (self.type, self.slave_type, self.type)
+                                      )
+            if not self.slave_type:
+                self.module.warn("Connection 'slave-type' property automatically set to 'bond' "
+                                 "because of using 'bond-slave' connection type.")
+                options.update({
+                    'connection.slave-type': 'bond',
+                })
         elif self.type == 'bridge':
             options.update({
                 'bridge.ageing-time': self.ageingtime,
@@ -1675,16 +1735,36 @@ class Nmcli(object):
                     'team.runner-fast-rate': self.runner_fast_rate,
                 })
         elif self.type == 'bridge-slave':
+            if self.slave_type and self.slave_type != 'bridge':
+                self.module.fail_json(msg="Connection type '%s' cannot be combined with '%s' slave-type. "
+                                          "Allowed slave-type for '%s' is 'bridge'."
+                                          % (self.type, self.slave_type, self.type)
+                                      )
+            if not self.slave_type:
+                self.module.warn("Connection 'slave-type' property automatically set to 'bridge' "
+                                 "because of using 'bridge-slave' connection type.")
+                options.update({'connection.slave-type': 'bridge'})
+            self.module.warn(
+                "Connection type as 'bridge-slave' implies 'ethernet' connection with 'bridge' slave-type. "
+                "Consider using slave_type='bridge' with necessary type."
+            )
             options.update({
-                'connection.slave-type': 'bridge',
                 'bridge-port.path-cost': self.path_cost,
                 'bridge-port.hairpin-mode': self.hairpin,
                 'bridge-port.priority': self.slavepriority,
             })
         elif self.type == 'team-slave':
-            options.update({
-                'connection.slave-type': 'team',
-            })
+            if self.slave_type and self.slave_type != 'team':
+                self.module.fail_json(msg="Connection type '%s' cannot be combined with '%s' slave-type. "
+                                          "Allowed slave-type for '%s' is 'team'."
+                                          % (self.type, self.slave_type, self.type)
+                                      )
+            if not self.slave_type:
+                self.module.warn("Connection 'slave-type' property automatically set to 'team' "
+                                 "because of using 'team-slave' connection type.")
+                options.update({
+                    'connection.slave-type': 'team',
+                })
         elif self.tunnel_conn_type:
             options.update({
                 'ip-tunnel.local': self.ip_tunnel_local,
@@ -1869,6 +1949,12 @@ class Nmcli(object):
     @property
     def slave_conn_type(self):
         return self.type in (
+            'ethernet',
+            'bridge',
+            'bond',
+            'vlan',
+            'team',
+            'wifi',
             'bond-slave',
             'bridge-slave',
             'team-slave',
@@ -2260,6 +2346,7 @@ def main():
             state=dict(type='str', required=True, choices=['absent', 'present']),
             conn_name=dict(type='str', required=True),
             master=dict(type='str'),
+            slave_type=dict(type='str', choices=['bond', 'bridge', 'team']),
             ifname=dict(type='str'),
             type=dict(type='str',
                       choices=[
@@ -2416,7 +2503,7 @@ def main():
         if nmcli.runner_fast_rate is not None and nmcli.runner != "lacp":
             nmcli.module.fail_json(msg="runner-fast-rate is only allowed for runner lacp")
     # team-slave checks
-    if nmcli.type == 'team-slave':
+    if nmcli.type == 'team-slave' or nmcli.slave_type == 'team':
         if nmcli.master is None:
             nmcli.module.fail_json(msg="Please specify a name for the master when type is %s" % nmcli.type)
         if nmcli.ifname is None:
diff --git a/tests/unit/plugins/modules/test_nmcli.py b/tests/unit/plugins/modules/test_nmcli.py
index 60bd8b99de..577d30e97d 100644
--- a/tests/unit/plugins/modules/test_nmcli.py
+++ b/tests/unit/plugins/modules/test_nmcli.py
@@ -4031,6 +4031,7 @@ def test_bond_connection_unchanged(mocked_generic_connection_diff_check, capfd):
             state=dict(type='str', required=True, choices=['absent', 'present']),
             conn_name=dict(type='str', required=True),
             master=dict(type='str'),
+            slave_type=dict(type=str, choices=['bond', 'bridge', 'team']),
             ifname=dict(type='str'),
             type=dict(type='str',
                       choices=[
@@ -4258,3 +4259,294 @@ def test_macvlan_mod(mocked_generic_connection_modify, capfd):
     results = json.loads(out)
     assert not results.get('failed')
     assert results['changed']
+
+
+TESTCASE_SLAVE_TYPE_BRIDGE_CONNECTION = [
+    {
+        'type': 'ethernet',
+        'conn_name': 'fake_conn',
+        'ifname': 'fake_eth0',
+        'state': 'present',
+        'slave_type': 'bridge',
+        'master': 'fake_br0',
+        '_ansible_check_mode': False,
+    }
+]
+
+
+TESTCASE_SLAVE_TYPE_BRIDGE_CONNECTION_SHOW_OUTPUT = """\
+connection.id:                          fake_conn
+connection.type:                        802-3-ethernet
+connection.interface-name:              fake_eth0
+connection.autoconnect:                 yes
+connection.master:                      --
+connection.slave-type:                  --
+802-3-ethernet.mtu:                     auto
+"""
+
+
+TESTCASE_SLAVE_TYPE_BRIDGE_CONNECTION_UNCHANGED_SHOW_OUTPUT = """\
+connection.id:                          fake_conn
+connection.type:                        802-3-ethernet
+connection.interface-name:              fake_eth0
+connection.autoconnect:                 yes
+connection.master:                      fake_br0
+connection.slave-type:                  bridge
+802-3-ethernet.mtu:                     auto
+"""
+
+
+@pytest.fixture
+def mocked_slave_type_bridge_create(mocker):
+    mocker_set(mocker,
+               execute_return=None,
+               execute_side_effect=(
+                   (0, TESTCASE_SLAVE_TYPE_BRIDGE_CONNECTION_SHOW_OUTPUT, ""),
+                   (0, "", ""),
+               ))
+
+
+@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SLAVE_TYPE_BRIDGE_CONNECTION, indirect=['patch_ansible_module'])
+def test_create_slave_type_bridge(mocked_slave_type_bridge_create, capfd):
+    """
+    Test : slave for bridge created
+    """
+
+    with pytest.raises(SystemExit):
+        nmcli.main()
+
+    assert nmcli.Nmcli.execute_command.call_count == 1
+    arg_list = nmcli.Nmcli.execute_command.call_args_list
+    args, kwargs = arg_list[0]
+
+    assert args[0][0] == '/usr/bin/nmcli'
+    assert args[0][1] == 'con'
+    assert args[0][2] == 'add'
+    assert args[0][3] == 'type'
+    assert args[0][4] == 'ethernet'
+    assert args[0][5] == 'con-name'
+    assert args[0][6] == 'fake_conn'
+    con_master_index = args[0].index('connection.master')
+    slave_type_index = args[0].index('connection.slave-type')
+    assert args[0][con_master_index + 1] == 'fake_br0'
+    assert args[0][slave_type_index + 1] == 'bridge'
+
+    out, err = capfd.readouterr()
+    results = json.loads(out)
+    assert not results.get('failed')
+    assert results['changed']
+
+
+@pytest.fixture
+def mocked_create_slave_type_bridge_unchanged(mocker):
+    mocker_set(mocker,
+               connection_exists=True,
+               execute_return=(0, TESTCASE_SLAVE_TYPE_BRIDGE_CONNECTION_UNCHANGED_SHOW_OUTPUT, ""))
+
+
+@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SLAVE_TYPE_BRIDGE_CONNECTION, indirect=['patch_ansible_module'])
+def test_slave_type_bridge_unchanged(mocked_create_slave_type_bridge_unchanged, capfd):
+    """
+    Test : Existent slave for bridge unchanged
+    """
+    with pytest.raises(SystemExit):
+        nmcli.main()
+
+    out, err = capfd.readouterr()
+    results = json.loads(out)
+    assert not results.get('failed')
+    assert not results['changed']
+
+
+TESTCASE_SLAVE_TYPE_BOND_CONNECTION = [
+    {
+        'type': 'ethernet',
+        'conn_name': 'fake_conn',
+        'ifname': 'fake_eth0',
+        'state': 'present',
+        'slave_type': 'bond',
+        'master': 'fake_bond0',
+        '_ansible_check_mode': False,
+    }
+]
+
+
+TESTCASE_SLAVE_TYPE_BOND_CONNECTION_SHOW_OUTPUT = """\
+connection.id:                          fake_conn
+connection.type:                        802-3-ethernet
+connection.interface-name:              fake_eth0
+connection.autoconnect:                 yes
+connection.master:                      --
+connection.slave-type:                  --
+802-3-ethernet.mtu:                     auto
+"""
+
+
+TESTCASE_SLAVE_TYPE_BOND_CONNECTION_UNCHANGED_SHOW_OUTPUT = """\
+connection.id:                          fake_conn
+connection.type:                        802-3-ethernet
+connection.interface-name:              fake_eth0
+connection.autoconnect:                 yes
+connection.master:                      fake_bond0
+connection.slave-type:                  bond
+802-3-ethernet.mtu:                     auto
+"""
+
+
+@pytest.fixture
+def mocked_slave_type_bond_create(mocker):
+    mocker_set(mocker,
+               execute_return=None,
+               execute_side_effect=(
+                   (0, TESTCASE_SLAVE_TYPE_BOND_CONNECTION_SHOW_OUTPUT, ""),
+                   (0, "", ""),
+               ))
+
+
+@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SLAVE_TYPE_BOND_CONNECTION, indirect=['patch_ansible_module'])
+def test_create_slave_type_bond(mocked_slave_type_bond_create, capfd):
+    """
+    Test : slave for bond created
+    """
+
+    with pytest.raises(SystemExit):
+        nmcli.main()
+
+    assert nmcli.Nmcli.execute_command.call_count == 1
+    arg_list = nmcli.Nmcli.execute_command.call_args_list
+    args, kwargs = arg_list[0]
+
+    assert args[0][0] == '/usr/bin/nmcli'
+    assert args[0][1] == 'con'
+    assert args[0][2] == 'add'
+    assert args[0][3] == 'type'
+    assert args[0][4] == 'ethernet'
+    assert args[0][5] == 'con-name'
+    assert args[0][6] == 'fake_conn'
+    con_master_index = args[0].index('connection.master')
+    slave_type_index = args[0].index('connection.slave-type')
+    assert args[0][con_master_index + 1] == 'fake_bond0'
+    assert args[0][slave_type_index + 1] == 'bond'
+
+    out, err = capfd.readouterr()
+    results = json.loads(out)
+    assert not results.get('failed')
+    assert results['changed']
+
+
+@pytest.fixture
+def mocked_create_slave_type_bond_unchanged(mocker):
+    mocker_set(mocker,
+               connection_exists=True,
+               execute_return=(0, TESTCASE_SLAVE_TYPE_BOND_CONNECTION_UNCHANGED_SHOW_OUTPUT, ""))
+
+
+@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SLAVE_TYPE_BOND_CONNECTION, indirect=['patch_ansible_module'])
+def test_slave_type_bond_unchanged(mocked_create_slave_type_bond_unchanged, capfd):
+    """
+    Test : Existent slave for bridge unchanged
+    """
+    with pytest.raises(SystemExit):
+        nmcli.main()
+
+    out, err = capfd.readouterr()
+    results = json.loads(out)
+    assert not results.get('failed')
+    assert not results['changed']
+
+
+TESTCASE_SLAVE_TYPE_TEAM_CONNECTION = [
+    {
+        'type': 'ethernet',
+        'conn_name': 'fake_conn',
+        'ifname': 'fake_eth0',
+        'state': 'present',
+        'slave_type': 'team',
+        'master': 'fake_team0',
+        '_ansible_check_mode': False,
+    }
+]
+
+
+TESTCASE_SLAVE_TYPE_TEAM_CONNECTION_SHOW_OUTPUT = """\
+connection.id:                          fake_conn
+connection.type:                        802-3-ethernet
+connection.interface-name:              fake_eth0
+connection.autoconnect:                 yes
+connection.master:                      --
+connection.slave-type:                  --
+802-3-ethernet.mtu:                     auto
+"""
+
+
+TESTCASE_SLAVE_TYPE_TEAM_CONNECTION_UNCHANGED_SHOW_OUTPUT = """\
+connection.id:                          fake_conn
+connection.type:                        802-3-ethernet
+connection.interface-name:              fake_eth0
+connection.autoconnect:                 yes
+connection.master:                      fake_team0
+connection.slave-type:                  team
+802-3-ethernet.mtu:                     auto
+"""
+
+
+@pytest.fixture
+def mocked_slave_type_team_create(mocker):
+    mocker_set(mocker,
+               execute_return=None,
+               execute_side_effect=(
+                   (0, TESTCASE_SLAVE_TYPE_TEAM_CONNECTION_SHOW_OUTPUT, ""),
+                   (0, "", ""),
+               ))
+
+
+@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SLAVE_TYPE_TEAM_CONNECTION, indirect=['patch_ansible_module'])
+def test_create_slave_type_team(mocked_slave_type_team_create, capfd):
+    """
+    Test : slave for bond created
+    """
+
+    with pytest.raises(SystemExit):
+        nmcli.main()
+
+    assert nmcli.Nmcli.execute_command.call_count == 1
+    arg_list = nmcli.Nmcli.execute_command.call_args_list
+    args, kwargs = arg_list[0]
+
+    assert args[0][0] == '/usr/bin/nmcli'
+    assert args[0][1] == 'con'
+    assert args[0][2] == 'add'
+    assert args[0][3] == 'type'
+    assert args[0][4] == 'ethernet'
+    assert args[0][5] == 'con-name'
+    assert args[0][6] == 'fake_conn'
+    con_master_index = args[0].index('connection.master')
+    slave_type_index = args[0].index('connection.slave-type')
+    assert args[0][con_master_index + 1] == 'fake_team0'
+    assert args[0][slave_type_index + 1] == 'team'
+
+    out, err = capfd.readouterr()
+    results = json.loads(out)
+    assert not results.get('failed')
+    assert results['changed']
+
+
+@pytest.fixture
+def mocked_create_slave_type_team_unchanged(mocker):
+    mocker_set(mocker,
+               connection_exists=True,
+               execute_return=(0, TESTCASE_SLAVE_TYPE_TEAM_CONNECTION_UNCHANGED_SHOW_OUTPUT, ""))
+
+
+@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SLAVE_TYPE_TEAM_CONNECTION, indirect=['patch_ansible_module'])
+def test_slave_type_team_unchanged(mocked_create_slave_type_team_unchanged, capfd):
+    """
+    Test : Existent slave for bridge unchanged
+    """
+    with pytest.raises(SystemExit):
+        nmcli.main()
+
+    out, err = capfd.readouterr()
+    results = json.loads(out)
+    assert not results.get('failed')
+    assert not results['changed']