dns: forwarding: T2921: migrate to get_config_dict()

9 files changed, 122 insertions, 134 deletions

diff --git a/data/configd-include.json b/data/configd-include.json
index 0c75657e0..2711a29b8 100644
--- a/data/configd-include.json
+++ b/data/configd-include.json
@@ -2,6 +2,7 @@
 "bcast_relay.py",
 "dhcp_relay.py",
 "dhcpv6_relay.py",
+"dns_forwarding.py",
 "dynamic_dns.py",
 "firewall_options.py",
 "host_name.py",
diff --git a/data/templates/dns-forwarding/recursor.conf.tmpl b/data/templates/dns-forwarding/recursor.conf.tmpl
index d233b8abc..b0ae3cc61 100644
--- a/data/templates/dns-forwarding/recursor.conf.tmpl
+++ b/data/templates/dns-forwarding/recursor.conf.tmpl
@@ -21,7 +21,7 @@ max-cache-entries={{ cache_size }}
 max-negative-ttl={{ negative_ttl }}
 
 # ignore-hosts-file
-export-etc-hosts={{ export_hosts_file }}
+export-etc-hosts={{ 'no' if ignore_hosts_file is defined else 'yes' }}
 
 # listen-address
 local-address={{ listen_address | join(',') }}
diff --git a/data/templates/dns-forwarding/recursor.forward-zones.conf.tmpl b/data/templates/dns-forwarding/recursor.forward-zones.conf.tmpl
index e62b9bb81..90f35ae1c 100644
--- a/data/templates/dns-forwarding/recursor.forward-zones.conf.tmpl
+++ b/data/templates/dns-forwarding/recursor.forward-zones.conf.tmpl
@@ -19,10 +19,10 @@
 +.={{ n.dot_zone_ns }}
 {% endif %}
 
-{% if forward_zones %}
+{% if forward_zones is defined %}
 # zones added via 'service dns forwarding domain'
 {%   for zone, zonedata in forward_zones.items() %}
-{%     if zonedata['recursion-desired'] %}+{% endif %}{{ zone }}={{ zonedata['nslist']|join(', ') }}
+{{ "+" if zonedata['recursion_desired'] is defined }}{{ zone }}={{ zonedata['server']|join(', ') }}
 {%   endfor %}
 {% endif %}
 
diff --git a/data/templates/dns-forwarding/recursor.vyos-hostsd.conf.lua.tmpl b/data/templates/dns-forwarding/recursor.vyos-hostsd.conf.lua.tmpl
index 8fefae0b2..784d5c360 100644
--- a/data/templates/dns-forwarding/recursor.vyos-hostsd.conf.lua.tmpl
+++ b/data/templates/dns-forwarding/recursor.vyos-hostsd.conf.lua.tmpl
@@ -13,10 +13,10 @@ addNTA("{{ a }}.", "{{ tag }} alias")
 {%   endfor %}
 {% endif %}
 
-{% if forward_zones %}
+{% if forward_zones is defined %}
 -- from 'service dns forwarding domain'
 {%   for zone, zonedata in forward_zones.items() %}
-{%     if zonedata['addNTA'] %}
+{%     if zonedata['addnta'] is defined %}
 addNTA("{{ zone }}", "static")
 {%     endif %}
 {%   endfor %}
diff --git a/interface-definitions/dns-forwarding.xml.in b/interface-definitions/dns-forwarding.xml.in
index aaf8bb27d..07e63d54a 100644
--- a/interface-definitions/dns-forwarding.xml.in
+++ b/interface-definitions/dns-forwarding.xml.in
@@ -16,7 +16,7 @@
             <children>
               <leafNode name="cache-size">
                 <properties>
-                  <help>DNS forwarding cache size</help>
+                  <help>DNS forwarding cache size (default: 10000)</help>
                   <valueHelp>
                     <format>0-10000</format>
                     <description>DNS forwarding cache size</description>
@@ -25,6 +25,7 @@
                     <validator name="numeric" argument="--range 0-10000"/>
                   </constraint>
                 </properties>
+                <defaultValue>10000</defaultValue>
               </leafNode>
               <leafNode name="dhcp">
                 <properties>
@@ -37,7 +38,7 @@
               </leafNode>
               <leafNode name="dnssec">
                 <properties>
-                  <help>DNSSEC mode</help>
+                  <help>DNSSEC mode (default: process-no-validate)</help>
                   <completionHelp>
                     <list>off process-no-validate process log-fail validate</list>
                   </completionHelp>
