launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #11627
[Merge] lp:~jtv/maas/apply-dhcp_interfaces into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/apply-dhcp_interfaces into lp:maas with lp:~jtv/maas/pre-apply-dhcp_interfaces as a prerequisite.
Requested reviews:
MAAS Maintainers (maas-maintainers)
For more details, see:
https://code.launchpad.net/~jtv/maas/apply-dhcp_interfaces/+merge/122796
See the commit message. Pre-imp for this was extensive. Based on past experience we did not see any viable alternative to writing to this config file.
Jeroen
--
https://code.launchpad.net/~jtv/maas/apply-dhcp_interfaces/+merge/122796
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/apply-dhcp_interfaces into lp:maas.
=== modified file 'etc/celeryconfig.py'
--- etc/celeryconfig.py 2012-08-24 08:39:18 +0000
+++ etc/celeryconfig.py 2012-09-05 06:21:27 +0000
@@ -40,6 +40,10 @@
# ISC dhcpd configuration file.
DHCP_CONFIG_FILE = '/etc/dhcp/dhcpd.conf'
+# The default config for the ISC dhcpd. This is the only place where
+# we can configure which interfaces it should serve DHCP on.
+DHCP_DEFAULT_CONFIG_FILE = '/etc/default/isc-dhcp-server'
+
try:
import user_maasceleryconfig
=== modified file 'src/provisioningserver/dhcp/config.py'
--- src/provisioningserver/dhcp/config.py 2012-08-21 15:59:48 +0000
+++ src/provisioningserver/dhcp/config.py 2012-09-05 06:21:27 +0000
@@ -13,6 +13,7 @@
__all__ = [
"DHCPConfigError",
"get_config",
+ "get_default_config",
]
@@ -26,6 +27,14 @@
"""Exception raised for errors processing the DHCP config."""
+# Markers that bracket the default config snippet that this module will
+# (re)write.
+maas_config_markers = (
+ "## Begin MAAS settings. Do not edit; MAAS will overwrite these.",
+ "## End MAAS settings.",
+ )
+
+
template_content = dedent("""\
subnet {{subnet}} netmask {{subnet_mask}} {
next-server {{next_server}};
@@ -71,3 +80,66 @@
return template.substitute(params)
except NameError, error:
raise DHCPConfigError(*error.args)
+
+
+def find_list_item(item, in_list, starting_at=0):
+ """Return the index where `item` first occurs in `in_list`, or None."""
+ try:
+ return in_list.index(item, starting_at)
+ except ValueError:
+ return None
+
+
+def get_default_config(default_config, dhcp_interfaces):
+ """Return "default" ISC-DHCPD config that needs writing, or None.
+
+ This reads /etc/default/isc-dhcp-server and, if it does not match the
+ new configuration, produces a new text that it should be replaced with.
+
+ :param default_config: Name of the file containing the current default
+ config.
+ :param dhcp_interfaces: Space-separated string of network interfaces
+ to serve DHCP on.
+ :return: New contents for default isc-dhcp-server config, or None if
+ it does not need rewriting.
+ """
+ encoding = 'utf-8'
+ header, footer = maas_config_markers
+
+ with open(default_config, 'rb') as config_file:
+ existing_config = config_file.read()
+ lines = existing_config.decode(encoding).splitlines()
+
+ new_custom_section = 'INTERFACES="%s"' % dhcp_interfaces
+
+ header_index = find_list_item(header, lines)
+ if header_index is not None:
+ footer_index = find_list_item(footer, lines, header_index)
+ if footer_index is None:
+ # There's a header but no footer. Pretend we didn't see the
+ # header; just append a new custom section at the end. Any
+ # subsequent rewrite will replace the part starting at the
+ # header and ending at the header we will add here. At that
+ # point there will be no trace of the strange situation
+ # left.
+ header_index = None
+
+ if header_index is None:
+ # There was no MAAS custom section in this file. Append it at
+ # the end. Our INTERFACES setting will supersede any that was
+ # already in the file, but leave it in there.
+ lines += [
+ header,
+ new_custom_section,
+ footer,
+ ]
+ elif lines[(header_index + 1):footer_index] == [new_custom_section]:
+ return None
+ else:
+ # There is a MAAS custom section in the file. Replace it.
+ lines = (
+ lines[:(header_index + 1)] +
+ [new_custom_section] +
+ lines[footer_index:])
+
+ return '\n'.join(lines) + '\n'
=== modified file 'src/provisioningserver/dhcp/tests/test_config.py'
--- src/provisioningserver/dhcp/tests/test_config.py 2012-08-21 15:59:48 +0000
+++ src/provisioningserver/dhcp/tests/test_config.py 2012-09-05 06:21:27 +0000
@@ -14,12 +14,16 @@
from textwrap import dedent
+from maastesting.factory import factory
from maastesting.matchers import Contains
from provisioningserver.dhcp import config
from provisioningserver.pxe.tftppath import compose_bootloader_path
+from provisioningserver.testing.testcase import PservTestCase
import tempita
-from testtools import TestCase
-from testtools.matchers import MatchesRegex
+from testtools.matchers import (
+ EndsWith,
+ MatchesRegex,
+ )
# Simple test version of the DHCP template. Contains parameter
# substitutions, but none that aren't also in the real template.
@@ -54,7 +58,7 @@
)
-class TestDHCPConfig(TestCase):
+class TestDHCPConfig(PservTestCase):
def patch_template(self, template_content=sample_template):
"""Patch the DHCP config template with the given contents."""
@@ -87,3 +91,99 @@
params = make_sample_params()
output = config.get_config(**params)
self.assertThat(output, Contains(compose_bootloader_path()))
+
+ def test_get_default_config_returns_None_if_no_changes_needed(self):
+ dhcp_interfaces = factory.make_name('interface')
+ existing_file = self.make_file(
+ contents=config.get_default_config(
+ self.make_file(), dhcp_interfaces))
+ self.assertIsNone(
+ config.get_default_config(existing_file, dhcp_interfaces))
+
+ def test_get_default_config_adds_custom_section_initially(self):
+ dhcp_interfaces = factory.make_name('interface')
+ header, footer = config.maas_config_markers
+ self.assertIn(
+ '\n'.join([
+ header,
+ 'INTERFACES="%s"' % dhcp_interfaces,
+ footer,
+ ]),
+ config.get_default_config(self.make_file(), dhcp_interfaces))
+
+ def test_get_default_config_puts_custom_config_at_end(self):
+ header, footer = config.maas_config_markers
+ self.assertThat(
+ config.get_default_config(
+ self.make_file(), factory.make_name('interface')),
+ EndsWith(footer + '\n'))
+
+ def test_get_default_config_replaces_custom_section_only(self):
+ # Any configuration text outside of the custom section is left
+ # intact, but the text inside gets replaced.
+ old_interfaces = factory.make_name('old-interface')
+ header, footer = config.maas_config_markers
+ original_config = self.make_file(contents=dedent("""\
+ # Text before custom config.
+ INTERFACES=""
+ %s
+ INTERFACES="%s"
+ %s
+ # Text after custom config.
+ """) % (header, old_interfaces, footer))
+ new_interfaces = factory.make_name('new-interface')
+ new_contents = dedent("""\
+ # Text before custom config.
+ INTERFACES=""
+ %s
+ INTERFACES="%s"
+ %s
+ # Text after custom config.
+ """) % (header, new_interfaces, footer)
+ self.assertEqual(
+ new_contents,
+ config.get_default_config(original_config, new_interfaces))
+
+ def test_get_default_config_ignores_header_without_footer(self):
+ # If the footer of the custom config section is not found,
+ # get_default_config will pretend that the header is not there
+ # and append a new custom section. This does mean that there
+ # will be two headers and one footer; a subsequent rewrite will
+ # replace everything from the first header to the footer.
+ header, footer = config.maas_config_markers
+ original_config = self.make_file(contents=dedent("""\
+ %s
+ # Presumable custom section without footer.
+ """) % header)
+ new_interfaces = factory.make_name('new-interface')
+ new_contents = dedent("""\
+ %s
+ # Presumable custom section without footer.
+ %s
+ INTERFACES="%s"
+ %s
+ """) % (header, header, new_interfaces, footer)
+ self.assertEqual(
+ new_contents,
+ config.get_default_config(original_config, new_interfaces))
+
+ def test_get_default_config_ignores_second_header(self):
+ # If there are two headers and one footer in the config file,
+ # get_default_config will ignore the second header.
+ header, footer = config.maas_config_markers
+ original_config = self.make_file(contents=dedent("""\
+ %s
+ # Presumable custom section but with second header.
+ %s
+ # Definite custom section.
+ %s
+ """) % (header, header, footer))
+ new_interfaces = factory.make_name('new-interface')
+ new_contents = dedent("""\
+ %s
+ INTERFACES="%s"
+ %s
+ """) % (header, new_interfaces, footer)
+ self.assertEqual(
+ new_contents,
+ config.get_default_config(original_config, new_interfaces))
=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py 2012-09-03 13:29:29 +0000
+++ src/provisioningserver/tasks.py 2012-09-05 06:21:27 +0000
@@ -31,7 +31,10 @@
)
from celery.task import task
-from celeryconfig import DHCP_CONFIG_FILE
+from celeryconfig import (
+ DHCP_CONFIG_FILE,
+ DHCP_DEFAULT_CONFIG_FILE,
+ )
from provisioningserver.auth import (
record_api_credentials,
record_maas_url,
@@ -302,17 +305,32 @@
raise
+def sudo_write_file(filename, contents, mode=0744, encoding='ascii'):
+ """(Re)write a file, atomically, under sudo. Use with extreme care."""
+ proc = Popen([
+ 'sudo', 'maas-provision', 'atomic-write',
+ '--filename', filename,
+ '--mode', oct(mode),
+ ],
+ stdin=PIPE)
+ proc.communicate(contents.encode(encoding))
+
+
@task
def write_dhcp_config(**kwargs):
"""Write out the DHCP configuration file and restart the DHCP server.
:param **kwargs: Keyword args passed to dhcp.config.get_config()
"""
- output = config.get_config(**kwargs).encode("ascii")
- proc = Popen(
- ["sudo", "maas-provision", "atomic-write", "--filename",
- DHCP_CONFIG_FILE, "--mode", "744"], stdin=PIPE)
- proc.communicate(output)
+ sudo_write_file(DHCP_CONFIG_FILE, config.get_config(**kwargs))
+
+ # The interfaces that the DHCP server should serve on can only be
+ # set through a variable INTERFACES in /etc/default/isc-dhcp-server.
+ default_config = config.get_default_config(
+ DHCP_DEFAULT_CONFIG_FILE, kwargs['dhcp_interfaces'])
+ if default_config is not None:
+ sudo_write_file(DHCP_DEFAULT_CONFIG_FILE, default_config)
+
restart_dhcp_server()
=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py 2012-09-04 10:20:35 +0000
+++ src/provisioningserver/tests/test_tasks.py 2012-09-05 06:21:27 +0000
@@ -15,7 +15,10 @@
from datetime import datetime
import os
import random
-from subprocess import CalledProcessError
+from subprocess import (
+ CalledProcessError,
+ PIPE,
+ )
from apiclient.creds import convert_tuple_to_string
from celeryconfig import DHCP_CONFIG_FILE
@@ -157,6 +160,7 @@
def make_dhcp_config_params(self):
"""Fake up a dict of dhcp configuration parameters."""
param_names = [
+ 'dhcp_interfaces',
'omapi_key',
'subnet',
'subnet_mask',
@@ -237,15 +241,13 @@
write_dhcp_config(**config_params)
# It should construct Popen with the right parameters.
- popen_args = mocked_popen.call_args[0][0]
- self.assertEqual(
- popen_args,
+ mocked_popen.assert_any_call(
["sudo", "maas-provision", "atomic-write", "--filename",
- DHCP_CONFIG_FILE, "--mode", "744"])
+ DHCP_CONFIG_FILE, "--mode", "0744"], stdin=PIPE)
# It should then pass the content to communicate().
content = config.get_config(**config_params).encode("ascii")
- mocked_proc.communicate.assert_called_once_with(content)
+ mocked_proc.communicate.assert_any_call(content)
# Finally it should restart the dhcp server.
check_call_args = mocked_check_call.call_args
@@ -253,6 +255,39 @@
check_call_args[0][0],
['sudo', 'service', 'isc-dhcp-server', 'restart'])
+ def test_write_dhcp_config_updates_config_in_etc_default_if_needed(self):
+ self.patch(tasks, 'check_call')
+ self.patch(tasks, 'sudo_write_file')
+ default_config = self.make_file(contents='INTERFACES=""\n')
+ self.patch(tasks, 'DHCP_DEFAULT_CONFIG_FILE', default_config)
+ config_params = self.make_dhcp_config_params()
+
+ write_dhcp_config(**config_params)
+
+ written_files = [
+ args[0][0] for args in tasks.sudo_write_file.call_args_list]
+ self.assertIn(default_config, written_files)
+ our_call = written_files.index(default_config)
+ self.assertIn(
+ 'INTERFACES="%s"' % config_params['dhcp_interfaces'],
+ tasks.sudo_write_file.call_args_list[our_call][0][1])
+
+ def test_write_dhcp_config_leaves_etc_default_alone_if_unchanged(self):
+ self.patch(tasks, 'check_call')
+ self.patch(tasks, 'sudo_write_file')
+ config_params = self.make_dhcp_config_params()
+ default_config = self.make_file(contents=config.get_default_config(
+ self.make_file(), config_params['dhcp_interfaces']))
+ self.patch(tasks, 'DHCP_DEFAULT_CONFIG_FILE', default_config)
+
+ write_dhcp_config(**config_params)
+
+ # The regular config file was written, but the default one
+ # was not.
+ self.assertEqual(
+ [DHCP_CONFIG_FILE],
+ [call[0][0] for call in tasks.sudo_write_file.call_args_list])
+
def test_restart_dhcp_server_sends_command(self):
recorder = FakeMethod()
self.patch(tasks, 'check_call', recorder)
Follow ups
