pppoe-server: T2314: remove boilerplate code and adjust

5 files changed, 508 insertions, 496 deletions

diff --git a/data/templates/pppoe-server/chap-secrets.tmpl b/data/templates/pppoe-server/chap-secrets.tmpl
index 907ac6ed7..dd00d7bd0 100644
--- a/data/templates/pppoe-server/chap-secrets.tmpl
+++ b/data/templates/pppoe-server/chap-secrets.tmpl
@@ -1,10 +1,10 @@
 # username  server  password  acceptable local IP addresses   shaper
-{% for user in authentication['local-users'] %}
-{% if authentication['local-users'][user]['state'] == 'enabled' %}
-{% if (authentication['local-users'][user]['upload']) and (authentication['local-users'][user]['download']) %}
-{{ "%-12s" | format(user) }} * {{ "%-16s" | format(authentication['local-users'][user]['passwd']) }} {{ "%-16s" | format(authentication['local-users'][user]['ip']) }} {{ authentication['local-users'][user]['download'] }} / {{ authentication['local-users'][user]['upload'] }}
+{% for user in local_users %}
+{% if user.state == 'enabled' %}
+{% if user.upload and user.download %}
+{{ "%-12s" | format(user.name) }} * {{ "%-16s" | format(user.password) }} {{ "%-16s" | format(user.ip) }} {{ user.download }} / {{ user.upload }}
 {% else %}
-{{ "%-12s" | format(user) }} * {{ "%-16s" | format(authentication['local-users'][user]['passwd']) }} {{ "%-16s" | format(authentication['local-users'][user]['ip']) }}
+{{ "%-12s" | format(user.name) }} * {{ "%-16s" | format(user.password) }} {{ "%-16s" | format(user.ip) }}
 {% endif %}
 {% endif %}
 {% endfor %}
diff --git a/data/templates/pppoe-server/pppoe.config.tmpl b/data/templates/pppoe-server/pppoe.config.tmpl
index d44c0aa93..f7c639f62 100644
--- a/data/templates/pppoe-server/pppoe.config.tmpl
+++ b/data/templates/pppoe-server/pppoe.config.tmpl
@@ -3,11 +3,11 @@
 [modules]
 log_syslog
 pppoe
-{% if authentication['mode'] == 'radius' %}
+{% if auth_mode == 'radius' %}
 radius
 {% endif %}
 ippool
-{% if ppp_options['ipv6'] != 'deny' %}
+{% if ppp_ipv6 != 'deny' %}
 ipv6pool
 ipv6_nd
 ipv6_dhcp
@@ -17,9 +17,8 @@ auth_pap
 auth_chap_md5
 auth_mschap_v1
 auth_mschap_v2
-#pppd_compat
 shaper
-{% if snmp == 'enable' or snmp == 'enable-ma' %}
+{% if snmp %}
 net-snmp
 {% endif %}
 {% if limits %}
@@ -27,7 +26,7 @@ connlimit
 {% endif %}
 
 [core]
-thread-count={{thread_cnt}}
+thread-count={{ thread_cnt }}
 
 [log]
 syslog=accel-pppoe,daemon
@@ -37,32 +36,31 @@ level=5
 {% if snmp == 'enable-ma' %}
 [snmp]
 master=1
-{% endif -%}
+{% endif %}
 
 [client-ip-range]
 disable
 
 {% if ppp_gw %}
 [ip-pool]
-gw-ip-address={{ppp_gw}}
+gw-ip-address={{ ppp_gw }}
 {% if client_ip_pool %}
-{{client_ip_pool}}
+{{ client_ip_pool }}
 {% endif -%}
-
 {% if client_ip_subnets %}
-{% for sn in client_ip_subnets %}
-{{sn}}
+{% for subnet in client_ip_subnets %}
+{{ subnet }}
 {% endfor %}
 {% endif %}
-{% endif -%}
+{% endif %}
 
 {% if client_ipv6_pool %}
 [ipv6-pool]
-{% for prfx in client_ipv6_pool['prefix']: %}
-{{prfx}}
+{% for prefix in client_ipv6_pool['prefix']: %}
+{{ prefix }}
 {% endfor %}
-{% for prfx in client_ipv6_pool['delegate-prefix']: %}
-delegate={{prfx}}
+{% for prefix in client_ipv6_pool['delegate-prefix']: %}
+delegate={{ prefix }}
 {% endfor %}
 {% endif %}
 
@@ -93,47 +91,39 @@ wins2={{wins[1]}}
 {% endif -%}
 {% endif -%}
 
-{% if authentication['mode'] == 'local' %}
+{% if auth_mode == 'local' %}
 [chap-secrets]
-chap-secrets=/etc/accel-ppp/pppoe/chap-secrets
-{% endif -%}
-
-{% if authentication['mode'] == 'radius' %}
+chap-secrets={{ chap_secrets_file }}
+{% elif auth_mode == 'radius' %}
 [radius]
-{% for rsrv in authentication['radiussrv']: %}
-server={{rsrv}},{{authentication['radiussrv'][rsrv]['secret']}},\
-req-limit={{authentication['radiussrv'][rsrv]['req-limit']}},\
-fail-time={{authentication['radiussrv'][rsrv]['fail-time']}}
-{% endfor %}
-{% if authentication['radiusopt']['timeout'] %}
-timeout={{authentication['radiusopt']['timeout']}}
-{% endif %}
-{% if authentication['radiusopt']['acct-timeout'] %}
-acct-timeout={{authentication['radiusopt']['acct-timeout']}}
-{% endif %}
-{% if authentication['radiusopt']['max-try'] %}
-max-try={{authentication['radiusopt']['max-try']}}
-{% endif %}
-{% if authentication['radiusopt']['nas-id'] %}
-nas-identifier={{authentication['radiusopt']['nas-id']}}
-{% endif %}
-{% if authentication['radiusopt']['nas-ip'] %}
-nas-ip-address={{authentication['radiusopt']['nas-ip']}}
+verbose=1
+{% for r in radius_server %}
+server={{ r.server }},{{ r.key }},auth-port={{ r.port }},req-limit=0,fail-time={{ r.fail_time }}
+{% endfor -%}
+
+acct-timeout={{ radius_acct_tmo }}
+timeout={{ radius_timeout }}
+max-try={{ radius_max_try }}
+{% if radius_nas_id %}
+nas-identifier={{ radius_nas_id }}
 {% endif -%}
