Merge ~chad.smith/cloud-init:feature/azure-to-network-v2 into cloud-init:master

[Chad Smith]

azure/net: generate_fallback_nic emits network v2 config instead of v1

The function generate_fallback_config is used by Azure by default when
not consuming IMDS configuration data. This function is also used by any
datasource which does not implement it's own network config. This simple
fallback configuration sets up dhcp on the most likely NIC. It will now
emit network v2 instead of network v1.

This is a step toward moving all components talking in v2 and allows us
to avoid costly conversions between v1 and v2 for newer distributions
which rely on netplan.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:850c0d90bdf26f45b5d771e77caee3d32d73ba74
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1026/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1026//rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

One inline question.

2c1cc00... by Chad Smith on 2019-08-06

azure/net: generate_fallback_nic emits network v2 config instead of v1

The function generate_fallback_config is used by Azure by default when
not consuming IMDS configuration data. This function is also used by any
datasource which does not implement it's own network config. This simple
fallback configuration sets up dhcp on the most likely NIC. It will now
emit network v2 instead of network v1.

This is a step toward moving all components talking in v2 and allows us
to avoid costly conversions between v1 and v2 for newer distributions
which rely on netplan.

[Ryan Harper (raharper)]

One debugging line needs removing and some questions inline.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:9af66a67ecc80685544fd0847884ab7ea6a5d644
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1028/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1028//rebuild

review: Needs Fixing (continuous-integration)

5106244... by Chad Smith on 2019-08-06

azure: add route-metric to net config on non-primary dhcp interfaces

To retain default network routes on primary dhcp interface, set the
route-metric to 100 for primary interface and 200+ for any secondary
interfaces. This ensures that any dhcp routes provided by the secondary
nics will not be used as a default route for external bound traffic.

[Ryan Harper (raharper)]

That looks nice! One comment on where to set maxDiff, but we can do that separately

review: Approve

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:805f4718126311bb4f6e9215f7fbfbdce6b0d8b5
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1030/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1030//rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:5106244d7e7105079ec689ca34d93a61b3c8e045
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1031/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1031//rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Validated on disco given eth0 (with addtional secondary IP) and eth1 (dhcp secondary NIC)
# QUESTION: am I missing a metric 200 on 10.0.0.8?

ubuntu@SRU-worked-azure:~$ python3 -c 'from cloudinit.sources.DataSourceAzure import get_metadata_from_imds; print(get_metadata_from_imds("eth0", 1))'
{'compute': {'location': 'eastus2', 'name': 'test-sru-disco', 'offer': 'UbuntuServer', 'osType': 'Linux', 'placementGroupId': '', 'platformFaultDomain': '0', 'platformUpdateDomain': '0', 'publisher': 'Canonical', 'resourceGroupName': 'srugroup1', 'sku': '19.04-DAILY', 'subscriptionId': '12aad61c-6de4-4e53-a6c6-5aff52a83777', 'tags': '', 'version': '19.04.201908010', 'vmId': 'b838afe4-23fd-4f34-b66c-622767857444', 'vmScaleSetName': '', 'vmSize': 'Standard_DS1_v2', 'zone': ''}, 'network': {'interface': [{'ipv4': {'ipAddress': [{'privateIpAddress': '10.0.0.5', 'publicIpAddress': '40.84.36.124'}, {'privateIpAddress': '10.0.0.23', 'publicIpAddress': ''}], 'subnet': [{'address': '10.0.0.0', 'prefix': '24'}]}, 'ipv6': {'ipAddress': []}, 'macAddress': '000D3A7B7C34'}, {'ipv4': {'ipAddress': [{'privateIpAddress': '10.0.0.8', 'publicIpAddress': ''}], 'subnet': [{'address': '10.0.0.0', 'prefix': '24'}]}, 'ipv6': {'ipAddress': []}, 'macAddress': '000D3A7B9E10'}]}}
ubuntu@SRU-worked-azure:~$ cat /etc/netplan/50-cloud-init.yaml
# This file is generated from information provided by
# the datasource. Changes to it will not persist across an instance.
# To disable cloud-init's network configuration capabilities, write a file
# /etc/cloud/cloud.cfg.d/99-disable-network-config.cfg with the following:
# network: {config: disabled}
network:
    ethernets:
        eth0:
            addresses:
            - 10.0.0.23/24
            dhcp4: true
            dhcp4-overrides:
                route-metric: 100
            match:
                macaddress: 00:0d:3a:7b:7c:34
            set-name: eth0
        eth1:
            dhcp4: true
            dhcp4-overrides:
                route-metric: 200
            match:
                macaddress: 00:0d:3a:7b:9e:10
            set-name: eth1
    version: 2
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
    link/ether 00:0d:3a:7b:7c:34 brd ff:ff:ff:ff:ff:ff
    inet 10.0.0.23/24 brd 10.0.0.255 scope global eth0
       valid_lft forever preferred_lft forever
    inet 10.0.0.5/24 brd 10.0.0.255 scope global secondary eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::20d:3aff:fe7b:7c34/64 scope link
       valid_lft forever preferred_lft forever
