dhcp-server: T3672: only one failover peer is supported

(cherry picked from commit a8ccf72c222caad8cd7aaca9bca773be39e87f5c)

4 files changed, 97 insertions, 99 deletions

diff --git a/data/templates/dhcp-server/dhcpd.conf.tmpl b/data/templates/dhcp-server/dhcpd.conf.tmpl
index f64192acf..23917b303 100644
--- a/data/templates/dhcp-server/dhcpd.conf.tmpl
+++ b/data/templates/dhcp-server/dhcpd.conf.tmpl
@@ -35,32 +35,25 @@ option wpad-url code 252 = text;
 {%   endfor %}
 
 {% endif %}
-{% if shared_network_name is defined and shared_network_name is not none %}
-{%   for network, network_config in shared_network_name.items() if network_config.disable is not defined %}
-{%     if network_config.subnet is defined and network_config.subnet is not none %}
-{%       for subnet, subnet_config in network_config.subnet.items() %}
-{%         if subnet_config.failover is defined and subnet_config.failover is defined and subnet_config.failover.name is defined and subnet_config.failover.name is not none %}
-# Failover configuration for {{ subnet }}
-failover peer "{{ subnet_config.failover.name }}" {
-{%           if subnet_config.failover.status == 'primary' %}
+{% if failover is defined and failover is not none %}
+{%   set dhcp_failover_name = 'VyOS-DHCP-failover-peer' %}
+# DHCP failover configuration
+failover peer "{{ dhcp_failover_name }}" {
+{%   if failover.status == 'primary' %}
     primary;
     mclt 1800;
     split 128;
-{%           elif subnet_config.failover.status == 'secondary' %}
+{%   elif failover.status == 'secondary' %}
     secondary;
-{%           endif %}
-    address {{ subnet_config.failover.local_address }};
+{%   endif %}
+    address {{ failover.source_address }};
     port 520;
-    peer address {{ subnet_config.failover.peer_address }};
+    peer address {{ failover.remote }};
     peer port 520;
     max-response-delay 30;
     max-unacked-updates 10;
     load balance max seconds 3;
 }
-{%         endif %}
-{%       endfor %}
-{%     endif %}
-{%   endfor %}
 {% endif %}
 {% if listen_address is defined and listen_address is not none %}
 
@@ -182,23 +175,17 @@ shared-network {{ network | replace('_','-') }} {
         }
 {%           endfor %}
 {%         endif %}
-{%         if subnet_config.failover is defined and subnet_config.failover.name is defined and subnet_config.failover.name is not none %}
         pool {
-            failover peer "{{ subnet_config.failover.name }}";
+{%         if subnet_config.enable_failover is defined %}
+            failover peer "{{ dhcp_failover_name }}";
             deny dynamic bootp clients;
+{%         endif %}
 {%           if subnet_config.range is defined and subnet_config.range is not none %}
 {%             for range, range_options in subnet_config.range.items() %}
             range {{ range_options.start }} {{ range_options.stop }};
 {%             endfor %}
 {%           endif %}
         }
-{%         else %}
-{%           if subnet_config.range is defined and subnet_config.range is not none %}
-{%             for range, range_options in subnet_config.range.items() %}
-        range {{ range_options.start }} {{ range_options.stop }};
-{%             endfor %}
-{%           endif %}
-{%         endif %}
     }
 {%       endfor %}
 {%     endif %}
diff --git a/interface-definitions/dhcp-server.xml.in b/interface-definitions/dhcp-server.xml.in
index 3a1eee60e..10384947a 100644
--- a/interface-definitions/dhcp-server.xml.in
+++ b/interface-definitions/dhcp-server.xml.in
@@ -16,6 +16,46 @@
               <valueless/>
             </properties>
           </leafNode>
