tunnel: T3173: path MTU discovery option should be valueless

3 files changed, 136 insertions, 80 deletions

diff --git a/interface-definitions/interfaces-tunnel.xml.in b/interface-definitions/interfaces-tunnel.xml.in
index 273aa648e..39e274840 100644
--- a/interface-definitions/interfaces-tunnel.xml.in
+++ b/interface-definitions/interfaces-tunnel.xml.in
@@ -177,44 +177,42 @@
                   <help>IPv4 specific tunnel parameters</help>
                 </properties>
                 <children>
-                  <leafNode name="pmtu-discovery">
+                  <leafNode name="no-pmtu-discovery">
                     <properties>
-                      <help>Path MTU Discovery parameters</help>
-                      <completionHelp>
-                        <list>disable enable</list>
-                      </completionHelp>
-                      <valueHelp>
-                        <format>disable</format>
-                        <description>Disable Path MTU Discovery for tunnel</description>
-                      </valueHelp>
-                      <valueHelp>
-                        <format>enable</format>
-                        <description>Enable Path MTU Discovery for tunnel (by default)</description>
-                      </valueHelp>
-                      <constraint>
-                        <regex>^(disable|enable)$</regex>
-                      </constraint>
+                      <help>Disable path MTU discovery</help>
+                      <valueless/>
                     </properties>
                   </leafNode>
                   <leafNode name="ttl">
                     <properties>
-                      <help>Time to live field</help>
+                      <help>Time to live (default: 0)</help>
                       <valueHelp>
-                        <format>0-255</format>
-                        <description>Time to live (default 255)</description>
+                        <format>0</format>
+                        <description>Copy value from original IP header</description>
+                      </valueHelp>
+                      <valueHelp>
+                        <format>1-255</format>
+                        <description>Time to Live</description>
                       </valueHelp>
                       <constraint>
                         <validator name="numeric" argument="--range 0-255"/>
                       </constraint>
                       <constraintErrorMessage>TTL must be between 0 and 255</constraintErrorMessage>
                     </properties>
-                    <defaultValue>255</defaultValue>
+                    <defaultValue>0</defaultValue>
                   </leafNode>
                   <leafNode name="tos">
                     <properties>
-                      <help>Type of Service (TOS)</help>
+                      <help>Type of Service (default: 0)</help>
+                      <completionHelp>
+                        <list>inherit</list>
+                      </completionHelp>
+                      <valueHelp>
+                        <format>0</format>
+                        <description>Copy value from original IP header</description>
+                      </valueHelp>
                       <valueHelp>
-                        <format>0-99</format>
+                        <format>1-99</format>
                         <description>Type of Service (TOS)</description>
                       </valueHelp>
                       <constraint>
@@ -222,7 +220,7 @@
                       </constraint>
                       <constraintErrorMessage>TOS must be between 0 and 99</constraintErrorMessage>
                     </properties>
-                    <defaultValue>inherit</defaultValue>
+                    <defaultValue>0</defaultValue>
                   </leafNode>
                   <leafNode name="key">
                     <properties>
@@ -234,7 +232,7 @@
                       <constraint>
                         <validator name="numeric" argument="--range 0-4294967295"/>
                       </constraint>
-                      <constraintErrorMessage>key must be between 0-4294967295</constraintErrorMessage>
+                      <constraintErrorMessage>Key must be in range 0-4294967295</constraintErrorMessage>
                     </properties>
                   </leafNode>
                 </children>
@@ -249,7 +247,7 @@
                       <help>Encaplimit field</help>
                       <valueHelp>
                         <format>0-255</format>
-                        <description>Encaplimit (default 4)</description>
+                        <description>Encaplimit (default: 4)</description>
                       </valueHelp>
                       <constraint>
                         <validator name="numeric" argument="--range 0-255"/>
diff --git a/python/vyos/ifconfig/tunnel.py b/python/vyos/ifconfig/tunnel.py
index 1af4f8e72..7e3f9565a 100644
--- a/python/vyos/ifconfig/tunnel.py
+++ b/python/vyos/ifconfig/tunnel.py
@@ -48,31 +48,42 @@ class _Tunnel(Interface):
         },
     }
 
+    default = {
+        'local' : '',
+        'remote': '',
+        'dev' : '',
+        'ttl' : '',
+        'tos' : '',
+        'key' : '',
+    }
+    options = Interface.options + list(default.keys())
+
     # TODO: This is surely used for more than tunnels
     # TODO: could be refactored elsewhere
