Repository: Azure/WALinuxAgent (python)

Issue:
[2.2.0+]Fail to send hostname to DHCP server during provisioning in RHEL-7
Description of problem:
In RHEL-7, if NetworkManager is enabled, the hostname is not sent to DHCP server during provisioning, so that the VM internal DNS is not set to the DNS server.

Version-Release number of selected component (if applicable):
WALinuxAgent-2.2.0+

RHEL Version:
RHEL-7.3
NetworkManager-1.4.0-12

How reproducible:
100%

Steps to Reproduce:
1. Create a RHEL-7.3 VM on Azure with WALA-2.2.0+ installed(NetworkManager enabled)
2. Check if send hostname to DHCP server:
~]# cat /var/lib/NetworkManager/dhclient-eth0.conf|grep send
There's no "send host-name" line.
3. Check DNS:
1). Get domain-name(like wala73220e.i6.internal.cloudapp.net): cat /etc/resolv.conf|grep search
2). Get hostname(like wala73220e): cat /etc/hostname
3). Check DNS:
~]# nslookup wala73220e.wala73220e.i6.internal.cloudapp.net
Server:                168.63.129.16
Address:        168.63.129.16#53

** server can't find wala73220e.wala73220e.i6.internal.cloudapp.net: NXDOMAIN

Actual Result:
The hostname is not sent to the DHCP server
The domain name is not added into DNS server

Expect Result:
The hostname is sent to the DHCP server, such as:
~]# cat /var/lib/NetworkManager/dhclient-eth0.conf |grep send
send host-name "wala73220e"; # added by NetworkManager

The domain name is added into DNS server, such as:
~]# nslookup wala73220e.wala73220e.i6.internal.cloudapp.net
Server:                168.63.129.16
Address:        168.63.129.16#53

Name:        wala73220e.wala73220e.i6.internal.cloudapp.net
Address: 10.0.0.5


Additional Info:
There're 2 root causes:

1. WALA-2.2.0 use "hostname" and write "/etc/hostname" file to set hostname, which cannot send hostname message into dbus. NetworkManager get hostname from dbus, then add "send host-name" into /var/lib/NetworkManager/dhclient-eth0.conf

2. WALA-2.2.0 write "DHCP_HOSTNAME=hostname" into /etc/sysconfig/network-scripts/ifcfg-eth0, but there's no '\n' after this line. So that NetworkManager cannot read it.

NetworkManager sends the hostname to the DHCP server using the following strategy:
- if the ifcfg file specifies a DHCP_HOSTNAME, that value is sent to the DHCP server

- if DHCP_HOSTNAME is not set, NM uses the system hostname set through hostnamectl, but only if it is not '(none)', 'localhost', 'localhost6' or 'localhost.localdomain'. If it is one of these, no hostname is sent

According to the NetworkManager developer suggestion, the best way to have NetworkManager send the hostname is to simply set a valid system hostname using 'hostnamectl set-hostname myhostname' and reactivate the connection with ifup.
The DHCP_HOSTNAME is only a way to override the system configured hostname.

So we suggest to:
1. use "hostnamectl set-hostname" command to set hostname in RHEL-7.
2. remove the "DHCP_HOSTNAME". If you need to use this parameter, please add '\n' after it.

Pull request description:
ensure config file is updated correctly
In `fileutil.update_conf_file` we do not append a final newline, which can cause values to become unreadable. Also, when updating a value, we can potentially add multiple newline characters. This change addresses both cases.

- fixes #538 

/cc @brendandixon @jinhyunr 

Patch:
diff --git a/azurelinuxagent/common/osutil/redhat.py b/azurelinuxagent/common/osutil/redhat.py
index 8e86f6ec..fef9e152 100644
--- a/azurelinuxagent/common/osutil/redhat.py
+++ b/azurelinuxagent/common/osutil/redhat.py
@@ -84,7 +84,8 @@ class Redhat6xOSUtil(DefaultOSUtil):
     def set_dhcp_hostname(self, hostname):
         ifname = self.get_if_name()
         filepath = "/etc/sysconfig/network-scripts/ifcfg-{0}".format(ifname)
