Merge ~harald-jensas/cloud-init:bug/1806014 into cloud-init:master

[Harald Jensås]

[Ryan Harper (raharper)]

Thanks for sorting this out. One more element here is related to kernel defaults; at least on Ubuntu the kernel defaults the interfaces accept_ra value to 1; which allows RAs on interfaces that aren't forwarding.

The code below which defaults accept-ra to False is going to break that. I would like to suggest that only *if* accept-ra is set in the source config (for example in the openstack network_data.json where dhcp6 stateful is specified, then it makes sense to include accept-ra: true in the interface config.

In the renderers, if accept-ra is *not* defined, I'd like to avoid emitting any RA related config at all.

Now that cloud-init handles dhcp6-stateless/stateful; I think we only want to enable RA on stateful dhcp6, otherwise we should leave the kernel defaults.

Additionally, ifupdown (eni) can toggle these values as well:

accept_ra [0, 1, 2]
autoconf [0, 1]

review: Needs Fixing

b8ddae4... by Harald Jensås on 2019-10-18

net: fix subnet_is_ipv6() for stateless|stateful

Function return false for ipv6_dhcpv6-stateless|stateful,
the eni renderer does not add '6' to 'inet' which is
incorrect.

The subnet_is_ipv6() function is updated to also return
true if startswith('ipv6').

LP: #1848690

[Harald Jensås (harald-jensas)]

> Thanks for sorting this out. One more element here is related to kernel
> defaults; at least on Ubuntu the kernel defaults the interfaces accept_ra
> value to 1; which allows RAs on interfaces that aren't forwarding.
>
> The code below which defaults accept-ra to False is going to break that. I
> would like to suggest that only *if* accept-ra is set in the source config
> (for example in the openstack network_data.json where dhcp6 stateful is
> specified, then it makes sense to include accept-ra: true in the interface
> config.
>
> In the renderers, if accept-ra is *not* defined, I'd like to avoid emitting
> any RA related config at all.
>
> Now that cloud-init handles dhcp6-stateless/stateful; I think we only want to
> enable RA on stateful dhcp6, otherwise we should leave the kernel defaults.
>
> Additionally, ifupdown (eni) can toggle these values as well:
>
> accept_ra [0, 1, 2]
> autoconf [0, 1]

Thanks Ryan, this all makes sense.
I have updated the patch to not just enable if accept-ra: true. But also disable if false. And it should not change the OS defaults if accept-ra is not defined at all.

Unmerged commits

577ccfa... by Harald Jensås on 2019-10-14

net: IPv6 implement accept-ra RA's

Router advertisements are required for the default route
to be set up.

sysconf: IPV6_FORCE_ACCEPT_RA controls accept_ra sysctl.
eni: mode static and mode dhcp 'accept_ra' controls sysctl.

Add 'accept-ra: true|false' parameter to config v1 and
v2. When True: accept_ra is set to '1'. When False:
accept_ra is set to '0'. When not defined in config the
value is left to the operating system default.

Also, set IPV6_FORCE_ACCEPT_RA=yes in sysconfig when
using Openstack and dhcpv6-stateful.

LP: #1806014
LP: #1808647

b8ddae4... by Harald Jensås on 2019-10-18

net: fix subnet_is_ipv6() for stateless|stateful

Function return false for ipv6_dhcpv6-stateless|stateful,
the eni renderer does not add '6' to 'inet' which is
incorrect.

The subnet_is_ipv6() function is updated to also return
true if startswith('ipv6').

LP: #1848690

diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
index 530922b..458c229 100644
--- a/cloudinit/net/eni.py
+++ b/cloudinit/net/eni.py
@@ -399,6 +399,7 @@ class Renderer(renderer.Renderer):
     def _render_iface(self, iface, render_hwaddress=False):
         sections = []
         subnets = iface.get('subnets', {})
+        accept_ra = iface.pop('accept-ra', None)
         if subnets:
             for index, subnet in enumerate(subnets):
                 ipv4_subnet_mtu = None
@@ -415,9 +416,18 @@ class Renderer(renderer.Renderer):
                         subnet['type'] == 'ipv6_dhcpv6-stateful'):
                     # Configure network settings using DHCP or DHCPv6
                     iface['mode'] = 'dhcp'
+                    if accept_ra is not None:
+                        # Accept router advertisements (0=off, 1=on)
+                        iface['accept_ra'] = ('1' if util.is_true(accept_ra)
+                                              else '0')
                 elif subnet['type'] == 'ipv6_dhcpv6-stateless':
                     # Configure network settings using SLAAC from RAs
                     iface['mode'] = 'auto'
+                elif subnet_is_ipv6(subnet) and subnet['type'] == 'static':
+                    if accept_ra is not None:
+                        # Accept router advertisements (0=off, 1=on)
+                        iface['accept_ra'] = ('1' if util.is_true(accept_ra)
+                                              else '0')
 
                 # do not emit multiple 'auto $IFACE' lines as older (precise)
                 # ifupdown complains
diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
index e54a34e..78023f8 100644
--- a/cloudinit/net/netplan.py
+++ b/cloudinit/net/netplan.py
@@ -51,7 +51,8 @@ def _extract_addresses(config, entry, ifname):
          'mtu': 1480,
          'netmask': 64,
          'type': 'static'}],
