Skip to content

Commit

Permalink
dladm_vnic python3 fixes and tests (#132)
Browse files Browse the repository at this point in the history
* * Explicitly add types for all module arguments.
* Fix logic to match the `is_valid_{unicast_mac,vlan_id}` function names and don't allow VLAN 0.
* Add tests for this module.

* Add changelog fragment.

* Fix sanity tests.

* No longer need to ignore 'doc-missing-type'.

* Update changelog fragment.

Co-authored-by: Felix Fontein <[email protected]>

* The valid VID range is 1-4094, confirmed from the Illumos source code.

* Update changelog fragment.

Co-authored-by: Felix Fontein <[email protected]>

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit cfe13ea)
  • Loading branch information
jbronn authored and Patchback committed Oct 15, 2020
1 parent 17a76f0 commit 0be2d78
Show file tree
Hide file tree
Showing 7 changed files with 272 additions and 13 deletions.
4 changes: 4 additions & 0 deletions changelogs/fragments/131_dladm_vnic.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
bugfixes:
- dladm_vnic - fixed issue where setting vlan in Python 3 caused a type error (https://github.com/ansible-collections/community.network/issues/131).
- dladm_vnic - vlan IDs 0 and 4095 are now correctly identified as invalid (https://github.com/ansible-collections/community.network/pull/132).
25 changes: 15 additions & 10 deletions plugins/modules/network/illumos/dladm_vnic.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@
description:
- VNIC name.
required: true
type: str
link:
description:
- VNIC underlying link name.
required: true
type: str
temporary:
description:
- Specifies that the VNIC is temporary. Temporary VNICs
Expand All @@ -38,19 +40,22 @@
required: false
default: false
aliases: [ "macaddr" ]
type: str
vlan:
description:
- Enable VLAN tagging for this VNIC. The VLAN tag will have id
I(vlan).
required: false
default: false
aliases: [ "vlan_id" ]
type: int
state:
description:
- Create or delete Solaris/illumos VNIC.
required: false
default: "present"
choices: [ "present", "absent" ]
type: str
'''

EXAMPLES = '''
Expand Down Expand Up @@ -176,22 +181,22 @@ def is_valid_unicast_mac(self):

mac_re = re.match(self.UNICAST_MAC_REGEX, self.mac)

return mac_re is None
return mac_re is not None

def is_valid_vlan_id(self):

return 0 <= self.vlan <= 4095
return 0 < self.vlan < 4095


def main():
module = AnsibleModule(
argument_spec=dict(
name=dict(required=True),
link=dict(required=True),
mac=dict(default=None, aliases=['macaddr']),
vlan=dict(default=None, aliases=['vlan_id']),
temporary=dict(default=False, type='bool'),
state=dict(default='present', choices=['absent', 'present']),
name=dict(type='str', required=True),
link=dict(type='str', required=True),
mac=dict(type='str', aliases=['macaddr']),
vlan=dict(type='int', aliases=['vlan_id']),
temporary=dict(type='bool', default=False),
state=dict(type='str', default='present', choices=['absent', 'present']),
),
supports_check_mode=True
)
Expand All @@ -208,7 +213,7 @@ def main():
result['temporary'] = vnic.temporary

if vnic.mac is not None:
if vnic.is_valid_unicast_mac():
if not vnic.is_valid_unicast_mac():
module.fail_json(msg='Invalid unicast MAC address',
mac=vnic.mac,
name=vnic.name,
Expand All @@ -218,7 +223,7 @@ def main():
result['mac'] = vnic.mac

if vnic.vlan is not None:
if vnic.is_valid_vlan_id():
if not vnic.is_valid_vlan_id():
module.fail_json(msg='Invalid VLAN tag',
mac=vnic.mac,
name=vnic.name,
Expand Down
1 change: 0 additions & 1 deletion tests/sanity/ignore-2.10.txt
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,6 @@ plugins/modules/network/illumos/dladm_vlan.py validate-modules:doc-required-mism
plugins/modules/network/illumos/dladm_vlan.py validate-modules:parameter-type-not-in-doc
plugins/modules/network/illumos/dladm_vnic.py pylint:blacklisted-name
plugins/modules/network/illumos/dladm_vnic.py validate-modules:doc-default-does-not-match-spec
plugins/modules/network/illumos/dladm_vnic.py validate-modules:doc-missing-type
plugins/modules/network/illumos/flowadm.py pylint:blacklisted-name
plugins/modules/network/illumos/flowadm.py validate-modules:doc-choices-do-not-match-spec
plugins/modules/network/illumos/flowadm.py validate-modules:doc-missing-type
Expand Down
1 change: 0 additions & 1 deletion tests/sanity/ignore-2.11.txt
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,6 @@ plugins/modules/network/illumos/dladm_vlan.py validate-modules:doc-required-mism
plugins/modules/network/illumos/dladm_vlan.py validate-modules:parameter-type-not-in-doc
plugins/modules/network/illumos/dladm_vnic.py pylint:blacklisted-name
plugins/modules/network/illumos/dladm_vnic.py validate-modules:doc-default-does-not-match-spec
plugins/modules/network/illumos/dladm_vnic.py validate-modules:doc-missing-type
plugins/modules/network/illumos/flowadm.py pylint:blacklisted-name
plugins/modules/network/illumos/flowadm.py validate-modules:doc-choices-do-not-match-spec
plugins/modules/network/illumos/flowadm.py validate-modules:doc-missing-type
Expand Down
1 change: 0 additions & 1 deletion tests/sanity/ignore-2.9.txt
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,6 @@ plugins/modules/network/illumos/dladm_vlan.py validate-modules:doc-missing-type
plugins/modules/network/illumos/dladm_vlan.py validate-modules:parameter-type-not-in-doc
plugins/modules/network/illumos/dladm_vnic.py pylint:blacklisted-name
plugins/modules/network/illumos/dladm_vnic.py validate-modules:doc-default-does-not-match-spec
plugins/modules/network/illumos/dladm_vnic.py validate-modules:doc-missing-type
plugins/modules/network/illumos/flowadm.py pylint:blacklisted-name
plugins/modules/network/illumos/flowadm.py validate-modules:doc-choices-do-not-match-spec
plugins/modules/network/illumos/flowadm.py validate-modules:doc-missing-type
Expand Down
Empty file.
253 changes: 253 additions & 0 deletions tests/unit/plugins/modules/network/illumos/test_dladm_vnic.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,253 @@
# Copyright (c) 2020 Justin Bronn <[email protected]>
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

from __future__ import absolute_import, division, print_function
__metaclass__ = type

import json

import pytest
from ansible.module_utils.basic import AnsibleModule
from ansible_collections.community.network.plugins.modules.network.illumos import (
dladm_vnic,
)
from ansible_collections.community.network.tests.unit.plugins.modules.utils import (
set_module_args,
)


DLADM = "/usr/sbin/dladm"


def mocker_vnic_set(mocker, vnic_exists=False, rc=0, out="", err=""):
"""
Common mocker object
"""
get_bin_path = mocker.patch.object(AnsibleModule, "get_bin_path")
get_bin_path.return_value = DLADM
run_command = mocker.patch.object(AnsibleModule, "run_command")
run_command.return_value = (rc, out, err)
vnic_exists_func = mocker.patch.object(dladm_vnic.VNIC, "vnic_exists")
vnic_exists_func.return_value = vnic_exists


@pytest.fixture
def mocked_vnic_create(mocker):
mocker_vnic_set(mocker)


@pytest.fixture
def mocked_vnic_delete(mocker):
mocker_vnic_set(mocker, vnic_exists=True)


def test_vnic_create(mocked_vnic_create, capfd):
"""
vnic creation
"""
vnic_name = "vnic0"
vnic_link = "e1000g0"
set_module_args(
{
"name": vnic_name,
"link": vnic_link,
"state": "present",
"_ansible_check_mode": False,
}
)
with pytest.raises(SystemExit):
dladm_vnic.main()

assert AnsibleModule.run_command.call_count == 1
args = AnsibleModule.run_command.call_args_list[0][0]
assert args[0][0] == DLADM
assert args[0][1] == "create-vnic"
assert args[0][2] == "-l"
assert args[0][3] == vnic_link
assert args[0][4] == vnic_name

out, err = capfd.readouterr()
results = json.loads(out)
assert not results.get("failed")
assert results["changed"]


def test_vnic_delete(mocked_vnic_delete, capfd):
"""
vnic deletion
"""
vnic_name = "net0"
vnic_link = "xge1"
vnic_temp = (False, True)

for temp in vnic_temp:
set_module_args(
{
"name": vnic_name,
"state": "absent",
"link": vnic_link,
"temporary": temp,
"_ansible_check_mode": False,
}
)
with pytest.raises(SystemExit):
dladm_vnic.main()

out, err = capfd.readouterr()
results = json.loads(out)
assert not results.get("failed")
assert results["changed"]

assert AnsibleModule.run_command.call_count == len(vnic_temp)
for i, call_args in enumerate(AnsibleModule.run_command.call_args_list):
args = call_args[0][0]
print(args)
assert args[0] == DLADM
assert args[1] == "delete-vnic"
if vnic_temp[i]:
assert args[2] == '-t'
assert args[3] == vnic_name
else:
assert args[2] == vnic_name


def test_vnic_create_vlan(mocked_vnic_create, capfd):
"""
vnic creation with valid vlan
"""
vnic_name = "vnic0"
vnic_link = "e1000g0"
vnic_vlans = (1, "23", 23, 4094)
vnic_temp = (True, False, False, True)

for vlan, temp in zip(vnic_vlans, vnic_temp):
set_module_args(
{
"name": vnic_name,
"link": vnic_link,
"state": "present",
"temporary": temp,
"vlan": vlan,
"_ansible_check_mode": False,
}
)
with pytest.raises(SystemExit):
dladm_vnic.main()

out, err = capfd.readouterr()
results = json.loads(out)
assert not results.get("failed")
assert results["changed"]

assert AnsibleModule.run_command.call_count == len(vnic_vlans)
for i, call_args in enumerate(AnsibleModule.run_command.call_args_list):
args = call_args[0][0]
assert args[0] == DLADM
assert args[1] == "create-vnic"
if vnic_temp[i]:
assert args[2] == "-t"
arg_idx = 3
else:
arg_idx = 2
assert args[arg_idx] == "-v"
assert args[arg_idx + 1] == int(vnic_vlans[i])
assert args[arg_idx + 2] == "-l"
assert args[arg_idx + 3] == vnic_link
assert args[arg_idx + 4] == vnic_name


def test_vnic_create_vlan_invalid(mocked_vnic_create, capfd):
"""
vnic creation failures with invalid vlan
"""
vnic_name = "vnic1"
vnic_link = "e1000g1"
vnic_vlans = ("foo", ["bar"], 0, 4095)

for vlan in vnic_vlans:
set_module_args(
{
"name": vnic_name,
"link": vnic_link,
"state": "present",
"vlan": vlan,
"_ansible_check_mode": False,
}
)
with pytest.raises(SystemExit):
dladm_vnic.main()

out, err = capfd.readouterr()
results = json.loads(out)
assert results.get("failed")


def test_vnic_create_mac(mocked_vnic_create, capfd):
"""
vnic creation with valid mac address
"""
vnic_name = "vnic0"
vnic_link = "ibg0"
vnic_macs = ("00:20:91:de:ad:be", "00:0c:29:be:ef:be")
vnic_temp = (False, True)

for mac, temp in zip(vnic_macs, vnic_temp):
set_module_args(
{
"name": vnic_name,
"link": vnic_link,
"mac": mac,
"state": "present",
"temporary": temp,
"_ansible_check_mode": False,
}
)
with pytest.raises(SystemExit):
dladm_vnic.main()

out, err = capfd.readouterr()
results = json.loads(out)
assert not results.get("failed")
assert results["changed"]

assert AnsibleModule.run_command.call_count == len(vnic_macs)
for i, call_args in enumerate(AnsibleModule.run_command.call_args_list):
args = call_args[0][0]
assert args[0] == DLADM
assert args[1] == "create-vnic"
if vnic_temp[i]:
assert args[2] == "-t"
arg_idx = 3
else:
arg_idx = 2
assert args[arg_idx] == "-m"
assert args[arg_idx + 1] == vnic_macs[i]
assert args[arg_idx + 2] == "-l"
assert args[arg_idx + 3] == vnic_link
assert args[arg_idx + 4] == vnic_name


def test_vnic_create_mac_invalid(mocked_vnic_create, capfd):
"""
vnic creation with an invalid mac address
"""
vnic_name = "vnic0"
vnic_link = "ibg0"
mac_invalid_args = ("01:20:91:de:ad:be", "00:0c:29:be:ef:")

for mac in mac_invalid_args:
set_module_args(
{
"name": vnic_name,
"link": vnic_link,
"state": "present",
"mac": mac,
"_ansible_check_mode": False,
}
)
with pytest.raises(SystemExit):
dladm_vnic.main()

out, err = capfd.readouterr()
results = json.loads(out)
assert results.get("failed")

0 comments on commit 0be2d78

Please sign in to comment.