Merge ~pengpengs/cloud-init:fix/vmware-gosc-fail-with-staticIP-and-no-gateway into cloud-init:master

[Pengpeng Sun]

VMWare: Fix a network config bug in vm with static IPv4 and no gateway.

The issue is when customize a VM with static IPv4 and without gateway, it
will still extend route list and will loop a gateways list which is None.
This fix is to make sure when no gateway is here, it will not extend route
list.

LP: #1766538

[Scott Moser (smoser)]

Pengpeng,

This looks good, thank you.

I put together at test that shows failure.

Could you please incorporate it into your merge proposal?

I hope that it adds a fairly easy way to test the conversion.
You do not *have* to add any additional tests, but anything you
wanted to add would be greately appreciated.

Then I think we can get this merged.

Thanks.

[Pengpeng Sun (pengpengs)]

Hi Scott,

Thanks for your quick reply.
As you might know, this is the first time I submit code change to cloud-init. What's the test failure you mentioned? I just ran 'tox' after code change locally, any other testing needed?

Thanks,
pengpeng

[Scott Moser (smoser)]

PengPeng,
I'm for being confusing. I apparently failed to put a link to a pastebin.

http://paste.ubuntu.com/p/X2WgS82vJN/

The test that I added there 'test_primary_nic_without_gateway' demonstrates the failure that you have fixed.

[Pengpeng Sun (pengpengs)]

Scott,

Thanks for adding the test, I read it in pastebin. Now I know how to add a test according to code change.
One thing, my fix is to address failure when non primary nic without gateway, I'm writing tests but get 'pycodestyle: commands failed' when run tox, I will try to fix it and amend my commit.

Thanks,
Pengpeng

82646d6... by Pengpeng Sun on 2018-08-23

Merge branch 'fix/vmware-gosc-fail-with-staticIP-and-no-gateway' of ssh://git.launchpad.net/~pengpengs/cloud-init into fix/vmware-gosc-fail-with-staticIP-and-no-gateway

14b0155... by Pengpeng Sun on 2018-08-23

Merge branch 'fix/vmware-gosc-fail-with-staticIP-and-no-gateway' of ssh://git.launchpad.net/~pengpengs/cloud-init into fix/vmware-gosc-fail-with-staticIP-and-no-gateway

[Pengpeng Sun (pengpengs)]

Scott,
Please help to double-check my merge request, from the latest Diff I can see the test has been added.
tox gave me 'congratulations :)'.

But I pushed twice, one to ssh://git.launchpad.net/~pengpengs/cloud-init fix/vmware-gosc-fail-with-staticIP-and-no-gateway, the second to fix/vmware-gosc-fail-with-staticIP-and-no-gateway.
What's the right way to update this merge?

anyway, please let me know if there are any changes needed.

Thanks,
Pengpeng

[Scott Moser (smoser)]

Hi,

Thanks. This looks great.
The way you pushed is fine. Generally, launchpad will figure things out and it did correctly here.

One other thing I would like to see is adding a "negative" test for this path.

Add a test named "test_non_primary_nic_with_gateway", and just adjust the config and result as they should be.

Then we'll make sure that we don't accidently break the common case where we *do* have a gateway.

Thanks.
Scott

review: Approve

[Scott Moser (smoser)]

I accidently hit 'Approve', but meant to hit 'needs fixing'.

Please add the test, and *then* i will approve.

Thanks for your patience, and contributions to cloud-init.

review: Needs Fixing

efc50d3... by Pengpeng Sun on 2018-08-24

Merge branch 'fix/vmware-gosc-fail-with-staticIP-and-no-gateway' of ssh://git.launchpad.net/~pengpengs/cloud-init into fix/vmware-gosc-fail-with-staticIP-and-no-gateway

[Pengpeng Sun (pengpengs)]

Hi Scott,

I added 2 more tests "test_non_primary_nic_with_gateway" and "test_a_primary_nic_with_gateway".
The first one is to test a *non* primary nic have a gateway, the second one is to test a primary nic have a gateway. Please help to review them.
tox gave me all pass.
Thanks again for your help on test.

Best regards,
Pengpeng

[Scott Moser (smoser)]

review: Approve

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

review: Approve (continuous-integration)

diff --git a/cloudinit/sources/helpers/vmware/imc/config_nic.py b/cloudinit/sources/helpers/vmware/imc/config_nic.py
index 3ef8c62..e1890e2 100644
--- a/cloudinit/sources/helpers/vmware/imc/config_nic.py
+++ b/cloudinit/sources/helpers/vmware/imc/config_nic.py
@@ -164,7 +164,7 @@ class NicConfigurator(object):
             return ([subnet], route_list)
 
         # Add routes if there is no primary nic
-        if not self._primaryNic:
+        if not self._primaryNic and v4.gateways:
             route_list.extend(self.gen_ipv4_route(nic,
                                                   v4.gateways,
                                                   v4.netmask))