3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
    link/ether 00:0d:3a:7b:9e:10 brd ff:ff:ff:ff:ff:ff
    inet 10.0.0.8/24 brd 10.0.0.255 scope global eth1
       valid_lft forever preferred_lft forever
    inet6 fe80::20d:3aff:fe7b:9e10/64 scope link
       valid_lft forever preferred_lft forever

ubuntu@SRU-worked-azure:~$ ip route show
default via 10.0.0.1 dev eth0 proto dhcp src 10.0.0.5 metric 100
10.0.0.0/24 dev eth0 proto kernel scope link src 10.0.0.23
10.0.0.0/24 dev eth1 proto kernel scope link src 10.0.0.8
168.63.129.16 via 10.0.0.1 dev eth0 proto dhcp src 10.0.0.5 m...

[Chad Smith (chad.smith)]

Landing as secondary NICs with unique gateways is not a primary network use-case in Azure. Netplan additional route-metric overrides on secondary NICs has been validated on OpenStack vms where unique gateways per-nic are simpler to setup.

diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index f3cec79..ea707c0 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -265,32 +265,23 @@ def find_fallback_nic(blacklist_drivers=None):
 
 
 def generate_fallback_config(blacklist_drivers=None, config_driver=None):
-    """Determine which attached net dev is most likely to have a connection and
-       generate network state to run dhcp on that interface"""
-
+    """Generate network cfg v2 for dhcp on the NIC most likely connected."""
     if not config_driver:
         config_driver = False
 
     target_name = find_fallback_nic(blacklist_drivers=blacklist_drivers)
-    if target_name:
-        target_mac = read_sys_net_safe(target_name, 'address')
-        nconf = {'config': [], 'version': 1}
-        cfg = {'type': 'physical', 'name': target_name,
-               'mac_address': target_mac, 'subnets': [{'type': 'dhcp'}]}
-        # inject the device driver name, dev_id into config if enabled and
-        # device has a valid device driver value
-        if config_driver:
-            driver = device_driver(target_name)
-            if driver:
-                cfg['params'] = {
-                    'driver': driver,
-                    'device_id': device_devid(target_name),
-                }
-        nconf['config'].append(cfg)
-        return nconf
-    else:
+    if not target_name:
         # can't read any interfaces addresses (or there are none); give up
         return None
+    target_mac = read_sys_net_safe(target_name, 'address')
+    cfg = {'dhcp4': True, 'set-name': target_name,
+           'match': {'macaddress': target_mac.lower()}}
+    if config_driver:
+        driver = device_driver(target_name)
+        if driver:
+            cfg['match']['driver'] = driver
+    nconf = {'ethernets': {target_name: cfg}, 'version': 2}
+    return nconf
 
 
 def extract_physdevs(netcfg):
diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
index 0ca576b..c0c415d 100644
--- a/cloudinit/net/network_state.py
+++ b/cloudinit/net/network_state.py
@@ -596,6 +596,7 @@ class NetworkStateInterpreter(object):
           eno1:
             match:
               macaddress: 00:11:22:33:44:55