-      'type: physical'
+      'type: physical',
+      'accept-ra': 'true'
     }
 
     An entry dictionary looks like:
@@ -144,6 +145,8 @@ def _extract_addresses(config, entry, ifname):
         ns = entry.get('nameservers', {})
         ns.update({'search': searchdomains})
         entry.update({'nameservers': ns})
+    if 'accept-ra' in config:
+        entry.update({'accept-ra': config.get('accept-ra')})
 
 
 def _extract_bond_slaves_by_name(interfaces, entry, bond_master):
diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
index c0c415d..cc8beaa 100644
--- a/cloudinit/net/network_state.py
+++ b/cloudinit/net/network_state.py
@@ -22,7 +22,8 @@ NETWORK_STATE_REQUIRED_KEYS = {
 }
 NETWORK_V2_KEY_FILTER = [
     'addresses', 'dhcp4', 'dhcp6', 'gateway4', 'gateway6', 'interfaces',
-    'match', 'mtu', 'nameservers', 'renderer', 'set-name', 'wakeonlan'
+    'match', 'mtu', 'nameservers', 'renderer', 'set-name', 'wakeonlan',
+    'accept-ra'
 ]
 
 NET_CONFIG_TO_V2 = {
@@ -340,7 +341,8 @@ class NetworkStateInterpreter(object):
             'name': 'eth0',
             'subnets': [
                 {'type': 'dhcp4'}
-             ]
+             ],
+            'accept-ra': 'true'
         }
         '''
 
@@ -370,6 +372,7 @@ class NetworkStateInterpreter(object):
             'address': None,
             'gateway': None,
             'subnets': subnets,
+            'accept-ra': command.get('accept-ra')
         })
         self._network_state['interfaces'].update({command.get('name'): iface})
         self.dump_network_state()
@@ -613,6 +616,7 @@ class NetworkStateInterpreter(object):
               driver: ixgbe
             set-name: lom1
             dhcp6: true
+            accept-ra: true
           switchports:
             match:
               name: enp2*
@@ -641,7 +645,7 @@ class NetworkStateInterpreter(object):
             driver = match.get('driver', None)
             if driver:
                 phy_cmd['params'] = {'driver': driver}
-            for key in ['mtu', 'match', 'wakeonlan']:
+            for key in ['mtu', 'match', 'wakeonlan', 'accept-ra']:
                 if key in cfg:
                     phy_cmd[key] = cfg[key]
 
@@ -919,7 +923,7 @@ def is_ipv6_addr(address):
 def subnet_is_ipv6(subnet):
     """Common helper for checking network_state subnets for ipv6."""
     # 'static6' or 'dhcp6'
-    if subnet['type'].endswith('6'):
+    if subnet['type'].endswith('6') or subnet['type'].startswith('ipv6'):
         # This is a request for DHCPv6.
         return True
     elif subnet['type'] == 'static' and is_ipv6_addr(subnet.get('address')):
diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
index 4e65676..3fe5793 100644
--- a/cloudinit/net/sysconfig.py
+++ b/cloudinit/net/sysconfig.py
@@ -333,6 +333,9 @@ class Renderer(renderer.Renderer):
                 if old_key == 'mac_address' and iface['type'] != 'physical':
                     continue
                 iface_cfg[new_key] = old_value
+        accept_ra = iface.get('accept-ra', None)
+        if accept_ra is not None:
+            iface_cfg['IPV6_FORCE_ACCEPT_RA'] = util.is_true(accept_ra)
 
     @classmethod
     def _render_subnets(cls, iface_cfg, subnets, has_default_route):
@@ -343,11 +346,16 @@ class Renderer(renderer.Renderer):
         for i, subnet in enumerate(subnets, start=len(iface_cfg.children)):
             mtu_key = 'MTU'
             subnet_type = subnet.get('type')
-            if subnet_type == 'dhcp6' or subnet_type == 'ipv6_dhcpv6-stateful':
+            if subnet_type == 'dhcp6':
                 # TODO need to set BOOTPROTO to dhcp6 on SUSE
                 iface_cfg['IPV6INIT'] = True
                 # Configure network settings using DHCPv6
                 iface_cfg['DHCPV6C'] = True
+            elif subnet_type == 'ipv6_dhcpv6-stateful':
+                iface_cfg['IPV6INIT'] = True
+                # Configure network settings using DHCPv6
+                iface_cfg['DHCPV6C'] = True
+                iface_cfg['IPV6_FORCE_ACCEPT_RA'] = True
             elif subnet_type == 'ipv6_dhcpv6-stateless':
                 iface_cfg['IPV6INIT'] = True
                 # Configure network settings using SLAAC from RAs
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index f5a9cae..62b6ac6 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -1070,6 +1070,104 @@ NETWORK_CONFIGS = {
                 """),
         },
     },
+    'dhcpv6_accept_ra': {
+        'expected_eni': textwrap.dedent("""\
+            auto lo
+            iface lo inet loopback
+
+            auto iface0
+            iface iface0 inet6 dhcp
+                accept_ra 1
+        """).rstrip(' '),
+        'expected_netplan': textwrap.dedent("""
+            network:
+                version: 2
+                ethernets:
+                    iface0:
+                        accept-ra: 'true'
+                        dhcp6: true
+        """).rstrip(' '),
+        'yaml_v1': textwrap.dedent("""\
+            version: 1
+            config:
+              - type: 'physical'
+                name: 'iface0'
+                subnets:
+                - {'type': 'dhcp6'}
+                accept-ra: 'true'
+        """).rstrip(' '),
+        'yaml_v2': textwrap.dedent("""\
+            version: 2
+            ethernets:
+                iface0:
+                    dhcp6: true
+                    accept-ra: 'true'
+        """).rstrip(' '),
+        'expected_sysconfig': {
+            'ifcfg-iface0': textwrap.dedent("""\
+                BOOTPROTO=none
+                DEVICE=iface0
+                DHCPV6C=yes
+                IPV6INIT=yes
+                IPV6_FORCE_ACCEPT_RA=yes
+                DEVICE=iface0
+                NM_CONTROLLED=no
+                ONBOOT=yes
+                STARTMODE=auto
+                TYPE=Ethernet
+                USERCTL=no
+            """),
+        },
+    },
+    'dhcpv6_reject_ra': {
+        'expected_eni': textwrap.dedent("""\
+            auto lo
+            iface lo inet loopback
+
+            auto iface0
+            iface iface0 inet6 dhcp
+                accept_ra 0
+        """).rstrip(' '),
+        'expected_netplan': textwrap.dedent("""
+            network:
+                version: 2
+                ethernets:
+                    iface0:
+                        accept-ra: 'false'
+                        dhcp6: true
+        """).rstrip(' '),
+        'yaml_v1': textwrap.dedent("""\
+            version: 1
+            config:
+            - type: 'physical'
+              name: 'iface0'
+              subnets:
+              - {'type': 'dhcp6'}
+              accept-ra: 'false'
+        """).rstrip(' '),
+        'yaml_v2': textwrap.dedent("""\
+            version: 2
+            ethernets:
+                iface0:
+                    dhcp6: true
+                    accept-ra: 'false'
+        """).rstrip(' '),
+        'expected_sysconfig': {
+            'ifcfg-iface0': textwrap.dedent("""\
+                BOOTPROTO=none
+                DEVICE=iface0
+                DHCPV6C=yes
+                IPV6INIT=yes
+                IPV6_FORCE_ACCEPT_RA=no
+                DEVICE=iface0
+                NM_CONTROLLED=no
+                ONBOOT=yes
+                STARTMODE=auto
+                TYPE=Ethernet
+                USERCTL=no
+            """),
+        },
+    },
     'dhcpv6_stateless': {
         'expected_eni': textwrap.dedent("""\
         auto lo
@@ -1137,6 +1235,7 @@ NETWORK_CONFIGS = {
             DEVICE=iface0
             DHCPV6C=yes
             IPV6INIT=yes
+            IPV6_FORCE_ACCEPT_RA=yes
             DEVICE=iface0
             NM_CONTROLLED=no
             ONBOOT=yes
@@ -2857,6 +2956,34 @@ USERCTL=no
         self._compare_files_to_expected(entry[self.expected_name], found)
         self._assert_headers(found)
 
+    def test_dhcpv6_accept_ra_config_v1(self):
+        entry = NETWORK_CONFIGS['dhcpv6_accept_ra']
+        found = self._render_and_read(network_config=yaml.load(
+            entry['yaml_v1']))
+        self._compare_files_to_expected(entry[self.expected_name], found)
+        self._assert_headers(found)
+
+    def test_dhcpv6_accept_ra_config_v2(self):
+        entry = NETWORK_CONFIGS['dhcpv6_accept_ra']
+        found = self._render_and_read(network_config=yaml.load(
+            entry['yaml_v2']))
+        self._compare_files_to_expected(entry[self.expected_name], found)
+        self._assert_headers(found)
+
+    def test_dhcpv6_reject_ra_config_v1(self):
+        entry = NETWORK_CONFIGS['dhcpv6_reject_ra']
+        found = self._render_and_read(network_config=yaml.load(
+            entry['yaml_v1']))
+        self._compare_files_to_expected(entry[self.expected_name], found)
+        self._assert_headers(found)
+
+    def test_dhcpv6_reject_ra_config_v2(self):
+        entry = NETWORK_CONFIGS['dhcpv6_reject_ra']
+        found = self._render_and_read(network_config=yaml.load(
+            entry['yaml_v2']))
+        self._compare_files_to_expected(entry[self.expected_name], found)
+        self._assert_headers(found)
+
     def test_dhcpv6_stateless_config(self):
         entry = NETWORK_CONFIGS['dhcpv6_stateless']
         found = self._render_and_read(network_config=yaml.load(entry['yaml']))
@@ -3918,6 +4045,22 @@ class TestNetplanRoundTrip(CiTestCase):
             entry['expected_netplan'].splitlines(),
             files['/etc/netplan/50-cloud-init.yaml'].splitlines())
 
+    def testsimple_render_dhcpv6_accept_ra(self):
+        entry = NETWORK_CONFIGS['dhcpv6_accept_ra']
+        files = self._render_and_read(network_config=yaml.load(
+            entry['yaml_v1']))
+        self.assertEqual(
+            entry['expected_netplan'].splitlines(),
+            files['/etc/netplan/50-cloud-init.yaml'].splitlines())
+
+    def testsimple_render_dhcpv6_reject_ra(self):
+        entry = NETWORK_CONFIGS['dhcpv6_reject_ra']
+        files = self._render_and_read(network_config=yaml.load(
+            entry['yaml_v1']))
+        self.assertEqual(
+            entry['expected_netplan'].splitlines(),
+            files['/etc/netplan/50-cloud-init.yaml'].splitlines())
+
     def testsimple_render_all(self):
         entry = NETWORK_CONFIGS['all']
         files = self._render_and_read(network_config=yaml.load(entry['yaml']))
@@ -4048,6 +4191,38 @@ class TestEniRoundTrip(CiTestCase):
             entry['expected_eni'].splitlines(),
             files['/etc/network/interfaces'].splitlines())
 
+    def testsimple_render_dhcpv6_stateless(self):
+        entry = NETWORK_CONFIGS['dhcpv6_stateless']
+        files = self._render_and_read(network_config=yaml.load(
+            entry['yaml']))
+        self.assertEqual(
+            entry['expected_eni'].splitlines(),
+            files['/etc/network/interfaces'].splitlines())
+
+    def testsimple_render_dhcpv6_stateful(self):
+        entry = NETWORK_CONFIGS['dhcpv6_stateless']
+        files = self._render_and_read(network_config=yaml.load(
+            entry['yaml']))
+        self.assertEqual(
+            entry['expected_eni'].splitlines(),
+            files['/etc/network/interfaces'].splitlines())
+
+    def testsimple_render_dhcpv6_accept_ra(self):
+        entry = NETWORK_CONFIGS['dhcpv6_accept_ra']
+        files = self._render_and_read(network_config=yaml.load(
+            entry['yaml_v1']))
+        self.assertEqual(
+            entry['expected_eni'].splitlines(),
+            files['/etc/network/interfaces'].splitlines())
+
+    def testsimple_render_dhcpv6_reject_ra(self):
+        entry = NETWORK_CONFIGS['dhcpv6_reject_ra']
+        files = self._render_and_read(network_config=yaml.load(
+            entry['yaml_v1']))
+        self.assertEqual(
+            entry['expected_eni'].splitlines(),
+            files['/etc/network/interfaces'].splitlines())
+
     def testsimple_render_manual(self):
         """Test rendering of 'manual' for 'type' and 'control'.