diff --git a/changelog/51068.fixed.md b/changelog/51068.fixed.md new file mode 100644 index 000000000000..bb14a054589e --- /dev/null +++ b/changelog/51068.fixed.md @@ -0,0 +1 @@ +Made network.managed stop reporting changes when there are none. diff --git a/salt/states/network.py b/salt/states/network.py index 35dacb71c681..1aeb41eb0482 100644 --- a/salt/states/network.py +++ b/salt/states/network.py @@ -621,30 +621,16 @@ def managed(name, enabled=True, **kwargs): # Bring up/shutdown interface try: - # Get Interface current status + # Get Interfaces information interfaces = salt.utils.network.interfaces() - interface_status = False - if name in interfaces: - interface_status = interfaces[name].get("up") + + # Check the interface current status + if name.split(":")[0] in interfaces: + interface_status = interfaces[name.split(":")[0]].get("up") else: - for iface in interfaces: - if "secondary" in interfaces[iface]: - for second in interfaces[iface]["secondary"]: - if second.get("label", "") == name: - interface_status = True - if iface == "lo": - if "inet" in interfaces[iface]: - inet_data = interfaces[iface]["inet"] - if len(inet_data) > 1: - for data in inet_data: - if data.get("label", "") == name: - interface_status = True - if "inet6" in interfaces[iface]: - inet6_data = interfaces[iface]["inet6"] - if len(inet6_data) > 1: - for data in inet6_data: - if data.get("label", "") == name: - interface_status = True + interface_status = False + + # Handle interface as desired if enabled: if "noifupdown" not in kwargs: if interface_status: diff --git a/tests/integration/states/test_network.py b/tests/integration/states/test_network.py deleted file mode 100644 index efc5768e485a..000000000000 --- a/tests/integration/states/test_network.py +++ /dev/null @@ -1,76 +0,0 @@ -""" - :codeauthor: :email: `Justin Anderson ` - - tests.integration.states.network - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -""" - -import pytest - -from tests.support.case import ModuleCase -from tests.support.mixins import SaltReturnAssertsMixin - - -@pytest.mark.destructive_test -class NetworkTest(ModuleCase, SaltReturnAssertsMixin): - """ - Validate network state module - """ - - def setUp(self): - os_family = self.run_function("grains.get", ["os_family"]) - os = self.run_function("grains.get", ["os"]) - if os_family not in ("RedHat", "Debian") or os == "VMware Photon OS": - self.skipTest( - "Network state only supported on RedHat and Debian based systems." - "The network state does not currently work on VMware Photon OS." - ) - if ( - os_family == "RedHat" - and self.run_function("grains.get", ["osmajorrelease"]) >= 10 - ): - self.skipTest( - "Network state doesn't fully support NetworkManager only systems." - ) - - @pytest.mark.slow_test - def test_managed(self): - """ - network.managed - """ - state_key = "network_|-dummy0_|-dummy0_|-managed" - - ret = self.run_function("state.sls", mods="network.managed", test=True) - self.assertEqual( - "Interface dummy0 is set to be added.", ret[state_key]["comment"] - ) - - @pytest.mark.slow_test - def test_routes(self): - """ - network.routes - """ - state_key = "network_|-routes_|-dummy0_|-routes" - expected_changes = "Interface dummy0 routes are set to be added." - - ret = self.run_function("state.sls", mods="network.routes", test=True) - - self.assertEqual( - ret[state_key]["comment"], "Interface dummy0 routes are set to be added." - ) - - @pytest.mark.slow_test - def test_system(self): - """ - network.system - """ - state_key = "network_|-system_|-system_|-system" - - global_settings = self.run_function("ip.get_network_settings") - ret = self.run_function("state.sls", mods="network.system", test=True) - self.assertIn( - "Global network settings are set to be {}".format( - "added" if not global_settings else "updated" - ), - ret[state_key]["comment"], - ) diff --git a/tests/pytests/integration/states/test_network.py b/tests/pytests/integration/states/test_network.py new file mode 100644 index 000000000000..1935999c2fb8 --- /dev/null +++ b/tests/pytests/integration/states/test_network.py @@ -0,0 +1,489 @@ +""" +Tests for the network state +""" + +import logging +import socket + +import distro +import pytest + +log = logging.getLogger(__name__) + +pytestmark = [ + pytest.mark.slow_test, + pytest.mark.destructive_test, +] + + +@pytest.fixture +def context(salt_call_cli): + """ + establishes the context for each test, so each of them execute as an entire separate test. + """ + + # Map dummy interface + dummy_interface = { + "iface_enabled": True, + "iface_name": "salttest0", + "iface_proto": "manual" if "debian" in distro.id() + distro.like() else "none", + "iface_type": "eth", + } + + # take actions for each distro + if "debian" in distro.id() + distro.like(): + # backup config file in debian + salt_call_cli.run( + "cmd.run", cmd="cp -p /etc/network/interfaces /etc/network/interfaces.bkp" + ) + + # check if ifupdown is installed + ifupdown_installed = salt_call_cli.run( + "pkg.info_installed", "ifupdown", failhard="false" + ) + # and install it if needed + if not ifupdown_installed.data: + salt_call_cli.run("pkg.install", "ifupdown") + else: # rhel based + # "network-scripts" is only available in devel on major_version > 9 + if int(distro.major_version()) >= 9: + # hence check if it is enabled + rhel_devel_enabled = salt_call_cli.run("pkg.get_repo", "devel") + # and enable it if needed + if "enabled" in rhel_devel_enabled.data and not int( + rhel_devel_enabled.data["enabled"] + ): + salt_call_cli.run("pkg.mod_repo", "devel", enabled=1) + + # check if network-scripts is installed + networkscripts_installed = salt_call_cli.run( + "pkg.info_installed", "network-scripts" + ) + # and install it if needed + if not networkscripts_installed.data: + salt_call_cli.run("pkg.install", "network-scripts") + + # check if dummy module is loaded + kmod_isLoaded = salt_call_cli.run("kmod.is_loaded", "dummy") + # and load it if needed + if not kmod_isLoaded.data: + salt_call_cli.run("kmod.load", "dummy") + + # setup dummy interface + salt_call_cli.run( + "cmd.run", + cmd="ip link add {0} type dummy; ifdown {0}".format( + dummy_interface["iface_name"] + ), + ) + + # yield dummy interface data + yield dummy_interface + + # teardown dummy interface + salt_call_cli.run( + "cmd.run", cmd="ip link delete {}".format(dummy_interface["iface_name"]) + ) + + # remove module if it was not loaded before + if not kmod_isLoaded.data: + salt_call_cli.run("kmod.remove", "dummy") + + # take actions for each distro + if "debian" in distro.id() + distro.like(): + # remove package if it was not installed before + if not ifupdown_installed.data: + salt_call_cli.run("pkg.purge", "ifupdown") + + # restore OS network config to previous + salt_call_cli.run( + "cmd.run", cmd="mv /etc/network/interfaces.bkp /etc/network/interfaces" + ) + else: # rhel based + # restore OS network config to previous + salt_call_cli.run( + "cmd.run", + cmd="rm /etc/sysconfig/network-scripts/ifcfg-{}".format( + dummy_interface["iface_name"] + ), + ) + + # remove package if it was not installed before + if not networkscripts_installed.data: + salt_call_cli.run("pkg.remove", "network-scripts") + + # for major_version >= 9, disable the repo if it was not enabled at first + if int(distro.major_version()) >= 9: + if "enabled" in rhel_devel_enabled.data and not int( + rhel_devel_enabled.data["enabled"] + ): + salt_call_cli.run("pkg.mod_repo", "devel", enabled=0) + + +@pytest.mark.skip_if_not_root +@pytest.mark.skipif( + "debian" not in distro.id() + distro.like() + and "rhel" not in distro.id() + distro.like() + or ("rhel" in distro.id() + distro.like() and distro.major_version() >= 10), + reason="Network state only supports Debian and/or RHEL based systems <= 9", +) +@pytest.mark.usefixtures("context", "salt_call_cli", "salt_master") +class TestNetwork: + def addInterface( + self, + context, + salt_call_cli, + salt_master, + iname=None, + ienabled=None, + iproto=None, + itype=None, + testFlag=False, + ): + """ + Shortcut to add interface in each test as needed. + """ + # Map default values to context + iname = context["iface_name"] if iname is None else iname + ienabled = context["iface_enabled"] if ienabled is None else ienabled + iproto = context["iface_proto"] if iproto is None else iproto + itype = context["iface_type"] if itype is None else itype + + # Map state content + state_contents = """ + {0}_interface: + network.managed: + - name: {0} + - enabled: {1} + - proto: {2} + - type: {3} + """.format( + iname, ienabled, iproto, itype + ) + + # "Add" state to salt-master base env + state_tempfile = salt_master.state_tree.base.temp_file( + "dummy_interface.sls", state_contents + ) + + # "Get" a run of the state + with state_tempfile: + return salt_call_cli.run("state.apply", "dummy_interface", test=testFlag) + + def test_managed_addInterface0(self, context, salt_call_cli, salt_master): + """ + network.managed add new interface with test flag + """ + # Execute test + ret = self.addInterface( + context=context, + salt_call_cli=salt_call_cli, + salt_master=salt_master, + testFlag=True, + ) + + ## and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is None + assert state_run["comment"] == "Interface {} is set to be added.".format( + context["iface_name"] + ) + assert state_run["changes"] == {} + + def test_managed_addInterface1(self, context, salt_call_cli, salt_master): + """ + network.managed add new interface + """ + # Execute test + ret = self.addInterface( + context=context, salt_call_cli=salt_call_cli, salt_master=salt_master + ) + + ## and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is True + assert state_run["comment"] == "Interface {} added.".format( + context["iface_name"] + ) + assert state_run["changes"]["interface"] == "Added network interface." + + def test_managed_existingInterfaceNoChanges0( + self, context, salt_call_cli, salt_master + ): + """ + network.managed with existing interfaces without changing configs with test flag + """ + # Add test interface + self.addInterface( + context=context, salt_call_cli=salt_call_cli, salt_master=salt_master + ) + + # Execute test + ret = self.addInterface( + context=context, + salt_call_cli=salt_call_cli, + salt_master=salt_master, + testFlag=True, + ) + + # and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is True + assert state_run["comment"] == "Interface {} is up to date.".format( + context["iface_name"] + ) + assert state_run["changes"] == {} + + def test_managed_existingInterfaceNoChanges1( + self, context, salt_call_cli, salt_master + ): + """ + network.managed with existing interfaces without changing configs + """ + # Add test interface + self.addInterface( + context=context, salt_call_cli=salt_call_cli, salt_master=salt_master + ) + + # Execute test + ret = self.addInterface( + context=context, salt_call_cli=salt_call_cli, salt_master=salt_master + ) + + # and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is True + assert state_run["comment"] == "Interface {} is up to date.".format( + context["iface_name"] + ) + assert state_run["changes"] == {} + + def test_managed_existingInterfaceWithChanges0( + self, context, salt_call_cli, salt_master + ): + """ + network.managed with changes to existing interface with test flag + """ + # Add test interface + self.addInterface( + context=context, salt_call_cli=salt_call_cli, salt_master=salt_master + ) + + # Execute test + ret = self.addInterface( + context=context, + salt_call_cli=salt_call_cli, + salt_master=salt_master, + iproto="dhcp", + testFlag=True, + ) + + ## and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is None + assert ( + "Interface {} is set to be updated:".format(context["iface_name"]) + in state_run["comment"] + ) + + def test_managed_existingInterfaceWithChanges1( + self, context, salt_call_cli, salt_master + ): + """ + network.managed with changes to existing interface and try to start it (previously down) + """ + # Add test interface + self.addInterface( + context=context, + salt_call_cli=salt_call_cli, + salt_master=salt_master, + ienabled=False, + ) + + # Map change per OS + temp_iproto = "loopback" if "debian" in distro.id() + distro.like() else "bootp" + + # Execute test + ret = self.addInterface( + context=context, + salt_call_cli=salt_call_cli, + salt_master=salt_master, + iproto=temp_iproto, + ) + + # and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is True + assert ( + "Interface {} updated.".format(context["iface_name"]) + in state_run["comment"] + ) + assert ( + "Interface {} is up".format(context["iface_name"]) + in state_run["changes"]["status"] + ) + + def test_managed_existingInterfaceWithChanges2( + self, context, salt_call_cli, salt_master + ): + """ + network.managed with changes to existing interface and set disabled (previously enabled) + """ + # Add test interface + self.addInterface( + context=context, salt_call_cli=salt_call_cli, salt_master=salt_master + ) + + # Map change per OS + temp_iproto = "loopback" if "debian" in distro.id() + distro.like() else "bootp" + + # Execute test + ret = self.addInterface( + context=context, + salt_call_cli=salt_call_cli, + salt_master=salt_master, + ienabled=False, + iproto=temp_iproto, + ) + + # and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is True + assert ( + "Interface {} updated.".format(context["iface_name"]) + in state_run["comment"] + ) + assert ( + "Interface {} down".format(context["iface_name"]) + in state_run["changes"]["status"] + ) + + def test_managed_existingInterfaceWithChanges3( + self, context, salt_call_cli, salt_master + ): + """ + network.managed with changes to existing interface and try to restart it (previously enabled) + """ + # Add test interface + self.addInterface( + context=context, salt_call_cli=salt_call_cli, salt_master=salt_master + ) + + # Map change per OS + temp_iproto = "loopback" if "debian" in distro.id() + distro.like() else "bootp" + + # Execute test + ret = self.addInterface( + context=context, + salt_call_cli=salt_call_cli, + salt_master=salt_master, + iproto=temp_iproto, + ) + + # and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is True + assert ( + "Interface {} updated.".format(context["iface_name"]) + in state_run["comment"] + ) + assert ( + "Interface {} restart to validate".format(context["iface_name"]) + in state_run["changes"]["status"] + ) + + def test_routes(self, salt_call_cli, salt_master): + """ + network.routes add empty routes + """ + # Set test route name + test_route = "salttest_route" + + # Map state content + state_contents = """ + {0}: + network.routes: + - name: {0} + - routes: [] + """.format( + test_route + ) + + # "Add" state to salt-master base env + state_tempfile = salt_master.state_tree.base.temp_file( + "dummy_route.sls", state_contents + ) + + # "Get" a run of the state + with state_tempfile: + ret = salt_call_cli.run("state.apply", "dummy_route", test=True) + + # and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + if "debian" in distro.id() + distro.like(): + assert state_run["result"] is None + assert ( + state_run["comment"] + == f"Interface {test_route} routes are set to be added." + ) + else: + assert state_run["result"] is True + assert ( + state_run["comment"] == f"Interface {test_route} routes are up to date." + ) + + def test_system(self, salt_call_cli, salt_master): + """ + network.system add/change with test flag + """ + # Map state content + state_contents = """ + test_network_system: + network.system: + - enabled: true + - hostname: {} + """.format( + socket.gethostname() + ) + + # "Add" state to salt-master base env + state_tempfile = salt_master.state_tree.base.temp_file( + "dummy_system.sls", state_contents + ) + + # Get network settings + global_settings = salt_call_cli.run("ip.get_network_settings") + + # "Get" a run of the state + with state_tempfile: + ret = salt_call_cli.run("state.apply", "dummy_system", test=True) + + # and validate results + assert ret.returncode == 0 + assert ret.data + state_run = next(iter(ret.data.values())) + assert state_run["result"] is None + assert ( + "Global network settings are set to be {}".format( + "added" if not global_settings.data else "updated" + ) + in state_run["comment"] + ) diff --git a/tests/pytests/unit/states/test_network.py b/tests/pytests/unit/states/test_network.py index 6ae2530e1254..d432bab26811 100644 --- a/tests/pytests/unit/states/test_network.py +++ b/tests/pytests/unit/states/test_network.py @@ -141,6 +141,31 @@ def test_managed(): ) +def test_managed_alias_no_spurious_change(): + """ + Test that network.managed does not report a spurious status change when + an alias interface (e.g. lo:alias1) is already configured and the parent + interface is up. Regression test for issue #51068. + """ + with patch("salt.states.network.salt.utils.network", MockNetwork()), patch( + "salt.states.network.salt.loader", MockGrains() + ): + dunder_salt = { + "ip.get_interface": MagicMock(return_value="A"), + "ip.build_interface": MagicMock(return_value="A"), + "saltutil.refresh_grains": MagicMock(return_value=True), + } + + with patch.dict(network.__salt__, dunder_salt): + # When the alias is already up-to-date (old == new) and the + # parent interface is up, changes must be empty (no spurious + # "Interface lo:alias1 is up" entry). + ret = network.managed("lo:alias1", type="eth") + assert ret["result"] is True + assert ret["comment"] == "Interface lo:alias1 is up to date." + assert ret["changes"] == {} + + def test_routes(): """ Test to manage network interface static routes.