-{% if authentication['radiusopt']['dae-srv'] %}
-dae-server={{authentication['radiusopt']['dae-srv']['ip-addr']}}:\
-{{authentication['radiusopt']['dae-srv']['port']}},\
-{{authentication['radiusopt']['dae-srv']['secret']}}
+{% if radius_nas_ip %}
+nas-ip-address={{ radius_nas_ip }}
+{% endif -%}
+{% if radius_source_address %}
+bind={{ radius_source_address }}
+{% endif -%}
+
+{% if radius_dynamic_author %}
+dae-server={{ radius_dynamic_author.server }}:{{ radius_dynamic_author.port }},{{ radius_dynamic_author.key }}
 {% endif -%}
-gw-ip-address={{ppp_gw}}
-verbose=1
 
-{% if authentication['radiusopt']['shaper'] %}
+{% if radius_shaper_attr %}
 [shaper]
 verbose=1
-attr={{authentication['radiusopt']['shaper']['attr']}}
-{% if authentication['radiusopt']['shaper']['vendor'] %}
-vendor={{authentication['radiusopt']['shaper']['vendor']}}
+attr={{ radius_shaper_attr }}
+{% if radius_shaper_vendor %}
+vendor={{ radius_shaper_vendor }}
 {% endif -%}
 {% endif -%}
 {% endif %}
@@ -144,84 +134,69 @@ check-ip=1
 {% if not sesscrtl == 'disable' %}
 single-session={{sesscrtl}}
 {% endif -%}
-{% if ppp_options['ccp'] %}
+{% if ppp_ccp %}
 ccp=1
 {% endif %}
-{% if ppp_options['min-mtu'] %}
-min-mtu={{ppp_options['min-mtu']}}
-{% else %}
-min-mtu={{mtu}}
-{% endif %}
-{% if ppp_options['mru'] %}
-mru={{ppp_options['mru']}}
-{% endif %}
-{% if ppp_options['mppe'] %}
-mppe={{ppp_options['mppe']}}
+{% if ppp_min_mtu %}
+min-mtu={{ ppp_min_mtu }}
 {% else %}
-mppe=prefer
+min-mtu={{ mtu }}
 {% endif %}
-{% if ppp_options['lcp-echo-interval'] %}
-lcp-echo-interval={{ppp_options['lcp-echo-interval']}}
-{% else %}
-lcp-echo-interval=30
-{% endif %}
-{% if ppp_options['lcp-echo-timeout'] %}
-lcp-echo-timeout={{ppp_options['lcp-echo-timeout']}}
-{% endif %}
-{% if ppp_options['lcp-echo-failure'] %}
-lcp-echo-failure={{ppp_options['lcp-echo-failure']}}
-{% else %}
-lcp-echo-failure=3
+{% if ppp_mru %}
+mru={{ ppp_mru }}
 {% endif %}
-{% if ppp_options['ipv4'] %}
-ipv4={{ppp_options['ipv4']}}
+mppe={{ ppp_mppe }}
+lcp-echo-interval={{ ppp_echo_interval }}
+lcp-echo-timeout={{ ppp_echo_timeout }}
+lcp-echo-failure={{ ppp_echo_failure }}
+{% if ppp_ipv4 %}
+ipv4={{ ppp_ipv4 }}
 {% endif %}
 {% if client_ipv6_pool %}
 ipv6=allow
 {% endif %}
 
-{% if ppp_options['ipv6'] %}
-ipv6={{ppp_options['ipv6']}}
-{% if ppp_options['ipv6-intf-id'] %}
-ipv6-intf-id={{ppp_options['ipv6-intf-id']}}
+{% if ppp_ipv6 %}
+ipv6={{ ppp_ipv6 }}
+{% if ppp_ipv6_intf_id %}
+ipv6-intf-id={{ ppp_ipv6_intf_id }}
 {% endif %}
-{% if ppp_options['ipv6-peer-intf-id'] %}
-ipv6-peer-intf-id={{ppp_options['ipv6-peer-intf-id']}}
+{% if ppp_ipv6_peer_intf_id %}
+ipv6-peer-intf-id={{ ppp_ipv6_peer_intf_id }}
 {% endif %}
-{% if ppp_options['ipv6-accept-peer-intf-id'] %}
-ipv6-accept-peer-intf-id={{ppp_options['ipv6-accept-peer-intf-id']}}
+{% if ppp_ipv6_accept_peer_intf_id %}
+ipv6-accept-peer-intf-id={{ ppp_ipv6_accept_peer_intf_id }}
 {% endif %}
 {% endif %}
-mtu={{mtu}}
+mtu={{ mtu }}
 
 [pppoe]
 verbose=1
-{% if concentrator %}
-ac-name={{concentrator}}
-{% endif %}
-{% if interface %}
-{% for int in interface %}
-interface={{int}}
-{% if interface[int]['vlans'] %}
-vlan-mon={{int}},{{interface[int]['vlans']|join(',')}}
-interface=re:{{int}}\.\d+
+ac-name={{ concentrator }}
+
+{% if interfaces %}
+{% for interface in interfaces %}
+interface={{ interface.name }}
+{% if interface.vlans %}
+vlan-mon={{ interface.name }},{{ interface.vlans | join(',') }}
+interface=re:{{ interface.name }}\.\d+
 {% endif %}
 {% endfor -%}
 {% endif -%}
 
 {% if svc_name %}