+              driver: hv_netsvc
             wakeonlan: true
             dhcp4: true
             dhcp6: false
@@ -631,15 +632,18 @@ class NetworkStateInterpreter(object):
                 'type': 'physical',
                 'name': cfg.get('set-name', eth),
             }
-            mac_address = cfg.get('match', {}).get('macaddress', None)
+            match = cfg.get('match', {})
+            mac_address = match.get('macaddress', None)
             if not mac_address:
                 LOG.debug('NetworkState Version2: missing "macaddress" info '
                           'in config entry: %s: %s', eth, str(cfg))
-            phy_cmd.update({'mac_address': mac_address})
-
+            phy_cmd['mac_address'] = mac_address
+            driver = match.get('driver', None)
+            if driver:
+                phy_cmd['params'] = {'driver': driver}
             for key in ['mtu', 'match', 'wakeonlan']:
                 if key in cfg:
-                    phy_cmd.update({key: cfg.get(key)})
+                    phy_cmd[key] = cfg[key]
 
             subnets = self._v2_to_v1_ipcfg(cfg)
             if len(subnets) > 0:
diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py
index e6e77d7..d2e38f0 100644
--- a/cloudinit/net/tests/test_init.py
+++ b/cloudinit/net/tests/test_init.py
@@ -212,9 +212,9 @@ class TestGenerateFallbackConfig(CiTestCase):
         mac = 'aa:bb:cc:aa:bb:cc'
         write_file(os.path.join(self.sysdir, 'eth1', 'address'), mac)
         expected = {
-            'config': [{'type': 'physical', 'mac_address': mac,
-                        'name': 'eth1', 'subnets': [{'type': 'dhcp'}]}],
-            'version': 1}
+            'ethernets': {'eth1': {'match': {'macaddress': mac},
+                                   'dhcp4': True, 'set-name': 'eth1'}},
+            'version': 2}
         self.assertEqual(expected, net.generate_fallback_config())
 
     def test_generate_fallback_finds_dormant_eth_with_mac(self):
@@ -223,9 +223,9 @@ class TestGenerateFallbackConfig(CiTestCase):
         mac = 'aa:bb:cc:aa:bb:cc'
         write_file(os.path.join(self.sysdir, 'eth0', 'address'), mac)
         expected = {
-            'config': [{'type': 'physical', 'mac_address': mac,
-                        'name': 'eth0', 'subnets': [{'type': 'dhcp'}]}],
-            'version': 1}
+            'ethernets': {'eth0': {'match': {'macaddress': mac}, 'dhcp4': True,
+                                   'set-name': 'eth0'}},
+            'version': 2}
         self.assertEqual(expected, net.generate_fallback_config())
 
     def test_generate_fallback_finds_eth_by_operstate(self):
@@ -233,9 +233,10 @@ class TestGenerateFallbackConfig(CiTestCase):
         mac = 'aa:bb:cc:aa:bb:cc'
         write_file(os.path.join(self.sysdir, 'eth0', 'address'), mac)
         expected = {
-            'config': [{'type': 'physical', 'mac_address': mac,
-                        'name': 'eth0', 'subnets': [{'type': 'dhcp'}]}],
-            'version': 1}
+            'ethernets': {
+                'eth0': {'dhcp4': True, 'match': {'macaddress': mac},
+                         'set-name': 'eth0'}},
+            'version': 2}
         valid_operstates = ['dormant', 'down', 'lowerlayerdown', 'unknown']
         for state in valid_operstates:
             write_file(os.path.join(self.sysdir, 'eth0', 'operstate'), state)
diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index d2fad9b..e6ed2f3 100755
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -1241,7 +1241,7 @@ def parse_network_config(imds_metadata):
                     privateIpv4 = addr4['privateIpAddress']
                     if privateIpv4:
                         if dev_config.get('dhcp4', False):
-                            # Append static address config for nic > 1
+                            # Append static address config for ip > 1
                             netPrefix = intf['ipv4']['subnet'][0].get(
                                 'prefix', '24')
                             if not dev_config.get('addresses'):
