Merge pull request #2029 from indrajitr/ddclient-improvement-round-3

dns: T5144: Refactor smoke tests for dynamic dns operation

3 files changed, 93 insertions, 81 deletions

diff --git a/smoketest/scripts/cli/test_service_dns_dynamic.py b/smoketest/scripts/cli/test_service_dns_dynamic.py
index 044d053b4..11d411cb4 100755
--- a/smoketest/scripts/cli/test_service_dns_dynamic.py
+++ b/smoketest/scripts/cli/test_service_dns_dynamic.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2019-2020 VyOS maintainers and contributors
+# Copyright (C) 2019-2023 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
@@ -30,13 +30,16 @@ DDCLIENT_PID = '/run/ddclient/ddclient.pid'
 
 base_path = ['service', 'dns', 'dynamic']
 hostname = 'test.ddns.vyos.io'
+zone = 'vyos.io'
+password = 'paSS_@4ord'
 interface = 'eth0'
 
+
 def get_config_value(key):
     tmp = cmd(f'sudo cat {DDCLIENT_CONF}')
-    tmp = re.findall(r'\n?{}=+(.*)'.format(key), tmp)
-    tmp = tmp[0].rstrip(', \\')
-    return tmp
+    vals = re.findall(r'\n?{}=([.-@_A-Za-z0-9]+),? \\'.format(key), tmp)
+    return vals[0] if vals else ''
+
 
 class TestServiceDDNS(VyOSUnitTestSHIM.TestCase):
     def tearDown(self):
@@ -50,13 +53,12 @@ class TestServiceDDNS(VyOSUnitTestSHIM.TestCase):
         # PID file must no londer exist after process exited
         self.assertFalse(os.path.exists(DDCLIENT_PID))
 
-    def test_dyndns_service(self):
+    # IPv4 standard DDNS service configuration
+    def test_dyndns_service_standard(self):
         ddns = ['address', interface, 'service']
