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
(cherry picked from commit 228a2a7885e1b04d4180fe8daa2992884decaf6d)

11 files changed, 85 insertions, 51 deletions

diff --git a/ironic/common/dhcp_factory.py b/ironic/common/dhcp_factory.py
index aaff635df..fa294cb46 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 d224a576d..d82018c29 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__)
@@ -228,7 +229,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 0ed9a21ff..806a00cf4 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': 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
diff --git a/ironic/dhcp/base.py b/ironic/dhcp/base.py
index 94f61fd09..6e694be01 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 a474c501d..b885c1851 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 db4df5838..2ce888e28 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 c7b9e201d..181c9299e 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 80c9fd4ef..b3423e17c 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': '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,
@@ -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': '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,
@@ -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': '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,
diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py
index af6d2451c..ab3ca59f2 100644
--- a/ironic/tests/unit/drivers/modules/network/test_common.py
+++ b/ironic/tests/unit/drivers/modules/network/test_common.py
@@ -1008,7 +1008,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 3708125cc..65c413b0e 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 000000000..167d83463
--- /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
+    <https://bugs.launchpad.net/ironic/+bug/1717236>`.