@@ -62,9 +63,10 @@
                     <description>Full blown DNSSEC validation. Send SERVFAIL to clients on bogus responses.</description>
                   </valueHelp>
                   <constraint>
-                    <regex>(off|process-no-validate|process|log-fail|validate)</regex>
+                    <regex>^(off|process-no-validate|process|log-fail|validate)$</regex>
                   </constraint>
                 </properties>
+                <defaultValue>process-no-validate</defaultValue>
               </leafNode>
               <tagNode name="domain">
                 <properties>
@@ -146,7 +148,7 @@
               </leafNode>
               <leafNode name="negative-ttl">
                 <properties>
-                  <help>Maximum amount of time negative entries are cached</help>
+                  <help>Maximum amount of time negative entries are cached (default: 3600)</help>
                   <valueHelp>
                     <format>0-7200</format>
                     <description>Seconds to cache NXDOMAIN entries</description>
@@ -155,6 +157,7 @@
                     <validator name="numeric" argument="--range 0-7200"/>
                   </constraint>
                 </properties>
+                <defaultValue>3600</defaultValue>
               </leafNode>
               <leafNode name="name-server">
                 <properties>
diff --git a/smoketest/scripts/cli/test_service_dns_forwarding.py b/smoketest/scripts/cli/test_service_dns_forwarding.py
index 717b5b56d..5e2f3dfbd 100755
--- a/smoketest/scripts/cli/test_service_dns_forwarding.py
+++ b/smoketest/scripts/cli/test_service_dns_forwarding.py
@@ -24,6 +24,7 @@ from vyos.util import process_named_running
 
 CONFIG_FILE = '/run/powerdns/recursor.conf'
 FORWARD_FILE = '/run/powerdns/recursor.forward-zones.conf'
+HOSTSD_FILE = '/run/powerdns/recursor.vyos-hostsd.conf.lua'
 PROCESS_NAME= 'pdns-r/worker'
 
 base_path = ['service', 'dns', 'forwarding']
@@ -69,6 +70,9 @@ class TestServicePowerDNS(unittest.TestCase):
         # configure DNSSEC
         self.session.set(base_path + ['dnssec', 'validate'])
 
+        # Do not use local /etc/hosts file in name resolution
+        self.session.set(base_path + ['ignore-hosts-file'])
+
         # commit changes
         self.session.commit()
 
@@ -88,6 +92,10 @@ class TestServicePowerDNS(unittest.TestCase):
         tmp = get_config_value('max-negative-ttl')
         self.assertEqual(tmp, negative_ttl)
 
+        # Do not use local /etc/hosts file in name resolution
+        tmp = get_config_value('export-etc-hosts')
+        self.assertEqual(tmp, 'no')
+
         # Check for running process
         self.assertTrue(process_named_running(PROCESS_NAME))
 
@@ -130,6 +138,11 @@ class TestServicePowerDNS(unittest.TestCase):
         tmp = get_config_value(r'\+.', file=FORWARD_FILE)
         self.assertEqual(tmp, ', '.join(nameservers))
 
+        # Do not use local /etc/hosts file in name resolution
+        # default: yes
+        tmp = get_config_value('export-etc-hosts')
+        self.assertEqual(tmp, 'yes')
+
         # Check for running process
         self.assertTrue(process_named_running(PROCESS_NAME))
 
@@ -141,21 +154,39 @@ class TestServicePowerDNS(unittest.TestCase):
         for address in listen_adress:
             self.session.set(base_path + ['listen-address', address])
 
-        domains = ['vyos.io', 'vyos.net']
+        domains = ['vyos.io', 'vyos.net', 'vyos.com']
         nameservers = ['192.0.2.1', '192.0.2.2']
         for domain in domains:
             for nameserver in nameservers:
                 self.session.set(base_path + ['domain', domain, 'server', nameserver])
 
+            # Test 'recursion-desired' flag for only one domain
+            if domain == domains[0]:
+                self.session.set(base_path + ['domain', domain, 'recursion-desired'])
+
+            # Test 'negative trust anchor' flag for the second domain only
+            if domain == domains[1]:
+                self.session.set(base_path + ['domain', domain, 'addnta'])
+
         # commit changes
         self.session.commit()
 
+        # Test configured name-servers
+        hosts_conf = read_file(HOSTSD_FILE)
         for domain in domains:
-            tmp = get_config_value(domain, file=FORWARD_FILE)
+            # Test 'recursion-desired' flag for the first domain only
+            if domain == domains[0]: key =f'\+{domain}'
+            else: key =f'{domain}'
+            tmp = get_config_value(key, file=FORWARD_FILE)
             self.assertEqual(tmp, ', '.join(nameservers))
 
+            # Test 'negative trust anchor' flag for the second domain only
+            if domain == domains[1]:
+                self.assertIn(f'addNTA("{domain}", "static")', hosts_conf)
+
         # Check for running process
         self.assertTrue(process_named_running(PROCESS_NAME))
 
 if __name__ == '__main__':
     unittest.main()
+
diff --git a/src/conf_mode/dns_forwarding.py b/src/conf_mode/dns_forwarding.py
index d6eb76d91..5101c1e79 100755
--- a/src/conf_mode/dns_forwarding.py
+++ b/src/conf_mode/dns_forwarding.py
@@ -17,14 +17,17 @@
 import os
 
 from sys import exit
-from copy import deepcopy
 
 from vyos.config import Config
+from vyos.configdict import dict_merge
 from vyos.hostsd_client import Client as hostsd_client
-from vyos import ConfigError
-from vyos.util import call, chown
+from vyos.util import call
+from vyos.util import chown
+from vyos.util import vyos_dict_search
 from vyos.template import render
+from vyos.xml import defaults
 
+from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
 
@@ -35,116 +38,63 @@ pdns_rec_hostsd_lua_conf_file = f'{pdns_rec_run_dir}/recursor.vyos-hostsd.conf.l
 pdns_rec_hostsd_zones_file = f'{pdns_rec_run_dir}/recursor.forward-zones.conf'
 pdns_rec_config_file = f'{pdns_rec_run_dir}/recursor.conf'
 
-default_config_data = {
-    'allow_from': [],
-    'cache_size': 10000,
-    'export_hosts_file': 'yes',
-    'listen_address': [],
-    'name_servers': [],
-    'negative_ttl': 3600,
-    'system': False,
-    'domains': {},
-    'dnssec': 'process-no-validate',
-    'dhcp_interfaces': []
-}
-
 hostsd_tag = 'static'
 
-def get_config(conf):
-    dns = deepcopy(default_config_data)
+def get_config(config=None):
+    if config:
+        conf = config
+    else:
+        conf = Config()
     base = ['service', 'dns', 'forwarding']
-
     if not conf.exists(base):
         return None
 
-    conf.set_level(base)
-
-    if conf.exists(['allow-from']):
-        dns['allow_from'] = conf.return_values(['allow-from'])
-
-    if conf.exists(['cache-size']):
-        cache_size = conf.return_value(['cache-size'])
-        dns['cache_size'] = cache_size
-
-    if conf.exists('negative-ttl'):
-        negative_ttl = conf.return_value(['negative-ttl'])
-        dns['negative_ttl'] = negative_ttl
-
-    if conf.exists(['domain']):
-        for domain in conf.list_nodes(['domain']):
-            conf.set_level(base + ['domain', domain])
-            entry = {
-                'nslist': bracketize_ipv6_addrs(conf.return_values(['server'])),
-                'addNTA': conf.exists(['addnta']),
-                'recursion-desired': conf.exists(['recursion-desired'])
-            }
-            dns['domains'][domain] = entry
-
-        conf.set_level(base)
+    dns = conf.get_config_dict(base, key_mangling=('-', '_'), get_first_key=True)
+    # We have gathered the dict representation of the CLI, but there are default
+    # options which we need to update into the dictionary retrived.
+    default_values = defaults(base)
+    dns = dict_merge(default_values, dns)
 
-    if conf.exists(['ignore-hosts-file']):
-        dns['export_hosts_file'] = "no"
+    # some additions to the default dictionary
+    if 'system' in dns:
+        base_nameservers = ['system', 'name-server']
+        if conf.exists(base_nameservers):
+            dns.update({'system_name_server': conf.return_values(base_nameservers)})
 