+          <node name="failover">
+            <properties>
+              <help>DHCP failover configuration</help>
+            </properties>
+            <children>
+              #include <include/source-address-ipv4.xml.i>
+              <leafNode name="remote">
+                <properties>
+                  <help>IPv4 remote address used for connectio</help>
+                  <valueHelp>
+                    <format>ipv4</format>
+                    <description>IPv4 address of failover peer</description>
+                  </valueHelp>
+                  <constraint>
+                    <validator name="ipv4-address"/>
+                  </constraint>
+                </properties>
+              </leafNode>
+              <leafNode name="status">
+                <properties>
+                  <help>Failover hierarchy</help>
+                  <completionHelp>
+                    <list>primary secondary</list>
+                  </completionHelp>
+                  <valueHelp>
+                    <format>primary</format>
+                    <description>Configure this server to be the primary node</description>
+                  </valueHelp>
+                  <valueHelp>
+                    <format>secondary</format>
+                    <description>Configure this server to be the secondary node</description>
+                  </valueHelp>
+                  <constraint>
+                    <regex>^(primary|secondary)$</regex>
+                  </constraint>
+                  <constraintErrorMessage>Invalid DHCP failover peer status</constraintErrorMessage>
+                </properties>
+              </leafNode>
+            </children>
+          </node>
           <leafNode name="global-parameters">
             <properties>
               <help>Additional global parameters for DHCP server. You must
@@ -128,6 +168,12 @@
                     </properties>
                   </leafNode>
                   #include <include/dhcp-server-domain-search.xml.i>
+                  <leafNode name="enable-failover">
+                    <properties>
+                      <help>Enable DHCP failover support for this subnet</help>
+                      <valueless/>
+                    </properties>
+                  </leafNode>
                   <leafNode name="exclude">
                     <properties>
                       <help>IP address to exclude from DHCP lease range</help>
@@ -141,58 +187,6 @@
                       <multi/>
                     </properties>
                   </leafNode>
-                  <node name="failover">
-                    <properties>
-                      <help>DHCP failover parameters</help>
-                    </properties>
-                    <children>
-                      <leafNode name="local-address">
-                        <properties>
-                          <help>IP address for failover peer to connect [REQUIRED]</help>
-                          <valueHelp>
-                            <format>ipv4</format>
-                            <description>IPv4 address to exclude from lease range</description>
-                          </valueHelp>
-                          <constraint>
-                            <validator name="ipv4-address"/>
-                          </constraint>
-                        </properties>
-                      </leafNode>
-                      <leafNode name="name">
-                        <properties>
-                          <help>DHCP failover peer name [REQUIRED]</help>
-                          <constraint>
-                            <regex>[-_a-zA-Z0-9.]+</regex>
-                          </constraint>
-                          <constraintErrorMessage>Invalid failover peer name. May only contain letters, numbers and .-_</constraintErrorMessage>
-                        </properties>
-                      </leafNode>
-                      <leafNode name="peer-address">
-                        <properties>
-                          <help>IP address of failover peer [REQUIRED]</help>
-                          <valueHelp>
-                            <format>ipv4</format>
-                            <description>IPv4 address of failover peer</description>
-                          </valueHelp>
-                          <constraint>
-                            <validator name="ipv4-address"/>
-                          </constraint>
-                        </properties>
-                      </leafNode>
-                      <leafNode name="status">
-                        <properties>
-                          <help>DHCP failover peer status (primary|secondary) [REQUIRED]</help>
-                          <completionHelp>
-                            <list>primary secondary</list>
-                          </completionHelp>
-                          <constraint>
-                            <regex>^(primary|secondary)$</regex>
-                          </constraint>
-                          <constraintErrorMessage>Invalid DHCP failover peer status</constraintErrorMessage>
-                        </properties>
-                      </leafNode>
-                    </children>
-                  </node>
                   <leafNode name="ip-forwarding">
                     <properties>
                       <help>Enable IP forwarding on client</help>
diff --git a/src/conf_mode/dhcp_server.py b/src/conf_mode/dhcp_server.py
index 8d6cef8b7..5b3809017 100755
--- a/src/conf_mode/dhcp_server.py
+++ b/src/conf_mode/dhcp_server.py
@@ -148,9 +148,9 @@ def verify(dhcp):
                           'At least one DHCP shared network must be configured.')
 
     # Inspect shared-network/subnet
-    failover_names = []
     listen_ok = False
     subnets = []
+    failover_ok = False
 
     # A shared-network requires a subnet definition
     for network, network_config in dhcp['shared_network_name'].items():
@@ -159,11 +159,19 @@ def verify(dhcp):
                               'lease subnet must be configured.')
 
         for subnet, subnet_config in network_config['subnet'].items():
+            # All delivered static routes require a next-hop to be set
             if 'static_route' in subnet_config:
                 for route, route_option in subnet_config['static_route'].items():
                     if 'next_hop' not in route_option:
                         raise ConfigError(f'DHCP static-route "{route}" requires router to be defined!')
 
