Skip to content

Commit

Permalink
Change pxe dhcp options name to codes.
Browse files Browse the repository at this point in the history 
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 c377f5c
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
  • Loading branch information
@jumpojoy
jumpojoy committed Oct 13, 2017
1 parent 2d486ca commit 228a2a7
Show file tree
Hide file tree
Showing 11 changed files with 85 additions and 51 deletions.
8 changes: 3 additions & 5 deletions ironic/common/dhcp_factory.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,10 @@ def update_dhcp(self, task, dhcp_opts, ports=None):
::
[{'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
Expand Down
4 changes: 3 additions & 1 deletion ironic/common/neutron.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
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__)
Expand Down Expand Up @@ -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
Expand Down
40 changes: 32 additions & 8 deletions ironic/common/pxe_utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@

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."""
Expand Down Expand Up @@ -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': DHCP_TFTP_SERVER_ADDRESS,
'opt_value': CONF.pxe.tftp_server})
dhcp_opts.append({'opt_name': '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
Expand Down
16 changes: 6 additions & 10 deletions ironic/dhcp/base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,10 @@ def update_port_dhcp_opts(self, port_id, dhcp_options, token=None):
::
[{'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
Expand All @@ -56,12 +54,10 @@ def update_dhcp_opts(self, task, options, vifs=None):
::
[{'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
Expand Down
16 changes: 6 additions & 10 deletions ironic/dhcp/neutron.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,10 @@ def update_port_dhcp_opts(self, port_id, dhcp_options, token=None):
::
[{'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
Expand All @@ -72,12 +70,10 @@ def update_dhcp_opts(self, task, options, vifs=None):
::
[{'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
Expand Down
6 changes: 4 additions & 2 deletions ironic/drivers/modules/network/common.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -415,7 +417,7 @@ def port_changed(self, task, port_obj):
# 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(
Expand Down
2 changes: 1 addition & 1 deletion ironic/tests/unit/common/test_neutron.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def _test_add_ports_to_network(self, is_client_id,

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}
Expand Down
31 changes: 20 additions & 11 deletions ironic/tests/unit/common/test_pxe_utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -621,18 +621,21 @@ def _dhcp_options_for_instance(self, ip_version=4):
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': 'server-ip-address',
{'opt_name': '66',
'opt_value': '192.0.2.1',
'ip_version': ip_version},
{'opt_name': 'tftp-server',
{'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}
]
with task_manager.acquire(self.context, self.node.uuid) as task:
self.assertEqual(expected_info,
Expand Down Expand Up @@ -689,35 +692,41 @@ def _dhcp_options_for_instance_ipxe(self, task, boot_file):

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': 'server-ip-address',
{'opt_name': '66',
'opt_value': '192.0.2.1',
'ip_version': 4},
{'opt_name': 'tftp-server',
{'opt_name': '150',
'opt_value': '192.0.2.1',
'ip_version': 4},
{'opt_name': 'bootfile-name',
{'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}]

self.assertItemsEqual(expected_info,
pxe_utils.dhcp_options_for_instance(task))

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': 'server-ip-address',
{'opt_name': '66',
'opt_value': '192.0.2.1',
'ip_version': 4},
{'opt_name': 'tftp-server',
{'opt_name': '150',
'opt_value': '192.0.2.1',
'ip_version': 4},
{'opt_name': 'tag:ipxe,bootfile-name',
{'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}]

self.assertItemsEqual(expected_info,
Expand Down
2 changes: 1 addition & 1 deletion ironic/tests/unit/drivers/modules/network/test_common.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,7 @@ def test_port_changed_address_no_vif_id(self, mac_update_mock):
@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)
Expand Down
4 changes: 2 additions & 2 deletions ironic/tests/unit/drivers/modules/network/test_neutron.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,9 @@ def _test_configure_tenant_networks(self, client_mock, is_client_id=False,
}
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()
Expand Down
7 changes: 7 additions & 0 deletions releasenotes/notes/use-dhcp-option-numbers-8b0b0efae912ff5f.yaml
View file
Original file line number Diff line number Diff line change
@@ -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
<https://bugs.launchpad.net/ironic/+bug/1717236>`.

0 comments on commit 228a2a7

Please sign in to comment.