-    if conf.exists(['name-server']):
-        dns['name_servers'] = bracketize_ipv6_addrs(
-                conf.return_values(['name-server']))
-
-    if conf.exists(['system']):
-        dns['system'] = True
-
-    if conf.exists(['listen-address']):
-        dns['listen_address'] = conf.return_values(['listen-address'])
-
-    if conf.exists(['dnssec']):
-        dns['dnssec'] = conf.return_value(['dnssec'])
-
-    if conf.exists(['dhcp']):
-        dns['dhcp_interfaces'] = conf.return_values(['dhcp'])
+        base_nameservers_dhcp = ['system', 'name-servers-dhcp']
+        if conf.exists(base_nameservers_dhcp):
+            dns.update({'system_name_server_dhcp': conf.return_values(base_nameservers_dhcp)})
 
     return dns
 
-def bracketize_ipv6_addrs(addrs):
-    """Wraps each IPv6 addr in addrs in [], leaving IPv4 addrs untouched."""
-    return ['[{0}]'.format(a) if a.count(':') > 1 else a for a in addrs]
-
-def verify(conf, dns):
+def verify(dns):
     # bail out early - looks like removal from running config
-    if dns is None:
+    if not dns:
         return None
 
-    if not dns['listen_address']:
-        raise ConfigError(
-            "Error: DNS forwarding requires a listen-address")
-
-    if not dns['allow_from']:
-        raise ConfigError(
-                "Error: DNS forwarding requires an allow-from network")
-
-    if dns['domains']:
-        for domain in dns['domains']:
-            if not dns['domains'][domain]['nslist']:
-                raise ConfigError((
-                    f'Error: No server configured for domain {domain}'))
-
-    no_system_nameservers = False
-    conf.set_level([])
-    if dns['system'] and not (
-            conf.exists(['system', 'name-server']) or
-            conf.exists(['system', 'name-servers-dhcp']) ):
-        no_system_nameservers = True
-        print(("DNS forwarding warning: No 'system name-server' or "
-                "'system name-servers-dhcp' set\n"))
-
-    if (no_system_nameservers or not dns['system']) and not (
-            dns['name_servers'] or dns['dhcp_interfaces']):
-        print(("DNS forwarding warning: No 'dhcp', 'name-server' or 'system' "
-            "nameservers set. Forwarding will operate as a recursor.\n"))
+    if 'listen_address' not in dns:
+        raise ConfigError('DNS forwarding requires a listen-address')
+
+    if 'allow_from' not in dns:
+        raise ConfigError('DNS forwarding requires an allow-from network')
+
+    # we can not use vyos_dict_search() when testing for domain servers
+    # as a domain will contains dot's which is out dictionary delimiter.
+    if 'domain' in dns:
+        for domain in dns['domain']:
+            if 'server' not in dns['domain'][domain]:
+                raise ConfigError(f'No server configured for domain {domain}!')
+
+    if 'system' in dns:
+        if not ('system_name_server' in dns or 'system_name_server_dhcp' in dns):
+            print("Warning: No 'system name-server' or 'system " \
+                  "name-servers-dhcp' configured")
 
     return None
 
 def generate(dns):
     # bail out early - looks like removal from running config
-    if dns is None:
+    if not dns:
         return None
 
     render(pdns_rec_config_file, 'dns-forwarding/recursor.conf.tmpl',
@@ -154,17 +104,18 @@ def generate(dns):
             dns, trim_blocks=True, user=pdns_rec_user, group=pdns_rec_group)
 
     # if vyos-hostsd didn't create its files yet, create them (empty)
-    for f in [pdns_rec_hostsd_lua_conf_file, pdns_rec_hostsd_zones_file]:
-        with open(f, 'a'):
+    for file in [pdns_rec_hostsd_lua_conf_file, pdns_rec_hostsd_zones_file]:
+        with open(file, 'a'):
             pass
-        chown(f, user=pdns_rec_user, group=pdns_rec_group)
+        chown(file, user=pdns_rec_user, group=pdns_rec_group)
 
     return None
 
 def apply(dns):
-    if dns is None:
+    if not dns:
         # DNS forwarding is removed in the commit
-        call("systemctl stop pdns-recursor.service")
+        call('systemctl stop pdns-recursor.service')
+
         if os.path.isfile(pdns_rec_config_file):
             os.unlink(pdns_rec_config_file)
     else:
@@ -174,8 +125,8 @@ def apply(dns):
         # add static nameservers to hostsd so they can be joined with other
         # sources
         hc.delete_name_servers([hostsd_tag])