-service-name={{svc_name|join(',')}}
+service-name={{ svc_name|join(',') }}
 {% endif -%}
 
 {% if pado_delay %}
-pado-delay={{pado_delay}}
+pado-delay={{ pado_delay }}
 {% endif %}
 
-{% if limits %}
+{% if limits_burst or limits_connections or limits_connections %}
 [connlimit]
-limit={{limits['conn-limit']}}
-burst={{limits['burst']}}
-timeout={{limits['timeout']}}
+limit={{ limits_connections }}
+burst={{ limits_burst }}
+timeout={{ limits_timeout }}
 {% endif %}
 
 [cli]
diff --git a/interface-definitions/service-pppoe.xml.in b/interface-definitions/service_pppoe-server.xml.in
index b4950ede1..39f4093a7 100644
--- a/interface-definitions/service-pppoe.xml.in
+++ b/interface-definitions/service_pppoe-server.xml.in
@@ -2,7 +2,7 @@
 <interfaceDefinition>
   <node name="service">
     <children>
-      <node name="pppoe-server" owner="${vyos_conf_scripts_dir}/service-pppoe.py">
+      <node name="pppoe-server" owner="${vyos_conf_scripts_dir}/service_pppoe-server.py">
         <properties>
           <help>Point to Point over Ethernet (PPPoE) Server</help>
           <priority>900</priority>
