Merge ~rjschwei/cloud-init:setDefRoute into cloud-init:master

[Robert Schweikert]

net/sysconfig: Handle default route setup for dhcp configured NICs

When the network configuration has a default route configured and
another network device that is configured with dhcp, SUSE sysconfig
output should not accept the default route provided by the dhcp
server.

LP: #1812117

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

PASSED: Continuous integration, rev:af3d1694fa54eb6d3ba66ae0c8e2ee5044284844
https://jenkins.ubuntu.com/server/job/cloud-init-ci/527/
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/527/rebuild

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

Apologies for the delay in getting to this! There are a couple of things inline that I think need to be reworked somewhat, as well as a few more minor improvements suggested.

review: Needs Fixing

[Robert Schweikert (rjschwei)]

modified the @property implementation as suggested to reduce overhead, "better" naming for method that determines if there is a default route.

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

PASSED: Continuous integration, rev:02b1d8585c803690e5a8571dd3e4b2422d2c4ab5
https://jenkins.ubuntu.com/server/job/cloud-init-ci/603/
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/603/rebuild

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

Hey Robert,

Thanks for the changes and your continued work on this. :)

On Wed, Feb 27, 2019 at 01:12:42AM -0000, Robert Schweikert wrote:
> > - def iter_routes(self, filter_func=None):
>
> Given that we have all the properties collected together, well the
> iterator interrupts the collection , while I was here I figured I'd do
> a little bit of
> "maintenance/cleanliness/code-smell/whatever-name-you-prefer" work.

Cool, thanks, just checking I hadn't missed anything. :)

> > + def _find_default_route(self):
>
> Well there is a naming pattern in use in the code base using "maybe"
> so this could be renamed to "_maybe_has_default_route" fine by me.

Yep, that works!

> > def render_network_state(self, network_state, templates=None, target=None):
> > + # Force the knowledge of a default route for the network state
> > + # into the renderer, this is needed to write the proper ifcfg-
> > + # on SUSE distros
> > + self.__class__._network_default_route = network_state.has_default_route
>
> That is not something I can answer. The decision to implement most of
> the internals of the renderer as "classmethod" was not mine.

Ah, I hadn't realised it was all classmethods, that's less than ideal.
That said, something like https://paste.ubuntu.com/p/SgnhHZkyKB/ is what
I had in mind here, and that works even given the fact we don't have an
instance to store data on.

Thanks!

[Robert Schweikert (rjschwei)]

OK, back to you, sorry for the delay. Modified the code according to https://paste.ubuntu.com/p/SgnhHZkyKB/ just a style topic as far as I am concerned.

[Dan Watkins (oddbloke)]

Thanks Robert! I absolutely disagree that it's just a style topic, though; arbitrarily setting class-level attributes outside of the class definition makes the code much harder to reason about.

review: Approve

[Ryan Harper (raharper)]

review: Approve

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

PASSED: Continuous integration, rev:15205e8382e2959a3b11fa8c721c810a949a0539
https://jenkins.ubuntu.com/server/job/cloud-init-ci/632/
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/632/rebuild

review: Approve (continuous-integration)

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

FAILED: Autolanding.
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/cloud-init-autoland-test/186/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

review: Needs Fixing (continuous-integration)

[Dan Watkins (oddbloke)]

Robert, it looks like your tests are no longer correct due to modifications to master; could you rebase onto/merge from master and re-push, please?

c7eece4... by Robert Schweikert on 2019-03-12

- Rebase and fix tests

[Robert Schweikert (rjschwei)]

Should be all better now

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

PASSED: Continuous integration, rev:c7eece435f5a32b6ece9d7c01bcf0f1d0892d4af
https://jenkins.ubuntu.com/server/job/cloud-init-ci/637/
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/637/rebuild

review: Approve (continuous-integration)

diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
index 539b76d..4d19f56 100644
--- a/cloudinit/net/network_state.py
+++ b/cloudinit/net/network_state.py
@@ -148,6 +148,7 @@ class NetworkState(object):
         self._network_state = copy.deepcopy(network_state)
         self._version = version
         self.use_ipv6 = network_state.get('use_ipv6', False)
+        self._has_default_route = None
 
     @property
     def config(self):
@@ -157,14 +158,6 @@ class NetworkState(object):
     def version(self):
         return self._version
 
-    def iter_routes(self, filter_func=None):
-        for route in self._network_state.get('routes', []):
-            if filter_func is not None:
-                if filter_func(route):
-                    yield route
-            else:
-                yield route
-
     @property
     def dns_nameservers(self):
         try:
@@ -179,6 +172,12 @@ class NetworkState(object):
         except KeyError:
             return []
 
+    @property
+    def has_default_route(self):
+        if self._has_default_route is None:
+            self._has_default_route = self._maybe_has_default_route()
+        return self._has_default_route
+
     def iter_interfaces(self, filter_func=None):
         ifaces = self._network_state.get('interfaces', {})
         for iface in six.itervalues(ifaces):