-        if dns['name_servers']:
-            hc.add_name_servers({hostsd_tag: dns['name_servers']})
+        if 'name_server' in dns:
+            hc.add_name_servers({hostsd_tag: dns['name_server']})
 
         # delete all nameserver tags
         hc.delete_name_server_tags_recursor(hc.get_name_server_tags_recursor())
@@ -184,32 +135,33 @@ def apply(dns):
         # our own tag (static)
         hc.add_name_server_tags_recursor([hostsd_tag])
 
-        if dns['system']:
+        if 'system' in dns:
             hc.add_name_server_tags_recursor(['system'])
         else:
             hc.delete_name_server_tags_recursor(['system'])
 
         # add dhcp nameserver tags for configured interfaces
-        for intf in dns['dhcp_interfaces']:
-            hc.add_name_server_tags_recursor(['dhcp-' + intf, 'dhcpv6-' + intf ])
+        if 'system_name_server_dhcp' in dns:
+            for interface in dns['system_name_server_dhcp']:
+                hc.add_name_server_tags_recursor(['dhcp-' + interface,
+                                                  'dhcpv6-' + interface ])
 
         # hostsd will generate the forward-zones file
         # the list and keys() are required as get returns a dict, not list
         hc.delete_forward_zones(list(hc.get_forward_zones().keys()))
-        if dns['domains']:
-            hc.add_forward_zones(dns['domains'])
+        if 'domain' in dns:
+            hc.add_forward_zones(dns['domain'])
 
         # call hostsd to generate forward-zones and its lua-config-file
         hc.apply()
 
         ### finally (re)start pdns-recursor
-        call("systemctl restart pdns-recursor.service")
+        call('systemctl restart pdns-recursor.service')
 
 if __name__ == '__main__':
     try:
-        conf = Config()
-        c = get_config(conf)
-        verify(conf, c)
+        c = get_config()
+        verify(c)
         generate(c)
         apply(c)
     except ConfigError as e:
diff --git a/src/services/vyos-hostsd b/src/services/vyos-hostsd
index 0079f7e5c..59dbeda17 100755
--- a/src/services/vyos-hostsd
+++ b/src/services/vyos-hostsd
@@ -107,16 +107,17 @@
 #
 ### forward_zones
 ## Additional zones added to pdns-recursor forward-zones-file.
-## If recursion-desired is true, '+' will be prepended to the zone line.
-## If addNTA is true, a NTA will be added via lua-config-file.
+## If recursion_desired is true, '+' will be prepended to the zone line.
+## If addnta is true, a NTA (Negative Trust Anchor) will be added via
+## lua-config-file.
 #
 # { 'type': 'forward_zones',
 #   'op': 'add',
 #   'data': {
 #       '<str zone>': {
-#           'nslist': ['<str nameserver>', ...],
-#           'addNTA': <bool>,
-#           'recursion-desired': <bool>
+#           'server': ['<str nameserver>', ...],
+#           'addnta': <bool>,
+#           'recursion_desired': <bool>
 #         }
 #       ...
 #     }
@@ -305,12 +306,12 @@ tag_regex_schema = op_type_schema.extend({
 forward_zone_add_schema = op_type_schema.extend({
     'data': {
         str: {
-            'nslist': [str],
-            'addNTA': bool,
-            'recursion-desired': bool
+            'server': [str],
+            'addnta': Any({}, None),
+            'recursion_desired': Any({}, None),
             }
         }
-    }, required=True)
+    }, required=False)
 
 hosts_add_schema = op_type_schema.extend({
     'data': {
@@ -586,7 +587,7 @@ if __name__ == '__main__':
 
     context = zmq.Context()
     socket = context.socket(zmq.REP)
-    
+
     # Set the right permissions on the socket, then change it back
     o_mask = os.umask(0o007)
     socket.bind(SOCKET_PATH)
diff --git a/src/utils/vyos-hostsd-client b/src/utils/vyos-hostsd-client
index 48ebc83f7..d4d38315a 100755
--- a/src/utils/vyos-hostsd-client
+++ b/src/utils/vyos-hostsd-client
@@ -99,9 +99,9 @@ try:
             raise ValueError("--nameservers is required for this operation")
         client.add_forward_zones(
                 { args.add_forward_zone: {
-                    'nslist': args.nameservers,
-                    'addNTA': args.addnta,
-                    'recursion-desired': args.recursion_desired
+                    'server': args.nameservers,
+                    'addnta': args.addnta,
+                    'recursion_desired': args.recursion_desired
                     }
                 })
     elif args.delete_forward_zones: