Merge pull request #2966 from HollyGurza/T6020

vrrp: T6020: vrrp health-check script not applied correctly

4 files changed, 152 insertions, 18 deletions

diff --git a/data/templates/high-availability/keepalived.conf.j2 b/data/templates/high-availability/keepalived.conf.j2
index f34ce64e2..240161748 100644
--- a/data/templates/high-availability/keepalived.conf.j2
+++ b/data/templates/high-availability/keepalived.conf.j2
@@ -33,6 +33,24 @@ global_defs {
     notify_fifo_script /usr/libexec/vyos/system/keepalived-fifo.py
 }
 
+{# Sync group has own health-check scripts T6020 #}
+{% if vrrp.sync_group is vyos_defined %}
+{%     for name, sync_group_config in vrrp.sync_group.items() if sync_group_config.disable is not vyos_defined %}
+{%         if sync_group_config.health_check is vyos_defined %}
+vrrp_script healthcheck_sg_{{ name }} {
+{%             if sync_group_config.health_check.script is vyos_defined %}
+    script "{{ sync_group_config.health_check.script }}"
+{%             elif sync_group_config.health_check.ping is vyos_defined %}
+    script "/usr/bin/ping -c1 {{ sync_group_config.health_check.ping }}"
+{%             endif %}
+    interval {{ sync_group_config.health_check.interval }}
+    fall {{ sync_group_config.health_check.failure_count }}
+    rise 1
+}
+{%         endif %}
+{%     endfor %}
+{% endif %}
+
 {% if vrrp.group is vyos_defined %}
 {%     for name, group_config in vrrp.group.items() if group_config.disable is not vyos_defined %}
 {%         if group_config.health_check is vyos_defined %}
@@ -132,7 +150,8 @@ vrrp_instance {{ name }} {
 {%             endfor %}
     }
 {%         endif %}
-{%         if group_config.health_check is vyos_defined %}
+{# Sync group member can't use own health check script #}
+{%         if group_config.health_check is vyos_defined and group_config._is_sync_group_member is not vyos_defined %}
     track_script {
         healthcheck_{{ name }}
     }
@@ -152,16 +171,12 @@ vrrp_sync_group {{ name }} {
 {%         endif %}
     }
 
-{# Health-check scripts should be in section sync-group if member is part of the sync-group T4081 #}
-{%         if vrrp.group is vyos_defined %}
-{%             for name, group_config in vrrp.group.items() if group_config.disable is not vyos_defined %}
-{%                 if group_config.health_check.script is vyos_defined and name in sync_group_config.member %}
+{%         if sync_group_config.health_check is vyos_defined %}
     track_script {
-        healthcheck_{{ name }}
+        healthcheck_sg_{{ name }}
     }
-{%                 endif %}
-{%             endfor %}
 {%         endif %}
+
 {%         if conntrack_sync_group is vyos_defined(name) %}
 {%             set vyos_helper = "/usr/libexec/vyos/vyos-vrrp-conntracksync.sh" %}
     notify_master "{{ vyos_helper }} master {{ name }}"
diff --git a/interface-definitions/high-availability.xml.in b/interface-definitions/high-availability.xml.in
index aef57f8ae..558404882 100644
--- a/interface-definitions/high-availability.xml.in
+++ b/interface-definitions/high-availability.xml.in
@@ -345,6 +345,55 @@
                   </completionHelp>
                 </properties>
               </leafNode>
+              <node name="health-check">
+                <properties>
+                  <help>Health check</help>
+                </properties>
+                <children>
+                  <leafNode name="failure-count">
+                    <properties>
+                      <help>Health check failure count required for transition to fault</help>
+                      <constraint>
+                        <validator name="numeric" argument="--positive" />
+                      </constraint>
+                    </properties>
+                    <defaultValue>3</defaultValue>
+                  </leafNode>
+                  <leafNode name="interval">
+                    <properties>
+                      <help>Health check execution interval in seconds</help>
+                      <constraint>
+                        <validator name="numeric" argument="--positive"/>
+                      </constraint>
+                    </properties>
+                    <defaultValue>60</defaultValue>
+                  </leafNode>
+                  <leafNode name="ping">
+                    <properties>
+                      <help>ICMP ping health check</help>
+                      <valueHelp>
+                        <format>ipv4</format>
+                        <description>IPv4 ping target address</description>
+                      </valueHelp>
+                      <valueHelp>
+                        <format>ipv6</format>
+                        <description>IPv6 ping target address</description>
+                      </valueHelp>
+                      <constraint>
+                        <validator name="ip-address"/>
+                      </constraint>
+                    </properties>
+                  </leafNode>
+                  <leafNode name="script">
+                    <properties>
+                      <help>Health check script file</help>
+                      <constraint>
+                        <validator name="script"/>
+                      </constraint>
+                    </properties>
+                  </leafNode>
+                </children>
+              </node>
               #include <include/vrrp-transition-script.xml.i>
             </children>
           </tagNode>
diff --git a/smoketest/scripts/cli/test_high-availability_vrrp.py b/smoketest/scripts/cli/test_high-availability_vrrp.py
index 1bb35e422..9ba06aef6 100755
--- a/smoketest/scripts/cli/test_high-availability_vrrp.py
+++ b/smoketest/scripts/cli/test_high-availability_vrrp.py
@@ -264,5 +264,58 @@ class TestVRRP(VyOSUnitTestSHIM.TestCase):
         self.assertIn(f'    {peer_address_1}', config)
         self.assertIn(f'    {peer_address_2}', config)
 
+    def test_check_health_script(self):
+        sync_group = 'VyOS'
+
+        for group in groups:
+            vlan_id = group.lstrip('VLAN')
+            vip = f'100.64.{vlan_id}.1/24'
+            group_base = base_path + ['vrrp', 'group', group]
+
+            self.cli_set(['interfaces', 'ethernet', vrrp_interface, 'vif', vlan_id, 'address', inc_ip(vip, 1) + '/' + vip.split('/')[-1]])
+
+            self.cli_set(group_base + ['interface', f'{vrrp_interface}.{vlan_id}'])
+            self.cli_set(group_base + ['address', vip])
+            self.cli_set(group_base + ['vrid', vlan_id])
+
+            self.cli_set(group_base + ['health-check', 'ping', '127.0.0.1'])
+
+        # commit changes
+        self.cli_commit()
+
+        for group in groups:
+            config = getConfig(f'vrrp_instance {group}')
+            self.assertIn(f'track_script', config)
+
+        self.cli_set(base_path + ['vrrp', 'sync-group', sync_group, 'member', groups[0]])
+
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        self.cli_delete(base_path + ['vrrp', 'group', groups[0], 'health-check'])
+        self.cli_commit()
+
+        for group in groups[1:]:
+            config = getConfig(f'vrrp_instance {group}')
+            self.assertIn(f'track_script', config)
+
+        config = getConfig(f'vrrp_instance {groups[0]}')
+        self.assertNotIn(f'track_script', config)
+
+        config = getConfig(f'vrrp_sync_group {sync_group}')
+        self.assertNotIn(f'track_script', config)
+
+        self.cli_set(base_path + ['vrrp', 'sync-group', sync_group, 'health-check', 'ping', '127.0.0.1'])
+
+        # commit changes
+        self.cli_commit()
+
+        config = getConfig(f'vrrp_instance {groups[0]}')
+        self.assertNotIn(f'track_script', config)
+
+        config = getConfig(f'vrrp_sync_group {sync_group}')
+        self.assertIn(f'track_script', config)
+
+
 if __name__ == '__main__':
     unittest.main(verbosity=2)
diff --git a/src/conf_mode/high-availability.py b/src/conf_mode/high-availability.py
index 59d49ea67..c726db8b2 100755
--- a/src/conf_mode/high-availability.py
+++ b/src/conf_mode/high-availability.py
@@ -86,16 +86,7 @@ def verify(ha):
                     raise ConfigError(f'Authentication requires both type and passwortd to be set in VRRP group "{group}"')
 
             if 'health_check' in group_config:
-                health_check_types = ["script", "ping"]
-                from vyos.utils.dict import check_mutually_exclusive_options
-                try:
-                    check_mutually_exclusive_options(group_config["health_check"], health_check_types, required=True)
-                except ValueError:
-                    Warning(f'Health check configuration for VRRP group "{group}" will remain unused ' \
-                            f'until it has one of the following options: {health_check_types}')
-                    # XXX: health check has default options so we need to remove it
-                    # to avoid generating useless config statements in keepalived.conf
-                    del group_config["health_check"]
+                _validate_health_check(group, group_config)
 
             # Keepalived doesn't allow mixing IPv4 and IPv6 in one group, so we mirror that restriction
             # We also need to make sure VRID is not used twice on the same interface with the
@@ -146,11 +137,22 @@ def verify(ha):
     # Check sync groups
     if 'vrrp' in ha and 'sync_group' in ha['vrrp']:
         for sync_group, sync_config in ha['vrrp']['sync_group'].items():
+            if 'health_check' in sync_config:
+                _validate_health_check(sync_group, sync_config)
+
             if 'member' in sync_config:
                 for member in sync_config['member']:
                     if member not in ha['vrrp']['group']:
                         raise ConfigError(f'VRRP sync-group "{sync_group}" refers to VRRP group "{member}", '\
                                           'but it does not exist!')
+                    else:
+                        ha['vrrp']['group'][member]['_is_sync_group_member'] = True
+                        if ha['vrrp']['group'][member].get('health_check') is not None:
+                            raise ConfigError(
+                                f'Health check configuration for VRRP group "{member}" will remain unused '
+                                f'while it has member of sync group "{sync_group}" '
+                                f'Only sync group health check will be used'
+                            )
 
     # Virtual-server
     if 'virtual_server' in ha:
@@ -172,6 +174,21 @@ def verify(ha):
                 raise ConfigError(f'Port is required but not set for virtual-server "{vs}" real-server "{rs}"')
 
 
+def _validate_health_check(group, group_config):
+    health_check_types = ["script", "ping"]
+    from vyos.utils.dict import check_mutually_exclusive_options
+    try:
+        check_mutually_exclusive_options(group_config["health_check"],
+                                         health_check_types, required=True)
+    except ValueError:
+        Warning(
+            f'Health check configuration for VRRP group "{group}" will remain unused ' \
+            f'until it has one of the following options: {health_check_types}')
+        # XXX: health check has default options so we need to remove it
+        # to avoid generating useless config statements in keepalived.conf
+        del group_config["health_check"]
+
+
 def generate(ha):
     if not ha or 'disable' in ha:
         if os.path.isfile(systemd_override):