@@ -188,6 +187,32 @@ class NetworkState(object):
                 if filter_func(iface):
                     yield iface
 
+    def iter_routes(self, filter_func=None):
+        for route in self._network_state.get('routes', []):
+            if filter_func is not None:
+                if filter_func(route):
+                    yield route
+            else:
+                yield route
+
+    def _maybe_has_default_route(self):
+        for route in self.iter_routes():
+            if self._is_default_route(route):
+                return True
+        for iface in self.iter_interfaces():
+            for subnet in iface.get('subnets', []):
+                for route in subnet.get('routes', []):
+                    if self._is_default_route(route):
+                        return True
+        return False
+
+    def _is_default_route(self, route):
+        default_nets = ('::', '0.0.0.0')
+        return (
+            route.get('prefix') == 0
+            and route.get('network') in default_nets
+            )
+
 
 @six.add_metaclass(CommandHandlerMeta)
 class NetworkStateInterpreter(object):
diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
index 19b3e60..e59753d 100644
--- a/cloudinit/net/sysconfig.py
+++ b/cloudinit/net/sysconfig.py
@@ -322,7 +322,7 @@ class Renderer(renderer.Renderer):
                 iface_cfg[new_key] = old_value
 
     @classmethod
-    def _render_subnets(cls, iface_cfg, subnets):
+    def _render_subnets(cls, iface_cfg, subnets, has_default_route):
         # setting base values
         iface_cfg['BOOTPROTO'] = 'none'
 
@@ -331,6 +331,7 @@ class Renderer(renderer.Renderer):
             mtu_key = 'MTU'
             subnet_type = subnet.get('type')
             if subnet_type == 'dhcp6':
+                # TODO need to set BOOTPROTO to dhcp6 on SUSE
                 iface_cfg['IPV6INIT'] = True
                 iface_cfg['DHCPV6C'] = True
             elif subnet_type in ['dhcp4', 'dhcp']:
@@ -375,9 +376,9 @@ class Renderer(renderer.Renderer):
         ipv6_index = -1
         for i, subnet in enumerate(subnets, start=len(iface_cfg.children)):
             subnet_type = subnet.get('type')
-            if subnet_type == 'dhcp6':
-                continue
-            elif subnet_type in ['dhcp4', 'dhcp']:
+            if subnet_type in ['dhcp', 'dhcp4', 'dhcp6']:
+                if has_default_route and iface_cfg['BOOTPROTO'] != 'none':
+                    iface_cfg['DHCLIENT_SET_DEFAULT_ROUTE'] = False
                 continue
             elif subnet_type == 'static':
                 if subnet_is_ipv6(subnet):
@@ -443,6 +444,8 @@ class Renderer(renderer.Renderer):
                     # TODO(harlowja): add validation that no other iface has
                     # also provided the default route?
                     iface_cfg['DEFROUTE'] = True
+                    if iface_cfg['BOOTPROTO'] in ('dhcp', 'dhcp4', 'dhcp6'):
+                        iface_cfg['DHCLIENT_SET_DEFAULT_ROUTE'] = True
                     if 'gateway' in route:
                         if is_ipv6 or is_ipv6_addr(route['gateway']):
                             iface_cfg['IPV6_DEFAULTGW'] = route['gateway']
@@ -493,7 +496,9 @@ class Renderer(renderer.Renderer):
             iface_cfg = iface_contents[iface_name]
             route_cfg = iface_cfg.routes
 
-            cls._render_subnets(iface_cfg, iface_subnets)
+            cls._render_subnets(
+                iface_cfg, iface_subnets, network_state.has_default_route
+            )
             cls._render_subnet_routes(iface_cfg, route_cfg, iface_subnets)
 
     @classmethod
@@ -518,7 +523,9 @@ class Renderer(renderer.Renderer):
 
             iface_subnets = iface.get("subnets", [])
             route_cfg = iface_cfg.routes
-            cls._render_subnets(iface_cfg, iface_subnets)
+            cls._render_subnets(
+                iface_cfg, iface_subnets, network_state.has_default_route
+            )
             cls._render_subnet_routes(iface_cfg, route_cfg, iface_subnets)
 
             # iter_interfaces on network-state is not sorted to produce
@@ -547,7 +554,9 @@ class Renderer(renderer.Renderer):
 
             iface_subnets = iface.get("subnets", [])
             route_cfg = iface_cfg.routes
-            cls._render_subnets(iface_cfg, iface_subnets)
+            cls._render_subnets(
+                iface_cfg, iface_subnets, network_state.has_default_route
+            )
             cls._render_subnet_routes(iface_cfg, route_cfg, iface_subnets)
 
     @staticmethod
@@ -608,7 +617,9 @@ class Renderer(renderer.Renderer):
 
             iface_subnets = iface.get("subnets", [])
             route_cfg = iface_cfg.routes