diff --git a/src/conf_mode/service-pppoe.py b/src/conf_mode/service-pppoe.py
deleted file mode 100755
index aee4ee61b..000000000
--- a/src/conf_mode/service-pppoe.py
+++ /dev/null
@@ -1,390 +0,0 @@
-#!/usr/bin/env python3
-#
-# Copyright (C) 2018-2020 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
-# published by the Free Software Foundation.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-import os
-import re
-
-from sys import exit
-
-from vyos.config import Config
-from vyos import ConfigError
-from vyos.util import call
-from vyos.template import render
-
-chap_secrets = r'/run/accel-pppd/chap-secrets'
-pppoe_conf = r'/run/accel-pppd/pppoe.conf'
-
-def get_config():
-    c = Config()
-    if not c.exists('service pppoe-server'):
-        return None
-
-    config_data = {
-        'concentrator': 'vyos-ac',
-        'authentication': {
-            'local-users': {
-            },
-            'mode': 'local',
-            'radiussrv': {},
-            'radiusopt': {}
-        },
-        'client_ip_pool': '',
-        'client_ip_subnets': [],
-        'client_ipv6_pool': {},
-        'interface': {},
-        'ppp_gw': '',
-        'svc_name': [],
-        'dns': [],
-        'dnsv6': [],
-        'wins': [],
-        'mtu': '1492',
-        'ppp_options': {},
-        'limits': {},
-        'snmp': 'disable',
-        'sesscrtl': 'replace',
-        'pado_delay': ''
-    }
-
-    c.set_level(['service', 'pppoe-server'])
-    # general options
-    if c.exists(['access-concentrator']):
-        config_data['concentrator'] = c.return_value(['access-concentrator'])
-    if c.exists(['service-name']):
-        config_data['svc_name'] = c.return_values(['service-name'])
-    if c.exists(['interface']):
-        for intfc in c.list_nodes(['interface']):
-            config_data['interface'][intfc] = {'vlans': []}
-            if c.exists(['interface', intfc, 'vlan-id']):
-                config_data['interface'][intfc]['vlans'] += c.return_values(
-                    ['interface', intfc, 'vlan-id'])
-            if c.exists(['interface', intfc, 'vlan-range']):
-                config_data['interface'][intfc]['vlans'] += c.return_values(
-                    ['interface', intfc, 'vlan-range'])
-    if c.exists(['local-ip']):
-        config_data['ppp_gw'] = c.return_value(['local-ip'])
-    if c.exists(['dns-servers']):
-        if c.return_value(['dns-servers', 'server-1']):
-            config_data['dns'].append(
-                c.return_value(['dns-servers', 'server-1']))
-        if c.return_value(['dns-servers', 'server-2']):
-            config_data['dns'].append(
-                c.return_value(['dns-servers', 'server-2']))
-    if c.exists(['dnsv6-servers']):
-        if c.return_value(['dnsv6-servers', 'server-1']):
-            config_data['dnsv6'].append(
-                c.return_value(['dnsv6-servers', 'server-1']))
-        if c.return_value(['dnsv6-servers', 'server-2']):
-            config_data['dnsv6'].append(
-                c.return_value(['dnsv6-servers', 'server-2']))
-        if c.return_value(['dnsv6-servers', 'server-3']):
-            config_data['dnsv6'].append(
-                c.return_value(['dnsv6-servers', 'server-3']))
-    if c.exists(['wins-servers']):
-        if c.return_value(['wins-servers', 'server-1']):
-            config_data['wins'].append(
-                c.return_value(['wins-servers', 'server-1']))
-        if c.return_value(['wins-servers', 'server-2']):
-            config_data['wins'].append(
-                c.return_value(['wins-servers', 'server-2']))
-    if c.exists(['client-ip-pool']):
-        if c.exists(['client-ip-pool', 'start']):
-            config_data['client_ip_pool'] = c.return_value(
-                ['client-ip-pool start'])
-            if c.exists(['client-ip-pool stop']):
-                config_data['client_ip_pool'] += '-' + re.search(
-                    '[0-9]+$', c.return_value(['client-ip-pool', 'stop'])).group(0)
-            else:
-                raise ConfigError('client ip pool stop required')
-        if c.exists(['client-ip-pool', 'subnet']):
-            config_data['client_ip_subnets'] = c.return_values(
-                ['client-ip-pool', 'subnet'])
-    if c.exists(['client-ipv6-pool', 'prefix']):
-        config_data['client_ipv6_pool'][
-            'prefix'] = c.return_values(['client-ipv6-pool', 'prefix'])
-        if c.exists(['client-ipv6-pool', 'delegate-prefix']):
-            config_data['client_ipv6_pool']['delegate-prefix'] = c.return_values(
-                ['client-ipv6-pool', 'delegate-prefix'])
-    if c.exists(['limits']):
-        if c.exists(['limits', 'burst']):
-            config_data['limits']['burst'] = str(
-                c.return_value(['limits', 'burst']))
-        if c.exists(['limits', 'timeout']):
-            config_data['limits']['timeout'] = str(
-                c.return_value(['limits', 'timeout']))
-        if c.exists(['limits', 'connection-limit']):
-            config_data['limits']['conn-limit'] = str(
-                c.return_value(['limits', 'connection-limit']))
-    if c.exists(['snmp']):
-        config_data['snmp'] = 'enable'
-    if c.exists(['snmp', 'master-agent']):
-        config_data['snmp'] = 'enable-ma'
-
-    # authentication mode local
-    if not c.exists(['authentication', 'mode']):
-        raise ConfigError('pppoe-server authentication mode required')
-
-    if c.exists(['authentication', 'mode', 'local']):
-        if c.exists(['authentication', 'local-users', 'username']):
-            for usr in c.list_nodes(['authentication', 'local-users', 'username']):
-                config_data['authentication']['local-users'].update(
-                    {
-                        usr: {
-                            'passwd': None,
-                            'state': 'enabled',
-                            'ip': '*',
-                            'upload': None,
-                            'download': None
-                        }
-                    }
-                )
-                if c.exists(['authentication', 'local-users', 'username', usr, 'password']):
-                    config_data['authentication']['local-users'][usr]['passwd'] = c.return_value(
-                        ['authentication', 'local-users', 'username', usr, 'password'])
-                if c.exists(['authentication', 'local-users', 'username', usr, 'disable']):
-                    config_data['authentication'][
-                        'local-users'][usr]['state'] = 'disable'
-                if c.exists(['authentication', 'local-users', 'username', usr, 'static-ip']):
-                    config_data['authentication']['local-users'][usr]['ip'] = c.return_value(
-                        ['authentication', 'local-users', 'username', usr, 'static-ip'])
-                if c.exists(['authentication', 'local-users', 'username', usr, 'rate-limit', 'download']):
-                    config_data['authentication']['local-users'][usr]['download'] = c.return_value(
-                        ['authentication', 'local-users', 'username', usr, 'rate-limit', 'download'])
-                if c.exists(['authentication', 'local-users', 'username', usr, 'rate-limit', 'upload']):
-                    config_data['authentication']['local-users'][usr]['upload'] = c.return_value(
-                        ['authentication', 'local-users', 'username', usr, 'rate-limit', 'upload'])
-
-        # authentication mode radius servers and settings
-
-    if c.exists(['authentication', 'mode', 'radius']):
-        config_data['authentication']['mode'] = 'radius'
-        rsrvs = c.list_nodes(['authentication', 'radius-server'])
-        for rsrv in rsrvs:
-            if c.return_value(['authentication', 'radius-server', rsrv, 'fail-time']) == None:
-                ftime = '0'
-            else:
-                ftime = str(
-                    c.return_value(['authentication', 'radius-server', rsrv, 'fail-time']))
-            if c.return_value(['authentication', 'radius-server', rsrv, 'req-limit']) == None:
-                reql = '0'
-            else:
-                reql = str(
-                    c.return_value(['authentication', 'radius-server', rsrv, 'req-limit']))
-            config_data['authentication']['radiussrv'].update(
-                {
-                    rsrv: {
-                        'secret': c.return_value(['authentication', 'radius-server', rsrv, 'secret']),
-                        'fail-time': ftime,
-                        'req-limit': reql
-                    }
-                }
-            )
-
-        # advanced radius-setting
-        if c.exists(['authentication', 'radius-settings']):
-            if c.exists(['authentication', 'radius-settings', 'acct-timeout']):
-                config_data['authentication']['radiusopt']['acct-timeout'] = c.return_value(
-                    ['authentication', 'radius-settings', 'acct-timeout'])
-            if c.exists(['authentication', 'radius-settings', 'max-try']):
-                config_data['authentication']['radiusopt'][
-                    'max-try'] = c.return_value(['authentication', 'radius-settings', 'max-try'])
-            if c.exists(['authentication', 'radius-settings', 'timeout']):
-                config_data['authentication']['radiusopt'][
-                    'timeout'] = c.return_value(['authentication', 'radius-settings', 'timeout'])
-            if c.exists(['authentication', 'radius-settings', 'nas-identifier']):
-                config_data['authentication']['radiusopt']['nas-id'] = c.return_value(
-                    ['authentication', 'radius-settings', 'nas-identifier'])
-            if c.exists(['authentication', 'radius-settings', 'nas-ip-address']):
-                config_data['authentication']['radiusopt']['nas-ip'] = c.return_value(
-                    ['authentication', 'radius-settings', 'nas-ip-address'])
-            if c.exists(['authentication', 'radius-settings', 'dae-server']):
-                config_data['authentication']['radiusopt'].update(
-                    {
-                        'dae-srv': {
-                            'ip-addr': c.return_value(['authentication', 'radius-settings', 'dae-server', 'ip-address']),
-                            'port': c.return_value(['authentication', 'radius-settings', 'dae-server', 'port']),
-                            'secret': str(c.return_value(['authentication', 'radius-settings', 'dae-server', 'secret']))
-                        }
-                    }
-                )
-            # filter-id is the internal accel default if attribute is empty
-            # set here as default for visibility which may change in the future
-            if c.exists(['authentication', 'radius-settings', 'rate-limit', 'enable']):
-                if not c.exists(['authentication', 'radius-settings', 'rate-limit', 'attribute']):
-                    config_data['authentication']['radiusopt']['shaper'] = {
-                        'attr': 'Filter-Id'
-                    }
-                else:
-                    config_data['authentication']['radiusopt']['shaper'] = {
-                        'attr': c.return_value(['authentication', 'radius-settings', 'rate-limit', 'attribute'])
-                    }
-                if c.exists(['authentication', 'radius-settings', 'rate-limit', 'vendor']):
-                    config_data['authentication']['radiusopt']['shaper'][
-                        'vendor'] = c.return_value(['authentication', 'radius-settings', 'rate-limit', 'vendor'])
-
-    if c.exists(['mtu']):
-        config_data['mtu'] = c.return_value(['mtu'])
-
-    # ppp_options
-    ppp_options = {}
-    if c.exists(['ppp-options']):
-        if c.exists(['ppp-options', 'ccp']):
-            ppp_options['ccp'] = c.return_value(['ppp-options', 'ccp'])
-        if c.exists(['ppp-options', 'min-mtu']):
-            ppp_options['min-mtu'] = c.return_value(['ppp-options', 'min-mtu'])
-        if c.exists(['ppp-options', 'mru']):
-            ppp_options['mru'] = c.return_value(['ppp-options', 'mru'])
-        if c.exists(['ppp-options', 'mppe deny']):
-            ppp_options['mppe'] = 'deny'
-        if c.exists(['ppp-options', 'mppe', 'require']):
-            ppp_options['mppe'] = 'require'
-        if c.exists(['ppp-options', 'mppe', 'prefer']):
-            ppp_options['mppe'] = 'prefer'
-        if c.exists(['ppp-options', 'lcp-echo-failure']):
-            ppp_options['lcp-echo-failure'] = c.return_value(
-                ['ppp-options', 'lcp-echo-failure'])
-        if c.exists(['ppp-options', 'lcp-echo-interval']):
-            ppp_options['lcp-echo-interval'] = c.return_value(
-                ['ppp-options', 'lcp-echo-interval'])
-        if c.exists(['ppp-options', 'ipv4']):
-            ppp_options['ipv4'] = c.return_value(['ppp-options', 'ipv4'])
-        if c.exists(['ppp-options', 'ipv6']):
-            ppp_options['ipv6'] = c.return_value(['ppp-options', 'ipv6'])
-        if c.exists(['ppp-options', 'ipv6-accept-peer-intf-id']):
-            ppp_options['ipv6-accept-peer-intf-id'] = 1
-        if c.exists(['ppp-options', 'ipv6-intf-id']):
-            ppp_options['ipv6-intf-id'] = c.return_value(
-                ['ppp-options', 'ipv6-intf-id'])
-        if c.exists(['ppp-options', 'ipv6-peer-intf-id']):
-            ppp_options['ipv6-peer-intf-id'] = c.return_value(
-                ['ppp-options', 'ipv6-peer-intf-id'])
-        if c.exists(['ppp-options', 'lcp-echo-timeout']):
-            ppp_options['lcp-echo-timeout'] = c.return_value(
-                ['ppp-options', 'lcp-echo-timeout'])
-
-    if len(ppp_options) != 0:
-        config_data['ppp_options'] = ppp_options
-
-    if c.exists(['session-control']):
-        config_data['sesscrtl'] = c.return_value(['session-control'])
-
-    if c.exists(['pado-delay']):
-        config_data['pado_delay'] = '0'
-        a = {}
-        for id in c.list_nodes(['pado-delay']):
-            if not c.return_value(['pado-delay', id, 'sessions']):
-                a[id] = 0
-            else:
-                a[id] = c.return_value(['pado-delay', id, 'sessions'])
-
-        for k in sorted(a.keys()):
-            if k != sorted(a.keys())[-1]:
-                config_data['pado_delay'] += ",{0}:{1}".format(k, a[k])
-            else:
-                config_data['pado_delay'] += ",{0}:{1}".format('-1', a[k])
-
-    return config_data
-
-
-def verify(c):
-    if c == None:
-        return None
-    # vertify auth settings
-    if c['authentication']['mode'] == 'local':
-        if not c['authentication']['local-users']:
-            raise ConfigError(
-                'pppoe-server authentication local-users required')
-
-        for usr in c['authentication']['local-users']:
-            if not c['authentication']['local-users'][usr]['passwd']:
-                raise ConfigError('user ' + usr + ' requires a password')
-            # if up/download is set, check that both have a value
-            if c['authentication']['local-users'][usr]['upload']:
-                if not c['authentication']['local-users'][usr]['download']:
-                    raise ConfigError(
-                        'user ' + usr + ' requires download speed value')
-            if c['authentication']['local-users'][usr]['download']:
-                if not c['authentication']['local-users'][usr]['upload']:
-                    raise ConfigError(
-                        'user ' + usr + ' requires upload speed value')
-
-    if c['authentication']['mode'] == 'radius':
-        if len(c['authentication']['radiussrv']) == 0:
-            raise ConfigError('radius server required')
-        for rsrv in c['authentication']['radiussrv']:
-            if c['authentication']['radiussrv'][rsrv]['secret'] == None:
-                raise ConfigError(
-                    'radius server ' + rsrv + ' needs a secret configured')
-
-    # local ippool and gateway settings config checks
-
-    if c['client_ip_subnets'] or c['client_ip_pool']:
-        if not c['ppp_gw']:
-            raise ConfigError('pppoe-server local-ip required')
-
-    if c['ppp_gw'] and not c['client_ip_subnets'] and not c['client_ip_pool']:
-        print ("Warning: No pppoe client IPv4 pool defined")
-
-
-def generate(c):
-    if not c:
-        return None
-
-    dirname = os.path.dirname(pppoe_conf)
-    if not os.path.exists(dirname):
-        os.mkdir(dirname)
-
-    # accel-cmd reload doesn't work so any change results in a restart of the
-    # daemon
-    try:
-        if os.cpu_count() == 1:
-            c['thread_cnt'] = 1
-        else:
-            c['thread_cnt'] = int(os.cpu_count() / 2)
-    except KeyError:
-        if os.cpu_count() == 1:
-            c['thread_cnt'] = 1
-        else:
-            c['thread_cnt'] = int(os.cpu_count() / 2)
-
-    render(pppoe_conf, 'pppoe-server/pppoe.config.tmpl', c, trim_blocks=True)
-
-    if c['authentication']['local-users']:
-        old_umask = os.umask(0o077)
-        render(chap_secrets, 'pppoe-server/chap-secrets.tmpl', c, trim_blocks=True)
-        os.umask(old_umask)
-
-    return c
-
-
-def apply(c):
-    if not c:
-        call('systemctl stop accel-ppp@pppoe.service')
-        if os.path.exists(pppoe_conf):
-            os.unlink(pppoe_conf)
-
-        return None
-
-    call('systemctl restart accel-ppp@pppoe.service')
-
-if __name__ == '__main__':
-    try:
-        c = get_config()
-        verify(c)
-        generate(c)
-        apply(c)
-    except ConfigError as e:
-        print(e)
-        exit(1)
diff --git a/src/conf_mode/service_pppoe-server.py b/src/conf_mode/service_pppoe-server.py
new file mode 100755
index 000000000..8b7f5a0e9
--- /dev/null
+++ b/src/conf_mode/service_pppoe-server.py
@@ -0,0 +1,427 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2018-2020 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
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import re
+
+from copy import deepcopy
+from stat import S_IRUSR, S_IWUSR, S_IRGRP
+from sys import exit
+
+from vyos.config import Config
+from vyos import ConfigError
+from vyos.util import call
+from vyos.template import render
+
+pppoe_conf = r'/run/accel-pppd/pppoe.conf'
+pppoe_chap_secrets = r'/run/accel-pppd/pppoe.chap-secrets'
+
+default_config_data = {
+    'auth_mode': 'local',
+    'chap_secrets_file': pppoe_chap_secrets, # used in Jinja2 template
+    'client_ip_pool': '',
+    'client_ip_subnets': [],
+    'client_ipv6_pool': {},
+    'concentrator': 'vyos-ac',
+    'interfaces': [],
+    'local_users' : [],
+
+    'svc_name': [],
+    'dns': [],
+    'dnsv6': [],
+    'wins': [],
+    'mtu': '1492',
+
+    'limits_burst': '',
+    'limits_connections': '',
+    'limits_timeout': '',
+
+    'pado_delay': '',
+    'ppp_ccp': False,
+    'ppp_gw': '',
+    'ppp_ipv4': '',
+    'ppp_ipv6': '',
+    'ppp_ipv6_accept_peer_intf_id': False,
+    'ppp_ipv6_intf_id': '',
+    'ppp_ipv6_peer_intf_id': '',
+    'ppp_echo_failure': '3',
+    'ppp_echo_interval': '30',
+    'ppp_echo_timeout': '0',
+    'ppp_min_mtu': '',
+    'ppp_mppe': 'prefer',
+    'ppp_mru': '',
+
+    'radius_server': [],
+    'radius_acct_tmo': '3',
+    'radius_max_try': '3',
+    'radius_timeout': '3',
+    'radius_nas_id': '',
+    'radius_nas_ip': '',
+    'radius_shaper_attr': '',
+    'radius_shaper_vendor': '',
+    'radius_dynamic_author': '',
+    'sesscrtl': 'replace',
+    'snmp': False,
+}
+
+def get_config():
+    conf = Config()
+    base_path = ['service', 'pppoe-server']
+    if not conf.exists(base_path):
+        return None
+
+    conf.set_level(base_path)
+    pppoe = deepcopy(default_config_data)
+
+    cpu = os.cpu_count()
+    if cpu > 1:
+        pppoe['thread_cnt'] = int(cpu/2)
+
+    # general options
+    if conf.exists(['access-concentrator']):
+        pppoe['concentrator'] = conf.return_value(['access-concentrator'])
+
+    if conf.exists(['service-name']):
+        pppoe['svc_name'] = conf.return_values(['service-name'])
+
+    if conf.exists(['interface']):
+        for interface in conf.list_nodes(['interface']):
+            conf.set_level(base_path + ['interface', interface])
+            tmp = {
+                'name': interface,
+                'vlans': []
+            }
+
+            if conf.exists(['vlan-id']):
+                tmp['vlans'] += conf.return_values(['vlan-id'])
+
+            if conf.exists(['vlan-range']):
+                tmp['vlans'] += conf.return_values(['vlan-range'])
+
+            pppoe['interfaces'].append(tmp)
+
+    conf.set_level(base_path)
+
+    if conf.exists(['local-ip']):
+        pppoe['ppp_gw'] = conf.return_value(['local-ip'])
+
+    if conf.exists(['dns-servers']):
+        for server in ['server-1', 'server-2']:
+            if conf.return_value(['dns-servers', server]):
+                tmp = conf.return_value(['dns-servers', server])
+                pppoe['dns'].append(tmp)
+
+
+    if conf.exists(['dnsv6-servers']):
+        for server in ['server-1', 'server-2', 'server-3']:
+            if conf.return_value(['dnsv6-servers', server]):
+                tmp = conf.return_value(['dnsv6-servers', server])
+                pppoe['dnsv6'].append(tmp)
+
+    if conf.exists(['wins-servers']):
+        for server in ['server-1', 'server-2']:
+            if conf.return_value(['wins-servers', server]):
+                tmp = conf.return_value(['wins-servers', server])
+                pppoe['wins'].append(tmp)
+
+    if conf.exists(['client-ip-pool']):
+        if conf.exists(['client-ip-pool', 'start']) and conf.exists(['client-ip-pool', 'stop']):
+            start = conf.return_value(['client-ip-pool', 'start'])
+            stop  = conf.return_value(['client-ip-pool', 'stop'])
+            pppoe['client_ip_pool'] = start + '-' + re.search('[0-9]+$', stop).group(0)
+
+        if conf.exists(['client-ip-pool', 'subnet']):
+            pppoe['client_ip_subnets'] = conf.return_values(['client-ip-pool', 'subnet'])
+
+    if conf.exists(['client-ipv6-pool', 'prefix']):
+        pppoe['client_ipv6_pool']['prefix'] = conf.return_values(['client-ipv6-pool', 'prefix'])
+        if conf.exists(['client-ipv6-pool', 'delegate-prefix']):
+            pppoe['client_ipv6_pool']['delegate-prefix'] = conf.return_values(['client-ipv6-pool', 'delegate-prefix'])
+
+    if conf.exists(['limits']):
+        if conf.exists(['limits', 'burst']):
+            pppoe['limits_burst'] = conf.return_value(['limits', 'burst'])
+
+        if conf.exists(['limits', 'connection-limit']):
+            pppoe['limits_connections'] = conf.return_value(['limits', 'connection-limit'])
+
+        if conf.exists(['limits', 'timeout']):
+            pppoe['limits_timeout'] = conf.return_value(['limits', 'timeout'])
+
+
+    if conf.exists(['snmp']):
+        pppoe['snmp'] = True
+
+    if conf.exists(['snmp', 'master-agent']):
+        pppoe['snmp'] = 'enable-ma'
+
+    # authentication mode local
+    if conf.exists(['authentication', 'mode']):
+        pppoe['auth_mode'] = conf.return_value(['authentication', 'mode'])
+
+    if conf.exists(['authentication', 'local-users']):
+        for username in conf.list_nodes(['authentication', 'local-users', 'username']):
+            user = {
+                'name' : username,
+                'password' : '',
+                'state' : 'enabled',
+                'ip' : '*',
+                'upload' : None,
+                'download' : None
+            }
+            conf.set_level(base_path + ['authentication', 'local-users', 'username', username])
+
+            if conf.exists(['password']):
+                user['password'] = conf.return_value(['password'])
+
+            if conf.exists(['disable']):
+                user['state'] = 'disable'
+
+            if conf.exists(['static-ip']):
+                user['ip'] = conf.return_value(['static-ip'])
+
+            if conf.exists(['rate-limit', 'download']):
+                user['download'] = conf.return_value(['rate-limit', 'download'])
+
+            if conf.exists(['rate-limit', 'upload']):
+                user['upload'] = conf.return_value(['rate-limit', 'upload'])
+
+            pppoe['local_users'].append(user)
+
+    conf.set_level(base_path)
+    #
+    # authentication mode radius servers and settings
+    if conf.exists(['authentication', 'mode', 'radius']):
+
+        for server in conf.list_nodes(['authentication', 'radius-server']):
+            radius = {
+                'server' : server,
+                'key' : '',
+                'fail_time' : 0,
+                'port' : '1812'
+            }
+
+            conf.set_level(base_path + ['authentication', 'radius', 'server', server])
+
+            if conf.exists(['fail-time']):
+                radius['fail-time'] = conf.return_value(['fail-time'])
+
+            if conf.exists(['port']):
+                radius['port'] = conf.return_value(['port'])
+
+            if conf.exists(['secret']):
+                radius['key'] = conf.return_value(['secret'])
+
+            if not conf.exists(['disable']):
+                pppoe['radius_server'].append(radius)
+
+        #
+        # advanced radius-setting
+        conf.set_level(base_path + ['authentication', 'radius-settings'])
+
+        if conf.exists(['acct-timeout']):
+            pppoe['radius_acct_tmo'] = conf.return_value(['acct-timeout'])
+
+        if conf.exists(['max-try']):
+            pppoe['radius_max_try'] = conf.return_value(['max-try'])
+
+        if conf.exists(['timeout']):
+            pppoe['radius_timeout'] = conf.return_value(['timeout'])
+
+        if conf.exists(['nas-identifier']):
+            pppoe['radius_nas_id'] = conf.return_value(['nas-identifier'])
+
+        if conf.exists(['nas-ip-address']):
+            pppoe['radius_nas_ip'] = conf.return_value(['nas-ip-address'])
+
+        # Dynamic Authorization Extensions (DOA)/Change Of Authentication (COA)
+        if conf.exists(['dynamic-author']):
+            dae = {
+                'port' : '',
+                'server' : '',
+                'key' : ''
+            }
+
+            if conf.exists(['dynamic-author', 'ip-address']):
+                dae['server'] = conf.return_value(['dynamic-author', 'ip-address'])
+
+            if conf.exists(['dynamic-author', 'port']):
+                dae['port'] = conf.return_value(['dynamic-author', 'port'])
+
+            if conf.exists(['dynamic-author', 'secret']):
+                dae['key'] = conf.return_value(['dynamic-author', 'secret'])
+
+            pppoe['radius_dynamic_author'] = dae
+
+        # RADIUS based rate-limiter
+        if conf.exists(['rate-limit', 'enable']):
+            pppoe['radius_shaper_attr'] = 'Filter-Id'
+            c_attr = ['rate-limit', 'enable', 'attribute']
+            if conf.exists(c_attr):
+                pppoe['radius_shaper_attr'] = conf.return_value(c_attr)
+
+            c_vendor = ['rate-limit', 'enable', 'vendor']
+            if conf.exists(c_vendor):
+                pppoe['radius_shaper_vendor'] = conf.return_value(c_vendor)
+
+    # re-set config level
+    conf.set_level(base_path)
+
+    if conf.exists(['mtu']):
+        pppoe['mtu'] = conf.return_value(['mtu'])
+
+    if conf.exists(['session-control']):
+        pppoe['session_control'] = conf.return_value(['session-control'])
+
+    # ppp_options
+    if conf.exists(['ppp-options']):
+        conf.set_level(base_path + ['ppp-options'])
+
+        if conf.exists(['ccp']):
+            pppoe['ppp_ccp'] = True
+
+        if conf.exists(['ipv4']):
+            pppoe['ppp_ipv4'] = conf.return_value(['ipv4'])
+
+        if conf.exists(['ipv6']):
+            pppoe['ppp_ipv6'] = conf.return_value(['ipv6'])
+
+        if conf.exists(['ipv6-accept-peer-intf-id']):
+            pppoe['ppp_ipv6_peer_intf_id'] = True
+
+        if conf.exists(['ipv6-intf-id']):
+            pppoe['ppp_ipv6_intf_id'] = conf.return_value(['ipv6-intf-id'])
+
+        if conf.exists(['ipv6-peer-intf-id']):
+            pppoe['ppp_ipv6_peer_intf_id'] = conf.return_value(['ipv6-peer-intf-id'])
+
+        if conf.exists(['lcp-echo-failure']):
+            pppoe['ppp_echo_failure'] = conf.return_value(['lcp-echo-failure'])
+
+        if conf.exists(['lcp-echo-failure']):
+            pppoe['ppp_echo_interval'] = conf.return_value(['lcp-echo-failure'])
+
+        if conf.exists(['lcp-echo-timeout']):
+            pppoe['ppp_echo_timeout'] = conf.return_value(['lcp-echo-timeout'])
+
+        if conf.exists(['min-mtu']):
+            pppoe['ppp_min_mtu'] = conf.return_value(['min-mtu'])
+
+        if conf.exists(['mppe']):
+            pppoe['ppp_mppe'] = conf.return_value(['mppe'])
+
+        if conf.exists(['mru']):
+            pppoe['ppp_mru'] = conf.return_value(['mru'])
+
+    if conf.exists(['pado-delay']):
+        pppoe['pado_delay'] = '0'
+        a = {}
+        for id in conf.list_nodes(['pado-delay']):
+            if not conf.return_value(['pado-delay', id, 'sessions']):
+                a[id] = 0
+            else:
+                a[id] = conf.return_value(['pado-delay', id, 'sessions'])
+
+        for k in sorted(a.keys()):
+            if k != sorted(a.keys())[-1]:
+                pppoe['pado_delay'] += ",{0}:{1}".format(k, a[k])
+            else:
+                pppoe['pado_delay'] += ",{0}:{1}".format('-1', a[k])
+
+    return pppoe
+
+
+def verify(pppoe):
+    if not pppoe:
+        return None
+
+    # vertify auth settings
+    if pppoe['auth_mode'] == 'local':
+        if not pppoe['local_users']:
+            raise ConfigError('PPPoE local auth mode requires local users to be configured!')
+
+        for user in pppoe['local_users']:
+            username = user['name']
+            if not user['password']:
+                raise ConfigError(f'Password required for local user "{username}"')
+
+            # if up/download is set, check that both have a value
+            if user['upload'] and not user['download']:
+                raise ConfigError(f'Download speed value required for local user "{username}"')
+
+            if user['download'] and not user['upload']:
+                raise ConfigError(f'Upload speed value required for local user "{username}"')
+
+    elif pppoe['auth_mode'] == 'radius':
+        if len(pppoe['radius_server']) == 0:
+            raise ConfigError('RADIUS authentication requires at least one server')
+
+        for radius in pppoe['radius_server']:
+            if not radius['key']:
+                server = radius['server']
+                raise ConfigError(f'Missing RADIUS secret key for server "{{ server }}"')
+
+    # local ippool and gateway settings config checks
+    if pppoe['client_ip_subnets'] or pppoe['client_ip_pool']:
+        if not pppoe['ppp_gw']:
+            raise ConfigError('PPPoE server requires local IP to be configured')
+
+    if pppoe['ppp_gw'] and not pppoe['client_ip_subnets'] and not pppoe['client_ip_pool']:
+        print("Warning: No PPPoE client pool defined")
+
+    return None
+
+
+def generate(pppoe):
+    if not pppoe:
+        return None
+
+    dirname = os.path.dirname(pppoe_conf)
+    if not os.path.exists(dirname):
+        os.mkdir(dirname)
+
+    render(pppoe_conf, 'pppoe-server/pppoe.config.tmpl', c, trim_blocks=True)
+
+    if pppoe['local_users']:
+        render(pppoe_chap_secrets, 'pppoe-server/chap-secrets.tmpl', c, trim_blocks=True)
+        os.chmod(pppoe_chap_secrets, S_IRUSR | S_IWUSR | S_IRGRP)
+    else:
+        if os.path.exists(pppoe_chap_secrets):
+             os.unlink(pppoe_chap_secrets)
+
+    return None
+
+
+def apply(pppoe):
+    if not pppoe:
+        call('systemctl stop accel-ppp@pppoe.service')
+        for file in [pppoe_conf, pppoe_chap_secrets]:
+            if os.path.exists(file):
+                os.unlink(file)
+
+        return None
+
+    call('systemctl restart accel-ppp@pppoe.service')
+
+if __name__ == '__main__':
+    try:
+        c = get_config()
+        verify(c)
+        generate(c)
+        apply(c)
+    except ConfigError as e:
+        print(e)
+        exit(1)