Repository: Rackspace-DOT/nova-agent

Issue:
preserve firewalld zone
Firewalld is used in the Red Hat ecosystem (Fedora, RHEL, and CentOS).  To bind a network interface to a firewalld zone involves add `ZONE=zonename` to the ifcfg-ethX file.  It would be great if nova-agent can preserve zones in those files when making it's changes.

Discussion:

Patch:
diff --git a/novaagent/libs/centos.py b/novaagent/libs/centos.py
index cfe1afb..b39f9a6 100644
--- a/novaagent/libs/centos.py
+++ b/novaagent/libs/centos.py
@@ -5,6 +5,7 @@ from __future__ import absolute_import
 
 import logging
 import os
+import re
 
 
 from subprocess import Popen
@@ -32,7 +33,12 @@ class ServerOS(DefaultOS):
             self.interface_file_prefix,
             ifname
         )
+        # Check and see if there are extra arguments in ifcfg file
+        extra_args = self._check_for_extra_settings(interface_file)
+
+        # Backup the interface file
         utils.backup_file(interface_file)
+
         with open(interface_file, 'w') as iffile:
             iffile.write('# Automatically generated, do not edit\n\n')
             iffile.write('# Label {0}\n'.format(iface['label']))
@@ -92,6 +98,33 @@ class ServerOS(DefaultOS):
             iffile.write('ONBOOT=yes\n')
             iffile.write('NM_CONTROLLED=no\n')
 