-            cls._render_subnets(iface_cfg, iface_subnets)
+            cls._render_subnets(
+                iface_cfg, iface_subnets, network_state.has_default_route
+            )
             cls._render_subnet_routes(iface_cfg, route_cfg, iface_subnets)
 
     @classmethod
@@ -620,7 +631,9 @@ class Renderer(renderer.Renderer):
             iface_cfg.kind = 'infiniband'
             iface_subnets = iface.get("subnets", [])
             route_cfg = iface_cfg.routes
-            cls._render_subnets(iface_cfg, iface_subnets)
+            cls._render_subnets(
+                iface_cfg, iface_subnets, network_state.has_default_route
+            )
             cls._render_subnet_routes(iface_cfg, route_cfg, iface_subnets)
 
     @classmethod
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index e3b9e02..468d544 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -860,6 +860,7 @@ NETWORK_CONFIGS = {
                 BOOTPROTO=dhcp
                 DEFROUTE=yes
                 DEVICE=eth99
+                DHCLIENT_SET_DEFAULT_ROUTE=yes
                 DNS1=8.8.8.8
                 DNS2=8.8.4.4
                 DOMAIN="barley.maas sach.maas"
@@ -1234,6 +1235,7 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
             'ifcfg-bond0.200': textwrap.dedent("""\
                 BOOTPROTO=dhcp
                 DEVICE=bond0.200
+                DHCLIENT_SET_DEFAULT_ROUTE=no
                 NM_CONTROLLED=no
                 ONBOOT=yes
                 PHYSDEV=bond0
@@ -1333,6 +1335,7 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
             'ifcfg-eth5': textwrap.dedent("""\
                 BOOTPROTO=dhcp
                 DEVICE=eth5
+                DHCLIENT_SET_DEFAULT_ROUTE=no
                 HWADDR=98:bb:9f:2c:e8:8a
                 NM_CONTROLLED=no
                 ONBOOT=no
@@ -1988,6 +1991,23 @@ CONFIG_V1_SIMPLE_SUBNET = {
                              'type': 'static'}],
                 'type': 'physical'}]}
 
+CONFIG_V1_MULTI_IFACE = {
+    'version': 1,
+    'config': [{'type': 'physical',
+                'mtu': 1500,
+                'subnets': [{'type': 'static',
+                             'netmask': '255.255.240.0',
+                             'routes': [{'netmask': '0.0.0.0',
+                                         'network': '0.0.0.0',
+                                         'gateway': '51.68.80.1'}],
+                             'address': '51.68.89.122',
+                             'ipv4': True}],
+                'mac_address': 'fa:16:3e:25:b4:59',
+                'name': 'eth0'},
+               {'type': 'physical',
+                'mtu': 9000,
+                'subnets': [{'type': 'dhcp4'}],
+                'mac_address': 'fa:16:3e:b1:ca:29', 'name': 'eth1'}]}
 
 DEFAULT_DEV_ATTRS = {
     'eth1000': {
@@ -2460,6 +2480,49 @@ USERCTL=no
         respath = '/etc/resolv.conf'
         self.assertNotIn(respath, found.keys())
 
+    def test_network_config_v1_multi_iface_samples(self):
+        ns = network_state.parse_net_config_data(CONFIG_V1_MULTI_IFACE)
+        render_dir = self.tmp_path("render")
+        os.makedirs(render_dir)
+        renderer = self._get_renderer()
+        renderer.render_network_state(ns, target=render_dir)
+        found = dir2dict(render_dir)
+        nspath = '/etc/sysconfig/network-scripts/'
+        self.assertNotIn(nspath + 'ifcfg-lo', found.keys())
+        expected_i1 = """\
+# Created by cloud-init on instance boot automatically, do not edit.
+#
+BOOTPROTO=none
+DEFROUTE=yes
+DEVICE=eth0
+GATEWAY=51.68.80.1
+HWADDR=fa:16:3e:25:b4:59
+IPADDR=51.68.89.122
+MTU=1500
+NETMASK=255.255.240.0
+NM_CONTROLLED=no
+ONBOOT=yes
+STARTMODE=auto
+TYPE=Ethernet
+USERCTL=no
+"""
+        self.assertEqual(expected_i1, found[nspath + 'ifcfg-eth0'])
+        expected_i2 = """\
+# Created by cloud-init on instance boot automatically, do not edit.
+#
+BOOTPROTO=dhcp
+DEVICE=eth1
+DHCLIENT_SET_DEFAULT_ROUTE=no
+HWADDR=fa:16:3e:b1:ca:29
+MTU=9000
+NM_CONTROLLED=no
+ONBOOT=yes
+STARTMODE=auto
+TYPE=Ethernet
+USERCTL=no
+"""
+        self.assertEqual(expected_i2, found[nspath + 'ifcfg-eth1'])
+
     def test_config_with_explicit_loopback(self):
         ns = network_state.parse_net_config_data(CONFIG_V1_EXPLICIT_LOOPBACK)
         render_dir = self.tmp_path("render")