@@ -1251,6 +1251,11 @@ def parse_network_config(imds_metadata):
                                     ip=privateIpv4, prefix=netPrefix))
                         else:
                             dev_config['dhcp4'] = True
+                            # non-primary interfaces should have a higher
+                            # route-metric (cost) so default routes prefer
+                            # primary nic due to lower route-metric value
+                            dev_config['dhcp4-overrides'] = {
+                                'route-metric': (idx + 1) * 100}
                 for addr6 in intf['ipv6']['ipAddress']:
                     privateIpv6 = addr6['privateIpAddress']
                     if privateIpv6:
diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
index 2de2aea..4d57ceb 100644
--- a/tests/unittests/test_datasource/test_azure.py
+++ b/tests/unittests/test_datasource/test_azure.py
@@ -12,6 +12,7 @@ from cloudinit.tests.helpers import (
     HttprettyTestCase, CiTestCase, populate_dir, mock, wrap_and_call,
     ExitStack, resourceLocation)
 
+import copy
 import crypt
 import httpretty
 import json
@@ -129,6 +130,26 @@ NETWORK_METADATA = {
     }
 }
 
+SECONDARY_INTERFACE = {
+    "macAddress": "220D3A047598",
+    "ipv6": {
+        "ipAddress": []
+    },
+    "ipv4": {
+        "subnet": [
+            {
+                "prefix": "24",
+                "address": "10.0.1.0"
+            }
+        ],
+        "ipAddress": [
+            {
+                "privateIpAddress": "10.0.1.5",
+            }
+        ]
+    }
+}
+
 MOCKPATH = 'cloudinit.sources.DataSourceAzure.'
 
 
@@ -619,8 +640,43 @@ scbus-1 on xpt0 bus 0
             'ethernets': {
                 'eth0': {'set-name': 'eth0',
                          'match': {'macaddress': '00:0d:3a:04:75:98'},
-                         'dhcp4': True}},
+                         'dhcp4': True,
+                         'dhcp4-overrides': {'route-metric': 100}}},
+            'version': 2}
+        dsrc = self._get_ds(data)
+        dsrc.get_data()
+        self.assertEqual(expected_network_config, dsrc.network_config)
+
+    def test_network_config_set_from_imds_route_metric_for_secondary_nic(self):
+        """Datasource.network_config adds route-metric to secondary nics."""
+        sys_cfg = {'datasource': {'Azure': {'apply_network_config': True}}}
+        odata = {}
+        data = {'ovfcontent': construct_valid_ovf_env(data=odata),
+                'sys_cfg': sys_cfg}
+        expected_network_config = {
+            'ethernets': {
+                'eth0': {'set-name': 'eth0',
+                         'match': {'macaddress': '00:0d:3a:04:75:98'},
+                         'dhcp4': True,
+                         'dhcp4-overrides': {'route-metric': 100}},
+                'eth1': {'set-name': 'eth1',
+                         'match': {'macaddress': '22:0d:3a:04:75:98'},
+                         'dhcp4': True,
+                         'dhcp4-overrides': {'route-metric': 200}},
+                'eth2': {'set-name': 'eth2',
+                         'match': {'macaddress': '33:0d:3a:04:75:98'},
+                         'dhcp4': True,
+                         'dhcp4-overrides': {'route-metric': 300}}},
             'version': 2}
+        imds_data = copy.deepcopy(NETWORK_METADATA)
+        imds_data['network']['interface'].append(SECONDARY_INTERFACE)
+        third_intf = copy.deepcopy(SECONDARY_INTERFACE)
+        third_intf['macAddress'] = third_intf['macAddress'].replace('22', '33')
+        third_intf['ipv4']['subnet'][0]['address'] = '10.0.2.0'
+        third_intf['ipv4']['ipAddress'][0]['privateIpAddress'] = '10.0.2.6'
+        imds_data['network']['interface'].append(third_intf)
+
+        self.m_get_metadata_from_imds.return_value = imds_data
         dsrc = self._get_ds(data)
         dsrc.get_data()
         self.assertEqual(expected_network_config, dsrc.network_config)