-    _command_set = {**Interface._command_set, **{
-        'multicast': {
-            'validate': lambda v: assert_list(v, ['enable', 'disable']),
-            'convert': enable_to_on,
-            'shellcmd': 'ip link set dev {ifname} multicast {value}',
-        },
-        'allmulticast': {
-            'validate': lambda v: assert_list(v, ['enable', 'disable']),
-            'convert': enable_to_on,
-            'shellcmd': 'ip link set dev {ifname} allmulticast {value}',
-        },
-    }}
-
+    _command_set = {
+        **Interface._command_set,
+        **{
+            'multicast': {
+                'validate': lambda v: assert_list(v, ['enable', 'disable']),
+                'convert': enable_to_on,
+                'shellcmd': 'ip link set dev {ifname} multicast {value}',
+            },
+            'allmulticast': {
+                'validate': lambda v: assert_list(v, ['enable', 'disable']),
+                'convert': enable_to_on,
+                'shellcmd': 'ip link set dev {ifname} allmulticast {value}',
+            },
+        }
+    }
     _create_cmd = 'ip tunnel add {ifname} mode {type}'
 
-    def __init__(self, ifname, **config):
-        self.config = deepcopy(config) if config else {}
-        super().__init__(ifname, **config)
-
     def _create(self):
         # add " option-name option-name-value ..." for all options set
-        options = " ".join(["{} {}".format(k, self.config[k])
-                            for k in self.options if k in self.config and self.config[k]])
+        options = ' '.join(['{} {}'.format(k, self.config[k])
+                            for k,v in self.config.items() if v and k not in
+                                                    ['ifname', 'type', 'raw']])
+        if 'raw' in self.config:
+            options += ' ' + ' '.join(self.config['raw'])
         self._cmd('{} {}'.format(self._create_cmd.format(**self.config), options))
         self.set_admin_state('down')
 
@@ -80,14 +91,13 @@ class _Tunnel(Interface):
         change = 'ip tunnel change {ifname} mode {type}'
 
         # add " option-name option-name-value ..." for all options set
-        options = " ".join(["{} {}".format(k, self.config[k])
-                            for k in self.options if k in self.config and self.config[k]])
+        options = ' '.join(['{} {}'.format(k, self.config[k])
+                            for k,v in self.config.items() if v and k not in
+                                                    ['ifname', 'type', 'raw']])
+        if 'raw' in self.config:
+            options += ' ' + ' '.join(self.config['raw'])
         self._cmd('{} {}'.format(change.format(**self.config), options))
 
-    @classmethod
-    def get_config(cls):
-        return dict(zip(cls.options, ['']*len(cls.options)))
-
     def get_mac(self):
         """
         Get current interface MAC (Media Access Contrl) address used.
@@ -141,8 +151,13 @@ class GREIf(_Tunnel):
     https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/ip/link_gre.c
     """
 
-    default = {'type': 'gre'}
-    options = ['local', 'remote', 'dev', 'ttl', 'tos', 'key']
+    default = {
+        **_Tunnel.default,
+        **{
+            'type': 'gre',
+            'raw' : ['pmtudisc'], # parameters that we can pass raw to ip command
+        },
+    }
 
 # GreTap also called GRE Bridge
 class GRETapIf(_Tunnel):
@@ -151,18 +166,20 @@ class GRETapIf(_Tunnel):
 
     https://en.wikipedia.org/wiki/TUN/TAP
     """
-
     # no multicast, ttl or tos for gretap
-
     definition = {
         **_Tunnel.definition,
         **{
             'bridgeable': True,
         },
     }
-
-    default = {'type': 'gretap'}
-    options = ['local', 'remote', 'ttl',]
+    default = {
+        'type': 'gretap',
+        'local': '',
+        'remote': '',
+        'dev': '',
+        'raw' : ['pmtudisc'], # parameters that we can pass raw to ip command
+    }
 
     _create_cmd = 'ip link add name {ifname} type {type}'
 
@@ -177,10 +194,16 @@ class IP6GREIf(_Tunnel):
     https://tools.ietf.org/html/rfc7676
     https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/ip/link_gre6.c
     """
-
-    default = {'type': 'ip6gre'}
-    options = ['local', 'remote', 'dev', 'encaplimit',
-               'hoplimit', 'tclass', 'flowlabel']
+    default = {
+        **_Tunnel.default,
+        **{
+            'type': 'ip6gre',
+            'encaplimit': '',
+            'hoplimit': '',
+            'tclass': '',
+            'flowlabel': '',
+        },
+    }
 
 class IPIPIf(_Tunnel):
     """
@@ -189,12 +212,15 @@ class IPIPIf(_Tunnel):
     For more information please refer to:
     https://tools.ietf.org/html/rfc2003
     """
-
     # IPIP does not allow to pass multicast, unlike GRE
     # but the interface itself can be set with multicast
-
-    default = {'type': 'ipip'}
-    options = ['local', 'remote', 'dev', 'ttl', 'tos', 'key']
+    default = {
+        **_Tunnel.default,
+        **{
+            'type': 'ipip',
+            'raw' : ['pmtudisc'], # parameters that we can pass raw to ip command
+        },
+    }
 
 class IPIP6If(_Tunnel):
     """
@@ -203,10 +229,16 @@ class IPIP6If(_Tunnel):
     For more information please refer to:
     https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/ip/link_ip6tnl.c
     """
