From 228a2a7885e1b04d4180fe8daa2992884decaf6d Mon Sep 17 00:00:00 2001 From: Vasyl Saienko Date: Thu, 21 Sep 2017 13:39:28 +0000 Subject: [PATCH] Change pxe dhcp options name to codes. There is difference between dhcp option names in different backends. This patch changes options name to code according to [0]. [0] https://www.iana.org/assignments/bootp-dhcp-parameters/bootp-dhcp-parameters.xhtml Closes-Bug: 1717236 This is an updated version of c377f5cbbd034e16b68a3fc30e138b03badc9c94 which problems with PXE and dnsmasq due to buggy dnsmasq code which uses siaddr field to specify tftp server. They are addressed now by always sending server-ip-address to make sure that dnsmasq works. More information about siaddr and option 150,66 can be found in informational RFC https://tools.ietf.org/html/rfc5859 Change-Id: I55487d867979bf6bb4cf228fcf6408beae955d2b --- ironic/common/dhcp_factory.py | 8 ++-- ironic/common/neutron.py | 4 +- ironic/common/pxe_utils.py | 40 ++++++++++++---- ironic/dhcp/base.py | 16 +++---- ironic/dhcp/neutron.py | 16 +++---- ironic/drivers/modules/network/common.py | 6 ++- ironic/tests/unit/common/test_neutron.py | 2 +- ironic/tests/unit/common/test_pxe_utils.py | 47 +++++++++++-------- .../drivers/modules/network/test_common.py | 2 +- .../drivers/modules/network/test_neutron.py | 4 +- ...-dhcp-option-numbers-8b0b0efae912ff5f.yaml | 7 +++ 11 files changed, 93 insertions(+), 59 deletions(-) create mode 100644 releasenotes/notes/use-dhcp-option-numbers-8b0b0efae912ff5f.yaml diff --git a/ironic/common/dhcp_factory.py b/ironic/common/dhcp_factory.py index aaff635df3..fa294cb463 100644 --- a/ironic/common/dhcp_factory.py +++ b/ironic/common/dhcp_factory.py @@ -72,12 +72,10 @@ class DHCPFactory(object): :: - [{'opt_name': 'bootfile-name', + [{'opt_name': '67', 'opt_value': 'pxelinux.0'}, - {'opt_name': 'server-ip-address', - 'opt_value': '123.123.123.456'}, - {'opt_name': 'tftp-server', - 'opt_value': '123.123.123.123'}] + {'opt_name': '66', + 'opt_value': '123.123.123.456'}] :param ports: A dict with keys 'ports' and 'portgroups' and dicts as values. Each dict has key/value pairs of the form diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py index 1cb0b0d3d2..4bba8d124f 100644 --- a/ironic/common/neutron.py +++ b/ironic/common/neutron.py @@ -18,6 +18,7 @@ from oslo_utils import uuidutils from ironic.common import exception from ironic.common.i18n import _ from ironic.common import keystone +from ironic.common.pxe_utils import DHCP_CLIENT_ID from ironic.conf import CONF LOG = log.getLogger(__name__) @@ -237,7 +238,8 @@ def add_ports_to_network(task, network_uuid, security_groups=None): body['port']['binding:profile'] = binding_profile client_id = ironic_port.extra.get('client-id') if client_id: - client_id_opt = {'opt_name': 'client-id', 'opt_value': client_id} + client_id_opt = {'opt_name': DHCP_CLIENT_ID, + 'opt_value': client_id} extra_dhcp_opts = body['port'].get('extra_dhcp_opts', []) extra_dhcp_opts.append(client_id_opt) body['port']['extra_dhcp_opts'] = extra_dhcp_opts diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py index 0ed9a21ff7..806a00cf49 100644 --- a/ironic/common/pxe_utils.py +++ b/ironic/common/pxe_utils.py @@ -33,6 +33,13 @@ LOG = logging.getLogger(__name__) PXE_CFG_DIR_NAME = 'pxelinux.cfg' +DHCP_CLIENT_ID = '61' # rfc2132 +DHCP_TFTP_SERVER_NAME = '66' # rfc2132 +DHCP_BOOTFILE_NAME = '67' # rfc2132 +DHCP_TFTP_SERVER_ADDRESS = '150' # rfc5859 +DHCP_IPXE_ENCAP_OPTS = '175' # Tentatively Assigned +DHCP_TFTP_PATH_PREFIX = '210' # rfc5071 + def get_root_dir(): """Returns the directory where the config files and images will live.""" @@ -317,30 +324,47 @@ def dhcp_options_for_instance(task): if dhcp_provider_name == 'neutron': # Neutron use dnsmasq as default DHCP agent, add extra config # to neutron "dhcp-match=set:ipxe,175" and use below option - dhcp_opts.append({'opt_name': 'tag:!ipxe,bootfile-name', + dhcp_opts.append({'opt_name': "tag:!ipxe,%s" % DHCP_BOOTFILE_NAME, 'opt_value': boot_file}) - dhcp_opts.append({'opt_name': 'tag:ipxe,bootfile-name', + dhcp_opts.append({'opt_name': "tag:ipxe,%s" % DHCP_BOOTFILE_NAME, 'opt_value': ipxe_script_url}) else: # !175 == non-iPXE. # http://ipxe.org/howto/dhcpd#ipxe-specific_options - dhcp_opts.append({'opt_name': '!175,bootfile-name', + dhcp_opts.append({'opt_name': "!%s,%s" % (DHCP_IPXE_ENCAP_OPTS, + DHCP_BOOTFILE_NAME), 'opt_value': boot_file}) - dhcp_opts.append({'opt_name': 'bootfile-name', + dhcp_opts.append({'opt_name': DHCP_BOOTFILE_NAME, 'opt_value': ipxe_script_url}) else: - dhcp_opts.append({'opt_name': 'bootfile-name', + dhcp_opts.append({'opt_name': DHCP_BOOTFILE_NAME, 'opt_value': boot_file}) # 210 == tftp server path-prefix or tftp root, will be used to find # pxelinux.cfg directory. The pxelinux.0 loader infers this information # from it's own path, but Petitboot needs it to be specified by this # option since it doesn't use pxelinux.0 loader. - dhcp_opts.append({'opt_name': '210', + dhcp_opts.append({'opt_name': DHCP_TFTP_PATH_PREFIX, 'opt_value': get_tftp_path_prefix()}) - dhcp_opts.append({'opt_name': 'server-ip-address', + dhcp_opts.append({'opt_name': DHCP_TFTP_SERVER_NAME, 'opt_value': CONF.pxe.tftp_server}) - dhcp_opts.append({'opt_name': 'tftp-server', + dhcp_opts.append({'opt_name': DHCP_TFTP_SERVER_ADDRESS, + 'opt_value': CONF.pxe.tftp_server}) + + # NOTE(vsaienko) set this option specially for dnsmasq case as it always + # sets `siaddr` field which is treated by pxe clients as TFTP server + # see page 9 https://tools.ietf.org/html/rfc2131. + # If `server-ip-address` is not provided dnsmasq sets `siaddr` to dnsmasq's + # IP which breaks PXE booting as TFTP server is configured on ironic + # conductor host. + # http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=blob;f=src/dhcp-common.c;h=eae9ae3567fe16eb979a484976c270396322efea;hb=a3303e196e5d304ec955c4d63afb923ade66c6e8#l572 # noqa + # There is an informational RFC which describes how options related to + # tftp 150,66 and siaddr should be used https://tools.ietf.org/html/rfc5859 + # All dhcp servers we've tried: contrail/dnsmasq/isc just silently ignore + # unknown options but potentially it may blow up with others. + # Related bug was opened on Neutron side: + # https://bugs.launchpad.net/neutron/+bug/1723354 + dhcp_opts.append({'opt_name': 'server-ip-address', 'opt_value': CONF.pxe.tftp_server}) # Append the IP version for all the configuration options diff --git a/ironic/dhcp/base.py b/ironic/dhcp/base.py index 94f61fd091..6e694be01e 100644 --- a/ironic/dhcp/base.py +++ b/ironic/dhcp/base.py @@ -36,12 +36,10 @@ class BaseDHCP(object): :: - [{'opt_name': 'bootfile-name', + [{'opt_name': '67', 'opt_value': 'pxelinux.0'}, - {'opt_name': 'server-ip-address', - 'opt_value': '123.123.123.456'}, - {'opt_name': 'tftp-server', - 'opt_value': '123.123.123.123'}] + {'opt_name': '66', + 'opt_value': '123.123.123.456'}] :param token: An optional authentication token. :raises: FailedToUpdateDHCPOptOnPort @@ -56,12 +54,10 @@ class BaseDHCP(object): :: - [{'opt_name': 'bootfile-name', + [{'opt_name': '67', 'opt_value': 'pxelinux.0'}, - {'opt_name': 'server-ip-address', - 'opt_value': '123.123.123.456'}, - {'opt_name': 'tftp-server', - 'opt_value': '123.123.123.123'}] + {'opt_name': '66', + 'opt_value': '123.123.123.456'}] :param vifs: A dict with keys 'ports' and 'portgroups' and dicts as values. Each dict has key/value pairs of the form diff --git a/ironic/dhcp/neutron.py b/ironic/dhcp/neutron.py index a474c501d3..b885c18512 100644 --- a/ironic/dhcp/neutron.py +++ b/ironic/dhcp/neutron.py @@ -47,12 +47,10 @@ class NeutronDHCPApi(base.BaseDHCP): :: - [{'opt_name': 'bootfile-name', + [{'opt_name': '67', 'opt_value': 'pxelinux.0'}, - {'opt_name': 'server-ip-address', - 'opt_value': '123.123.123.456'}, - {'opt_name': 'tftp-server', - 'opt_value': '123.123.123.123'}] + {'opt_name': '66', + 'opt_value': '123.123.123.456'}] :param token: optional auth token. :raises: FailedToUpdateDHCPOptOnPort @@ -72,12 +70,10 @@ class NeutronDHCPApi(base.BaseDHCP): :: - [{'opt_name': 'bootfile-name', + [{'opt_name': '67', 'opt_value': 'pxelinux.0'}, - {'opt_name': 'server-ip-address', - 'opt_value': '123.123.123.456'}, - {'opt_name': 'tftp-server', - 'opt_value': '123.123.123.123'}] + {'opt_name': '66', + 'opt_value': '123.123.123.456'}] :param vifs: a dict of Neutron port/portgroup dicts to update DHCP options on. The port/portgroup dict key should be Ironic port UUIDs, and the values diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index 39d9453d66..d01f9cfb3a 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -24,6 +24,7 @@ from ironic.common import exception from ironic.common.i18n import _ from ironic.common import network from ironic.common import neutron +from ironic.common.pxe_utils import DHCP_CLIENT_ID from ironic.common import states from ironic.common import utils from ironic import objects @@ -245,7 +246,8 @@ def plug_port_to_tenant_network(task, port_like_obj, client=None): local_link_info.append(port_like_obj.local_link_connection) client_id = port_like_obj.extra.get('client-id') if client_id: - client_id_opt = ({'opt_name': 'client-id', 'opt_value': client_id}) + client_id_opt = ({'opt_name': DHCP_CLIENT_ID, + 'opt_value': client_id}) # NOTE(sambetts) Only update required binding: attributes, # because other port attributes may have been set by the user or @@ -415,7 +417,7 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin): # from the neutron port if vif: api = dhcp_factory.DHCPFactory() - client_id_opt = {'opt_name': 'client-id', + client_id_opt = {'opt_name': DHCP_CLIENT_ID, 'opt_value': updated_client_id} api.provider.update_port_dhcp_opts( diff --git a/ironic/tests/unit/common/test_neutron.py b/ironic/tests/unit/common/test_neutron.py index 6fa0c14e43..a5b9c8d560 100644 --- a/ironic/tests/unit/common/test_neutron.py +++ b/ironic/tests/unit/common/test_neutron.py @@ -170,7 +170,7 @@ class TestNeutronNetworkActions(db_base.DbTestCase): if is_client_id: expected_body['port']['extra_dhcp_opts'] = ( - [{'opt_name': 'client-id', 'opt_value': self._CLIENT_ID}]) + [{'opt_name': '61', 'opt_value': self._CLIENT_ID}]) # Ensure we can create ports self.client_mock.create_port.return_value = { 'port': self.neutron_port} diff --git a/ironic/tests/unit/common/test_pxe_utils.py b/ironic/tests/unit/common/test_pxe_utils.py index 80c9fd4ef8..b3423e17cf 100644 --- a/ironic/tests/unit/common/test_pxe_utils.py +++ b/ironic/tests/unit/common/test_pxe_utils.py @@ -621,18 +621,21 @@ class TestPXEUtils(db_base.DbTestCase): self.config(tftp_server='192.0.2.1', group='pxe') self.config(pxe_bootfile_name='fake-bootfile', group='pxe') self.config(tftp_root='/tftp-path/', group='pxe') - expected_info = [{'opt_name': 'bootfile-name', + expected_info = [{'opt_name': '67', 'opt_value': 'fake-bootfile', 'ip_version': ip_version}, {'opt_name': '210', 'opt_value': '/tftp-path/', 'ip_version': ip_version}, + {'opt_name': '66', + 'opt_value': '192.0.2.1', + 'ip_version': ip_version}, + {'opt_name': '150', + 'opt_value': '192.0.2.1', + 'ip_version': ip_version}, {'opt_name': 'server-ip-address', 'opt_value': '192.0.2.1', - 'ip_version': ip_version}, - {'opt_name': 'tftp-server', - 'opt_value': '192.0.2.1', - 'ip_version': ip_version}, + 'ip_version': ip_version} ] with task_manager.acquire(self.context, self.node.uuid) as task: self.assertEqual(expected_info, @@ -689,17 +692,20 @@ class TestPXEUtils(db_base.DbTestCase): self.config(dhcp_provider='isc', group='dhcp') expected_boot_script_url = 'http://192.0.3.2:1234/boot.ipxe' - expected_info = [{'opt_name': '!175,bootfile-name', + expected_info = [{'opt_name': '!175,67', 'opt_value': boot_file, 'ip_version': 4}, + {'opt_name': '66', + 'opt_value': '192.0.2.1', + 'ip_version': 4}, + {'opt_name': '150', + 'opt_value': '192.0.2.1', + 'ip_version': 4}, + {'opt_name': '67', + 'opt_value': expected_boot_script_url, + 'ip_version': 4}, {'opt_name': 'server-ip-address', 'opt_value': '192.0.2.1', - 'ip_version': 4}, - {'opt_name': 'tftp-server', - 'opt_value': '192.0.2.1', - 'ip_version': 4}, - {'opt_name': 'bootfile-name', - 'opt_value': expected_boot_script_url, 'ip_version': 4}] self.assertItemsEqual(expected_info, @@ -707,17 +713,20 @@ class TestPXEUtils(db_base.DbTestCase): self.config(dhcp_provider='neutron', group='dhcp') expected_boot_script_url = 'http://192.0.3.2:1234/boot.ipxe' - expected_info = [{'opt_name': 'tag:!ipxe,bootfile-name', + expected_info = [{'opt_name': 'tag:!ipxe,67', 'opt_value': boot_file, 'ip_version': 4}, + {'opt_name': '66', + 'opt_value': '192.0.2.1', + 'ip_version': 4}, + {'opt_name': '150', + 'opt_value': '192.0.2.1', + 'ip_version': 4}, + {'opt_name': 'tag:ipxe,67', + 'opt_value': expected_boot_script_url, + 'ip_version': 4}, {'opt_name': 'server-ip-address', 'opt_value': '192.0.2.1', - 'ip_version': 4}, - {'opt_name': 'tftp-server', - 'opt_value': '192.0.2.1', - 'ip_version': 4}, - {'opt_name': 'tag:ipxe,bootfile-name', - 'opt_value': expected_boot_script_url, 'ip_version': 4}] self.assertItemsEqual(expected_info, diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index 35b7a8a94b..6c76540b2d 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -970,7 +970,7 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts') def test_port_changed_client_id(self, dhcp_update_mock): expected_extra = {'vif_port_id': 'fake-id', 'client-id': 'fake2'} - expected_dhcp_opts = [{'opt_name': 'client-id', 'opt_value': 'fake2'}] + expected_dhcp_opts = [{'opt_name': '61', 'opt_value': 'fake2'}] self.port.extra = expected_extra with task_manager.acquire(self.context, self.node.id) as task: self.interface.port_changed(task, self.port) diff --git a/ironic/tests/unit/drivers/modules/network/test_neutron.py b/ironic/tests/unit/drivers/modules/network/test_neutron.py index 3708125cc5..65c413b0e8 100644 --- a/ironic/tests/unit/drivers/modules/network/test_neutron.py +++ b/ironic/tests/unit/drivers/modules/network/test_neutron.py @@ -328,9 +328,9 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): } if is_client_id: port1_body['port']['extra_dhcp_opts'] = ( - [{'opt_name': 'client-id', 'opt_value': client_ids[0]}]) + [{'opt_name': '61', 'opt_value': client_ids[0]}]) port2_body['port']['extra_dhcp_opts'] = ( - [{'opt_name': 'client-id', 'opt_value': client_ids[1]}]) + [{'opt_name': '61', 'opt_value': client_ids[1]}]) with task_manager.acquire(self.context, self.node.id) as task: self.interface.configure_tenant_networks(task) client_mock.assert_called_once_with() diff --git a/releasenotes/notes/use-dhcp-option-numbers-8b0b0efae912ff5f.yaml b/releasenotes/notes/use-dhcp-option-numbers-8b0b0efae912ff5f.yaml new file mode 100644 index 0000000000..167d834632 --- /dev/null +++ b/releasenotes/notes/use-dhcp-option-numbers-8b0b0efae912ff5f.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Uses standard DHCP option codes instead of dnsmasq-specific option names, + because different backends use different option names. This fixes the + `compatibility issues with neutron's DHCP backends + `.