diff --git a/README.md b/README.md index 3573950..3052bf6 100644 --- a/README.md +++ b/README.md @@ -22,11 +22,6 @@ pytest --cov=setup_ovs --cov-report=term-missing --cov-report=xml Branch coverage is enabled in `pyproject.toml`, so the report covers both the statement and the branch criteria. -A handful of tests are marked `xfail(strict=True)`. Each one documents a bug -found while writing the suite and pins the current, wrong behaviour: the -suite fails again the day the bug is fixed, which forces the marker to be -removed along with the fix. Their `reason` field states the defect. - ## Reproducible build The wheel is byte-for-byte reproducible provided `SOURCE_DATE_EPOCH` is set. diff --git a/setup_ovs/check.py b/setup_ovs/check.py index e2de505..5ef169d 100644 --- a/setup_ovs/check.py +++ b/setup_ovs/check.py @@ -32,6 +32,8 @@ def configuration_check(config): raise SetupOVSConfigException( "The configuration should be a dictionary" ) + dpdk_interfaces = [] + system_interfaces = [] if "bridges" in config: if not isinstance(config["bridges"], list): raise SetupOVSConfigException( @@ -51,7 +53,12 @@ def configuration_check(config): raise SetupOVSConfigException( "A port must be a dictionary" ) - _check_port_configuration(bridge["name"], port) + _check_port_configuration( + bridge["name"], + port, + dpdk_interfaces, + system_interfaces, + ) if "other_config" in bridge: attribute_value = ( [bridge["other_config"]] @@ -93,28 +100,59 @@ def configuration_check(config): logging.info("Configuration check: OK") -def _attribute_is_a_port( - attribute_name, attribute_value, bridge_name, port_name +def _attribute_is_in_range( + attribute_name, attribute_value, bridge_name, port_name, maximum ): """ - Check if an attribute is a TCP or UDP port + Check if an attribute is an integer between 0 and maximum :param attribute_name: The attribute name :param attribute_value: The attribute value :param bridge_name: The attribute bridge name :param port_name: The attribute port name + :param maximum: The highest accepted value, included """ if not isinstance(attribute_value, int): raise SetupOVSConfigException( "Bridge {} Port {}: attribute {} must be an " "integer".format(bridge_name, port_name, attribute_name) ) - if attribute_value < 0 or attribute_value > 4095: + if attribute_value < 0 or attribute_value > maximum: raise SetupOVSConfigException( "Bridge {} Port {}: attribute {} must be in range 0 to " - "4,095".format(bridge_name, port_name, attribute_name) + "{:,}".format(bridge_name, port_name, attribute_name, maximum) ) +def _attribute_is_a_port( + attribute_name, attribute_value, bridge_name, port_name +): + """ + Check if an attribute is a TCP or UDP port + :param attribute_name: The attribute name + :param attribute_value: The attribute value + :param bridge_name: The attribute bridge name + :param port_name: The attribute port name + """ + _attribute_is_in_range( + attribute_name, attribute_value, bridge_name, port_name, 65535 + ) + + +def _attribute_is_a_vlan_tag( + attribute_name, attribute_value, bridge_name, port_name +): + """ + Check if an attribute is a 802.1Q VLAN identifier + :param attribute_name: The attribute name + :param attribute_value: The attribute value + :param bridge_name: The attribute bridge name + :param port_name: The attribute port name + """ + _attribute_is_in_range( + attribute_name, attribute_value, bridge_name, port_name, 4095 + ) + + def _attribute_is_an_ipv4( attribute_name, attribute_value, bridge_name, port_name ): @@ -155,15 +193,27 @@ def _attribute_is_a_mac( ) -def _check_port_configuration(bridge_name, port): +PORT_TYPES = ( + "internal", + "tap", + "system", + "dpdk", + "dpdkvhostuserclient", + "vxlan", +) + +VLAN_MODES = ("access", "native-tagged", "native-untagged", "trunk") + +MAC_CAPABLE_TYPES = ("tap", "dpdkvhostuserclient") + + +def _check_name_and_type(bridge_name, port): """ - Helper method for _configuration_check which checks the port - configuration + Check the two mandatory port attributes :param bridge_name: the bridge name in which the port take from :param port: the port configuration + :return: the port name and the port type """ - dpdk_interfaces = [] - system_interfaces = [] if "name" not in port: raise SetupOVSConfigException( "Bridge {}: Port without name attribute".format(bridge_name) @@ -175,115 +225,178 @@ def _check_port_configuration(bridge_name, port): "Bridge {}: Port without type attribute".format(bridge_name) ) port_type = port["type"] - if port_type not in ( - "internal", - "tap", - "system", - "dpdk", - "dpdkvhostuserclient", - "vxlan", - ): + if port_type not in PORT_TYPES: raise SetupOVSConfigException( "Bridge {} Port {}: Bad type value: {}".format( bridge_name, port_name, port_type ) ) - if port_type in ("dpdk", "system"): - if "interface" not in port: - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute interface is required with " - "type {}".format(bridge_name, port_name, port_type) + return port_name, port_type + + +def _lspci_address(bridge_name, port_name, interface): + """ + Convert a NIC PCI address in the lspci format + :param bridge_name: the port bridge name + :param port_name: the port name + :param interface: the interface PCI address + :return: the address in the lspci format + """ + match = helpers.PCI_ADDRESS_MATCHER.match(interface) + if not match: + raise SetupOVSConfigException( + "Bridge {} Port {}: NIC {} is not a PCI address." + " Invalid format".format(bridge_name, port_name, interface) + ) + match_group = match.groupdict() + lspci_nic_address_part1 = int(match_group["part1"], 16) + lspci_nic_address_part2 = int(match_group["part2"], 16) + lspci_nic_address_part3 = int(match_group["part3"], 16) + return ( + f"{lspci_nic_address_part1:02x}:" + f"{lspci_nic_address_part2:02x}." + f"{lspci_nic_address_part3:01x}" + ) + + +def _check_dpdk_interface( + bridge_name, port_name, interface, dpdk_interfaces +): + """ + Check a DPDK port interface and claim the NIC + :param bridge_name: the port bridge name + :param port_name: the port name + :param interface: the interface PCI address + :param dpdk_interfaces: the NICs already claimed, appended to by this call + """ + lspci_nic_address = _lspci_address(bridge_name, port_name, interface) + if not helpers.dry_run: + try: + helpers.run_command( + "/usr/bin/lspci -mm | /bin/grep -q " + lspci_nic_address, + shell=True, ) - interface = port["interface"] - if port_type == "dpdk": - # Check interface is a PCI address - match = helpers.PCI_ADDRESS_MATCHER.match(interface) - if not match: - raise SetupOVSConfigException( - "Bridge {} Port {}: NIC {} is not a PCI address." - " Invalid format".format(bridge_name, port_name, interface) + except subprocess.CalledProcessError: + raise SetupOVSConfigException( + "Bridge {} Port {}: Can't find the NIC {}".format( + bridge_name, port_name, interface ) - - # Convert the NIC PCI address in the lspci format - match_group = match.groupdict() - lspci_nic_address_part1 = int(match_group["part1"], 16) - lspci_nic_address_part2 = int(match_group["part2"], 16) - lspci_nic_address_part3 = int(match_group["part3"], 16) - lspci_nic_address = ( - f"{lspci_nic_address_part1:02x}:" - f"{lspci_nic_address_part2:02x}." - f"{lspci_nic_address_part3:01x}" ) - if not helpers.dry_run: - try: - helpers.run_command( - "/usr/bin/lspci -mm | /bin/grep -q " - + lspci_nic_address, - shell=True, - ) - except subprocess.CalledProcessError: - raise SetupOVSConfigException( - "Bridge {} Port {}: Can't find the NIC {}".format( - bridge_name, port_name, interface - ) - ) + if interface in dpdk_interfaces: + raise SetupOVSConfigException( + "Bridge {} Port {}: NIC {} already used in another " + "port".format(bridge_name, port_name, interface) + ) + dpdk_interfaces.append(interface) - if interface in dpdk_interfaces: - raise SetupOVSConfigException( - "Bridge {} Port {}: NIC {} already used in another " - "port".format(bridge_name, port_name, interface) - ) - dpdk_interfaces.append(interface) + +def _check_system_interface( + bridge_name, port_name, interface, system_interfaces +): + """ + Check a system port interface and claim the NIC + :param bridge_name: the port bridge name + :param port_name: the port name + :param interface: the network interface name + :param system_interfaces: the NICs already claimed, appended to by this + call + """ + if not os.path.isdir( + os.path.join("/proc/sys/net/ipv4/conf/", interface) + ): + message = ( + "Bridge {} Port {}: could not find the network " + "interface {}".format(bridge_name, port_name, interface) + ) + if helpers.dry_run: + logging.error(message) else: - if not os.path.isdir( - os.path.join("/proc/sys/net/ipv4/conf/", interface) - ): - if helpers.dry_run: - logging.error( - "Bridge {} Port {}: could not find the network " - "interface {}".format( - bridge_name, port_name, interface - ) - ) - else: - raise SetupOVSConfigException( - "Bridge {} Port {}: could not find the network " - "interface {}".format( - bridge_name, port_name, interface - ) - ) - if interface in system_interfaces: - raise SetupOVSConfigException( - "Bridge {} Port {}: {} already used in another" - " port".format(bridge_name, port_name, interface) - ) - system_interfaces.append(interface) - else: + raise SetupOVSConfigException(message) + if interface in system_interfaces: + raise SetupOVSConfigException( + "Bridge {} Port {}: {} already used in another" + " port".format(bridge_name, port_name, interface) + ) + system_interfaces.append(interface) + + +def _check_interface( + bridge_name, port_name, port_type, port, dpdk_interfaces, + system_interfaces +): + """ + Check the interface attribute, which only the dpdk and system types use + :param bridge_name: the port bridge name + :param port_name: the port name + :param port_type: the port type + :param port: the port configuration + :param dpdk_interfaces: the DPDK NICs already claimed + :param system_interfaces: the system NICs already claimed + """ + if port_type not in ("dpdk", "system"): if "interface" in port: logging.warning( "Bridge {} Port {}: attribute interface is ignored when " " type is not system nor dpdk".format(bridge_name, port_name) ) + return + if "interface" not in port: + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute interface is required with " + "type {}".format(bridge_name, port_name, port_type) + ) + interface = port["interface"] + if port_type == "dpdk": + _check_dpdk_interface( + bridge_name, port_name, interface, dpdk_interfaces + ) + else: + _check_system_interface( + bridge_name, port_name, interface, system_interfaces + ) + + +def _check_vxlan_attributes(bridge_name, port_name, port_type, port): + """ + Check the attributes a vxlan port requires, and warn about them on any + other type + :param bridge_name: the port bridge name + :param port_name: the port name + :param port_type: the port type + :param port: the port configuration + """ + if port_type == "vxlan": + for attribute in ("key", "remote_ip"): + if attribute not in port: + raise SetupOVSConfigException( + "Bridge {} Port {}: {} must be set if type is " + "vxlan".format(bridge_name, port_name, attribute) + ) + return for attribute in ("key", "remote_ip", "remote_port"): if attribute in port: - if port["type"] == "vxlan": - if attribute != "remote_port" and attribute not in port: - raise SetupOVSConfigException( - "Bridge {} Port {}: {} must be set if type is " - "vxlan".format(bridge_name, port_name, attribute) - ) - else: - logging.warning( - "Bridge {} Port {}: attribute {} is ignored" - " when type is not vxlan".format( - bridge_name, port_name, attribute - ) + logging.warning( + "Bridge {} Port {}: attribute {} is ignored" + " when type is not vxlan".format( + bridge_name, port_name, attribute ) - if "vlan" in port: - _attribute_is_a_port("tag", port["tag"], bridge_name, port_name) + ) + + +def _check_vlan_attributes(bridge_name, port_name, port): + """ + Check the tag, trunks and vlan_mode attributes + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ + if "tag" in port: + _attribute_is_a_vlan_tag("tag", port["tag"], bridge_name, port_name) if "trunks" in port: trunks = ( - [port["trunks"]] if isinstance(port["trunks"], int) else port["trunks"] + [port["trunks"]] + if isinstance(port["trunks"], int) + else port["trunks"] ) if not isinstance(trunks, list): raise SetupOVSConfigException( @@ -291,83 +404,161 @@ def _check_port_configuration(bridge_name, port): " an integer list".format(bridge_name, port_name) ) for trunk in trunks: - _attribute_is_a_port("trunks", trunk, bridge_name, port_name) - if "vlan_mode" in port and port["vlan_mode"] not in ( - "access", - "native-tagged", - "native-untagged", - "trunk", - ): + _attribute_is_a_vlan_tag("trunks", trunk, bridge_name, port_name) + if "vlan_mode" in port and port["vlan_mode"] not in VLAN_MODES: raise SetupOVSConfigException( "Bridge {} Port {}: Bad vlan_mode value: {}".format( bridge_name, port_name, port["vlan_mode"] ) ) - for attribute in ( - "ingress_policing_rate", - "ingress_policing_burst", - ): - if attribute in port: - if not isinstance(port[attribute], int): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} must be an " - "integer".format(bridge_name, port_name, attribute) - ) + + +def _check_integer_attributes(bridge_name, port_name, port): + """ + Check the policing attributes and remote_port + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ + for attribute in ("ingress_policing_rate", "ingress_policing_burst"): + if attribute in port and not isinstance(port[attribute], int): + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute {} must be an " + "integer".format(bridge_name, port_name, attribute) + ) if "remote_port" in port: _attribute_is_a_port( "remote_port", port["remote_port"], bridge_name, port_name ) + + +def _check_string_attributes(bridge_name, port_name, port): + """ + Check the attributes which must be plain strings + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ for attribute in ("key", "remote_ip", "hook_file"): - if attribute in port: - if not isinstance(port[attribute], str): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} must be a " - "string".format(bridge_name, port_name, attribute) - ) - if attribute == "remote_ip": - _attribute_is_an_ipv4( - attribute, port[attribute], bridge_name, port_name - ) - for attribute in ("other_config", "ip"): - if attribute in port: - attribute_value = ( - [port[attribute]] - if isinstance(port[attribute], str) - else port[attribute] + if attribute not in port: + continue + if not isinstance(port[attribute], str): + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute {} must be a " + "string".format(bridge_name, port_name, attribute) ) - if not isinstance(attribute_value, list): + if attribute == "remote_ip": + _attribute_is_an_ipv4( + attribute, port[attribute], bridge_name, port_name + ) + + +def _check_ip_element(bridge_name, port_name, port, attribute, element): + """ + Check one element of the ip attribute + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + :param attribute: the attribute name + :param element: the element to check + """ + _attribute_is_an_ipv4(attribute, element, bridge_name, port_name) + if port["type"] not in MAC_CAPABLE_TYPES: + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute {} only works if" + " interface is tap or" + " dpdkvhostuserclient".format(bridge_name, port_name, attribute) + ) + + +def _as_string_list(bridge_name, port_name, attribute, port): + """ + Read an attribute accepting a string or a string list, as a list + :param bridge_name: the port bridge name + :param port_name: the port name + :param attribute: the attribute name + :param port: the port configuration + :return: the attribute value as a list + """ + attribute_value = ( + [port[attribute]] + if isinstance(port[attribute], str) + else port[attribute] + ) + if not isinstance(attribute_value, list): + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute {} must be a string or " + "a string list".format(bridge_name, port_name, attribute) + ) + return attribute_value + + +def _check_list_attributes(bridge_name, port_name, port): + """ + Check the attributes accepting a string or a string list + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ + if "other_config" in port: + for element in _as_string_list( + bridge_name, port_name, "other_config", port + ): + if not isinstance(element, str): raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} must be a string or " - "a string list".format(bridge_name, port_name, attribute) - ) - for element in attribute_value: - if attribute == "other_config": - if not isinstance(element, str): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} must be a string" - " or a string list".format( - bridge_name, port_name, attribute - ) - ) - else: - _attribute_is_an_ipv4( - attribute, element, bridge_name, port_name + "Bridge {} Port {}: attribute {} must be a string" + " or a string list".format( + bridge_name, port_name, "other_config" ) - if port["type"] not in ("tap", "dpdkvhostuserclient"): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} only works if" - " interface is tap or" - " dpdkvhostuserclient".format( - bridge_name, port_name, attribute - ) - ) - if "mac" in port: - _attribute_is_a_mac("mac", port["mac"], bridge_name, port_name) - if port["type"] not in ("tap", "dpdkvhostuserclient"): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute mac only works if" - " interface is tap or" - " dpdkvhostuserclient".format( - bridge_name, port_name, attribute ) - ) + if "ip" in port: + for element in _as_string_list(bridge_name, port_name, "ip", port): + _check_ip_element(bridge_name, port_name, port, "ip", element) + + +def _check_mac_attribute(bridge_name, port_name, port): + """ + Check the mac attribute + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ + if "mac" not in port: + return + _attribute_is_a_mac("mac", port["mac"], bridge_name, port_name) + if port["type"] not in MAC_CAPABLE_TYPES: + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute mac only works if" + " interface is tap or" + " dpdkvhostuserclient".format(bridge_name, port_name) + ) + + +def _check_port_configuration( + bridge_name, port, dpdk_interfaces, system_interfaces +): + """ + Helper method for _configuration_check which checks the port + configuration + :param bridge_name: the bridge name in which the port take from + :param port: the port configuration + :param dpdk_interfaces: the DPDK NICs already claimed by another port, + appended to by this call + :param system_interfaces: the system NICs already claimed by another + port, appended to by this call + """ + port_name, port_type = _check_name_and_type(bridge_name, port) + _check_interface( + bridge_name, + port_name, + port_type, + port, + dpdk_interfaces, + system_interfaces, + ) + _check_vxlan_attributes(bridge_name, port_name, port_type, port) + _check_vlan_attributes(bridge_name, port_name, port) + _check_integer_attributes(bridge_name, port_name, port) + _check_string_attributes(bridge_name, port_name, port) + _check_list_attributes(bridge_name, port_name, port) + _check_mac_attribute(bridge_name, port_name, port) diff --git a/setup_ovs/helpers.py b/setup_ovs/helpers.py index 2d39215..3ecc7c6 100644 --- a/setup_ovs/helpers.py +++ b/setup_ovs/helpers.py @@ -48,13 +48,13 @@ def run_command(*cmd_args, **kargs): if not dry_run: if "check" not in kargs: kargs["check"] = True - if ( - logging.getLogger().getEffectiveLevel() != logging.DEBUG - and "stdout" not in kargs - and ( - "capture_output" not in kargs - or not kargs["capture_output"] - ) - ): - kargs["stdout"] = subprocess.DEVNULL - return subprocess.run(cmd_args, **kargs) + if ( + logging.getLogger().getEffectiveLevel() != logging.DEBUG + and "stdout" not in kargs + and ( + "capture_output" not in kargs + or not kargs["capture_output"] + ) + ): + kargs["stdout"] = subprocess.DEVNULL + return subprocess.run(cmd_args, **kargs) diff --git a/setup_ovs/ovs.py b/setup_ovs/ovs.py index 791f431..715cc15 100644 --- a/setup_ovs/ovs.py +++ b/setup_ovs/ovs.py @@ -209,9 +209,13 @@ def _create_bridges(config, dpdk_bridges): if "tag" in port: cmd_args.append("tag={}".format(port["tag"])) if "trunks" in port: + trunks = ( + [port["trunks"]] + if isinstance(port["trunks"], int) + else port["trunks"] + ) cmd_args.append( - "trunks=" - + ",".join([str(tag) for tag in port["trunks"]]) + "trunks=" + ",".join([str(tag) for tag in trunks]) ) if port_type not in ("tap", "system"): cmd_args += [ @@ -241,7 +245,9 @@ def _create_bridges(config, dpdk_bridges): ] if "remote_port" in port: cmd_args.append( - "options:remote_port=" + port["remote_port"] + "options:remote_port={}".format( + port["remote_port"] + ) ) if "external-ids" in port: diff --git a/tests/test_check.py b/tests/test_check.py index 8de7af2..25040ea 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -165,13 +165,25 @@ def test_rejects_unknown_type(self): @pytest.mark.parametrize( "port_type", - ["internal", "tap", "dpdkvhostuserclient", "vxlan"], + ["internal", "tap", "dpdkvhostuserclient"], ) def test_accepts_types_needing_no_interface(self, port_type): check.configuration_check( bridge_with_port({"name": "p0", "type": port_type}) ) + def test_accepts_a_complete_vxlan_port(self): + check.configuration_check( + bridge_with_port( + { + "name": "p0", + "type": "vxlan", + "key": "42", + "remote_ip": "10.0.0.1", + } + ) + ) + def test_warns_when_interface_is_ignored(self, caplog): check.configuration_check( bridge_with_port( @@ -321,16 +333,29 @@ def test_rejects_out_of_range_trunk(self, value): assert_rejects(config, "range 0 to") - @pytest.mark.xfail( - strict=True, - reason="the tag range check is guarded by 'vlan' in port while the " - "attribute consumed by ovs._create_bridges is 'tag', so tag is never " - "validated", - ) - def test_rejects_out_of_range_tag(self): - config = bridge_with_port({"name": "p0", "type": "tap", "tag": 9999}) + @pytest.mark.parametrize("value", [-1, 4096, 9999]) + def test_rejects_out_of_range_tag(self, value): + config = bridge_with_port({"name": "p0", "type": "tap", "tag": value}) - assert_rejects(config, "range 0 to") + assert_rejects(config, "range 0 to 4,095") + + def test_rejects_non_integer_tag(self): + config = bridge_with_port({"name": "p0", "type": "tap", "tag": "10"}) + + assert_rejects(config, "must be an integer") + + @pytest.mark.parametrize("value", [0, 10, 4095]) + def test_accepts_in_range_tag(self, value): + check.configuration_check( + bridge_with_port({"name": "p0", "type": "tap", "tag": value}) + ) + + def test_vlan_without_tag_is_not_a_crash(self): + # "vlan" is not consumed by ovs._create_bridges. It used to gate the + # tag check and raised KeyError when tag was absent. + check.configuration_check( + bridge_with_port({"name": "p0", "type": "tap", "vlan": 10}) + ) class TestPolicingAndVxlan: @@ -367,24 +392,31 @@ def test_accepts_complete_vxlan_port(self): def test_rejects_non_integer_remote_port(self): config = bridge_with_port( - {"name": "p0", "type": "vxlan", "remote_port": "4789"} + { + "name": "p0", + "type": "vxlan", + "key": "42", + "remote_ip": "10.0.0.1", + "remote_port": "4789", + } ) assert_rejects(config, "must be an integer") - def test_rejects_negative_remote_port(self): + @pytest.mark.parametrize("value", [-1, 65536]) + def test_rejects_out_of_range_remote_port(self, value): config = bridge_with_port( - {"name": "p0", "type": "vxlan", "remote_port": -1} + { + "name": "p0", + "type": "vxlan", + "key": "42", + "remote_ip": "10.0.0.1", + "remote_port": value, + } ) - assert_rejects(config, "range 0 to") + assert_rejects(config, "range 0 to 65,535") - @pytest.mark.xfail( - strict=True, - reason="_attribute_is_a_port enforces the VLAN tag range 0-4095 on " - "remote_port too, so the IANA VXLAN port 4789 is refused. A TCP/UDP " - "port goes up to 65535", - ) def test_accepts_the_iana_vxlan_port(self): check.configuration_check( bridge_with_port( @@ -399,13 +431,20 @@ def test_accepts_the_iana_vxlan_port(self): ) def test_rejects_non_string_key(self): - config = bridge_with_port({"name": "p0", "type": "vxlan", "key": 42}) + config = bridge_with_port( + { + "name": "p0", + "type": "vxlan", + "key": 42, + "remote_ip": "10.0.0.1", + } + ) assert_rejects(config, "must be a string") def test_rejects_malformed_remote_ip(self): config = bridge_with_port( - {"name": "p0", "type": "vxlan", "remote_ip": "10.0.0"} + {"name": "p0", "type": "vxlan", "key": "42", "remote_ip": "10.0.0"} ) assert_rejects(config, "IPv4 address") @@ -424,17 +463,25 @@ def test_rejects_non_string_hook_file(self): assert_rejects(config, "must be a string") - @pytest.mark.xfail( - strict=True, - reason="the 'must be set if type is vxlan' branch is unreachable: it " - "sits inside 'if attribute in port' and then tests " - "'attribute not in port'. ovs._create_bridges later raises KeyError " - "on such a config", - ) def test_rejects_vxlan_port_without_key_nor_remote_ip(self): config = bridge_with_port({"name": "p0", "type": "vxlan"}) - assert_rejects(config, "vxlan") + assert_rejects(config, "must be set if type is vxlan") + + @pytest.mark.parametrize("missing", ["key", "remote_ip"]) + def test_rejects_vxlan_port_missing_one_attribute(self, missing): + port = { + "name": "p0", + "type": "vxlan", + "key": "42", + "remote_ip": "10.0.0.1", + } + del port[missing] + + assert_rejects( + bridge_with_port(port), + "{} must be set if type is vxlan".format(missing), + ) class TestIpAndMac: @@ -517,12 +564,6 @@ def test_mac_rejection_message_names_the_mac_attribute(self): class TestDuplicateInterfaces: - @pytest.mark.xfail( - strict=True, - reason="dpdk_interfaces/system_interfaces are locals of " - "_check_port_configuration, which runs once per port, so the " - "duplicate-NIC guard can never fire", - ) def test_rejects_the_same_dpdk_nic_on_two_ports(self, run_command): config = { "bridges": [ @@ -545,3 +586,64 @@ def test_rejects_the_same_dpdk_nic_on_two_ports(self, run_command): } assert_rejects(config, "already used") + + def test_rejects_the_same_dpdk_nic_on_two_bridges(self, run_command): + # A NIC is claimed by a single port anywhere in the configuration, + # so the guard spans every bridge and not only the current one. + port = {"name": "p0", "type": "dpdk", "interface": "0000:3b:00.0"} + config = { + "bridges": [ + {"name": "br0", "ports": [dict(port, name="p0")]}, + {"name": "br1", "ports": [dict(port, name="p1")]}, + ] + } + + assert_rejects(config, "already used") + + def test_rejects_the_same_system_nic_on_two_ports( + self, existing_interfaces + ): + port = {"name": "p0", "type": "system", "interface": "eth0"} + config = { + "bridges": [ + { + "name": "br0", + "ports": [dict(port, name="p0"), dict(port, name="p1")], + } + ] + } + + assert_rejects(config, "already used") + + def test_accepts_distinct_nics(self, run_command, existing_interfaces): + check.configuration_check( + { + "bridges": [ + { + "name": "br0", + "ports": [ + { + "name": "p0", + "type": "dpdk", + "interface": "0000:3b:00.0", + }, + { + "name": "p1", + "type": "dpdk", + "interface": "0000:3b:00.1", + }, + { + "name": "p2", + "type": "system", + "interface": "eth0", + }, + { + "name": "p3", + "type": "system", + "interface": "eth1", + }, + ], + } + ] + } + ) diff --git a/tests/test_helpers.py b/tests/test_helpers.py index f821214..72d64fe 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -177,12 +177,6 @@ def fake_run(cmd_args, **kwargs): with pytest.raises(subprocess.CalledProcessError): helpers.run_command("/bin/false") - @pytest.mark.xfail( - strict=True, - reason="run_command returns without running anything when the caller " - "passes check explicitly: the subprocess.run call sits inside the " - "'if \"check\" not in kargs' branch", - ) def test_explicit_check_still_runs_the_command(self, monkeypatch): recorded = {} monkeypatch.setattr( @@ -194,6 +188,19 @@ def test_explicit_check_still_runs_the_command(self, monkeypatch): helpers.run_command("/bin/false", check=False) assert recorded["cmd_args"] == ("/bin/false",) + assert recorded["kwargs"]["check"] is False + + def test_explicit_check_still_silences_stdout(self, monkeypatch): + recorded = {} + monkeypatch.setattr( + subprocess, + "run", + lambda a, **k: recorded.update(cmd_args=a, kwargs=k), + ) + + helpers.run_command("/bin/true", check=False) + + assert recorded["kwargs"]["stdout"] == subprocess.DEVNULL class TestMatchers: diff --git a/tests/test_ovs.py b/tests/test_ovs.py index 5452fe7..43e0c4e 100644 --- a/tests/test_ovs.py +++ b/tests/test_ovs.py @@ -192,7 +192,7 @@ def test_vxlan_remote_port_is_optional(self, run_command): "type": "vxlan", "remote_ip": "10.0.0.1", "key": "42", - "remote_port": "4789", + "remote_port": 4789, } ) ) @@ -205,6 +205,7 @@ def test_vxlan_remote_port_is_optional(self, run_command): ("vlan_mode", "access", "vlan_mode=access"), ("tag", 10, "tag=10"), ("trunks", [1, 2], "trunks=1,2"), + ("trunks", 5, "trunks=5"), ("ofport_request", 7, "ofport_request=7"), ], )