From f7085c188320b55534f1a315d823c002ad75b653 Mon Sep 17 00:00:00 2001 From: Sanket Date: Mon, 7 Aug 2017 11:44:54 +0530 Subject: [PATCH] [GCE] Validate security group at the time of creation Openstack Security Group should be validated if they are compatible with GCE firewall rules. If not we should raise approriate error. This fix processes security group info in BEFORE_RESPONSE event of security group and rollbacks earlier created security group if not compatible. We can not use BEFORE_CREATE/PRECOMMIT_CREATE as they do not contain required security group rules info. Change-Id: I5f1fc67208085ef399f3dcfe5fdec63d4f2ffc51 Closes-bug: #1709002 --- .../plugins/ml2/drivers/gce/mech_gce.py | 61 +++++++++++++++---- .../tests/plugins/ml2/drivers/gce/test_gce.py | 34 ++++++++++- 2 files changed, 79 insertions(+), 16 deletions(-) diff --git a/neutron/neutron/plugins/ml2/drivers/gce/mech_gce.py b/neutron/neutron/plugins/ml2/drivers/gce/mech_gce.py index 2193570..e6e262a 100644 --- a/neutron/neutron/plugins/ml2/drivers/gce/mech_gce.py +++ b/neutron/neutron/plugins/ml2/drivers/gce/mech_gce.py @@ -11,8 +11,7 @@ License for the specific language governing permissions and limitations under the License. """ -import random - +from googleapiclient import errors as gce_errors from neutron._i18n import _ from neutron.callbacks import events from neutron.callbacks import registry @@ -26,6 +25,8 @@ from neutron_lib import exceptions as e from oslo_log import log import ipaddr +import random + try: from neutron_lib.plugins import directory except ImportError: @@ -66,11 +67,15 @@ class GceMechanismDriver(api.MechanismDriver): events.BEFORE_DELETE) registry.subscribe(self.secgroup_callback, resources.SECURITY_GROUP, events.BEFORE_UPDATE) + registry.subscribe(self.secgroup_callback, resources.SECURITY_GROUP, + events.BEFORE_RESPONSE) registry.subscribe(self.secgroup_callback, resources.SECURITY_GROUP_RULE, events.BEFORE_DELETE) registry.subscribe(self.secgroup_callback, resources.SECURITY_GROUP_RULE, events.BEFORE_UPDATE) + registry.subscribe(self.secgroup_callback, + resources.SECURITY_GROUP_RULE, events.BEFORE_CREATE) def _gce_network_name(self, context): return 'net-' + context.current[api.ID] @@ -152,7 +157,7 @@ class GceMechanismDriver(api.MechanismDriver): def _gce_secgrp_id(self, openstack_id): return "secgrp-" + openstack_id - def _convert_secgrp_rule_to_gce(self, rule, network_link): + def _convert_secgrp_rule_to_gce(self, rule, network_link, validate=False): if rule['ethertype'] != 'IPv4': raise sg.SecurityGroupRuleInvalidEtherType( ethertype=rule['ethertype'], values=('IPv4', )) @@ -164,8 +169,9 @@ class GceMechanismDriver(api.MechanismDriver): 'allowed': [{}], 'destinationRanges': [], } - gce_rule['name'] = self._gce_secgrp_id(rule['id']) - gce_rule['network'] = network_link + if not validate: + gce_rule['name'] = self._gce_secgrp_id(rule['id']) + gce_rule['network'] = network_link directions = { 'ingress': 'INGRESS', @@ -217,13 +223,22 @@ class GceMechanismDriver(api.MechanismDriver): operation = gceutils.create_firewall_rule(compute, project, gce_rule) gceutils.wait_for_operation(compute, project, operation) + def _validate_secgrp_rule(self, rule): + try: + self._convert_secgrp_rule_to_gce( + rule, network_link=None, validate=True) + except Exception as e: + LOG.exception("An error occurred while creating security " + "group: %s" % e) + raise e + def _update_secgrp_rule(self, context, rule_id): compute, project = self.gce_svc, self.gce_project name = self._gce_secgrp_id(rule_id) try: gce_firewall_info = gceutils.get_firewall_rule( compute, project, name) - except gceutils.HttpError: + except gce_errors.HttpError: return try: @@ -254,7 +269,7 @@ class GceMechanismDriver(api.MechanismDriver): "as firewall rule update not GCE compatible." % name) operation = gceutils.delete_firewall_rule(compute, project, name) gceutils.wait_for_operation(compute, project, operation) - except gceutils.HttpError: + except gce_errors.HttpError: pass def _create_secgrp_rules_if_needed(self, context, secgrp_ids): @@ -276,10 +291,24 @@ class GceMechanismDriver(api.MechanismDriver): try: gce_rule_name = self._gce_secgrp_id(secgrp_rule['id']) gceutils.get_firewall_rule(compute, project, gce_rule_name) - except gceutils.HttpError: + except gce_errors.HttpError: self._create_secgrp_rule(context, secgrp_rule, network_link) + def _validate_secgrp(self, context, secgrp): + secgrp_rules = secgrp['security_group_rules'] + try: + for secgrp_rule in secgrp_rules: + self._validate_secgrp_rule(secgrp_rule) + except Exception as e: + try: + core_plugin = NeutronManager.get_plugin() + except AttributeError: + core_plugin = directory.get_plugin() + LOG.info('Rollback create security group: %s' % secgrp['id']) + core_plugin.delete_security_group(context, secgrp['id']) + raise e + def _update_secgrp(self, context, secgrp_id): try: core_plugin = NeutronManager.get_plugin() @@ -315,6 +344,7 @@ class GceMechanismDriver(api.MechanismDriver): return True def secgroup_callback(self, resource, event, trigger, **kwargs): + LOG.debug("Secgrp_callback: %s %s %s" % (resource, event, kwargs)) if resource == resources.SECURITY_GROUP_RULE: context = kwargs['context'] if event == events.BEFORE_DELETE: @@ -323,11 +353,16 @@ class GceMechanismDriver(api.MechanismDriver): elif event == events.BEFORE_UPDATE: rule_id = kwargs['security_group_rule_id'] self._update_secgrp_rule(context, rule_id) + elif event == events.BEFORE_CREATE: + rule = kwargs['security_group_rule'] + self._validate_secgrp_rule(rule) elif resource == resources.SECURITY_GROUP: if event == events.BEFORE_DELETE: context = kwargs['context'] - security_group_id = kwargs.get('security_group_id') - if security_group_id: - self._delete_secgrp(context, security_group_id) - else: - LOG.warn("Security group ID not found in delete request") + security_group_id = kwargs['security_group_id'] + self._delete_secgrp(context, security_group_id) + elif event == events.BEFORE_RESPONSE: + if kwargs['method_name'] == 'security_group.create.end': + context = kwargs['context'] + secgrp = kwargs['data']['security_group'] + self._validate_secgrp(context, secgrp) diff --git a/neutron/tests/plugins/ml2/drivers/gce/test_gce.py b/neutron/tests/plugins/ml2/drivers/gce/test_gce.py index 2fe39d4..17b394b 100644 --- a/neutron/tests/plugins/ml2/drivers/gce/test_gce.py +++ b/neutron/tests/plugins/ml2/drivers/gce/test_gce.py @@ -10,14 +10,16 @@ WARRANTIES OR CONDITIONS OF ANY KIND, either expressed or implied. See the License for the specific language governing permissions and limitations under the License. """ - +import httplib2 import mock import os +from googleapiclient import errors as gce_errors from neutron.extensions import securitygroup as sg from neutron.manager import NeutronManager from neutron.plugins.ml2.drivers.gce.mech_gce import GceMechanismDriver -from neutron.plugins.ml2.drivers.gce.mech_gce import SecurityGroupInvalidDirection # noqa +from neutron.plugins.ml2.drivers.gce.mech_gce import \ + SecurityGroupInvalidDirection # noqa from neutron.tests import base from neutron.tests.common.gce import gce_mock from neutron.tests.common.gce.gce_mock import FakeNeutronManager @@ -138,7 +140,7 @@ class GCENeutronTestCase(test_sg.SecurityGroupsTestCase, base.BaseTestCase): sg_rule = self.get_fake_sg_rule() gce_rule = self._driver._convert_secgrp_rule_to_gce( sg_rule, NETWORK_LINK) - self.assertTrue(isinstance(gce_rule, dict)) + self.assertIsInstance(gce_rule, dict) @mock.patch('neutron.common.gceutils.wait_for_operation') @mock.patch('neutron.common.gceutils.create_firewall_rule') @@ -153,6 +155,13 @@ class GCENeutronTestCase(test_sg.SecurityGroupsTestCase, base.BaseTestCase): self._driver.gce_project, gce_mock.fake_operation()) + @mock.patch(neutron_get_plugin) + def test_validate_sg_rule(self, mock_plugin): + mock_plugin.side_effect = FakeNeutronManager + sg_rule = self.get_fake_sg_rule() + self.assertIsNone( + self._driver._validate_secgrp_rule(sg_rule)) + @mock.patch(neutron_get_plugin) @mock.patch('neutron.common.gceutils.wait_for_operation') @mock.patch('neutron.common.gceutils.update_firewall_rule') @@ -179,3 +188,22 @@ class GCENeutronTestCase(test_sg.SecurityGroupsTestCase, base.BaseTestCase): mock_delete.assert_called_once_with(self._driver.gce_svc, self._driver.gce_project, "secgrp-" + sg_rule['id']) + + @mock.patch('neutron.common.gceutils.wait_for_operation') + @mock.patch('neutron.common.gceutils.delete_firewall_rule') + def test_delete_sg_rule_with_error(self, mock_delete, mock_wait): + http_error = gce_errors.HttpError( + httplib2.Response({ + 'status': 404, + 'reason': 'Not Found' + }), + content='') + mock_delete.side_effect = http_error + mock_wait.side_effect = gce_mock.wait_for_operation + sg_rule = self.get_fake_sg_rule() + self.assertIsNone( + self._driver._delete_secgrp_rule(self.context, sg_rule['id'])) + mock_delete.assert_called_once_with(self._driver.gce_svc, + self._driver.gce_project, + "secgrp-" + sg_rule['id']) + mock_wait.assert_not_called()