Merge lp:~smoser/cloud-init/trunk.net-improve-lo-dns into lp:~cloud-init-dev/cloud-init/trunk

[Scott Moser]

improvements to eni rendering

Some improvements here, and some bug fixes.
 - bring curtin revno 394's change to support post-up for interface aliases.
 - sort attributes per interface for nicer order and consistent rendering
 - use arrays for each 'section' rather than content += . This allows better
   separation of the sections and also will perform better as long strings
   with += are slow.
 - improve how 'lo' is handled. If a network state that was being rendered
   had an entry for 'lo', then the rendered ENI would have 2 'lo' sections.
 - no longer skip 'lo' sections when loading an ENI in parse_deb_config
 - fix inet value for subnets, don't add interface attributes to alias
   (LP: #1588547)

Also add some tests of reading yaml and rendering ENI.

[lp:~smoser/cloud-init/trunk.net-improve-lo-dns]

1261. By Scott Moser on 2016-06-21

fix english in comment

1262. By Scott Moser on 2016-06-21

net: fix inet value for subnets, don't add interface attributes to alias

[copied from curtin revno 390]
Apply two separate fixes for configuring bonding with ip aliases.

Curtin re-used the interface's inet value for each subnet that might
be configured. In the case where the configuration included an ipv4
address after an ipv6 one resulted in emitting 'inet6' for ipv4 address
which is not correct. Resolve this issue by calculating the inet
value independent of the current status of the iface, using the subnet
config instead.

When rendering a network_config which includes ip alias interfaces
do not emit any attributes, like MTU, or bond/bridge options Including
these values is almost always wrong or will result in confusing
behavior on the target system.

1263. By Scott Moser on 2016-06-21

make 2 of 3 tests pass

1264. By Scott Moser on 2016-06-21

fix flake8

1265. By Scott Moser on 2016-06-21

no longer skip 'lo' elements when reading. pass 3rd test.

[Scott Moser (smoser)]

passes tox and the 3 added tests now.
The tests are somewhat brittle as they expect exactly rendered ENI content for the provided network yaml.

[lp:~smoser/cloud-init/trunk.net-improve-lo-dns]

1266. By Scott Moser on 2016-07-14

merge with trunk

1267. By Scott Moser on 2016-07-14

merge from trunk.lp1602373

this merges in the render_hwaddress support.
newly added tests still run, so hwaddress seems correctly getting in.

1268. By Scott Moser on 2016-07-14

merge from trunk

[Ryan Harper (raharper)]

> - use arrays for each 'section' rather than content += . This allows better
> separation of the sections and also will perform better as long strings
> with += are slow.

+1 for better separation. I don't think there is significant difference:

For an 100-line eni (that's rather high for typical usage)

% python3 t.py
Concat: 7.116918564017396
Array : 7.6817819369607605

% python2 t.py
Generating data...
Concat: 8.51635122299
Array : 7.93058490753

For a 10-line eni

% python3 t.py
Generating data...
Concat: 1.0464658139972016
Array : 1.2140009650029242

% python2 t.py
Generating data...
Concat: 0.956037998199
Array : 1.08836984634

% cat t.py
import timeit

concat = """
c=''
for x in range(0, 100):
    c += 'iface eth0 inet static'
"""

append = """
c = []
for x in range(0, 100):
 c.append('iface eth0 inet static')
"""

print('Generating data...')
print("Concat: %s" % timeit.timeit(concat))
print("Array : %s" % timeit.timeit(append))

[Ryan Harper (raharper)]

Looks good. Could we add unittests for distro using apply_network, including one without a _write_network so we can test that fallback logic?

[lp:~smoser/cloud-init/trunk.net-improve-lo-dns]

1269. By Scott Moser on 2016-07-14

merge from trunk

[Scott Moser (smoser)]

i added a test of the _write_network fallback in the fix for 1602373 (revno 1255 on trunk).
and pulled that in here.
going to merge this in now.

=== modified file 'cloudinit/net/eni.py'
--- cloudinit/net/eni.py	2016-07-14 02:36:23 +0000
+++ cloudinit/net/eni.py	2016-07-14 18:38:19 +0000
@@ -12,6 +12,7 @@
 #    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 copy
 import glob
 import os
 import re
@@ -42,7 +43,7 @@
 
 # TODO: switch valid_map based on mode inet/inet6
 def _iface_add_subnet(iface, subnet):
-    content = ""
+    content = []
     valid_map = [
         'address',
         'netmask',
@@ -61,15 +62,21 @@
                 value = " ".join(value)
             if '_' in key:
                 key = key.replace('_', '-')
-            content += "    {0} {1}\n".format(key, value)
+            content.append("    {0} {1}".format(key, value))
 
-    return content
+    return sorted(content)
 
 
 # TODO: switch to valid_map for attrs
-
-def _iface_add_attrs(iface):
-    content = ""
+def _iface_add_attrs(iface, index):
+    # If the index is non-zero, this is an alias interface. Alias interfaces
+    # represent additional interface addresses, and should not have additional
+    # attributes. (extra attributes here are almost always either incorrect,
+    # or are applied to the parent interface.) So if this is an alias, stop
+    # right here.
+    if index != 0:
+        return []
+    content = []
     ignore_map = [
         'control',
         'index',
@@ -79,19 +86,21 @@
         'subnets',
         'type',
     ]
+    renames = {'mac_address': 'hwaddress'}
     if iface['type'] not in ['bond', 'bridge', 'vlan']:
         ignore_map.append('mac_address')
 
     for key, value in iface.items():
-        if value and key not in ignore_map:
-            if type(value) == list:
-                value = " ".join(value)
-            content += "    {0} {1}\n".format(key, value)
-
-    return content
-
-
-def _iface_start_entry(iface, index):
+        if not value or key in ignore_map:
+            continue
+        if type(value) == list:
+            value = " ".join(value)
+        content.append("    {0} {1}".format(renames.get(key, key), value))
+
+    return sorted(content)
+
+
+def _iface_start_entry(iface, index, render_hwaddress=False):
     fullname = iface['name']
     if index != 0:
         fullname += ":%s" % index
@@ -107,8 +116,13 @@
     subst = iface.copy()
     subst.update({'fullname': fullname, 'cverb': cverb})
 
-    return ("{cverb} {fullname}\n"
-            "iface {fullname} {inet} {mode}\n").format(**subst)
+    lines = [
+        "{cverb} {fullname}".format(**subst),
+        "iface {fullname} {inet} {mode}".format(**subst)]
+    if render_hwaddress and iface.get('mac_address'):
+        lines.append("    hwaddress {mac_address}".format(**subst))
+
+    return lines
 
 
 def _parse_deb_config_data(ifaces, contents, src_dir, src_path):
@@ -262,10 +276,6 @@
     for name, data in ifaces.items():
         # devname is 'eth0' for name='eth0:1'
         devname = name.partition(":")[0]
-        if devname == "lo":
-            # currently provding 'lo' in network config results in duplicate
-            # entries. in rendered interfaces file. so skip it.
-            continue
         if devname not in devs:
             devs[devname] = {'type': 'physical', 'name': devname,
                              'subnets': []}
@@ -324,10 +334,10 @@
         1. http://askubuntu.com/questions/168033/
                  how-to-set-static-routes-in-ubuntu-server
         """
-        content = ""
+        content = []
         up = indent + "post-up route add"
         down = indent + "pre-down route del"
-        eol = " || true\n"
+        or_true = " || true"
         mapping = {
             'network': '-net',
             'netmask': 'netmask',
@@ -336,34 +346,84 @@
         }
         if route['network'] == '0.0.0.0' and route['netmask'] == '0.0.0.0':
             default_gw = " default gw %s" % route['gateway']
-            content += up + default_gw + eol
-            content += down + default_gw + eol
+            content.append(up + default_gw + or_true)
+            content.append(down + default_gw + or_true)
         elif route['network'] == '::' and route['netmask'] == 0:
             # ipv6!
             default_gw = " -A inet6 default gw %s" % route['gateway']
-            content += up + default_gw + eol
-            content += down + default_gw + eol
+            content.append(up + default_gw + or_true)
+            content.append(down + default_gw + or_true)
         else:
             route_line = ""
             for k in ['network', 'netmask', 'gateway', 'metric']:
                 if k in route:
                     route_line += " %s %s" % (mapping[k], route[k])
-            content += up + route_line + eol
-            content += down + route_line + eol
+            content.append(up + route_line + or_true)
+            content.append(down + route_line + or_true)
         return content
 
+    def _render_iface(self, iface, render_hwaddress=False):
+        sections = []
+        subnets = iface.get('subnets', {})
+        if subnets:
+            for index, subnet in zip(range(0, len(subnets)), subnets):
+                iface['index'] = index
+                iface['mode'] = subnet['type']
+                iface['control'] = subnet.get('control', 'auto')
+                subnet_inet = 'inet'
+                if iface['mode'].endswith('6'):
+                    # This is a request for DHCPv6.
+                    subnet_inet += '6'
+                elif iface['mode'] == 'static' and ":" in subnet['address']:
+                    # This is a static IPv6 address.
+                    subnet_inet += '6'
+                iface['inet'] = subnet_inet
+                if iface['mode'].startswith('dhcp'):
+                    iface['mode'] = 'dhcp'
+
+                lines = list(
+                    _iface_start_entry(
+                        iface, index, render_hwaddress=render_hwaddress) +
+                    _iface_add_subnet(iface, subnet) +
+                    _iface_add_attrs(iface, index)
+                )
+                for route in subnet.get('routes', []):
+                    lines.extend(self._render_route(route, indent="    "))
+
+                if len(subnets) > 1 and index == 0:
+                    tmpl = "    post-up ifup %s:%s\n"
+                    for i in range(1, len(subnets)):
+                        lines.append(tmpl % (iface['name'], i))
+
+                sections.append(lines)
+        else:
+            # ifenslave docs say to auto the slave devices
+            lines = []
+            if 'bond-master' in iface:
+                lines.append("auto {name}".format(**iface))
+            lines.append("iface {name} {inet} {mode}".format(**iface))
+            lines.extend(_iface_add_attrs(iface, index=0))
+            sections.append(lines)
+        return sections
+
     def _render_interfaces(self, network_state, render_hwaddress=False):
         '''Given state, emit etc/network/interfaces content.'''
 
-        content = ""
-        content += "auto lo\niface lo inet loopback\n"
+        # handle 'lo' specifically as we need to insert the global dns entries
+        # there (as that is the only interface that will be always up).
+        lo = {'name': 'lo', 'type': 'physical', 'inet': 'inet',
+              'subnets': [{'type': 'loopback', 'control': 'auto'}]}
+        for iface in network_state.iter_interfaces():
+            if iface.get('name') == "lo":
+                lo = copy.deepcopy(iface)
 
         nameservers = network_state.dns_nameservers
         if nameservers:
-            content += "    dns-nameservers %s\n" % (" ".join(nameservers))
+            lo['subnets'][0]["dns_nameservers"] = (" ".join(nameservers))
+
         searchdomains = network_state.dns_searchdomains
         if searchdomains:
-            content += "    dns-search %s\n" % (" ".join(searchdomains))
+            lo['subnets'][0]["dns_search"] = (" ".join(searchdomains))
 
         ''' Apply a sort order to ensure that we write out
             the physical interfaces first; this is critical for
@@ -375,45 +435,21 @@
             'bridge': 2,
             'vlan': 3,
         }
+
+        sections = []
+        sections.extend(self._render_iface(lo))
         for iface in sorted(network_state.iter_interfaces(),
                             key=lambda k: (order[k['type']], k['name'])):
 
-            if content[-2:] != "\n\n":
-                content += "\n"
-            subnets = iface.get('subnets', {})
-            if subnets:
-                for index, subnet in zip(range(0, len(subnets)), subnets):
-                    if content[-2:] != "\n\n":
-                        content += "\n"
-                    iface['index'] = index
-                    iface['mode'] = subnet['type']
-                    iface['control'] = subnet.get('control', 'auto')
-                    if iface['mode'].endswith('6'):
-                        iface['inet'] += '6'
-                    elif (iface['mode'] == 'static' and
-                          ":" in subnet['address']):
-                        iface['inet'] += '6'
-                    if iface['mode'].startswith('dhcp'):
-                        iface['mode'] = 'dhcp'
-
-                    content += _iface_start_entry(iface, index)
-                    if render_hwaddress and iface.get('mac_address'):
-                        content += "    hwaddress %s" % iface['mac_address']
-                    content += _iface_add_subnet(iface, subnet)
-                    content += _iface_add_attrs(iface)
-                    for route in subnet.get('routes', []):
-                        content += self._render_route(route, indent="    ")
-            else:
-                # ifenslave docs say to auto the slave devices
-                if 'bond-master' in iface:
-                    content += "auto {name}\n".format(**iface)
-                content += "iface {name} {inet} {mode}\n".format(**iface)
-                content += _iface_add_attrs(iface)
+            if iface.get('name') == "lo":
+                continue
+            sections.extend(
+                self._render_iface(iface, render_hwaddress=render_hwaddress))
 
         for route in network_state.iter_routes():
-            content += self._render_route(route)
+            sections.append(self._render_route(route))
 
-        return content
+        return '\n\n'.join(['\n'.join(s) for s in sections]) + "\n"
 
     def render_network_state(self, target, network_state):
         fpeni = os.path.join(target, self.eni_path)

=== modified file 'tests/unittests/helpers.py'
--- tests/unittests/helpers.py	2016-06-10 21:22:17 +0000
+++ tests/unittests/helpers.py	2016-07-14 18:38:19 +0000
@@ -264,6 +264,18 @@
             fp.close()
 
 
+def dir2dict(startdir, prefix=None):
+    flist = {}
+    if prefix is None:
+        prefix = startdir
+    for root, dirs, files in os.walk(startdir):
+        for fname in files:
+            fpath = os.path.join(root, fname)
+            key = fpath[len(prefix):]
+            flist[key] = util.load_file(fpath)
+    return flist
+
+
 try:
     skipIf = unittest.skipIf
 except AttributeError:

=== modified file 'tests/unittests/test_net.py'
--- tests/unittests/test_net.py	2016-07-14 02:03:42 +0000
+++ tests/unittests/test_net.py	2016-07-14 18:38:19 +0000
@@ -6,6 +6,7 @@
 from cloudinit.sources.helpers import openstack
 from cloudinit import util
 
+from .helpers import dir2dict
 from .helpers import mock
 from .helpers import TestCase
 
@@ -17,6 +18,8 @@
 import os
 import shutil
 import tempfile
+import textwrap
+import yaml
 
 DHCP_CONTENT_1 = """
 DEVICE='eth0'
@@ -141,6 +144,283 @@
     }
 ]
 
+EXAMPLE_ENI = """
+auto lo
+iface lo inet loopback
+   dns-nameservers 10.0.0.1
+   dns-search foo.com
+
+auto eth0
+iface eth0 inet static
+        address 1.2.3.12
+        netmask 255.255.255.248
+        broadcast 1.2.3.15
+        gateway 1.2.3.9
+        dns-nameservers 69.9.160.191 69.9.191.4
+auto eth1
+iface eth1 inet static
+        address 10.248.2.4
+        netmask 255.255.255.248
+        broadcast 10.248.2.7
+"""
+
+RENDERED_ENI = """
+auto lo
+iface lo inet loopback
+    dns-nameservers 10.0.0.1
+    dns-search foo.com
+
+auto eth0
+iface eth0 inet static
+    address 1.2.3.12
+    broadcast 1.2.3.15
+    dns-nameservers 69.9.160.191 69.9.191.4
+    gateway 1.2.3.9
+    netmask 255.255.255.248
+
+auto eth1
+iface eth1 inet static
+    address 10.248.2.4
+    broadcast 10.248.2.7
+    netmask 255.255.255.248
+""".lstrip()
+
+NETWORK_CONFIGS = {
+    'small': {
+        'expected_eni': textwrap.dedent("""\
+            auto lo
+            iface lo inet loopback
+                dns-nameservers 1.2.3.4 5.6.7.8
+                dns-search wark.maas
+
+            iface eth1 inet manual
+
+            auto eth99
+            iface eth99 inet dhcp
+                post-up ifup eth99:1
+
+
+            auto eth99:1
+            iface eth99:1 inet static
+                address 192.168.21.3/24
+                dns-nameservers 8.8.8.8 8.8.4.4
+                dns-search barley.maas sach.maas
+                post-up route add default gw 65.61.151.37 || true
+                pre-down route del default gw 65.61.151.37 || true
+        """).rstrip(' '),
+        'yaml': textwrap.dedent("""
+            version: 1
+            config:
+                # Physical interfaces.
+                - type: physical
+                  name: eth99
+                  mac_address: "c0:d6:9f:2c:e8:80"
+                  subnets:
+                      - type: dhcp4
+                      - type: static
+                        address: 192.168.21.3/24
+                        dns_nameservers:
+                          - 8.8.8.8
+                          - 8.8.4.4
+                        dns_search: barley.maas sach.maas
+                        routes:
+                          - gateway: 65.61.151.37
+                            netmask: 0.0.0.0
+                            network: 0.0.0.0
+                            metric: 2
+                - type: physical
+                  name: eth1
+                  mac_address: "cf:d6:af:48:e8:80"
+                - type: nameserver
+                  address:
+                    - 1.2.3.4
+                    - 5.6.7.8
+                  search:
+                    - wark.maas
+        """),
+    },
+    'all': {
+        'expected_eni': ("""\
+auto lo
+iface lo inet loopback
+    dns-nameservers 8.8.8.8 4.4.4.4 8.8.4.4
+    dns-search barley.maas wark.maas foobar.maas
+
+iface eth0 inet manual
+
+auto eth1
+iface eth1 inet manual
+    bond-master bond0
+    bond-mode active-backup
+
+auto eth2
+iface eth2 inet manual
+    bond-master bond0
+    bond-mode active-backup
+
+iface eth3 inet manual
+
+iface eth4 inet manual
+
+# control-manual eth5
+iface eth5 inet dhcp
+
+auto bond0
+iface bond0 inet6 dhcp
+    bond-mode active-backup
+    bond-slaves none
+    hwaddress aa:bb:cc:dd:ee:ff
+
+auto br0
+iface br0 inet static
+    address 192.168.14.2/24
+    bridge_ports eth3 eth4
+    bridge_stp off
+    post-up ifup br0:1
+
+
+auto br0:1
+iface br0:1 inet6 static
+    address 2001:1::1/64
+
+auto bond0.200
+iface bond0.200 inet dhcp
+    vlan-raw-device bond0
+    vlan_id 200
+
+auto eth0.101
+iface eth0.101 inet static
+    address 192.168.0.2/24
+    dns-nameservers 192.168.0.10 10.23.23.134
+    dns-search barley.maas sacchromyces.maas brettanomyces.maas
+    gateway 192.168.0.1
+    mtu 1500
+    vlan-raw-device eth0
+    vlan_id 101
+    post-up ifup eth0.101:1
+
+
+auto eth0.101:1
+iface eth0.101:1 inet static
+    address 192.168.2.10/24
+
+post-up route add -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
+pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
+"""),
+        'yaml': textwrap.dedent("""
+            version: 1
+            config:
+                # Physical interfaces.
+                - type: physical
+                  name: eth0
+                  mac_address: "c0:d6:9f:2c:e8:80"
+                - type: physical
+                  name: eth1
+                  mac_address: "aa:d6:9f:2c:e8:80"
+                - type: physical
+                  name: eth2
+                  mac_address: "c0:bb:9f:2c:e8:80"
+                - type: physical
+                  name: eth3
+                  mac_address: "66:bb:9f:2c:e8:80"
+                - type: physical
+                  name: eth4
+                  mac_address: "98:bb:9f:2c:e8:80"
+                # specify how ifupdown should treat iface
+                # control is one of ['auto', 'hotplug', 'manual']
+                # with manual meaning ifup/ifdown should not affect the iface
+                # useful for things like iscsi root + dhcp
+                - type: physical
+                  name: eth5
+                  mac_address: "98:bb:9f:2c:e8:8a"
+                  subnets:
+                    - type: dhcp
+                      control: manual
+                # VLAN interface.
+                - type: vlan
+                  name: eth0.101
+                  vlan_link: eth0
+                  vlan_id: 101
+                  mtu: 1500
+                  subnets:
+                    - type: static
+                      address: 192.168.0.2/24
+                      gateway: 192.168.0.1
+                      dns_nameservers:
+                        - 192.168.0.10
+                        - 10.23.23.134
+                      dns_search:
+                        - barley.maas
+                        - sacchromyces.maas
+                        - brettanomyces.maas
+                    - type: static
+                      address: 192.168.2.10/24
+                # Bond.
+                - type: bond
+                  name: bond0
+                  # if 'mac_address' is omitted, the MAC is taken from
+                  # the first slave.
+                  mac_address: "aa:bb:cc:dd:ee:ff"
+                  bond_interfaces:
+                    - eth1
+                    - eth2
+                  params:
+                    bond-mode: active-backup
+                  subnets:
+                    - type: dhcp6
+                # A Bond VLAN.
+                - type: vlan
+                  name: bond0.200
+                  vlan_link: bond0
+                  vlan_id: 200
+                  subnets:
+                      - type: dhcp4
+                # A bridge.
+                - type: bridge
+                  name: br0
+                  bridge_interfaces:
+                      - eth3
+                      - eth4
+                  ipv4_conf:
+                      rp_filter: 1
+                      proxy_arp: 0
+                      forwarding: 1
+                  ipv6_conf:
+                      autoconf: 1
+                      disable_ipv6: 1
+                      use_tempaddr: 1
+                      forwarding: 1
+                      # basically anything in /proc/sys/net/ipv6/conf/.../
+                  params:
+                      bridge_stp: 'off'
+                      bridge_fd: 0
+                      bridge_maxwait: 0
+                  subnets:
+                      - type: static
+                        address: 192.168.14.2/24
+                      - type: static
+                        address: 2001:1::1/64 # default to /64
+                # A global nameserver.
+                - type: nameserver
+                  address: 8.8.8.8
+                  search: barley.maas
+                # global nameservers and search in list form
+                - type: nameserver
+                  address:
+                    - 4.4.4.4
+                    - 8.8.4.4
+                  search:
+                    - wark.maas
+                    - foobar.maas
+                # A global route.
+                - type: route
+                  destination: 10.0.0.0/8
+                  gateway: 11.0.0.1
+                  metric: 3
+        """).lstrip(),
+    }
+}
+
 
 def _setup_test(tmp_dir, mock_get_devicelist, mock_sys_netdev_info,
                 mock_sys_dev_path):
@@ -354,6 +634,53 @@
         self.assertEqual(found, self.simple_cfg)
 
 
+class TestEniRoundTrip(TestCase):
+    def setUp(self):
+        super(TestCase, self).setUp()
+        self.tmp_dir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, self.tmp_dir)
+
+    def _render_and_read(self, network_config=None, state=None, eni_path=None,
+                         links_prefix=None, netrules_path=None):
+        if network_config:
+            ns = network_state.parse_net_config_data(network_config)
+        elif state:
+            ns = state
+        else:
+            raise ValueError("Expected data or state, got neither")
+
+        if eni_path is None:
+            eni_path = 'etc/network/interfaces'
+
+        renderer = eni.Renderer(
+            config={'eni_path': eni_path, 'links_path_prefix': links_prefix,
+                    'netrules_path': netrules_path})
+
+        renderer.render_network_state(self.tmp_dir, ns)
+        return dir2dict(self.tmp_dir)
+
+    def testsimple_convert_and_render(self):
+        network_config = eni.convert_eni_data(EXAMPLE_ENI)
+        files = self._render_and_read(network_config=network_config)
+        self.assertEqual(
+            RENDERED_ENI.splitlines(),
+            files['/etc/network/interfaces'].splitlines())
+
+    def testsimple_render_all(self):
+        entry = NETWORK_CONFIGS['all']
+        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_small(self):
+        entry = NETWORK_CONFIGS['small']
+        files = self._render_and_read(network_config=yaml.load(entry['yaml']))
+        self.assertEqual(
+            entry['expected_eni'].splitlines(),
+            files['/etc/network/interfaces'].splitlines())
+
+
 def _gzip_data(data):
     with io.BytesIO() as iobuf:
         gzfp = gzip.GzipFile(mode="wb", fileobj=iobuf)