+            if len(extra_args) > 0:
+                for argument in extra_args:
+                    iffile.write('{0}\n'.format(argument))
+
+    def _check_for_extra_settings(self, interface_file):
+        add_args = []
+
+        # The below setting are set in _setup_interface and also ignoring lines
+        # that start with # (comments) and lines with spaces at the beginning
+        known_settings = [
+            '^BOOTPROTO=', '^DEVICE=', '^GATEWAY=', '^IPV6INIT=', '^IPV6ADDR=',
+            '^IPV6_DEFAULTGW=', '^ONBOOT=', '^NM_CONTROLLED=', '^DNS\d+?=',
+            '^IPADDR\d+?=', '^NETMASK\d+?=', '^#', '^\s+'
+        ]
+        log.debug('Checking for additional arguments for ifcfg')
+        pattern = re.compile('|'.join(known_settings))
+        with open(interface_file, 'r') as file:
+            for line in file:
+                if not pattern.search(line):
+                    add_args.append(line.strip())
+
+        log.debug(
+            'Found {0} extra arguments to '
+            'add to ifcfg file'.format(len(add_args))
+        )
+        return add_args
+
     def _setup_routes(self, ifname, iface):
         route_file = '{0}/{1}-{2}'.format(
             self.netconfig_dir,


Test patch:
diff --git a/tests/fixtures/network.py b/tests/fixtures/network.py
index a33fe7b..02d4878 100644
--- a/tests/fixtures/network.py
+++ b/tests/fixtures/network.py
@@ -17,7 +17,9 @@ CENTOS_IFCFG_ETH1 = [
     'IPADDR=10.208.227.239\n',
     'NETMASK=255.255.224.0\n',
     'ONBOOT=yes\n',
-    'NM_CONTROLLED=no\n'
+    'NM_CONTROLLED=no\n',
+    'ZONE=TestFirewalldZone\n',
+    'TEST_OPTION=TEST_VALUE\n'
 ]
 
 CENTOS_IFCFG_ETH0 = [
@@ -38,7 +40,9 @@ CENTOS_IFCFG_ETH0 = [
     'DNS1=69.20.0.164\n',
     'DNS2=69.20.0.196\n',
     'ONBOOT=yes\n',
-    'NM_CONTROLLED=no\n'
+    'NM_CONTROLLED=no\n',
+    'ZONE=TestFirewalldZone\n',
+    'TEST_OPTION=TEST_VALUE\n'
 ]
 
 ETH0_INTERFACE = {
diff --git a/tests/tests_libs_centos.py b/tests/tests_libs_centos.py
index 697d643..82b3248 100644
--- a/tests/tests_libs_centos.py
+++ b/tests/tests_libs_centos.py
@@ -46,7 +46,14 @@ class TestHelpers(TestCase):
 
     def setup_temp_interface_config(self, interface):
         with open('/tmp/ifcfg-{0}'.format(interface), 'a+') as f:
-            f.write('This is a test file')
+            f.write(
+                'IPADDR999=1.1.1.1\n'
+                'ZONE=TestFirewalldZone\n'
+                '# Comment in file\n'
+                ' Starts with a space\n'
+                '      # Mulitple spaces\n'
+                'TEST_OPTION=TEST_VALUE\n'
+            )
 
     def setup_temp_hostname(self):
         with open('/tmp/hostname', 'a+') as f:
@@ -120,17 +127,20 @@ class TestHelpers(TestCase):
                             ) as ifcfg_files:
                                 ifcfg_files.return_value = ['/tmp/ifcfg-eth1']
                                 with mock.patch(
-                                    'novaagent.libs.centos.Popen'
-                                ) as p:
-                                    p.return_value.communicate.return_value = (
-                                        'out', 'error'
-                                    )
-                                    p.return_value.returncode = 0
-                                    result = temp.resetnetwork(
-                                        'name',
-                                        'value',
-                                        'dummy_client'
-                                    )
+                                    'novaagent.libs.centos.ServerOS.'
+                                    '_check_for_extra_settings'
+                                ) as check:
+                                    check.return_value = []
+                                    with mock.patch(
+                                        'novaagent.libs.centos.Popen'
+                                    ) as p:
+                                        p.return_value.communicate.return_value = ('out', 'error')  # noqa
+                                        p.return_value.returncode = 0
+                                        result = temp.resetnetwork(
+                                            'name',
+                                            'value',
+                                            'dummy_client'
+                                        )
 
         self.assertEqual(
             result,
@@ -197,29 +207,33 @@ class TestHelpers(TestCase):
                                 'novaagent.utils.get_ifcfg_files_to_remove'
                             ) as ifcfg_files:
                                 ifcfg_files.return_value = ['/tmp/ifcfg-eth1']
-
-                                mock_popen = mock.Mock()
-                                mock_comm = mock.Mock()
-                                mock_comm.return_value = ('out', 'error')
-                                mock_popen.side_effect = [
-                                    mock.Mock(
-                                        returncode=1,
-                                        communicate=mock_comm
-                                    ),
-                                    mock.Mock(
-                                        returncode=0,
-                                        communicate=mock_comm
-                                    )
-                                ]
                                 with mock.patch(
-                                    'novaagent.libs.centos.Popen',
-                                    side_effect=mock_popen
-                                ):
-                                    result = temp.resetnetwork(
-                                        'name',
-                                        'value',
-                                        'dummy_client'
-                                    )
+                                    'novaagent.libs.centos.ServerOS.'
+                                    '_check_for_extra_settings'
+                                ) as check:
+                                    check.return_value = []
+                                    mock_popen = mock.Mock()
+                                    mock_comm = mock.Mock()
+                                    mock_comm.return_value = ('out', 'error')
+                                    mock_popen.side_effect = [
+                                        mock.Mock(
+                                            returncode=1,
+                                            communicate=mock_comm
+                                        ),
+                                        mock.Mock(
+                                            returncode=0,
+                                            communicate=mock_comm
+                                        )
+                                    ]
+                                    with mock.patch(
+                                        'novaagent.libs.centos.Popen',
+                                        side_effect=mock_popen
+                                    ):
+                                        result = temp.resetnetwork(
+                                            'name',
+                                            'value',
+                                            'dummy_client'
+                                        )
 
         self.assertEqual(
             result,
@@ -287,17 +301,20 @@ class TestHelpers(TestCase):
                             ) as ifcfg_files:
                                 ifcfg_files.return_value = ['/tmp/ifcfg-eth1']
                                 with mock.patch(
-                                    'novaagent.libs.centos.Popen'
-                                ) as p:
-                                    p.return_value.communicate.return_value = (
-                                        'out', 'error'
-                                    )
-                                    p.return_value.returncode = 0
-                                    result = temp.resetnetwork(
-                                        'name',
-                                        'value',
-                                        'dummy_client'
-                                    )
+                                    'novaagent.libs.centos.ServerOS.'
+                                    '_check_for_extra_settings'
+                                ) as check:
+                                    check.return_value = []
+                                    with mock.patch(
+                                        'novaagent.libs.centos.Popen'
+                                    ) as p:
+                                        p.return_value.communicate.return_value = ('out', 'error')  # noqa
+                                        p.return_value.returncode = 0
+                                        result = temp.resetnetwork(
+                                            'name',
+                                            'value',
+                                            'dummy_client'
+                                        )
 
         self.assertEqual(
             result,
@@ -358,17 +375,20 @@ class TestHelpers(TestCase):
                             ) as ifcfg_files:
                                 ifcfg_files.return_value = ['/tmp/ifcfg-eth1']
                                 with mock.patch(
-                                    'novaagent.libs.centos.Popen'
-                                ) as p:
-                                    p.return_value.communicate.return_value = (
-                                        'out', 'error'
-                                    )
-                                    p.return_value.returncode = 1
-                                    result = temp.resetnetwork(
-                                        'name',
-                                        'value',
-                                        'dummy_client'
-                                    )
+                                    'novaagent.libs.centos.ServerOS.'
+                                    '_check_for_extra_settings'
+                                ) as check:
+                                    check.return_value = []
+                                    with mock.patch(
+                                        'novaagent.libs.centos.Popen'
+                                    ) as p:
+                                        p.return_value.communicate.return_value = ('out', 'error')  # noqa
+                                        p.return_value.returncode = 1
+                                        result = temp.resetnetwork(
+                                            'name',
+                                            'value',
+                                            'dummy_client'
+                                        )
 
         self.assertEqual(
             result,
@@ -446,15 +466,20 @@ class TestHelpers(TestCase):
                                         '/tmp/ifcfg-eth1'
                                     ]
                                     with mock.patch(
-                                        'novaagent.libs.centos.Popen'
-                                    ) as p:
-                                        p.return_value.communicate.return_value = ('out', 'error')  # noqa
-                                        p.return_value.returncode = 0
-                                        result = temp.resetnetwork(
-                                            'name',
-                                            'value',
-                                            'dummy_client'
-                                        )
+                                        'novaagent.libs.centos.ServerOS.'
+                                        '_check_for_extra_settings'
+                                    ) as check:
+                                        check.return_value = []
+                                        with mock.patch(
+                                            'novaagent.libs.centos.Popen'
+                                        ) as p:
+                                            p.return_value.communicate.return_value = ('out', 'error')  # noqa
+                                            p.return_value.returncode = 0
+                                            result = temp.resetnetwork(
+                                                'name',
+                                                'value',
+                                                'dummy_client'
+                                            )
 
         self.assertEqual(
             result,
@@ -531,15 +556,20 @@ class TestHelpers(TestCase):
                                         '/tmp/ifcfg-eth1'
                                     ]
                                     with mock.patch(
-                                        'novaagent.libs.centos.Popen'
-                                    ) as p:
-                                        p.return_value.communicate.return_value = ('out', 'error')  # noqa
-                                        p.return_value.returncode = 1
-                                        result = temp.resetnetwork(
-                                            'name',
-                                            'value',
-                                            'dummy_client'
-                                        )
+                                        'novaagent.libs.centos.ServerOS.'
+                                        '_check_for_extra_settings'
+                                    ) as check:
+                                        check.return_value = []
+                                        with mock.patch(
+                                            'novaagent.libs.centos.Popen'
+                                        ) as p:
+                                            p.return_value.communicate.return_value = ('out', 'error')  # noqa
+                                            p.return_value.returncode = 1
+                                            result = temp.resetnetwork(
+                                                'name',
+                                                'value',
+                                                'dummy_client'
+                                            )
 
         self.assertEqual(
             result,
@@ -571,6 +601,23 @@ class TestHelpers(TestCase):
             'Localhost ifcfg file was moved out of the way and should not have'
         )
 
+    def test_check_extra_args(self):
+        self.setup_temp_interface_config('eth1')
+        temp = centos.ServerOS()
+        interface_file = '/tmp/ifcfg-eth1'
+
+        extra_args = temp._check_for_extra_settings(interface_file)
+        self.assertEqual(
+            len(extra_args),
+            2,
+            'Did not get proper number of arguments from check'
+        )
+        self.assertEqual(
+            extra_args,
+            ['ZONE=TestFirewalldZone', 'TEST_OPTION=TEST_VALUE'],
+            'Did not get proper extra arguments from check'
+        )
+
     def test_setup_routes(self):
         self.setup_temp_route()
         temp = centos.ServerOS()