Merge ~rmccabe/cloud-init:bug1693251 into cloud-init:master

[Ryan McCabe]

In cases where the config json specifies nameserver entries, NetworkManager, if enabled, will clobber the /etc/resolv.conf that cloud-init has produced, which can break dns. If at least one interface is configured for dhcp, you might end up with a resolv.conf that doesn't function as intended, and if you don't have any interfaces that get dns from dhcp, NetworkManager could clobber resolv.conf with an empty file.

This patch adds a mechanism for dropping additional configuration into /etc/NetworkManager/conf.d/. I figured at some point, somebody will need to set some other keypair to stop NetworkManager from misbehaving, so I added something for that instead of simply writing the particular 2 lines of config to fix the resolv.conf issue.

This patch only writes out that configuration file if necessary (as opposed to creating an empty file when there is no config needed). I wasn't sure which was preferred, so if that's not ideal, I could write out the empty file for consistency.

This patch will also write that config "(dns=none") out, too, for the specific problem noted in #1693251

LP: #1693251

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

PASSED: Continuous integration, rev:7a49497502aad683297f36666c4d025a66330cc4
https://jenkins.ubuntu.com/server/job/cloud-init-ci/489/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/489
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/489
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/489
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/489
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/489

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

review: Approve (continuous-integration)

[Lars Kellogg-Stedman (larsks)]

I've added a couple of inline comments.

[Lars Kellogg-Stedman (larsks)]

Other than a minor quibble about where we are rendering the header for the NM configuration file, I think this looks fine.

review: Approve

[Scott Moser (smoser)]

Some replies.
I like to get those addressed.
Over all this is very good though. Thank you Ryan!

review: Needs Fixing

[Ryan McCabe (rmccabe)]

Regarding the first question, I tested for network_state.dns_nameservers instead of self.dns_path because the test will always evaluate to true if it were testing only self.dns_path, since 'etc/resolv.conf' is provided as a default, and we don't want to drop the additional config for NM when no dns servers are provided.

Regarding the second question, I did the rendering the way I did in the patch to be consistent with how the rest of the files are being rendered, since the _make_header() function that writes out the "Created cloud-init..." lines is defined at the top of that sysconfig.py file. I could move it elsewhere so it can be reused, though, if you'd prefer that.

[Scott Moser (smoser)]

I'm fine with what is here now.

review: Approve

diff --git a/cloudinit/distros/parsers/networkmanager_conf.py b/cloudinit/distros/parsers/networkmanager_conf.py
new file mode 100644
index 0000000..ac51f12
--- /dev/null
+++ b/cloudinit/distros/parsers/networkmanager_conf.py
@@ -0,0 +1,23 @@
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# Author: Ryan McCabe <[e-mail]>
+#
+# This file is part of cloud-init. See LICENSE file for license information.
+
+import configobj
+
+# This module is used to set additional NetworkManager configuration
+# in /etc/NetworkManager/conf.d
+#
+
+
+class NetworkManagerConf(configobj.ConfigObj):
+    def __init__(self, contents):
+        configobj.ConfigObj.__init__(self, contents,
+                                     interpolation=False,
+                                     write_empty_values=False)
+
+    def set_section_keypair(self, section_name, key, value):
+        if section_name not in self.sections:
+            self.main[section_name] = {}
+        self.main[section_name] = {key: value}
diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
index f7d4548..b5c90a6 100644
--- a/cloudinit/net/sysconfig.py
+++ b/cloudinit/net/sysconfig.py
@@ -5,6 +5,7 @@ import re
 
 import six
 
+from cloudinit.distros.parsers import networkmanager_conf
 from cloudinit.distros.parsers import resolv_conf
 from cloudinit import util
 
@@ -252,6 +253,9 @@ class Renderer(renderer.Renderer):
         self.netrules_path = config.get(
             'netrules_path', 'etc/udev/rules.d/70-persistent-net.rules')
         self.dns_path = config.get('dns_path', 'etc/resolv.conf')
+        nm_conf_path = 'etc/NetworkManager/conf.d/99-cloud-init.conf'
+        self.networkmanager_conf_path = config.get('networkmanager_conf_path',
+                                                   nm_conf_path)
 
     @classmethod
     def _render_iface_shared(cls, iface, iface_cfg):
@@ -447,6 +451,21 @@ class Renderer(renderer.Renderer):
             content.add_search_domain(searchdomain)
         return "\n".join([_make_header(';'), str(content)])
 
+    @staticmethod
+    def _render_networkmanager_conf(network_state):
+        content = networkmanager_conf.NetworkManagerConf("")
+
+        # If DNS server information is provided, configure
+        # NetworkManager to not manage dns, so that /etc/resolv.conf
+        # does not get clobbered.
+        if network_state.dns_nameservers:
+            content.set_section_keypair('main', 'dns', 'none')
+
+        if len(content) == 0:
+            return None
+        out = "".join([_make_header(), "\n", "\n".join(content.write()), "\n"])
+        return out
+
     @classmethod
     def _render_bridge_interfaces(cls, network_state, iface_contents):
         bridge_filter = renderer.filter_by_type('bridge')
@@ -507,6 +526,12 @@ class Renderer(renderer.Renderer):
             resolv_content = self._render_dns(network_state,
                                               existing_dns_path=dns_path)
             util.write_file(dns_path, resolv_content, file_mode)
+        if self.networkmanager_conf_path:
+            nm_conf_path = util.target_path(target,
+                                            self.networkmanager_conf_path)
+            nm_conf_content = self._render_networkmanager_conf(network_state)
+            if nm_conf_content:
+                util.write_file(nm_conf_path, nm_conf_content, file_mode)
         if self.netrules_path:
             netrules_content = self._render_persistent_net(network_state)
             netrules_path = util.target_path(target, self.netrules_path)
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 0a88caf..c8c7628 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -156,6 +156,13 @@ USERCTL=no
 ;
 nameserver 172.19.0.12
 """.lstrip()),
+            ('etc/NetworkManager/conf.d/99-cloud-init.conf',
+             """
+# Created by cloud-init on instance boot automatically, do not edit.
+#
+[main]
+dns = none
+""".lstrip()),
             ('etc/udev/rules.d/70-persistent-net.rules',
              "".join(['SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ',
                       'ATTR{address}=="fa:16:3e:ed:9a:59", NAME="eth0"\n']))]
@@ -217,6 +224,13 @@ USERCTL=no
 ;
 nameserver 172.19.0.12
 """.lstrip()),
+            ('etc/NetworkManager/conf.d/99-cloud-init.conf',
+             """
+# Created by cloud-init on instance boot automatically, do not edit.
+#
+[main]
+dns = none
+""".lstrip()),
             ('etc/udev/rules.d/70-persistent-net.rules',
              "".join(['SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ',
                       'ATTR{address}=="fa:16:3e:ed:9a:59", NAME="eth0"\n']))]
@@ -300,6 +314,13 @@ USERCTL=no
 ;
 nameserver 172.19.0.12
 """.lstrip()),
+            ('etc/NetworkManager/conf.d/99-cloud-init.conf',
+             """
+# Created by cloud-init on instance boot automatically, do not edit.
+#
+[main]
+dns = none
+""".lstrip()),
             ('etc/udev/rules.d/70-persistent-net.rules',
              "".join(['SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ',
                       'ATTR{address}=="fa:16:3e:ed:9a:59", NAME="eth0"\n']))]