@@ -925,6 +981,7 @@ scbus-1 on xpt0 bus 0
         expected_cfg = {
             'ethernets': {
                 'eth0': {'dhcp4': True,
+                         'dhcp4-overrides': {'route-metric': 100},
                          'match': {'macaddress': '00:0d:3a:04:75:98'},
                          'set-name': 'eth0'}},
             'version': 2}
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 1840ade..4f7e420 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -2156,7 +2156,7 @@ DEFAULT_DEV_ATTRS = {
         "carrier": False,
         "dormant": False,
         "operstate": "down",
-        "address": "07-1C-C6-75-A4-BE",
+        "address": "07-1c-c6-75-a4-be",
         "device/driver": None,
         "device/device": None,
         "name_assign_type": "4",
@@ -2207,6 +2207,39 @@ class TestGenerateFallbackConfig(CiTestCase):
     @mock.patch("cloudinit.net.sys_dev_path")
     @mock.patch("cloudinit.net.read_sys_net")
     @mock.patch("cloudinit.net.get_devicelist")
+    def test_device_driver_v2(self, mock_get_devicelist, mock_read_sys_net,
+                              mock_sys_dev_path):
+        """Network configuration for generate_fallback_config is version 2."""
+        devices = {
+            'eth0': {
+                'bridge': False, 'carrier': False, 'dormant': False,
+                'operstate': 'down', 'address': '00:11:22:33:44:55',
+                'device/driver': 'hv_netsvc', 'device/device': '0x3',
+                'name_assign_type': '4'},
+            'eth1': {
+                'bridge': False, 'carrier': False, 'dormant': False,
+                'operstate': 'down', 'address': '00:11:22:33:44:55',
+                'device/driver': 'mlx4_core', 'device/device': '0x7',
+                'name_assign_type': '4'},
+
+        }
+
+        tmp_dir = self.tmp_dir()
+        _setup_test(tmp_dir, mock_get_devicelist,
+                    mock_read_sys_net, mock_sys_dev_path,
+                    dev_attrs=devices)
+
+        network_cfg = net.generate_fallback_config(config_driver=True)
+        expected = {
+            'ethernets': {'eth0': {'dhcp4': True, 'set-name': 'eth0',
+                                   'match': {'macaddress': '00:11:22:33:44:55',
+                                             'driver': 'hv_netsvc'}}},
+            'version': 2}
+        self.assertEqual(expected, network_cfg)
+
+    @mock.patch("cloudinit.net.sys_dev_path")
+    @mock.patch("cloudinit.net.read_sys_net")
+    @mock.patch("cloudinit.net.get_devicelist")
     def test_device_driver(self, mock_get_devicelist, mock_read_sys_net,
                            mock_sys_dev_path):
         devices = {
@@ -2486,7 +2519,7 @@ class TestRhelSysConfigRendering(CiTestCase):
 #
 BOOTPROTO=dhcp
 DEVICE=eth1000
-HWADDR=07-1C-C6-75-A4-BE
+HWADDR=07-1c-c6-75-a4-be
 NM_CONTROLLED=no
 ONBOOT=yes
 STARTMODE=auto
@@ -3030,7 +3063,7 @@ class TestOpenSuseSysConfigRendering(CiTestCase):
 #
 BOOTPROTO=dhcp
 DEVICE=eth1000
-HWADDR=07-1C-C6-75-A4-BE
+HWADDR=07-1c-c6-75-a4-be
 NM_CONTROLLED=no
 ONBOOT=yes
 STARTMODE=auto
@@ -3342,13 +3375,13 @@ class TestNetplanNetRendering(CiTestCase):
 
         expected = """
 network:
-    version: 2
     ethernets:
         eth1000:
             dhcp4: true
             match:
                 macaddress: 07-1c-c6-75-a4-be
             set-name: eth1000
+    version: 2
 """
         self.assertEqual(expected.lstrip(), contents.lstrip())
         self.assertEqual(1, mock_clean_default.call_count)