-        fileutil.update_conf_file(filepath, 'DHCP_HOSTNAME',
+        fileutil.update_conf_file(filepath,
+                                  'DHCP_HOSTNAME',
                                   'DHCP_HOSTNAME={0}'.format(hostname))
 
     def get_dhcp_lease_endpoint(self):
@@ -117,5 +118,11 @@ class RedhatOSUtil(Redhat6xOSUtil):
         DefaultOSUtil.openssl_to_openssh(self, input_file, output_file)
 
     def get_dhcp_lease_endpoint(self):
-        # centos7 has this weird naming with double hyphen like /var/lib/dhclient/dhclient--eth0.lease
-        return self.get_endpoint_from_leases_path('/var/lib/dhclient/dhclient-*.lease')
+        # dhclient
+        endpoint = self.get_endpoint_from_leases_path('/var/lib/dhclient/dhclient-*.lease')
+
+        if endpoint is None:
+            # NetworkManager
+            endpoint = self.get_endpoint_from_leases_path('/var/lib/NetworkManager/dhclient-*.lease')
+
+        return endpoint
diff --git a/azurelinuxagent/common/utils/fileutil.py b/azurelinuxagent/common/utils/fileutil.py
index b0b6fb75..8713d0c6 100644
--- a/azurelinuxagent/common/utils/fileutil.py
+++ b/azurelinuxagent/common/utils/fileutil.py
@@ -140,9 +140,9 @@ def update_conf_file(path, line_start, val, chk_err=False):
     if not os.path.isfile(path) and chk_err:
         raise IOError("Can't find config file:{0}".format(path))
     conf = read_file(path).split('\n')
-    conf = [x for x in conf if not x.startswith(line_start)]
+    conf = [x for x in conf if x is not None and len(x) > 0 and not x.startswith(line_start)]
     conf.append(val)
-    write_file(path, '\n'.join(conf))
+    write_file(path, '\n'.join(conf) + '\n')
 
 def search_file(target_dir_name, target_file_name):
     for root, dirs, files in os.walk(target_dir_name):
diff --git a/azurelinuxagent/pa/deprovision/default.py b/azurelinuxagent/pa/deprovision/default.py
index 74f38544..ced87eef 100644
--- a/azurelinuxagent/pa/deprovision/default.py
+++ b/azurelinuxagent/pa/deprovision/default.py
@@ -88,8 +88,9 @@ class DeprovisionHandler(object):
         dirs_to_del = ["/var/lib/dhclient", "/var/lib/dhcpcd", "/var/lib/dhcp"]
         actions.append(DeprovisionAction(fileutil.rm_dirs, dirs_to_del))
 
-        # For Freebsd
-        actions.append(DeprovisionAction(fileutil.rm_files, ["/var/db/dhclient.leases.hn0"]))
+        # For Freebsd, NM controlled
+        actions.append(DeprovisionAction(fileutil.rm_files, ["/var/db/dhclient.leases.hn0",
+                                                             "/var/lib/NetworkManager/dhclient-*.lease"]))
 
     def del_lib_dir(self, warnings, actions):
         dirs_to_del = [conf.get_lib_dir()]


Test patch:
diff --git a/tests/utils/test_file_util.py b/tests/utils/test_file_util.py
index f16f4099..76fb15ba 100644
--- a/tests/utils/test_file_util.py
+++ b/tests/utils/test_file_util.py
@@ -15,13 +15,12 @@
 # Requires Python 2.4+ and Openssl 1.0+
 #
 
-from tests.tools import *
 import uuid
-import unittest
-import os
-import sys
-from azurelinuxagent.common.future import ustr
+
 import azurelinuxagent.common.utils.fileutil as fileutil
+from azurelinuxagent.common.future import ustr
+from tests.tools import *
+
 
 class TestFileOperations(AgentTestCase):
 
@@ -117,5 +116,66 @@ class TestFileOperations(AgentTestCase):
 
         self.assertEqual(set(expected_files), set(actual_files))
 
+    @patch('os.path.isfile')
+    def test_update_conf_file(self, _):
+        new_file = "\
+DEVICE=eth0\n\
+ONBOOT=yes\n\
+BOOTPROTO=dhcp\n\
+TYPE=Ethernet\n\
+USERCTL=no\n\
+PEERDNS=yes\n\
+IPV6INIT=no\n\
+NM_CONTROLLED=yes\n"
+
+        existing_file = "\
+DEVICE=eth0\n\
+ONBOOT=yes\n\
+BOOTPROTO=dhcp\n\
+TYPE=Ethernet\n\
+DHCP_HOSTNAME=existing\n\
+USERCTL=no\n\
+PEERDNS=yes\n\
+IPV6INIT=no\n\
+NM_CONTROLLED=yes\n"
+
+        bad_file = "\
+DEVICE=eth0\n\
+ONBOOT=yes\n\
+BOOTPROTO=dhcp\n\
+TYPE=Ethernet\n\
+USERCTL=no\n\
+PEERDNS=yes\n\
+IPV6INIT=no\n\
+NM_CONTROLLED=yes\n\
+DHCP_HOSTNAME=no_new_line"
+
+        updated_file = "\
+DEVICE=eth0\n\
+ONBOOT=yes\n\
+BOOTPROTO=dhcp\n\
+TYPE=Ethernet\n\
+USERCTL=no\n\
+PEERDNS=yes\n\
+IPV6INIT=no\n\
+NM_CONTROLLED=yes\n\
+DHCP_HOSTNAME=test\n"
+
+        path = 'path'
+        with patch.object(fileutil, 'write_file') as patch_write:
+            with patch.object(fileutil, 'read_file', return_value=new_file):
+                fileutil.update_conf_file(path, 'DHCP_HOSTNAME', 'DHCP_HOSTNAME=test')
+                patch_write.assert_called_once_with(path, updated_file)
+
+        with patch.object(fileutil, 'write_file') as patch_write:
+            with patch.object(fileutil, 'read_file', return_value=existing_file):
+                fileutil.update_conf_file(path, 'DHCP_HOSTNAME', 'DHCP_HOSTNAME=test')
+                patch_write.assert_called_once_with(path, updated_file)
+
+        with patch.object(fileutil, 'write_file') as patch_write:
+            with patch.object(fileutil, 'read_file', return_value=bad_file):
+                fileutil.update_conf_file(path, 'DHCP_HOSTNAME', 'DHCP_HOSTNAME=test')
+                patch_write.assert_called_once_with(path, updated_file)
+
 if __name__ == '__main__':
     unittest.main()