Merge branch 't4203-dhcp' into current

* t4203-dhcp:
  smoketest: dhcp: T4203: move testcase to base class
  static: T4203: obey interface dhcp default route distance
  interface: T4203: prevent DHCP client restart if not necessary

10 files changed, 136 insertions, 50 deletions

diff --git a/data/templates/frr/staticd.frr.tmpl b/data/templates/frr/staticd.frr.tmpl
index bfe959c1d..5d833228a 100644
--- a/data/templates/frr/staticd.frr.tmpl
+++ b/data/templates/frr/staticd.frr.tmpl
@@ -17,10 +17,10 @@ vrf {{ vrf }}
 {% endif %}
 {# IPv4 default routes from DHCP interfaces #}
 {% if dhcp is defined and dhcp is not none %}
-{%   for interface in dhcp %}
+{%   for interface, interface_config in dhcp.items() %}
 {%     set next_hop = interface | get_dhcp_router %}
 {%     if next_hop is defined and next_hop is not none %}
-{{ ip_prefix }} route 0.0.0.0/0 {{ next_hop }} {{ interface }} tag 210 210
+{{ ip_prefix }} route 0.0.0.0/0 {{ next_hop }} {{ interface }} tag 210 {{ interface_config.distance }}
 {%     endif %}
 {%   endfor %}
 {% endif %}
diff --git a/interface-definitions/include/interface/dhcp-options.xml.i b/interface-definitions/include/interface/dhcp-options.xml.i
index b65b0802a..f62b06640 100644
--- a/interface-definitions/include/interface/dhcp-options.xml.i
+++ b/interface-definitions/include/interface/dhcp-options.xml.i
@@ -30,12 +30,13 @@
         <help>Distance for the default route from DHCP server</help>
         <valueHelp>
           <format>u32:1-255</format>
-          <description>Distance for the default route from DHCP server (default 210)</description>
+          <description>Distance for the default route from DHCP server (default: 210)</description>
         </valueHelp>
         <constraint>
           <validator name="numeric" argument="--range 1-255"/>
         </constraint>
       </properties>
+      <defaultValue>210</defaultValue>
     </leafNode>
     <leafNode name="reject">
       <properties>
diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py
index e7f515ea9..f2ec93520 100644
--- a/python/vyos/configdict.py
+++ b/python/vyos/configdict.py
@@ -319,34 +319,42 @@ def is_source_interface(conf, interface, intftype=None):
 def get_dhcp_interfaces(conf, vrf=None):
     """ Common helper functions to retrieve all interfaces from current CLI
     sessions that have DHCP configured. """
-    dhcp_interfaces = []
+    dhcp_interfaces = {}
     dict = conf.get_config_dict(['interfaces'], get_first_key=True)
     if not dict:
         return dhcp_interfaces
 
     def check_dhcp(config, ifname):
-        out = []
+        tmp = {}
         if 'address' in config and 'dhcp' in config['address']:
+            options = {}
+            if 'dhcp_options' in config and 'default_route_distance' in config['dhcp_options']:
+                options.update({'distance' : config['dhcp_options']['default_route_distance']})
             if 'vrf' in config:
-                if vrf is config['vrf']: out.append(ifname)
-            else: out.append(ifname)
-        return out
+                if vrf is config['vrf']: tmp.update({ifname : options})
+            else: tmp.update({ifname : options})
+        return tmp
 
     for section, interface in dict.items():
-        for ifname, ifconfig in interface.items():
+        for ifname in interface:
+            # we already have a dict representation of the config from get_config_dict(),
+            # but with the extended information from get_interface_dict() we also
+            # get the DHCP client default-route-distance default option if not specified.
+            ifconfig = get_interface_dict(conf, ['interfaces', section], ifname)
+
             tmp = check_dhcp(ifconfig, ifname)
-            dhcp_interfaces.extend(tmp)
+            dhcp_interfaces.update(tmp)
             # check per VLAN interfaces
             for vif, vif_config in ifconfig.get('vif', {}).items():
                 tmp = check_dhcp(vif_config, f'{ifname}.{vif}')
-                dhcp_interfaces.extend(tmp)
+                dhcp_interfaces.update(tmp)
             # check QinQ VLAN interfaces
             for vif_s, vif_s_config in ifconfig.get('vif-s', {}).items():
                 tmp = check_dhcp(vif_s_config, f'{ifname}.{vif_s}')
-                dhcp_interfaces.extend(tmp)
+                dhcp_interfaces.update(tmp)
                 for vif_c, vif_c_config in vif_s_config.get('vif-c', {}).items():
                     tmp = check_dhcp(vif_c_config, f'{ifname}.{vif_s}.{vif_c}')
-                    dhcp_interfaces.extend(tmp)
+                    dhcp_interfaces.update(tmp)
 
     return dhcp_interfaces
 
@@ -405,6 +413,12 @@ def get_interface_dict(config, base, ifname=''):
     if 'deleted' not in dict:
         dict = dict_merge(default_values, dict)
 
+        # If interface does not request an IPv4 DHCP address there is no need
+        # to keep the dhcp-options key
+        if 'address' not in dict or 'dhcp' not in dict['address']:
+            if 'dhcp_options' in dict:
+                del dict['dhcp_options']
+
     # XXX: T2665: blend in proper DHCPv6-PD default values
     dict = T2665_set_dhcpv6pd_defaults(dict)
 
@@ -423,6 +437,15 @@ def get_interface_dict(config, base, ifname=''):
     bond = is_member(config, ifname, 'bonding')
     if bond: dict.update({'is_bond_member' : bond})
 
+    # Check if any DHCP options changed which require a client restat
+    for leaf_node in ['client-id', 'default-route-distance', 'host-name',
+                 'no-default-route', 'vendor-class-id']:
+        dhcp = leaf_node_changed(config, ['dhcp-options', leaf_node])
+        if dhcp:
+            dict.update({'dhcp_options_old' : dhcp})
+            # one option is suffiecient to set 'dhcp_options_old' key
+            break
+
     # Some interfaces come with a source_interface which must also not be part
     # of any other bond or bridge interface as it is exclusivly assigned as the
     # Kernels "lower" interface to this new "virtual/upper" interface.
@@ -466,10 +489,25 @@ def get_interface_dict(config, base, ifname=''):
             # XXX: T2665: blend in proper DHCPv6-PD default values
             dict['vif'][vif] = T2665_set_dhcpv6pd_defaults(dict['vif'][vif])
 
+            # If interface does not request an IPv4 DHCP address there is no need
+            # to keep the dhcp-options key
+            if 'address' not in dict['vif'][vif] or 'dhcp' not in dict['vif'][vif]['address']:
+                if 'dhcp_options' in dict['vif'][vif]:
+                    del dict['vif'][vif]['dhcp_options']
+
         # Check if we are a member of a bridge device
         bridge = is_member(config, f'{ifname}.{vif}', 'bridge')
         if bridge: dict['vif'][vif].update({'is_bridge_member' : bridge})
 
+        # Check if any DHCP options changed which require a client restat
+        for leaf_node in ['client-id', 'default-route-distance', 'host-name',
+                     'no-default-route', 'vendor-class-id']:
+            dhcp = leaf_node_changed(config, ['vif', vif, 'dhcp-options', leaf_node])
+            if dhcp:
+                dict['vif'][vif].update({'dhcp_options_old' : dhcp})
+                # one option is suffiecient to set 'dhcp_options_old' key
+                break
+
     for vif_s, vif_s_config in dict.get('vif_s', {}).items():
         default_vif_s_values = defaults(base + ['vif-s'])
         # XXX: T2665: we only wan't the vif-s defaults - do not care about vif-c
@@ -491,10 +529,26 @@ def get_interface_dict(config, base, ifname=''):
             # XXX: T2665: blend in proper DHCPv6-PD default values
             dict['vif_s'][vif_s] = T2665_set_dhcpv6pd_defaults(dict['vif_s'][vif_s])
 
+            # If interface does not request an IPv4 DHCP address there is no need
+            # to keep the dhcp-options key
+            if 'address' not in dict['vif_s'][vif_s] or 'dhcp' not in \
+                dict['vif_s'][vif_s]['address']:
+                if 'dhcp_options' in dict['vif_s'][vif_s]:
+                    del dict['vif_s'][vif_s]['dhcp_options']
+
         # Check if we are a member of a bridge device
         bridge = is_member(config, f'{ifname}.{vif_s}', 'bridge')
         if bridge: dict['vif_s'][vif_s].update({'is_bridge_member' : bridge})
 
+        # Check if any DHCP options changed which require a client restat
+        for leaf_node in ['client-id', 'default-route-distance', 'host-name',
+                     'no-default-route', 'vendor-class-id']:
+            dhcp = leaf_node_changed(config, ['vif-s', vif_s, 'dhcp-options', leaf_node])
+            if dhcp:
+                dict['vif_s'][vif_s].update({'dhcp_options_old' : dhcp})
+                # one option is suffiecient to set 'dhcp_options_old' key
+                break
+
         for vif_c, vif_c_config in vif_s_config.get('vif_c', {}).items():
             default_vif_c_values = defaults(base + ['vif-s', 'vif-c'])
 
@@ -516,11 +570,28 @@ def get_interface_dict(config, base, ifname=''):
                 dict['vif_s'][vif_s]['vif_c'][vif_c] = T2665_set_dhcpv6pd_defaults(
                     dict['vif_s'][vif_s]['vif_c'][vif_c])
 
+                # If interface does not request an IPv4 DHCP address there is no need
+                # to keep the dhcp-options key
+                if 'address' not in dict['vif_s'][vif_s]['vif_c'][vif_c] or 'dhcp' \
+                    not in dict['vif_s'][vif_s]['vif_c'][vif_c]['address']:
+                    if 'dhcp_options' in dict['vif_s'][vif_s]['vif_c'][vif_c]:
+                        del dict['vif_s'][vif_s]['vif_c'][vif_c]['dhcp_options']
+
             # Check if we are a member of a bridge device
             bridge = is_member(config, f'{ifname}.{vif_s}.{vif_c}', 'bridge')
             if bridge: dict['vif_s'][vif_s]['vif_c'][vif_c].update(
                 {'is_bridge_member' : bridge})
 
+            # Check if any DHCP options changed which require a client restat
+            for leaf_node in ['client-id', 'default-route-distance', 'host-name',
+                         'no-default-route', 'vendor-class-id']:
+                dhcp = leaf_node_changed(config, ['vif-s', vif_s, 'vif-c', vif_c,
+                                                  'dhcp-options', leaf_node])
+                if dhcp:
+                    dict['vif_s'][vif_s]['vif_c'][vif_c].update({'dhcp_options_old' : dhcp})
+                    # one option is suffiecient to set 'dhcp_options_old' key
+                    break
+
     # Check vif, vif-s/vif-c VLAN interfaces for removal
     dict = get_removed_vlans(config, dict)
     return dict
@@ -545,7 +616,6 @@ def get_vlan_ids(interface):
 
     return vlan_ids
 
-
 def get_accel_dict(config, base, chap_secrets):
     """
     Common utility function to retrieve and mangle the Accel-PPP configuration
diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index 91c7f0c33..cf1887bf6 100755
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -1228,12 +1228,11 @@ class Interface(Control):
         options_file = f'{config_base}_{ifname}.options'
         pid_file = f'{config_base}_{ifname}.pid'
         lease_file = f'{config_base}_{ifname}.leases'
-
-        # Stop client with old config files to get the right IF_METRIC.
         systemd_service = f'dhclient@{ifname}.service'
-        if is_systemd_service_active(systemd_service):
-            self._cmd(f'systemctl stop {systemd_service}')
 
+        # 'up' check is mandatory b/c even if the interface is A/D, as soon as
+        # the DHCP client is started the interface will be placed in u/u state.
+        # This is not what we intended to do when disabling an interface.
         if enable and 'disable' not in self._config:
             if dict_search('dhcp_options.host_name', self._config) == None:
                 # read configured system hostname.
@@ -1244,16 +1243,19 @@ class Interface(Control):
                     tmp = {'dhcp_options' : { 'host_name' : hostname}}
                     self._config = dict_merge(tmp, self._config)
 
-            render(options_file, 'dhcp-client/daemon-options.tmpl',
-                   self._config)
-            render(config_file, 'dhcp-client/ipv4.tmpl',
-                   self._config)
+            render(options_file, 'dhcp-client/daemon-options.tmpl', self._config)
+            render(config_file, 'dhcp-client/ipv4.tmpl', self._config)
 
-            # 'up' check is mandatory b/c even if the interface is A/D, as soon as
-            # the DHCP client is started the interface will be placed in u/u state.
-            # This is not what we intended to do when disabling an interface.
-            return self._cmd(f'systemctl restart {systemd_service}')
+            # When the DHCP client is restarted a brief outage will occur, as
+            # the old lease is released a new one is acquired (T4203). We will
+            # only restart DHCP client if it's option changed, or if it's not
+            # running, but it should be running (e.g. on system startup)
+            if 'dhcp_options_old' in self._config or not is_systemd_service_active(systemd_service):
+                return self._cmd(f'systemctl restart {systemd_service}')
+            return None
         else:
+            if is_systemd_service_active(systemd_service):
+                self._cmd(f'systemctl stop {systemd_service}')
             # cleanup old config files
             for file in [config_file, options_file, pid_file, lease_file]:
                 if os.path.isfile(file):
diff --git a/smoketest/scripts/cli/base_interfaces_test.py b/smoketest/scripts/cli/base_interfaces_test.py
index 9de961249..013c78837 100644
--- a/smoketest/scripts/cli/base_interfaces_test.py
+++ b/smoketest/scripts/cli/base_interfaces_test.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2019-2021 VyOS maintainers and contributors
+# Copyright (C) 2019-2022 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -56,6 +56,7 @@ def is_mirrored_to(interface, mirror_if, qdisc):
 
 class BasicInterfaceTest:
     class TestCase(VyOSUnitTestSHIM.TestCase):
+        _test_dhcp = False
         _test_ip = False
         _test_mtu = False
         _test_vlan = False
@@ -96,6 +97,31 @@ class BasicInterfaceTest:
             for intf in self._interfaces:
                 self.assertNotIn(intf, interfaces())
 
+            # No daemon that was started during a test should remain running
+            for daemon in ['dhcp6c', 'dhclient']:
+                self.assertFalse(process_named_running(daemon))
+
+        def test_dhcp_disable_interface(self):
+            if not self._test_dhcp:
+                self.skipTest('not supported')
+
+            # When interface is configured as admin down, it must be admin down
+            # even when dhcpc starts on the given interface
+            for interface in self._interfaces:
+                self.cli_set(self._base_path + [interface, 'disable'])
+
+                # Also enable DHCP (ISC DHCP always places interface in admin up
+                # state so we check that we do not start DHCP client.
+                # https://phabricator.vyos.net/T2767
+                self.cli_set(self._base_path + [interface, 'address', 'dhcp'])
+
+            self.cli_commit()
+
+            # Validate interface state
+            for interface in self._interfaces:
+                flags = read_file(f'/sys/class/net/{interface}/flags')
+                self.assertEqual(int(flags, 16) & 1, 0)
+
         def test_span_mirror(self):
             if not self._mirror_interfaces:
                 self.skipTest('not supported')
diff --git a/smoketest/scripts/cli/test_interfaces_bonding.py b/smoketest/scripts/cli/test_interfaces_bonding.py
index 1d9a887bd..4f2fe979a 100755
--- a/smoketest/scripts/cli/test_interfaces_bonding.py
+++ b/smoketest/scripts/cli/test_interfaces_bonding.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2020 VyOS maintainers and contributors
+# Copyright (C) 2020-2022 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -28,6 +28,7 @@ from vyos.util import read_file
 class BondingInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
+        cls._test_dhcp = True
         cls._test_ip = True
         cls._test_ipv6 = True
         cls._test_ipv6_pd = True
diff --git a/smoketest/scripts/cli/test_interfaces_bridge.py b/smoketest/scripts/cli/test_interfaces_bridge.py
index 4f7e03298..f2e111425 100755
--- a/smoketest/scripts/cli/test_interfaces_bridge.py
+++ b/smoketest/scripts/cli/test_interfaces_bridge.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2020-2021 VyOS maintainers and contributors
+# Copyright (C) 2020-2022 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -31,6 +31,7 @@ from vyos.validate import is_intf_addr_assigned
 class BridgeInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
+        cls._test_dhcp = True
         cls._test_ip = True
         cls._test_ipv6 = True
         cls._test_ipv6_pd = True
diff --git a/smoketest/scripts/cli/test_interfaces_ethernet.py b/smoketest/scripts/cli/test_interfaces_ethernet.py
index ae3d5ed96..ee7649af8 100755
--- a/smoketest/scripts/cli/test_interfaces_ethernet.py
+++ b/smoketest/scripts/cli/test_interfaces_ethernet.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2020-2021 VyOS maintainers and contributors
+# Copyright (C) 2020-2022 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -102,6 +102,7 @@ def get_certificate_count(interface, cert_type):
 class EthernetInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
+        cls._test_dhcp = True
         cls._test_ip = True
         cls._test_ipv6 = True
         cls._test_ipv6_pd = True
@@ -146,24 +147,6 @@ class EthernetInterfaceTest(BasicInterfaceTest.TestCase):
 
         self.cli_commit()
 
-    def test_dhcp_disable_interface(self):
-        # When interface is configured as admin down, it must be admin down
-        # even when dhcpc starts on the given interface
-        for interface in self._interfaces:
-            self.cli_set(self._base_path + [interface, 'disable'])
-
-            # Also enable DHCP (ISC DHCP always places interface in admin up
-            # state so we check that we do not start DHCP client.
-            # https://phabricator.vyos.net/T2767
-            self.cli_set(self._base_path + [interface, 'address', 'dhcp'])
-
-        self.cli_commit()
-
-        # Validate interface state
-        for interface in self._interfaces:
-            flags = read_file(f'/sys/class/net/{interface}/flags')
-            self.assertEqual(int(flags, 16) & 1, 0)
-
     def test_offloading_rps(self):
         # enable RPS on all available CPUs, RPS works woth a CPU bitmask,
         # where each bit represents a CPU (core/thread). The formula below
diff --git a/smoketest/scripts/cli/test_interfaces_macsec.py b/smoketest/scripts/cli/test_interfaces_macsec.py
index e4280a5b7..5b10bfa44 100755
--- a/smoketest/scripts/cli/test_interfaces_macsec.py
+++ b/smoketest/scripts/cli/test_interfaces_macsec.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2020-2021 VyOS maintainers and contributors
+# Copyright (C) 2020-2022 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -40,6 +40,7 @@ def get_cipher(interface):
 class MACsecInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
+        cls._test_dhcp = True
         cls._test_ip = True
         cls._test_ipv6 = True
         cls._base_path = ['interfaces', 'macsec']
diff --git a/smoketest/scripts/cli/test_interfaces_pseudo_ethernet.py b/smoketest/scripts/cli/test_interfaces_pseudo_ethernet.py
index ae899cddd..adcadc5eb 100755
--- a/smoketest/scripts/cli/test_interfaces_pseudo_ethernet.py
+++ b/smoketest/scripts/cli/test_interfaces_pseudo_ethernet.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2020-2021 VyOS maintainers and contributors
+# Copyright (C) 2020-2022 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -23,6 +23,7 @@ from base_interfaces_test import BasicInterfaceTest
 class PEthInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
+        cls._test_dhcp = True
         cls._test_ip = True
         cls._test_ipv6 = True
         cls._test_ipv6_pd = True