-
-    default = {'type': 'ipip6'}
-    options = ['local', 'remote', 'dev', 'encaplimit',
-               'hoplimit', 'tclass', 'flowlabel']
+    default = {
+        **_Tunnel.default,
+        **{
+            'type': 'ipip6',
+            'encaplimit': '',
+            'hoplimit': '',
+            'tclass': '',
+            'flowlabel': '',
+        },
+    }
 
 class IP6IP6If(IPIP6If):
     """
@@ -215,7 +247,12 @@ class IP6IP6If(IPIP6If):
     For more information please refer to:
     https://tools.ietf.org/html/rfc2473
     """
-    default = {'type': 'ip6ip6'}
+    default = {
+        **_Tunnel.default,
+        **{
+            'type': 'ip6ip6',
+        },
+    }
 
 
 class SitIf(_Tunnel):
@@ -225,9 +262,12 @@ class SitIf(_Tunnel):
     For more information please refer to:
     https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/ip/link_iptnl.c
     """
-
-    default = {'type': 'sit'}
-    options = ['local', 'remote', 'dev', 'ttl', 'tos', 'key']
+    default = {
+        **_Tunnel.default,
+        **{
+            'type': 'sit',
+        },
+    }
 
 class Sit6RDIf(SitIf):
     """
@@ -236,7 +276,14 @@ class Sit6RDIf(SitIf):
     https://en.wikipedia.org/wiki/IPv6_rapid_deployment
     """
     # TODO: check if key can really be used with 6RD
-    options = ['remote', 'ttl', 'tos', 'key', '6rd-prefix', '6rd-relay-prefix']
+    default = {
+        **_Tunnel.default,
+        **{
+            'type': '6rd',
+            '6rd_prefix' : '',
+            '6rd_relay_prefix' : '',
+        },
+    }
 
     def _create(self):
         # do not call _Tunnel.create, building fully here
diff --git a/src/conf_mode/interfaces-tunnel.py b/src/conf_mode/interfaces-tunnel.py
index f79405388..d2fcf3121 100755
--- a/src/conf_mode/interfaces-tunnel.py
+++ b/src/conf_mode/interfaces-tunnel.py
@@ -126,10 +126,14 @@ def verify(tunnel):
     if 'source_interface' in tunnel:
         verify_interface_exists(tunnel['source_interface'])
 
-    # ttl != 0 and nopmtudisc are incompatible
-    if 'pmtu_discovery' in tunnel['parameters']['ip']:
-        if 'disable' in tunnel['parameters']['ip']['pmtu_discovery'] and "0" not in tunnel['parameters']['ip']['ttl']:
-            raise ConfigError('ip ttl should be set to "0" with option "pmtu-discovery disable"')
+    # TTL != 0 and nopmtudisc are incompatible, parameters and ip use default
+    # values, thus the keys are always present.
+    if dict_search('parameters.ip.no_pmtu_discovery', tunnel) != None:
+        if dict_search('parameters.ip.ttl', tunnel) != '0':
+            raise ConfigError('Disabled PMTU requires TTL set to "0"!')
+        if tunnel['encapsulation'] in ['ipip6', 'ip6ip6', 'ip6gre']:
+            raise ConfigError('Can not disable PMTU discovery for given encapsulation')
+
 
 def generate(tunnel):
     return None
@@ -169,7 +173,6 @@ def apply(tunnel):
         'local_ip'                   : 'local',
         'remote_ip'                  : 'remote',
         'source_interface'           : 'dev',
-        'parameters.ip.pmtu_discovery' : 'pmtud',
         'parameters.ip.ttl'          : 'ttl',
         'parameters.ip.tos'          : 'tos',
         'parameters.ip.key'          : 'key',
@@ -180,7 +183,10 @@ def apply(tunnel):
     if tunnel['encapsulation'] in ['ipip6', 'ip6ip6', 'ip6gre']:
         mappingv6 = {
             # this                       :  get_config()
-            'parameters.ipv6.encaplimit' : 'encaplimit'
+            'parameters.ipv6.encaplimit' : 'encaplimit',
+            'parameters.ipv6.flowlabel'  : 'flowlabel',
+            'parameters.ipv6.hoplimit'   : 'hoplimit',
+            'parameters.ipv6.tclass'     : 'flowlabel'
         }
         mapping.update(mappingv6)
 
@@ -188,6 +194,11 @@ def apply(tunnel):
         if dict_search(our_key, tunnel) and their_key in conf:
             conf[their_key] = dict_search(our_key, tunnel)
 
+    if dict_search('parameters.ip.no_pmtu_discovery', tunnel) != None:
+        if 'pmtudisc' in conf['raw']:
+            conf['raw'].remove('pmtudisc')
+        conf['raw'].append('nopmtudisc')
+
     tun = klass(tunnel['ifname'], **conf)
     tun.change_options()
     tun.update(tunnel)