-        services = {'cloudflare': {'protocol': 'cloudflare', 'zone': 'vyos.io'},
+        services = {'cloudflare': {'protocol': 'cloudflare'},
                     'freedns': {'protocol': 'freedns', 'username': 'vyos_user'},
                     'zoneedit': {'protocol': 'zoneedit1', 'username': 'vyos_user'}}
-        password = 'vyos_pass'
-        zone = 'vyos.io'
 
         for svc, details in services.items():
             self.cli_delete(base_path)
@@ -78,44 +80,46 @@ class TestServiceDDNS(VyOSUnitTestSHIM.TestCase):
                 # commit changes again - now it should work
                 self.cli_commit()
 
+            # Check the generating config parameters
+            self.assertEqual(get_config_value('use'), 'if')
+            self.assertEqual(get_config_value('if'), interface)
+            self.assertEqual(get_config_value('password'), password)
+
             for opt in details.keys():
                 if opt == 'username':
-                    self.assertTrue(get_config_value('login') == details[opt])
+                    self.assertEqual(get_config_value('login'), details[opt])
                 else:
-                    self.assertTrue(get_config_value(opt) == details[opt])
-
-            self.assertTrue(get_config_value('use') == 'if')
-            self.assertTrue(get_config_value('if') == interface)
+                    self.assertEqual(get_config_value(opt), details[opt])
 
-    def test_dyndns_rfc2136(self):
-        # Check if DDNS service can be configured and runs
-        ddns = ['address', interface, 'rfc2136', 'vyos']
-        srv = 'ns1.vyos.io'
-        zone = 'vyos.io'
-        ttl = '300'
-
-        with tempfile.NamedTemporaryFile(prefix='/config/auth/') as key_file:
-            key_file.write(b'S3cretKey')
+    # IPv6 only DDNS service configuration
+    def test_dyndns_service_ipv6(self):
+        ddns = ['address', interface, 'service', 'dynv6']
+        proto = 'dyndns2'
+        user = 'none'
+        password = 'paSS_4ord'
+        srv = 'ddns.vyos.io'
+        ip_version = 'ipv6'
 
-            self.cli_set(base_path + ddns + ['key', key_file.name])
-            self.cli_set(base_path + ddns + ['host-name', hostname])
-            self.cli_set(base_path + ddns + ['server', srv])
-            self.cli_set(base_path + ddns + ['ttl', ttl])
-            self.cli_set(base_path + ddns + ['zone', zone])
+        self.cli_set(base_path + ddns + ['ip-version', ip_version])
+        self.cli_set(base_path + ddns + ['protocol', proto])
+        self.cli_set(base_path + ddns + ['server', srv])
+        self.cli_set(base_path + ddns + ['username', user])
+        self.cli_set(base_path + ddns + ['password', password])
+        self.cli_set(base_path + ddns + ['host-name', hostname])
 
-            # commit changes
-            self.cli_commit()
+        # commit changes
+        self.cli_commit()
 
-            # Check some generating config parameters
-            self.assertEqual(get_config_value('protocol'), 'nsupdate')
-            self.assertTrue(get_config_value('password') == key_file.name)
-            self.assertTrue(get_config_value('server') == srv)
-            self.assertTrue(get_config_value('zone') == zone)
-            self.assertTrue(get_config_value('ttl') == ttl)
-            self.assertEqual(get_config_value('use'), 'if')
-            self.assertEqual(get_config_value('if'), interface)
+        # Check the generating config parameters
+        self.assertEqual(get_config_value('usev6'), 'ifv6')
+        self.assertEqual(get_config_value('ifv6'), interface)
+        self.assertEqual(get_config_value('protocol'), proto)
+        self.assertEqual(get_config_value('server'), srv)
+        self.assertEqual(get_config_value('login'), user)
+        self.assertEqual(get_config_value('password'), password)
 
-    def test_dyndns_dual(self):
+    # IPv4+IPv6 dual DDNS service configuration
+    def test_dyndns_service_dual_stack(self):
         ddns = ['address', interface, 'service']
         services = {'cloudflare': {'protocol': 'cloudflare', 'zone': 'vyos.io'},
                     'freedns': {'protocol': 'freedns', 'username': 'vyos_user'}}
@@ -133,43 +137,47 @@ class TestServiceDDNS(VyOSUnitTestSHIM.TestCase):
             # commit changes
             self.cli_commit()
 
-            # Check some generating config parameters
+            # Check the generating config parameters
+            self.assertEqual(get_config_value('usev4'), 'ifv4')
+            self.assertEqual(get_config_value('usev6'), 'ifv6')
+            self.assertEqual(get_config_value('ifv4'), interface)
+            self.assertEqual(get_config_value('ifv6'), interface)
+            self.assertEqual(get_config_value('password'), password)
+
             for opt in details.keys():
                 if opt == 'username':
-                    self.assertTrue(get_config_value('login') == details[opt])
+                    self.assertEqual(get_config_value('login'), details[opt])
                 else:
-                    self.assertTrue(get_config_value(opt) == details[opt])
+                    self.assertEqual(get_config_value(opt), details[opt])
 
-            self.assertTrue(get_config_value('usev4') == 'ifv4')
-            self.assertTrue(get_config_value('usev6') == 'ifv6')
-            self.assertTrue(get_config_value('ifv4') == interface)
-            self.assertTrue(get_config_value('ifv6') == interface)
+    def test_dyndns_rfc2136(self):
+        # Check if DDNS service can be configured and runs
+        ddns = ['address', interface, 'rfc2136', 'vyos']
+        srv = 'ns1.vyos.io'
+        zone = 'vyos.io'
+        ttl = '300'
 
-    def test_dyndns_ipv6(self):
-        ddns = ['address', interface, 'service', 'dynv6']
-        proto = 'dyndns2'
-        user = 'none'
-        password = 'paSS_4ord'
-        srv = 'ddns.vyos.io'
-        ip_version = 'ipv6'
+        with tempfile.NamedTemporaryFile(prefix='/config/auth/') as key_file:
+            key_file.write(b'S3cretKey')
 
-        self.cli_set(base_path + ddns + ['host-name', hostname])
-        self.cli_set(base_path + ddns + ['username', user])
-        self.cli_set(base_path + ddns + ['password', password])
-        self.cli_set(base_path + ddns + ['protocol', proto])
-        self.cli_set(base_path + ddns + ['server', srv])
-        self.cli_set(base_path + ddns + ['ip-version', ip_version])
+            self.cli_set(base_path + ddns + ['server', srv])
+            self.cli_set(base_path + ddns + ['zone', zone])
+            self.cli_set(base_path + ddns + ['key', key_file.name])
+            self.cli_set(base_path + ddns + ['ttl', ttl])
+            self.cli_set(base_path + ddns + ['host-name', hostname])
 
-        # commit changes
-        self.cli_commit()
+            # commit changes
+            self.cli_commit()
+
+            # Check some generating config parameters
+            self.assertEqual(get_config_value('use'), 'if')
+            self.assertEqual(get_config_value('if'), interface)
+            self.assertEqual(get_config_value('protocol'), 'nsupdate')
+            self.assertEqual(get_config_value('server'), srv)
+            self.assertEqual(get_config_value('zone'), zone)
+            self.assertEqual(get_config_value('password'), key_file.name)
+            self.assertEqual(get_config_value('ttl'), ttl)
 
-        # Check some generating config parameters
-        self.assertEqual(get_config_value('protocol'), proto)
-        self.assertEqual(get_config_value('login'), user)
-        self.assertEqual(get_config_value('password'), password)
-        self.assertEqual(get_config_value('server'), srv)
-        self.assertEqual(get_config_value('usev6'), 'ifv6')
-        self.assertEqual(get_config_value('ifv6'), interface)
 
 if __name__ == '__main__':
     unittest.main(verbosity=2)
diff --git a/src/conf_mode/dns_dynamic.py b/src/conf_mode/dns_dynamic.py
index f97225370..e070a3502 100755
--- a/src/conf_mode/dns_dynamic.py
+++ b/src/conf_mode/dns_dynamic.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2018-2020 VyOS maintainers and contributors
+# Copyright (C) 2018-2023 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
@@ -51,20 +51,21 @@ def get_config(config=None):
 
     dyndns = conf.get_config_dict(base_level, key_mangling=('-', '_'), get_first_key=True)
 
-    for address in dyndns['address']:
-        # Apply service specific defaults (stype = ['rfc2136', 'service'])
-        for svc_type in dyndns['address'][address]:
-            default_values = defaults(base_level + ['address', svc_type])
-            for svc_cfg in dyndns['address'][address][svc_type]:
-                dyndns['address'][address][svc_type][svc_cfg] = dict_merge(
-                    default_values, dyndns['address'][address][svc_type][svc_cfg])
+    if 'address' in dyndns:
+        for address in dyndns['address']:
+            # Apply service specific defaults (svc_type = ['rfc2136', 'service'])
+            for svc_type in dyndns['address'][address]:
+                default_values = defaults(base_level + ['address', svc_type])
+                for svc_cfg in dyndns['address'][address][svc_type]:
+                    dyndns['address'][address][svc_type][svc_cfg] = dict_merge(
+                        default_values, dyndns['address'][address][svc_type][svc_cfg])
 
     dyndns['config_file'] = config_file
     return dyndns
 
 def verify(dyndns):
     # bail out early - looks like removal from running config
-    if not dyndns:
+    if not dyndns or 'address' not in dyndns:
         return None
 
     for address in dyndns['address']:
@@ -97,16 +98,18 @@ def verify(dyndns):
 
                 if config['ip_version'] == 'both':
                     if config['protocol'] not in dualstack_supported:
-                        raise ConfigError(f'"{config["protocol"]}" does not support IPv4 and IPv6 at the same time')
+                        raise ConfigError(f'"{config["protocol"]}" does not support '
+                                          f'both IPv4 and IPv6 at the same time')
                     # dyndns2 protocol in ddclient honors dual stack only for dyn.com (dyndns.org)
                     if config['protocol'] == 'dyndns2' and 'server' in config and config['server'] != 'members.dyndns.org':
-                        raise ConfigError(f'"{config["protocol"]}" for "{config["server"]}" does not support IPv4 and IPv6 at the same time')
+                        raise ConfigError(f'"{config["protocol"]}" does not support '
+                                          f'both IPv4 and IPv6 at the same time for "{config["server"]}"')
 
     return None
 
 def generate(dyndns):
     # bail out early - looks like removal from running config
-    if not dyndns:
+    if not dyndns or 'address' not in dyndns:
         return None
 
     render(config_file, 'dns-dynamic/ddclient.conf.j2', dyndns)
@@ -114,7 +117,8 @@ def generate(dyndns):
     return None
 
 def apply(dyndns):
-    if not dyndns:
+    # bail out early - looks like removal from running config
+    if not dyndns or 'address' not in dyndns:
         call('systemctl stop ddclient.service')
         if os.path.exists(config_file):
             os.unlink(config_file)
diff --git a/src/op_mode/dns_dynamic.py b/src/op_mode/dns_dynamic.py
index d41a74db3..76ca5249b 100755
--- a/src/op_mode/dns_dynamic.py
+++ b/src/op_mode/dns_dynamic.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2018-2020 VyOS maintainers and contributors
+# Copyright (C) 2018-2023 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