+            # DHCP failover needs at least one subnet that uses it
+            if 'enable_failover' in subnet_config:
+                if 'failover' not in dhcp:
+                    raise ConfigError(f'Can not enable failover for "{subnet}" in "{network}".\n' \
+                                      'Failover is not configured globally!')
+                failover_ok = True
+
             # Check if DHCP address range is inside configured subnet declaration
             if 'range' in subnet_config:
                 networks = []
@@ -192,23 +200,6 @@ def verify(dhcp):
                     tmp = IPRange(range_config['start'], range_config['stop'])
                     networks.append(tmp)
 
-            if 'failover' in subnet_config:
-                for key in ['local_address', 'peer_address', 'name', 'status']:
-                    if key not in subnet_config['failover']:
-                        raise ConfigError(f'Missing DHCP failover parameter "{key}"!')
-
-                # Failover names must be uniquie
-                if subnet_config['failover']['name'] in failover_names:
-                    name = subnet_config['failover']['name']
-                    raise ConfigError(f'DHCP failover names must be unique:\n' \
-                                      f'{name} has already been configured!')
-                failover_names.append(subnet_config['failover']['name'])
-
-                # Failover requires start/stop ranges for pool
-                if 'range' not in subnet_config:
-                    raise ConfigError(f'DHCP failover requires at least one start-stop range to be configured\n'\
-                                      f'within shared-network "{network}, {subnet}" for using failover!')
-
             # Exclude addresses must be in bound
             if 'exclude' in subnet_config:
                 for exclude in subnet_config['exclude']:
@@ -252,6 +243,15 @@ def verify(dhcp):
                     if net.overlaps(net2):
                         raise ConfigError('Conflicting subnet ranges: "{net}" overlaps "{net2}"!')
 
+    if 'failover' in dhcp:
+        if not failover_ok:
+            raise ConfigError('DHCP failover must be enabled for at least one subnet!')
+
+        for key in ['source_address', 'remote', 'status']:
+            if key not in dhcp['failover']:
+                tmp = key.replace('_', '-')
+                raise ConfigError(f'DHCP failover requires "{tmp}" to be specified!')
+
     for address in (dict_search('listen_address', dhcp) or []):
         if is_addr_assigned(address):
             listen_ok = True
diff --git a/src/migration-scripts/dhcp-server/5-to-6 b/src/migration-scripts/dhcp-server/5-to-6
index 7f447ac17..39bbb9f50 100755
--- a/src/migration-scripts/dhcp-server/5-to-6
+++ b/src/migration-scripts/dhcp-server/5-to-6
@@ -29,16 +29,16 @@ file_name = sys.argv[1]
 with open(file_name, 'r') as f:
     config_file = f.read()
 
-base = ['service', 'dhcp-server', 'shared-network-name']
+base = ['service', 'dhcp-server']
 config = ConfigTree(config_file)
 
-if not config.exists(base):
+if not config.exists(base + ['shared-network-name']):
     # Nothing to do
     exit(0)
 
 # Run this for every instance if 'shared-network-name'
-for network in config.list_nodes(base):
-    base_network = base + [network]
+for network in config.list_nodes(base + ['shared-network-name']):
+    base_network = base + ['shared-network-name', network]
 
     if not config.exists(base_network + ['subnet']):
         continue
@@ -60,6 +60,23 @@ for network in config.list_nodes(base):
         if config.exists(base_subnet + ['dns-server']):
             config.rename(base_subnet + ['dns-server'], 'name-server')
 
+
+        # T3672: ISC DHCP server only supports one failover peer
+        if config.exists(base_subnet + ['failover']):
+            # There can only be one failover configuration, if none is present
+            # we add the first one
+            if not config.exists(base + ['failover']):
+                local = config.return_value(base_subnet + ['failover', 'local-address'])
+                remote = config.return_value(base_subnet + ['failover', 'peer-address'])
+                status = config.return_value(base_subnet + ['failover', 'status'])
+
+                config.set(base + ['failover', 'remote'], value=remote)
+                config.set(base + ['failover', 'source-address'], value=local)
+                config.set(base + ['failover', 'status'], value=status)
+
+            config.delete(base_subnet + ['failover'])
+            config.set(base_subnet + ['enable-failover'])
+
 try:
     with open(file_name, 'w') as f:
         f.write(config.to_string())