diff --git a/tests/unittests/test_vmware_config_file.py b/tests/unittests/test_vmware_config_file.py
index 036f687..602dedb 100644
--- a/tests/unittests/test_vmware_config_file.py
+++ b/tests/unittests/test_vmware_config_file.py
@@ -2,11 +2,15 @@
 # Copyright (C) 2016 VMware INC.
 #
 # Author: Sankar Tanguturi <[e-mail]>
+#         Pengpeng Sun <[e-mail]>
 #
 # This file is part of cloud-init. See LICENSE file for license information.
 
 import logging
+import os
 import sys
+import tempfile
+import textwrap
 
 from cloudinit.sources.DataSourceOVF import get_network_config_from_conf
 from cloudinit.sources.DataSourceOVF import read_vmware_imc
@@ -343,4 +347,115 @@ class TestVmwareConfigFile(CiTestCase):
         conf = Config(cf)
         self.assertEqual("test-script", conf.custom_script_name)
 
+
+class TestVmwareNetConfig(CiTestCase):
+    """Test conversion of vmware config to cloud-init config."""
+
+    def _get_NicConfigurator(self, text):
+        fp = None
+        try:
+            with tempfile.NamedTemporaryFile(mode="w", dir=self.tmp_dir(),
+                                             delete=False) as fp:
+                fp.write(text)
+                fp.close()
+            cfg = Config(ConfigFile(fp.name))
+            return NicConfigurator(cfg.nics, use_system_devices=False)
+        finally:
+            if fp:
+                os.unlink(fp.name)
+
+    def test_non_primary_nic_without_gateway(self):
+        """A non primary nic set is not required to have a gateway."""
+        config = textwrap.dedent("""\
+            [NETWORK]
+            NETWORKING = yes
+            BOOTPROTO = dhcp
+            HOSTNAME = myhost1
+            DOMAINNAME = eng.vmware.com
+
+            [NIC-CONFIG]
+            NICS = NIC1
+
+            [NIC1]
+            MACADDR = 00:50:56:a6:8c:08
+            ONBOOT = yes
+            IPv4_MODE = BACKWARDS_COMPATIBLE
+            BOOTPROTO = static
+            IPADDR = 10.20.87.154
+            NETMASK = 255.255.252.0
+            """)
+        nc = self._get_NicConfigurator(config)
+        self.assertEqual(
+            [{'type': 'physical', 'name': 'NIC1',
+              'mac_address': '00:50:56:a6:8c:08',
+              'subnets': [
+                  {'control': 'auto', 'type': 'static',
+                   'address': '10.20.87.154', 'netmask': '255.255.252.0'}]}],
+            nc.generate())
+
+    def test_non_primary_nic_with_gateway(self):
+        """A non primary nic set can have a gateway."""
+        config = textwrap.dedent("""\
+            [NETWORK]
+            NETWORKING = yes
+            BOOTPROTO = dhcp
+            HOSTNAME = myhost1
+            DOMAINNAME = eng.vmware.com
+
+            [NIC-CONFIG]
+            NICS = NIC1
+
+            [NIC1]
+            MACADDR = 00:50:56:a6:8c:08
+            ONBOOT = yes
+            IPv4_MODE = BACKWARDS_COMPATIBLE
+            BOOTPROTO = static
+            IPADDR = 10.20.87.154
+            NETMASK = 255.255.252.0
+            GATEWAY = 10.20.87.253
+            """)
+        nc = self._get_NicConfigurator(config)
+        self.assertEqual(
+            [{'type': 'physical', 'name': 'NIC1',
+              'mac_address': '00:50:56:a6:8c:08',
+              'subnets': [
+                  {'control': 'auto', 'type': 'static',
+                   'address': '10.20.87.154', 'netmask': '255.255.252.0'}]},
+             {'type': 'route', 'destination': '10.20.84.0/22',
+              'gateway': '10.20.87.253', 'metric': 10000}],
+            nc.generate())
+
+    def test_a_primary_nic_with_gateway(self):
+        """A primary nic set can have a gateway."""
+        config = textwrap.dedent("""\
+            [NETWORK]
+            NETWORKING = yes
+            BOOTPROTO = dhcp
+            HOSTNAME = myhost1
+            DOMAINNAME = eng.vmware.com
+
+            [NIC-CONFIG]
+            NICS = NIC1
+
+            [NIC1]
+            MACADDR = 00:50:56:a6:8c:08
+            ONBOOT = yes
+            IPv4_MODE = BACKWARDS_COMPATIBLE
+            BOOTPROTO = static
+            IPADDR = 10.20.87.154
+            NETMASK = 255.255.252.0
+            PRIMARY = true
+            GATEWAY = 10.20.87.253
+            """)
+        nc = self._get_NicConfigurator(config)
+        self.assertEqual(
+            [{'type': 'physical', 'name': 'NIC1',
+              'mac_address': '00:50:56:a6:8c:08',
+              'subnets': [
+                  {'control': 'auto', 'type': 'static',
+                   'address': '10.20.87.154', 'netmask': '255.255.252.0',
+                   'gateway': '10.20.87.253'}]}],
+            nc.generate())
+
+
 # vi: ts=4 expandtab