From: David Edery Date: Mon, 12 Oct 2015 08:11:50 +0000 (+0300) Subject: Enable specific extra_dhcp_opt to be left blank X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=3837427189a54e6c052d773d629bac53d95702dc;p=openstack-build%2Fneutron-build.git Enable specific extra_dhcp_opt to be left blank In some cases there is a need for a blank value of the "router" extra dhcp option. This fix addresses this issue by introducing a simple mechanism in the code that allows specific extra dhcp options to be left blank by applying a different opt_value validation of the data received from the REST call and different validation on the value that will be written to the DB. This fix also takes into consideration bug #1257467 which claims that in case of a blank "server-ip-address" option a segmentation fault occurs in dnsmasq. I did not check this claim with a newer dnsmasq version since it seemed logical that the list of potentially blank options should be limited to options that are known to work well when blank and have functional justification for being blank (e.g. "router") APIImpact "router" and "classless-static-route" extra dhcp options can be blank (e.g. opt-name="router", opt-value="") DocImpact During port creation/update, Specific extra-dhcp-options can be left blank ("router" and/or "classless-static-route"). This causes dnsmasq to have an empty option in the "opts" file related to the network to which the port is related. For example: tag:tag0,option:classless-static-route, tag:tag0,option:router, Closes-Bug: #1437695 Related-Bug: #1257467 Change-Id: I0ac76e132c4bd86da39863674d4cf8a22dad7034 --- diff --git a/neutron/db/extradhcpopt_db.py b/neutron/db/extradhcpopt_db.py index 09d64c70d..c6d1c6e48 100644 --- a/neutron/db/extradhcpopt_db.py +++ b/neutron/db/extradhcpopt_db.py @@ -59,13 +59,26 @@ class ExtraDhcpOptMixin(object): """Mixin class to add extra options to the DHCP opts file and associate them to a port. """ + + def _is_valid_opt_value(self, opt_name, opt_value): + + # If the dhcp opt is blank-able, it shouldn't be saved to the DB in + # case that the value is None + if opt_name in edo_ext.VALID_BLANK_EXTRA_DHCP_OPTS: + return opt_value is not None + + # Otherwise, it shouldn't be saved to the DB in case that the value + # is None or empty + return bool(opt_value) + def _process_port_create_extra_dhcp_opts(self, context, port, extra_dhcp_opts): if not extra_dhcp_opts: return port with context.session.begin(subtransactions=True): for dopt in extra_dhcp_opts: - if dopt['opt_value']: + if self._is_valid_opt_value(dopt['opt_name'], + dopt['opt_value']): ip_version = dopt.get('ip_version', 4) db = ExtraDhcpOpt( port_id=port['id'], @@ -107,12 +120,18 @@ class ExtraDhcpOptMixin(object): if upd_rec['opt_value'] is None: context.session.delete(opt) else: - if opt['opt_value'] != upd_rec['opt_value']: + if (self._is_valid_opt_value( + opt['opt_name'], + upd_rec['opt_value']) and + opt['opt_value'] != + upd_rec['opt_value']): opt.update( {'opt_value': upd_rec['opt_value']}) break else: - if upd_rec['opt_value'] is not None: + if self._is_valid_opt_value( + upd_rec['opt_name'], + upd_rec['opt_value']): ip_version = upd_rec.get('ip_version', 4) db = ExtraDhcpOpt( port_id=id, diff --git a/neutron/extensions/extra_dhcp_opt.py b/neutron/extensions/extra_dhcp_opt.py index 8d3063b22..8bc4bf699 100644 --- a/neutron/extensions/extra_dhcp_opt.py +++ b/neutron/extensions/extra_dhcp_opt.py @@ -4,7 +4,7 @@ # you may not use this file except in compliance with the License. # You may obtain a copy of the License at # -# http://www.apache.org/licenses/LICENSE-2.0 +# http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, @@ -27,45 +27,59 @@ class ExtraDhcpOptBadData(exceptions.InvalidInput): message = _("Invalid data format for extra-dhcp-opt: %(data)s") -def _validate_list_of_dict_or_none(data, key_specs=None): +# Valid blank extra dhcp opts +VALID_BLANK_EXTRA_DHCP_OPTS = ('router', 'classless-static-route') + +# Common definitions for maximum string field length +DHCP_OPT_NAME_MAX_LEN = 64 +DHCP_OPT_VALUE_MAX_LEN = 255 + +EXTRA_DHCP_OPT_KEY_SPECS = { + 'id': {'type:uuid': None, 'required': False}, + 'opt_name': {'type:not_empty_string': DHCP_OPT_NAME_MAX_LEN, + 'required': True}, + 'opt_value': {'type:not_empty_string_or_none': DHCP_OPT_VALUE_MAX_LEN, + 'required': True}, + 'ip_version': {'convert_to': attr.convert_to_int, + 'type:values': [4, 6], + 'required': False} +} + + +def _validate_extra_dhcp_opt(data, key_specs=None): if data is not None: if not isinstance(data, list): raise ExtraDhcpOptBadData(data=data) for d in data: - msg = attr._validate_dict(d, key_specs) + if d['opt_name'] in VALID_BLANK_EXTRA_DHCP_OPTS: + msg = attr._validate_string_or_none(d['opt_value'], + DHCP_OPT_VALUE_MAX_LEN) + else: + msg = attr._validate_dict(d, key_specs) if msg: raise ExtraDhcpOptBadData(data=msg) -attr.validators['type:list_of_dict_or_none'] = _validate_list_of_dict_or_none +attr.validators['type:list_of_extra_dhcp_opts'] = _validate_extra_dhcp_opt # Attribute Map EXTRADHCPOPTS = 'extra_dhcp_opts' -# Common definitions for maximum string field length -DHCP_OPT_NAME_MAX_LEN = 64 -DHCP_OPT_VALUE_MAX_LEN = 255 - CLIENT_ID = "client-id" EXTENDED_ATTRIBUTES_2_0 = { 'ports': { - EXTRADHCPOPTS: - {'allow_post': True, - 'allow_put': True, - 'is_visible': True, - 'default': None, - 'validate': { - 'type:list_of_dict_or_none': { - 'id': {'type:uuid': None, 'required': False}, - 'opt_name': {'type:not_empty_string': DHCP_OPT_NAME_MAX_LEN, - 'required': True}, - 'opt_value': {'type:not_empty_string_or_none': - DHCP_OPT_VALUE_MAX_LEN, - 'required': True}, - 'ip_version': {'convert_to': attr.convert_to_int, - 'type:values': [4, 6], - 'required': False}}}}}} + EXTRADHCPOPTS: { + 'allow_post': True, + 'allow_put': True, + 'is_visible': True, + 'default': None, + 'validate': { + 'type:list_of_extra_dhcp_opts': EXTRA_DHCP_OPT_KEY_SPECS + } + } + } +} class Extra_dhcp_opt(extensions.ExtensionDescriptor): diff --git a/neutron/tests/unit/extensions/test_extra_dhcp_opt.py b/neutron/tests/unit/extensions/test_extra_dhcp_opt.py index 68ecd748d..41ad18e85 100644 --- a/neutron/tests/unit/extensions/test_extra_dhcp_opt.py +++ b/neutron/tests/unit/extensions/test_extra_dhcp_opt.py @@ -106,6 +106,21 @@ class TestExtraDhcpOpt(ExtraDhcpOptDBTestCase): self._check_opts(expected, port['port'][edo_ext.EXTRADHCPOPTS]) + def test_create_port_with_empty_router_extradhcpopts(self): + opt_list = [{'opt_name': 'router', + 'opt_value': ''}, + {'opt_name': 'server-ip-address', + 'opt_value': '123.123.123.456'}, + {'opt_name': 'tftp-server', + 'opt_value': '123.123.123.123'}] + + params = {edo_ext.EXTRADHCPOPTS: opt_list, + 'arg_list': (edo_ext.EXTRADHCPOPTS,)} + + with self.port(**params) as port: + self._check_opts(opt_list, + port['port'][edo_ext.EXTRADHCPOPTS]) + def test_create_port_with_extradhcpopts_ipv4_opt_version(self): opt_list = [{'opt_name': 'bootfile-name', 'opt_value': 'pxelinux.0', @@ -266,6 +281,28 @@ class TestExtraDhcpOpt(ExtraDhcpOptDBTestCase): res = req.get_response(self.api) self.assertEqual(res.status_int, webob.exc.HTTPBadRequest.code) + def test_update_port_with_blank_router_extradhcpopt(self): + opt_list = [{'opt_name': 'bootfile-name', + 'opt_value': 'pxelinux.0', + 'ip_version': 4}, + {'opt_name': 'tftp-server', + 'opt_value': '123.123.123.123', + 'ip_version': 4}, + {'opt_name': 'router', + 'opt_value': '123.123.123.1', + 'ip_version': 4}] + upd_opts = [{'opt_name': 'router', + 'opt_value': '', + 'ip_version': 4}] + expected_opts = copy.deepcopy(opt_list) + for i in expected_opts: + if i['opt_name'] == upd_opts[0]['opt_name']: + i['opt_value'] = upd_opts[0]['opt_value'] + break + + self._test_update_port_with_extradhcpopts(opt_list, upd_opts, + expected_opts) + def test_update_port_with_extradhcpopts_ipv6_change_value(self): opt_list = [{'opt_name': 'bootfile-name', 'opt_value': 'pxelinux.0',