improvements to eni rendering

Some improvements here, and some bug fixes.
 - bring curtin revno 394's 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.

3 files changed, 438 insertions, 63 deletions

diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
index 0221f55d..eff5b924 100644
--- a/cloudinit/net/eni.py
+++ b/cloudinit/net/eni.py
@@ -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 @@ NET_CONFIG_OPTIONS = [
 
 # 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 @@ def _iface_add_subnet(iface, subnet):
                 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 @@ def _iface_add_attrs(iface):
         '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)
+        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 content
+    return sorted(content)
 
 
-def _iface_start_entry(iface, index):
+def _iface_start_entry(iface, index, render_hwaddress=False):
     fullname = iface['name']
     if index != 0:
         fullname += ":%s" % index
@@ -107,8 +116,13 @@ def _iface_start_entry(iface, index):
     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 @@ def _ifaces_to_net_config_data(ifaces):
     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 @@ class Renderer(renderer.Renderer):
         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 @@ class Renderer(renderer.Renderer):
         }
         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 @@ class Renderer(renderer.Renderer):
             '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)
diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
index 8d46a8bf..972245df 100644
--- a/tests/unittests/helpers.py
+++ b/tests/unittests/helpers.py
@@ -264,6 +264,18 @@ def populate_dir(path, files):
             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:
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 6f4dad13..41b9a6d0 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -6,6 +6,7 @@ from cloudinit.net import sysconfig
 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 json
 import os
 import shutil
 import tempfile
+import textwrap
+import yaml
 
 DHCP_CONTENT_1 = """
 DEVICE='eth0'
@@ -141,6 +144,283 @@ nameserver 172.19.0.12
     }
 ]
 
+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 @@ class TestCmdlineConfigParsing(TestCase):
         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)